Repository navigation
perf(vk): with batched submits (RADV), send a batch at once while the GPU is idle (#1993) - #1999
Conversation
… GPU is idle (#1993) The held batch waited for the next wait or present while the GPU had finished everything sent, so it sat idle until the game waited for it (serial replay's mid-frame late-page wait above all). Proposed and measured by mrdev023 in #1993: RX 9070 XT, RADV, Kokiri Forest, 33-41 fps -> 46-61, no flashes; batching still groups submits while the GPU is busy. RX_VK_BATCH_IDLE=0 keeps the old hold. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Tested this branch as is (
Same gain as in #1993, no flashes. |
…e tests' fixtures stub both) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hckrman101
left a comment
There was a problem hiding this comment.
The change looks right to me. One thing should be checked before merge: the hardware this is aimed at.
Correctness. Sending a held batch early can't reorder anything. queue_flush_locked still runs under dev.present_mu, on the one queue, and signals timeline, rec_timeline and batch_tl with the same values it would have used later. batch_flushed is only read or written under that lock (recording_wait, queue_flush_locked, now batch_gpu_idle), so the replay, present and recorder threads gain no new race. Waits that need the batch keep their own flushes (wait/wait_value/vk_timeline_wait -> FR_VALUE/FR_WAIT, wait_recorded -> FR_WAIT, recording_wait -> FR_SLOT, present -> FR_PRESENT), so nothing new depends on this flush. It only makes them find the batch already sent more often. The first submit works: batch_tl and batch_flushed both start at 0, so it goes out at once. A lost device just makes vkGetSemaphoreCounterValue fail and the batch stays held, as before. The second commit only moves the check into batch_gpu_idle(), so mrdev023's run on 35a8aaa still applies to this head.
Gating. batch_gpu_idle() is only called inside the batch_on() branch, so drivers without batching (everything but RADV unless RX_VK_BATCH=1) don't change.
Perf. The numbers are there, and mrdev023 reran this branch (41-57 vs 29-38 fps, 0 flashes in 3044 frames). The catch is that both runs are one machine (9070 XT + 9950X3D at 5120x1440 with FSR 4), and that machine is GPU-bound. #1993 also notes that RX_VK_BATCH=0 was faster than the default there (44-55 vs 41-48). So on that box, much of the win comes from batching less. Batching was turned on for RADV for the opposite case: issue 413's 8060S, where each submit paid for the userptr imports again (~70 -> ~6 ms of queue submit a frame). On a setup where the GPU keeps up with the recorder, done >= batch_flushed holds at almost every submit, so most submits go out alone and we're close to RX_VK_BATCH=0 again. The Steam Deck is Tier 1 and RADV, so this goes default-on there. The NVIDIA RX_VK_BATCH=1 A/B you plan won't show that cost, because NVIDIA doesn't have the per-submit userptr cost. Could someone run it on a Deck or an RADV APU, or on a lighter RADV setup (native res, no FSR) where the GPU isn't the bottleneck? Check that the stats line's flushes a frame and the queue-submit time stay well below what RX_VK_BATCH=0 gives. If it does regress, the fix could be to flush on idle only once the batch is a few buffers big or has been held for a while, like RX_VK_BATCH_POLL_US does.
#1905. No duplicate and no textual conflict. #1905 changes wait_recorded's flush, which main already does unconditionally as FR_WAIT since the load-hang fix, into a conditional FR_READBACK. This PR touches submit_recording and not the FR enum or the stats line. The two complement each other: this one sends a batch early while the GPU is idle, and the readback flush still covers a batch held while the GPU was busy. #1905 is in conflict with main as it stands and also carries unrelated TAAU shader files, so it needs a rebase anyway.
Nit: the description says the late-page wait went from ~1.4 to 0.64 ms. #1993 says ~8.4 -> ~3.8 ms a frame and mrdev023's rerun ~7.9 -> ~4.6 ms, so the description's number is probably per wait or from another build.
CI: the only failure is tests/test_log_batch.py on windows-2022 (GetTickCount64() - t0 < 200), a timing check in logging that has nothing to do with this change. Ubuntu checks, the llvm-mingw Vulkan build and the AppImage pass.
| // (RX_VK_BATCH_MAX=<command buffers>: held at most this many, default 32) | ||
| static const size_t cap = getenv("RX_VK_BATCH_MAX") ? (size_t)std::max(1, atoi(getenv("RX_VK_BATCH_MAX"))) : 32; | ||
| if (pend.cbs.size() >= cap) flush_reason = FR_CAP, queue_flush_locked(); | ||
| if (!pend.cbs.empty() && batch_gpu_idle()) flush_reason = FR_OTHER, queue_flush_locked(); // (#1993) |
There was a problem hiding this comment.
Nit: these flushes are counted as FR_OTHER, so in the stats line they're mixed with the other untagged flushes (a parallel segment's own submit, queue_flush(), device_wait_idle_locked). An FR_IDLE reason, its own column in the stats line (one more stub in the two test fixtures), would show directly how many of the ~14 flushes a frame this causes. That's also the number to watch on a Deck or APU run.
|
A/B on Windows (RTX 4060 Ti, batching forced on with
With the batch held, the GPU sits idle and loses ~12 fps here, matching what #1993 measured on RADV. The idle flush brings batching back to the unbatched frame rate, and Windows' default (batching off) is unchanged. 🤖 Generated with Claude Code |
hckrman101
left a comment
There was a problem hiding this comment.
Re-review at da8a8a2. Since our review the only new commit is the merge of main. The PR's own diff (vk_replay.cpp and the two test stubs) is byte-for-byte what I reviewed at 23c47ad, so the correctness part of that review still holds.
Thanks for the Windows A/B. It's useful: holding the batch costs ~12 fps on the 4060 Ti too, and the idle flush gets batching back to the unbatched rate with no crashes. Windows' default is unchanged.
It also confirms why the RADV APU run is still open. When the GPU keeps up, the idle flush makes batched submits behave about like RX_VK_BATCH=0. That's fine on NVIDIA and on mrdev023's GPU-bound 9070 XT, where unbatched was already as fast or faster. But batching is on for RADV because of issue 413's 8060S, where unbatched was the slow path (each submit paid for the userptr imports again, ~70 ms of queue submit a frame). The Deck is Tier 1 and gets this by default, and neither A/B covers that case. So my request stands: one run on a Deck or an RADV APU (8060S/780M) with RX_VK_BATCH_IDLE=0, this PR, and RX_VK_BATCH=0, comparing fps, flushes a frame and queue-submit ms from the stats line. If the PR comes out close to RX_VK_BATCH=0 there, a minimum batch size or a minimum hold time before the idle flush would keep most of the win without paying that cost.
Still open from before, both nits: the idle flushes are counted as FR_OTHER (inline thread above; an FR_IDLE column would make the APU run easy to read), and the description's late-page wait (~1.4 -> 0.64 ms) doesn't match #1993's or mrdev023's per-frame numbers (~7.9 -> ~4.6 ms).
CI: the failures (test_make_game_ngx.py on Ubuntu and Windows 1/2, test_pipeline_state_portable.py on Windows 2/2, actionlint's SC2012 in linux-appimage.yml) fail the same way on main's run at 21fa1365, so they're not from this PR. The Vulkan build, AppImage and builder pass. Mergeable, no conflicts.
…ggle stub lacks switching; actionlint SC2012) (#2092) Main has been red since #2046 and the builder track switch landed: - **tests/test_pipeline_state_portable.py** (Windows): the fixture compiles with \`-Werror\` and clang on Windows marks \`fopen\` deprecated. It now passes \`-D_CRT_SECURE_NO_WARNINGS\`, as the other fixture tests do. - **tests/test_make_game_ngx.py** (all OSes): \`Builder.apply_mode\` now calls \`self.switching()\`, and the test's stub had none. It gets \`switching=lambda: False\`. - **.github/workflows/linux-appimage.yml** (actionlint, from #2079): the object count used `ls | wc -l` (shellcheck SC2012). It counts with a glob array, as the loop above it already does. Checked locally: both pass (fixture built with clang++ and Vulkan headers 1.4.345; test_make_game_ngx 17 tests OK). Unblocks #1999 and the other open PRs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…c fixture getenv deprecation) (#2109) Main is red again. [#1866](#1866) added this to the finish block of `make_game.main()`: ```python if fsr_wanted(a, out) and getattr(a, "fetch_fsr_runtime", False): ``` `tests/test_finish_output.py` and `tests/test_linux_update.py` (`test_a_packaged_build_puts_the_launcher_in_the_folder_and_the_game_in_bin`) run that block on its own, without `a`, so both fail with `NameError: name 'a' is not defined`. Both tests now pass an argument namespace with FSR off (`fsr=False, no_fsr=False, update=None, fetch_fsr_runtime=False`). The production code is unchanged. Checked locally: test_finish_output passes. The test_linux_update case only runs on Linux; CI covers it. Locally on Windows that file has unrelated symlink failures that are the same before and after this change. This unblocks #1999 and the other open PRs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Those came from my first write-up in #1993, and they were my misreading. The log line
I don't think it is, after profiling it today (
So on this box the frame is the CPU replay plus the mid-frame wait, and the GPU mostly waits for work. That also explains why |
… by default The idle send (NotRexed#1999) costs a Steam Deck LCD (Van Gogh, RADV) ~5 ms of queue submit a frame in Kokiri Forest on current main (11-12 ms with it, 6.6-7.1 without; 18.6-18.8 fps -> 20.0-20.7), where each vkQueueSubmit costs ~1 ms. The rule is limited to Van Gogh by device ID (0x163f LCD, 0x1435 OLED); every other GPU, other APUs included (780M, 890M, 8060S, unmeasured), keeps it. RX_VK_BATCH_IDLE=1 or 0 still chooses, and the log says which. Assisted-by: Claude Opus 5.5 (Anthropic) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Implements #1993's proposal by @mrdev023, as a default where batching is on.
With batched submits (on by default with RADV, so Linux AMD including the Steam Deck;
RX_VK_BATCH=1elsewhere), a held batch waited for the next wait, present or the 32-buffer cap. That happened even when the GPU had finished everything it was sent. So the GPU sat idle until the game waited for it, and with serial replay that's mid-frame, attake_capture's late-page wait.Now, when a submit joins the batch and the GPU has completed every batch sent so far (
batch_tl>=batch_flushed), the batch goes out at once. While the GPU is busy, submits are still grouped.The reporter's numbers (RX 9070 XT, RADV, Kokiri Forest, FSR 4, 5120x1440):
Flushes went from about 5.7 a frame to about 10 of 3.7 command buffers each, and the late-page wait from about 1.4 to 0.64 ms.
RX_VK_BATCH_IDLE=0keeps the old behaviour.Not yet checked here: I have no RADV machine. I'll A/B it on Windows with
RX_VK_BATCH=1(NVIDIA) for fps and flashes before merging. Drivers other than RADV are unchanged, since batching stays off there by default.🤖 Generated with Claude Code