Repository navigation
perf(agc): pipeline DMA_DATA when APS5_PIPELINE_DMA=1 - #6208
Conversation
…S5_PIPELINED_DRAWS) With APS5_PIPELINED_DRAWS=1, queue 0's worker decodes, captures and recompiles a draw and hands it to a committer thread that records it (Graphics::Draw under the GPU lock), so the two run in parallel instead of one after the other. Work the committer has not reached yet is tracked by the guest ranges it will write; a later packet that reads or writes them, or one the pipeline cannot order, drains it. Ordered as commit items instead of drains: - labels (WRITE_DATA, RELEASE_MEM) and same-queue waits on them; APS5_PIPELINE_DRAIN_LABELS=1 drains; - the end of a queue-0 submission and a released REWIND tail; APS5_PIPELINE_DRAIN_SUBMISSIONS=1 drains; - the title's buffer fill kernels; APS5_PIPELINE_DRAIN_FILLS=1 drains; - DMA_DATA immediates and copies (APS5_PIPELINE_DMA=1). A draw's write ranges include the storage images it writes and its exact color and depth target ranges (APS5_EXACT_DRAW_WRITES=1), not estimates. Draw-path costs the pipeline exposed: - ShaderMemory moves, so a commit owns its capture's pages; - a submission made before the worker's current collect epoch began keeps the epoch, so a worker behind a queue of submissions walks a surface once per catch-up (APS5_SUBMISSION_EPOCH_EACH=1 bumps at every submission); a thread remembers the ranges it walked for the whole epoch; - the pipeline signals only threads that wait; the pending-image and unit-shadow registries take a spinning slim lock instead of std::mutex (winpthreads contention was kernel events); ScanPending skips the registry walk when no pending image can overlap; - fewer heap allocations per recorded draw (reused descriptor sets and scratch vectors, buffer pool size classes kept). GTA V Performance mode, prologue bank interior, RTX 5080: 11.7 -> about 20 fps together with the cheaper captures below it. Off by default here; the follow-up turns it on. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Co-Authored-By: Gjon Gojcevic <7807870+gjonn@users.noreply.github.com>
DrawWriteRanges decoded every written storage image element, and DecodeTextureResource refuses a zero base address, so a draw with a null storage image bound failed (PPSA01317) instead of skipping it. The element is now skipped by the binder's own null rule (base address words zero) before it is decoded. agc_driver_draw_write_ranges binds a null image next to a written one. Co-Authored-By: Gjon Gojcevic <7807870+gjonn@users.noreply.github.com> Co-Authored-By: SleglJan <90033145+SleglJan@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The pipelined label path treated every nonzero RELEASE_MEM INT_SEL as an end-of-pipe interrupt;
main's PacketMemory excludes 3 (send data after write confirm). agc_driver failed on it with
APS5_PIPELINED_DRAWS=1 ("release with INT_SEL send data after write confirm raised an interrupt").
Co-Authored-By: TheAggie <67454+TheAggie@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
DMA_DATA on queue 0 joins the draw pipeline unless APS5_NO_PIPELINE_DMA is set. A copy that does not overlap uncommitted draws, a SET_PREDICATION, and a NOP other than a dispatch reset leave the queue in place. The source bytes are snapshotted in packet order, an immediate fill still notes depth and color metadata, and a predication that reads an uncommitted range waits for it. Marker push and pop no longer allocate a string per packet.
|
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.
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.
|
adriadam10
left a comment
There was a problem hiding this comment.
Built this on its own commit (the stack below it is not merged; #6203, its base, was closed as a duplicate of #1201) and ran the libSceAgcDriver tests:
agc_driver_pipelined_dma(added here) fails: "the copy did not see the fill before it, or wrote past its bytes". Please make it pass and check it fails without theenqueueDmaPacketchange.agc_driver_pm4segfaults: the PR drops the marker push/pop inPm4.cpp:986-991, buttests/Pm4.cpp:220still checksstate.markers.back() == "abc"and the unbalanced-pop case at:223. Either keep the marker stack behaviour or update those tests and say why nothing readsqueue.markers.- Turning the DMA pipeline on by default (
PacketMemory.cpp,APS5_NO_PIPELINE_DMA) is a perf change with no numbers. Please post before/after (same scene, several runs) and a Linux test that covers the overlap cases ofkeepsDrawPipelinecase 0x50 (copy overlapping an uncommitted draw write, dst == 1/2, 0 bytes) and the predication wait. - The PR conflicts with
mainand stacks on #6203, which was closed as a duplicate of #1201: please rebase it on #1201 (or onmainonce #1201 is in) and it can be re-reviewed then.
|
Heads-up from testing #1202 on current main (#1201 merged): with |
|
@ivica00 one data point for the Since this PR is being reworked onto #1201 and that part needs no snapshotting, I have opened it on its own as #6538, with a test that holds the pipeline busy and counts the drains for a prefetch and for a copy. If you would rather fold it in here, say so and I will close mine. AI-assisted (Claude Code) |
A copy reads its source when the pipeline runs it, so it sees the fill ahead of it. Large immediate fills stay buffer fills, host copies stay counted, and marker push and pop still update the queue.
|
Rebased onto current
Locally, on an RX 9070 XT: |
|
A data point from a title where Profiled runs (
The Frame rate in the 100–110 s window, no That scene hitches hard either way (p90 350–690 ms), so these windows don't show a difference. All 6 runs (3 per build) reached their limits. AI-assisted: yes (Claude Code) |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the rebase; the points from the earlier review are addressed in the diff (copy reads its source when the pipeline runs it, marker stack kept, copiesToHostMemory counted, overlap/GDS/zero-byte/predication tests). Remaining:
- The only measured gain for this change is the
DST_SELnowhere prefetch (@TheAggie, 24-25% → 37-43% on a PCSX2 scene), and that case is already in #6538 (keepsDrawPipelinecase 0x50, same file). Please rebase on top of it once merged and drop the duplicate part. - For the rest (copies and fills on the pipeline by default) the data in this thread shows no gain: on Borderlands 4 (PPSA01494, Linux, RTX 4070 Ti) the 0x50 drains disappear but the worker's time in drains goes 4,491 → 5,641 ms and frame times are unchanged. Turning a default on needs a before/after that shows a win on at least one title; please post one, or keep it behind
APS5_PIPELINE_DMA=1. PacketExecution.cpp:46changes the NOP rule from an allow-list (0, 0x09, 0x0b, 0x0c, 0x1a) to "everything except 0x09". Please say which other NOP operations exist and why none needs the pipeline drained (or keep the allow-list plus 0x0b/0x0c).
|
Tested this on Windows (RX 9070 XT, AMD 26.8.1) with Stellar Blade, which issues a lot of DMA_DATA on queue 0, merged on top of current main together with other draw-pipeline work. Measured with
No pipeline failures or crashes across three ~10 minute runs with it on by default, and nothing looked wrong while playing. Frame time in Stellar Blade did not move much, because the queue-0 worker now waits on the draw committer at other packets instead, so this does not show up as FPS here yet. The drain removal itself works as described. 🤖 Generated with Claude Code |
|
A possible ordering gap in const auto address = packet[4] | (static_cast<std::uint64_t>(packet[5]) << 32u);
return !DrawPipeline::Queue0().Overlaps(address, bytes);The source is never checked. A memory-to-memory copy that I hit it while working on a change that keeps some small copies off the pipeline: a fill followed by a copy of the filled range read zeros. Checking the source as well fixed it: const auto source = Pm4::DmaSource(packet);
if ((source == 0 || source == 3) && DrawPipeline::Queue0().Overlaps(packet[2] | (static_cast<std::uint64_t>(packet[3]) << 32u), bytes)) return false;
return !DrawPipeline::Queue0().Overlaps(address, bytes);I haven't reproduced it on this PR's head alone, only on a branch that sends more copies down the inline path. On this PR's head an unaligned copy of a range a queued fill writes should show it. I can write that test if it helps. |
…rites Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressing the remaining review:
Pushed in bf47f84. |
|
Picking this up while @adriadam10 is away. Thanks: the enqueue is opt-in now and the NOP allow-list is back. Two things remain. (1) |
|
Data point where this helps: The Smurfs - Dreams (PPSA21607, Unreal Engine), Linux, RX 6650 XT / RADV Mesa 26.2.4, main 9f25d0b + this PR (plus local unrelated rendering fixes). In the Smurfs' Village, AI-assisted (Claude Code). |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed on
Opcode
The DMA test sets |
The NOP allow-list and opt-in gating are done; the #6538 overlap now only affects merge order.
|
Merged while @adriadam10 is away. Of your three points, the NOP allow-list and the opt-in gating are done, and the overlap with #6538 now sits inside the opt-in path, so whichever of the two lands second resolves one small conflict. Full CI is green on this head. |
What
DMA_DATAon queue 0 joins the draw pipeline whenAPS5_PIPELINE_DMA=1.keepsDrawPipelineuses that same switch: with it unset, everyDMA_DATAdrains, including a non-overlapping copy and aDST_SELnowhere prefetch. A copy whose source or destination overlaps an uncommitted draw drains first. A copy that does not, aSET_PREDICATION, and a NOP from the allow-list (plain NOP, dispatch reset, marker push and pop, context state) leave the queue in place. Other custom NOP packets still drain.SET_PREDICATION(opcode0x20) only stores the predicate address and operation on the queue. It does not read that memory. The later predicated packet drains when the predicate overlaps a queued write.A copy reads its source when the pipeline runs it, so it sees an earlier fill. An immediate fill above 64 KiB is still stored as a buffer fill, a copy into host memory is still counted, and a predication that reads an uncommitted range waits for that range. Marker push and pop still update the marker stack.
Tested
Not re-run in this update. The previous run of
agc_driver_pipelined_dma_testsandagc_driver_testswas on an AMD Radeon RX 9070 XT, Windows 10. The DMA test setsAPS5_PIPELINE_DMA=1and covers fill-then-copy ordering, a prefetch (DST_SELnowhere), a zero-byte copy, a GDS copy, an unaligned copy off a pending write, onto a pending write, and of a pending write,SET_PREDICATION, marker push/pop, a predicated packet whose predicate overlaps a pending write, andDISPATCH_RESETstaying on the pipeline.No new title-session numbers from this machine. The measurements already on this PR (Borderlands 4, Stellar Blade) do not show a frame-time win for copies and fills on by default, so pipelining stays opt-in. @Pacuka measured The Smurfs - Dreams with
APS5_PIPELINE_DMA=1(about 7.9 to 9.4 fps in the village). @TheAggie measured the prefetch case on RetroArch's PCSX2 core; that part is #6538, which is still open, so the nowhere case stays here until that lands.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)