Repository navigation
fix(agc): forget the blocks kept for the CPU when an upload restamps their units - #15457
Merged
oneandonlydean merged 2 commits intoNov 3, 2026
Merged
Conversation
…ispatches Each check fills a texel the CPU rewrote, lets 2,048 threads of 32 groups run image_atomic_add or image_atomic_umax on it, and reads it back with a second dispatch; it runs with 32- and 64-lane waves. The umax check comes after the add check on the same texel, so its CPU rewrite lands while the add's results are still pending and the storage image keeps the CPU's bytes before the umax dispatch writes it again.
…their units boykopovar#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.
|
The new contended image-atomic test reproduces the stale value on RADV with main's production file and passes with this change. Linux CachyOS (kernel 7.3.1-1-cachyos), GCC 16.2.1 Release, Radeon RX 9060 XT (RADV GFX1200, Mesa 26.2.5, Vulkan 1.4.354), Khronos validation layer 1.4.363. Local merge of head
Reproductioncmake --build build-main --target agc_driver_image_atomics_tests -j 4
export ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1
export VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation VK_LOADER_DEBUG=layer
export VK_LAYER_PATH="$HOME/.local/share/anyps5-tester/vk-layers/usr/share/vulkan/explicit_layer.d"
ctest --test-dir build-main -R '^agc_driver_image_atomics$' --repeat until-fail:3 -VFor the counter-test, the new fixture is retained while only the production file is restored to main; run the command three separate times to retain all expected failures, then restore the candidate file. No game or console behavior was tested. AI-assisted independent hardware test run (GPT-6.1 Terra medium). |
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)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 517 tests)Skipped (58)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
✅ 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 (2, not counted against this PR)Validation IDs already present on main (4 IDs in 517 tests)Skipped (48)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. |
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
StorageTexture::upload now drops the kept-for-CPU blocks of each unit it restamps. The new contended image-atomic checks pass on lavapipe with this change and fail with main's Texture.cpp.
What I ran
Head 858915127445f9dd183a34192d84193665f0da89, main 4f41f4c375f5abb487dbd184fbc150f31994d762, merged with --no-ff (tree 2222ab4d9ef92600bd34325af6b0d1a54647cb68). Machine: Debian 13 VM, 2× Xeon Gold 6136 (32 vCPU), GCC 14.2.0, Release, Vulkan through lavapipe (Mesa 26.1.6, LLVM 19).
git diff --check 4f41f4c375f5abb487dbd184fbc150f31994d762...858915127445f9dd183a34192d84193665f0da89
python3 tools/check_conventions.py --base 4f41f4c375f5abb487dbd184fbc150f31994d762
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn ctest --test-dir build -V -R '^agc_driver_image_atomics$'
# negative control
git show 4f41f4c375f5abb487dbd184fbc150f31994d762:core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
ninja -C build -j32 -l32 libs
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build --output-on-failure -R '^agc_driver_image_atomics$'
git show 2222ab4d9ef92600bd34325af6b0d1a54647cb68:core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
ninja -C build -j32 -l32 libs
# 3 runs
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build --output-on-failure -R '^agc_driver_image_atomics'
git status --porcelain
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
Configure / build / libs (script) |
ok |
| Selected tests (script, 2,146 run) | 2,025 passed, 114 expected skips, 0 new failures, 0 timeouts. The 7 known failures also fail on main: agc_driver_division, the denormal-mode tests, unwrap_self_…, vector_constant_factor_tininess |
| Validation layer comparison (script, 770 tests) | Layer loaded in 769 tests and missing in none; 0 tests with new VUID- |
agc_driver_image_atomics (merged) |
passed in 0.56–0.63 s |
agc_driver_image_atomics{,64,_bitwise,_float} ×3 (merged) |
12/12 passed, each under 1 s |
| Worktree after restore | git status --porcelain empty |
Merged run, tail of -V output
[gpu] storage image 0x55a9b0469000: guest memory changed while GPU results were pending; keeping the CPU's blocks
[src/Texture.cpp:3188 upload] StorageTexture upload submit
[src/Texture.cpp:3190 upload] StorageTexture upload done
[gpu] storage image 0x55a9b0469000 destroyed with GPU results pending
[gpu] storage image 0x55a9b046e000 destroyed with GPU results pending
image atomics tests passed
1/1 Test #1224: agc_driver_image_atomics ......... Passed 0.63 sec
Negative control
I put back main's core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp, which is the PR's only production file, and kept the new test. After rebuilding, agc_driver_image_atomics failed with the same message the description quotes:
AGC graphics: image_atomic_umax contended by 2048 wave32 threads on one texel: the texel holds 5, expected 4293705069
This is lavapipe evidence for the macOS/MoltenVK and RADV results in the description and in RNK123's counter-test. After I restored the merged file and rebuilt, the test passed in three runs, and the worktree matched the merged tree again.
Findings
Blocking
None.
Non-blocking
None.
I also checked the fix against the code around it:
forgetKeptAtUnwatch(layer)runs instampLayersbefore the unit gets its new generation (Texture.cpp#L2916-L2919).stampLayershas four callers: fast clear, the import path (whole and partial), the selected CPU layers and the whole CPU upload. In each one the restamped units really take the guest bytes or the guest's clear. A record that survived the upload could therefore only be wrong.- On watched memory,
keptForCpu(L4798-L4805) compares the recorded generation with the new one, so the record already ended there and nothing changes. On unwatched memory the generation stays 0, so before this change the record never ended. That matches the description. - The write-back records its kept blocks after the upload's restamp (L6603-L6606), so the new forget cannot erase a record made by the same write-back.
- The existing debt entry in
docs/dev/debt/libSceAgcDriver.mdalready says "A block a write-back kept for the CPU stays the CPU's until its unit is uploaded again". The change makes that statement true outside the write watch too, so the entry needs no edit. The PR adds no code comments. - Checklist lines are present: Depends on, Fixes, Unblocks, GPUs tested (Apple M3 Max / MoltenVK) and AI-assisted. The new checks finish well under the 30 s limit.
Not verified
- I did not run the change on any hardware GPU. RNK123 (RADV GFX1200) and TechLevelZero (RADV NAVI24, NVIDIA RTX 2000 Ada) report runs with no new failures.
- I ran the wave64 checks only on the merged tree. With
main's file the test stops at the first wave32 umax failure, so I did not see the wave64 result onmain. - I did not test any title.
Contributor
Author
|
Thanks @XairRrR, @RNK123 and @TechLevelZero. The negative controls on lavapipe and RADV match what I saw on MoltenVK: with |
12 tasks done
Contributor
|
Data point from Linux / RADV, with a second upstream test that shows this.
With and the last case flushes the same line for the same image and size, then fails with "store over untracked: the first result was not stored": block 0 stays the CPU's at generation 0 and does not take the stored value, as the PR describes for memory outside the write watch. With the PR merged, the earlier case still keeps the block at its own end, and the last case's flushes of that image carry no "keeps" line. Radeon RX 9070 XT (RADV GFX1201), Mesa 26.2.4, Linux 7.2.10 (Arch-based), GCC 16.2.1, Release build with AI-assisted: yes (Claude Code) |
Contributor
Author
|
Thanks @SleglJan, that is a second way in. The last case of |
Contributor
Author
|
A macOS data point for Apple M3 Max, MoltenVK 1.4.2 from Vulkan SDK 1.4.363.0, macOS 27.0.1, Release, With |
12 tasks done
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
Since #14302, when a storage write-back keeps blocks of a unit for the CPU, it records them with the unit's generation. While that record holds, new GPU results leave the unit's pending-since as it was, so a CPU store made before them still wins at the next write-back. The record is meant to end at the unit's next re-upload, which gives the unit a new generation. On memory outside the write watch the generation is 0 before and after the upload, so the record never ends: every later result of the unit keeps the first pending-since, the CPU store that the upload just took counts as newer than those results, and the next write-back drops them for the CPU's bytes.
agc_driver_image_atomicsgains contention checks that show it. 2,048 threads in 32 groups runimage_atomic_add, thenimage_atomic_umax, on one texel of an 8x8 R32_UINT image in a global array outside the write watch, each check followed by a read-back dispatch, with 32- and 64-lane waves. The umax check rewrites the texel while the add's results are pending; the umax dispatch keeps the CPU's bytes and re-uploads them, and onmainthe read-back dispatch keeps them again, so the texel reads back its initial 5 instead of the largest stored value.StorageTexture::uploadnow forgets the kept blocks of each unit it restamps, since the image takes the guest bytes there. On watched memory nothing changes: the new generation already ended the record.Tested
macOS 27.0.1, Apple M3 Max (36 GB), MoltenVK 1.4.2 from Vulkan SDK 1.4.363.0, Release build with
-DBUILD_TESTING=ON.maindoes not run its libraries on macOS, so both sides aremain4f41f4c37with the macOS port (the squash of #8658, which its author upstreams in parts) and one local build fix for #12919's test (areinterpret_castfromconst void*that Clang refuses). Both sides carry the new test; the change side adds the fix.agc_driver_image_atomics: without the fix the umax check fails with "image_atomic_umax contended by 2048 wave32 threads on one texel: the texel holds 5, expected 4293705069" (2 runs of 2); with it the test passes (3 runs of 3).VK_LAYER_KHRONOS_validation,ANYPS5_REQUIRE_VULKAN=1andANYPS5_NO_SHADER_CACHE=1,ctest --test-dir build-macos -R '<pattern>' -Voveragc_driver_image_atomics,agc_driver_image_atomics_bitwise,agc_driver_image_atomics_float,agc_storage_*,agc_unit_*,agc_shared_write*,agc_write_tracking,agc_driver_storage_*andagc_driver_recorder(36 tests): neither log reports the layer missing.agc_driver_image_atomicsgoes from failed to passed and every other test keeps its status: 34 pass, andagc_unit_flags_allocation_failure_populatedfails on both sides (libc++'svector::assignfrees the old array before it allocates the new one). Each test has the sameVUID-errors on both sides, 19 in all,VUID-VkDeviceCreateInfo-pProperties-04451from MoltenVK's portability subset.python3 tools/check_conventions.py --base origin/mainpasses.tools/pr_overlap.pycould not compare 38 open pull requests that conflict withmainand shareTexture.cpporImageAtomics.cpp. Reading their diffs, eight mention the kept-block record (feat(agc): trace ranges that leave the write watch #7604, feat(agc): look up table images at run time through a device image table on the GPU heap (APS5_GPU_PATH) #7642, fix(agc): preserve CPU writes after resident linear target draws #8246, perf(agc): expand single-key DCC storage uploads on the GPU from the host import #8484, feat(agc): serve lane-keyed, pointer and runtime image-table columns #10193, fix(agc): compare pending storage results over a surface whose page walk fails #12792, feat(agc): resolve table samplers through device image keys #13928, feat(agc): resolve runtime sampler addresses through device image keys #13929), none to change when it ends; feat(agc): look up table images at run time through a device image table on the GPU heap (APS5_GPU_PATH) #7642, perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508, feat(agc): serve lane-keyed, pointer and runtime image-table columns #10193, feat(agc): resolve table samplers through device image keys #13928 and feat(agc): resolve runtime sampler addresses through device image keys #13929 also add lines toImageAtomics.cpp.Not tested: Linux and Windows GPUs, where the new checks run on native 32-bit image atomics through the same storage image path, and any title.
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)Context
Found while rebasing our macOS branch onto
main: our local emulation of 64-bit image atomics on MoltenVK has the same contention checks inagc_driver_image_atomics64, and they started failing after #14302.