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

Dynamic offset validation error when buffer range is VK_WHOLE_SIZE #2846

Closed
llyzs opened this issue May 31, 2021 · 7 comments
Closed

Dynamic offset validation error when buffer range is VK_WHOLE_SIZE #2846

llyzs opened this issue May 31, 2021 · 7 comments

Comments

@llyzs
Copy link

llyzs commented May 31, 2021

When buffer range is VK_WHOLE_SIZE and dynamic offset is larger than zero, the validation layer issues the following error:

Validation Error: [ VUID-vkCmdBindDescriptorSets-pDescriptorSets-01979 ] Object 0: handle = 0x7ffe368a1ea8, type = VK_OBJECT_TYPE_COMMAND_BUFFER; Object 1: handle = 0x2e237000001b59c, type = VK_OBJECT_TYPE_DESCRIPTOR_SET; Object 2: handle = 0x1285ab0000012cb2, type = VK_OBJECT_TYPE_BUFFER; | MessageID = 0xa159a763 | vkCmdBindDescriptorSets(): pDynamicOffsets[0] is 0x5700, but must be zero since the buffer descriptor's range is VK_WHOLE_SIZE in descriptorSet #0 binding #0 descriptor[0]. The Vulkan spec states: For each dynamic uniform or storage buffer binding in pDescriptorSets, the sum of the effective offset, as defined above, and the range of the binding must be less than or equal to the size of the buffer (https://vulkan.lunarg.com/doc/view/1.2.176.1/linux/1.2-extensions/vkspec.html#VUID-vkCmdBindDescriptorSets-pDescriptorSets-01979)

However according to the spec "range is the size in bytes that is used for this descriptor update, or VK_WHOLE_SIZE to use the range from offset to the end of the buffer.". Validation should to check the range if its value is VK_WHOLE_SIZE because its meaning is not a fixed size, but a dynamic size from the offset to the end of the buffer.

If we are forced to use a specific range number other than VK_WHOLE_SIZE to bypass the error, we will need to write different descriptor for each different dynamic offset, which makes the whole point of dynamic offset useless.

If my understanding is correct, the spec should also clarify that VUID-vkCmdBindDescriptorSets-pDescriptorSets-01979 only apply if range is not VK_WHOLE_SIZE.

@ncesario-lunarg
Copy link
Contributor

Thanks for the issue @llyzs!

It seems like the effective offset could still go past the end of the buffer, so I'm not sure 01979 can/should be completely ignored when range == VK_WHOLE_SIZE. Looks like maybe

if ((bound_range == VK_WHOLE_SIZE) && (offset > 0)) {
should be removed and we instead test against an "effective range" here. @sfricke-samsung any thoughts?

ncesario-lunarg added a commit to ncesario-lunarg/Vulkan-ValidationLayers that referenced this issue May 31, 2021
Fix offset check when VK_WHOLE_SIZE is used.

Closes KhronosGroup#2846.

Change-Id: I9c1c1104f808922b1a6e4168622a913673bde075
ncesario-lunarg added a commit to ncesario-lunarg/Vulkan-ValidationLayers that referenced this issue May 31, 2021
Fix offset check when VK_WHOLE_SIZE is used.

Closes KhronosGroup#2846.

Change-Id: I9c1c1104f808922b1a6e4168622a913673bde075
@sfricke-samsung
Copy link
Contributor

So if we have a 64 byte buffer and we set

VkBuffer buffer; // 64 bytes
VkDescriptorBufferInfo info_a = {buffer, 8, VK_WHOLE_SIZE};
VkDescriptorBufferInfo info_b = {buffer, 8, 32};

With no dynamic offset,

  • info_a is using buffer[8:64]
  • info_b is using buffer[8:40]

if my pDynamicOffsets is 16, to my knowledge it is shifting the offset to

  • info_a is using buffer[24:80] (over range)
  • info_b is using buffer[24:56]

I guess the

the sum of the effective offset, as defined above

I read the spec saying

For VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC and VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC descriptor types, offset is the base offset from which the dynamic offset is applied and range is the static size used for all dynamic offsets.

that VK_WHOLE_SIZE is does not act as a dynamic size here

@llyzs
Copy link
Author

llyzs commented Jun 1, 2021

@sfricke-samsung

My opinion is that the spec regarding "range is the static size used for all dynamic offsets" statement is somewhat "ambiguous". The "range" value here can be treated as an "effective range" before dynamic offset is applied as you wrote, or can be treated as whatever value it contains. In your example above, one could argue that when pDynamicOffsets is 16, the actual descriptor can be shifted like this:

  • info_a is {buffer, 8 + 16, VK_WHOLE_SIZE}
  • info_b is {buffer, 8 + 16, 32}

I think the spec needs to be more specific when the range value is VK_WHOLE_SIZE here.

@llyzs
Copy link
Author

llyzs commented Jun 1, 2021

Our use case is that we create a single big 64k UBO and a single descriptor with range = VK_WHOLE_SIZE, which contains dozens of chunks of data of different sizes. When binding the UBO we use dynamic offsets with this single UBO descriptor. It worked without any issue until this validation error pops up recently (although rendering is never an issue).

If we are going to follow the "effective range before dynamic offset applied" interpretation of VK_WHOLE_SIZE, we will need to create multiple descriptors of different sizes in order to avoid the error, and performance impact could also be a concern.

@Tobski
Copy link
Contributor

Tobski commented Jun 1, 2021

Hi @llyzs - unfortunately @sfricke-samsung is interpreting the spec correctly. As far as I understand it, the end point of a buffer's range is baked into the descriptor on most GPUs - so baking multiple descriptors actually is the only option. If you're not using robust buffer access and you're staying within the actual buffer range it is likely fine most of the time. But it's one of those things that will likely blow up in unexpected ways at the worst possible moment (e.g. on an end user device).

If you're targeting platforms supporting VK_KHR/EXT_buffer_device_address you might want to consider using device addresses via push constants as a way to push uniform data dynamically. That has no "end" to the range so you can just update the pointer on every draw with the new offset.

@Tobski
Copy link
Contributor

Tobski commented Jun 1, 2021

Oh also, FWIW the spec is nowhere near as clear here as it should be so we should definitely get the spec text updated..

@llyzs
Copy link
Author

llyzs commented Jun 1, 2021

@Tobski Thanks for the clarification and the spec update proposal. We will update our application accordingly. Closing this issue.

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

No branches or pull requests

4 participants