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

add Best Practice check for vkAllocateDescriptorSets returning VK_SUCCESS when it should've VK_OUT_OF_POOL_MEMORY #8759

Open
AlexRouSg opened this issue Oct 25, 2024 · 2 comments
Labels
BestPractices Best practices layer

Comments

@AlexRouSg
Copy link

Too often do people going between GPU vendors, especially the newer coders, get tripped up by vkAllocateDescriptorSets returning VK_SUCCESS even tho the pool sizes was too small to allocate the set.

I would like to suggest this get a Best Practice check tho none of the current options seem too suited, maybe a new "PC Portability Best Practice"? This check should effectively be a ref counter incrementing on vkAllocateDescriptorSets and decrementing on vkFreeDescriptorSets or vkResetDescriptorPool, and then if vkAllocateDescriptorSets succeeds but the ref counter overflows the sizes on creation does it emit a warning.

@AlexRouSg AlexRouSg added the Incomplete Missing Validation VUs to be added label Oct 25, 2024
@spencer-lunarg spencer-lunarg added BestPractices Best practices layer and removed Incomplete Missing Validation VUs to be added labels Oct 26, 2024
@spencer-lunarg
Copy link
Contributor

spencer-lunarg commented Oct 26, 2024

I am all for this in Best Practices, but the issue is how to decide when to report it

ref counter overflows the sizes

what is the overflow limit?

There is a BestPractices-Arm-vkAllocateDescriptorSets-suboptimal-reuse check that might be similar

@AlexRouSg
Copy link
Author

AlexRouSg commented Oct 26, 2024

So the general idea I had is hook on to vkCreateDescriptorPool and have like a class containing

class DescriptorPoolRefCounter {
class Counter {
    int limit;
    int counter;
}
int maxSets;
std::unordered_map<VkDescriptorType, Counter> descriptorTypes;
}

amd then

vkCreateDescriptorPool(... VkDescriptorPoolCreateInfo info) {
    DescriptorPoolRefCounter counter;
    counter.maxSets = info.maxSets
    for (int i = 0; i < info.poolSizeCount; i++) {
        counter.descriptorTypes[info.pPoolSizes[i].type] = DescriptorPoolRefCounter::Counter(info.pPoolSizes[i].descriptorCount, 0);
    }
}

Then every call to vkAllocateDescriptorSets we lookup what are the descriptor counts in the set layout and increment the counters for the respective VkDescriptorType, call the actual vkAllocateDescriptorSets, if it returns VK_SUCCESS we loop over all the counters and check counter > limit if true then emit the warning

EDIT:
I forgot about

If multiple VkDescriptorPoolSize structures containing the same descriptor type appear in the pPoolSizes array then the pool will be created with enough storage for the total number of descriptors of each type.

So the for loop in vkCreateDescriptorPool needs to do an addition rather than an assignment

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

No branches or pull requests

2 participants