Repository navigation
fix(agc): stop frame capture deltas from collecting the previous delta's pages again - #14137
thuanquang wants to merge 17 commits into
Conversation
|
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.
|
|
@thuanquang I reviewed this and tested it on an RTX 3080 (NVIDIA 621.01, Windows 11). Your new tests pass, but I found one lost write: a late range can permanently lose a clear to zero. Results on The bug. Lines are from this head. The collect at
The replay keeps the old bytes from step 2. Reproduced with a test. It follows those steps, then checks the last delta holding the block:
The test is below if it's useful. Suggested fix. Write added-range blocks even when they're all zero, since compression keeps that cheap. I'd add a test of this kind, because Merge order. Once this is fixed, I'm happy for #14137 to go first. #12568 is stacked and still waits on its dependencies, so your offer to post that rebase would be very welcome. A separate source audit found no other lost write in the watermark logic. It didn't cover address reuse beyond what Testvoid testLateClear(const std::filesystem::path& directory) {
constexpr std::size_t Size = 1u << 20;
constexpr std::size_t Page = 4096;
auto* recorder = AgcDriver::FrameRecorder::Active();
check(recorder != nullptr, "frame recorder is disabled");
recorder->FrameBoundary();
auto* late = allocate(Size);
std::fill_n(late, Size, std::byte{0});
auto* x = late + 2 * FrameCaptureWriter::BlockBytes;
auto* shader = reinterpret_cast<Shader*>(x + 2 * Page);
*shader = {};
shader->file_header = 0x34333231u;
shader->version = 0x18u;
shader->header_size = sizeof(Shader);
auto* neighbor = x + FrameCaptureWriter::BlockBytes;
std::fill_n(neighbor, Page, std::byte{0x5a});
recorder->RegisterShader(shader);
std::fill_n(neighbor, Page, std::byte{0});
recorder->Suspend([] {});
recorder->Suspend([] {});
recorder->FrameBoundary();
const FrameCaptureReader reader(directory);
std::vector<std::byte> last;
std::size_t holding = 0;
for (const auto& chunk : reader.Chunks()) {
if (chunk.kind != FrameMemoryKind::Delta || chunk.address > address(neighbor) || address(neighbor) >= chunk.address + chunk.bytes) continue;
const auto bytes = reader.Read(chunk);
last.assign(bytes.begin() + static_cast<std::ptrdiff_t>(address(neighbor) - chunk.address), bytes.begin() + static_cast<std::ptrdiff_t>(address(neighbor) - chunk.address + 1));
++holding;
}
std::puts(("late clear: " + std::to_string(holding) + " deltas hold the neighbour, last byte " + std::to_string(last.empty() ? -1 : static_cast<int>(last[0]))).c_str());
check(holding != 0, "the neighbour is in no memory delta");
check(last[0] == std::byte{0}, "the clear of the neighbour is in no memory delta");
}AI-assisted: yes (Claude Code) |
…d pending result Co-Authored-By: Claude <noreply@anthropic.com>
…a's pages again Co-Authored-By: Claude <noreply@anthropic.com>
The delta that adds a range the registry gained during the capture writes only the range's nonzero 64 KiB blocks. The previous commit makes that delta collect a watched or unwatched range before reading it, so the next delta no longer records what the CPU wrote there before the add, and a skipped all-zero block kept whatever the replay already held at its address: bytes a shader registration's delta recorded there before the CPU cleared them, or results of replayed GPU work at an address the title reused or made CPU-readable during the capture. The delta that adds such a range now writes every block of it. The test is chitters2004's from the review. Co-authored-by: chitters2004 <55252868+chitters2004@users.noreply.github.com> Co-Authored-By: Claude <noreply@anthropic.com>
4e1f9bb to
c7a4fc9
Compare
|
@chitters2004 thanks for testing this and for the test. You're right: the collect at the add ( The new head Your test is in as A host-allocation variant of your test stays stale on
Thanks for the order: #14137 goes first, and I'll post the #12568 rebase onto this head as a branch on my fork. Besides the per-collect |
|
The description identifies tracker gaps that the previous repeated read could cover. Please guard against losing that coverage when advancing each range to the returned generation. For example, after a first delta collects a CPU-written page, a guest CPU store to another byte of that page can land between the two walks in Please add a controlled regression for that interleaving and preserve the recorded CPU bytes while removing duplicate pages, or resolve the tracker prerequisite before enabling this cutoff. A fresh page note from |
✅ NVIDIA GPU test: passedPR head
Unstable on main (5, 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 (4, 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. |
…t collect Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@thuanquang @oneandonlydean Here are the controlled regressions oneandonlydean asked for, plus a fix, so this doesn't have to wait: It fast-forwards this PR's branch, with no rebase or force-push. Anyone with push access to the branch can take it: Tests, in
Fix, in
So the first is a regression from the returned-generation cutoff, and the second is already lost on With the fix, Limits:
AI-assisted: yes (Claude Code) |
… format 1 capture State mode gpu_write_before_delta activates a driver recorder on a mock Vulkan device that notes a GPU write over a guest page, so the driver's pending-write snapshot covers it, and installs a flush hook that lands the write (a 0x5a fill) when an access reaches the page. The capture's memory delta holds the page as a coherent recorder reads it after that write landed and the CPU cleared it. The replayed page must read 0, and the write must have landed once: restoring the delta while the write is still pending lets the next read land the fill over it. State mode calls_v1 rewrites a capture's calls.bin magic to "aps5-frame-calls 1" and checks that restoreMemory refuses it with a message that says to record the capture again, before it restores the captured range. Co-Authored-By: Claude <noreply@anthropic.com>
…refuse calls format 1 FrameRecorder::readGuest reads range and written-block deltas through GuestMemory::ReadCommitted, which first lands the GPU writes the driver still has pending over the range, so such a delta holds the bytes after those writes. The replay restored a delta without landing them, so a write still pending over the range in the replay landed after the restore and replaced the delta's bytes. The replay now reads each delta chunk, calls AgcDriverPrepareCommandWrite_nid_no_patch over its range and then copies it, the order chitters2004's boykopovar#12567 uses in restoreGroup. A calls format 1 capture does not record whether GPU writes had landed before each memory delta was read: main's recorder read its deltas without landing them. calls.bin is now format 2 ("aps5-frame-calls 2"). FrameCaptureReader still reads format 1 and reports the format (Version()), and agc_frame_replay refuses a format 1 capture before it restores any memory, with a message that says to record it again. Co-authored-by: chitters2004 <55252868+chitters2004@users.noreply.github.com> Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
A GPU write's collect notes the pages of its block that are newly mapped (a view mapped during the capture, a Linux page registered again) as CPU stores, but raises only the block stamp, not the CPU block stamp CollectWrittenBlocks admits blocks on, so a delta lists them only when another CPU store raised that stamp. agc_write_tracking_fresh_view (Windows) drives that through a section view mapped after the first collect and fails on main. agc_driver_frame_replay_fresh_note (Windows, from c9a87b4) checks the same through a capture; no row drives the Linux case (a page registered again). The agc_write_tracking sequence gains two guards that pass on main: a page only an import window stamped is listed, and a block only a GPU write stamped lists nothing. Co-Authored-By: Claude <noreply@anthropic.com>
…WrittenBlocks CollectWrittenBlocks skipped every block whose CPU block stamp was not newer than the delta's generation. A fresh collect around a GPU write notes its newly mapped pages as CPU stores and raises the block stamp only, so those pages were listed only when another CPU store raised the CPU block stamp, and a capture otherwise replayed them with the bytes of the mapping they replaced. A block is now also admitted when its block stamp is newer, and lists the pages whose CPU page note is newer. A block admitted only on its block stamp lists nothing else, so GPU and driver stores alone still add no pages. Only the frame recorder calls CollectWrittenBlocks. Co-Authored-By: Claude <noreply@anthropic.com>
…store agc_driver_frame_replay_store_window stores a label with the callback form of GuestMemory::StoreOwnBytes, which gives no bytes, while another thread writes the same page, then stores into four more pages with StoreOwnBytes (given bytes), Write, AgcDriverStoreGuestLabel and WriteChanged, with nothing else stored in them. The other thread's byte must be in a memory delta, and the four pages in none. After the capture has finished, a driver store must no longer be listed as a CPU write. agc_driver_frame_replay_store_window_unwritten makes the same driver store, with the same store from another thread during it, into a page the CPU has not written, so the page has no CPU page note. On main the second walk of the driver store takes the other thread's page as the driver's own, so no delta lists it and both tests fail. agc_driver_frame_replay_store_window_stopped stops the capture when it starts (APS5_FRAME_CAPTURE_MIN_FREE_BYTES above any free space) and checks that a driver store made after that is not listed as a CPU write. Co-Authored-By: Claude <noreply@anthropic.com>
…s bytes StoreOwnBytes gains a callback form that also receives a span, into which the store writes the bytes it stores. Write, the form of StoreOwnBytes that takes its source bytes, and AgcDriverStoreGuestLabel use it. GuestMemory::SetOwnStoreCompare sets a switch that nothing reads yet. The span is always empty here, so every store runs as before. Co-Authored-By: Claude <noreply@anthropic.com>
…mpared agc_write_tracking_own_store_images (Windows; on Linux the last check of agc_write_tracking) first checks that a driver store which gives no bytes, made before GuestMemory::SetOwnStoreCompare is turned on, lists no page. It then turns the switch on and checks that a Write, a store of given bytes, a guest label store and a write-back with nothing else stored in their pages list no page; that a store another thread makes beside the bytes of a two-page driver store lists only the page it is in; that a store another thread makes over a driver store's bytes is listed; that a store another thread makes that undoes a driver store which gives no bytes is listed; and that a listed page is not listed again. Each check collects on its own, and all failures are reported together. Co-Authored-By: Claude <noreply@anthropic.com>
… while capturing GuestMemory::StoreOwnBytes walks the pages of a store before and after the driver's store and takes the pages the second walk finds written as the driver's own, so a store another thread made to those pages in between reached a frame capture delta only when another CPU store got the page listed. main starts each delta from a generation read before the previous delta's collects, so it lists again the pages that collect found written and keeps such a store in them: chitters2004's agc_driver_frame_replay_store_between_walks (c9a87b4) passes on main and fails at c9a87b4, where each delta starts from the generation its collect returned. While a capture records (the recorder switches this on with GuestMemory::SetOwnStoreCompare before its first collect and off when it stops or finishes), a driver store from the CPU that gives its bytes copies its pages before the first walk and applies those bytes to the copy; a page the second walk takes that differs from the copy, or can no longer be read, is noted, and CollectWrittenBlocks lists it. A store that does not give its bytes (MEM_SEMAPHORE, buffer copies on the CPU) notes every page its second walk takes. No stamp changes, so UnchangedSince, StoredOver, UnchangedSinceCollected and ChangedBlocks answer as before, with or without a capture. The import-window walk notes the pages it finds changed the same way; they already carry CPU page notes, so nothing more is listed for them. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…image GuestMemory::WriteChangedKeepingGuest (from main) skips a changed byte whose guest byte no longer equals the uploaded one, but the store image a capture compares against still took every changed byte, so a page where the write-back kept such a guest byte and stored another byte differed from its image and was listed in the next memory delta although no other thread wrote it during the write-back. The same image also matched a page where another thread, during the write-back, put the uploaded byte back before the write-back read it or wrote the shader's byte over a kept one, so that store was not recorded. The image now takes a changed byte only where its copy of the page still equals the uploaded byte, as the write-back itself does. agc_write_tracking_own_store_images (agc_write_tracking on Linux) gains a keep-guest write-back over a page the guest wrote after the upload; it fails without this change. Co-Authored-By: Claude <noreply@anthropic.com>
…s out of the write watch Co-Authored-By: Claude <noreply@anthropic.com>
|
@oneandonlydean, this push takes your first route inside this PR. The tracker change is in its own commits (
Two cases can still be lost, by reading, and are in the debt file:
@chitters2004, thank you for the tests, the fix and the push commands. I had a tracker change for both cases under way, so I kept it and took your tests. On an RX 6700 XT they fail in 3 of 3 runs on As agreed in #12568's thread, this push also has the four replay items you asked for on #12567: Windows 11 26200, Ryzen 5 5600X, RX 6700 XT (gfx1031), AMD driver 32.0.21045.16010, WinLibs GCC 15.2.0 r7, Release, full builds. Counts, term drops and the validation-layer comparison are in the description. Linux: a WSL2 lavapipe build of this head passes AI-assisted: yes (Claude Code) |
✅ AMD GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (8, not counted against this PR)Validation IDs already present on main (4 IDs in 522 tests)Skipped (57)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. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (4 IDs in 522 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
left a comment
There was a problem hiding this comment.
Reviewed — approved
The merged tree builds, and all 28 frame capture/replay and write-tracking tests pass on native Linux with the guest write watch active. That includes the capture rows the description says have not run on Linux. A three-stage negative control made every new row fail, and I found nothing blocking.
What I ran
Head 702492e63ea6f9b96280654c1b1733a02e059377, main 4f41f4c375f5abb487dbd184fbc150f31994d762, merged with --no-ff (tree 23da7e8b9297967958c6b8f5082d0550652a760e). Machine: Debian 13 VM, 32 vCPU Xeon Gold 6136, GCC 14.2.0, Release, Ninja, BUILD_TESTING=ON, lavapipe (Mesa 26.1.6), no GPU.
git diff --check 4f41f4c375f5abb487dbd184fbc150f31994d762...702492e63ea6f9b96280654c1b1733a02e059377
python3 tools/check_conventions.py --base 4f41f4c375f5abb487dbd184fbc150f31994d762
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "frame_(replay|capture)|^agc_write_tracking"
ctest --test-dir build -V -R "agc_driver_frame_replay_(store_window|late_clear|once)$"
# negative control, stages A-C below, each followed by:
ninja -C build -j32 -l32 agc_driver_frame_replay_state_tests agc_write_tracking_tests
ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "agc_driver_frame_replay_|^agc_write_tracking"
# restore, then:
ninja -C build -j32 -l32 agc_driver_frame_replay_state_tests agc_write_tracking_tests libs
git status --porcelain
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
Configure, build, libs (script) |
passed |
| Selected tests (script): 2216 run | 0 new failures, 0 timeouts. The 7 known failures also fail on main (agc_driver_division, the denormal-mode rows, vector_constant_factor_tininess, one unwrap test) |
| Validation layer (script): 770 tests, layer loaded in 769, missing in none | 0 tests with new VUID- lines |
Focused set: frame replay/capture plus agc_write_tracking* |
28/28 pass |
| Capture rows really record on this host | yes, see below |
The capture rows run on native Linux (excerpt)
1747: [frame-capture] started at frame boundary 1: 1 ranges, 0.1 MiB snapshot in 0.0 s
1747: [frame-capture] 1 frames, 3 calls, 2 memory deltas (1.1 MiB), 1.2 MiB raw, 0.0 MiB stored in .../frame-replay-late_clear
1747: frame replay state test passed
1751: [frame-capture] started at frame boundary 1: 2 ranges, 0.1 MiB snapshot in 0.0 s
1751: [frame-capture] 1 frames, 1 calls, 1 memory deltas (0.0 MiB), 0.1 MiB raw, 0.0 MiB stored in .../frame-replay-store_window
agc_write_tracking reports Passed, not Skipped, so on Linux it also runs CheckOwnStoreImages.
The description says the new capture rows and agc_write_tracking have not run on Linux, because the author's WSL2 kernel has no asynchronous userfaultfd write protection. On this kernel (6.12) the write watch is available. Here they run and pass: once, pending_result, partly_covered, late_clear, store_between_walks, store_window, store_window_unwritten, store_window_stopped, gpu_write_before_delta, calls_v1, agc_frame_capture and agc_write_tracking, including the Linux tail of own_store_images. fresh_note, agc_write_tracking_fresh_view and the separate own_store_images registration are Windows-only and were not run.
Negative control
The new tests call the new API (SetOwnStoreCompare, the span form of StoreOwnBytes, FrameCaptureReader::Version()). Restoring every production file from main would therefore not compile, so I reverted in three stages.
A. FrameRecorder.hpp, FrameRecorder.cpp and tools/FrameReplay.cpp restored from main (tracker and reader as merged). Six rows failed:
Stage A failures
store_window_unwritten: the store another thread made during a driver store into a page the CPU has not written is in no memory delta and not in the snapshot, not 126
pending_result: the memory delta before the first suspend point read the page before the result pending over it was stored: it holds 0, not 1; the memory delta that added the range committed during the capture read the page before the result pending over it was stored: it holds 0, not 1
calls_v1: the replay restored a calls format 1 capture, which does not record whether GPU writes had landed before its memory deltas were read
gpu_write_before_delta: the replay restored a memory delta while a GPU write over its range was still pending, and the next read landed that write over it: the page holds byte 90, not the 0 the capture recorded after the write
store_window: the store another thread made in the page of a driver store during the store is in no memory delta and not in the snapshot, not 126
once: the page written before the first suspend point is in memory delta groups 1 2 4, not only in group 1; ... the second registration of the shader collected its blocks again
late_clear, store_between_walks and partly_covered pass with main's recorder. This matches the description: main's repeated read covers them, and they guard the new cutoff.
B. Merged recorder, with two tracker terms disabled in GuestMemory.cpp: compare forced to false in storeOwn, and the block-stamp admission in CollectWrittenBlocks removed (if (!cpu) continue;). Four rows failed: store_window, store_window_unwritten, store_between_walks ("has byte 0, not the 107"), and agc_write_tracking ("a store another thread made beside a driver store during the store did not list exactly its page; ... over a driver store's bytes ... was not listed; ... that undid a driver store which gives no bytes was not listed").
C. Two single terms dropped: the keep-guest condition in writeChanged's image update, and || collected in the added-range loop. Two rows failed: agc_write_tracking ("a keep-guest write-back that kept a guest byte was listed as a CPU write") and late_clear ("group 1 of 1 holding it) has byte 90, not the 0").
After restoring with git show 23da7e8b…:<path>, git status --porcelain was empty, the rebuild including libs succeeded, and the focused set passed 28/28 again.
Findings
Blocking
None.
Non-blocking
- The compare switch stays on if
start()throws after turning it on.FrameRecorder::start()callsGuestMemory::SetOwnStoreCompare(true)atcore/libs/prx/libSceAgcDriver/Execution/src/FrameRecorder.cpp:548. It then creates the writer at line 553, and that can throw: FrameRecorder.cpp L547-L553.FrameCaptureWriter's constructor throws when the capture directory cannot be created (FrameCapture.cpp L202-L205). On that path neitherstop()norfinish()runs. The process-wide switch then stays on with no capture, so every driver store from the CPU keeps copying its pages under the tracker lock and noting pages inwindowPages. Onmainthis throw left no global state behind.- Suggested fix: turn the switch on right after the writer exists and before the reset collects at line 556. That keeps the order the description asks for (on before the reset collects). Alternatively, use a guard that turns it off when
start()exits by an exception.
- Suggested fix: turn the switch on right after the writer exists and before the reset collects at line 556. That keeps the order the description asks for (on before the reset collects). Alternatively, use a guard that turns it off when
Not verified
- The Windows-only rows (
fresh_note,agc_write_tracking_fresh_view, theown_store_imagesregistration) and any Windows-only paths of the store compare (the section view and fresh-collect cases): this host is Linux. - The Persona 5 Royal capture sizes (1,114.4 MiB on
mainagainst 610.6 MiB here) and the--strictreplays: I did not launch any title. - The capture-time cost of the store copies, and the memory
windowPagesuses in long runs, beyond the author's single-run figures. - The validation comparison covers 770 tests. None of the focused frame capture tests creates a Vulkan instance, so the layer had nothing to check for them, as the author also notes.
- Behaviour on hardware GPUs: lavapipe only here. Two contributor CI runs on this head, AMD RX 6300 (RADV) and NVIDIA RTX 2000 Ada, reported 0 new failures and no new validation IDs.
GuestMemory's storeOwn keeps main's complete flag next to this branch's imaged flag and store-image span. WriteChangedIfHeld, new on main, stores through the form that gives no bytes: its compare-exchange write-back stops at the first byte it loses, so while a capture records, every page its second walk takes is listed, as for MEM_SEMAPHORE packets and buffer copies on the CPU. Co-Authored-By: Claude <noreply@anthropic.com>
…us delta's pages again Keep the shader marks across full deltas, as boykopovar#14137 does. A mark is the generation the walk of exactly its span returned (or the one read before the walk when the span holds a page the driver stored into), so every page of the span stamped at or below it was collected; a full delta never lowers that. For a span in a range full deltas collect, the piece slots a full delta sets are already at or above every older mark. A readable range full deltas do not collect has no slots, so its span started again from the capture-start floor after every full delta and collected every page written since the start again; it now starts from its own mark. Co-Authored-By: Claude <noreply@anthropic.com>
…rite Back to 78b9590's checkRanToEnd. headerStrings comes from the old PR 3 stack under this branch (20dfa19); main, boykopovar#12568, boykopovar#14137 and PR 3's current heads (replay-bench/pr3-rw3 3366c14, pr3-rw4 88fb93b850) do not have it, so the split's test commits would not compile there. The text search needs only FrameCaptureReader::Header(), and it is exact for headers the recorder writes: jsonString escapes every quote inside a value, so ["stopped: and ,"stopped: after "warnings":[ can only begin a warning. Co-Authored-By: Claude <noreply@anthropic.com>
|
Merged @XairRrR thank you for the Linux run, which covers the capture rows I couldn't run here. AI-assisted: yes (Claude Code) |
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — approved
The only change since my last review is the merge of main 1c8c0efc8c (ca4553365f). It resolves the storeOwn conflict with #13021 correctly. The focused set passes on the merged tree, and the negative control still makes the tracker rows fail. One non-blocking finding from my last review is still open.
What changed since my last review
- The range-diff shows commits 1–14 unchanged (
f0ef36adee…702492e63e). The only new commit is the mergeca4553365fofmain1c8c0efc8c. - The merge resolution is in
GuestMemory.cpp:storeOwnnow takes this PR'simagedflag together with #13021'scompletedefault argument.WriteChangedIfHeld, which is new onmain, stores through the form that gives no bytes (storeOwn(address, size, false, …)atcore/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp:3170). That gives it an empty image.WindowCopy::Holdsreturns false for an empty image, so while a capture records, every page its second walk takes is listed. This matches the description and the debt entry: a compare-exchange write-back that can stop at the first lost byte has no complete image to compare against, so treating it like the MEM_SEMAPHORE and CPU-copy stores is the conservative choice.
- The description and a comment now report results for this head (author-reported, Windows / RX 6700 XT).
- No test file changed in the PR's own commits.
What I ran
Head ca4553365ffc41b27ee5aed87322616b071db376, main 1c8c0efc8c41314402e6966da5bef6ea3d639d10, merged tree 0d003ec7f41be0a5c2e7c456ca988f6c2878f520. Debian 13 VM, Xeon Gold 6136, GCC 14.2.0, Release, lavapipe, no GPU.
git diff --check 1c8c0efc8c41314402e6966da5bef6ea3d639d10...ca4553365ffc41b27ee5aed87322616b071db376
python3 tools/check_conventions.py --base 1c8c0efc8c41314402e6966da5bef6ea3d639d10
git diff 1c8c0efc8c41314402e6966da5bev... 0d003ec7f4... -- GuestMemory.cpp GuestMemory.hpp
ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "frame_(replay|capture)|^agc_write_tracking|^agc_driver_native_multisample_depth$"
# negative control: compare forced to false in storeOwn
ninja -C build -j32 -l32 agc_driver_frame_replay_state_tests agc_write_tracking_tests
ctest --test-dir build -j8 --test-load 32 -R "agc_driver_frame_replay_(store_|once)|^agc_write_tracking$"
# restore with git show 0d003ec7f4…:<path>, then
ninja -C build -j32 -l32 agc_driver_frame_replay_state_tests agc_write_tracking_tests libs
ctest --test-dir build -j8 --test-load 32 -R "agc_driver_frame_replay_(store_|once)|^agc_write_tracking$"
git status --porcelain
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
Configure, build, libs (script) |
passed |
| Selected tests (script): 2245 run | 0 new failures, 0 timeouts. The 7 known failures also fail on main |
| Validation (script): 784 tests, layer loaded in 783, missing in none | 1 test with new VUID- lines, agc_driver_image_gather_volume. That test came in with main's side of the merge (ImageGatherVolume.cpp is not in the PR's files) and is marked "test not run on main" because the baseline 1e658521a0 is older than main. It is not counted against this PR |
Focused set, including agc_driver_native_multisample_depth (a WriteChangedIfHeld caller) |
29/29 pass |
| Negative control after the merge | 4 rows fail, as expected |
| Restore | worktree clean, rebuild including libs OK, 6/6 pass |
Negative control (compare forced off on the merged tree)
agc_driver_frame_replay_store_window_unwritten ...***Failed
agc_driver_frame_replay_store_between_walks ......***Failed
agc_driver_frame_replay_store_window .............***Failed
agc_write_tracking ...............................***Failed
33% tests passed, 4 tests failed out of 6
Previous findings
- Still open (non-blocking): the compare switch stays on if
start()throws after turning it on.GuestMemory::SetOwnStoreCompare(true)still runs atcore/libs/prx/libSceAgcDriver/Execution/src/FrameRecorder.cpp:548, beforestd::make_unique<FrameCaptureWriter>(directory)at line 553, and the writer can throw: FrameRecorder.cpp L547-L553. The fix suggested last time still applies: turn the switch on after the writer exists and before the reset collects, or add a guard that turns it off whenstart()exits by an exception.
New findings
Blocking
None.
Non-blocking
None.
Not verified
- The Windows-only rows (
fresh_note,agc_write_tracking_fresh_view, the separateown_store_imagesregistration): this host runs Linux. - Capture sizes and
--strictreplays on a title: I launched no title. - The capture-time cost of the full listing of
WriteChangedIfHeldpages. It is not measured anywhere. - Hardware GPUs: I ran on lavapipe only. The contributor CI runs on AMD and NVIDIA are for the previous head
702492e63e.
…a's pages again A memory delta kept the write generation it read before its write-watch walk and used it as the next delta's floor. The walk raises the generation before it stamps the pages it finds written, so the next delta collected every page the previous delta had already written. Each 64 KiB piece of a captured range now keeps the generation its own collection returned, read under the tracker lock right after that piece's walk, and the next delta collects only the pages stamped after it. The capture-start floor is read under the tracker lock too. A piece whose walk lists a page the driver stored into since the previous full delta's walk keeps the generation read before the walk, so the next delta collects again every page of that piece whose CPU write this walk found. A replay restores a delta while a submission still waits on a gate the delta holds, so only a later delta has the page as it is after that submission's writes. The window starts at the previous full delta's walk, not at the last delta's read, so a shader registration between the driver store and the next full delta does not close it. A shader delta's span starts from the lowest floor of its range's pieces and keeps its own mark across full deltas, used again only for a span the earlier walk covered. A readable range full deltas do not collect has no piece floors, so its marks are its only floor above the capture start. A delta read with submissions in flight still arms its pages, but a later delta no longer collects all of them again, so most of these owners live until the capture ends, where they are checked and flushed, and a driver store without a serial into such a page stops the capture. Known loss, as before this change outside one delta: a title store that lands inside a driver store's own collect (StoreOwnBytes, WriteChanged) gets only a driver stamp, and no delta records it until the title writes the page again. Also as before this change: a page the driver stored into and the title then overwrote is collected once more only when no other collect took the title's write before the delta. A later driver store's own walk over the same page, or a queue worker's collect, stamps that write first, and the next delta skips the page. The start-floor race has no deterministic test. Port of boykopovar#14137 (cb90a3c) onto the per-piece write-watch walk. Co-Authored-By: Claude <noreply@anthropic.com>
The delta that adds a range the registry gained during the capture read it without collecting it and left out its all-zero 64 KiB pieces, so the next delta collected again every page the CPU had written there before the add. That delta now collects each watched piece of a range deltas collect before reading it, takes the piece's floor from that collection (or, when the piece holds a page the driver stored into since the previous full delta's walk, from the generation read before the walk), and writes the piece even when it is all zero, because no later delta records what the CPU wrote there before the add. Unwatched pieces keep the zero skip, which differs from boykopovar#14137, which writes every block. Port of boykopovar#14137 (c7a4fc9) onto the per-piece write-watch walk. Co-Authored-By: Claude <noreply@anthropic.com>
…frame capture checks pending_result: a delta reads its pages through the coherent path, so a result pending over a page is stored before the read. partly_covered: a page written before the capture, in a shader block the captured ranges cover only in part, is in no delta, and a page written after the capture starts in the same block is in the registration's delta, so the first check cannot pass on a capture without a shader delta. Both pass on the previous recorder. Co-Authored-By: Claude <noreply@anthropic.com>
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (8 IDs in 541 tests)Skipped (49)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. |
What
docs/dev/BUILD.mddescribes each memory delta of a frame capture as the guest pages the CPU wrote since the previous call that handed work to the GPU. Onmaineach delta also collects again the pages the previous delta collected (line numbers onmain91ecd2d974):emitDeltagives every rangesince = generation, readsmark = WriteGeneration()before the walk and setsgeneration = markafter it (FrameRecorder.cppL329-334).CollectWrittenBlocksraises the tracker generation and only then stamps the pages its walk finds (GuestMemory.cppL1681, L1608-1610). So every page a delta's own walk reports carries a stamp newer thanmark, and the next delta lists it again.markthe same way (L401, L409) and start fromgeneration(L405), andemitDeltaclears their marks (L335). A page a full delta collected is therefore collected again by the next shader delta of its 64 KiB block as well.Each repeat is read and written again, and a delta made only of repeats still waits for the driver to go idle before the call it precedes (L422), so that call is recorded with nothing in flight. In a Persona 5 Royal capture made on
mainat86fb985abb(the delta bookkeeping is unchanged since), the second full delta (group 47) holds every page of the first, 2,610.9 MiB by the chunk headers.This change:
keeps, for each watched or unwatched range, the generation its last
CollectWrittenBlocksreturned (read under the tracker lock after the walk), and starts the range's next collect there.start()takes these values from its reset walks;keeps, for each shader block, the generation its last collect returned. A block lying wholly inside those ranges starts from the newer of that value and the oldest value of its ranges, so it skips what their full deltas took. Any other block starts from its own value, and the first time from the tracker generation read right after the reset walks (
GuestMemory::TrackerGeneration), so it never lists a page stamped before the capture started;collects a watched or unwatched range the registry gained just before reading it, so the next delta takes only what was written after that collect, and writes every 64 KiB block of such a range in the delta that adds it, zero or not.
mainwrites only the nonzero blocks there and records what the CPU wrote before the add, a clear included, in its next delta. That delta now starts after the add's collect, so a skipped all-zero block would keep what the replay already holds at its address (found by @chitters2004;late_clearbelow);reads every delta through
GuestMemory::ReadCommitted, as the snapshot already does. A delta read onmainusesReadCommittedUnsynced, so it can miss a result the driver still keeps on the GPU for that page (pending_resultbelow). Since the next delta no longer reads the page again, the read now stores those results first;has the replay land those results too:
agc_frame_replayreads each delta chunk, callsAgcDriverPrepareCommandWrite_nid_no_patchover its range and then copies it, the order fix(agc): read capture deltas and replay them while queues wait on the title #12567'srestoreGroupuses.calls.binbecomes format 2 (aps5-frame-calls 2);FrameCaptureReaderstill reads format 1 and reports the format (Version()), and the replay refuses a format 1 capture before it restores any memory, since such a capture does not record whether GPU writes had landed before each delta was read. With theReadCommittedread in the bullet above, these are the four items @chitters2004 asked for on fix(agc): read capture deltas and replay them while queues wait on the title #12567 and we agreed on in fix(agc): leave GPU-stored words out of frame capture deltas #12568's thread;closes the two tracker gaps @oneandonlydean named, which
main's repeated read covered:GuestMemory::StoreOwnBytes. While a capture records (the recorder turnsGuestMemory::SetOwnStoreCompareon instart()before its reset collects, and off instop()andfinish()), a driver store from the CPU that gives its bytes (Write,WriteChanged, the guest label store, the span form ofStoreOwnBytes, and a newStoreOwnBytesform whose callback writes the bytes it stores into a span it receives) copies its page-rounded range before its first walk and applies its own bytes to the copy; a page its second walk takes that then differs from the copy, or can no longer be read, is noted, andCollectWrittenBlockslists it. A store that gives no bytes (MEM_SEMAPHORE signal and wait, buffer copies on the CPU, andmain's compare-exchange write-backWriteChangedIfHeld, which stops at the first byte it loses) notes every page its second walk takes. A keep-guest write-back (WriteChangedKeepingGuest, fix(agc): copy only the dwords a shader changed back from a GPU-copied buffer #6185) leaves the guest's bytes in its copy, as its store does. No stamp changes, soUnchangedSince,StoredOver,UnchangedSinceCollectedandChangedBlocksanswer as before, with or without a capture;MarkWrittenfresh collect noted.CollectWrittenBlocksalso admits a 64 KiB block whose block stamp is newer thansince, so such a page is listed: the fresh collect raises the block stamp, not the CPU block stamp the loop checked first. The whole clip of a block is listed only when its CPU block stamp is newer and no page in the clip has a newer CPU note.agc_driver_frame_replay_store_between_walksandagc_driver_frame_replay_fresh_noteare @chitters2004's tests (c9a87b435c, unchanged). His fix (5458ad80b2) is not taken; the reason and the measurements are in the comment with this push.Not changed here:
Unwatch, an import that unwatches it, a failed fresh collect) after a delta collected it is not recorded until the tracker notes the page again;mainread such a page once more in the next delta. A delta's own walk also skips every 64 KiB block that holds such a page, as onmain. Debt entry "Pages that leave the write watch during a frame capture".mainalso recorded it when the previous delta had found the page written. Debt entry "Driver stores during a frame capture".WriteChangedIfHeld) are listed after every such store, and the replay writes the captured bytes there over what its own semaphore or copy stored. How much of the casino capture below they make up, and whether they change replayed waits, is not measured.windowPagesmap in every run; onlyCollectWrittenBlocksreads it, and its memory is not measured.UnchangedSinceCollected, the CPU flag ofChangedBlocksandStoredOverbeside the stored bytes, as onmain. This PR changes only whatCollectWrittenBlockslists while a capture records. Debt entry "CPU stores during driver stores".Write,WriteChanged, the guest label store or the span form ofStoreOwnBytes. On Windows none of them runs anything between its two walks, andWriteChangedand the label store run nothing there on any OS; on Linux the span form and aWriteof 4 KiB or more call the shared-backing writer between the walks, which no row drives. Each store builds its copy from its own input (source, value, current), never by reading memory back.start()turns the switch on after its reset collects instead of before them.main.Side effect: a call whose delta would have held only repeated pages now gets no memory record and no idle wait, so a new capture records it at the depth the title had; a delta made only of pages of stores that give no bytes still gets both. That is not the aim of this PR: #12568 makes every full delta wait for a settled driver while work is in flight, and recording the in-flight depth stays with the follow-up stacked on #12568 (#15136).
Overlap (trial merges of each open PR's head with this head, listing only conflicts this PR adds to what the PR already has with
main91ecd2d974):GuestMemory.cppstruct and write-tracking test registration; the merge ofmainkeeps both sides.tools/FrameReplay.cpp, the mode list of the state tests and the frame capture paragraph ofdocs/dev/BUILD.md. It has the identicalreadGuestchange and already flushes before each delta copy inrestoreGroup; whichever merges second drops its copy.FrameRecorder.hpp/.cpp, the tool, the mode list and BUILD.md. As before, it moves theCollectWrittenBlocksloop behind its settled wait and collects per 64 KiB piece; whichever merges second keeps asinceper collect call set to the value that call returned, gives every piece a starting value from a reset walk, dropsshaderMarks.clear()fromemitDeltaand passes!collected && zerofor added ranges. I'll post a rebase of fix(agc): leave GPU-stored words out of frame capture deltas #12568 onto this PR.restoreGroup, which has no flush; fix(agc): read capture deltas and replay them while queues wait on the title #12567, stacked on it, adds the flush there. Whichever of this PR and fix(agc): release a parked REWIND in frame replay with its whole memory group, and write REWIND control words last #12318 merges second keeps theAgcDriverPrepareCommandWrite_nid_no_patchcall before each delta copy.FrameRecorder::finish().storeOwn's signature next to this PR's newimagedparameter and the compare switch inGuestMemory.cpp.WindowCopy::Holds, where this PR adds the store-image compare.FrameRecorder.hpp, BUILD.md), feat(agc): count replayed frames as presents for the performance trace #13984 (mine;FrameReplayState.cpp), fix(agc): relaunch the frame replay when a host region blocks a captured GPU range #12696 and feat(agc): list a capture's shaders and submissions and trace replay serials #11436 (the mode list) conflict mechanically.main.Tested
Windows 11 26200, Ryzen 5 5600X, Radeon RX 6700 XT (gfx1031), AMD driver 32.0.21045.16010, WinLibs GCC 15.2.0 r7, Release,
BUILD_TESTING=ON, full builds with submodules at their pins:main91ecd2d9743b831b575d293e6c8fafbe737e1e88(merged into this branch) and this PR's head702492e63e.This head (
ca4553365f) mergesmain1c8c0efc8cto resolve the conflict with #13021 inGuestMemory.cpp:storeOwnkeeps #13021'scompleteflag next to this PR'simagedflag, andWriteChangedIfHeld(new onmain) stores through the form that gives no bytes. On it, the focused set below passes 36 of 36 in 3 of 3 runs (main1c8c0efc8c: 23 of 23),agc_driver_native_multisample_depth(aWriteChangedIfHeldcaller) 1 of 1 in 3 runs on both, the validation-layer run shows 0VUID-lines on both, andgit diff --checkis clean. Every other result below is from the previous head702492e63e(main91ecd2d974merged), whose capture and tracker code is the same apart from that store.Modes of
agc_driver_frame_replay_state_testsdriveFrameRecorderdirectly and need no GPU. From the previous revision:agc_driver_frame_replay_oncewrites a page before each of two suspend points and commits a range during the capture. Between the suspend points it writes a page in the second of the two 64 KiB blocks a shader header marks, and registers that shader twice; the header shares the first page's block. The page written before each suspend point, and the written page of the added range, must each be in exactly one memory delta, the one before their suspend point, and the second registration must collect nothing. Every page of both ranges is written before the capture or before the add, so no snapshot or delta read is a page's first access.agc_driver_frame_replay_pending_resultinstalls a flush hook that stores a result word over a page the CPU wrote, standing in for a storage result the driver keeps on the GPU until the page is read. The delta before the first suspend point, and the delta that adds a range committed during the capture, must each hold the stored value.agc_driver_frame_replay_partly_coveredwrites a 16 KiB allocation before the capture and registers a shader whose header lies in it, so the header's 64 KiB block is only partly inside the captured ranges. The written page must be in no memory delta, since the snapshot holds it.agc_driver_frame_replay_late_clear(@chitters2004's test from this thread) starts the capture, then commits a range, writes a page in it, registers a shader whose header lies in the 64 KiB block before that page's block, clears the page and reaches two suspend points. The last memory delta holding the page must hold the clear.New in this push:
gpu_write_before_delta: a delta copy over a GPU write the replay still has pending must hold the delta's bytes;calls_v1: a calls format 1 capture is refused before any memory is restored.store_between_walks,fresh_note(@chitters2004's).store_window: a label store through theStoreOwnBytesform that gives no bytes while another thread writes the same page, then four driver stores with given bytes into pages nothing else writes; the other thread's byte must be in a delta and the four pages in none, and a driver store afterfinish()must not be listed.store_window_unwritten: the same store from another thread into a page the CPU has not written.store_window_stopped: the capture stops when it starts (APS5_FRAME_CAPTURE_MIN_FREE_BYTESabove any free space); a driver store after that must not be listed.agc_write_tracking_fresh_view(Windows): a section view whose first page alone is collected before a GPU write's fresh collect takes the rest; those pages must be listed.agc_write_trackinggains two guards: a page only an import window stamped is listed, a block only a GPU write stamped lists nothing.agc_write_tracking_own_store_images(Windows; on Linux the last check ofagc_write_tracking): a driver store made before the compare is on lists nothing; then with it on, aWrite, a store of given bytes, a label store, a write-back and a keep-guest write-back over a page the guest wrote list nothing; a store from another thread beside the bytes of a two-page driver store lists only its page; one over a driver store's bytes is listed; one that undoes a store which gives no bytes is listed; a listed page is not listed again.Each row, 3 runs per tree:
main91ecd2d974+ the rowsstore_between_walksfresh_notestore_windowstore_window_unwrittenstore_window_stoppedagc_write_tracking_fresh_viewagc_write_tracking_own_store_imagesmain(it callsSetOwnStoreCompareand the newStoreOwnBytesform)Messages on
main(store_between_walkspasses there becausemain's repeated read records the store; it guards this PR's cutoff, aslate_cleardoes):With the three tracker commits reverted on this head (
git revert --no-commitof the keep-guest, store-compare and fresh-collect fixes;GuestMemory.cppthen equalsmain's apart from theStoreOwnBytesform refactor), the same four fail with the same messages, and also:Each term of the tracker change, dropped one at a time (incremental build of the two test executables and
libs), turns a named row red. 31 drop runs (29 distinct terms) on2710134746(D14 and Dkeep again on this head), each red: the window-page note and listing (D1, D2, D19), the held stamp kind (D3, D9a, D9b), the window passed to the second walk (D4), which stores take an image (D5, D6, D20, D21), the store and page offsets into the image (D7, D23), the empty-image check before the compare (D8), each store's own bytes applied to its image (D10-D14), the recorder turning the compare on (D15), the fresh and CPU block-stamp admission terms (D16, D17, and the same two again as TB-no-stamp-admission and TB-no-cpu-admission), the whole-clip fallback condition (D18), the copy itself (D22), the keep-guest rule (Dkeep), the switch forced on, default and setter (Don), the compare forced on outside a capture (Dcmp), and the switch cleared infinish()(Dfin) and instop()(Dstop). For example Dstop:store_window_stopped: a driver store made after the capture stopped was listed as a CPU write; Dkeep:own_store_images: a keep-guest write-back that kept a guest byte was listed as a CPU write.ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 ctest --test-dir build -R "frame_(replay|capture)|^agc_write_tracking|^agc_(resident_direct_remap|rewatched_direct_shared|import_alias_shared|compared_shared_generations)$", 3 runs per build, then one run withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationand-V, a local Vulkan-ValidationLayers build, implicit layers disabled andVK_LOADER_DEBUG=layer:VUID-linesmain91ecd2d974702492e63eNo test in this set creates a Vulkan instance (the loader inserted the layer 0 times in both logs), so the layer had nothing to check here.
ANYPS5_REQUIRE_VULKAN=1 ctest -R "^(agc|shader)_"on the previous local revision2710134746(the same code; this head adds one premise check to a test and rewords a debt entry): 1310 passed, 15 skipped, 3 failed of 1328. The three,agc_driver_direct_write_completion,agc_driver_scratch_poolandagc_driver_sample_order_grid_reference, also fail onmain91ecd2d974on this GPU (3 of 3 runs each).Linux: a WSL2 build of this head (Ubuntu 24.04, lavapipe) compiles, and
gpu_write_before_delta,calls_v1,order,zero,tokens,blocking,unwatchedandagc_frame_capturepass there (3 runs withAPS5_WRITE_WATCH_IMPORTS=unwatch, 1 without).agc_write_trackingskips and the capture rows cannot start: this WSL2 kernel (5.15) has no asynchronous userfaultfd write protection, so the guest write watch is unavailable. The new rows have not run on Linux.Persona 5 Royal (PPSA05110), casino gameplay, shared GPU, one 20-frame capture per build, each started 45 s after the gameplay HUD appeared in its own run of the same route:
main91ecd2d974recorded 1,554 memory groups and 1,114.4 MiB of deltas, this head 692 and 610.6 MiB (capture.jsonmemory_groups,delta_bytes). The runs reached the scene separately, so the title's own work differs between them.agc_frame_replay --strictof each capture through its own build, twice each: 20 frames and 120 submits, 0 unfinished, 0 pacing timeouts, and both reference frames pixel-identical (pixels_identical1.000000).Not tested: macOS (frame capture throws there), a native Linux run, other GPUs. @chitters2004's runs of his two tests on an RTX 3080 (
c9a87b435c,5458ad80b2) and TechLevelZero's NVIDIA CI run ofc7a4fc97bbare on earlier revisions.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code