Repository navigation
fix(agc): order driver stores over a depth plane by record order - #15621
Open
Astronaut10 wants to merge 2 commits into
Open
Astronaut10 wants to merge 2 commits into
Astronaut10 wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015W3M6LnkU48VGkEHYRTjou
12 tasks done
XairRrR
approved these changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — approved
The fix is now ordered by record order: storing a depth plane's pending aliasing images before SyncFromMemory takes its generation, and keeping units drawn after a driver store, restores the drawn depth. Each half is covered by a check in AliasedStoreAfterDrawCase that fails when that half is disabled.
What I ran
Head eab4ee3ac6013f9d82b969ddd775ab1ee8fb929d, main 1c8c0efc8c41314402e6966da5bef6ea3d639d10, merged tree 9dd4f135b3e899321d23e326c68162fadb65808f (Debian 13 VM, GCC 14.2.0, lavapipe Mesa 26.1.6 / LLVM 19, no GPU).
git diff --check 1c8c0efc8c41314402e6966da5bef6ea3d639d10...eab4ee3ac6013f9d82b969ddd775ab1ee8fb929d
python3 tools/check_conventions.py --base 1c8c0efc8c41314402e6966da5bef6ea3d639d10
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer \
ctest --test-dir build -V -R "depth_plane_sampling|image_gather_volume"
# main's six production files restored:
ninja -C build -j32 -l32 agc_driver_image_gather_volume_tests
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer \
ctest --test-dir build -V -R "^agc_driver_image_gather_volume$"
# variants A and B (see Negative control), each:
ninja -C build -j32 -l32 agc_driver_depth_plane_sampling_tests
env ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build --output-on-failure -R depth_plane_sampling
# merged files restored, rebuilt, rerun; git status --porcelain clean
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
Configure / build / libs (script) |
ok |
| Selected tests (script) | 2243 ran, no new failures or timeouts. The 7 known failures also fail on main |
agc_driver_depth_plane_sampling / _evicting |
passed (4.7 s / 8.5 s). Both print "no write watch" for the two write-watch cases, as the description says |
agc_driver_depth_plane_sampling_unimported (new) |
passed (5.1 s), and runs the write-watch cases |
| Validation, depth suite (3 tests) | layer loaded, no VUID- lines, no "was not found but was requested" |
Validation, agc_driver_image_gather_volume |
8 VkSamplerCreateInfo-addressMode* VUID lines with the PR, the same 8 with main's production files, so they come from main. The PR does not touch sampler code |
VUID lines, image_gather_volume, merged tree and main production files (identical)
2 VUID-VkSamplerCreateInfo-addressModeU-01079
2 VUID-VkSamplerCreateInfo-addressModeU-parameter
2 VUID-VkSamplerCreateInfo-addressModeV-parameter
2 VUID-VkSamplerCreateInfo-addressModeW-parameter
Negative control
With main's production files fully restored, the new test cannot build, because it calls NextRecordOrder and KeepNewerDepthUnits. So I disabled each half of the fix separately in the merged tree:
- A: the
StorePendingImages(resource)call inSyncFromMemory(core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp:1367) removed.agc_driver_depth_plane_sampling_unimportedfails:
D32 plane drawn after an aliasing image's results were recorded, sampled after those results were stored: texel 0 read 0x3f800000, expected 0x3e800000 - B:
KeepNewerDepthUnitsmade to always returnstamp()(DepthSurface.cpp:3206).agc_driver_depth_plane_sampling_unimportedfails:
D32 plane sampled after a GPU write recorded before its draw completed: texel 0 read 0x3e800000, expected 0x3ec00000
Both match the messages in the description. In both variants the two import-on entries still pass, because they skip these cases. Afterwards I restored the merged files, rebuilt, and reran the three entries (all passed). git status --porcelain was empty.
Findings
Blocking
None.
Non-blocking
- The keep inside storage-image stores is not exercised end to end. In variant B, the aliased-image check (sample = drawn depth, plane store writes drawn depth) still passes. That check depends only on the pre-sync store. The record-order guard is proven only by the direct
KeepNewerDepthUnitscalls in the test (core/libs/prx/libSceAgcDriver/tests/execution/DepthPlaneSampling.cpp:410and:414). Two paths have no test that fails without them: the wrapper inStorageTexture::writeBackLayers(core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp:6606-6626) and the twoGuestBufferMemory::WriteBackstamps. The description already says the imported-buffer completion is not exercised. Suggestion for a follow-up: a case where an image's results are recorded before a depth draw over the same plane and stored only after it, outside any bind, so that only thewriteBackLayerskeep protects the drawn units. - I checked the locking and found no problem.
KeepNewerDepthUnitstakesGpuMutexand thensurfacesMutex, the same order as the rest ofDepthSurface.cpp; both mutexes are recursive.stamp()runs outside both locks.FlushPendinggathers its list underpending.mutexafter takingGpuMutex, so calling the keep fromwriteBackLayersadds no inverted lock order that I could find. The post-stamp re-check usesUnchangedSinceCollected, which ignores driver stamps, asStampKeepingNewerResultsdoes. - The debt entry added to
docs/dev/debt/libSceAgcDriver.mdstates the remaining gap (BDA completion stamps throughmarkWrittenPagescarry no record order). It links a file that exists (Graphics/src/BdaResources.cpp, which definesmarkWrittenPages) and is in the right debt file. No code comments were added.
Not verified
- No run on real GPUs here. The only GPU named is AMD RX 9070 XT (RADV); this machine has only lavapipe.
- I did not check the Astro Bot (PPSA21564) frame-dump results or the performance numbers (about -1.8 % FPS against the earlier base, about 3 ms of draw-thread time per frame); no title was run.
- I did not check the PPSA05684 regression on the title; only
GuestWriteAfterDrawCasepasses (in the unimported entry). - I did not check the BDA (
markWrittenPages) and imported-buffer completion paths end to end.
Collaborator
|
Hi Could the new A dedicated argument for this entry could keep the other suite invocations' partial coverage while requiring these two cases here. Please check both the unavailable-watch result and a watched run. |
Contributor
✅ 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)Validation IDs already present on main (8 IDs in 532 tests)Skipped (50)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. |
github-actions
Bot
added
the
review queue
Contributor
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (7, not counted against this PR)Validation IDs already present on main (8 IDs in 532 tests)Skipped (61)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
…missing The unimported entry exists to run the guest write and aliased store cases after a draw. Without a write watch both cases returned early and the entry still passed. It now passes --require-write-watch and returns the skip code when either case could not run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Done in Checked both ways on an RX 9070 XT (RADV), validation layer loaded, no
|
Contributor
✅ 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)Validation IDs already present on main (8 IDs in 532 tests)Skipped (50)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. |
Contributor
|
Hey folks, I checked Astronaut10's latest Windows 11 Pro 26300, WinLibs GCC 15.2.0 Release, SPIRV-Tools enabled. GPUs were Intel UHD 770, driver 32.0.101.7088, and RTX 4070 SUPER, driver 621.01. Every GPU run required Vulkan, disabled the shader cache and loaded the Khronos validation layer. All three depth-plane sampling entries passed on both GPUs. The unimported entry ran both write-watch cases. With I also checked the two existing regression controls on each GPU:
The surrounding selected tests gave:
Intel's failure is There were no VUID or synchronization-validation reports in these runs. Full builds of main and the tested merge completed, conventions pass, and the checkout is clean. The end-to-end writeback-wrapper coverage noted in the review is still open. I tried an additional mirror fixture, but it also passed after bypassing that wrapper, so it did not qualify as a regression test and is excluded from this submission. The ZIP contains the pinned run receipts, logs, control patches and hashes. Full-suite, game and performance validation remain outstanding. AI-assisted investigation with independent source and evidence review. |
Contributor
✅ AMD 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 (7, not counted against this PR)Validation IDs already present on main (8 IDs in 532 tests)Skipped (61)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Contributor
|
Hey folks, I found why the earlier wrapper-test attempt was passing with the protection removed, and now have a test that catches it. The CPU write-back path calls The new Test-only commit
The main comparison carries the identical new test function and its memory assertion helper, with a small argument-parser adaptation. It carries none of this PR's production changes. The archive includes that exact comparison patch as well as the wrapper-removal control. Full Windows Release builds passed for the candidate, the main comparison and the restored candidate. The three existing sampling entries passed on both GPUs in the final and restored runs. These runs used GCC 15.2.0, bundled SPIRV-Tools enabled, fresh shader compilation and the Khronos validation layer. Every tested invocation loaded the layer, with no VUID or synchronization-validation errors. NVIDIA driver 621.01, Intel driver 32.0.101.7088, Windows 11 Pro 26300. The patch and author-preserving mbox are attached for this PR's review. This test depends on #15621. The new regression is verified on the NVIDIA watched-import path. Intel, Linux, macOS and title behavior remain outside that new coverage. The two AI-assisted implementation, investigation and independent review with OpenAI Codex. Patch and test logs — SHA-256 |
Contributor
|
A GFX8 data point: AMD Radeon RX 570 (RADV POLARIS10), Mesa 26.2.5, SteamOS 3.8.28 (Arch distrobox), GCC 16.2.1, Release. Head
The 1,327 AGC, shader, VideoOut and Vulkan tests, once each:
AI-assisted: yes (Claude Code) |
Superseabas
approved these changes
Nov 6, 2026
Superseabas
left a comment
Superseabas
left a comment
Contributor
There was a problem hiding this comment.
Windows run with AMD's own driver, on the integrated Radeon of a Ryzen 7 7700X, and on an RTX 5070 Ti. The earlier AMD results here are RADV on Linux and the Windows ones are Intel and NVIDIA. Everything passes, both halves of the fix are covered on both GPUs, and nothing regresses.
Head db1f5844d8 merged onto main ed41d86d32 (clean merge, local only). Windows 11 Home 10.0.26200, WinLibs GCC 16.2.0 (x86_64-ucrt-posix-seh, r3), Release. ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, no missing-layer line, ANYPS5_GPU selecting the device, confirmed by Selected GPU:
- AMD Radeon of a Ryzen 7 7700X (
1002:164e, integrated), AMD 26.10.2 (32.0.21045.20006) - NVIDIA GeForce RTX 5070 Ti (
10de:2c05), 621.01
Depth-plane tests. All 16 agc_driver_depth_plane_* tests on main, and the 17 on this PR (with _sampling_unimported), pass 3 of 3 on both GPUs. No VUID- lines. Write watch works on both devices (host imports of write-watched memory keep the write watch: yes), so the two write-watch cases run in every entry: no "not checked" line, and _sampling_unimported passes instead of skipping.
Red checks, each a local edit of the merged tree, 3 runs per GPU of agc_driver_depth_plane_sampling, _sampling_evicting and _sampling_unimported:
| Edit | Result on both GPUs |
|---|---|
| none | 3 of 3 |
StorePendingImages(resource) removed from the sync |
0 of 3: D32 plane drawn after an aliasing image's results were recorded, sampled after those results were stored: texel 0 read 0x3f800000, expected 0x3e800000 |
KeepNewerDepthUnits returning stamp() before it collects the units to keep |
0 of 3: D32 plane sampled after a GPU write recorded before its draw completed: texel 0 read 0x3e800000, expected 0x3ec00000 |
A revert of the six driver files to main doesn't build, because the test calls NextRecordOrder and KeepNewerDepthUnits directly. So these edits keep the functions and switch off one half each.
Whole AGC set. ctest -R '^agc' -j 8 --timeout 300: 1,387 of 1,389 pass on the RTX and 1,386 on the Radeon. Those are the same failures as main on this machine, plus the new entry, which passes. All are tracked: agc_driver_direct_write_completion (#15564), agc_driver_quad_pixel_mask on the RTX (#14341), and on the Radeon agc_driver_scratch_pool (#15566) and agc_driver_sample_order_grid_reference (#9503).
check_conventions.py --base main passes. Astro Bot itself isn't tested here.
AI-assisted: yes (Claude Code)
12 tasks done
12 tasks done
|
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.
|
github-actions
Bot
added
needs rebase
This branch has not been deployed
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
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.
What
#12238 reloads a sampled single-sample D32 Z64KBX depth plane from guest memory when the tracker says the guest wrote it after its bind. The tracker also stamps the driver's own stores, by when they are stamped rather than when their results were recorded. In Astro Bot (PPSA21564) Aerial Garden a colour image aliasing a 1920x1080 depth plane is stored once per frame by the bind's own mirror refresh, after the bind took its generation, with results recorded before the bind; at the sample all 135 units look guest-written and the drawn depth is replaced by colour bytes, so the cloud sea and haze render as teal water.
This keeps #12238's reload and orders driver stores against depth draws:
NextRecordOrder).KeepNewerDepthUnitslets a driver store (storage image layer stores, the direct and copied-back branches ofGuestBufferMemory::WriteBack) invalidate only units whose last draw was recorded before it, asStorageTexture::StampKeepingNewerResultsalready does for storage images.SyncFromMemoryfirst stores pending images inside the plane (not its own mirror), so stores its refresh would trigger fall inside the bind's generation.GuestWriteAfterDrawCase) is still reloaded.markWrittenPages) carry no record order and are excluded from this fix; listed indocs/dev/debt/libSceAgcDriver.md.New test
AliasedStoreAfterDrawCase(image results pending over the plane, bind + draw, image stored, sample = drawn depth, plane store writes drawn depth to memory, plus the record-order guard called directly with a GPU write recorded before / after the draw; an imported-buffer completion is not exercised end to end). New ctest entryagc_driver_depth_plane_sampling_unimported(APS5_NO_HOST_IMPORT=1): with host imports on, the write-watch cases of this suite (#12238'sGuestWriteAfterDrawCaseand the new case) print "no write watch" and return without checking, while the rest of the suite runs; this entry runs them.Related: #15551 (open) changes the line after the one this adds in
GuestBufferMemory::MarkDirectWrites(the storage image write order taken under the tracker lock); the two order different things and compose, but whichever merges second needs a one-line rebase.Tested
Linux (CachyOS, kernel 7.3.0-4-cachyos), AMD Radeon RX 9070 XT (RADV GFX1201), Mesa 26.2.5-arch3.1, Astro Bot PPSA21564 01.007.
On
main086c0686c8(this branch,eab4ee3ac6):ctest -R 'agc|guest_memory'withANYPS5_REQUIRE_VULKAN=1: the same statuses asmainplus the new entry (passed), 1,288 against 1,287.agc_driver_scattered_runsandagc_driver_tessellation(segfault) fail onmainas well.agc_driver_depth_plane_samplingand_evictingprint "no write watch" for the two write-watch cases;agc_driver_depth_plane_sampling_unimportedruns them and passes.ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ctest -V),mainand this branch: layer loaded in both (vulkaninforeports it; no "was not found but was requested" line), 244 and 245 passed, the same sixVUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605lines (all fromagc_driver_srgb8_color_target) and 20WARNING-VkImageSubresourceRange-layerCount-compatibilitylines in both; this branch adds noVUID-.APS5_DUMP_FRAMES=40, every 25th frame, one run per side):mainhas no cloud sea (bottom third 1-3 % white in all 11 in-level dumps; the first shows teal water where the clouds are), this branch has it (80, 71, 61, 50, 37, 21, 10, 6, 2 % in its 9 in-level dumps, falling as the camera leaves the clouds, the same course as on the earlier base). Frames available on request.On the earlier base
3ab5a11d66(the same change as450bc82830, before the rebase; the cost numbers below were not repeated on currentmain):GuestWriteAfterDrawCase(PPSA05684's regression case) passes; PPSA05684 itself was not run.APS5_PROFILE_DRAW, 2 runs per side, against the base with the line removed) puts the new code's own cost at about 3 ms per frame of draw-thread time, mostly waiting for the storeSyncFromMemorynow makes at the depth bind; that thread was about 74 % busy and the frame did not get longer, so it did not show on the critical path there.tools/trace_digest.pyof theaps5-summary.jsonlof one adjacent pair (APS5_TRACE_SUMMARY_EVERY=10, the level starts at about 19 s, the measured window is 40 s after it); the summary files of all eight runs are available on request. NoAPS5_TRACE_FRAMESwindow trace was captured for these runs.base 3ab5a11
this change (450bc82830 on 3ab5a11)
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)🤖 Generated with Claude Code
https://claude.ai/code/session_015W3M6Kw7yNRginUW6VUeE2