Rewrite descriptor set layouts to be more robust against application bugs #1672
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HansKristian-Work
force-pushed
the
descriptor-ub-exploration
branch
9 times, most recently
from
22c3b0b to
0493bc5
Compare
HansKristian-Work
changed the title
2 tasks
|
With radv main and amdvlk 2023Q3.1 on a 7900xtx I've tested a short section in The Witcher 3, Meet Your Maker, The Last of Us Part 1, Kena: Bridge of Spirits, HITMAN 3, Battlefield 1 and Age of Wonders 4. No issues noticed in regards to this PR. |
doitsujin
reviewed
Aug 31, 2023
With descriptor layout rewrite, we'll emit completely new shaders, so make sure we start a new Fossilize bucket. Signed-off-by: Hans-Kristian Arntzen <[email protected]>
We really need to be able to squeeze all metadata for a UAV into 16 bytes. This is necessary for upcoming change where we place SSBO at 32 byte offset instead of 16 byte. The use of goofy unions sharing a flags field is questionable, but you gotta do what you gotta do ... Signed-off-by: Hans-Kristian Arntzen <[email protected]>
This was changed after the fact in Vulkan. Signed-off-by: Hans-Kristian Arntzen <[email protected]>
For the upcoming refactor, we'll have four possible pipeline layouts:
- Split mutable (ideal):
First set: (texture, texel buffer)
Second set: (UBO, SSBO)
- May or may not have embedded mutable where typed and untyped live in
separate halves of a descriptor.
- Full mutable (ANV until descriptor buffers are supported):
- Cannot make this robust since we cannot emit sibling descriptors.
- Non-mutable:
- *shrug*
The bindless info lookup is also accelerated here since for mutable we
statically know the info index. For non-mutable, fallback to a lookup.
We could in theory add static indices for that as well, but that gets
messy when we have to consider SSBO being used or not ...
Signed-off-by: Hans-Kristian Arntzen <[email protected]>
Since we have to be more robust than before, UBO and storage images will emit extra NULL descriptors either before or after their payload. Signed-off-by: Hans-Kristian Arntzen <[email protected]>
We'll be packing UBO and SSBO together. Signed-off-by: Hans-Kristian Arntzen <[email protected]>
Signed-off-by: Hans-Kristian Arntzen <[email protected]>
Rather than MUTABLE + SSBO, we now have
MUTABLE{TEXTURE/TEXEL_BUFFER} + MUTABLE{SSBO/UBO}.
This required quite a lot of refactoring to make work, since there were
various assumptions about the old layout.
The new code is arguably much cleaner because there's now separate
MUTABLE_SET_RAW and MUTABLE_SET_TYPED flags which can control which
types are included in the mutable sets. The hack to force full mutable
types in embedded mode are replaced by just simply adding both flags
which forces all types to be enabled.
Signed-off-by: Hans-Kristian Arntzen <[email protected]>
Signed-off-by: Hans-Kristian Arntzen <[email protected]>
When MUTABLE_SPLIT_TYPES is used write out more descriptors.
- For UBO, write a NULL texel buffer. This will translate to NULL image
just fine on RADV and NV based on our tests.
- For storage image, it gets slightly more tricky:
- If we have embedded SSBO / UBO placed above the storage image area, NULL
that out.
- If we have planar metadata, there isn't much we can do. Sampling an
SSBO/UBO from the storage image will lead to UB.
- For non embedded path, always clear out the RAW side with a NULL
SSBO. (This is an option if we *have* to support 32 byte descriptor +
buggy apps, just disable the embedded path on a per-game basis if we
*really* have to).
- For sampled image, there are similar concerns:
- If 64 byte, the FMASK descriptor functions as a NULL descriptor for
non-MSAA images (99.9999% of cases in D3D12 content), it will
automatically clear RAW side to NULL. Convenient!
- If 32 byte, same concern as storage image apply. For non-embedded,
write out sibling RAW to NULL.
Effectively, this means that we don't get SINGLE_DESCRIPTOR anymore,
but copy paths on both AMD and NV are now handled in separate code that
ignores the flags anyways. Only ANV will care about the legacy mutable
path after this ...
Signed-off-by: Hans-Kristian Arntzen <[email protected]>
Signed-off-by: Hans-Kristian Arntzen <[email protected]>
Signed-off-by: Hans-Kristian Arntzen <[email protected]>
Signed-off-by: Hans-Kristian Arntzen <[email protected]>
Signed-off-by: Hans-Kristian Arntzen <[email protected]>
HansKristian-Work
force-pushed
the
descriptor-ub-exploration
branch
from
7c7b686 to
7fbfa7d
Compare
doitsujin
approved these changes
Aug 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change is mostly motivated by Armored Core VI being bugged w.r.t. descriptor usage.
I now avoid hangs on RTX 3070 and RDNA1 on the Balteus fight.
Based on my in-depth tests, I've proven that NV and AMD are robust against broken descriptor types on native drivers. They will at the very least not crash the device. With the new layout, we are far, far, far more robust against issues like these. The only concerning scenarios left are:
NV:
AMD:
ANV: