fix(vk): with per-queue fences, a CPU read-back waits for the GPU work that writes it again #2125

github-sync · 2026-11-11T04:10:26Z

rakefire-impala commented

2026-11-11 04:10:26 +00:00

(Migrated from github.com)

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:

#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.
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.
#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).
#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.
#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.

> **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.

hckrman101 (Migrated from github.com) approved these changes

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

hckrman101 (Migrated from github.com) left a comment

Read through this against current main. The change is small and does what the description says: with the sync replay and per-queue fences on, sync_settle() lets on only the stopped work whose waits are met (it's the same burst/schedule that wait_queue_empty() does in the non-async case, minus the RX_VK_CATCHUP_FORCE escape), so the work that writes the range gets recorded, its write-back value is known, and wait_recorded waits for exactly that. Nothing is forced, and with RX_VK_PER_QUEUE_FENCES=0, RX_VK_ASYNC=1 or RX_VK_SYNC_WAITS=0 the branch is unchanged. The texture branch below already settles from the same entry point on the game's threads, so there's no new locking pattern. The history in the description (the !unforced skip from #523 outliving the reason #449 removed) matches the code, and I appreciate the edit at the top being upfront that this doesn't fix the #1005 flashes.

The cost is real but measured and bounded (about 0.17 ms per read on the main thread in your run), and far cheaper than the RX_VK_PER_QUEUE_FENCES=0 workaround.

On #2222 (Brett-D's 250 ms limit on the read-back's wait_value): no conflict. I applied both diffs to current main one after the other and both apply cleanly, and neither test checks text the other changes. They also mostly cover different setups. #2222's Deck hang is with RX_VK_EVENT_WAITS=0, where sync_waits_on() is false, so unforced is false and this PR changes nothing there. On the default path this PR sends more read-backs through wait_recorded(v, UINT64_MAX, ...), which #2222 deliberately leaves unbounded. That's fine as it stands (#1276 already fixed the batch flush there), but if a hang like #2222's ever turns up with per-queue fences on, that's the wait to look at.

One nit inline. CI is green. Looks good to me.

Read through this against current main. The change is small and does what the description says: with the sync replay and per-queue fences on, `sync_settle()` lets on only the stopped work whose waits are met (it's the same burst/schedule that `wait_queue_empty()` does in the non-async case, minus the `RX_VK_CATCHUP_FORCE` escape), so the work that writes the range gets recorded, its write-back value is known, and `wait_recorded` waits for exactly that. Nothing is forced, and with `RX_VK_PER_QUEUE_FENCES=0`, `RX_VK_ASYNC=1` or `RX_VK_SYNC_WAITS=0` the branch is unchanged. The texture branch below already settles from the same entry point on the game's threads, so there's no new locking pattern. The history in the description (the `!unforced` skip from #523 outliving the reason #449 removed) matches the code, and I appreciate the edit at the top being upfront that this doesn't fix the #1005 flashes. The cost is real but measured and bounded (about 0.17 ms per read on the main thread in your run), and far cheaper than the `RX_VK_PER_QUEUE_FENCES=0` workaround. On #2222 (Brett-D's 250 ms limit on the read-back's `wait_value`): no conflict. I applied both diffs to current main one after the other and both apply cleanly, and neither test checks text the other changes. They also mostly cover different setups. #2222's Deck hang is with `RX_VK_EVENT_WAITS=0`, where `sync_waits_on()` is false, so `unforced` is false and this PR changes nothing there. On the default path this PR sends more read-backs through `wait_recorded(v, UINT64_MAX, ...)`, which #2222 deliberately leaves unbounded. That's fine as it stands (#1276 already fixed the batch flush there), but if a hang like #2222's ever turns up with per-queue fences on, that's the wait to look at. One nit inline. CI is green. Looks good to me.

gpu/vk/vk_replay.cpp

					
				@ -2299,1 +2303,4 @@

				                unforced ? sync_settle() : wait_queue_empty();

				                w = true;

				            }

				            const auto w1 = std::chrono::steady_clock::now();

Nit: with per-queue fences this now marks every read-back that finds stopped work as waited, even when the settle had nothing to let on and v <= completed() afterwards. Before, that path only counted a real wait_recorded/wait_value. So the N of 3000 waited ... ms each line isn't quite like-for-like with main (it also pulls the per-wait average down). Not a problem for correctness. If you want the numbers to compare cleanly, you could set w only when the settle actually recorded something, or just mention it in the comment.

Nit: with per-queue fences this now marks every read-back that finds stopped work as waited, even when the settle had nothing to let on and `v <= completed()` afterwards. Before, that path only counted a real `wait_recorded`/`wait_value`. So the `N of 3000 waited ... ms each` line isn't quite like-for-like with main (it also pulls the per-wait average down). Not a problem for correctness. If you want the numbers to compare cleanly, you could set `w` only when the settle actually recorded something, or just mention it in the comment.