SecondHalfGames / yakui

yakui is a declarative Rust UI library for games
Apache License 2.0
222 stars 18 forks source link

Fix validation warnings on macOS #165

Closed kanerogers closed 1 month ago

kanerogers commented 1 month ago

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

kanerogers commented 1 month ago

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.

Ralith commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

[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 commented 1 month ago

Aw. Well, probably no harm in the overkill.