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

WIP: MVKDescriptor: Prevent batched write combined image-sampler from overwriting each other in argument buffer #2427

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dboyan
Copy link
Contributor

@dboyan dboyan commented Jan 24, 2025

Currently samplerXXArray is translated to one textureXX_array and a single sampler in MSL, consuming 2 bindings in Metal regardless of the array size. This is already not an accurate translation. In the meantime, the descriptor set update does not consider the difference in numbers between the approaches. Despite the inaccuracy, the current solution "works" in some cases when not using argument buffers. They are broken when argument buffer is enabled.

The underlying reason is that image (texture) and sampler are in the same index space when used in argument buffers, and when updated in a batch (as sampler array) using combined image-sampler descriptor type, the sampler is overwritten by the next texture. This change avoids this kind of overwriting.

The ultimate solution would be properly implement texture/sampler arrays and account for the semantic differences between Vulkan and Metal in descriptor sets. But let's make it less broken first as the appropriate solution would require considerable effort.

Not a full fix, but addresses the symptom of #2403 .

…writing each other in argument buffer

Currently samplerXXArray is translated to one textureXX_array and a single
sampler in MSL, consuming 2 bindings in Metal regardless of the array size.
This is already not an accurate translation. In the meantime, the descriptor
set update does not consider the difference in numbers between the approaches.
Despite the inaccuracy, the current solution "works" in some cases when not using
argument buffers. They are broken when argument buffer is enabled.

The underlying reason is that image (texture) and sampler are in the same index
space when used in argument buffers, and when updated in a batch (as sampler array)
using combined image-sampler descriptor type, the sampler is overwritten by the next
texture. This change avoids this kind of overwriting.

The ultimate solution would be properly implement texture/sampler arrays and account
for the semantic differences between Vulkan and Metal in descriptor sets. But let's
make it less broken first as the appropriate solution would require considerable effort.
@dboyan dboyan marked this pull request as draft January 24, 2025 17:56
@dboyan
Copy link
Contributor Author

dboyan commented Jan 24, 2025

Now I think what I said above is not fully correct. The issue I referenced might also have the incorrect application itself as a contributing factor. However, I think we still need some sort of fix because the current argument buffer logic makes the behavior too broken.

@billhollings billhollings changed the title MVKDescriptor: Prevent batched write combined image-sampler from overwriting each other in argument buffer WIP: MVKDescriptor: Prevent batched write combined image-sampler from overwriting each other in argument buffer Feb 7, 2025
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.

1 participant