perf(libSceAgcDriver): keep storage surfaces tracked around unwatching host imports

[Mouad-Scriptz-0]

What

On Intel's i915 a VK_EXT_external_memory_host import resolves the guest write watch of its pages (the start-up probe reports "host imports resolve write protection: yes"), so the driver stops trusting the watch for every imported range and a storage surface in such a range compares its whole guest copy with its snapshot at every use. Minecraft (PPSA17221) keeps about 250 such surfaces, atlases of 4 MiB among them, and its render thread ran at 89 % CPU with every stack sample in StorageTexture::Refresh → compareUntracked → memcmp, with the GPU idle. Three causes stacked up:

• An import spans the whole run of adjacent allocations inside the 8 MiB import window, so importing one 64 KiB vertex buffer unwatched every texture allocated next to it. On a device whose imports unwatch their range the window is now one allocation (shrinkImportWindow, 64 KiB): a buffer's import leaves its neighbours watched. The existing window test keeps the 8 MiB behaviour for other devices; agc_driver_bda_resources gains unwatchedImportWindowTests: after SetImportWatch(Unwatch) an import covers the allocation it serves, a view across two allocations is refused, and the import is reused.
• A storage surface imported its own range for GPU-direct uploads and write-backs, unwatching itself. StorageImportFor uses an existing import but does not make one for a range the watch still covers on such a device (ExistingHostImport, HostImportsUnwatch); the staged paths serve the upload and clear sites. A refresh with a pending alias, a copy source and a write-back still import: the device path needs the import, and the staged upload's CPU read would flush the alias's results instead of borrowing them.
• A surface whose range is unwatched compares its guest copy at every use, since nothing else can see a CPU store there; the cost is attacked from the other side: an unwatched import that no lookup used for sixty frames retires at the flip and its range returns to the write watch, its blocks stamped as possibly written like the import window stamps them (RetireIdleImports, GuestMemory::Rewatch); a surface without a guest copy under a range an import unwatches takes one at once (StorageTexture::NoteRangeUnwatched). Minecraft imports a texture's allocation only while its compute uploader writes it through a buffer binding; two frames later the range is tracked again and the comparisons end. A range the GPU writes every frame stays imported and compared. The retirement runs only on devices whose imports unwatch their range. agc_driver_graphics gains idleImportRetireTests.
• refreshImports re-unwatched every unwatched import at every registry generation change (200,000 Unwatch calls in about a minute of Minecraft); it now checks GuestMemory::Watched first.

Tested

Fedora 44 (kernel 7.2.8, GCC 16.2.1, Ninja, Release, BUILD_TESTING=ON, Intel Arc Meteor Lake, Mesa 26.2.3), worktree on main 33ae282: agc_driver_bda_resources passes with the new case; tools/check_conventions.py --base origin/main and git diff --check pass. Minecraft (PPSA17221): the render thread no longer spends its time in the untracked comparisons (the 250 surfaces were in the neighbours of imported buffers); the home screen runs at the same frame rate as before the PR once an unrelated in-place vertex input sync is out of the way (separate PR), 60 fps on this device. Not tested on Windows, where imports are compared by design and the window change does not apply unless the probe says so.

Checklist

• Based on current main; no other open PR implements the same functions
• One topic per PR; follow-ups go in a new PR
• No comments in code except technical debt
• Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt
• No notes, investigation .md files or images added to the repository (attach them to this PR)
• New third-party code is a submodule built from source
• Generic behaviour, not specific to one title
• Depends on: none
• Unblocks: PPSA17221 (no Game bring-up discussion yet)
• AI-assisted: yes (Claude Code)

🤖 Generated with Claude Code

[Scramblrrrr]

Windows/007 cross-test planning: the import-window and storage-import changes look relevant to the comparison hot spot. Before carrying the frame-scoped memo, could you add a regression for an unwatched surface changed by the CPU between two synchronized uses in the same frame (no flip)? A completed first draw/dispatch, CPU store, then a second draw is ordered, so it need not race the console; a frame serial alone cannot validate the old snapshot there. The new stamp also retains only ForgetSerial parity, so a completed forget/remap within that frame deserves an invalidation check. I have not built this head or reproduced a failure yet; these are source-review concerns, not measured Windows results. Preparing a focused comparison fixture while my current heap-corruption repro runs. AI-assisted: yes (Codex).

[Scramblrrrr]

