fix(vk): a fence wait covers every part of a split submission, not only the first #2419

Merged

willcrisis merged 3 commits from fix/value-rec-last-part into main

2026-11-11 20:29:17 +00:00

willcrisis commented

2026-11-11 18:10:58 +00:00

(Migrated from github.com)

Cause

A submission longer than RX_VK_CHUNK draws (default 120), or one stopped at an event wait and finished later, is recorded in several recordings that all carry its value. submit_recording maps the value to its recording with value_rec.emplace, which keeps the first one: when the last part finishes the submission, the key is already there and nothing changes. So wait_recorded (the game's fence waits in wait(), and the exposure readback wait) returns once the first ~120 draws are done.

With no VRAM mirror of the buffer pools (integrated GPUs), the later parts read guest memory in place, and the game has by then written the next frame's uniforms, vertices and exposure meter into it. The HUD is drawn late in the frame, so it gets the wrong transforms most often; the stale exposure readback gives the whole-screen flashes.

myylkn found the same root cause on a Radeon 8060S (#997, with a patch in a comment). This PR is a variant of that patch:

find and an update in place rather than value_rec[v], so it builds against tests/draw_stream_visibility_test.cpp, which models value_rec as a std::multimap.
It keeps registering r.value when the recording does not signal (the parts split off before the last), raised to the latest seq. A wait on a stopped submission still finds its recording, instead of falling back to the timeline wait, which forces stopped work past its event waits.

Change

gpu/vk/vk_replay.cpp, submit_recording: each value maps to the latest recording that carries it.
tests/test_value_rec_last_part.py: runs the statements cut from submit_recording, for a submission split in three parts and for a stopped submission finished in a later recording. It fails on main (value 10 -> recording 1 (expected 3), value 11 -> recording 4 (expected 5)) and passes with the change.
docs/tested-configurations.md: the three runs below.

Tested

ROG Ally X (Ryzen Z1 Extreme, RADV PHOENIX, Mesa 26.2.3), Linux (Anatase, Fedora-based), counting one-frame luminance spikes in screen recordings (a frame brighter or darker than both its neighbours):

v2.0.13, Hyrule Field	One-frame spikes	15/255 or more	`GPU waits in 5 s`
without the change (69 s)	104	15	4–109 sync waits, 31–232 ms
with the change (42 s)	3	0 (the largest is a sword hit)	103–137 sync waits, 1.4–1.8 s

Then about a day and a half of play with the change, rebuilt for v2.0.14 and v2.1.0 (the renderer alone, RX_VK_ONLY=1): no flashes or stray HUD seen in Hyrule Field, Death Mountain Trail and Goron City, in Desktop Mode and in Steam's Gaming Mode. A 57 s Gaming Mode recording on v2.0.14 had no one-frame spikes at all.

It costs frame rate on this device. The FPS overlay in the field read roughly 45–70 before and 38–49 after (two recordings, not a controlled benchmark): part of the old frame rate was the game running ahead of the GPU. The waits now last as long as the GPU work they stand for.

python -I -B tests/run_all.py with the Vulkan headers: the same results on a clean checkout of main with and without the change, apart from the new test, which passes. test_draw_stream_visibility, test_capture_slot_ownership and test_parallel_rb_refresh, which compile submit_recording, pass.

Not tested: Windows, NVIDIA and discrete AMD cards (the early wait is there too, but their VRAM mirror is synced at the submit, which should hide it; not measured), RX_VK_PARALLEL segments (off by default; the same mapping applies to them), and the Steam Deck.

Part of #997 (AMD handhelds and APUs tracker).
#1795 copies uniform buffers on GPUs without mirrors for the same symptoms. The two can coexist: that one keeps a late write from reaching the GPU; this one makes the fence waits cover the work they stand for. An A/B of #1795 with this change in and RX_VK_UB_SNAPSHOT=0 would show how much of its effect was this wait.
#2125 (merged) lets stopped work finish before a CPU readback. Complementary: the readback's wait that follows goes through value_rec, which this corrects.

Screenshots

Before (v2.0.13, Hyrule Field and Castle Town; each strip is the frame before, the broken frame, the frame after):

After (v2.0.13 with the change, Hyrule Field, fights included; the last strip is the largest one-frame spike left, a sword hit):

The analysis and the change were made with AI assistance (Claude); the testing and recordings are mine.

🤖 Generated with Claude Code

On AMD integrated GPUs (ROG Ally, Legion Go, Steam Deck) the picture breaks for one frame at a time: the HUD drawn stretched across the screen, grass flat white, the whole screen flashing. The game rewrites memory the GPU is still reading, because its fence waits return after the first part of a submission. ## Cause A submission longer than `RX_VK_CHUNK` draws (default 120), or one stopped at an event wait and finished later, is recorded in several recordings that all carry its value. `submit_recording` maps the value to its recording with `value_rec.emplace`, which keeps the **first** one: when the last part finishes the submission, the key is already there and nothing changes. So `wait_recorded` (the game's fence waits in `wait()`, and the exposure readback wait) returns once the first ~120 draws are done. With no VRAM mirror of the buffer pools (integrated GPUs), the later parts read guest memory in place, and the game has by then written the next frame's uniforms, vertices and exposure meter into it. The HUD is drawn late in the frame, so it gets the wrong transforms most often; the stale exposure readback gives the whole-screen flashes. myylkn found the same root cause on a Radeon 8060S (#997, with a patch in a comment). This PR is a variant of that patch: - `find` and an update in place rather than `value_rec[v]`, so it builds against `tests/draw_stream_visibility_test.cpp`, which models `value_rec` as a `std::multimap`. - It keeps registering `r.value` when the recording does not signal (the parts split off before the last), raised to the latest seq. A wait on a stopped submission still finds its recording, instead of falling back to the timeline wait, which forces stopped work past its event waits. ## Change - `gpu/vk/vk_replay.cpp`, `submit_recording`: each value maps to the latest recording that carries it. - `tests/test_value_rec_last_part.py`: runs the statements cut from `submit_recording`, for a submission split in three parts and for a stopped submission finished in a later recording. It fails on `main` (`value 10 -> recording 1 (expected 3)`, `value 11 -> recording 4 (expected 5)`) and passes with the change. - `docs/tested-configurations.md`: the three runs below. ## Tested ROG Ally X (Ryzen Z1 Extreme, RADV PHOENIX, Mesa 26.2.3), Linux (Anatase, Fedora-based), counting one-frame luminance spikes in screen recordings (a frame brighter or darker than both its neighbours): | v2.0.13, Hyrule Field | One-frame spikes | 15/255 or more | `GPU waits in 5 s` | |---|---|---|---| | without the change (69 s) | 104 | 15 | 4–109 sync waits, 31–232 ms | | with the change (42 s) | 3 | 0 (the largest is a sword hit) | 103–137 sync waits, 1.4–1.8 s | Then about a day and a half of play with the change, rebuilt for v2.0.14 and v2.1.0 (the renderer alone, `RX_VK_ONLY=1`): no flashes or stray HUD seen in Hyrule Field, Death Mountain Trail and Goron City, in Desktop Mode and in Steam's Gaming Mode. A 57 s Gaming Mode recording on v2.0.14 had no one-frame spikes at all. **It costs frame rate on this device.** The FPS overlay in the field read roughly 45–70 before and 38–49 after (two recordings, not a controlled benchmark): part of the old frame rate was the game running ahead of the GPU. The waits now last as long as the GPU work they stand for. `python -I -B tests/run_all.py` with the Vulkan headers: the same results on a clean checkout of `main` with and without the change, apart from the new test, which passes. `test_draw_stream_visibility`, `test_capture_slot_ownership` and `test_parallel_rb_refresh`, which compile `submit_recording`, pass. **Not tested:** Windows, NVIDIA and discrete AMD cards (the early wait is there too, but their VRAM mirror is synced at the submit, which should hide it; not measured), `RX_VK_PARALLEL` segments (off by default; the same mapping applies to them), and the Steam Deck. ## Related - Part of #997 (AMD handhelds and APUs tracker). - #1795 copies uniform buffers on GPUs without mirrors for the same symptoms. The two can coexist: that one keeps a late write from reaching the GPU; this one makes the fence waits cover the work they stand for. An A/B of #1795 with this change in and `RX_VK_UB_SNAPSHOT=0` would show how much of its effect was this wait. - #2125 (merged) lets stopped work finish before a CPU readback. Complementary: the readback's wait that follows goes through `value_rec`, which this corrects. ## Screenshots Before (v2.0.13, Hyrule Field and Castle Town; each strip is the frame before, the broken frame, the frame after): <img width="1920" height="335" alt="before-1" src="https://github.com/user-attachments/assets/4d3ee32e-8575-462e-b972-b3bb16ec2f3e" /> <img width="1920" height="335" alt="before-2" src="https://github.com/user-attachments/assets/2231f72b-3394-46b0-b67d-647674712a29" /> <img width="1920" height="335" alt="before-3" src="https://github.com/user-attachments/assets/d2d645a7-bf2a-41c3-979a-19b2a36562ff" /> <img width="1920" height="335" alt="before-4" src="https://github.com/user-attachments/assets/366157ae-41e2-48a4-b4cd-7fc96f817eaa" /> After (v2.0.13 with the change, Hyrule Field, fights included; the last strip is the largest one-frame spike left, a sword hit): <img width="1920" height="333" alt="after-1" src="https://github.com/user-attachments/assets/9f025025-1034-47ab-bfe9-0ea1476db075" /> <img width="1920" height="333" alt="after-2-largest-spike-is-a-sword-hit" src="https://github.com/user-attachments/assets/e7e26d16-ba97-4982-bed0-670306352113" /> The analysis and the change were made with AI assistance (Claude); the testing and recordings are mine. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Joey-Magnus (Migrated from github.com) approved these changes

2026-11-11 20:08:30 +00:00

Joey-Magnus (Migrated from github.com) left a comment

This looks right to me, and it's a well-made PR: a clear cause, a fixture cut from the real code, and before/after recordings on hardware. I confirmed that emplace keeps the first part's seq, and that maybe_split (gpu/vk/vk_replay.cpp:1321), the parallel segments (:1480) and finish_recording (:1892) all submit recordings that carry the same r.value. Before this change, wait_recorded therefore waited on the first part. I ran test_value_rec_last_part, test_draw_stream_visibility, test_capture_slot_ownership and test_parallel_rb_refresh on the PR's head, and they pass. As a negative control I pointed the new test at main's vk_replay.cpp with --source: it fails the two cases you list. The frame-rate cost is honest and expected, since the waits now cover the work they stand for.

question gpu/vk/vk_replay.cpp:382-388 and :2143-2150: a window remains while a submission is still being recorded. Once the first part has gone through maybe_split, value_rec[v] is that part's seq until the last part is submitted. wait() doesn't take replay_mu, and q_last/value_queue are set before submit_job (:4261). So another game thread that waits on that queue's fence in that window still has mine true (the queue isn't blocked) and returns after the first part only. The same holds after a stopped submission is let go and while its remainder is being recorded. This is much narrower than before, when the mapping stayed wrong for good, so I don't think it should hold the PR up. If you want to close it, one option is to mark the value as "still recording" (for example, a set under rec_map_mu that run_submit fills while it records and finish_recording clears). wait_recorded would then return false for a marked value and fall back to the timeline wait. If it isn't worth it, a one-line note in the comment that the map is final only once the last part is submitted would help the next reader.

minor: the PR conflicts with main in docs/tested-configurations.md only (rows added at the top since). The code merges cleanly.

This looks right to me, and it's a well-made PR: a clear cause, a fixture cut from the real code, and before/after recordings on hardware. I confirmed that `emplace` keeps the first part's `seq`, and that `maybe_split` (`gpu/vk/vk_replay.cpp:1321`), the parallel segments (`:1480`) and `finish_recording` (`:1892`) all submit recordings that carry the same `r.value`. Before this change, `wait_recorded` therefore waited on the first part. I ran `test_value_rec_last_part`, `test_draw_stream_visibility`, `test_capture_slot_ownership` and `test_parallel_rb_refresh` on the PR's head, and they pass. As a negative control I pointed the new test at main's `vk_replay.cpp` with `--source`: it fails the two cases you list. The frame-rate cost is honest and expected, since the waits now cover the work they stand for. **question** `gpu/vk/vk_replay.cpp:382-388` and `:2143-2150`: a window remains while a submission is still being recorded. Once the first part has gone through `maybe_split`, `value_rec[v]` is that part's `seq` until the last part is submitted. `wait()` doesn't take `replay_mu`, and `q_last`/`value_queue` are set before `submit_job` (`:4261`). So another game thread that waits on that queue's fence in that window still has `mine` true (the queue isn't blocked) and returns after the first part only. The same holds after a stopped submission is let go and while its remainder is being recorded. This is much narrower than before, when the mapping stayed wrong for good, so I don't think it should hold the PR up. If you want to close it, one option is to mark the value as "still recording" (for example, a set under `rec_map_mu` that `run_submit` fills while it records and `finish_recording` clears). `wait_recorded` would then return false for a marked value and fall back to the timeline wait. If it isn't worth it, a one-line note in the comment that the map is final only once the last part is submitted would help the next reader. **minor**: the PR conflicts with main in `docs/tested-configurations.md` only (rows added at the top since). The code merges cleanly.