Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The VkColorCube app doesn't log failure status #77

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+33 −13
Open

The VkColorCube app doesn't log failure status #77

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
513ef99
The VkColorCube app doesn't log failure status
jcortell68 Mar 15, 2023
bd9f67d
Fixed some accidental changes in previous commit
jcortell68 Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 30 additions & 16 deletions source/examples/vulkan/vk_color_cube/vk_color_cube.cc
View file
Original file line number Diff line number Diff line change
@@ -372,15 +372,15 @@ bool AMDVulkanDemo::InitializeGpa()
gpu_perf_api_helper_.gpa_function_table_->GpaRegisterLoggingCallback(gpa_log_types, gpu_perf_api_helper_.gpaLoggingCallback);
if (status_register_callback != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to register GPA logging callback.");
LogStatus(status_register_callback, "ERROR: Failed to register GPA logging callback");
return false;
}

GpaStatus status_gpa_initialize = gpu_perf_api_helper_.gpa_function_table_->GpaInitialize(kGpaInitializeDefaultBit);

if (status_gpa_initialize != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to initialize GPA.");
LogStatus(status_gpa_initialize, "ERROR: Failed to initialize GPA");
return false;
}

@@ -421,7 +421,6 @@ bool AMDVulkanDemo::InitializeGpa()
if (kGpaStatusOk != status)
{
AMDVulkanDemoVkUtils::Log("ERROR: GpaGetFuncTable failed with status %d", status);
delete gpa_function_table;
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this gpa_function_table is supposed to be deleted? It gets allocated earlier in this function, so it should probably be deleted here to avoid the small memory leak.


@@ -440,7 +439,7 @@ bool AMDVulkanDemo::InitializeGpa()
status = gpu_perf_api_helper_.gpa_function_table_->GpaRegisterLoggingCallback(gpa_log_types, gpu_perf_api_helper_.gpaLoggingCallback);
if (status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to register GPA logging callback.");
LogStatus(status, "ERROR: Failed to register GPA logging callback");
return false;
}

@@ -864,7 +863,7 @@ bool AMDVulkanDemo::InitializeVulkan()

if (gpa_open_context_status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to open GPA context.");
LogStatus(gpa_open_context_status, "ERROR: Failed to open GPA context");
return false;
}

@@ -878,7 +877,7 @@ bool AMDVulkanDemo::InitializeVulkan()
GpaStatus get_sample_types_status = gpu_perf_api_helper_.gpa_function_table_->GpaGetSupportedSampleTypes(gpa_context_id_, &sample_types);
if (get_sample_types_status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to get supported GPA sample types.");
LogStatus(get_sample_types_status, "ERROR: Failed to get supported GPA sample types");
return false;
}

@@ -887,7 +886,7 @@ bool AMDVulkanDemo::InitializeVulkan()

if (gpa_create_session_status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to create GPA session.");
LogStatus(gpa_create_session_status, "ERROR: Failed to create GPA session");
return false;
}

@@ -914,6 +913,7 @@ bool AMDVulkanDemo::InitializeVulkan()
if (gpa_status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("Failed to enable counter: %s", it->c_str());
LogStatus(gpa_status);
}
success_enable_counter &= gpa_status == kGpaStatusOk;
}
@@ -941,7 +941,7 @@ bool AMDVulkanDemo::InitializeVulkan()

if (gpa_enable_all_counters_status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to enable all GPA counters.");
LogStatus(gpa_enable_all_counters_status, "ERROR: Failed to enable all GPA counters");
return false;
}

@@ -950,7 +950,7 @@ bool AMDVulkanDemo::InitializeVulkan()
GpaStatus gpa_get_pass_count_status = gpu_perf_api_helper_.gpa_function_table_->GpaGetPassCount(gpa_session_id_, &required_pass_count_);
if (gpa_get_pass_count_status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to get the number of required GPA passes.");
LogStatus(gpa_get_pass_count_status, "ERROR: Failed to get the number of required GPA passes");
return false;
}

@@ -965,7 +965,7 @@ bool AMDVulkanDemo::InitializeVulkan()

if (gpa_begin_session_status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to begin GPA session.");
LogStatus(gpa_begin_session_status, "ERROR: Failed to begin GPA session");
return false;
}
}
@@ -1654,7 +1654,7 @@ bool AMDVulkanDemo::InitializeVulkan()

if (gpa_end_session_status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to end GPA session.");
LogStatus(gpa_end_session_status, "ERROR: Failed to end GPA session");
}
}

