Repository navigation
Conversation
|
Would this fix the write-read wait from #1133? |
|
Yes — this addresses the same class of problem as #1133. The readback wait in |
|
I built it as AppImage and I can confirm the texture flickering that #1133 fixed returned. Does this need a rebase? |
hckrman101
left a comment
hckrman101
left a comment
There was a problem hiding this 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 invk_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 ownvk_memory.cppchange. 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.
| // set for another's flush, which queue_flush_locked's empty return would do) | ||
| { | ||
| std::lock_guard lk(dev.present_mu); | ||
| if (seq > rec_out && !pend.cbs.empty()) flush_reason = FR_READBACK, queue_flush_locked(); |
There was a problem hiding this comment.
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.
A read of GPU results on the sync replay waited on the recordings' timeline without sending the batch: when the recording holding the write-back was still held (the flushes were present-time, 60/61 of them), the read blocked the rest of the frame for the display thread's present to flush it - 163 of 3000 reads at 23-28 ms each, ~1.4 blocking waits a frame (the ~25 ms pipeline bubble of the cpu-bottleneck investigation, its NotRexed#2 remaining lever). wait_recorded now flushes the batch first, gated on the recording still being held (its seq above what the last flush sent out): the GPU starts the write-back at read demand instead of at present. Empty or older batches are not flushed, and the reason is counted in the batched-submits log line as 'readback'.
26b9466 to
c0e8479
Compare
|
Yes — rebased onto current main (c0e8479), 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 |
|
Your review was of the pre-force-push branch — it is now a single commit ( On the substance we agree: with #1276's unconditional The one residual from our version is the conditional gate (flush only while the recording is still held, |
|
@DarkGhostHunter since you were testing the AppImage build: we are closing this PR in favour of #1276, whose flush in |
…k that writes it again (#2125) > **EDIT (2026-11-10):** the first version of this PR said it fixes the cyan flashes and the UI thrown about from #1005. It doesn't. My runs with and without it also differed in Render resolution and session length, and the cyan that reproduces reliably turned out to come from VRAM mirrors given back while idle, after a pause with the game in the background and (mostly) after a save reload: #2154. What this PR does fix is below. ## What it fixes #296 made every CPU read of GPU results wait for the GPU work that writes them. Without that wait, OoT's exposure flashed for a frame (#296: "OoT exposure flashes"). With per-queue fences on (the default since #523) that guarantee has a hole. When the work that writes the range is stopped at a wait that is already met (for example a CPU event the game signalled, which is applied at the replay's next look at the queues), the read-back finds no pending write-back, waits for nothing, and reads the range as it was: an earlier frame's result. This PR closes that hole. Before the lookup the read-back lets that stopped work on, never forcing anything, so its write-back is known and the read waits for exactly that work. ## Why it's worth merging - **It restores #296's guarantee on the default path.** The hole looks accidental. #523 skipped the read-back's catch-up because a catch-up then pushed stopped work past its waits. #449 had already made the catch-up safe, 53 minutes before #523 merged, so the skip outlived its reason. - **It's cheap.** A read-back that has to wait now takes about 1 ms (0.65 ms median), the time for the GPU to do the write. `RX_VK_PER_QUEUE_FENCES=0`, the workaround in #1005, closes the same hole by waiting for every queue's work: 19–87 ms per read-back on the same card, 12.5 fps against 31.8 with this PR at the same settings. - **It's small and contained.** One `if` in `invalidate`, using the same `sync_settle()` the texture branch further down already calls. Nothing changes with `RX_VK_PER_QUEUE_FENCES=0`, `RX_VK_ASYNC=1` or `RX_VK_SYNC_WAITS=0`. - **It's covered.** `tests/test_readback_settle.py` fails on main and passes with this change. What I haven't seen is a visible change I can pin on it alone. It's a correctness fix for a known bug class (#296), not a fix for a reported picture problem. ## How the hole works `invalidate` looks up the range's write-back value, which is known only once the work that writes the range is recorded. #523 skipped the read-back's catch-up with per-queue fences on. So work stopped at a wait that was already met stayed unrecorded, its write-back was unknown, and the read went ahead with nothing to wait for. ## Why the read-back changes again The read-back in `invalidate` has changed several times, each step fixing the one before: 1. **#296** (evilmfkr): a CPU read of GPU results waits for the work that writes it back. Today each range remembers the submission that writes it back (`wb_pages_note`, noted when that work is recorded), and `invalidate` waits for that value. Before the lookup the replay is caught up, so work handed over can note its write-back first. 2. **The catch-up forced stopped work.** In the sync replay, that catch-up pushed work stopped at an event wait past its wait, ahead of the work it waits for: the fog and exposure of the frame before, for a frame. 3. **#449** (azlekayn, merged 2026-11-08 08:19 UTC): a catch-up lets on only the stopped work whose waits are met, and forces nothing (`wait_queue_empty` in the sync replay, `sync_settle`). 4. **#523** (hckrman101, merged 2026-11-08 09:12 UTC): per-queue fences. With them on, the read-back skips its catch-up (`!unforced`) and waits on the recordings' timeline instead. That skip was written while a catch-up still forced (step 2). It merged 53 minutes after #449 had made the catch-up safe, so the skip outlived its reason. 5. **#1276** (Odrannnn): the recordings' wait sends the held batch first (RADV). `RX_VK_PER_QUEUE_FENCES=0` goes back to the old path: a catch-up, then a wait on the shared timeline. That timeline is signalled only as far as no older submission of any queue is stopped, so each read-back also waits for stopped work whose waits are not met, which waits for other threads' submissions: tens of milliseconds. ## Change In `invalidate`'s read-back, with per-queue fences on, settle before the lookup: `sync_settle()` applies the CPU's pending signals and lets on the stopped work whose waits are met, never forcing anything (the same call the texture branch further down already makes). That work is recorded, its write-back value is known, and `wait_recorded` waits for exactly that recording, as before. Stopped work whose waits are not met is not waited for. - With `RX_VK_PER_QUEUE_FENCES=0`, `RX_VK_ASYNC=1` or `RX_VK_SYNC_WAITS=0` nothing changes: those still call `wait_queue_empty()` as before. - `RX_VK_READBACK_NO_CATCHUP` still turns the catch-up off. ## What it changes, measured RX 5500M (RDNA1, 4 GB), Windows 11, AMD driver, main `92907f7` with #1581. Read-back figures come from the `readbacks of GPU results: N of 3000 waited` lines. | | main | `RX_VK_PER_QUEUE_FENCES=0` | this PR | |---|---|---|---| | read-backs that waited, per 3000 | ~890 | 65–68 | ~710 | | time per wait | 0.07 ms (0.02–0.19) | 19–87 ms | 0.65 ms median, 1.28 mean | | main thread's time waiting, per 3000 reads | 24 ms | — | 499 ms | On main the waits are tiny because they usually wait for nothing: the work that writes the range has not been recorded yet. With this PR a read-back waits for the GPU to finish the write. On the main thread that is about 0.17 ms per read, roughly 24 ms in each second of play at that run's read rate (about 2.4% of the thread). Of that run's 122 read-back lines (3000 reads each), six averaged more than 5 ms a wait; the highest, 30 ms, was during an area load. Frame rate against `RX_VK_PER_QUEUE_FENCES=0`, the workaround in #1005, at the same settings (Render resolution 50%, Hyrule Market): | | `RX_VK_PER_QUEUE_FENCES=0` | this PR | |---|---|---| | 5-second stretches | 57 | 116 | | mean fps | 12.5 | 31.8 | | stretches under 8 fps | 29 (51%) | 4 (3%) | I did not run main and this PR at the same settings on the same route, so this is no claim that the PR is faster than main. ## Tests `tests/test_readback_settle.py` (new) reads the source and checks two things: the read-back settles before the write-back lookup, and it no longer skips its catch-up with per-queue fences. It fails on current main and passes with this change. `tests/test_recorded_wait_flush.py` still passes. `tests/run_all.py` on main `bdfe09c` with this change: 311 passed. The 6 that fail are the same 6 that fail on plain main on this PC. Rebased onto `fab8cb3` since, with no conflict, and the two tests above pass there. It also merges cleanly with #1905, which changes the same wait's batch flush. Related: #1005, #523, #449, #296.
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)
FR_READBACKin the batch enum, printed in the stats log linerec_outtracks the recordings' timeline value the last flush sentwait_recorded()fires only when the exact recording being waited on is still held in the batchSafety
wait_recordedcall sites hold no locks (present_mu is a leaf)RX_VK_BATCH=0the pend is always empty so it never firesRX_VK_ASYNC=1the readback goes throughwait_value()which already flushes — this fix is load-bearing on sync mode onlyTesting
Built and deployed on RX 6700 XT. With
RX_VK_ASYNC=1the readback counter reads 0 (expected — the async path already flushes). WithRX_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.