Repository navigation
fix(agc): keep a settled import compared while a storage image overlaps it - #7476
oneandonlydean merged 2 commits into
Conversation
…ps it Since boykopovar#6678, a Windows host import under 1 MiB returns to the write watch once its deferred settle read has completed, and GuestMemory::Rewatch stamps every block of it as a CPU store. The return waited only while storage results were pending over the import. A storage image can take its generation before the stamp and have results pending only after it: an image's first upload takes its generation and then imports its block, a recorded draw marks its targets pending once it is recorded, and the settle is polled at the import lookups between the two. The image's next refresh then takes the stamp for a CPU store over its results, drops them and uploads the guest bytes again, so the GPU's output is lost. On an RTX 3070 this also splits the first two draws of agc_driver_address_passes into two passes. A settled import that a live storage image overlaps now stays compared, as every import under 1 MiB was before boykopovar#6678. Imports with no image over them still return to the write watch. Keeping the import compared, rather than waiting for the image to go, checks each settled import against the live images once instead of at every poll. The [buffers] line counts the imports kept compared. agc_import_settle_storage_image makes an image whose first upload imports its block, waits for the settle read, clears the image on the GPU, then refreshes and stores it. On the old code the refresh drops the clear.
ARCHITECTURE.md and the libSceAgcDriver debt entry now say that a settled Windows import under 1 MiB returns to the write watch only when no storage image overlaps it, and why.
|
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.
|
|
I am preparing a Windows/RTX 5070 Ti (617.42) cross-test of 3ea0be6 on our pinned 007 integration c872fab (base main e5c2b3c plus the public shared-table/resource stack). Current title logs contain the pending-GPU-results/CPU-blocks diagnostic, but I am not attributing each occurrence to this race without the synthetic check. I will first run the new regression and the import/watch/address-pass controls, then report title rendering and counters separately. No duplicate fix from me. A separate opt-in wave64 trial is active now, so no builds or GPU tests will overlap it. AI-assisted: Codex. |
|
Windows cross-test of #7476 head 3ea0be6 on integration 8e0bd2c (pushed codex/first-light-oct18-current), RTX 5070 Ti / NVIDIA 617.42, Windows 11 Pro 26200: all 14 targeted import/watch/address-pass controls passed, 7.51 s, zero skips, ANYPS5_REQUIRE_VULKAN=1. With only rewatchSettledImports restored to the integration parent, the unchanged new agc_import_settle_storage_image fails in 0.42 s: 1 back in the watch / 0 kept compared, 16384 of 16384 texels lost. The restored fix then passes the broader 65-control set in 28.84 s, zero skips, including the new regression, shader memory, image tables, relocation, storage ordering, depth dispatch and HDR. Qualification: my first post-negative restore preserved an older file timestamp, so Ninja reused the negative object and that intermediate run still failed. I corrected the timestamp, verified GuestBufferMemory.cpp was compiled again, and obtained the final 65/65 result above. This was a local build-state error, not a claimed source regression. No game was running during builds/native checks. Source merged cleanly; two documentation conflicts were resolved preserving our existing draw-input/store-range notes and this PR's live-image-overlap wording. Matched patched modules are now staged for the title cross-test; no title rendering/FPS result yet. Prior baseline and wave trial each contain eight pending-results/CPU-blocks diagnostics, but I have not attributed those occurrences to this race. Raw title data stays local. AI-assisted: Codex. |
|
Cross-test of 3ea0be6's code changes (docs left out, they conflict on main 9caecfd) on older NVIDIA GPUs and on Intel. Windows 11, WinLibs GCC 15.2.0 Release,
On these Turing GPUs the failure is intermittent, with the same two lines and the pending-results/CPU-blocks line before them, so a 3-run check can pass by chance; with this PR it passed every run. AI-assisted: Claude. |
|
@jacob6769 thank you for 40 runs per cell on three GPUs: that's what shows an intermittent failure a 3-run check can miss, and it's now in the description. One note for anyone applying it the same way: the PR merges cleanly as a branch, and only its debt-file hunk fails as a patch on |
4 participants
What
A regression from our #6678. On Windows, a storage image can lose its GPU results when the 64 KiB block under it returns to the write watch, not only split a render pass.
How it happens (
main3de84fa):GuestMemory::Rewatchstamps every block as a CPU store (GuestMemory.cpp:1325-1340).StorageTexture::AnyPendingOverlaps,GuestBufferMemory.cpp:1064).Texture.cpp:2176) and then imports its block (Texture.cpp:2253). A recorded draw marks its targets pending only once it is recorded (Draw.cpp:1574), and the settle is polled at the import lookups in between (GuestBufferMemory.cpp:1396,3004). So the stamp can land after the image's generation and before its results are pending.Texture.cpp:1927-1934), drops them (1963-1965) and uploads the guest bytes again (1997). It prints:Where it showed: thuanquang bisected
agc_driver_address_passesfailing on an RX 6700 XT to #6678 (#7414). On an RTX 3070,main3de84fa fails it 3 of 3 runs, "2 address-based draws, 2 began a pass, 0 continued one", with the line above, and passes 3 of 3 withAPS5_NO_DEFERRED_SETTLES=1. In that test both draws render the same pixels, so only the pass count shows the loss.The change:
StorageTexture::AnyLiveOverlaps). Imports with no image over them still return to the write watch.[buffers]profile line counts the imports kept compared.ARCHITECTURE.mdand thelibSceAgcDriverdebt entry say so.What it leaves as it is:
CollectWritesreturns 0 outside the watch, so the drop rule never applied to them.Rewatchdoes nothing.Related: #7414
Tested
An RTX 3070 Laptop GPU, Windows 11, NVIDIA 617.42 (
VK_LOADER_DRIVERS_SELECT='*nv*', one device):rewatchSettledImportsback as onmain(the test,AnyLiveOverlapsand the counter kept),agc_import_settle_storage_imagefails 3 of 3 runs: "(i) the refresh after the image's GPU results dropped 16384 of 16384 texels: the import's return to the write watch stamped its block as a CPU store after the image took its generation".agc_driver_address_passesandagc_driver_address_passes_targets_split, perf(agc): settle Windows imports under 1 MiB without waiting #6678'sagc_import_settle_deferred,agc_import_settle_deferred_other_family,agc_import_settle_first_useandagc_import_settle_retire,agc_import_watch,agc_import_watch_other_family,agc_write_tracking,agc_storage_cpu_snapshot,agc_host_import_retireandagc_driver_host_import_windows.agc_driver_address_passespasses with deferred settles on.An RTX 5070 Ti, Windows 11 Pro 26200, NVIDIA 617.42, run by Scramblrrrr on their integration tree 8e0bd2c with this PR's 3ea0be6 merged in,
ANYPS5_REQUIRE_VULKAN=1(comment):rewatchSettledImportsback as on their tree,agc_import_settle_storage_imagefails: 1 import back in the watch, 0 kept compared, 16384 of 16384 texels lost;NVIDIA GeForce RTX 2060 and GTX 1650 (driver 32.0.16.1088) and Intel UHD Graphics 770 (32.0.101.6129), Windows 11, run by jacob6769 with this PR's code changes on
main9caecfd (its docs left out), WinLibs GCC 15.2.0, Release,ANYPS5_REQUIRE_VULKAN=1(comment).agc_driver_address_passes, 40 runs per cell:mainmain+ test(agc): measure address passes over settled host imports #7414main+ this PRagc_import_settle_storage_imagepasses 10 of 10 on both, and the tenagc_import_*and address-pass tests pass on each.agc_import_settle_*test skips, the new one included.GitHub's Linux (lavapipe, Mesa 25.2.8) and Windows runners (verify run): both build, and libc and libkernel pass on both. The new test is registered only on Windows, where GitHub's runner has no Vulkan device, so it skips there.
Windows, the full CI build (fork Build): 1469 of 1469 tests pass, the new one skipping, and
check_skipspasses.Linux, a run that mirrors CI's Linux job (a cloud machine, Ubuntu 24.04, GCC 13.3, Mesa lavapipe 25.2.8 with LLVM 20.1.2):
main3de84fa and this branch have the same 1,437 tests, with no status changes. The 5 failures on both are that machine's IPv6 tests. One of the branch's two full runs also failedguest_avplayer, which doesn't link the AGC driver; it passed in the other run and in 20 of 20 reruns.agc_import_watch,agc_driver_address_passesandagc_driver_address_passes_targets_splitpass 20 of 20 runs each.New test,
agc_import_settle_storage_image(Windows with a GPU): an image whose first upload imports its 64 KiB block waits for the import's settle read, is cleared to red on the GPU, then refreshed and stored. Every texel must be red, and the import must have stayed compared. On the old code the refresh drops the clear.Not tested: a title; AMD hardware; the settle path on Intel, where its tests skip.
Checklist
main3de84fa; no other open PR implements the same functions. Overlaps (git merge-treewith each PR merged intomaincb1d6e5), both in docs, in adjacent lines where both sides stay:docs/dev/ARCHITECTURE.md: perf(agc): copy draw inputs on NVIDIA's Windows driver instead of binding host imports in place #6763 (draw inputs copied on NVIDIA's Windows driver instead of bound in place from a host import) adds a bullet just before the write-watch bullet this PR extends.docs/dev/debt/libSceAgcDriver.md: fix(agc): save pending results when a fresh write collect fails #7432 (pending results saved when a fresh write collect fails) edits the "Pending storage-image results over memory outside the write watch" entry, the line before the host-pointer imports entry this PR edits.NotImplemented_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)