MagicBeanDev/Hookshot

Archived

Watch

Fork 0

vk: a readback's wait flushes the held batch (FR_READBACK) #1905

Closed

kafasam wants to merge 1 commit from readback-flush-only into main

kafasam commented

2026-11-10 19:57:46 +00:00

(Migrated from github.com)

What

When a readback waits for a GPU write-back, the held command buffer batch is flushed before the wait, so the GPU starts processing it immediately instead of waiting for the next present. This closes the ~23-28ms readback latency bubble measured in the graveyard investigation.

Why

The investigation measured 163 of 3000 reads waiting 23-28ms because results only landed when the display thread's present flushed the batch (flush reasons were 60/61 "present"). The code's own design comment at vk_replay.cpp line 132 promised a readback flush that was never wired.

Changes (one file, ~20 lines)

New flush reason FR_READBACK in the batch enum, printed in the stats log line
rec_out tracks the recordings' timeline value the last flush sent
The flush in wait_recorded() fires only when the exact recording being waited on is still held in the batch

Safety

Only sends already-recorded command buffers — stopped queues are never forced past their event waits
Both wait_recorded call sites hold no locks (present_mu is a leaf)
Fires at most once per held recording (no flush loops)
With RX_VK_BATCH=0 the pend is always empty so it never fires
With RX_VK_ASYNC=1 the readback goes through wait_value() which already flushes — this fix is load-bearing on sync mode only

Testing

Built and deployed on RX 6700 XT. With RX_VK_ASYNC=1 the readback counter reads 0 (expected — the async path already flushes). With RX_VK_ASYNC=0 (sync), the fix addresses the measured 23-28ms bubble.

AI assistance disclosure: the analysis and fix were developed with AI assistance, verified and tested by a human on the affected hardware.

## What When a readback waits for a GPU write-back, the held command buffer batch is flushed **before** the wait, so the GPU starts processing it immediately instead of waiting for the next present. This closes the ~23-28ms readback latency bubble measured in the graveyard investigation. ## Why The investigation measured 163 of 3000 reads waiting 23-28ms because results only landed when the display thread's present flushed the batch (flush reasons were 60/61 "present"). The code's own design comment at vk_replay.cpp line 132 promised a readback flush that was never wired. ## Changes (one file, ~20 lines) - New flush reason `FR_READBACK` in the batch enum, printed in the stats log line - `rec_out` tracks the recordings' timeline value the last flush sent - The flush in `wait_recorded()` fires only when the exact recording being waited on is still held in the batch ## Safety - Only sends already-recorded command buffers — stopped queues are never forced past their event waits - Both `wait_recorded` call sites hold no locks (present_mu is a leaf) - Fires at most once per held recording (no flush loops) - With `RX_VK_BATCH=0` the pend is always empty so it never fires - With `RX_VK_ASYNC=1` the readback goes through `wait_value()` which already flushes — this fix is load-bearing on sync mode only ## Testing Built and deployed on RX 6700 XT. With `RX_VK_ASYNC=1` the readback counter reads 0 (expected — the async path already flushes). With `RX_VK_ASYNC=0` (sync), the fix addresses the measured 23-28ms bubble. AI assistance disclosure: the analysis and fix were developed with AI assistance, verified and tested by a human on the affected hardware.

DarkGhostHunter commented

2026-11-10 21:33:29 +00:00

(Migrated from github.com)

Would this fix the write-read wait from #1133?

kafasam commented

2026-11-10 22:06:39 +00:00

(Migrated from github.com)

Yes — this addresses the same class of problem as #1133. The readback wait in wait_recorded() was the sync-mode path where the batch wasn't flushed before waiting (the design comment at vk_replay.cpp line 132 promised a readback flush that was never wired). With RX_VK_ASYNC=1 the async path (wait_value()) already flushes, which is why the bubble was only measured in sync mode. This fix wires the flush for the sync path, closing the gap for both configurations.

