Repository navigation
perf(agc): pipeline indirect draws behind the packet worker (APS5_PIPELINE_INDIRECT) - #1896
fisherjoey wants to merge 3 commits into
Conversation
|
Additional platform validation on standalone PR head Windows: passed — Windows 11, supported WinLibs GCC 15.2.0 UCRT/POSIX/SEH r7; CMake 4.3.1 / Ninja. Release / Tests: Reproduce after building the test targets: Ran with Vulkan: native AMD RX 6900 XT, driver 32.0.21045.5002. Integration limitation: a local merge against main |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for taking indirect draws from #1202. Not mergeable yet, for two reasons:
- The branch carries the whole #1201/#1202/#1747 stack (55 files, 7 commits that are not yours) and conflicts with
main. All the bases are still open drafts, and @MichonGoddijn231849 said on #1201 and #1747 that the stack is parked while it is ported onto the prepared-shader path from #639. This PR can only land after they do. - Because that port changes where draws are decoded and recompiled, the drain/overlap checks in
Driver::drawmay need to move with it.
What would help: keep this as a draft, and once the bases land (or the port is published) rebase onto main so the diff is only Draw.cpp, DrawPipeline.{hpp,cpp}, WriteEvidence.cpp and DrawPipelineCommits.cpp, tick "Based on current main", and rerun your Demon's Souls numbers on that head. We'll review it then.
|
Thanks, makes sense. I've moved this to draft. Once the #1201/#1202/#1747 stack lands on AI-assisted: yes |
|
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.
|
…ar#1896, picked onto main for a Village pair; local fast tree) APS5_PIPELINE_INDIRECT=1 (default off) stops draining the draw pipeline before every indirect draw. The worker reads the argument records (CPU path) after checking them against pending commit writes and pending labels, drains only on an overlap, and enqueues the expanded records as one commit; GPU-side indirect draws are enqueued as they are. Validated cache hits whose captured ranges overlap a pending write drain and retranslate. Pipeline depth defaults to 64 with the knob on. The [draw] pipelined line reports committer busy %, indirect commits and argument-overlap drains.
8ba9dda to
94c7a1a
Compare
|
This pull request is labeled |
github-actions
Bot
added
needs rebase
|
@adriadam10 Demon's Souls numbers for Setup: PPSA01341 v1.07, RTX 5070 Ti (595.91.07), Ryzen 5 7600X, Linux. Six 900 s runs from boot into Outpost Passage, alternating on and off. One caveat up front: these ran on our DS stack (main plus our open draw-path PRs), and that stack carries its own port of this PR's indirect path rather than the head
So the indirect path is about 10% slower here. No pipelined draw failed, and every one retired on the worker. Where the time goes:
With about one drain every 50 draws, the worker can't get far ahead of 15 to 25 ms draws sitting on the committer. Even with the forwarding added, I'd expect this to roughly break even on DS, not win. I'm leaving it in draft. If you want a measurement on the actual head after a restack onto main, I can do that. AI-assisted: yes |
|
Integration note: current |
|
@fisherjoey, following @MarcZX10's note: the focused #10756 candidate only deletes Your current report rewrite retains that append. I propose landing the small diagnostic correction independently first, then keeping the read-counter group once when this report rewrite is rebased. It does not depend on or change the indirect-draw path. Please flag any integration concern; the defect and qualification status will stay in #10756 rather than duplicate the full report here. Thanks to Marc for the independent confirmation and heads-up. AI-assisted: yes (Codex). Source-only follow-up: a three-way check of |
…like direct draws boykopovar#1896's expanded indirect commit enqueued without the index ranges its records read, so a deferred write-back over an index buffer could land before the queued draw read it (bd6d47a orders this for direct draws). Each expanded indexed record now adds its index range to the commit's reads.
|
@MarcZX10 @Amoenus Noted. When I rebase this branch, AI-assisted: yes |
Under APS5_PROFILE_DRAW a [committer] line every 10 s lists how many queue-0 draws and dispatches recorded inline on the packet worker instead of on the draw committer, by reason and PM4 opcode, and how many pipeline drains each opcode caused with the time spent waiting for them.
…LINE_INDIRECT With the knob on, a GPU-path indirect draw is enqueued like a direct draw and a CPU-path indirect draw puts all of its expanded records into one commit item instead of recording every record but the last inline under the GPU lock. The argument-overlap check at the start of the draw still drains when a queued item writes the records. The default pipeline depth becomes 64 with the knob on, and the [draw] pipelined line reports how busy the committer was. The indirect draw tests gain a variant that runs with the knob on.
A drain made for a capture or a validation read used the default opcode, so the per-opcode drain counts could not tell which packets wait on queued items. The drain now carries the opcode of the packet the worker is executing.
94c7a1a to
2850af2
Compare
|
Rebased onto main AI-assisted: yes |
|
@fisherjoey here are numbers from a second title on your 10-28 head, 2850af2, in case they help. The title, scene and machine all differ from your Demon's Souls runs, so these numbers don't speak to yours. Here the pipelined path came out ahead. Setup. Resident Evil Village (PPSA01556), content 01.210.000, at 3840x2160.
B averages 0.884 fps and A 0.704, so B is 25.6 % faster. Both B runs are above both A runs: B's slower run is 14.4 % over A's faster one. The same-phase pairs give +38.4 % and +14.4 %. It also composes with other open speed changes. On a tree with #12478, #12745 and #12921, merging this PR with your #9243 and #9248 gave about 24 % more fps on the same menu. All of those merges were clean, and the runs had 0 aborts and 0 failed imports. That pair doesn't separate this PR's share. |
|
@fisherjoey numbers from a third title on your 10-28 head 2850af2, on Linux, in gameplay rather than a menu, with alternated runs. They don't pool with your Demon's Souls runs or with the Resident Evil Village menu numbers above; the title, scene and machine differ. In the last speed series the switch read faster on both declared headlines (two earlier series read not faster, by a different window and by too few valid runs; both below), a glitch-count series read "no" in both directions, and drain time and the dispatch prologue rose as in your Demon's Souls read while capture + recompile did not change. Setup
Every series below had its rule written down before its first run, and both outcomes are reported either way. Speed, series 4 (after the reboot): I faster on both rules Pairs alternated and rotated, X I / I X / X I, one title boot each, all in one machine boot (22:17-00:28). Rule: "I is faster in the scene only if each I scene presents/s is above the highest X scene presents/s, over the three valid pairs"; the same form on the prompt FPS. Validity: our usual checks plus a desktop-memory bound relative to the series' first run.
Glitches, series 3 (after the reboot): neither arm more, both rules Same arms and alternation, 3 + 3, with a screenshot of every frame over the same 296-frame stretch of the walk in each run (coverage 1.000 on all six). A glitch frame is any shot over fixed thresholds (magenta share ≥ 1 %, magenta in the top-left quadrant ≥ 1 %, near-white ≥ 10 %, green ≥ 1 %, near-black ≥ 25 %, flat tiles ≥ 50 %, grid-edge ratio ≥ 1.10, a solid black box (≥ 0.5 % of the frame) absent from the shots 25 frames before and after, or a blackout). Rules, both ways: "I glitches more only if each I run's glitch-frame count in the band is above the highest X count", and the reverse.
Earlier series on the same build (before the reboot, stated for completeness, not pooled)
Mechanism (series 4, the gameplay window, per-arm mean over the three runs, I − X; recorded, not tests)
So on this title the switch takes about 250 GPU-side indirect draws per frame off queue 0's worker (−164 ms of DRAW_INDEX_INDIRECT packet time per frame; room waits −70 ms, at depth 64 against 8) and puts them on the committer (+23 points busy, +66 us per commit). As in your Demon's Souls read, drains per frame stay the same while their time rises (3.0× here), and the direct-dispatch prologue is longer (3.6× here; your read: 5.7 against 2.2 us). Capture + recompile per dispatch did not change here (157 → 155 us), where your read had it doubled per packet. Template-hit share 80.7 % against 80.5 %; validation memo misses 19, 19, 19 against 19, 25, 22 per run, about 8 per 100,000 lookups on both arms. One 10-s window of each arm from the walk (its Limits
Our base will run with the switch on from here. AI-assisted: yes (Claude Code). |
|
@fisherjoey With this PR merged onto Setup
ASan (abridged;
Without ASan, on the same merge, both abort: What changed under the PR (from reading the code; not a tested fix):
AI-assisted: yes (Claude Code) |
What
Since #1201/#1202, queue 0's direct draws are commit items on @MichonGoddijn231849's committer. Every indirect draw still drains the pipeline and records inline on the packet worker. In #1202 Michon said indirect draws weren't on his list and offered them to us (comment). This PR does that part on top of the committer now in
main.With
APS5_PIPELINE_INDIRECT=1(off by default):Driver::draw.commitDrawrecords them in packet order, so the GPU reads the arguments after every earlier commit.DrawPipeline::DrainIfOverlaps). An overlap drains first. The expanded records go in as one commit item.queryPendingWrite.[draw] pipelinedalso reports committer busy %, indirect commits and argument-overlap drains.Rect lists, deferred labels and
APS5_LOCKED_DRAW_PREPAREkeep the old drain-and-inline path.New test:
agc_driver_draw_pipeline_commits(tests/execution/DrawPipelineCommits.cpp). It checks three things. Arguments under a pending draw's write drain, and arguments elsewhere don't. Commits and a label item between two draws run in packet order, and the label can be answered while it is pending. A read over a pending item's writes drains on the queue-0 worker only.Related: #6931 (opened 2026-10-16) queues CPU-path indirect draws on the pipeline, on by default, and keeps GPU-side indirect draws draining. Its CPU-side part overlaps this PR's CPU-side part, and the two conflict in
Draw.cpp. See the last comment for where that stands.Closes #1895
Tested
Head
8ba9dda20, onmain6a32550fd. It also merges cleanly intomain17cfb1f4c. Nobara 44 x86-64 (Linux 7.2), GCC 16.2, Release,BUILD_TESTING=ON, SPIR-V Tools off. GPU tests ran on lavapipe (llvmpipe, LLVM 22.1.8).ctest -j4 --timeout 30 -R 'agc|pipelined|fill'(633 tests, 86 of them skip on lavapipe), 4 runs with the knob off and 4 withAPS5_PIPELINE_INDIRECT=1. The knob-off runs: 633/633 twice, 632/633 twice. The knob-on runs: the same. The only failure in all 8 runs isagc_driver_preparation_scope_settle("the link did not wait for the vertex shader's registration preparation"). Run alone, it failed 3 of 10 times with the knob off and 5 of 10 with it on, while another build was loading the machine. That test registers shaders and does no draws. The slowest test took 6.3 s.agc_driver_draw_pipeline_commits,agc_driver_pipelined_*, the*filltests and the indirect tests (18 tests), 10 runs with the knob off and 10 with it on: all pass every time.tools/check_conventions.py --base origin/mainandtools/check_test_registration.pypass.Demon's Souls has not been rerun on this head yet. The numbers below are from the old stack (#1201/#1202 as of 10-07 plus unrelated local changes), measured on an RTX 5070 Ti (driver 595.91.07): Outpost Passage through the debug start menu, 240 s runs,
APS5_PROFILE_DRAW=1, two interleaved 3+3 batches, ms per frame over the last 60 s of each run.APS5_PIPELINE_INDIRECT=1Graphics::Drawon the worker per 10 sIn those runs every indirect draw took the GPU-side path, so only the unit test covers the CPU-side branch.
Checklist
main; no other open PR implements the same functions (based on currentmain; perf(agc): queue CPU-path indirect draws on the draw pipeline #6931 overlaps the CPU-side part)NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository