Repository navigation
fix(agc): keep a CPU store that rewrote a pending block with its baseline bytes - #14302
Conversation
…line bytes A storage write-back stores the GPU results over a CPU-stamped range whose bytes still equal the image's baseline (boykopovar#1599). When the CPU stored those same bytes into a whole 64 KiB block of the image after the GPU results were recorded, that store is the later write. Control (PPSA34547) rewrites its per-frame constant buffers over memory a storage image covers; the bytes match the snapshot the image was uploaded from, so a write-back forced by a buffer import stored the stale texels over them and shaders read loop counts such as 0x43954395. Each tracked layer now records the write generation of its last GPU results; a whole-block range that a CPU store reached after it keeps the CPU's bytes. Edge blocks, whose stamps can come from bytes beside the image, keep the byte comparison.
…urvives the write-back
…ad keeps the CPU's block A write-back keeps a unit's block for the CPU, then the image is written again without a refresh (a dispatch's imported-buffer flush comes after its image lookups). With the memory still watched, the second write-back must keep the block; after the memory left the write watch, it must not throw for missing saved guest bytes and must keep the block too.
… before a re-upload A write-back that keeps blocks of a unit for the CPU leaves the unit stale at its old generation, and no saved guest bytes. A dispatch looks up its storage images before it flushes the images pending over its imported buffers, so it can write such a unit again without re-uploading it. Two things then went wrong: the new results reset the unit's pending-since generation, so a CPU store of the baseline bytes before them no longer counted as the later write and the stale texels were stored over it; and when the memory had left the write watch in between, the next write-back threw 'storing storage image results pending over memory outside the write watch without the guest bytes saved before them is not implemented'. Control (PPSA34547) stopped on that error once in 10 runs with the previous commit. The blocks a write-back keeps for the CPU are now recorded with the unit's generation (the record the write-watch exit already uses). While it holds, new results leave the unit's pending-since as it was, and results recorded outside the write watch save the unit's guest bytes first, so the next write-back compares with them and still keeps the recorded blocks.
…e watching Both tests now return 77 when the host has no write watching, so ctest reports them as skipped instead of passed. A failed allocation while watching is on stays a failure.
|
@hugodearaujo thanks for the fix. Reviewed at b649003. One change: in the new Not blocking: an edge-block case and a CPU store between two GPU writes to an already-pending layer are not covered. |
|
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.
|
|
@oneandonlydean Thanks, done in and with The edge-block case and a CPU store between two GPU writes to an already-pending layer are noted; I'll look at covering them separately. AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The new AGC regression cases appear targeted, but this head conflicts with open #8246 in Texture.cpp and has no recorded landing-order agreement or corresponding dependency declaration.
What I ran
Reviewed PR head 34d48019fdbdbe48c76364f438ed6e49084f24f6, targeting main at PR base c674f8c14da93df7a6b37657117ca3f58357477f. A merge preview against current main 26fa1212b82f76dd49cf12d65e445d6ff3e0844c is clean; a separate pairwise preview against open #8246 head 9377004c3c38bc505be16eafb509609386318a94 conflicts in Texture.cpp. Static checks ran in the Linux review environment; I did not invoke a local compiler or build.
git diff --check c674f8c14da93df7a6b37657117ca3f58357477f 34d48019fdbdbe48c76364f438ed6e49084f24f6
python3 tools/check_conventions.py --base c674f8c14da93df7a6b37657117ca3f58357477f --head 34d48019fdbdbe48c76364f438ed6e49084f24f6 --title 'fix(agc): keep a CPU store that rewrote a pending block with its baseline bytes' --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14302-34d48019fdbd/pr-body.md
git merge-tree --write-tree 26fa1212b82f76dd49cf12d65e445d6ff3e0844c 34d48019fdbdbe48c76364f438ed6e49084f24f6
git merge-tree --write-tree 34d48019fdbdbe48c76364f438ed6e49084f24f6 9377004c3c38bc505be16eafb509609386318a94
gh pr checks 14302 --repo boykopovar/AnyPS5
gh run view 41289132063 --repo boykopovar/AnyPS5 --log-failedResults
| Check | Result |
|---|---|
git diff --check |
Passed; no output. |
| Repository conventions and PR template | Passed with 0 errors and one debt-length warning. |
Merge preview against current main |
Clean; the merge preview emitted no conflict output. |
| Pairwise preview with #8246 | Content conflict in core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp. |
Hosted Windows job, head 34d48019 |
Failed: guest_pad exited with 0xc0000409; CTest reports one failure out of 2,583, with several tests skipped. This test is outside the changed paths, so I cannot attribute the failure to this PR. |
| Hosted conventions, labels, eligibility, library portability, report | Passed. Linux and specialized jobs were skipped. |
| New AGC tests (local) | Not run locally; they require a build. The PR description reports paired Linux/RADV results, including 17/18 tests passing on the branch with the remaining test explicitly skipped; I did not independently reproduce them. |
| ABI audit and test-quality heuristic | The precheck could not execute these optional checks because their helper scripts are absent from its isolated PR snapshot. No PRX export changes are in this diff. |
Raw output — conventions and diff check
docs/dev/debt/libSceAgcDriver.md:246: warning: [debt-length] Technical debt entry over 1000 characters: state the gap first (what differs from the console, what is unverified, what throws) and keep only the behaviour it needs: 3155 characters
1 findings, 0 errors; GitHub shows at most 10 error annotations, the job summary lists all
git diff --check produced no output.
Raw output — clean current-main preview
2c099548a0d859536097dad63f9e3f9a8acdba31
Raw output — pairwise preview
decc8c9f3d05bf844c83a376da87da1fe1157e1c
100644 4ea2a6f08a3af1cba780b9c86835df3ddfe592aa 1 core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
100644 8a6f697280065d2711b4e51f175038cc4d1984fd 2 core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
100644 d3a72b19ae352c24a9d25b17f6b3372b929c85bf 3 core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
Auto-merging core/libs/prx/libSceAgcDriver/CMakeLists.txt
Auto-merging core/libs/prx/libSceAgcDriver/Graphics/include/Texture.hpp
Auto-merging core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
Auto-merging core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
CONFLICT (content): Merge conflict in core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
Auto-merging core/libs/prx/libSceAgcDriver/tests/Graphics.cpp
Auto-merging docs/dev/ENVIRONMENT.md
Raw output — hosted checks
windows fail 0 https://github.com/boykopovar/AnyPS5/actions/runs/41289132063/job/124797886511
conventions pass 18s https://github.com/boykopovar/AnyPS5/actions/runs/41289129030/job/124796521573
labels pass 9s https://github.com/boykopovar/AnyPS5/actions/runs/41289129014/job/124796521584
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41289132063/job/124797887767
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41289132063/job/124797887530
relinker skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41289132063/job/124797887927
relinker-asan skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41289132063/job/124797887408
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41289132063/job/124797887669
eligibility pass 5s https://github.com/boykopovar/AnyPS5/actions/runs/41289132063/job/124796581354
library-portability pass 1m45s https://github.com/boykopovar/AnyPS5/actions/runs/41289132060/job/124796529417
report pass 44s https://github.com/boykopovar/AnyPS5/actions/runs/41289132081/job/124796529161
Raw output — failing Windows test
windows Build and run changed guest core/libs tests 2026-11-01T09:09:22.9080036Z 2100/2583 Test #2513: guest_pad ................................................................Exit code 0xc0000409***Exception: 1.75 sec
windows Build and run changed guest core/libs tests 2026-11-01T09:09:22.9096421Z FATAL: abort() (SIGABRT) on thread 10092
windows Build and run changed guest core/libs tests 2026-11-01T09:10:35.7707941Z 99% tests passed, 1 tests failed out of 2583
windows Build and run changed guest core/libs tests 2026-11-01T09:10:35.8058239Z 2513 - guest_pad (Exit code 0xc0000409)
Findings
Blocking
- Unresolved overlap with #8246. The open PR heads conflict when previewed together in
Texture.cpp.CONTRIBUTING.mdrequires authors of overlapping changes to agree which lands first and record the dependency inDepends onwhere applicable. This PR currently saysDepends on: none, and I found no recorded agreement with #8246's author. Please coordinate the order with that author and update the checklist; if this PR must land second, declare #8246 and rebase after the dependency lands. I am not asking either author to close their PR.
Non-blocking
- Windows CI failure needs triage. The failing test is
guest_pad, which is not among this PR's changed paths. The log does not establish a connection to this change. Please confirm whether it reproduces on the same head or compare against the current base before treating it as unrelated.
Sources
- Contribution rule for overlapping PRs
- #14302 write-back changes
- #8246 write-back changes
- Windows workflow run
Not verified
- No local configure, build, CTest, Linux runtime, GPU/title run, or console behavior was performed. The Linux/RADV evidence in the PR description is author-reported, not an independent run.
- I did not independently reproduce the Windows
guest_padfailure or establish whether it occurs on currentmain.
To get this to a good state
Please record the agreed landing order with #8246 and update Depends on accordingly, then investigate or document the Windows guest_pad result. I will refresh the PR head and rerun the same applicable checks on the updated revision.
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (2, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 484 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
@MarcZX10 on the overlap with #8246 (your PR, so I'm asking you directly). A trial merge of #8246's head
Neither change uses the other's code, so there is no functional dependency. My proposal: this PR goes first, since it is current with On the Windows job: the failing test is AI-assisted: yes (Claude Code) |
|
Merged
RX 9070 XT, RADV Mesa 26.2.6, with AI-assisted: yes (Claude Code) |
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (2, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 517 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
…their units #14302 records the blocks a storage write-back keeps for the CPU with the unit's generation, and keptForCpu holds while the unit keeps that generation: new GPU results then leave the unit's pending-since as it was, so a CPU store made before them still wins at the next write-back. A re-upload ends the record by giving the unit a new generation. On memory outside the write watch the generation is 0 before and after the upload, so the record held for good: every later result of the unit kept the pending-since of the first, the CPU store that the upload had just taken counted as newer than the new results, and the next write-back kept the CPU's bytes over them. The contention checks of agc_driver_image_atomics show it: their texel is in a global array outside the write watch. After the image_atomic_add check, the umax check rewrites the texel while the add's results are pending, the umax dispatch keeps those bytes and re-uploads them, and the read-back dispatch kept them again, so the texel read back its initial 5 instead of the largest stored value. An upload now forgets the kept blocks of each unit it restamps, since the image takes the guest bytes there.
What
A storage image write-back keeps the CPU's bytes in a 64 KiB block the write watch stamped after the layer's generation. #1599 made one exception: if the stamped range still holds the image's baseline bytes (the guest bytes the image was uploaded from), the GPU results are stored anyway. The debt file already lists the gap: a CPU store that writes the baseline value back after the GPU changed it can't be told apart from no store, so the GPU result wins.
Control (PPSA34547) runs into exactly that gap. It writes its per-frame constant buffers over memory that a storage image (format 13 or 29) still covers with pending results. The constants it writes match the bytes the image was uploaded from. A dispatch then binds that memory as a buffer, and the
imported buffer regionflush stores the stale texels over the constants. Several shaders take loop counts from those constant buffers (s_buffer_load_dword→s_cmp_lt_u32loop, for example program 0x1004260100 at pc 0xaec), so they read texel pairs as counts (0x43954395,0x46d446d4,0x025a000c).fix(agc): keep a CPU store that rewrote a pending block with its baseline bytesmarkLayersPendingnow records the write generation of each layer's last GPU results. When a layer first becomes pending, it collects the layer's range first, so CPU stores from before the results get older stamps.writeBackLayers, and the same rule insaveBeforeUnwatch), a range whose bytes equal the baseline still stores the GPU results, except when the range is a whole 64 KiB block of the layer and a CPU store (ChangedBlocks' CPU flag) reached it after that generation. In a whole block every stamped page holds the layer's own bytes, so that store is the later write, whatever value it wrote.agc_unchanged_cpu_stampskeeps passing.fix(agc): a unit kept for the CPU stays kept when it is written again before a re-uploadA write-back that keeps blocks of a unit for the CPU leaves the unit stale at its old generation, without saved guest bytes. A dispatch looks up its storage images before it flushes the images pending over its imported buffers (
ShaderResources, step (2)), so it can write such a unit again without re-uploading it. With 1. alone that went wrong twice:storing storage image results pending over memory outside the write watch without the guest bytes saved before them is not implemented. Control stopped on that once in 10 runs with 1. alone (below).The blocks a write-back keeps for the CPU are now recorded with the unit's generation, in the record
saveBeforeUnwatchalready keeps for the blocks it finds written when memory leaves the watch (keptAtUnwatch). While the record holds (the unit's generation hasn't moved), new results leave the unit's pending-since generation alone. If they land outside the write watch,markLayersPendingfirst saves the unit's guest bytes, so the next write-back compares with them and still keeps the recorded blocks. The next upload of the unit gives it a new generation, which ends the record.Related: #7048 (open) saves guest bytes for the same error in a wider set of units (any unit without saved bytes, whatever its generation). This PR covers only units with blocks a write-back kept for the CPU, and it also keeps those blocks at the next write-back.
Runtime evidence: Control, cold shader cache, a diagnostic build that logs each range a dispatch or draw captures that a storage write-back stored to within the previous second. Three separate runs show the same chain. Example (run
chi-9):The keep decision for that block (run
chk-1, the same chain on image 0x38b120000):The block was stamped after the layer's generation (
changed 1), butbaselineUnchanged 1dropped it from the kept set. WithoutAPS5_LOOP_GUARDthese runs end in a GPU ring timeout ([device-fault] unfinished dispatch: ... program 0x1004260100) or inbuffer descriptor uses an unsupported type (buffer 2 V# ff1a1514 ff1a1514 ff191612 ff171410), which is a vertex shader V# table filled with texels. With an empty shader cache this happened in about 30% of runs. A warm cache never hit it in our runs, because the timing differs.Tested
Linux (Arch, kernel 7.2.11), x86-64, RX 9070 XT, RADV (Mesa 26.2.6), on
maincc26ab4.New test
agc_storage_rewritten_blocks: a linear storage image over write-watched memory, cleared on the GPU and marked pending. The CPU then writes the baseline bytes back into one whole 64 KiB block, and the image is written back. The block keeps the CPU's bytes and the other blocks get the clear color. A second case writes the same bytes before the GPU results are recorded and expects the GPU results everywhere, as fix(agc): store GPU results over CPU-stamped bytes the CPU left unchanged #1599 does. Onmainthe first case fails:(c) a CPU store of the baseline bytes after the GPU results: the GPU results were stored over the CPU's later store at +65536. With the fix it passes.New test
agc_storage_rewritten_stale_units: a 64 KiB R_X storage image (8 block units) over write-watched memory, cleared red and marked pending; the CPU writes unit 1's baseline bytes back, and the write-back keeps unit 1 and stores red elsewhere. The image is then cleared blue and marked pending again without a refresh, and written back. It runs twice: once with the memory taken out of the write watch between the two write-backs, once with it still watched. Both expect unit 1 to keep the CPU's bytes and the other units to be blue.main+ the tests: fails at the first write-back ((r) memory that left the write watch: the first write-back stored over unit 1, which the CPU rewrote with its baseline bytes after the results).storing storage image results pending over memory outside the write watch without the guest bytes saved before them is not implemented. With that case moved second, the watched case fails:(r) watched memory: the second write-back stored the image's stale unit 1 over the CPU's store, which nothing re-uploaded.Paired run with
VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1,ctest -V -R 'agc_storage|agc_unchanged_cpu_stamps|agc_driver_recorder$|agc_slice_dirty|agc_import_watch'(18 tests):main+ the tests, 14 pass, andagc_storage_rewritten_blocks,agc_storage_rewritten_stale_unitsandagc_driver_recorder(which includes the first) fail. This branch, 17 pass. On both,agc_storage_clear_hand_overis skipped by its own skip code, and noVUID-line appears.Control (PPSA34547), new game, empty shader cache per run, about 420 s, base
mainde83e8f + fix(shader): load and store 16-bit data of converted packed images #11580 + fix(shader): leave pixel helpers out of EXEC values only where ballots skip them #12458, alternating runs (series 1 used commit 1 alone, series 2 this branch):buffer descriptor uses an unsupported type (buffer 2 V# ff000000 ff000000 ff000000 ff000000)at 377 s,... V# ff23201b ff221f1a ff231f1a ff24201bat 183 s)storing storage image results pending over memory outside the write watch without the guest bytes saved before them is not implemented, 127 s)... V# ff000000 ff000000 ff000000 ff000000at 183 s)Every other run reached the 420 s limit. The counts are too small to prove anything: series 2 alone gives a two-sided Fisher p of 1.0, and the base's 3 stops in 30 runs against 0 in 20 for this branch give p = 0.27. The base hit the abort less often in series 2 (1 in 20) than in earlier runs (about 30%). Series 2 never showed the error from series 1's commit-1 run, and its 20 runs with this branch had no stop at all.
Conventions: pass (0 errors; the debt entry length warning was already there before this change)
Rerun on this branch merged with
mainc674f8c (one conflict inmarkLayersPending, wheremainnow skips the capture when every unit is already pending; the pending generation is still recorded for every unit): the same 18 tests pass underVK_LAYER_KHRONOS_validation, withagc_storage_clear_hand_overskipped by its own skip code and noVUID-line.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)