GPUOpen-Drivers / AMDVLK

AMD Open Source Driver For Vulkan
MIT License
1.69k stars 160 forks source link

broken vkCmdBindVertexBuffers2 pSizes=NULL handling #321

Closed zmike closed 1 week ago

zmike commented 1 year ago

When NULL is passed for the pSizes array, the driver is supposed to calculate the sizes of the passed buffers. AMD seems to have specific alignment rules for the buffers such that the size of the buffer must be aligned to the stride (i.e., your padVertexBuffers workaround).

When NULL is passed, you are calculating the buffer size incorrectly as pBuffer->GetSize() - offset and then checking for the padVertexBuffers workaround afterward. The code should instead calculate the size applying this workaround unconditionally to avoid calculating a size that is broken.

See also https://gitlab.freedesktop.org/mesa/mesa/-/issues/8812

I've filed VKCTS coverage for this, so your driver will fail to pass those tests in the future if this issue is not resolved.

zmike commented 1 year ago

Hm okay, disregard, this is yet another case where I thought VUs had been added and they are still in progress.

zmike commented 1 year ago

Nope, I was sort of right the first time. The problem is the handling of robustBufferAccess2:

If robustBufferAccess2 is enabled, vertex input attributes are considered out of bounds if the
offset of the attribute in the bound vertex buffer range plus the size of the attribute is
greater than the byte size of the memory range bound to the vertex buffer binding.
▪ If a vertex input attribute is out of bounds, the raw data extracted are zero values, and
missing G, B, or A components are filled with (0,0,1).

Meanwhile, AMDVLK is universally using the behavior for when this feature is disabled, which invalidates the attribute.

The solution here is to always enable padVertexBuffers if robustBufferAccess2 is enabled.

binluoup commented 1 year ago

Thanks, I will check : )

jinjianrong commented 1 month ago

There is CTS failure if padVertexBuffers is always enable when robustBufferAccess2 is enabled. Currently only enable padVertexBuffers for Zink.

jinjianrong commented 1 week ago

Please check the fix with 2024.Q2.3 release