Repository navigation
Conversation
6 tasks done
11 tasks done
|
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.
|
Contributor
Author
|
The overlap with #6182 is the option list in the test binary's main (tests/Recorder.cpp), both add a |
11 tasks done
Contributor
Same-value late CPU store: GPU reproductionAt unchanged head Two-line reproducer on our fork; production is unchanged. This commit intentionally fails the test, not a proposed fix. Restoring the original head, rebuilding and rerunning both groups gives five passes each again. No internal skips; each run identifies the RTX 5080. Maximum iteration: 0.69s; same-value case 0.32s. Environment: Windows 11 Enterprise 26200, WinLibs GCC 15.2.0, Release, NVIDIA GeForce RTX 5080, driver 610.62 / 32.0.16.1062. Built the recorder target/dependencies; shader caches disabled, The shader sees Scope: public synthetic test on this PR head, not current-main integration, AMD/Linux/console coverage, or attribution of Sonic's loading stop. AI-assisted: yes (Codex). |
Contributor
Author
|
Thanks, the reproduction is right. A late CPU store of the value the keys already held is indistinguishable from no store at all to the value compare. That case matters: zeroing reused memory that was already zero is exactly how a freed metadata block gets taken over. So preservation has to include it. Pushed eb65a65, which adds write provenance on top of the value compare:
Limits:
In PPSA15304, two runs on top of current main (about 160,000 guarded key stores in the longer one) left no key block to a later CPU store, and neither run had a fault. So in that section the new check never changes what is stored, and it closes the same-value case for titles that do reuse such memory. |
This was referenced Oct 15, 2026
This was referenced Oct 15, 2026
Closed
ee6aba54
force-pushed
the
fix/agc-key-store-guard
branch
from
October 15, 2026 14:08
eb65a65 to
dea2cda
Compare
11 tasks done
…s GPU results A copied buffer's write-back stored every byte the GPU changed against the uploaded copy, even where the CPU had written the same bytes since the upload. The title's completion label is stored on the GPU before the CPU write-back runs, so a title may free and reuse the memory in between; the write-back then overwrote the allocator's free-list link with two dwords of shader output, and the next allocation of that size class faulted (PPSA15304: three general protection faults within minutes of walking, each at a free-list pop, each with a float pair in the popped link). WriteChanged now stores a byte only when the GPU changed it and the CPU has not changed it since the upload, a three-way compare of result, uploaded copy and current guest memory; the storage image, mirror and tiled color target write-backs share it. The test that filled guest memory with foreign bytes before the write-back expected the GPU's bytes to win; it now uploads what it holds, and a new test checks that CPU-changed bytes keep their value while the rest take the GPU's. (cherry picked from commit b7ee3fc376750b57115e0b5c2d0b1ef9d3d9f1f9)
…U-copied buffer A buffer region staged in device memory was copied back whole when its batch completed, so a CPU store the title made into the region meanwhile was overwritten by the GPU's copy of the bytes it had been given. The copy-in now keeps a snapshot of the uploaded bytes, and the copy-back runs a kernel that stores a dword only where the shader's result differs from the snapshot and the import still holds the snapshot's value: a dword the CPU wrote after the upload keeps the CPU's value, as the CPU write-back path does since the previous commit. Unaligned edges copy as before. APS5_NO_CHANGED_COPY_BACK=1 restores the whole copies, APS5_CHANGED_COPY_ALWAYS=1 makes the kernel store every dword, and APS5_TRACE_COPY_BACK=1 counts the dwords the import changed while the shader left them.
…on Windows The test binary needs the libc DLL next to it like the other recorder test modes; without it Windows stops it with 0xc0000135.
…was recorded The "uncompressed" key store recorded behind a storage write-back filled the keys' host import whenever the GPU reached it. A title that had released the metadata block by then and reused it (a descriptor table, for one) got the fill pattern over its own bytes; the next dispatch built from such a V# failed in the recompiler with an unsupported format or type. The queued key store now takes a snapshot of the key bytes at record time and the batch stores the 0xff pattern through the changed-copy kernel, so a dword the CPU wrote after the record keeps the CPU's value. A range whose only pending writers are earlier 0xff fills of the same keys stays guarded (the kernel need not repeat them); a pending kernel of the title writing the keys, or a range without a device address or a snapshot, is filled as before. The [labels] line counts the guarded stores; APS5_NO_GUARDED_KEY_STORE=1 restores the fills.
…hat precedes it The publish of the key range's edge units notes a pending write over the keys, which made the key store queued right after it fall back to the plain fill. The publish leaves the key bytes as they are (its slab holds the seed the import had), so the store's snapshot stays exact: publishes note their writes with their own value and the key store treats them like its own fills. (cherry picked from commit a3c8286502a94b13d2fb7676a76c82e87f677d76)
…ded before it A title's DCC key clear is a recorded fill of a known value over the keys. The key store queued behind it saw a pending writer it did not know and fell back to the plain fill, which was a third of the stores in PPSA15304 after boykopovar#5520 and brought the descriptor faults back. The snapshot now predicts the import: the bytes read at queue time with every pending fill of a known value applied in note order, so the kernel stores the 0xff pattern over the clear's bytes and still keeps a CPU store made after the record. APS5_TRACE_KEY_GUARD=1 prints the pending writers of the stores that stay unguarded. The test records a key clear before the store and expects the keys stored, then a late CPU store kept. (cherry picked from commit 3da4934731c9231976f395b8f517bd2b03578968)
…to the CPU The guarded key store stored the 0xff pattern wherever the import still held the snapshot's value, so a CPU store of the value the keys already held (zeroing reused memory that was zero) read as untouched and was overwritten. The queued store now collects the guest writes of its range before taking the snapshot and keeps that generation. When the store is recorded, every 64 KiB block of the range the CPU wrote since then is left as the CPU left it, and the kernel only runs over the other blocks, so a store is kept whatever value it wrote. Without write watching the value compare stays the only guard. The [labels] line counts the key blocks left to the CPU. The test places the keys across a block boundary and adds a late store of the value the keys held.
ee6aba54
force-pushed
the
fix/agc-key-store-guard
branch
from
October 15, 2026 14:15
dea2cda to
1b755cb
Compare
github-actions
Bot
added
needs rebase
10 tasks done
github-actions
Bot
added
the
unblocks title
12 tasks done
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
After a storage write-back the driver records a fill of the "uncompressed" DCC keys into the keys' host import (
StoreUncompressedOnGpu,Recorder::QueueKeyStore). The fill lands whenever the GPU reaches it. A title that waited for its label, released the metadata block and reused it for something else had the 0xff pattern written over its new bytes. In PPSA15304 the reused block held a descriptor table, and the next dispatch built from it stopped the title in the recompiler:Four commits:
APS5_NO_GUARDED_KEY_STORE=1restores the fills, and the[labels]line counts the guarded stores.APS5_TRACE_KEY_GUARD=1prints the pending writers of the stores that stay unguarded.[labels]line counts those blocks. Without write watching, the value compare stays the only guard.Scope: the guard covers the deferred key stores; the unit shadow publishes (slab to import) stay unguarded, since a kernel over every published dword read the snapshot and the import over PCIe and halved the present rate of PPSA15304. #4794 stores results right behind the draw instead; on this title that mode (then
APS5_EAGER_WRITEBACK=1) ran at 0.3 presents per second against 15 to 20 per 10 s with the deferred path and this guard, so the deferred path stays the one heavy titles need, and this guard keeps it safe where it crashed.Tested
Linux, Ubuntu 24.04, NVIDIA GeForce RTX 5080 (driver 595.99.02). New test
agc_guarded_key_store(a key clear recorded before the store: the keys end up stored; CPU stores made after the record survive, including one of the value the keys held, and the keys of a block the CPU left alone are stored).ctest -R agc_passes. PPSA15304 on currentmainwith #6185 alone stopped three times after 10 to 11 minutes in the section after the first drop with the descriptor fault above; with this change two runs of 20 and 25 minutes of continuous movement showed no fault, with 99.6 percent of the key stores guarded. Upstream'sAPS5_STORE_AT_LABEL=1did not prevent the fault in the same section.Checklist
main; no other open PR implements the same functions.mdfiles or images added to the repository