Repository navigation
fix(agc): read a frame capture's memory delta once the accepted submissions completed - #11197
Conversation
…ssions completed A delta read its blocks with ReadCommittedUnsynced while GPU work accepted before the call could still write them, so a capture could hold a block from before a DMA copy that the replay then wrote back over the copy's result. A delta with blocks to read now waits up to 10 s for the accepted submissions and records the serials after the wait; the capture stops with a warning when the work does not finish. The Windows quarantine of agc_driver_frame_replay is removed. Fixes boykopovar#10711 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I found a race that can leave the original stale-delta bug possible. Driver::WaitIdleFor snapshots accepted on entry and returns once completed reaches that snapshot (Driver/Lifecycle.cpp:253-255); it does not prevent later submissions from being accepted. The public submit wrapper calls recorder->Submit and then driver.Submit separately (Driver/EntryPoints.cpp:22-24), and the recorder lock is released before the driver submission. A second thread can therefore accept a submission while this wait is in progress. This code rereads serial progress at line 376 but proceeds to ReadCommittedUnsynced even if the new accepted is greater than completed, so that write can still land after the delta read. Please serialize acceptance against the wait/read or otherwise establish a stable idle point, and add a regression that accepts another write during the wait. The report also omits the required VK_LAYER_KHRONOS_validation run and VUID comparison for an AGC-driver change; CONTRIBUTING.md requires those on the same GPU for main and change.
|
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.
|
…r takes its next call The recorder released its lock between recording a submission, flip or suspend point and the driver accepting it. Another thread's delta could then wait for the accepted work, read its blocks, and record itself after a call whose work the driver had not accepted yet, so the replay submitted that work before applying stale bytes over its result. The entry points now pass the driver call to the recorder, which runs it under the same lock right after recording the call. agc_driver_frame_replay_concurrent submits copies from a second thread during the capture and checks that every memory delta was read after the submissions recorded before it had completed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@MarcZX10 thanks, the race was real. Fixed in Acceptance is now ordered with recording. Regression. Runs with ctest pinned to two cores with three busy-loop processes on them and
Validation layer. Vulkan SDK 1.4.363.0, same machine and GPU (Radeon 860M, AMD 24.30.58.08, Windows 11 10.0.26300).
Neither log has AI-assisted: yes (Claude Code) |
|
A Windows check of this head against #10711's reproduction, plus a run on three GPUs. Windows 11 Home 10.0.26200, WinLibs GCC 15.2.0 r7 (MinGW-w64 UCRT), Release. Under contention, as in LeandroLP23's reproduction on #8740.
On three GPUs. One GPU at a time, with Negative control. So on this machine the PR removes the Windows failure #10711 quarantined, and the test can come out of quarantine. AI-assisted: yes (Claude Code) |
|
@LeandroLP23 Thank you for picking this up. |
|
Edit: I missed that this is already covered on #11157. janzezno notes the "Frame capture" debt entry lists this case, and JJenkx has a game-independent two-queue reproducer. The data below is a second title for the same mechanism: the label is written by a RELEASE_MEM on queue 0x0, which matches the later-submission case in JJenkx's reproducer. @LeandroLP23 with this merged, a Ghost of Yōtei frame capture on Linux hits the 10 s timeout at call 78 in all 3 runs, after 3 memory groups. Before it, the same tree captured 10 frames. The description says the timeout path isn't exercised because it needs GPU work that never completes. Here it fires on every run, and the GPU work looks like it's waiting on the submitting thread that the wait holds. Setup: RTX 3080 Ti, Linux, my local GoY tree (main plus unmerged local commits) with
Each stopped run logs The address was A fourth run under gdb (boundary 26) hit the same timeout at the same address. I took two samples of all 94 threads, 3 s apart, starting about 1 s after the queue's timeout line. In both: Thread 84 is the only thread in My guess, not verified: the label is written by a later queue 0x0 submission that the game makes only after this submit returns. While I'm not proposing a fix. I can test a change on this setup, or share the full gdb log. AI-assisted: yes |
…e title A memory delta was read only once every accepted submission completed, so a capture stopped after 10 s whenever a queue waited on a REWIND control word or a label a later call stores, as in the two-queue reproducer on boykopovar#11157 and the Ghost of Yotei capture on boykopovar#11197. The driver gains a settled hold: AgcDriverHoldSettled succeeds once every queue has finished or is parked on a REWIND or WAIT_REG_MEM value the title has not stored, with no queued draw, unrecorded label or pending label write, and keeps submissions and parked queues where they are until AgcDriverReleaseSettled. The recorder reads each delta under that hold through the coherent path. The replay restores each delta under the same hold. Its breaker, built on boykopovar#12318, releases a parked REWIND by applying the first later memory group that sets its control word, only under a settled probe, one wait per pass, and no longer writes a value from a later delta for any other wait, which applied a later CPU store early. Pacing counts a running queue as progress and gives up after --pace-timeout-ms only once no queue runs, or after the busy timeout. The drain also ends after 10 s without a completion once no queue runs, and the busy timeout, ten minutes, is shared by pacing, the settled hold and the drain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
A frame capture could store a memory delta from before an earlier recorded DMA copy had landed.
FrameRecorder::writeDeltaread the delta's blocks withReadCommittedUnsyncedwhile GPU work accepted before the call could still write them. At replay the stale bytes were written back over the copy's result. That is the intermittent "the replay did not reproduce the second copy" ofagc_driver_frame_replayin #10711 (root cause in #8740 (comment)).WaitIdleFor), when any are still in flight. It records the accepted and completed serials after the wait. This is option 1 of that comment. A delta with nothing to read does not wait, so those calls keep their submissions overlapped.Submit,SubmitFlipandSuspendPointhand the driver call to the recorder. It runs the call under its lock right after recording it, so every call recorded before a delta has been accepted when the delta waits. Without this, another thread's submission recorded before the delta could be accepted during the wait or after the read, and the replay would submit it before applying stale bytes over its result.writeDeltaruns insideSubmitandSubmitFlip, which catch onlyProcessShutdown, so an exception would end the title.abandonIfDiskLowand the timeout sharestop(reason).agc_driver_frame_replayfrom test(agc): quarantine agc_driver_frame_replay on Windows until the recorder fix (#10711) #10715 is removed, with its debt entry.BUILD.mddescribes the wait, and the Frame capture debt entry gains the new way a capture can stop.@Sumaiter opened the same fix as #11185 (an unconditional wait that throws on timeout). The bot closed it at once for the open-PR limit, so I'm opening this one so the fix isn't blocked. Credit for the independent analysis goes to them. #11157 (janzezno) touches the same quarantine lines; the agreed order in #10711 is the fix first, then #11157 registers the test on Linux without its quarantine changes.
Tested
Windows 11 (10.0.26300), AMD Radeon 860M (AMD 24.30.58.08, LLPC), WinLibs GCC 15.2, Release with
BUILD_TESTING=ON, onmain07da04f947.frame_replay|frame_capturetests pass withANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1:agc_frame_capture,agc_driver_frame_capture_eop,agc_driver_frame_replay, the newagc_driver_frame_replay_concurrentand the sixagc_driver_frame_replay_*state tests.agc_driver_frame_replay_concurrentsubmits 64 DMA copies from a second thread during the capture. It checks that every memory delta was read once the submissions recorded before it had completed, and that the replay reproduces all 64 copies.agc_driver_frame_replayunder contention. ctest is pinned to two cores with three busy-loop processes on them andANYPS5_GPU=no-such-device, as on the runner; this is the setup that reproduced the CI failure.main07da04f947)b709c0fe3dagc_driver_frame_replay_concurrentunder the same contention: 100 of 100 on this head. With only the oldSubmitorder put back (record, release the lock, thendriver.Submit) it passes 66 of 100; the failures say a delta was read with a submission recorded before it unfinished.ctest -VwithVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1on this head and on its basemain07da04f947. Theframe_replay|frame_capturetests pass 10/10 against 9/9 onmain. They never load the Vulkan loader, so the layer has nothing to check there. The 22agc_driver_*GPU tests matchingflip|suspend|submit|eop|labelgive 20 passed and 2 skipped on both. There are 0VUID-lines in any log, and nowas not found but was requested by env var VK_INSTANCE_LAYERS. A direct run withVK_LOADER_DEBUG=allshows the layer inserted.check_conventions.py --base origin/mainpasses.pr_overlap.pyfinds feat(agc): capture and replay frames on Linux #11157 (above), and fix(agc): keep capture trace output nonfatal #10760, which adds an unrelated debt entry (capture trace output) next to the Frame capture one, so whichever lands second resolves a text conflict in that list.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)