Repository navigation
Conversation
A depth surface keeps mirroring guest memory after the guest has released and reused it, so a flush triggered by later GPU work over that range stores its pending results into live guest data. A pending fast clear made it worse: the storing synchronisation returned before the guest-write check, so the clear value was written over whatever the guest now keeps there. Record, per plane unit, the backing serial of the allocation registered over it at every synchronisation, and drop a unit whose allocation changed before storing. A storing synchronisation no longer skips the guest-write check when a clear is pending: the units the guest wrote are dropped and the clear is forgotten for the surface. The sampling path that retires a stale plane applies the same unit rule.
NFSS10
force-pushed
the
fix/stale-depth-plane-flush
branch
from
November 6, 2026 13:20
ad72b96 to
5de1309
Compare
Contributor
|
Windows check of head Setup: Windows 11 Enterprise 22621, WinLibs GCC 15.2 UCRT, Release, Radeon RX 6700 XT, AMD driver 26.10.1. Run through the Windows test queue with This PR changes
AI-assisted: yes. |
milescoviello
left a comment
milescoviello
left a comment
Contributor
There was a problem hiding this comment.
Read it against DepthSurface.cpp on main. The unit rule and the storing/non-storing split look right to me. One cost point and one coverage note.
- The refresh runs on every sync, including the draw path.
SyncFromMemorynow refreshesunitBackingwith oneGuestAllocationsLookup_nid_postfixper 64 KiB unit, before the early return. That's one registry lock and oneshared_ptrcopy per unit. It's reached from the depth-target bind (if (mirrorable(target)) surface->SyncFromMemory(...)) and the sampled path, not only from flushes. A 3840x2160 D32 plane is about 500 units, so about 500 locked lookups per bind. Two existing APIs cover it:GuestAllocationsLookupEach_nid_postfixdoes the whole plane under one lock.GuestAllocationsGeneration_nid_postfixchanges on every mapping mutation, so a surface that stores the generation it filledunitBackingat can skip the refresh when nothing was mapped or unmapped since.
On a draw-heavy title the second one makes this free.
- Reuse inside one registration. The debt entry already says it isn't covered. For what it's worth, STEINS;GATE RE:BOOT (PPSA36591, #15350) hits that case with storage images: the game's GPU Heap mspace reuses blocks inside one mapped direct-memory range, so
backingSerialstays the same there. That's a separate fix, not something this PR should take on.
AI-assisted: yes (Claude Code)
Contributor
|
Runs of this PR on two NVIDIA machines,
On the VM the pattern also matched AI-assisted: yes (Claude Code) |
Contributor
|
Linux/RADV validation-layer run of head Setup. CachyOS x86-64, kernel 7.3.1, AMD Radeon RX 9070 XT (RADV GFX1201), Mesa 26.2.5, Vulkan 1.4.363 loader with Khronos validation layer 1.4.363, GCC 16.2.1, Ninja, Release, Results.
The PPSA27616 crash itself remains Windows-verified by your replay runs; this arm covers the depth/storage regression suite on a second vendor with no validation regressions. AI-assisted: yes |
Contributor
✅ 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)Validation IDs already present on main (9 IDs in 594 tests)Skipped (54)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. |
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
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
A depth surface keeps its mirror registered after the guest releases the memory it covers, so a flush driven by later GPU work over that range stores pending results into live guest data. A pending fast clear made it silent: the storing synchronisation returned before the guest-write check, so the clear value (zero) was written over whatever the guest now keeps there. On PPSA27616 the fault read through a TLSF bin whose node sat inside a 64 KiB depth plane (
plane+0x7ff8) while TLSF control pointed at the guest heap.backingSerialof the allocation registered over it at every synchronisation, and a flush drops a unit whose allocation changed instead of storing into it. Per unit, because the rest of the plane can still be the guest's.SyncFromMemory(resource, storing)) no longer lets a pending clear skip the guest-write check: it drops the cleared units and forgets the clear for the surface.retireStoringStale, which retires a depth plane after it was sampled and overwritten, applies the same unit rule.storing=falsekeeps the previous early return.Related: #14969 covers the stencil half of the same bug and #15621 touches
SyncFromMemoryand the plane member block; this change does not include the stencil plane on purpose.Tested
APS5_PAD_INPUT_FILE.mainall 11 runs crashed; with this change none of the 17 did.main(e3a14a86e4andd23df92b38): 11 runs, all fatal,0xc0000005onUnityGfxDeviceWorker,rip 0x1417f2bfb, reading through a TLSF bin whose node is inside a depth plane.e8b1bd3d7f. Counting the two intermediate builds that carry the same depth rule: 17 runs, no fatal.[gpu] depth plane 0x257810000+0x40000 was re-registered with GPU results pending; they are dropped) and the session carries on.ctest -R 'depth|storage|write_tracking|guest_memory': 124/124 on this branch. The same suite passes onmainand on this branch withANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(that comparison ran on the immediately precedingmain,d23df92b38, whoseDepthSurface.cppis identical); neither log containswas not found but was requested by env var VK_INSTANCE_LAYERSand neither contains aVUID-line.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)