Repository navigation
feat(agc): sample multisampled depth through a shadow image - #7318
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
Overlap note after comparing #7256: the features are separate—#7256 adds single-sample layered depth views, while this PR adds a sampled shadow for native multisampled depth. The layered-depth path explicitly rejects multisampled layers, so I found no shared behavior or prerequisite. However, both change |
|
Thanks for splitting the shadow out of #5499. I ran the PR's own test on a device, and the shadow path returns stale depth. Run: RTX 4090, Linux, main a8581d3 + this PR at ea33567,
Every failure is With the copy forced every time, all four pass. So the copy and its barriers are fine, and the problem is the skip at DepthSurface.cpp:590: if (!drawn && sampleShadowWritten == written) return;It treats the shadow as current when it isn't. Two things in the code look like they cause it:
Could you:
Not blocking, but worth looking at in the same pass:
|
ea33567 to
ff332e7
Compare
|
Thanks for the detailed reproduction; it exposed a real stale-shadow path. I pushed The fix creates the shadow only on first sampling, gives every bound native surface a nonzero write generation, and advances the previous surface generation when another depth target becomes active. That makes a later sample of the no-longer-active target refresh its shadow. The regression now rasterizes one target, binds a second target, then samples and checks the first target\x27s depth readback. The exact rebased head builds I also changed shadow allocation to first sample, addressing the unused-shadow memory cost. The copy remains per use while the surface is the active draw target for correctness; I do not have a title measurement for that cost yet. |
|
I ran head ff332e7 on an Apple GPU. The new regression still catches a stale shadow, and the cause isn't device-specific. There's also a second failure, in Machine: Apple M2 Max, macOS 27.2, x86-64 build under Rosetta. Method: main 81cb863 + @mugurc's
Both drivers give the same results. The failure is the same one as in the earlier run: Where it goes stale. It fails in the second loop pass (offset 0.25). Every sample reads 0.0625, the depth from the offset-0 raster, where 0.3125 is expected. In that pass:
The first pass only works because binding A one-line fix. In Recorder::CountBarriers(Recorder::CommandClass::Draw, 1);
- return found->Sampled(words, resource, components);
+ return found->Sampled(words, resource, components, found == drawnDepth);With that change, all five Second failure: That's Full GPUs tested: Apple M2 Max (MoltenVK 1.4.2; MoltenVK main + SPIRV-Cross #2704) Written with AI assistance (Claude Code). I ran the builds and tests above on my machine. |
|
I checked your report against the current head The reproduced stale-shadow failure is therefore actionable. Please pass I have not pushed a change to your branch. My local GCC 12 test build is currently blocked earlier by an unrelated |
|
Fixed in Validation: |
|
Thanks for the rework. Review and CI both pass on 132dbd1, but the PR now conflicts with main after #6635 (MSAA array descriptor shape) merged, in It isn't a mechanical conflict, so it needs your rebase rather than ours. #6635 adds Could you rebase onto current main, keep #6635's array view for multisampled targets and keep both sets of test checks? After that we'll re-review the delta and run full CI. |
132dbd1 to
91be80b
Compare
|
Rebased this branch onto current main and resolved the #6635 overlap while preserving the array view type in the sample-view cache key and in texture creation, refreshing the bound depth shadow before cache reuse, and retaining both the existing array-view regression coverage and the new depth-surface reuse checks.\n\nValidation: the |
jotitadev
left a comment
There was a problem hiding this comment.
A device run of the rebased head 91be80b44, since the host you rebased on stops at the CPU guard and hosted CI hasn't run on it. Everything passes on an RTX 3070 and on lavapipe, and the new regression is what catches the stale shadow I hit on ff332e7ef.
Machine: Ubuntu 26.04, i7-8700, GCC 15.2, Release, BUILD_TESTING=ON, every execution test built. NVIDIA GeForce RTX 3070 (driver 595.99.02) and lavapipe (Mesa 26.0.3, LLVM 21.1.8) on the same host. PR merged onto main 84c1a4d9a (clean, so #6635 is in). ANYPS5_NO_SHADER_CACHE=1, -j1 --timeout 30.
Tests: agc_driver_native_multisample_depth, _depth_retirement, _depth_clears, _depth_clear_refusals, _depth_bda_clear, agc_driver_native_msaa_vertex_input and agc_driver_graphics (which has TestMultisampleDepthOwnership).
| RTX 3070 | lavapipe | |
|---|---|---|
head 91be80b44 |
7/7, 3 runs | 7/7 (depth_bda_clear skips, as on main) |
only the PR's two test files, on main |
the 6 native tests pass; agc_driver_graphics fails |
same |
PR, early return in refreshSampleShadow removed (always copy) |
6/6 native | pass |
PR, useSampleShadow = false (direct path) |
6/6 native | pass |
PR, drawnDepth->written = ++depthWrites removed from DepthSurfaceView |
multisample_depth, depth_clears, depth_clear_refusals fail |
same 3 fail |
- On lavapipe "pass" includes the
depth_bda_clearskip. agc_driver_graphicsonmainfails witha native MSAA depth attachment must keep sampled usage off when the shadow path is available. That is the updated ownership check, so it is expected. With the PR it passes.- With the line that advances the previous target's generation removed, the three failures are
native depth sampled fetch lost a raster sample or its component mapping, the same three tests and message as the stale shadow onff332e7ef. So the new "bind another target, then sample the first" case does guard that bump. The 6 native tests pass onmainbecausemainsamples the attachment directly and has no shadow to go stale. - The
2D_ARRAYchecks from #6635 in the same loop pass on both devices, so the array view over the shadow image comes back from the cache unchanged across the raster writes.
Full ^agc suite with the PR (731 tests, without display/flip/lds_atomics64): the same result as main on both devices. lavapipe 731/731 with the same 50 skips. RTX 3070 730/731 with the same skip, and the one failure is agc_driver_bda_device, which fails on main here too (#7584: a fence timeout, Vulkan result 2, in this run, and device lost, -4, on main).
Not run: Windows, AMD, any title, and the memory or copy cost of the shadow.
GPUs tested: NVIDIA GeForce RTX 3070 (595.99.02) and lavapipe (Mesa 26.0.3), Ubuntu 26.04
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Thanks for running the rebased head on both the RTX 3070 and lavapipe. The full reported suite passes on both, and the new regression catches the stale shadow image from the earlier head. I’ll record this as current-head device validation, with the tested merged-main composition including #6635. |
|
Read the diff against the earlier findings (stale shadow after switching attachments, |
What
Multisampled depth attachments currently add
VK_IMAGE_USAGE_SAMPLED_BITto the native image when a device supports it. Create a separate same-format, same-sample-count shadow image with sampled and transfer-destination usage instead, and copy the native depth samples into it before each per-use read that needs a refresh. This keeps sampled usage off the depth attachment and copies every sample without resolving it. If a device cannot create that shadow-image usage combination but can sample the original attachment as before, retain the direct path as a fallback.The native multisample depth regression now resolves the cached sampled view again after each raster write and checks the readback, so stale shadow contents fail the test across the supported D16/D32 and 2x/4x/8x cases.
This is the separate shadow-image prerequisite requested in the review of #5499. It does not implement the DB depth-copy draw; #5499 can build on this after it merges.
Tested
Rebased onto current
mainat5229b5b82; current headff332e7efdb05563ca03265d3511810c9f445a52.Linux x86-64, GCC 12.2.0, Release,
BUILD_TESTING=ON: configured and builtagc_driver_native_multisample_depth_tests. GCC 12 stops on an unrelated dependent-template parse error inSrtDescriptorEvaluation.cpp; for the local build only, I applied thetemplatedisambiguator in that file and restored it afterward. The changed AGC source and regression test compiled successfully on the rebased head.The regression now writes the first native multisample depth target, binds a second target, then samples the first while it is no longer the active draw target. It checks the depth readback. The shadow image is also allocated on first sampling, avoiding the extra image for eligible surfaces that are never sampled.
ctest --test-dir /tmp/AnyPS5-7318-review --output-on-failure -R '^agc_driver_native_multisample_depth$' --timeout 120aborts before Vulkan device selection and assertions at the project CPU-feature guard: this Xeon E5-1620 v2 lacks LZCNT, BMI1, BMI2, MOVBE, AVX2 and FMA. No local GPU runtime or title test is claimed. Conventions andgit diff --checkpass.Manual overlap review: #6146 changes the out-of-place depth-expand path; #6557 expands HTILE eligibility coverage in the same test fixture; neither implements shadow refresh. #7256 adds layered single-sample depth views and is a separate behavior, though both changes touch
DepthSurface.cppand will need manual integration if both merge. The overlap tool also cannot compare older conflicting PR bases, so its warning is not treated as proof of no overlap.Checklist
main; no other open PR implements the shadow-image pathNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository