Repository navigation
fix(agc): save the guest bytes of pending storage results when their range leaves the write watch - #5657
Merged
oneandonlydean merged 2 commits intoOct 14, 2026
Conversation
…range leaves the write watch On Windows, singly mapped direct memory is private since boykopovar#5596, so its host import is a direct import that leaves the write watch instead of a shared alias that stays watched. A storage image whose results were pending when its memory was imported had no saved guest bytes, and storing them threw. GuestMemory now calls a hook before Unwatch and before an ImportWatched that does not stay watched. Every image with results pending over the range saves the guest bytes of its pending units that have none, then collects and records the pieces the keep rule leaves to the CPU for the writes the watch saw. The next store and Refresh leave those pieces to the CPU and compare the rest with the saved bytes. A map-time import made without the GPU lock is deferred to the locked retry when pending results overlap the range.
|
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.
|
…e the range leaves the write watch A draw made outside the GPU lock (agc_driver_recorded_kept_bytes) imports a target's memory and leaves the watch there; the hook threw instead of saving. It now takes the lock, as FlushPending does.
Contributor
|
@oneandonlydean ran PPSA15657 on this PR as asked: Windows 11, RTX 4080, head db4fd5f merged into
One run only, so I can't say whether it comes later than without the PR. The log is available if useful. AI-assisted: yes (Claude Code) |
Collaborator
Author
|
@johnmaia thanks for running it. The fix in #5657 only covers ranges that leave the write watch through a host import or and attach the last ~300 lines of the log before the throw? We need the |
Merged
11 tasks done
11 tasks done
Contributor
|
@oneandonlydean ran it again with The flush trace has no line for the image that throws: the last log (300 lines)The full log is 1.4 MB (150 KB gzip). I can send it however suits you. AI-assisted: yes (Claude Code) |
11 tasks done
Collaborator
Author
|
Thanks, the trace helps. Every image it stores is at generation 0, so their memory was already outside the write watch when their results were written: this isn't a range leaving the watch with results pending, which is all #5657 and #5671 handle. The remaps and retired imports before the throw don't drop the watch either. What does leave pending results on such memory without saved bytes is an image copy or fill into memory another image overlaps: it dropped the saved bytes of every unit of that other image, including units it never touched that still had results pending, and the next store of those throws. #5707 limits that to the units the copy or fill covers. Could you try a build of |
Collaborator
Author
|
@johnmaia #5761 is |
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
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
Fixes the regression reported in #361 for Disney Epic Mickey: Rebrushed (PPSA15657). @johnmaia bisected it to #5596: on Windows
mainthe title stops during the level load or before the main menu's Continue withand the same build with #5596 reverted stays in the level for 10 minutes (comment).
Since #5596, singly mapped direct memory on Windows is private memory.
importAllocationfinds it writable and imports it directly (GuestBufferMemory.cpp:894), anddecideImportWatchmakes every direct import on Windows leave the watch (:542,:933→GuestMemory::ImportWatched(..., false)→unwatchLocked,GuestMemory.cpp:1223). Before #5596 the same memory was a shared view, imported throughMapAlias, and stayed watched. A storage image uploaded while the range was watched saves no guest bytes (Texture.cpp:2163,:2212,:3835,:3970). If its results were still pending when the range was imported, its next store found the range neither watched nor saved, which is the throw #5024 left (Texture.cpp:4339,:4486).This saves the bytes at the moment the range leaves the watch:
GuestMemory::SetUnwatchHook: called beforeUnwatchand before anImportWatchedthat does not stay watched, while the range's write stamps are still current.StorageTexture::SaveBeforeUnwatch: every image with results pending over the range reads the guest bytes of its pending units that have none, then collects the range. It records the pieces that the keep rule ofwriteBackLayersleaves to the CPU for the writes the watch saw, with the unit's generation.Refreshcounts them as CPU-written.ImportMappedRanges) is deferred to its locked retry when pending results overlap the run, as it already is when it would retire an import. Without the lock, the hook throws.What still throws is in the debt entry: units without a write generation, and ranges that leave the watch through a failed collect.
Tested
Linux (CachyOS, GCC 16.2.1, RTX 4090), Release,
BUILD_TESTING=ON, onmaine33258a:agc_driver_store_over_untrackedhas three new cases: the range leaves the watch throughUnwatchand through anImportWatchedthat does not stay watched, with the CPU write after the transition, and with the CPU write before it. Onmainit throws the message above. With this PR it passes in 3 of 3 runs. Without the recorded pieces, the write-before case loses all 4096 CPU-written bytes.ctest -R "guest_memory|agc_driver_graphics|storage|write_watch": 18 of 18 pass.ctest -R "agc_|guest_": 854 of 855 pass.agc_driver_bc_block_copyfails the same way on unmodifiedmain.tools/check_conventions.py --base origin/mainpasses.Not run on Windows or with PPSA15657. @johnmaia, could you run the title with the same input sequence?
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)