Skip to content

The VkColorCube app doesn't log failure status #77

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

The VkColorCube app doesn't log failure status #77

Show file tree
Hide file tree
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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.


Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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");
}
}

Expand Down Expand Up @@ -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();
Expand All @@ -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");
}
}

Expand All @@ -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");
}
}
}
Expand Down Expand Up @@ -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.
Expand Down
19 changes: 9 additions & 10 deletions source/examples/vulkan/vk_color_cube/vk_color_cube.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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_;
};

Expand Down Expand Up @@ -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_;

Expand Down
15 changes: 13 additions & 2 deletions source/gpu_perf_api_common/gpu_perf_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; \
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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*);
Expand Down