Yes — this addresses the same class of problem as #1133. The readback wait in `wait_recorded()` was the sync-mode path where the batch wasn't flushed before waiting (the design comment at vk_replay.cpp line 132 promised a readback flush that was never wired). With `RX_VK_ASYNC=1` the async path (`wait_value()`) already flushes, which is why the bubble was only measured in sync mode. This fix wires the flush for the sync path, closing the gap for both configurations.

DarkGhostHunter commented

2026-11-10 23:03:05 +00:00

(Migrated from github.com)

I built it as AppImage and I can confirm the texture flickering that #1133 fixed returned. Does this need a rebase?

hckrman101 (Migrated from github.com) reviewed

2026-11-11 02:16:21 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks for chasing the readback bubble. The diagnosis is right, but main already has this fix: #1276 (merged yesterday, after this branch's base) put an unconditional queue_flush_for(FR_WAIT) at the top of wait_recorded(), so in sync mode the held batch already goes out before the wait. Against main this PR has nothing left to add, and it conflicts there (the branch is ~920 commits behind and GitHub shows it as unmergeable). No CI ran on it either.

Two other things:

The diff isn't one file. The branch also carries 8716b2996 (the FSR 1 EASU+RCAS commit: two shaders, their SPIR-V headers and ~170 lines in vk_draw.cpp). That's #1439's work, and an older copy of it than #1439's head, so merging this would bring in the upscaler without its review. #1906 is stacked the same way: it carries this commit and the FSR one under its own vk_memory.cpp change. Each of these should be its own branch off current main.
@DarkGhostHunter's flicker report: that's the stale base, not this change. #1133 merged today and this branch predates it, so an AppImage built from it doesn't have the #1133 fix. (On their question: #1133 fixed tracking writes from direct buffer bindings, which is a different problem from this batch flush. This PR doesn't replace it.)

If you still want something here after rebasing, the gating you added (flush only when seq > rec_out, with its own FR_READBACK reason in the stats line) could be a small follow-up on top of #1276's flush, if it measurably cuts flushes. But on main the bubble should already be gone, so I'd close this one in favour of #1276.

Thanks for chasing the readback bubble. The diagnosis is right, but main already has this fix: #1276 (merged yesterday, after this branch's base) put an unconditional `queue_flush_for(FR_WAIT)` at the top of `wait_recorded()`, so in sync mode the held batch already goes out before the wait. Against main this PR has nothing left to add, and it conflicts there (the branch is ~920 commits behind and GitHub shows it as unmergeable). No CI ran on it either. Two other things: - **The diff isn't one file.** The branch also carries `8716b2996` (the FSR 1 EASU+RCAS commit: two shaders, their SPIR-V headers and ~170 lines in `vk_draw.cpp`). That's #1439's work, and an older copy of it than #1439's head, so merging this would bring in the upscaler without its review. #1906 is stacked the same way: it carries this commit and the FSR one under its own `vk_memory.cpp` change. Each of these should be its own branch off current main. - **@DarkGhostHunter's flicker report:** that's the stale base, not this change. #1133 merged today and this branch predates it, so an AppImage built from it doesn't have the #1133 fix. (On their question: #1133 fixed tracking writes from direct buffer bindings, which is a different problem from this batch flush. This PR doesn't replace it.) If you still want something here after rebasing, the gating you added (flush only when `seq > rec_out`, with its own `FR_READBACK` reason in the stats line) could be a small follow-up on top of #1276's flush, if it measurably cuts flushes. But on main the bubble should already be gone, so I'd close this one in favour of #1276.

gpu/vk/vk_replay.cpp