@@ -1814,9 +1814,10 @@ void AMDVulkanDemo::DrawScene()
// This example only renders one set of profiles (aka, only the number of passes needed to generate one set of results).
unsigned int profile_set = 0;

gpu_perf_api_helper_.PrintGpaSampleResults(gpa_context_id_, gpa_session_id_, profile_set, AMDVulkanDemo::kGpaSampleIdCube, print_debug_output_, Verify(), ConfirmSuccess());
gpu_perf_api_helper_.PrintGpaSampleResults(gpa_context_id_, gpa_session_id_, profile_set, AMDVulkanDemo::kGpaSampleIdWireframe, print_debug_output_, Verify(), ConfirmSuccess());
gpu_perf_api_helper_.PrintGpaSampleResults(gpa_context_id_, gpa_session_id_, profile_set, AMDVulkanDemo::kGpaSampleIdCubeAndWireframe, print_debug_output_, Verify(), ConfirmSuccess());
// NOTE: we can't loop over these because it is not guaranteed that the sample_ids will be 0-based and monotonically increasing.
gpu_perf_api_helper_.PrintGpaSampleResults(gpa_context_id_, gpa_session_id_, profile_set, 0, print_debug_output_, Verify(), ConfirmSuccess());
gpu_perf_api_helper_.PrintGpaSampleResults(gpa_context_id_, gpa_session_id_, profile_set, 1, print_debug_output_, Verify(), ConfirmSuccess());
gpu_perf_api_helper_.PrintGpaSampleResults(gpa_context_id_, gpa_session_id_, profile_set, 2, print_debug_output_, Verify(), ConfirmSuccess());

// Close the CSV file so that it actually gets saved out.
gpu_perf_api_helper_.CloseCSVFile();
@@ -1842,7 +1843,7 @@ void AMDVulkanDemo::Destroy()

if (gpa_delete_session_status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to delete GPA session.");
LogStatus(gpa_delete_session_status, "ERROR: Failed to delete GPA session");
}
}

@@ -1852,7 +1853,7 @@ void AMDVulkanDemo::Destroy()

if (gpa_close_context_status != kGpaStatusOk)
{
AMDVulkanDemoVkUtils::Log("ERROR: Failed to close GPA Context.");
LogStatus(gpa_close_context_status, "ERROR: Failed to close GPA Context");
}
}
}
@@ -2358,6 +2359,19 @@ void AMDVulkanDemo::PreBuildCommandBuffers(PrebuiltPerFrameResources* prebuilt_r
}
}

void AMDVulkanDemo::LogStatus(GpaStatus status, const char* msg)
{
auto status_as_str = gpu_perf_api_helper_.gpa_function_table_->GpaGetStatusAsStr(status);
if (msg != nullptr)
{
AMDVulkanDemoVkUtils::Log("%s. %s", msg, status_as_str);
}
else
{
AMDVulkanDemoVkUtils::Log("%s", status_as_str);
}
}

#if defined(VK_USE_PLATFORM_WIN32_KHR)

/// Window message processing callback.
19 changes: 9 additions & 10 deletions source/examples/vulkan/vk_color_cube/vk_color_cube.h
View file
Original file line number Diff line number Diff line change
@@ -142,13 +142,6 @@ class AMDVulkanDemo : public gpa_example::GpaSampleApp
/// Default window height.
const uint32_t kDefaultWindowHeight = 300;

/// Note that GPA sample IDs are client defined. However, the Vulkan GPA
/// extension assigns an ID to each sample (they are not client defined).
/// The GPA library manages the mapping between them. These are the former.
static constexpr GpaUInt32 kGpaSampleIdCube = 0;
static constexpr GpaUInt32 kGpaSampleIdWireframe = 1;
static constexpr GpaUInt32 kGpaSampleIdCubeAndWireframe = 2;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these being removed?

To clarify - these should be defined by the user / application. They do not need to be 0, 1, 2, but could be 4, 5, 12 (or even assigned based on the pointer to the unique object being drawn and profiled). As long as they are unique, the actual value does not matter. We prefer to keep these as user-defined variables for clarity and to show their connection between different GPA calls rather than have them as "magic numbers" that show up throughout the code.

#ifdef ANDROID
inline void SetWindow(ANativeWindow* native_window)
{
@@ -249,7 +242,7 @@ class AMDVulkanDemo : public gpa_example::GpaSampleApp

/// Sample Id that the application (not GPA) assigns to the cube.
/// The cube will have this same sample_id in all passes.
const GpaUInt32 gpa_sample_id = kGpaSampleIdCube;
const GpaUInt32 gpa_sample_id = 0;
} cube_;

