Repository navigation
fix(agc): hash an idle unit when its range leaves the write watch instead of at every upload - #10026
Conversation
…surface that is no longer fully write-watched markLayersPending saved the guest bytes of a newly pending unit only when it had no write generation. A unit that was last stored or uploaded while its memory was watched keeps a generation, so when a block of the surface left the write watch while the unit was idle (a flush for the access that caused the unwatch had just stored it, so the unwatch hook found it not pending) and the unit became pending again afterwards, it had neither the watch nor saved bytes to tell the CPU's writes from its results. Its write-back then refused: storing storage image results pending over memory outside the write watch without the guest bytes saved before them is not implemented. One such unit makes the whole span untracked, so every unit of the write-back needs its bytes. A unit without saved bytes and without a save on its way now takes its guest bytes when it becomes pending over a surface that is not fully watched, whatever its generation. The blocks the CPU or the driver wrote since the unit's generation are still told from the stamps while the unit's own blocks are watched and keep their bytes at the store, exactly as when the hook saves a pending unit (keepWrittenSince, shared with saveBeforeUnwatch); their writes are collected unit by unit, since a collect over a range holding an unwatched block does nothing. A unit whose own block is unwatched gets its bytes alone.
… is write-watched A write-back routes each kept range that is wholly watched into the unit shadow's slab, and marks the pieces shadowed at the generation its collect over the whole stored span returns. When a block of the span has left the watch that collect returns 0, MarkShadowed ignores a generation of 0, and the pieces stay in the slab without a record: never published, their results lost. Over a span with an unwatched block every piece now goes to the import, as the saved bytes of the store already are decided by the span.
…urface left the write watch A storage image over write-watched memory is uploaded, so its units have a write generation and no saved guest bytes. A block of the surface then leaves the write watch while nothing is pending, which the unwatch hook does not see, and the image is marked written again without a lookup (as the colour mirror of a depth plane is after each dispatch). The write-back cannot tell the CPU's writes from the results without saved bytes and refuses. agc_storage_pending_after_unwatch runs that flow with no CPU write, with the CPU writing a block of the surface after the results became pending again and with it writing before; the store has to succeed, keep the CPU's block and store the rest, the block that left the watch included. Where the image's host import takes the surface out of the write watch (a Windows runner), the test reports that it was not run.
…ay differ from its upload A unit uploaded while watched has no saved bytes. Once its block left the write watch the CPU's writes there are unstamped, and capturing the current bytes as the baseline when the results turn pending without a lookup stored the stale upload content over them for every texel the GPU did not write. Capture only when the unit is still watched or equals the bytes it was uploaded from; otherwise keep the refusal for that unit.
… its upload bytes Surfaces above 1 MiB keep no byte snapshot of their upload, so an unwatched unit with no saved bytes fell back to the unsaved-outside-watch refusal even when the CPU never wrote there. Keep a 128-bit hash of each unit's bytes taken when the unit was uploaded and compare it at markLayersPending: equal means the current bytes are the upload and are saved as the baseline, a different hash or none keeps the refusal. Units whose content came from elsewhere (borrowed from an alias, cleared, stored by a write-back) drop their hash. The pending-after-unwatch test also runs a 1024x1024 surface: an unwatched unit the CPU did not write is stored, one it wrote is refused with its bytes intact.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the stacked diff through d613893, including the declared dependency #7048. The follow-up replaces the per-upload hash with a hash captured at a proven write-watch transition, gates it by the write generation, and invalidates it on writes/refreshes; the focused tests cover the tested import-walk failure paths and pending-result behavior. The PR reports same-machine Pathless measurements in which #7048 alone regresses from 319–330 to 85–104 presents/10s and the combined stack returns to 304–316; those are the author’s Windows captures, not a local reproduction. Local diff whitespace, conventions, and test-registration checks pass. I could not run the focused AGC executable here: the available pre-existing binary aborts before test startup because this host lacks LZCNT, BMI1, BMI2, MOVBE, and AVX2. Hosted product jobs are skipped by the current workflow policy. Please preserve the dependency/merge order so #7048 is not delivered without its performance correction.
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed exact head d6138938c99c24a21b6dce947cf6cb31e0d9a89e. The change is correctly declared as depending on open #7048, and its author-provided test/performance report is unusually detailed. Local git diff --check and python3 tools/check_conventions.py --base upstream/main pass. The hosted eligibility, intake, conventions, labels, and report checks pass; hosted Linux and Windows product builds are skipped by repository policy. I could not run this Windows/GPU test suite locally.
I am requesting changes because the branch no longer merges cleanly into current main. python3 tools/check_main_sync.py --base upstream/main reports 9 commits ahead / 1,079 behind and a merge conflict in core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp; it also identifies eight changed files modified on both sides since the merge base. The repository's build guidance requires current-main builds and a clean merge before review/merge, so the reported tests and performance comparison against 83462cc57 do not validate the current merge result.
Please rebase onto current main, resolve the Texture.cpp conflict while preserving intervening changes, then rerun the focused write-watch/storage-image tests and the same-scene main versus rebased-PR comparison. Please include the exact tested heads and refresh the trace summaries/digests if the rebase changes the tested result. This request is for the concrete merge conflict and stale validation base; it is not about queue size.
Resolves boykopovar#7048's conflicts with main in Texture.cpp: markLayersPending keeps main's CommitTrims structure with boykopovar#7048's baseline condition, and the generation baseline constant stays where boykopovar#7048 moved it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tead of at every upload Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d613893 to
ec6ceec
Compare
Adapts this branch to main's TrackerLock (boykopovar#6169): markWritten and ImportWatched release the tracker around beforeUnwatchUnproven with Suspend and Resume, which do not publish the generation, since both stamp again after relocking. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Rebase detail for The hook needs the non-publishing pair: lock.Suspend();
beforeUnwatchUnproven(begin, static_cast<std::size_t>(end - begin));
lock.Resume();Keep I built and tested this adaptation on Windows 11 Pro 10.0.26300, RTX 3060 Laptop GPU, NVIDIA 617.42, WinLibs GCC 15.2.0 r7:
This is native integration evidence only. The current GTA III baseline run ended before the first mission, so I do not yet have a valid title or performance comparison for these heads. Linux and other GPU vendors are untested here. Prepared with AI assistance. |
|
@winstrex Persona 3 Reload (PPSA10872) on this head, on the scene where I measured #7048's slowdown (the start-up screens and logos) and the opening animation after it. Setup: Windows 11 Pro 26200, RTX 5090 (NVIDIA 617.42), Ryzen 7 9800X3D, WinLibs GCC 15.2.0 r7, Release, 3840x2160, default settings. Three builds that differ only in Every run gets the same scripted pad input: Cross at 40 s and 55 s for the online-features dialog, nothing else. Presents per 10 s (600 is the 60 FPS cap), one value per run:
So on this title the PR brings presents back to The refusal #7048 removes came back once with this PR. In 1 of 5 runs (the warm-up; the shader cache was already warm from the That's the shader that stops Not this PR: every run that didn't refuse, in all three builds, stops at about 185 s in the opening animation on Left out: one run of this PR (10026-1), where the launcher's console window covered the game; its start-up intervals were 519–539. A first The summaries and digests of every run: p3r-10026-summaries.zip Not covered: other GPUs, Linux, anything past the opening animation, enough runs for a refusal rate.
|
| # | seconds | presents | p50 | p90 | p99 | max | GPU busy | busiest worker | largest phase |
|---|---|---|---|---|---|---|---|---|---|
| 1 | 0-10 | 383 | 16.7 | 24.4 | 151.7 | 1589.3 | 4.7 | 69 % | Driver.Packet 32.0 |
| 2 | 10-20 | 600 | 16.7 | 16.8 | 16.9 | 17.1 | 3.7 | 90 % | Driver.Packet 20.1 |
| 3 | 20-30 | 600 | 16.7 | 16.9 | 17.3 | 29.0 | 3.9 | 74 % | Driver.Packet 18.3 |
| 4 | 30-40 | 600 | 16.7 | 16.8 | 16.9 | 17.3 | 3.9 | 97 % | Driver.Packet 23.9 |
| 5 | 40-50 | 584 | 16.7 | 16.9 | 26.9 | 107.4 | 4.1 | 97 % | Driver.Packet 32.3 |
| 6 | 50-60 | 434 | 16.7 | 25.6 | 149.9 | 157.3 | 5.1 | 90 % | Driver.Packet 34.9 |
| 7 | 60-70 | 600 | 16.7 | 16.9 | 18.3 | 24.1 | 5.6 | 88 % | Driver.Packet 23.0 |
| 8 | 70-80 | 600 | 16.7 | 16.9 | 17.0 | 18.1 | 4.3 | 88 % | Driver.Packet 20.5 |
| 9 | 80-90 | 600 | 16.7 | 16.9 | 17.0 | 17.3 | 4.2 | 99 % | Driver.Packet 30.0 |
| 10 | 90-100 | 600 | 16.7 | 16.9 | 17.0 | 17.3 | 4.2 | 100 % | Driver.Packet 33.1 |
| 11 | 100-110 | 600 | 16.7 | 16.9 | 16.9 | 17.0 | 4.2 | 100 % | Driver.Packet 33.1 |
| 12 | 110-120 | 600 | 16.7 | 16.9 | 16.9 | 17.3 | 4.2 | 100 % | Driver.Packet 33.1 |
| 13 | 120-130 | 600 | 16.7 | 16.9 | 17.0 | 17.3 | 4.2 | 100 % | Driver.Packet 33.1 |
| 14 | 130-140 | 600 | 16.7 | 16.9 | 16.9 | 17.0 | 4.2 | 100 % | Driver.Packet 33.1 |
| 15 | 140-150 | 600 | 16.7 | 16.9 | 16.9 | 17.0 | 4.2 | 100 % | Driver.Packet 33.1 |
| 16 | 150-160 | 600 | 16.7 | 16.9 | 16.9 | 17.0 | 4.2 | 100 % | Driver.Packet 33.1 |
| 17 | 160-170 | 600 | 16.7 | 16.9 | 17.0 | 17.0 | 4.2 | 100 % | Driver.Packet 33.1 |
| 18 | 170-180 | 600 | 16.7 | 16.9 | 16.9 | 17.0 | 4.2 | 100 % | Driver.Packet 33.1 |
p99 ms @___:@____________ 0-151.7
gpu ms %***#%@########### 0-5.6
main 9d164f0 + #7048 (93c878b merged locally)
18 intervals, 0.0 to 180.9 s, 2073 presents
NVIDIA GeForce RTX 5090, NVIDIA 617.42, Windows 10.0.26200, 3840x2160, commit 9d164f0 (dirty)
| # | seconds | presents | p50 | p90 | p99 | max | GPU busy | busiest worker | largest phase |
|---|---|---|---|---|---|---|---|---|---|
| 1 | 0-10 | 77 | 88.2 | 185.9 | 1852.8 | 1852.8 | 4.6 | 87 % | Driver.Packet 226.0 |
| 2 | 10-20 | 127 | 78.3 | 81.0 | 98.8 | 100.0 | 3.5 | 100 % | Driver.Packet 157.7 |
| 3 | 20-30 | 126 | 79.4 | 82.1 | 89.4 | 89.6 | 3.5 | 100 % | Driver.Packet 159.0 |
| 4 | 30-40 | 118 | 84.5 | 89.0 | 93.7 | 95.8 | 9.9 | 100 % | Driver.Packet 170.1 |
| 5 | 40-50 | 114 | 85.1 | 89.5 | 187.8 | 189.3 | 10.6 | 100 % | Driver.Packet 175.6 |
| 6 | 50-60 | 89 | 92.5 | 199.4 | 257.3 | 257.3 | 10.7 | 99 % | Driver.Packet 218.6 |
| 7 | 60-70 | 112 | 86.2 | 91.3 | 211.2 | 225.7 | 10.9 | 99 % | Driver.Packet 176.5 |
| 8 | 70-80 | 119 | 85.8 | 89.2 | 91.6 | 91.8 | 10.3 | 100 % | Driver.Packet 168.8 |
| 9 | 80-90 | 117 | 86.9 | 89.0 | 91.9 | 93.9 | 12.3 | 100 % | Driver.Packet 171.5 |
| 10 | 90-100 | 118 | 86.2 | 88.8 | 90.9 | 92.5 | 11.2 | 100 % | Driver.Packet 169.4 |
| 11 | 100-111 | 120 | 85.5 | 88.7 | 90.2 | 90.5 | 9.6 | 100 % | Driver.Packet 167.5 |
| 12 | 111-121 | 121 | 81.7 | 88.4 | 91.1 | 92.6 | 8.5 | 100 % | Driver.Packet 165.6 |
| 13 | 121-131 | 119 | 85.6 | 88.8 | 90.8 | 91.4 | 10.8 | 100 % | Driver.Packet 168.8 |
| 14 | 131-141 | 120 | 85.2 | 87.8 | 90.5 | 91.5 | 9.3 | 100 % | Driver.Packet 166.9 |
| 15 | 141-151 | 120 | 85.1 | 88.8 | 91.2 | 91.9 | 9.9 | 100 % | Driver.Packet 167.3 |
| 16 | 151-161 | 119 | 85.6 | 89.0 | 90.4 | 91.3 | 9.8 | 100 % | Driver.Packet 168.0 |
| 17 | 161-171 | 118 | 86.3 | 89.0 | 91.1 | 92.1 | 10.6 | 100 % | Driver.Packet 169.6 |
| 18 | 171-181 | 119 | 85.6 | 89.2 | 92.3 | 92.4 | 10.3 | 100 % | Driver.Packet 167.9 |
p99 ms @...___........... 0-1852.8
gpu ms ---#%%%%@%#*%###%% 0-12.3
this PR, df1b2d5
18 intervals, 0.0 to 180.2 s, 10419 presents
NVIDIA GeForce RTX 5090, NVIDIA 617.42, Windows 10.0.26200, 3840x2160, commit df1b2d5
| # | seconds | presents | p50 | p90 | p99 | max | GPU busy | busiest worker | largest phase |
|---|---|---|---|---|---|---|---|---|---|
| 1 | 0-10 | 392 | 16.7 | 27.5 | 164.8 | 1596.0 | 6.4 | 78 % | Driver.Packet 35.9 |
| 2 | 10-20 | 599 | 16.7 | 16.9 | 17.7 | 25.4 | 4.2 | 97 % | Driver.Packet 26.8 |
| 3 | 20-30 | 598 | 16.7 | 16.9 | 21.7 | 42.2 | 6.0 | 88 % | Driver.Packet 24.9 |
| 4 | 30-40 | 600 | 16.7 | 16.9 | 18.8 | 23.7 | 6.4 | 77 % | Driver.Packet 21.0 |
| 5 | 40-50 | 588 | 16.7 | 16.9 | 17.1 | 109.1 | 5.9 | 90 % | Driver.Packet 23.4 |
| 6 | 50-60 | 442 | 16.7 | 23.7 | 139.9 | 189.4 | 5.7 | 81 % | Driver.Packet 31.3 |
| 7 | 60-70 | 600 | 16.7 | 16.9 | 17.0 | 22.0 | 3.9 | 69 % | VideoOut.Flip 16.4 |
| 8 | 70-80 | 600 | 16.7 | 16.9 | 17.1 | 17.6 | 4.0 | 99 % | Driver.Packet 26.1 |
| 9 | 80-90 | 600 | 16.7 | 16.9 | 17.0 | 17.3 | 4.0 | 99 % | Driver.Packet 32.9 |
| 10 | 90-100 | 600 | 16.7 | 16.9 | 17.0 | 17.3 | 4.0 | 100 % | Driver.Packet 33.1 |
| 11 | 100-110 | 600 | 16.7 | 16.9 | 17.1 | 17.3 | 3.9 | 100 % | Driver.Packet 33.1 |
| 12 | 110-120 | 600 | 16.7 | 16.9 | 17.0 | 17.2 | 3.9 | 100 % | Driver.Packet 33.1 |
| 13 | 120-130 | 600 | 16.7 | 16.9 | 16.9 | 17.0 | 4.0 | 100 % | Driver.Packet 33.1 |
| 14 | 130-140 | 600 | 16.7 | 16.9 | 16.9 | 17.0 | 3.9 | 100 % | Driver.Packet 33.1 |
| 15 | 140-150 | 600 | 16.7 | 16.9 | 17.0 | 17.2 | 4.0 | 100 % | Driver.Packet 33.1 |
| 16 | 150-160 | 600 | 16.7 | 16.9 | 17.0 | 17.2 | 4.0 | 100 % | Driver.Packet 33.1 |
| 17 | 160-170 | 600 | 16.7 | 16.9 | 16.9 | 17.3 | 4.0 | 100 % | Driver.Packet 33.1 |
| 18 | 170-180 | 600 | 16.7 | 16.9 | 16.9 | 17.3 | 4.0 | 100 % | Driver.Packet 33.1 |
p99 ms @____%____________ 0-164.8
gpu ms @*%@%%************ 0-6.4
…ytes The refusal of results pending over memory outside the write watch without the guest bytes saved before them now names the first unit refused and why it has no baseline: its results became pending while watched and the range left the watch without its bytes saved, a save in flight did not complete, its saved bytes were dropped, no upload copy, bytes that differ from the upload copy, no hash for its current generation, an unwatch hook that ran without the CPU writes proven, CPU writes the watch saw before the unwatch, a unit that had pending results or saved bytes at the unwatch, or bytes that no longer match the hash. The unwatch hook now also visits the live textures when the CPU writes are not proven, only to record that it could not hash them. What is captured and what is refused does not change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…a baseline On Windows no range leaves the write watch without the unwatch hook: the host regions are fixed when the guest arena is reserved, the commit generation only decides when a later commit restores the watch, and every path that excludes a range runs the hook first. The Linux case, GuestWriteWatchUnregister called from libkernel, stays. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@oneandonlydean done in 660cef6fe. I dropped the three |
✅ NVIDIA GPU test: passedPR head
Failing on main too (4, 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 436 tests)Skipped (39)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. |
…PU lock ImportMappedRanges imports at map time without the GPU lock while holding the imports mutex, and refused only ranges that a pending storage image overlaps, the one case in which the unwatch hook took the GPU lock. Since the hook also hashes live storage images larger than 1 MiB, it took the GPU lock for them under the imports mutex while a GPU lock owner could wait for that mutex in HostImportFor. One predicate, UnwatchHookLocksGpu, now decides both when the hook takes the GPU lock and when the import without it is deferred to the GPU lock first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The current head does not merge cleanly with live main: GuestBufferMemory.cpp and Texture.cpp conflict, so this exact change cannot be validated against the required current-base tree.
What I ran
- PR head:
53c3cd4eee3b15cd6408cd78a8934c9fcfb1a8ec. - Current base:
main420b6a064eadaa7c5e50b080c7fad59c41150b4b. - Merge preview:
git merge-tree --write-tree upstream/main refs/remotes/upstream/pr-10026— conflicts inGuestBufferMemory.cppandTexture.cpp. - Environment: Debian 12 x86-64, GCC 12.2.0, CMake 3.25.1, Ninja 1.11.1; host Xeon E5-1620 v2. QEMU user-mode TCG ran with llvmpipe (LLVM 15.0.6).
- Commands:
cmake -S /dev/shm/AnyPS5-review-10026 -B /dev/shm/AnyPS5-build-10026 -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON
cmake --build /dev/shm/AnyPS5-build-10026 --target agc_driver_recorder_tests -j2
git -C /dev/shm/AnyPS5-review-10026 diff --check cda9c9bedf143010185aa17cf04e221389dd78b5...HEAD
git -C /marc/AnyPS5 merge-tree --write-tree upstream/main refs/remotes/upstream/pr-10026
ctest --test-dir /dev/shm/AnyPS5-build-10026 -R '^agc_unwatch_hook_lock_order$' -V --timeout 120
cd /dev/shm/AnyPS5-build-10026/core/libs/prx/libSceAgcDriver
APS5_NO_PIPELINE_JOURNAL=1 qemu-x86_64 -cpu max /dev/shm/AnyPS5-build-10026/tests/agc_driver_recorder_tests --unwatch-lock-order-onlyResults
| Check | Result |
|---|---|
| Current-main merge preview | Fails with content conflicts in GuestBufferMemory.cpp and Texture.cpp. |
Local configure and agc_driver_recorder_tests build |
Passed; target linked successfully. |
git diff --check against the PR base |
Passed with no output. |
| Hosted exact-head checks | Conventions and labels pass; no product build/test job is present for head 53c3cd4. |
Native agc_unwatch_hook_lock_order CTest |
Could not start the test: the host CPU gate aborts because this Xeon lacks LZCNT, BMI1, BMI2, MOVBE, AVX2 and FMA. |
QEMU --unwatch-lock-order-only run |
Skipped with exit 77: llvmpipe keeps host imports watched and does not provide the write-watch path this test requires. |
Unedited command output
# Final lines from the successful initial build
[955/956] Building CXX object core/libs/prx/libSceAgcDriver/CMakeFiles/agc_driver_recorder_tests.dir/tests/Recorder.cpp.o
[956/956] Linking CXX executable tests/agc_driver_recorder_tests
# git diff --check cda9c9bedf143010185aa17cf04e221389dd78b5...HEAD
(no output; exit 0)
# Current-main merge preview
CONFLICT (content): Merge conflict in core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
CONFLICT (content): Merge conflict in core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
# Native CTest
[src/HostCpu.cpp:193 operator()] host CPU Intel(R) Xeon(R) CPU E5-1620 v2 @ 3.70GHz: has SSE, SSE2, SSE3, SSSE3, SSE4.1, SSE4.2, POPCNT, CMPXCHG16B, LAHF/SAHF, AVX, F16C, AES, PCLMULQDQ, RDRAND, XSAVEOPT, OS AVX state (XCR0); lacks LZCNT, BMI1, BMI2, MOVBE, AVX2, FMA, RDSEED, ADX, SHA, CLFLUSHOPT, CLWB, RDPID, XSAVEC, SSE4A, PREFETCHW, CLZERO
[libc] unhandled exception 'St13runtime_error' thrown from 0x7f64bac4f9b7
terminate called after throwing an instance of 'std::runtime_error'
what(): The host CPU (Intel(R) Xeon(R) CPU E5-1620 v2 @ 3.70GHz) lacks LZCNT, BMI1, BMI2, MOVBE, AVX2, FMA. The PS5's Zen 2 CPU has these extensions and titles run their instructions natively, so this CPU cannot run them.
1/1 Test #618: agc_unwatch_hook_lock_order ......Subprocess aborted***Exception: 0.01 sec
# QEMU user-mode run
[src/HostCpu.cpp:193 operator()] host CPU QEMU TCG CPU version 2.5+: has SSE, SSE2, SSE3, SSSE3, SSE4.1, SSE4.2, POPCNT, LZCNT, BMI1, BMI2, MOVBE, CMPXCHG16B, LAHF/SAHF, AVX, AVX2, FMA, F16C, AES, PCLMULQDQ, RDRAND, ADX, CLFLUSHOPT, CLWB, XSAVEOPT, SSE4A, OS AVX state (XCR0); lacks RDSEED, SHA, RDPID, XSAVEC, PREFETCHW, CLZERO
Vulkan device: llvmpipe (LLVM 15.0.6, 256 bits)
Vulkan device: llvmpipe (LLVM 15.0.6, 256 bits)
host imports stay watched
no write watching: the unwatch hook's lock order at a map-time import not tested
Negative control
| Assertion | Code change made | Did the new test fail? |
|---|---|---|
| A map-time import is deferred before an unwatch hook can acquire the GPU mutex. | None; I did not mutate the code. The test could not execute its write-watch path on this host. | Not exercised. |
Findings
Blocking
- The exact PR head conflicts with current
maininGuestBufferMemory.cppandTexture.cpp. The repository's contribution instructions require rebasing a PR that no longer merges cleanly. Resolve both conflicts while preserving the currentmainchanges and this PR's lock-order fix.
Non-blocking
- I found no separate demonstrated defect in the new predicate from this static review. The new lock-order assertion remains unverified locally because the available host cannot exercise the required unwatch path.
Sources
- Contribution rule: a PR that no longer merges cleanly must be rebased
- AGC test requirements for a same-machine main/change run
- Reviewed commit
Not verified
No actual write-watch/unwatch scenario ran on this host, and no PS5-console behavior was tested. The latest NVIDIA report predates commit 53c3cd4; its results do not validate this new lock-order change. Full Linux/Windows PR builds and tests are not automatic under the repository's current workflow.
To get this to a good state
Rebase onto 420b6a064eadaa7c5e50b080c7fad59c41150b4b, resolve both conflicts, then run the focused agc_unwatch_hook_lock_order test on a supported host where the write-watch path executes. Per AGENTS.md, compare the same focused test on that host's main and updated head with Vulkan validation enabled. I will rerun the merge preview, conventions, and available focused checks on the new head.
…the imports mutex The host imports mutex now notes on its thread that it is held, and the unwatch hook aborts before it takes GuestMemory::GpuMutex if this thread holds the imports mutex without it. The check is always on in builds without NDEBUG and, in release builds, only with APS5_ASSERT_GPU_LOCK, which agc_unwatch_hook_lock_order now sets, so that test fails at the inverted acquisition itself instead of after its watchdog. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes requested
The new lock-order assertion is present, but head 8b14bde3ca1c256413582b8ec2d7d88889d560cd still conflicts with current main in GuestBufferMemory.cpp and Texture.cpp, and the available GPU report predates this head.
What I ran
Reviewed head 8b14bde3ca1c256413582b8ec2d7d88889d560cd against PR base cda9c9bedf143010185aa17cf04e221389dd78b5; current main is dc38224141e0b6096007b876b4c42fc9144d11b8. The merge preview is conflicted. Local checks ran in Linux; no compiler or build was invoked.
git merge-tree --write-tree refs/remotes/upstream/main refs/review/pr-10026-current
git diff --check cda9c9bedf143010185aa17cf04e221389dd78b5..refs/review/pr-10026-current
python3 tools/check_conventions.py --base cda9c9bedf143010185aa17cf04e221389dd78b5 --head refs/review/pr-10026-current
gh pr checks 10026 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Current-main merge preview | Conflicts in GuestBufferMemory.cpp and Texture.cpp |
| Diff check against PR base | Passed; no output |
| Repository conventions against PR base | Passed |
| Hosted checks | Conventions and labels passed on the exact head; no product test job ran |
| NVIDIA GPU test report | The latest report tests 660cef6fe9ada0979f22ed2dd9c76419ae36306b, not this head; it cannot verify the new lock-order assertion |
Unedited local check output
CONFLICT (content): Merge conflict in core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
CONFLICT (content): Merge conflict in core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
All conventions checks passed.
Findings
Blocking
- The PR does not merge cleanly with current
main, so the required current-base validation cannot be completed. Please rebase ontodc38224141e0b6096007b876b4c42fc9144d11b8and preserve bothmain's changes and this PR's lock-order fix. - The new assertion/test commit
8b14bde3is not covered by the posted NVIDIA report, which tested660cef6fe. After resolving the conflicts, run the focused unwatch-lock-order test on the new exact head and a same-machinemainbaseline with Vulkan validation, as required by the AGC test guidance.
Non-blocking
The conventions check passes against the PR base. I did not find another new source-level blocker in the added assertion, but its GPU path is not verified by the existing report.
Sources
- Lock-order assertion at the reviewed head
- Unwatch lock-order test registration at the reviewed head
- AGC test guidance at current main
- NVIDIA report for the older head
Not verified
No current-main merged build, focused GPU test on this head, or console run was performed. The latest NVIDIA test report is on an older commit.
To get this to a good state
Please resolve the two current-main conflicts, then run the focused lock-order test on the updated head and same-machine main baseline with Vulkan validation. I will rerun the merge preview, diff check, conventions, and review the new test evidence on that exact head.
|
@winstrex the import-lock fix still has a check/use gap. Could you cover publication between the precheck and the hook with a deterministic barrier regression, and make the decision remain valid until the hook completes? The new test creates the image before starting the mapping thread, so it only exercises the pre-existing-image case. This finding comes from the source interleaving; I have not reproduced it in a native run. Please carry the correction through #7048 as planned. At the checked #7048 head |
…eline - ImportMappedRanges: main's `replacing` structure, with UnwatchHookLocksGpu in place of AnyPendingOverlaps. - markLayersPending: main's `!layerPending.AllIn(first, count)` block around this branch's pendingMiss bookkeeping and captureGuestBytes(&unsaved, true). With every unit already pending, that bookkeeping changed nothing, so skipping it keeps the behaviour. - writeBackWindows: no unit shadow destination when the stored span is not watched or the image is at most SmallStoreBytes. - Tests: agc_storage_pending_after_unwatch and agc_unwatch_hook_lock_order move to tests/cmake/agc_driver_recorder_tests.cmake. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… hook completes ImportMappedRanges decides under the imports mutex, without the GPU lock, that the unwatch hook will not need the GPU lock, then releases the live and pending registries before the import runs the hook. A storage image published in between (StorageTexture::Create) or one whose first units became pending (markLayersPending) made the hook take the GPU lock under the imports mutex while the GPU lock owner waited for that mutex. A map-time import gate now orders them. A map-time import without the GPU lock takes, before the imports mutex, the 2 MiB stripes of its run exclusively and keeps them until its import returns; Create, from before its upload to its publication, and a first pending transition take the stripes of their image shared. Order: GPU lock, stripes (ascending index), imports mutex, live and pending registries. The run is recomputed once the stripes are held and they are taken again if it grew past them. Publications take no stripe while no map-time import is announced: a publisher announces itself and then checks for imports; an import announces itself and then waits, yielding, until no announced publisher remains. Checked builds abort when a publication waits for the gate while its thread holds the imports mutex or a registry mutex. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
✅ NVIDIA 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 (3, not counted against this PR)Validated but not compared (8, 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. |
✅ 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 (12, not counted against this PR)Validated but not compared (8, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 481 tests)Skipped (54)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. |
|
@oneandonlydean Thanks, the gap is real. I reproduced it locally by pausing the mapper between Fixed in f7bfe87, on top of a merge with current Alternatives I rejected:
To keep the cost low:
Tests (all with
Each of 12 mutations of the gate (no gate, publication-only hold, inverted transition, no drain, a single global stripe, no retry, and others) fails at least one of them. The full suite on my machine has the same 4 failures as Cost, measured with temporary counters (not in the PR), two runs per build in The Pathless and in Hades under the same conditions. Runs disturbed by other activity on the machine were discarded.
No publication waited on the gate in any run, so the render threads never blocked on it. The import side waited about 9 ms per drain, on the mapping thread only, because TechLevelZero's Linux GPU runs of f7bfe87 merged onto current #7048 head is still |
This PR's own commits, without #7048's below them: ec6ceece9, f27fcde50 and d73e64951.
What
#7048 stores results that land in a unit over memory that has left the write watch, as long as the unit's bytes still match what was uploaded. For surfaces over 1 MiB it checks this against a per-unit hash, which
upload()takes on every settled upload. That pass reads and hashes the whole surface each time. On The Pathless (PPSA01825) menu it costs about 70 % of the presents, with 89 % of the draw committer's samples inStorageTexture::upload. The hash is only read inmarkLayersPending, for a unit with a write generation, no saved bytes and a range that is no longer watched.This PR takes that hash once, when it can be trusted and is about to be needed: when the unit's range leaves the watch. This is the shape @oneandonlydean described in #7048. The first commits here are #7048's own, followed by a merge with
mainthat resolves #7048's conflict with it (described below). This PR's change is the next commit, ec6ceec. A second merge withmainon top of it adapts that change tomain's tracker lock, f27fcde makes the refusal name its cause, d73e649 corrects the debt entry, and a third merge withmain(2853a75c1) resolves a conflict with #9159 (all below).StorageTexture::SaveBeforeUnwatch) also visits the live textures over 1 MiB that overlap the range. For each unit there with a write generation, no pending results and no saved bytes, it collects the range's writes. If the watch shows no write since that generation, it records a 128-bit hash of the unit's bytes together with that generation.upload()andrefreshno longer hash anything.markLayersPendingcaptures such a unit only while its bytes still match the hash and the hash's generation is still the unit's. With no hash, or a mismatch, it refuses, as it does now. Surfaces of 1 MiB or less keep comparing with the copy taken at the upload, so the hook doesn't visit them.ImportWatchednow runs the hook over the two spans of an import that stays watched which left the watch without it: a span whose page walk fails before the import (the hook runs before the import) and one whose walk fails after it. These are the spans fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does #7505 covers. Whichever of the two lands second drops itsGuestMemory.cpphunk.MarkWritten(UnwatchWritesProven). A unit there has no baseline, and its capture is refused, as before.GuestWriteWatchUnregistercalled from libkernel on Linux when a mapping is aliased, unmapped or replaced. On Windows there are none: the host regions are fixed when the guest arena is reserved, the commit generation only decides when a later commit restores the watch, and every path that takes a range out of the watch runs the hook (d73e649 drops the Windows case I had listed). Taking the Linux case through the hook touches libkernel and is left for a separate PR. The new debt entry "Units without a baseline outside the write watch" lists them. A unit there has no hash, so results that land in it outside the watch throw when they are stored, as onmain.SetImportWalkFailureForTestlets the tests fail either walk on both platforms.The refusal names its cause (f27fcde, after @zaidhawk's Persona 3 Reload run below). What is captured and what is refused don't change. The refusal now ends with the first unit refused and why it has no baseline, for example
(unit 63 of 64: its range left the watch through the unwatch hook without its CPU writes proven (a failed collect or import page walk)). The causes:The hook records why it didn't hash a unit next to the hash, with the generation it applies to. To record the unproven case it now also visits the live textures over 1 MiB when the CPU writes aren't proven. That path takes
GpuMutexwhen one of them overlaps, and it only runs after a failed collect or a failed import page walk.A wait I found and fixed while measuring. In the first version, the hook took
GuestMemory::GpuMutexwhenever the range overlapped any live texture. Onmainit only takes it when a texture with pending results overlaps. During The Pathless's menu that happened hundreds of times, and the thread doing the unwatch waited for the draw committer to release the mutex: 296–316 presents per 10 s againstmain's 315–336. The hook's own code didn't show in the samples, because the cost was the wait. The hook now visits only textures over 1 MiB, the same limit as #7048's hash. Those are the only ones a hash is ever taken for, so in The Pathless it takes the mutex no more often thanmaindoes.What the hook costs. A local counter that is not part of this PR, over the menu of each title:
ImportWatchedexits were never reached.Merge with
main. Since c0dbd18 (perf(agc): draw finisher pipeline), #7048 conflicts withmaininStorageTexture::markLayersPending:mainreworked the loop that #7048 changes. Until #7048 is rebased, this branch carries a merge of #7048's head da9f0ea withmaine3d03fb (93c878b):main's structure (unsavedstarts empty, theCommitTrims()prefilter, thenassignand the full loop), with fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048's condition in the full loop. The prefilter checksnoBaseline && (layerGeneration == 0 || savedTicket == 0), which every unit fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048's condition selects passes, soAPS5_DRAW_PIPELINEdoesn't change which units are captured, and the byte comparison still runs only in the full loop. The merge also keepsGenerationBaselineByteswhere fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 moved it;maindeclared it a second time next to its newSingleKeyColorFor.guestMatchesUploadtoguestMatchesBaseline, on the line of the full loop that fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 changes.main9d164f0 (df1b2d5): fix(agc): answer a collect memo hit with the generation of the last finished tracker update #6169 (answer a collect memo hit with the generation of the last finished tracker update) madelockTracker()returnTrackerLock, which has nounlock()/lock(). This conflict is entirely in this PR's part.markWrittenand theleavelambda inImportWatchednow release the tracker aroundbeforeUnwatchUnprovenwithSuspend()andResume(). Git merged theImportWatchedlambda without a conflict, but it no longer compiled.TrackerLock::Unlock()is not used here, because it publishes the generation, and both paths stamp blocks again after relocking; fix(agc): answer a collect memo hit with the generation of the last finished tracker update #6169 releasesmarkWrittenthe same way. @eliasbruno124-dev reached the same adaptation and tested it on an RTX 3060 Laptop GPU (comment).maincf528e7 (2853a75): fix(agc): keep a rewatched import's memory compared once it is shared #9159 (keep a rewatched import's memory compared once it is shared) marks each span ofImportWatchedwhose page walk succeeds as compared once it is shared, on Windows, in the loop where this PR takes a span whose walk failed out of the watch through the hook. Both are kept: a span whose walk fails goes throughleaveand is skipped, and a span whose walk succeeds is marked withGuestArenaCompareResidentWhenShared, as onmain. fix(agc): keep a rewatched import's memory compared once it is shared #9159 doesn't add a way out of the watch without the hook. Marking a page compared stops arming its shared aliases but leaves the range covered, and a walk that is refused because of compared pages now stamps them and records them as refused, whichUnchangedSincereports as changed. For this PR that can only turn a hash into the "watch saw CPU writes" skip, never bypass the hook. I counted the hook's decisions with local counters that aren't part of the PR. In the write-watch, storage image and import tests, fix(agc): keep a rewatched import's memory compared once it is shared #9159's included, onlyagc_storage_pending_after_unwatchreaches a hook decision, and none of its decisions came from a refused walk; its one CPU-writes skip is the case it makes on purpose. In a run of The Pathless's menu the hook decided for no unit at all.main4017310 (b61555e17): the only conflict was indocs/dev/debt/libSceAgcDriver.md, where docs(debt): record the storage image refusal over uncommitted guest memory #9275 added an entry under the one this PR rewrites. Both are kept: this PR's two entries, then docs(debt): record the storage image refusal over uncommitted guest memory #9275's unchanged.mainf55190f (90be5898c), for the conflict with fix(agc): store resident linear color targets at completion points, limited to their drawn rectangles #9685 (store resident linear color targets at completion points, limited to their drawn rectangles) inTexture.cpp. fix(agc): store resident linear color targets at completion points, limited to their drawn rectangles #9685 gives a pending unit a generation per 64 KiB block (unitBlockGeneration), raised where a completion store wrote, and no longer refuses a unit without a baseline in a target with drawn rectangles. Both are kept:markLayersPending: this PR's capture with its baseline causes stays where it is, and fix(agc): store resident linear color targets at completion points, limited to their drawn rectangles #9685's loop that clears the blocks' completion generations replaces the one that marks the units pending.saveBeforeUnwatch: this PR moved the written-block check intokeepWrittenSince, whichcaptureGuestBytesalso calls. fix(agc): store resident linear color targets at completion points, limited to their drawn rectangles #9685's per-block generations are now inkeepWrittenSince. Its other caller runs before the units are marked pending, whereunitBlockGenerationreturns the unit's generation, as before.unknown && !drawn.rect), with this PR's cause inside it. The units fix(agc): store resident linear color targets at completion points, limited to their drawn rectangles #9685 no longer refuses need no cause.agc_driver_linear_target_cpu_writes: the target's memory leaves the watch after the draw that records its rectangle, so the hook runs over the pending rectangle before the completion store, and the store must still keep the drawn half and the CPU's later stores to the other half.main27ad724 (339783405), for the conflict with fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does #7505 (run the unwatch hook before an import whose range leaves the write watch whatever the import does), which runs the hook for every range that leaves the watch, as this PR does:ImportWatched: this PR's version is kept. fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does #7505 runs the hook of a span whose window walk fails inside the loop over the window walks, with the tracker lock suspended, so another thread can collect a later span before its window walk. That is the race @ahmadmuj found in this PR's b61555e, now onmain. Here every window walk finishes before the failed spans' hooks run under one suspension, andCheckImportWalkHookKeepsLaterWindowsreproduces the race. The rest ofmain's changes to the file, such as fix(agc): let imports that cannot write guest memory keep the watch armed #10369'simportMayWriteGuest, merged without a conflict.tests/WriteTracking.cpp: both sides are kept.main()runs this PR'sCheckImportWalkUnwatchHooksandCheckImportWalkHookKeepsLaterWindowsand fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does #7505'sCheckUnwatchHooks, which passes on this head.docs/dev/debt/libSceAgcDriver.md: this PR's first entry, with fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does #7505's "or whose page walk fails" in its parenthesis, and its second entry unchanged. fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does #7505's last two sentences are dropped: the second entry covers the units without saved bytes that they describe.maincda9c9b (bfe0e86a3), for the conflict with perf(agc): hash the pages of an import window on several threads #10655 (hash the pages of an import window on several threads) inImportWatched.main's hashing is kept (CheckRangeover the spans, thenHashPages), still under the tracker lock and before the first page walks, and so are this PR'sfailsWalkandleave. fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does #7505'ssaveBytesstays out, as in the sixth merge. perf(agc): hash the pages of an import window on several threads #10655's tests are inagc_write_tracking.Tested
Windows 11 (10.0.26300), NVIDIA GeForce RTX 5070 Ti, driver 591.86, WinLibs GCC 15.2.0 r7, Release, with Overwolf's implicit Vulkan layers disabled. All three builds are on
main83462cc:mainitself,mainmerged with #7048's head da9f0ea (e2c1fe440), andmainmerged with this PR's head (4989cab12). The merges are local and clean.The Pathless (PPSA01825), main menu:
main319–326 presents per 10 s,main+ fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 85–105,main+ fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 + this PR 307–316. TwoAPS5_TRACE_SECONDS=120:10captures per build, alternated, noAPS5_PROFILE_*.Measurements
mainmainGraphics.Drawper frame (ms)Isolation: fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 with its upload hash turned off locally and without this PR's hook (
hashUnits = false, a dirty local tree, alternated with two moremainruns) gives 314 and 316 presents on average (windows 316 and 328), against 321 and 322 formain(windows 319 and 319). So the 2–4 % left betweenmainand the two PRs together comes from the rest of fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048, not from the hook.APS5_PLAYGO_UNLISTED_CHUNKS=absent, warm caches,APS5_TRACE_EVENTS=1048576so no thread dropped events, andAPS5_TRACE_SUMMARY_EVERY=10. The traces are inpathless-traces-main-83462cc57.zip,pathless-traces-main-7048-e2c1fe440.zip,pathless-traces-pr-4989cab12.zipandpathless-traces-isolation-7048-no-upload-hash.zip.aps5-summary.jsonl,main,main+ fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048, thenmain+ fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 + this PR:tools/trace_digest.py,main:p99 ms :%@::::--------- 0-228.4
gpu ms ._@@@@@@@@@@@@@@ 0-18.1
tools/trace_digest.py,main+ fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048:p99 ms _.@::::::::::: 0-488.5
gpu ms ..@%%%%%%%%%%% 0-20.4
tools/trace_digest.py,main+ fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 + this PR:p99 ms :@%------------ 0-191.5
gpu ms ..@%@%@@@@@@@@@ 0-19.2
Palworld (PPSA23126): abort at compute shader 0x15c1486e00, "storing storage image results pending over memory outside the write watch without the guest bytes saved before them is not implemented" #7017's refusal on The Pathless: 60 s runs, alternated.
mainstopped onstoring storage image results pending over memory outside the write watch without the guest bytes saved before them is not implementedin 3 of 8 runs, 8 to 10 s in.main+ fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 + this PR stopped in none: 0 of 7 runs with the hook's first version (before the 1 MiB limit) and 0 of 6 with this head.LEGO Horizon Adventures (PPSA14632) and Hades (PPSA03355): one run per side,
mainagainstmain+ fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 + this PR. LEGO's loading screen gives 57–65 presents per 10 s against 63–65, and its menu 29–33 against 30–32. Hades gameplay gives 295–330 against 296–336. No refusal in any run. These two ran with the hook's first version; the 1 MiB limit only narrows the textures it visits.Persona 3 Reload (PPSA10872): I don't have it. @zaidhawk ran df1b2d5 on an RTX 5090 (comment): the start-up screens and the opening animation are back at
main's rate (596–600 presents per 10 s, against 117–122 with fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 alone), but 1 of 5 runs stopped on the refusal, which fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 alone didn't in 5. Its log doesn't say which case it was; f27fcde adds that to the message. They ran d73e649 again with 120 s runs (comment): 0 of 10 runs stopped on the refusal, against 2 of 5 onmain9d164f0 and 0 of 5 withmain+ fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048, so there is no cause line yet. Over both sessions this PR stopped in 1 of 15 runs, fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 alone in 0 of 10 andmainin 4 of 10. The start-up screens stayed at 594–596 presents per 10 s with this PR, against 120–122 with fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048.Tests:
agc_storage_pending_after_unwatchchecks the cause named in every refusal, with three new cases: the last block leaves the watch without the hook (refused at 1024×1024 for having no hash; stored at 256×256, which compares with the upload copy), the same while the results are pending (refused in both sizes), and a failed collect, on Windows (refused at 1024×1024 as unproven; stored at 256×256). Recording the wrong cause in the hook makes it fail.agc_storage_pending_after_unwatchpasses in both sizes (256×256 through the upload copy, 1024×1024 through the hash). New case: a CPU write into the last block while it is still watched, before it leaves the watch. The unit then has no baseline, and the capture is refused in both sizes with the CPU's bytes kept. The existing case with a CPU write after the unwatch, which is after the baseline, is still refused at 1024×1024.agc_write_tracking:CheckImportWalkUnwatchHooksreaches bothImportWatchedexits throughSetImportWalkFailureForTest. It checks that each runs the hook once while its block is still watched, before the import for the first exit and after it for the second, and with the CPU writes reported as not proven. It also checks thatUnwatchreports them as proven.CheckFailedCollectUnwatchnow also checks that a failed collect reports them as not proven.CheckImportWalkHookKeepsLaterWindowsfails the walk of only the first of two spans after the import, and its unwatch hook collects the second span while the lock is suspended, as another thread could. It checks that a page the import touched there without changing it still reads as unchanged, with its window walk's possible CPU touch. It fails on b61555e and passes from 43a9245.baselineMiss. A unit whose content changes through this emulator either has its hash forgotten (write-back, clear, alias borrow) or no longer matches it, so that check only refuses earlier.agc_storage_pending_after_unwatchno longer requires any case to be refused, so an implementation that stores these units safely still passes. It checks the cause named in a refusal, that a refused write-back leaves the CPU's bytes as they were, and that a store merges the GPU's texels with the CPU's bytes. Four new cases write the CPU's bytes into the last unit after GPU results that cover part of it were written: into the hashed unit, before the unwatch, after an unwatch without the hook, and before an unwatch through a failed collect. Accepting any of them would store the image's older bytes over the CPU's, and the byte checks catch it. Ignoring the hash or the upload copy, hashing a unit the watch saw written, or taking a missing hash for a baseline now fails on the bytes. Hashing after a failed collect is caught by the cause instead: the next collect still sees the CPU's write, so the hook refuses the unit for that write. A write the failed collect loses for good, which the test can't produce, is the case where it would store the wrong bytes.Validation layer: the write-watch, storage image, import, draw, recorder and VideoOut tests pass on
main83462cc (59) and onmain+ fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048 + this PR (60,agc_storage_pending_after_unwatchincluded), with noVUID-messages on either side. They ran withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1, and neither log haswas not found but was requested by env var VK_INSTANCE_LAYERS. The set isctest -V -R '^(agc_write_tracking.*|agc_storage_.*|agc_import_.*|agc_driver_store_over_untracked.*|agc_unit_shadow|agc_metadata_pass|agc_slice_dirty|agc_resident_present|agc_tiled_resolve|agc_queued_write_groups|agc_driver_graphics|agc_draw_.*|agc_recorder_.*|agc_driver_flip.*|video_out_flip_.*)$'.After the merge with
maine3d03fb, at ec6ceec, with NVIDIA driver 610.88:maine3d03fb (59) and on this head (60), withAPS5_DRAW_PIPELINEunset and set, with noVUID-messages on either side.APS5_TRACE_SECONDS=120:10control run per side: the menu's 10 s intervals give 327–330 presents onmainand 324–329 on this head.After the second merge, with
main9d164f0, at df1b2d5, with NVIDIA driver 610.88:main9d164f0 (59) and on this head (60), withAPS5_DRAW_PIPELINEunset and set, with noVUID-messages on either side.APS5_TRACE_SECONDS=120:10control runs per side, in alternated order: the menu's 10 s intervals give 303–313 and 299–318 presents onmain, and 293–307 and 302–320 on this head.After f27fcde, with NVIDIA driver 610.88 (d73e649 above it only changes the debt entry):
APS5_DRAW_PIPELINEunset and set, with noVUID-messages;main9d164f0 passes its 59 the same way (above).APS5_TRACE_SECONDS=120:10control runs per side, alternated withmain9d164f0: the menu's 10 s intervals give 290–305 and 287–321 presents at f27fcde (one interval of 260 in the second run), and 289–313 and 281–304 onmain. No refusal in any run.After the third merge, with
maincf528e7, at 2853a75, with NVIDIA driver 610.88:agc_rewatched_.*,agc_compared_.*,agc_driver_compared_.*andagc_driver_partially_resident.*), passes onmain(70) and on this head (71), withAPS5_DRAW_PIPELINEunset and set, with noVUID-messages on either side.APS5_TRACE_SECONDS=120:10control runs alternated withmaincf528e7: the menu's 10 s intervals from 20 s on give 332–345 and 308–360 presents on this head, and 342–358 and 351–363 onmain.mainstopped on the refusal in 1 of its 3 runs, and this head in none of its 3, nor in the one below. One more run of this head ran at 171–179 from start to end, withDriver.Packetat 54 ms per frame against 29 in the others; the same build gave 332–345 in its next run, so I've left that run out.ctestsuite with the GPU (--parallel 4 --timeout 120): 2278 of 2289 pass and 7 are skipped. The other 4 (log_macros,log_macros_timestamps,agc_host_import_cut_lifetimeandagc_driver_depth_plane_partial_store) fail the same way onmaincf528e7.After the fourth merge, with
main4017310, at b61555e, with NVIDIA driver 610.88:main(72) and on this head (73), withAPS5_DRAW_PIPELINEunset and set, with noVUID-messages on either side and the layer loaded.After the fix for the import walk hooks, at 43a9245, with NVIDIA driver 610.88:
main4017310 (72) and on this head (73), withAPS5_DRAW_PIPELINEunset and set, with noVUID-messages on either side and the layer loaded. In one run,mainwithAPS5_DRAW_PIPELINE=1failedagc_write_tracking_metadata_commit(an untouched leaf has a write stamp); it passed 5 of 5 reruns on each side, and this branch doesn't change what it tests.After the fifth merge, with
mainf55190f, at cfbe00c, with NVIDIA driver 610.88:main(72) and on this head (73), and fix(agc): store resident linear color targets at completion points, limited to their drawn rectangles #9685'sagc_driver_linear_target_*andagc_driver_address_passes_*tests pass on both (13), withAPS5_DRAW_PIPELINEunset and set, with noVUID-messages on either side and the layer loaded. The new case passes seeded and unseeded.ctestsuite with the GPU (--parallel 4 --timeout 120): 2363 of 2375 pass on this head and 7 are skipped. The other 5 (guest_wide_formatting,log_macros,log_macros_timestamps,agc_host_import_cut_lifetimeandagc_driver_depth_plane_partial_store) fail the same way onmainf55190f.public_headerstimed out at 120 s on both sides, leaving its 64 compile tests not run; rerun on their own with--timeout 1800, all 65 pass on both, in 53 s.video_out_flip_overlayfailed onmainonly, with an overlay's implicit Vulkan layer loaded, and passed when rerun with that layer disabled.APS5_TRACE_SECONDS=120:10control run per side at 3840x2160, because fix(agc): store resident linear color targets at completion points, limited to their drawn rectangles #9685 changes how render targets are stored: the menu's 10 s intervals from 30 s on give 359–374 presents onmainand 375–401 on this head. No refusal in either run.After the sixth merge, with
main27ad724, at 3397834, with NVIDIA driver 621.01:agc_driver_linear_target_*andagc_driver_address_passes_*tests without the one that flips, passes onmain(66) and on this head (67), withAPS5_DRAW_PIPELINEunset and set, with noVUID-messages on either side and the layer loaded. That includes fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does #7505'sCheckUnwatchHooksinagc_write_tracking.After the seventh merge, at bfe0e86, with NVIDIA driver 621.01, against
mainad622fc:agc_copy_guest_bytes, passes onmain(72) and on this head (73), withAPS5_DRAW_PIPELINEunset and set, with noVUID-messages on either side and the layer loaded.Full
ctestsuite at f27fcde (--parallel 4 --timeout 120): 2026 of 2036 pass and 5 are skipped. The other 5 (log_macros,log_macros_timestamps,agc_driver_sample_order,agc_driver_sample_order_grid_referenceandagc_driver_sample_order_recipe_depth_locations) fail the same way onmain9d164f0.The fork's CI wasn't run: its runner has no Vulkan device and skips the GPU tests.
tools/check_conventions.pypasses againstmain.tools/pr_overlap.py: fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does #7505 and fix(agc): keep a rewatched import's memory compared once it is shared #9159 touchImportWatched; fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does #7505 is described above, and fix(agc): keep a rewatched import's memory compared once it is shared #9159 only neighbours its lines. The other entries (perf(agc): keep a fast clear's guest bytes while they match unwatched memory #6108, perf(agc): clear single-key DCC surfaces on a verified uniform color #6848, perf(agc): draw finisher pipeline (APS5_DRAW_PIPELINE) #6946, feat(agc): look up table images at run time through a device image table on the GPU heap (APS5_GPU_PATH) #7642, docs(debt): record the storage image refusal over uncommitted guest memory #9275, perf(agc): GPU descriptor heap on VK_EXT_descriptor_heap (APS5_GPU_HEAP) #9508, fix(agc): free a completed batch's command buffer instead of keeping it #9565, perf(agc): keep constant RAM dumps and unregistered labels on the GPU without draining #9898) come from fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048's own changes, which this branch carries. perf(agc): track pending and borrowed texture units in 64-bit words #10886 builds on this PR (Depends on: #10026) and keeps itsmarkLayersPendingscan inside itsAllInguard.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)main, 0 of 13 with both) and its menu runs at 307–316 presents per 10 s instead of fix(agc): save the guest bytes of results that become pending over a surface that is no longer fully write-watched #7048's 85–105 (main: 319–326)pathless-traces-isolation-7048-no-upload-hash.zip
pathless-traces-main-7048-e2c1fe440.zip
pathless-traces-main-83462cc57.zip
pathless-traces-pr-4989cab12.zip