Main has queue_flush_for(FR_WAIT); at this point already (#1276). It flushes whenever the batch is non-empty, so the seq > rec_out check only narrows that. If you keep the narrowing, put it on top of main's version, and the rec_out/FR_READBACK bookkeeping should replace the FR_WAIT flush rather than stack with it.

Main has `queue_flush_for(FR_WAIT);` at this point already (#1276). It flushes whenever the batch is non-empty, so the `seq > rec_out` check only narrows that. If you keep the narrowing, put it on top of main's version, and the `rec_out`/`FR_READBACK` bookkeeping should replace the FR_WAIT flush rather than stack with it.

kafasam commented

2026-11-11 03:58:21 +00:00

(Migrated from github.com)

Yes — rebased onto current main (c0e8479f), sorry about that. The branch was cut before #1133 merged, so an AppImage built from it lacked the direct-buffer-write bookkeeping and capture-readback invalidation fixes — that's the flickering you saw, not something the flush introduced.

One thing worth noting for the rebuild: main has since added its own unconditional batch flush in wait_recorded() (FR_WAIT, for the RADV load hang). This PR keeps the conditional FR_READBACK flush instead — it only fires when the waited-on recording is actually still held (seq > rec_out && !pend.cbs.empty()), so it doesn't defeat batching for unrelated held submits, and the reason stays distinguishable in the flush stats. If the flickering persists on a rebuild from the updated branch, that would be a real interaction worth digging into.

Yes — rebased onto current main (c0e8479f), sorry about that. The branch was cut before #1133 merged, so an AppImage built from it lacked the direct-buffer-write bookkeeping and capture-readback invalidation fixes — that's the flickering you saw, not something the flush introduced. One thing worth noting for the rebuild: main has since added its own unconditional batch flush in `wait_recorded()` (`FR_WAIT`, for the RADV load hang). This PR keeps the conditional `FR_READBACK` flush instead — it only fires when the waited-on recording is actually still held (`seq > rec_out && !pend.cbs.empty()`), so it doesn't defeat batching for unrelated held submits, and the reason stays distinguishable in the flush stats. If the flickering persists on a rebuild from the updated branch, that would be a real interaction worth digging into.

kafasam commented

2026-11-11 06:18:49 +00:00

(Migrated from github.com)

Your review was of the pre-force-push branch — it is now a single commit (c0e8479f) rebased onto main at force-push time (fab8cb37), one file gpu/vk/vk_replay.cpp +14/−5, and GitHub reports it mergeable.

On the substance we agree: with #1276's unconditional queue_flush_for(FR_WAIT) at the top of wait_recorded(), the bubble is fixed on main and this has nothing left to carry — happy to close.

The one residual from our version is the conditional gate (flush only while the recording is still held, seq > rec_out, with its own FR_READBACK reason in the stats), so unrelated held batches aren't flushed. With #1993's idle-send on RADV, fewer spurious flushes may matter on Linux. If useful we can send a small follow-up PR with measured FR_READBACK vs FR_WAIT counts from a run; otherwise closing here is right.

Your review was of the pre-force-push branch — it is now a single commit (`c0e8479f`) rebased onto main at force-push time (`fab8cb37`), one file `gpu/vk/vk_replay.cpp` +14/−5, and GitHub reports it mergeable. On the substance we agree: with #1276's unconditional `queue_flush_for(FR_WAIT)` at the top of `wait_recorded()`, the bubble is fixed on main and this has nothing left to carry — happy to close. The one residual from our version is the conditional gate (flush only while the recording is still held, `seq > rec_out`, with its own FR_READBACK reason in the stats), so unrelated held batches aren't flushed. With #1993's idle-send on RADV, fewer spurious flushes may matter on Linux. If useful we can send a small follow-up PR with measured FR_READBACK vs FR_WAIT counts from a run; otherwise closing here is right.

kafasam commented

2026-11-11 06:19:00 +00:00

(Migrated from github.com)

@DarkGhostHunter since you were testing the AppImage build: we are closing this PR in favour of #1276, whose flush in wait_recorded() is already on main and covers the fix you were testing for — a fresh main build should carry it.

@DarkGhostHunter since you were testing the AppImage build: we are closing this PR in favour of #1276, whose flush in `wait_recorded()` is already on main and covers the fix you were testing for — a fresh main build should carry it.

Commenting is not possible because the repository is archived.

No reviewers