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

Fix validation warnings on macOS #165

Merged
merged 1 commit into from
May 19, 2024

Conversation

kanerogers
Copy link
Collaborator

@kanerogers kanerogers commented May 18, 2024

Sets various UPDATE_AFTER_BIND_POOL_BIT flags on descriptors to allow reasonable amounts of image samplers in our descriptor sets on platforms like macOS.

Closes #160

This commit sets various `UPDATE_AFTER_BIND_POOL_BIT` flags on descriptors
to allow reasonable amounts of image samplers in our descriptor sets.

Closes #160
@kanerogers kanerogers requested a review from Ralith as a code owner May 18, 2024 08:30
@kanerogers kanerogers self-assigned this May 18, 2024
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely this isn't macOS specific? It looks like we are, in fact, updating this descriptor set while commands may be in flight, so this is just necessary for correctness.

@kanerogers kanerogers merged commit 7346eb7 into main May 19, 2024
2 checks passed
@kanerogers
Copy link
Collaborator Author

Surely this isn't macOS specific?

In theory no, but in practice macOS is the only platform that has an absurd difference in the number of descriptors available with and without using VK_DESCRIPTOR_SET_LAYOUT_CREATE_UPDATE_AFTER_BIND_POOL_BIT (<100 vs essentially unlimited). I believe this is mostly a quirk of Metal/MoltenVK; Metal has very tight limits on the number of resources that can be passed to a shader (hence the low amount of descriptors), but this can be worked around through the use of argument buffers.

In MoltenVK, by default argument buffers are only used when VK_DESCRIPTOR_SET_LAYOUT_CREATE_UPDATE_AFTER_BIND_POOL_BIT is set, so the physical limits advertised (maxDescriptorSetSampledImages VS maxDescriptorSetUpdateAfterBindSampledImages) reflect this.

It all works fine regardless, but we just want to stay validation clean.

It looks like we are, in fact, updating this descriptor set while commands may be in flight, so this is just necessary for correctness.

I don't think this is the case. This is a bit pedantic/spec-lawyery, but even though we do update the descriptor set while commands are in flight,VK_DESCRIPTOR_BINDING_UPDATE_AFTER_BIND_BIT has a very narrow definition:

VK_DESCRIPTOR_BINDING_UPDATE_AFTER_BIND_BIT indicates that if descriptors in this binding are updated between when the descriptor set is bound in a command buffer and when that command buffer is submitted to a queue, then the submission will use the most recently set descriptors for this binding and the updates do not invalidate the command buffer.

Since we bind the descriptor set on line 262 of lib.rs, and don't make any further updates before we issue our cmd_draw_indexed on line 325, I don't think we actually make any use of VK_DESCRIPTOR_BINDING_UPDATE_AFTER_BIND_BIT except for the additional descriptors it gives us.

@kanerogers kanerogers deleted the 160_fix_validation_warnings_on_macos branch May 19, 2024 06:09
@Ralith
Copy link
Collaborator

Ralith commented May 19, 2024

and the updates do not invalidate the command buffer.

This is the critical part to my reading. If updates invalidate the command buffer without this flag, then we need it. There should be more explicit wording somewhere in the descriptor set update spec, though.

@kanerogers
Copy link
Collaborator Author

Yeah, I can't find anything more specific either. And what's more we don't get any validation warnings about updating descriptor sets after bind if we don't have this flag enabled - I know validation clean != spec compliant, but it seems like we could reasonably expect to see warnings if we were doing that.

@Ralith
Copy link
Collaborator

Ralith commented May 19, 2024

Here's the relevant text, from Descriptor Set Updates:

If the dstSet member of any element of pDescriptorWrites or pDescriptorCopies is bound, accessed, or modified by any command that was recorded to a command buffer which is currently in the recording or executable state, and any of the descriptor bindings that are updated were not created with the VK_DESCRIPTOR_BINDING_UPDATE_AFTER_BIND_BIT or VK_DESCRIPTOR_BINDING_UPDATE_UNUSED_WHILE_PENDING_BIT bits set, that command buffer becomes invalid.

See also VUID-vkUpdateDescriptorSets-None-03047:

The dstSet member of each element of pDescriptorWrites or pDescriptorCopies for bindings which were created without the VK_DESCRIPTOR_BINDING_UPDATE_AFTER_BIND_BIT or VK_DESCRIPTOR_BINDING_UPDATE_UNUSED_WHILE_PENDING_BIT bits set must not be used by any command that was recorded to a command buffer which is in the pending state

I think VK_DESCRIPTOR_BINDING_UPDATE_UNUSED_WHILE_PENDING_BIT might actually be a more precise description of the behavior we actually want here. Is that also sufficient to get the relaxed limits on macOS?

@kanerogers
Copy link
Collaborator Author

[relevant text..]

Okay, interesting - that makes more sense. And I suppose the reason we weren't seeing validation warnings is just because we hadn't tripped that specific case yet, even though it was perfectly possible.

I think VK_DESCRIPTOR_BINDING_UPDATE_UNUSED_WHILE_PENDING_BIT might actually be a more precise description of the behavior we actually want here. Is that also sufficient to get the relaxed limits on macOS?

Unfortunately not; it seems kind of weird that VK_DESCRIPTOR_BINDING_UPDATE_AFTER_BIND_BIT is overloaded to mean "update after bind" and "get different limits of descriptors", but that the spec even calls this out specifically:

Descriptor set layouts created with this bit set have alternate limits for the maximum number of descriptors per-stage and per-pipeline layout. The non-UpdateAfterBind limits only count descriptors in sets created without this flag. The UpdateAfterBind limits count all descriptors, but the limits may be higher than the non-UpdateAfterBind limits.

@Ralith
Copy link
Collaborator

Ralith commented May 19, 2024

Aw. Well, probably no harm in the overkill.

Uriopass pushed a commit to Uriopass/yakui that referenced this pull request Jun 30, 2024
This commit sets various `UPDATE_AFTER_BIND_POOL_BIT` flags on descriptors
to allow reasonable amounts of image samplers in our descriptor sets.

Closes SecondHalfGames#160
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.

vulkan: Fix validation warnings on macOS
2 participants