Focused Windows regression now available: Scramblrrrr@c4981fb (fixture only, older integration base with #3991). After a completed write-back, Refresh + a readback submitted and waited populate/observe the untracked snapshot; a CPU memset follows; another Refresh + waited readback must see 0xabababab. No flip and no concurrent GPU use. Existing policy passes (0.36s); replacing only compareUntracked's stamp/cacheable expressions with your new policy, using constant frame serial1 because this fixture never flips, fails: untracked comparison missed a synchronized CPU store before a flip (CTest exit8,0.37s). Restoring policy passes (0.37s). RTX5070Ti, Windows, GCC15.2, native Vulkan; no skips. This is an isolated policy probe, NOT a build of full4448 and NOT a game FPS result. No failing production mutation left in the branch; no title data in this fixture. Happy to cross-test a refined invalidation policy. AI-assisted: yes (Codex).

[Scramblrrrr]

Reviewed updated headbc616d758e740d0e6b3febee5d0936508a3afff8: the unsafe frame-scoped stamp is removed and the original comparison stamp restored, addressing the isolated policy regression I reported. The idle-unwatching import retirement/capture-on-unwatch approach needs its own lifetime and invalidation cross-test; not claiming the new full head tested or a Windows FPS gain. Currently running the 007 hull-v1 cross-test, so will keep this out until that completes. Thanks for retaining detection of ordered CPU stores. AI-assisted: yes (Codex).

[Mouad-Scriptz-0]

Both points are right, and the fixture shows it well. The frame-scoped memo was a heuristic: for an unwatched range nothing can see a CPU store, so a comparison at every use is the only correct policy, and I had argued a race where an ordered store (completed use, store, second use) is legal. The existing agc_driver_dcc_keys_first_write untracked-reuse case checks the same contract (refresh, CPU edit, refresh must see it, no GPU work between) and would have failed too; I had only run the graphics tests. The forget-serial parity has the same flaw: a completed forget inside the frame kept the parity and the stale snapshot.

Commit 1b6c33d drops the memo and restores the original stamp, and attacks the cost from the other side instead: an unwatched import that no lookup used for two frames is retired at the flip and its range goes back under the write watch, with its blocks stamped as possibly CPU-written so a surface there compares once and is tracked from then on (RetireIdleImports, GuestMemory::Rewatch). With the per-allocation import window and StorageImportFor from this PR (now only at the sites that upload from CPU-written memory: a surface with a pending alias, a copy source or a write-back imports as before, since the device path needs the import and the staging path's CPU read would flush the alias), a Minecraft texture is imported only while the title's compute uploader writes it through a buffer binding; two frames later the import retires, the range is watched again, and the per-use comparison ends with it. A range the GPU writes every frame stays imported and compared, correctly. The retirement runs only on a device whose imports unwatch their range (the probe's verdict), so Windows and devices with watched imports keep today's behaviour.

Your fixture passes unchanged under this policy, since the compare path is the original one. One more hole closed on the way: a watched surface created without a guest copy had nothing to compare against once a buffer import of its allocation unwatched it, so the import now reports the range and such surfaces take their copy at once (StorageTexture::NoteRangeUnwatched), which agc_driver_dcc_keys_first_write checks. agc_driver_bda_resources gains idleImportRetireTests: an import used this frame or one frame ago stays, one idle for sixty frames retires, its neighbour stays, the range is watched again when the write watch is available, and it can be imported again. agc_driver_storage_one_slice_alias now states the contract of an unwatching device explicitly: the watched block stays unimported and the alias writes back, where before it imported the block and skipped the check. Measured here on the Minecraft home screen: the retirement at two frames ping-ponged (80 imports and 80 retirements in 30 s, each re-stamping and re-snapshotting the range), so the bound is 60 frames now, and the rewatch stamps the range as possibly written like the import window does, since a definite CPU stamp made every surface with pending results write back. With that the frame rate matched the build before this PR; the gap I had chased (60 to 42 fps) turned out to be unrelated, an in-place vertex input waiting on the CPU for a compute write every frame, which I'm sending separately.

I'd welcome the cross-test on the RTX 5070 Ti; the retirement should be inert there (host imports keep their watch on NVIDIA by the probe), so the interesting check is that nothing else moved.

[oneandonlydean]

On Windows decideImportWatch sets unwatchImports = true for every device, without a probe, and RetireIdleImports and StorageImportFor (through HostImportsUnwatch) key on that flag. So both act there too, on the imports Windows compares, although the description says Windows keeps today's behaviour, and it wasn't run there. Please limit both to devices where the probe found that imports resolve the watch.

Three more things:

• The branch is still on 33ae282, before perf(agc): memoize untracked texture comparisons by the stores that reach the surface #2802, which keys the compareUntracked memo on the stores that reach the surface, for the same Intel compare hot spot. Please rebase and say what is still needed on top of it. It conflicts now anyway: docs/dev/TechnicalDebt.md was split, so the entry goes under docs/dev/debt/.
• The description now says the frame rate is the same with and without the PR. Perf PRs need a before/after for the change itself (perf(shader): cut hot-path overhead in recompile cache lookups #218): render-thread CPU or presents per 10 s, main against main plus this.
• The debt entry still says imports retire after two frames; IdleImportFrames is 60.

[oneandonlydean]

Conflicts with main in PacketExecution.cpp, GuestBufferMemory.cpp/.hpp, tests/BdaResources.cpp and docs/dev/debt/libSceAgcDriver.md after #3874, #4428 and #4515; please rebase. The probe-only limit and the debt entry are right now. The before/after for this change alone is still needed (#218).

[blawesom]

Status on the requested rebase — I attempted it and got partway, but I'm not posting a handoff branch because the result isn't correct yet. Details so the next attempt (mine or yours) starts from the real map:

Commit 1 (storage surfaces around unwatching) merges cleanly in spirit: main's newer guards kept, this PR's Watched()-check before unwatch, and the test families both kept. leasedRangeAt maps onto main's leasedRangeOwner (shared_ptr form).

Commit 2 (retire idle imports instead of memoizing) is the hard part: it restructures HostImportFor around frame-serial tracking (lastUseFrame) where main has since landed the LRU useCounter hit path inside the same function, and adds RetireIdleImports with the IdleImportFrames/PendingWriteSettled logic. My assembled merge builds, but three tests fail on it — agc_driver_graphics, agc_driver_host_import_windows (fast assert), and agc_driver_recorded_kept_bytes (timeout — likely a hang in the restructured path) — so the interaction between the two hit-tracking designs needs the author's pass, not my guess at which semantics win.

If it's useful I can push my work-in-progress branch for inspection, or take another run at it with the author's input on the hit-path semantics.

[blawesom]

Second rebase attempt — two of the three failures root-caused and fixed, one remains, branch pushed for inspection.

Following the hint that the retire-idle commit needs to fit main's LRU hit path: HostImportFor now keeps main's useCounter hit path intact and layers lastUseFrame stamping on both the hit and miss paths, so retirement works alongside the LRU rather than replacing it.

Fixed 1 — agc_driver_host_import_windows: the PR's keep condition in the refreshImports walk gates on a lease range starting at the import's base (leasedRangeAt), which predates main's window-ownership model — mid-range windows get retired and re-created, changing their serial (exactly what WindowsFollowTheirRange checks). Resolution: the walk keys on main's sameRange (owner-identity for partial windows), with this PR's Watched() guard and unwatched-notes layered inside it.

Fixed 2 — agc_driver_recorded_kept_bytes hang: the PR's StorageImportFor watched path routes through ExistingHostImport, which never calls ensurePinWaiter() — so when its refreshImports creates imports (which pin their ranges), a later guest free of those ranges spins forever in RequireUnpinned with no registered waiter. That looks like a genuine bug in the PR, exposed by main's newer test; fix is ensurePinWaiter() at the top of ExistingHostImport.

Remaining — agc_driver_store_over_untracked: "the pending result had saved guest bytes while its memory was write-watched". The test was substantially reworked on main after this PR branched (the PR passes its older version). The creation-path note (if (entry.unwatched) unwatchedNotes.emplace_back(...)) is unchanged from the PR, and gating the walk's note on range-start alignment doesn't clear it — so the question for the author is which note/capture conditions must hold for window imports under main's keep model: the notification fires for a surface whose memory is still write-watched at the test's check.

Current state on the branch: builds, conventions pass, 1284/1287 — store_over_untracked plus the known main regression (agc_driver_tessellation segfault, #4727). Branch is blawesom:rebase/4448 (2ea0a8ad8, your two commits, authorship unchanged) if it's useful as a starting point; the two fixes above are folded into the conflicted commit.

[oneandonlydean]

The second pass fixes agc_driver_host_import_windows and the agc_driver_recorded_kept_bytes hang, and leaves one failure. The hang looks like a real bug in this PR: StorageImportFor goes through ExistingHostImport, which never calls ensurePinWaiter(). @Mouad-Scriptz-0, can you take that branch, fix the remaining failure and post the before/after for this change alone (#218) from the 10-13 ask? If you'd rather not carry it, say so.

[Mouad-Scriptz-0]

Taken. The PR head is now @blawesom's rebase/4448 (2ea0a8ad8, both commits with authorship kept) plus one commit, 924aaa4bf:

• The store_over_untracked failure blawesom left is the staged upload. StorageImportFor keeps a watched surface out of its own import on a device whose imports unwatch, so such a surface takes the staged upload, and that path marked the guest snapshot valid unconditionally (layerOriginal.assign(trackedLayers, true)) where the direct path marks it valid only when the surface is not write-watched. The pending result then carried saved guest bytes while watched, which the test rejects. The commit makes the staged path follow the direct path's rule. Traced with gdb: no captureGuestBytes call touched the failing image; the flag came from upload alone.
• The rebase had kept the superseded debt entry (the per-frame compare of the first commit) next to the current one; the duplicate is dropped.

One failure remains in the same test, on this machine too (Fedora 44, Intel Arc Meteor Lake, Mesa 26.2, where the probe reports that imports resolve the write watch). The three single-surface leave-the-watch cases print "not run, the surface is not write-watched", since the write-back's HostImportFor still imports and so unwatches the surface. The fourth, "a surface whose first block left the write watch with the page before it", now fails one check later: the result did not reach a block the CPU left alone. With APS5_LOG_SYNC=1 APS5_TRACE_FLUSH=1 and gdb breakpoints:

1. the fixture's Settle saw AnyPending() true and flushed two images still pending from the previous case (their memory already unregistered), not this surface, whose image the driver thread created afterwards; so the store's result was never settled;
2. the Read at +0xe000 wrote the image back: keeps 1 CPU-written 64 KiB blocks (first stored byte at +0xf000, 1 ranges stored), through an import HostImportFor created inside writeBackLayers (the SaveBeforeUnwatch hook and NoteRangeUnwatched ran from there);
3. after FlushAllPending, every block of the surface still holds the untouched fill (0xdeadbeef), so those stored bytes never reached guest memory;
4. the fill's FillClear ran on the driver thread after that write-back and left the image pending at destruction.

So the path this PR newly takes on such a device, a watched surface uploaded staged and written back through an import created at the write-back, does not land its bytes before the CPU read. I have not yet read the write-back's segment copy and shadow publish to say where they go. @oneandonlydean, before I do: is the leave-the-watch group meant to run on a device whose imports unwatch, when the surface's own write-back imports it anyway? If the answer is that the write-back should use StorageImportFor too, that is the next change; if not, the fixture's "not run" exit belongs before the fill for this case as well.

The branch still sits on blawesom's base (143f13055); I will rebase onto current main with the fix for this failure, and the before/after capture for this change alone follows once the test is green.

[oneandonlydean]

@Mouad-Scriptz-0 Thanks for the trace, it made this quick to answer.

Yes, the leave-the-watch group is meant to run on a device whose imports unwatch, and the write-back should use StorageImportFor too. The point of this PR is that a watched storage surface stays watched on such a device. upload and Refresh now keep it out of a new import, but writeBackLayers (Texture.cpp:4691 on your head) still calls HostImportFor. So the first CPU read that stores the surface's results imports it and takes it out of the watch, and the tracking only lasts until that first write-back. The three "not run, the surface is not write-watched" lines are the test reporting exactly that: the driver's own write-back took the surface out of the watch before the test could. Moving the "not run" exit before the fill would hide the new path (staged upload, then write-back) on the only device class where it runs, so please don't do that. Those exits should stay for what they were written for: a host where the surface isn't watched at all.

When you make that change, please cover all the places that can create an import for a storage surface, not only the one in your trace:

• writeBackLayers: the whole-surface HostImportFor (Texture.cpp:4691);
• importSegments, which it falls back to: its per-piece HostImportFor (Texture.cpp:2626) would otherwise import the same surface piece by piece;
• CopyFrom's "not imported" check (Texture.cpp:4232) also creates an import as a side effect.

With no existing import, a watched surface then takes the readback write-back (vkCmdCopyImageToBuffer, then WriteChangedCommitted), which keeps the watch. Please include that path's cost in the before/after capture, since it is the price of staying tracked.

On the remaining failure: please merge current main and re-run before you read the segment copy. Your step 1 is a fixture race that main has already fixed. On your base, HoldPending returns as soon as StorageTexture::AnyPending() is true, so it accepted the two images left pending by the previous case. Main's #6744 makes it wait for this surface's own image (FindPending(surface) == FindLive(surface)), and adds a --pending-identity mode that checks this. On your base, the same race also lets the post-Fill HoldPending return before the fill is pending. That fits your step 4: the FillClear ran after the write-back and was still pending at destruction, and that alone would explain step 3's untouched blocks. So the failing check may come entirely from the stale fixture. If it still fails on a merged head, the trace is worth repeating there.

The merge is real work. It conflicts in 7 files (both GuestMemory files, VulkanDevice.hpp, PacketExecution.cpp, both GuestBufferMemory files, and the debt doc). Two points to watch:

• Rewatch. Main now has Rewatch(std::span<const std::pair<std::uint64_t, std::uint64_t>>), which does the work on Windows only (restore plus CPU stamp) and is a no-op on Linux. Your Rewatch(address, bytes) re-registers and stamps on Linux only and is a no-op on Windows. Both would compile side by side as overloads, so neither one would cover both platforms on its own. Please fold yours into main's span form, so that RetireIdleImports returns the range to the watch on both platforms.
• HostImportFor. Main's hit path now starts with the Windows-only settleUnwatchedImports under the same lock. Keep that call in your restructured function (inside the locked block, before the hit lookup), and keep the drain of unwatchedNotes outside the lock as you have it.

After you push the merged head with the write-back change, we'll review it and run full CI.