KhronosGroup / Vulkan-Docs

The Vulkan API Specification and related tools
Other
2.76k stars 463 forks source link

VkDescriptorSetVariableDescriptorCountAllocateInfo::pDescriptorCounts Documentation clarification/improvement #1909

Open ch45er opened 2 years ago

ch45er commented 2 years ago

The spec (version 1.3.223) says:

If descriptorSetCount is zero or this structure is not included in the pNext chain, then the variable lengths are considered to be zero. Otherwise, pDescriptorCounts[i] is the number of descriptors in the variable-sized descriptor binding in the corresponding descriptor set layout.

It isn't particularly clear whether pDescriptorCounts[i] being zero is legit or not. The spec doesn't seem to state what should happen in such case.

I've tried to allocate a descriptor set with some fixed descriptor bidings and a variable-sized binding, passing the corresponding pDescriptorCounts[i]=0 into vkAllocateDescriptorSets() on different GPUs. On NVIDIA (driver 516.59) this results in a (usable) descriptor being allocated. On AMD (driver 22.7.1) vkAllocateDescriptorSets() returns VK_ERROR_OUT_OF_POOL_MEMORY. Apparently, vendors handle this differently.

There seem to be no relevant paragraphs in the "Valid Usage" section of the spec, so the validation layer has nothing to say about this.

Tobski commented 2 years ago

There's no valid usage because this behavior is valid - what the spec says is that implementations guarantee at least the number you ask for can be allocated out of the pool; some implementations will return an error beyond that number, others will carry on allocating. This is all described in the specification of vkAllocateDescriptorSets.

Tobski commented 2 years ago

Worth noting - error conditions that return defined errors are not covered by valid usage, as the behavior is defined for those conditions.

ch45er commented 2 years ago

When I've asked at least 0 descriptors from a valid pool of known capacity, an implementation gave me nothing with VK_ERROR_OUT_OF_POOL_MEMORY. When I've asked at least 1 descriptors from the same pool, this very implementation gave me a valid descriptor set.

The spec section you are referring to says:

If the allocation fails due to no more space in the descriptor pool, and not because of system or device memory exhaustion, then VK_ERROR_OUT_OF_POOLMEMORY must be returned. (And this is the only specified case when VK_ERROR_OUT_OF_POOL_MEMORY must be returned.)_

If a pool is capable of allocating 1 descriptor, then it is also techincally capable of allocating 0 descriptors. A different implementation behaved identically in both cases.

The issue here is not really about pool capacity, but "is zero-length allocation legit or not". This is the matter of API design (convention) and not the hardware design; and therefore, should not be implementation-specific. API users should not do unnecessary guesswork.

Tobski commented 2 years ago

Ok so you're asking for 0 descriptors inside a pool that is already valid - I thought you were asking about a pool of size zero. That's absolutely banned by this valid usage:

VUID-VkDescriptorSetAllocateInfo-descriptorSetCount-arraylength descriptorSetCount must be greater than 0

Tobski commented 2 years ago

No wait hang on, it's 0 descriptors you're asking about ok hmm, hang on.

Tobski commented 2 years ago

Ok so there's nothing banning a set layout with 0 descriptors, now I see what you're getting at. I think technically this is a driver bug, but it's a corner case I imagine we haven't tested. I'll raise an internal bug, see what the likelihood of rolling out a driver fix is for this and if it's possible to roll fixes out everywhere we'll do that, otherwise we may ban it and add it back in with a maintenance extension.

ch45er commented 2 years ago

Ok so there's nothing banning a set layout with 0 descriptors, now I see what you're getting at.

Yes, exactly. Sorry if my explanation wasn't clear enough :|

I see why this is a corner case, no questions. Still, this is actually pretty easy to encounter: imagine a bindless renderer, that has something like this somewhere in shaders:

#extension GL_EXT_nonuniform_qualifier : enable

layout (set = 0, binding = /*first*/) ... // fixed stuff
layout (set = 0, binding = /*last*/) uniform sampler2D allTextures[];

It is possible that at certain time there aren't any textured objects to render, so no textures may be bound to the pipeline.

To support such shaders, one would require:

//VkPhysicalDeviceDescriptorIndexingFeatures indexingFeatures;
indexingFeatures.descriptorBindingPartiallyBound = VK_TRUE;
indexingFeatures.runtimeDescriptorArray = VK_TRUE;

This effectively lifts off many restrictions to descriptor set layout and allows passing actual length of allTextures via VkDescriptorSetVariableDescriptorCountAllocateInfo::pDescriptorCounts. One basically looses validation of allTextures binding.

While being able to set pDescriptorCounts[0] = 0; for such descriptor set is convenient (users can avoid additional pipeline layout), banning this behavior would also make sense (what is 0-length array anyway?). Either option is understandable.

Would be nice to have a line or two about this in the spec at some point. Currently it is unobvious what behaviour is correct, which is frustrating.

Tobski commented 2 years ago

FWIW I've raised the question internally to verify if this was intentional, and proposed CTS tests if we're expecting this to work so that drivers can't bug out on it in future.

ch45er commented 2 years ago

Thanks for the update, lets wait for the resolution.