/// @brief Container for objects related to drawing the wireframe.
@@ -263,7 +256,7 @@ class AMDVulkanDemo : public gpa_example::GpaSampleApp

/// Sample Id that the application (not GPA) assigns to the wireframe.
/// The wireframe will have this same sample_id in all passes.
const GpaUInt32 gpa_sample_id = kGpaSampleIdWireframe;
const GpaUInt32 gpa_sample_id = 1;
} wire_frame_;

/// @brief Container for objects related to drawing the cube and wireframe.
@@ -286,7 +279,7 @@ class AMDVulkanDemo : public gpa_example::GpaSampleApp

/// Sample Id that the application (not GPA) assigns to the cube wireframe.
/// The combined cube + wireframe sample will have this same sample_id in all passes.
const GpaUInt32 gpa_sample_id = kGpaSampleIdCubeAndWireframe;
const GpaUInt32 gpa_sample_id = 2;
} cube_and_wire_frame_;
};

@@ -394,6 +387,12 @@ class AMDVulkanDemo : public gpa_example::GpaSampleApp
/// @param [in] gpa_pass_index If GPA is enabled for these command buffers, this indicates which profile pass is being built; ignored if enable_gpa is false.
void PreBuildCommandBuffers(PrebuiltPerFrameResources* prebuilt_resources, VkFramebuffer frame_buffer, bool enable_gpa, uint32_t gpa_pass_index);

/// @brief Log the textual representation of a failure status code
///
/// @param [in] status the failure code
/// @param [in] msg optional additional context. Should not contain trailing punctuation.
void LogStatus(GpaStatus status, const char* msg=nullptr);

/// GPA helper.
GpaHelper gpu_perf_api_helper_;

15 changes: 13 additions & 2 deletions source/gpu_perf_api_common/gpu_perf_api.cc
View file
Original file line number Diff line number Diff line change
@@ -47,7 +47,7 @@ extern IGpaImplementor* gpa_imp; ///< GPA implementor instance.
} \
if (index >= num_counters) \
{ \
GPA_LOG_ERROR("Parameter %s is %d but must be less than %d.", #index, index, num_counters); \
GPA_LOG_ERROR("Parameter index is %d but must be less than %d.", index, num_counters); \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This inside a macro, so the text "index" could change depending on what is passed into the macro; therefore the original version of this code should stay.

return kGpaStatusErrorIndexOutOfRange; \
}

@@ -493,6 +493,15 @@ GPA_LIB_DECL GpaStatus GpaGetDeviceGeneration(GpaContextId gpa_context_id, GpaHw
case GDT_HW_GENERATION_GFX11:
*hardware_generation = kGpaHwGenerationGfx11;
break;
case GDT_HW_GENERATION_GFX104:
*hardware_generation = kGpaHwGenerationGfx104;
break;
case GDT_HW_GENERATION_GFX401:
*hardware_generation = kGpaHwGenerationGfx401;
break;
case GDT_HW_GENERATION_GFX402:
*hardware_generation = kGpaHwGenerationGfx402;
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not AMD HW generations.

case GDT_HW_GENERATION_LAST:
*hardware_generation = kGpaHwGenerationLast;
break;
@@ -847,6 +856,8 @@ GPA_LIB_DECL GpaStatus GpaDeleteSession(GpaSessionId gpa_session_id)

GPA_LIB_DECL GpaStatus GpaBeginSession(GpaSessionId gpa_session_id)
{
GPA_LOG_ERROR("jjjjjjjjjjjjj GpaBeginSession");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this is debugging code, please remove.

try
{
PROFILE_FUNCTION(GpaBeginSession);
@@ -1657,7 +1668,7 @@ static const char* kErrorString[] = {
GPA_ENUM_STRING_VAL(kGpaStatusErrorTimeout, "GPA Error: Attempt to Retrieve Data Failed due to Timeout."),
GPA_ENUM_STRING_VAL(kGpaStatusErrorLibAlreadyLoaded, "GPA Error: Library Is Already Loaded."),
GPA_ENUM_STRING_VAL(kGpaStatusErrorOtherSessionActive, "GPA Error: Other Session Is Active."),
GPA_ENUM_STRING_VAL(kGpaStatusErrorException, "GPA Error: Exception Occurred.")};
GPA_ENUM_STRING_VAL(kGpaStatusErrorException, "GPA Error: C++ Exception Occurred.")};

/// Size of kErrorString array.
static size_t kErrorStringSize = sizeof(kErrorString) / sizeof(const char*);