Repository navigation
fix(libSceAgcDriver): flush pending results at staged-clear sampled reads - #4794
Conversation
|
Windows test of #4794 (MinGW GCC 15.2, Release, Windows 10, RTX 4080 SUPER) on current
So the change to when image results are stored makes an existing test fail on current AI-assisted (Claude Code). |
Thanks for the report. DccKeysFirstWrite exercises pending-image and deferred write-back behavior, so I’ve made it explicitly enable APS5_DEFERRED_WRITEBACK=1 before graphics initialization. The PR’s eager-by-default behavior is unchanged; the deferred path remains available for these pending-state assertions. I rebuilt and ran the test on the PR branch, and it passes. |
|
I built this PR merged onto current main (fdefd76) and ran the existing execution tests that check for a pending image right after a store or draw (WSL2 Ubuntu, GCC 15.2, CI CMake configuration, Release, lavapipe). Besides
Each of them passes with The reason, on the PR head afb8881: with the new default, The linux and windows CI jobs were skipped on this PR, so none of these tests ran in CI. Setting AI-assisted: written with Claude Code; I checked the code paths and ran the tests described above. |
faadd26 to
40aceb7
Compare
Thanks for checking these tests. I rebased the PR onto current main and updated the affected tests to distinguish eager write-back from deferred pending-image behavior. ArrayLayerTarget, ImageSrgbLoad, and StorageOneSliceAlias still run with the eager default and verify their actual read-after-write results. DccKeysFirstWrite, PendingExactSurface, and DepthCompareCopy explicitly use deferred mode because they test pending-image or depth-copy behavior that depends on that mode. I rebuilt and ran all six tests locally on Windows: all passed with the eager default, and all passed with APS5_DEFERRED_WRITEBACK=1. The conventions check also passes. The Linux and Windows PR CI jobs are currently skipped, so they still need to run. |
CnRJay
left a comment
There was a problem hiding this comment.
The tests updated in the latest push now expect no pending image under the eager default. The bigger problem is what the new default costs every title, not only Bendy.
Eager write-back turns every storage-image or render-target write into a GPU→CPU round trip. markLayersPending runs when a draw or dispatch that writes the image is recorded, before the GPU has run it. WriteBack() at that point has to get the results into guest memory: submit the open batch, wait for the GPU to finish the producer, copy the image out and store it on the CPU. Doing that after every writing draw serializes the CPU and GPU on every render pass. The recorder exists to batch work and let the queue worker run ahead of the GPU, and this removes that overlap for every title that renders to an image, which is nearly all of them. APS5_EAGER_WRITEBACK was opt-in for that reason. Making it the default (and dropping the APS5_NO_TEXTURE_CACHE link) is a large performance regression even where it is correct.
Deferred write-back isn't wrong by itself: guest memory only needs the results when something reads guest memory, such as a CPU read, a DMA, or a sampled view that goes through guest bytes. The flush hook and FlushPending exist to write back exactly then. The whitening means one consumer in Bendy's feedback loop reads the surface without going through either. Most likely a cached sampled view or resident image of the same memory is reused without checking the pending image's version, or a texture lookup over an address range that only partially matches the pending image misses it. A fix for that consumer would keep deferral for everyone else:
- Find which lookup returns the stale content.
APS5_PROFILE_DRAW's sampled-texture and resident-target counters, orCaptureTracewithimage-writelines, will show whether the feedback read hits a cached sampled texture, a resident target or guest memory. - Make that path call
FlushPendingfor the range, or sample the pendingStorageTexturedirectly (the way resident targets are sampled) when its version is newer than the cached view.
Please also add before/after summary lines (APS5_TRACE_SECONDS) for one title other than Bendy. I'd expect the frame time to show the regression clearly.
|
Making eager write-back the default ( |
markLayersPending deferred its write-back, so a sampled-lookup of a surface read through a resident view fed the freshest GPU content back only at flip time. The menu composite loops its own trio as feedback each present, so the deferred view accumulated a pale frame until flip and whitened Bendy's opening menu against the (120, 87, 17) reference. Store a written image's results to guest memory at once, in recording order behind the draw that produced them, and keep the batched store behind APS5_DEFERRED_WRITEBACK=1 for comparison.
fd8f308 to
cf43ef6
Compare
adriadam10
left a comment
There was a problem hiding this comment.
Not merging this as is, for the reasons @oneandonlydean and @CnRJay already gave, which haven't been addressed:
Texture.cpp:3033makes eager write-back the default, so every image-writing draw/dispatch waits for the GPU and copies back to the CPU in every title, not only in this menu. Please keep deferral as the default.- The existing execution tests that fail with the eager default (
agc_driver_dcc_keys_first_write,depth_compare_copy,image_srgb_load,storage_one_slice_alias, per @xXPBSXx and @nikowolf123) now branch onAPS5_DEFERRED_WRITEBACK(ArrayLayerTarget.cpp,StorageOneSliceAlias.cpp,DepthCompareCopy.cpp...). They should stay as they are on main: they pin the deferred contract. - The root cause is the read in the feedback loop that misses the pending image. Fix that lookup (
FlushPendingthere, or sample the pending image when it is newer) and add a test that fails on main and passes with the fix.
Feel free to push that and we'll re-review.
Already working on it, should update soon 👍 |
…n tests The eager writeback default is reverted: Texture.hpp/Texture.cpp return to upstream/main, keeping deferred writeback the default. The execution tests (ArrayLayerTarget, DccKeysFirstWrite, DepthCompareCopy, ImageSrgbLoad, PendingExactSurface, StorageOneSliceAlias) return to their upstream/main state without deferred-mode branching.
…eads A title may stage DCC fast-clear keys while the draw results for the surface are still pending (Bendy stages its next frame clear behind the feedback loop). The cached-texture lookup only flushed pending results when no image was cached, so a read with staged clear keys served the clear over the results the pass had drawn. The lookup now flushes at that point for staged-clear keys, reads the keys uncompressed for that read, and keeps the flushed image out of the hit view. StagedClearSampledRead covers the case: it fails without the flush (the lookup serves without the pending results in guest memory) and passes with it.
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for reverting the eager default and restoring the main tests; the targeted FlushPending at the staged-clear sampled read is the direction asked for. Two things left:
StagedClearSampledRead.cpp:50,83:UnalignedBlockBytes = 2 * 65536 + 2048is passed toGuestWriteWatchRegister_nid_postfix, which rejects it on Linux (write watch: cannot register ...+0x20800 (Invalid argument)), soagc_driver_staged_clear_sampled_readfails on theRequireat line 87 (built merged onto current main, Release, RADV). Register a page-aligned size (asDccKeysFirstWrite.cppdoes withBlockBytes). Please also confirm the test fails without theShaderResources.cppchange once it runs.- The PR description still says eager write-back is the default and mentions
APS5_DEFERRED_WRITEBACKand the A/B toggle; please update What/Tested to describe the actual change (flush at staged-clear sampled reads) and what you tested on this head.
…on on POSIX The POSIX watch registration passed UnalignedBlockBytes (2*65536+2048), whose 2048 remainder is rejected by UFFDIO_REGISTER with EINVAL, so the test failed at the not-write-watched Require on Linux. Register and unregister ArenaBytes (3*65536), which is page-aligned; the Watched Require and Mutation().Add keep UnalignedBlockBytes so the Windows host-import refusal that preserves the write-watch is unchanged.
|
A data point from PPSA15304 (Linux, RTX 5080) for the direction here: storing results right behind the draw (the mode |
The eager-writeback commits added StorageTexture::StoreAtLabelRequested (APS5_STORE_AT_LABEL). Nothing reads that variable, so the helper is dead code; the deferred-writeback revert kept it by accident. Remove it so the PR changes only the staged-clear sampled-read flush and its test.
|
@adriadam10 @oneandonlydean @CnRJay - all review points are addressed on the current head (bc316c7); ready for re-review. Since the last review:
Verified on Windows: menu renders correctly (0/10 whitened frames across a 29 s soak), deferred-contract tests green (dcc_keys_first_write, depth_compare_copy, image_srgb_load, storage_one_slice_alias, image_load_packed), conventions pass, merges cleanly into current main. @CnRJay: with eager reverted there is no per-draw store left to measure, so the before/after APS5_TRACE_SECONDS request no longer applies. The fix adds one flush at the read, not a write-back mode change. Happy to provide traces for the flush path if you still want them. @ee6aba54: thanks for the PPSA15304 data point. It's exactly why this PR keeps deferral; the new flush reuses the same guarded FlushPending path your #6185/#6186 harden, so it composes with them. |
adriadam10
left a comment
There was a problem hiding this comment.
Checked on this head merged onto current main (Release, RADV): agc_driver_staged_clear_sampled_read passes, and fails when the ShaderResources.cpp change is reverted; the AgcDriver-scoped ctest is otherwise green (agc_driver_tessellation segfaults on main here too). The page-aligned registration and the PR description are fixed. Merging once the full Build is green.
What
Fix a coherence hole in the sampled-texture lookup: a read of guest memory could miss the pending results of draws that wrote the surface, and serve staged DCC fast-clear keys (or stale bytes) instead.
The lookup (
cachedTexture) only flushed pending results when no image was cached (source == nullptr). A lookup that hits a cached image and carries staged clear keys therefore skipped the flush: it served the clear over the results the pass had drawn. Deferring the write-back is fine — guest memory only needs results when something reads it — but this read is such a consumer and wasn't going through the flush. Any title that stages fast-clear keys for a surface while its draw results are still pending, and then samples that surface through a cached view, hits this; it is not specific to one game.The change, in
cachedTexture(ShaderResources.cpp): when a lookup has staged clear keys (IsDccClear/ClearSingle) over a pending image, callFlushPendingat that point, read the keys uncompressed for that read, and keep the flushed image out of the hit view. The flush goes through the guardedFlushPendingwrite-back path, so it inherits the late-store protections from #6185 / #6186.Activation is gated on staged-clear-key lookups only: titles that don't stage clear keys over a pending surface take no new flushes and pay nothing. Deferred write-back stays the default and the only mode; no env-var toggles. The existing execution tests are restored to their exact state on main (they pin the deferred contract and no longer branch on
APS5_DEFERRED_WRITEBACK).Net change vs main: ShaderResources.cpp (the flush, 12+/3−) and the new StagedClearSampledRead.cpp test.
Earlier revisions of this PR made write-back eager by default (
markLayersPendingstoring after every writing draw). That was reverted per review: eager turns every storage-image/render-target write into a GPU→CPU round trip and serializes the recorder (on PPSA15304, Linux RTX 5080: 0.3 presents/10 s with ~400 GB/10 s of write-backs vs 15–20 deferred). The hole was this one read, so the fix is this one read.Before: (main menu is unusable)

After: (main menu working properly)
eboot_HL15XpTqTg.mp4
Why (correctness)
The invariant: a sampled read of guest memory must see the results of draws that wrote the surface, including when those results are still pending on the GPU and the title has already staged fast-clear keys. The sampled-lookup path already forced
FlushPendingwhen nothing was cached, but the cached-image hit with staged clear keys skipped it — so the clear was applied over pending results that existed, and the read returned content no GPU state ever produced.Flushing at that lookup (and reading the keys uncompressed for the read) restores the invariant at the point of the read, without changing when write-back happens for anyone else. Deferral stays intact: guest memory is still only written when a consumer actually needs it (CPU read, DMA, or a sampled view over guest bytes), which is what keeps the recorder batching and the queue worker running ahead of the GPU.
Tested
dcc-keys refreshchurn, staged-clear flushes observed (flushed-staged)agc_driver_staged_clear_sampled_read(title-independent, exercises the invariant directly): stages a DCC clear over a pending surface and reads it back through a sampled view. Fails on the pre-fixShaderResources.cppwith "the sampled read did not store the pending results: the lookup serves without them in guest memory", passes with the fix. Its write-watch range is registered with a page-aligned size so it runs on Linux (UFFDIO_REGISTER); verified on Windows; the page-aligned registration addresses the Linux UFFDIO_REGISTER rejection (fails-without-fix confirmed on Windows at the same Require).dcc_keys_first_write,depth_compare_copy,image_srgb_load,storage_one_slice_alias,image_load_packedninja libs+ test targets) with no regressionspython tools/check_conventions.py --base c0a272537 --head HEADpassedgit diff --checkpassedChecklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs listed under Silent stubs where applicable-Depends on: none