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.

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

Fix issues found on linux with Vulkan GPU timers #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Alloyed
Copy link

@Alloyed Alloyed commented Feb 24, 2019

More details in code review comments. These changes were necessary to get microprofile working using GCC on linux, with Vulkan timers enabled.

@@ -8325,7 +8325,7 @@ void MicroProfileGpuFetchRange(VkCommandBuffer CommandBuffer, uint32_t nNode, ui
if (nCount <= 0)
return;

vkGetQueryPoolResults(S.pGPU->Devices[nNode], S.pGPU->QueryPool[nNode], nBegin, nCount, 8*nCount, &S.pGPU->nResults[nBegin], 8, VK_QUERY_RESULT_64_BIT|VK_QUERY_RESULT_PARTIAL_BIT );
vkGetQueryPoolResults(S.pGPU->Devices[nNode], S.pGPU->QueryPool[nNode], nBegin, nCount, 8*nCount, &S.pGPU->nResults[nBegin], 8, VK_QUERY_RESULT_64_BIT);
Copy link
Author

@Alloyed Alloyed Feb 24, 2019

Choose a reason for hiding this comment

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

https://www.khronos.org/registry/vulkan/specs/1.1-extensions/man/html/vkGetQueryPoolResults.html

VK_QUERY_RESULT_PARTIAL_BIT must not be used if the pool’s queryType is VK_QUERY_TYPE_TIMESTAMP.

maybe VK_QUERY_RESULT_WITH_AVAILABILITY_BIT is the intended behavior here? Without it we skip not-ready values, with it we write in 0 for not-ready values: I don't know enough about the surrounding code to say either way.

@@ -8540,6 +8540,18 @@ void MicroProfileGpuInitVulkan(VkDevice* pDevices, VkPhysicalDevice* pPhysicalDe

void MicroProfileGpuShutdown()
{
VkAllocationCallbacks* noAlloc = nullptr;
Copy link
Author

Choose a reason for hiding this comment

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

Implementing cleanup. without this my particular laptop will crash on shutdown

Copy link
Owner

Choose a reason for hiding this comment

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

looks good

@@ -12623,7 +12635,7 @@ bool MicroProfileHashTableSet(MicroProfileHashTable* pTable, uint64_t Key, uintp
while(1)
{
const uint32_t nLim = pTable->nLim;
uint32_t B = H % pTable->nAllocated;
uint32_t B = static_cast<uint32_t>(H) % pTable->nAllocated;
Copy link
Author

Choose a reason for hiding this comment

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

Triggers SIGFPE, because H is a uint64 and will overflow to get it to fit within the modulo operation. The cast makes the existing behavior explicit/not-error-y, but I have not checked to see what that means for the hash table generally

Copy link
Owner

Choose a reason for hiding this comment

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

-Can you move this to a seperate
-use C style casts.
-I count 3 instances of the bug, it seems you only have two of them. Can you add the last one. (one in each of Get/Set/Remove)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants