Repository navigation
perf(agc): import storage surfaces that cross the import window through a joined import - #6197
Open
mcsilva-dev wants to merge 1 commit into
Open
mcsilva-dev wants to merge 1 commit into
mcsilva-dev wants to merge 1 commit into
Conversation
…gh a joined import HostImportFor keeps a run inside one 8 MiB import window, so a storage surface that crosses a window boundary had no import and every upload of it took the CPU path: read guest memory, copy it into a staging buffer, detile in a batch of its own and wait for it. UFC 6 (PPSA23566) builds its transient render targets from a pool of 8 MiB pages and packs surfaces across page boundaries, so most of its storage uploads took that path: 1,700 CPU uploads (6.9 GiB) per 10 s at the title screen, half of the queue worker's time. HostImportSpanning falls back to a joined import over the contiguous registered ranges the surface covers, as buffers already do through importJoined; the storage texture paths use it. HostImportFor keeps its per-window contract. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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.
|
11 tasks done
0xcrstn
added a commit
to 0xcrstn/AnyPS5
that referenced
this pull request
Oct 15, 2026
…window (mcsilva-dev) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
0xcrstn
added a commit
to 0xcrstn/AnyPS5
that referenced
this pull request
Oct 15, 2026
…he FPS candidate Brings in ufc6/feedback (origin/main 760ffa6, boykopovar#6197, boykopovar#4286, boykopovar#5955) on top of the four FPS topics and boykopovar#6168. Conflict resolutions: - Execution Draw.cpp: keep the binding plan's single Regions() collection after the stage loop, adding the regions of reused or data-hit stages, and upstream's data-hit counters and cacheDrawStages signature. - Graphics Draw.cpp: recordDrawCommands takes both upstream's firstIndex and the dirty-record bound state; a stencil replay, like a face or channel split, drops the bound state. - The binding plan falls back to the full capture for stages whose plan uses image tables (ShaderMemory::UsesImageTables). Unbuilt and untested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0xcrstn
added a commit
to 0xcrstn/AnyPS5
that referenced
this pull request
Oct 15, 2026
…nd supersede handed-over units before saving HostImportSpanning (boykopovar#6197) joined registered ranges whose boundary falls inside a 64 KiB unit, so with unit shadows such surfaces took the GPU-direct path that main refuses for them (agc_storage_split_writeback, agc_driver_storage_write_back_overhang). The whole-surface import now goes through surfaceImport, which keeps the per-window import, refuses a join across a unit-misaligned boundary under unit shadows and joins otherwise, so 64 KiB-aligned pages keep the spanning import. The boykopovar#4286 port superseded handed-over units after main's capture of unsaved layers, whose flush stored the alias's dropped results first (agc_driver_alias_handover); the supersede now runs before it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
12 tasks done
EagleBearerYS
added a commit
to EagleBearerYS/AnyPS5
that referenced
this pull request
Oct 15, 2026
Merged
10 tasks done
This was referenced Oct 15, 2026
This was referenced Oct 15, 2026
Closed
fix(agc): place device memory in host memory when the driver refuses a device-local allocation
#5987
Merged
11 tasks done
11 tasks done
7 tasks done
This was referenced Oct 15, 2026
Open
Contributor
|
@mcsilva-dev I tried this on our Ghost of Yōtei branch today, and two of main's storage write-back tests fail with it. They fail on your head d7f5ffb by itself too, without our changes:
Replacing the six |
11 tasks done
Contributor
|
With this PR two existing tests fail under Mesa lavapipe, on Linux and on Windows: Runs. The fork CI builds the tests and runs the two ctest entries under lavapipe. Linux is ubuntu-latest (llvmpipe, LLVM 20.1.2). Windows is windows-latest with Mesa 26.2.4 lavapipe (llvmpipe, LLVM 23.1.2), where the test memory is private committed memory and the imports are host-pointer imports.
Messages, the same on both platforms:
Cause.
Change. One line in each test file; with both, all 34 tests of that job pass on a tree that contains this PR (38846598673): --- a/core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
+++ b/core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
@@ -4129,7 +4129,7 @@ void splitWriteBackTests(const Device& device, Recorder& recorder) {
{"64 KiB R_X 200x150, 3 mips, off a 64 KiB boundary, ranges split at 132 KiB", TextureTileMode::kR64KBX, 3, 4096, 135168, true, true, false, false},
{"linear 200x150, ranges split at 64 KiB", TextureTileMode::kLinear, 1, 0, 65536, true, true, false, false},
{"64 KiB R_X 200x150, 3 mips, second range of another access", TextureTileMode::kR64KBX, 3, 0, 131072, false, false, false, false},
- {"64 KiB R_X 200x150, 3 mips, ranges split inside a 64 KiB unit", TextureTileMode::kR64KBX, 3, 0, 135168, true, !UnitShadowEnabled(), false, false},
+ {"64 KiB R_X 200x150, 3 mips, ranges split inside a 64 KiB unit", TextureTileMode::kR64KBX, 3, 0, 135168, true, true, false, false},
{"64 KiB R_X 200x150, 3 mips, two adjacent allocations split at 128 KiB", TextureTileMode::kR64KBX, 3, 0, 131072, true, true, true, false},
{"64 KiB R_X 200x150, 3 mips, ranges split at 128 KiB, a second store outside the write watch", TextureTileMode::kR64KBX, 3, 0, 131072, true, true, false, true},
{"linear 200x150, ranges split at 64 KiB, a second store outside the write watch", TextureTileMode::kLinear, 1, 0, 65536, true, true, false, true},
--- a/core/libs/prx/libSceAgcDriver/tests/execution/StorageWriteBackOverhang.cpp
+++ b/core/libs/prx/libSceAgcDriver/tests/execution/StorageWriteBackOverhang.cpp
@@ -75,6 +75,7 @@ void Write(std::uint8_t* texels) {
AgcDriverWaitIdle_nid_postfix();
AgcDriver::Graphics::StorageTexture::FlushAllPending("test");
AgcDriverWaitIdle_nid_postfix();
+ AgcDriver::GuestMemory::FlushGpuWrites(reinterpret_cast<std::uintptr_t>(texels), SurfaceBytes);
}The first change makes the case expect the GPU-direct path whatever AI-assisted: yes |
This was referenced Oct 16, 2026
11 tasks done
Contributor
|
A second title with storage surfaces that cross the import window: MXGP 26 (PPSA30839). Its 2560x1440 storage images are 15 MiB ( Linux 6.8, RTX 4070 Ti (595.99.02), Release, pad input, 130 s runs.
In normal runs the frame rate doesn't change, because those runs upload only about 10 storage images in all. The difference is in the runs where several overlapping 2560x1440 images keep flushing and re-uploading each other (#6998): with this PR that didn't happen in 6 of 6. All 18 runs reached their limits. AI-assisted: yes (Claude Code) |
adriadam10
requested changes
Oct 18, 2026
adriadam10
left a comment
adriadam10
left a comment
Collaborator
There was a problem hiding this comment.
Thanks, the measurements on UFC 6 and MXGP 26 make the case for this clear. Before it can merge:
- Conflicts with
maininGuestBufferMemory.hppandGuestBufferMemory.cpp(real content conflicts, not just adjacency). Please rebase and rerunagc_driver_graphics_testsafter it. - Two existing tests fail with this change (reported above by @Celegans12 on Windows/RTX 3080 and by @AlxcBTW on lavapipe, Linux and Windows):
agc_storage_split_writeback("ranges split inside a 64 KiB unit: the write-back took the GPU-direct path") andagc_driver_storage_write_back_overhang. Both pass on your base. Please update them in this PR. @AlxcBTW's analysis and one-line fixes are in the thread: expect the GPU-direct path insplitWriteBackTests(Recorder.cpp, the "split inside a 64 KiB unit" row) and callAgcDriver::GuestMemory::FlushGpuWrites(...)at the end ofWrite()intests/execution/StorageWriteBackOverhang.cpp. - @Celegans12 reports that an aligned split also lost data on their setup. Please confirm the stored texels are right once the GPU work is complete on a hardware driver (not only lavapipe), and state in the PR that the split-inside-a-unit case now deliberately leaves the
importSegments()rule behind.
github-actions
Bot
added
perf
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
HostImportForkeeps an import run inside one 8 MiB import window (SetHostImportWindow(8 MiB)), andimportAccessreturns no import when the window run does not cover the range. A storage surface that crosses a window boundary therefore never has an import, and every upload of it takes the CPU path inStorageTexture::upload:GuestMemory::ReadCommittedof the whole surface, a staging copy, a detile in aCommandBatchof its own andSubmitAndWait.EA SPORTS UFC 6 (PPSA23566) builds its transient render targets in a pool it reserves at
0x11c0000000(+0x6f424000) and maps in 8 MiB pages (remap fixed ... +0x800000 prot=0xf3, each page on unrelated physical memory). The engine packs surfaces across page boundaries, so most of its storage surfaces cross a window. WithAPS5_PROFILE_DRAW=1at the title screen, per 10 s:i.e. about half of the queue worker's time went to these uploads. A temporary diagnostic that recorded the import refusal for each CPU upload named a single cause: every one was refused as not registered because the window run did not cover it, all inside the
0x11c0000000pool. The pool is remapped only during start-up (82 remaps, none after the title screen appears), so an import over it stays valid.HostImportSpanningkeepsHostImportFor's per-window contract (importWindowTestsstill requires that a view crossing the window is not imported) and, when it finds no import, falls back to a joined import over the contiguous registered ranges the range covers (crossedRun→importJoined, as buffers already do). The storage texture paths that look up an import for a whole surface use it.Tested
agc_driver_graphics_tests(Windows 11, WinLibs GCC 15.2.0, Release, AMD Radeon RX 9070 XT, driver 26.8.1): the newimportSpanningTestsfails onmainwitha surface crossing the import window was not imported through a joined import(checked by turningHostImportSpanninginto a plainHostImportFor) and passes with this change; the whole executable passes (Graphics validation tests passed).tools/check_conventions.py --base origin/mainpasses.APS5_TRACE_SUMMARY_EVERY=10. Both builds aremain760ffa6 plus one local change that the title needs to reach the title screen on 32 GB (registered shaders prepared at first use, see AGC: registered shaders keep decoded programs and resource plans resident; UFC 6 (PPSA23566) exhausts host memory on a 32 GB machine #6139); they differ only by this commit:APS5_PROFILE_DRAW=1main760ffa6 (+ local change)With the CPU uploads gone, the largest remaining storage cost on
mainis the alias round trip:storage refreshwrite-backs of ~8,800 surfaces (~9.6 GiB) per 10 s, the hand-overs #4286 targeted.Together with #4286. On the UFC bring-up fork from #5996 (c222bd2), same machine and scene, without
APS5_PROFILE_DRAW:The first two rows ran with the game on a hard disk and the third on the NVMe SSD; moving the same build between the two disks shortened start-up but did not change the title-screen frame time.
The hand-over takes an aliased surface's units on the device only when the surface is imported, and in this title the aliased surfaces were exactly the ones crossing a window. So this change is what lets #4286 act on Windows here; it may be worth another look at #4286 on top of it.
The title screen picture is the same with and without the change (compared on screenshots); I have not checked later scenes beyond the first cutscene starting.
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.mdfiles or images added to the repository (attach them to this PR)