The VkColorCube app doesn't log failure status#77
The VkColorCube app doesn't log failure status#77jcortell68 wants to merge 2 commits intoGPUOpen-Tools:masterfrom
Conversation
The failure status code being returned via the GPA calls aren't being logged by the VkColorCube app. It does log that what it's trying to do failed, but not why it failed (according to the GPA implementation). In my case a C++ exception was being thrown and that fact was just not surfaced at all. Also changed the textual string for kGpaStatusErrorException to mention it's specifically a C++ exception, as otherwise the word "exception" is generic/vague and could be interepreted as such.
PLohrmannAMD
left a comment
PLohrmannAMD
left a comment
There was a problem hiding this comment.
I do agree that the actual error status code should be propagated up, but there are other changes here that should not be included.
| 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); \ |
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
These are not AMD HW generations.
| GPA_LOG_ERROR("jjjjjjjjjjjjj GpaBeginSession"); | ||
|
|
There was a problem hiding this comment.
Assuming this is debugging code, please remove.
| /// 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; | ||
|
|
There was a problem hiding this comment.
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.
| if (kGpaStatusOk != status) | ||
| { | ||
| AMDVulkanDemoVkUtils::Log("ERROR: GpaGetFuncTable failed with status %d", status); | ||
| delete gpa_function_table; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
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.
Sorry Peter. I copied the files from my internal GPA repo to the external/github one and some unrelated, earlier internal changes bled in. I shold have checked the changed before doing the pull request. Cleaning up now... |
|
Mess fixed up. Again, sorry about that. |
2 participants
The failure status code being returned via the GPA calls aren't being logged by the VkColorCube app. It does log that what it's trying to do failed, but not why it failed (according to the GPA implementation). In my case a C++ exception was being thrown and that fact was just not surfaced at all.
Also changed the textual string for kGpaStatusErrorException to mention it's specifically a C++ exception, as otherwise the word "exception" is generic/vague and could be interepreted as such.