fix(vk): HUD and lighting artifacts on GPUs without buffer mirrors #1795

github-sync · 2026-11-10T16:57:57Z

Brett-D commented

2026-11-10 16:57:57 +00:00

(Migrated from github.com)

On integrated GPUs without VRAM mirrors, a uniform buffer bound in place is the game's own memory, and the game rewrites it right after the submit while the GPU still runs. #2464 fixed that for Intel; this PR extends the same copy to the other integrated GPUs (measured on a Steam Deck) and reuses the copy within a recording.

What it changes

Default: RX_VK_UB_SNAPSHOT is on when the pools have no mirror and the GPU is Intel (#2464's rule, kept) or integrated (VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU). Discrete GPUs without mirrors (GCN since #2343, RX_VK_MIRROR=0) keep main's default (off) until someone measures them.
Reuse: a copied uniform buffer serves the recording's later bindings of the same address and size while the capture view is unchanged (RX_VK_UB_SNAPSHOT_REUSE=0 turns it off). A copy whose read failed is not kept.
Switches: both are read as env_on reads a switch (unset uses the default; 0/off/no/false/empty is off).
Removed from this PR: the old third commit (event waits off without mirrors). It may come back as its own PR, with the design note, once the cause is clear.

Steam Deck, current main (`6749ba21` + this)

Kokiri Forest, Game Mode, 30 s frame logs, 80 screenshots a start. Frame generation was on, so FPS is presented; the game ran ~14-15 real.

	starts	presented FPS	p95	freezes
snapshot on (this PR)	~25	28.7-30.1	64-66 ms	0
snapshot off (main's default here)	3	30.1, 30.0, froze	54 ms	1: a capture's and a readback's write-back never came, in a conversation
main's defaults (snapshot off, event waits on)	3	27.8, 10.7 (a 767 ms stop at an event wait), load stall	68 / 469 ms	2 of 3 bad

With the snapshot off on current main, the screenshots are clean: #2419 did remove the slabs and stretched HUD seen before. The copy is still needed on the Deck for stability, at ~1 fps. The Linux suite in an Ubuntu 22.04 builder fails only what main fails there (test_prof_clock, test_cpp_preflight: the container's g++ 11).

On integrated GPUs without VRAM mirrors, a uniform buffer bound in place is the game's own memory, and the game rewrites it right after the submit while the GPU still runs. #2464 fixed that for Intel; this PR extends the same copy to the other integrated GPUs (measured on a Steam Deck) and reuses the copy within a recording. ## What it changes - **Default:** `RX_VK_UB_SNAPSHOT` is on when the pools have no mirror and the GPU is Intel (#2464's rule, kept) or integrated (`VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU`). Discrete GPUs without mirrors (GCN since #2343, `RX_VK_MIRROR=0`) keep main's default (off) until someone measures them. - **Reuse:** a copied uniform buffer serves the recording's later bindings of the same address and size while the capture view is unchanged (`RX_VK_UB_SNAPSHOT_REUSE=0` turns it off). A copy whose read failed is not kept. - **Switches:** both are read as `env_on` reads a switch (unset uses the default; 0/off/no/false/empty is off). - **Removed from this PR:** the old third commit (event waits off without mirrors). It may come back as its own PR, with the design note, once the cause is clear. ## Steam Deck, current main (`6749ba21` + this) Kokiri Forest, Game Mode, 30 s frame logs, 80 screenshots a start. Frame generation was on, so FPS is presented; the game ran ~14-15 real. | | starts | presented FPS | p95 | freezes | |---|---|---|---|---| | snapshot on (this PR) | ~25 | 28.7-30.1 | 64-66 ms | 0 | | snapshot off (main's default here) | 3 | 30.1, 30.0, froze | 54 ms | 1: a capture's and a readback's write-back never came, in a conversation | | main's defaults (snapshot off, event waits on) | 3 | 27.8, 10.7 (a 767 ms stop at an event wait), load stall | 68 / 469 ms | 2 of 3 bad | With the snapshot off on current main, the screenshots are clean: #2419 did remove the slabs and stretched HUD seen before. The copy is still needed on the Deck for stability, at ~1 fps. The Linux suite in an Ubuntu 22.04 builder fails only what main fails there (`test_prof_clock`, `test_cpp_preflight`: the container's g++ 11).

Brett-D commented

2026-11-10 17:25:27 +00:00

(Migrated from github.com)

@NotRexed When you have time, could you please review this? It fixes HUD and lighting artifacts on integrated GPUs (measured on a Steam Deck); discrete GPUs keep their current behavior. Thank you!

hckrman101 commented

2026-11-10 17:50:47 +00:00

(Migrated from github.com)

Heads-up: this conflicts with main now (a lot landed today). Could you merge main into the branch (or rebase) and resolve the conflicts? Thanks!

Brett-D commented

2026-11-11 01:39:52 +00:00

(Migrated from github.com)

Merged current main (f8efe05) into the branch. The only conflict was tests/run_all.py: main now reads each test's own declarations, so the two new tests declare RUN_ALL_ARGS = ["--cxx", "{cxx}"] (and skip on Windows, as their fixtures set the switch with POSIX setenv). The diff against main is the three commits again. Linux game-free suite: identical to clean main apart from the two added passing tests; a full Linux x86-64-v3 game build of main plus this compiles with 0 failures.

Merged current main (f8efe05) into the branch. The only conflict was tests/run_all.py: main now reads each test's own declarations, so the two new tests declare `RUN_ALL_ARGS = ["--cxx", "{cxx}"]` (and skip on Windows, as their fixtures set the switch with POSIX setenv). The diff against main is the three commits again. Linux game-free suite: identical to clean main apart from the two added passing tests; a full Linux x86-64-v3 game build of main plus this compiles with 0 failures.

hckrman101 (Migrated from github.com) reviewed

2026-11-11 02:18:08 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks for the merge with main and for the Steam Deck numbers, they make this easy to follow.

Uniform-buffer snapshot (commits 1-2): looks right to me. Binding a UB in place on a no-mirror device reads live guest memory, which breaks design rule 2 ("nothing the replay reads comes from live guest memory"). Copying through memory_read_as_submitted is the fix that rule asks for, and discrete GPUs keep their path. On the reuse cache, I checked:

the key is safe: cap_view_gen is bumped on every page stamp, every collected write and every capture publish, so a hit at the same view gives the bytes a fresh read would. That's the same rule the existing submitted_page cache uses.
ub_overlay runs before the cache, so inline UB updates later in the recording still win.
streams that are moved off stay alive for the recording (as c1_at relies on), and the map is cleared per recording in draw_begin_recording.
cur is thread_local, so there's no sharing between threads.

Nit: when memory_read_as_submitted returns false, the unread copy is now cached and served to later bindings. That's harmless in practice, since each binding used to get its own unread copy anyway.

Event waits off without mirrors (commit 3): I think this needs NotRexed's call, and I'd rather it went in separately.

docs/design.md §4 rule 1 says cross-queue waits are honoured and RX_VK_EVENT_WAITS=0 is "for A/B only". This makes it the default on every no-mirror device, including Windows iGPUs nobody has tested, and the doc isn't updated. The comment above sync_waits_on lists what the ordering fixed: lit surfaces black in 3-9% of frames, and passes reading another queue's output before it was written (55% of waits). Your 80-shot luminance numbers suggest that didn't come back in Kokiri, but that's one scene on one device.
I think your diagnosis is right, and it points at a bug for every GPU, not just iGPUs. On the default (non-async) path, register_texture_queued / register_sampler_queued apply at once on the game's thread. So a submission that stops at a wait and resumes later resolves handles against registrations made in the meantime, which breaks rule 3. Discrete GPUs only hit it less because the replay is less far behind. A fix that holds registrations while a queue is stopped, the way held_tex_releases already holds releases, would keep the ordering everywhere and fix the cause. Gating on memory_mirrors_on() uses "has mirrors" as a stand-in for "the replay runs far behind": a discrete user with RX_VK_MIRROR=0 silently loses event ordering, and a Deck user with RX_VK_MIRROR=1 gets the bug back.

CI: the three failing Python jobs fail on test_make_game_ngx.py and test_pipeline_state_portable.py, which fail on main too (run 42942398373). The two new tests pass on Linux and are declared with RUN_ALL_ARGS / RUN_ALL_SKIP. The Windows back-end build is green.

My suggestion: land commits 1-2 as they are, and either split commit 3 out or get NotRexed's sign-off on it, with docs/design.md rule 1 updated if it stays.

Thanks for the merge with main and for the Steam Deck numbers, they make this easy to follow. **Uniform-buffer snapshot (commits 1-2): looks right to me.** Binding a UB in place on a no-mirror device reads live guest memory, which breaks design rule 2 ("nothing the replay reads comes from live guest memory"). Copying through `memory_read_as_submitted` is the fix that rule asks for, and discrete GPUs keep their path. On the reuse cache, I checked: - the key is safe: `cap_view_gen` is bumped on every page stamp, every collected write and every capture publish, so a hit at the same view gives the bytes a fresh read would. That's the same rule the existing `submitted_page` cache uses. - `ub_overlay` runs before the cache, so inline UB updates later in the recording still win. - streams that are moved off stay alive for the recording (as `c1_at` relies on), and the map is cleared per recording in `draw_begin_recording`. - `cur` is thread_local, so there's no sharing between threads. Nit: when `memory_read_as_submitted` returns false, the unread copy is now cached and served to later bindings. That's harmless in practice, since each binding used to get its own unread copy anyway. **Event waits off without mirrors (commit 3): I think this needs NotRexed's call, and I'd rather it went in separately.** - `docs/design.md` §4 rule 1 says cross-queue waits are honoured and `RX_VK_EVENT_WAITS=0` is "for A/B only". This makes it the default on every no-mirror device, including Windows iGPUs nobody has tested, and the doc isn't updated. The comment above `sync_waits_on` lists what the ordering fixed: lit surfaces black in 3-9% of frames, and passes reading another queue's output before it was written (55% of waits). Your 80-shot luminance numbers suggest that didn't come back in Kokiri, but that's one scene on one device. - I think your diagnosis is right, and it points at a bug for every GPU, not just iGPUs. On the default (non-async) path, `register_texture_queued` / `register_sampler_queued` apply at once on the game's thread. So a submission that stops at a wait and resumes later resolves handles against registrations made in the meantime, which breaks rule 3. Discrete GPUs only hit it less because the replay is less far behind. A fix that holds registrations while a queue is stopped, the way `held_tex_releases` already holds releases, would keep the ordering everywhere and fix the cause. Gating on `memory_mirrors_on()` uses "has mirrors" as a stand-in for "the replay runs far behind": a discrete user with `RX_VK_MIRROR=0` silently loses event ordering, and a Deck user with `RX_VK_MIRROR=1` gets the bug back. **CI:** the three failing Python jobs fail on `test_make_game_ngx.py` and `test_pipeline_state_portable.py`, which fail on main too (run 42942398373). The two new tests pass on Linux and are declared with `RUN_ALL_ARGS` / `RUN_ALL_SKIP`. The Windows back-end build is green. My suggestion: land commits 1-2 as they are, and either split commit 3 out or get NotRexed's sign-off on it, with `docs/design.md` rule 1 updated if it stays.

gpu/vk/vk_draw.cpp

Nit: the return value is ignored, so with reuse on, an unreadable range's copy gets cached and reused. Not a regression (each binding used to get its own unread copy), but if (memory_read_as_submitted(...) && view) on the insert below would avoid it.

Nit: the return value is ignored, so with reuse on, an unreadable range's copy gets cached and reused. Not a regression (each binding used to get its own unread copy), but `if (memory_read_as_submitted(...) && view)` on the insert below would avoid it.

gpu/vk/vk_replay.cpp

					
				@ -1754,0 +1758,4 @@

				    static const bool v = [] {

				        const char* e = getenv("RX_VK_EVENT_WAITS");

				        const bool on = e ? atoi(e) != 0 : memory_mirrors_on();

				        logf("vk: queues %s (RX_VK_EVENT_WAITS=%d)", on ? "ordered by their events" : "replayed in submit order", on ? 1 : 0);

This ties the queue-ordering default to the mirror setting, so RX_VK_MIRROR now changes two unrelated things. The cause you describe (a stopped submission seeing registrations made since) looks like register_texture_queued applying immediately when !async_on(). Holding registrations while blocked_n > 0, as releases already are, might fix it for every GPU without dropping the ordering. Either way, this goes against design.md §4 rule 1, so that rule needs updating if this stays.

This ties the queue-ordering default to the mirror setting, so `RX_VK_MIRROR` now changes two unrelated things. The cause you describe (a stopped submission seeing registrations made since) looks like `register_texture_queued` applying immediately when `!async_on()`. Holding registrations while `blocked_n` > 0, as releases already are, might fix it for every GPU without dropping the ordering. Either way, this goes against design.md §4 rule 1, so that rule needs updating if this stays.

Brett-D commented

2026-11-11 09:07:06 +00:00

(Migrated from github.com)

Related finding from Steam Deck testing with this PR's defaults (no mirrors, so RX_VK_EVENT_WAITS=0): on current main, invalidate()'s readback wait can block forever in wait_value(). It hung the game after a save loaded in 2 of 8 starts. #2222 bounds that wait at 250 ms (0 of 4 hung, gameplay unchanged). The two PRs are independent, but #2222 is the one that matters for anyone running with RX_VK_EVENT_WAITS=0.

Related finding from Steam Deck testing with this PR's defaults (no mirrors, so `RX_VK_EVENT_WAITS=0`): on current main, `invalidate()`'s readback wait can block forever in `wait_value()`. It hung the game after a save loaded in 2 of 8 starts. #2222 bounds that wait at 250 ms (0 of 4 hung, gameplay unchanged). The two PRs are independent, but #2222 is the one that matters for anyone running with `RX_VK_EVENT_WAITS=0`.

Joey-Magnus (Migrated from github.com) reviewed

2026-11-11 21:46:08 +00:00

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

Two things on main change the picture since the Deck numbers were taken. #2419 (merged) fixed fence waits returning after the first part of a split submission, and its description lists the same Deck symptoms (stretched HUD, white grass, whole-screen flashes). Could you re-run the 40-shot test on current main with RX_VK_UB_SNAPSHOT unset? If the slabs are gone, the default flip may not be needed at all. Second, #2343 turned mirrors off by default on GCN discrete cards (vk_memory.cpp:2102). Keying on memory_mirrors_on() now also flips both defaults on RX 400/500 and Vega, and nobody has measured that. The rebase conflicts with main's env_on read at vk_draw.cpp:2774. The new reads use getenv/atoi and treat an empty RX_VK_UB_SNAPSHOT as on, which is the opposite of env_on (#2042), so please switch them to env_on. hckrman101's point on commit 3 (event waits off without mirrors, against docs/design.md rule 1) is still open. I'd split that commit out.

hckrman101 commented

2026-11-11 22:55:36 +00:00

(Migrated from github.com)

This now conflicts with main in gpu/vk/vk_draw.cpp. Could you merge main into the branch (or rebase) when you get a chance? I'll take another look once it's up to date.

This now conflicts with main in `gpu/vk/vk_draw.cpp`. Could you merge main into the branch (or rebase) when you get a chance? I'll take another look once it's up to date.

github-sync commented

2026-11-12 09:32:02 +00:00

@Brett-D commented on GitHub · 2026-11-12 08:54 UTC · original

@Joey-Magnus @hckrman101 Thanks for the reviews. I merged current main (46780503) and changed the PR as you asked:

Re-run on current main with RX_VK_UB_SNAPSHOT unset (main's default on AMD is off): the screenshots are clean now. No slabs and no stretched HUD in 160 shots, so #2419 fixed those. But 1 of 3 starts froze in a conversation, and the log shows a capture's and a readback's wait for write-back N timing out with the timeline stuck at N-1. Main's own defaults (snapshot off, event waits on) went 1 good start of 3: one stopped up to 767 ms at an event wait (10.7 fps, p95 469 ms), and one stalled while loading. With the copy, there were 0 freezes in ~25 starts, at ~1 fps. So I've kept the default on, narrowed as below.
GCN discrete cards: the default is now "Intel (#2464's rule) or VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU, with no mirrors", so mirrorless discrete GPUs keep main's behaviour. memory_mirrors_on() is gone and the PR uses main's pools_mirrored().
env_on: RX_VK_UB_SNAPSHOT and RX_VK_UB_SNAPSHOT_REUSE follow hookshot_env.h. The test covers unset, empty, 0/off/no/false and 1/on/yes/true, plus integrated, discrete and Intel.
Unreadable copy: a copy whose memory_read_as_submitted failed is no longer kept for reuse.
Event waits (old commit 3): removed from this PR, as you suggested. On the Deck, event ordering on current main still shows problems: the stalls above, and with the copy on, missing sunlit ground lighting in every shot at ~1.5 fps less. I haven't checked hckrman101's theory (hold texture registrations while blocked_n > 0). If that commit comes back, it will be its own PR with the design.md rule 1 update.

> [@Brett-D](https://github.com/Brett-D) commented on GitHub · 2026-11-12 08:54 UTC · [original](https://github.com/NotRexed/Hookshot/pull/1795#issuecomment-6819183992) @Joey-Magnus @hckrman101 Thanks for the reviews. I merged current main (`46780503`) and changed the PR as you asked: - **Re-run on current main with `RX_VK_UB_SNAPSHOT` unset (main's default on AMD is off):** the screenshots are clean now. No slabs and no stretched HUD in 160 shots, so #2419 fixed those. But 1 of 3 starts froze in a conversation, and the log shows a capture's and a readback's wait for write-back N timing out with the timeline stuck at N-1. Main's own defaults (snapshot off, event waits on) went 1 good start of 3: one stopped up to 767 ms at an event wait (10.7 fps, p95 469 ms), and one stalled while loading. With the copy, there were 0 freezes in ~25 starts, at ~1 fps. So I've kept the default on, narrowed as below. - **GCN discrete cards:** the default is now "Intel (#2464's rule) or `VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU`, with no mirrors", so mirrorless discrete GPUs keep main's behaviour. `memory_mirrors_on()` is gone and the PR uses main's `pools_mirrored()`. - **env_on:** `RX_VK_UB_SNAPSHOT` and `RX_VK_UB_SNAPSHOT_REUSE` follow `hookshot_env.h`. The test covers unset, empty, 0/off/no/false and 1/on/yes/true, plus integrated, discrete and Intel. - **Unreadable copy:** a copy whose `memory_read_as_submitted` failed is no longer kept for reuse. - **Event waits (old commit 3):** removed from this PR, as you suggested. On the Deck, event ordering on current main still shows problems: the stalls above, and with the copy on, missing sunlit ground lighting in every shot at ~1.5 fps less. I haven't checked hckrman101's theory (hold texture registrations while `blocked_n > 0`). If that commit comes back, it will be its own PR with the design.md rule 1 update.

github-sync commented

2026-11-12 14:41:35 +00:00

@Joey-Magnus approved on GitHub · 2026-11-12 14:32 UTC · original

My points are handled. The default now applies to Intel or VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU without mirrors, using main's pools_mirrored(), so GCN discrete cards keep main's behaviour. Both switches follow env_on's rule, a copy whose read failed isn't kept, and the event-wait commit is gone. Thanks for re-running on current main with the snapshot off, which answers whether the copy is still needed. I merged d89045e into current main (clean) and ran test_ub_snapshot_default and test_env_on, and both pass. The only red CI job is test_release_scripts, which is flaky.

One thing for maintainers: INTEGRATED_GPU covers more than the Deck. AMD APUs on Windows (the Ally, 680M and 780M laptops) get the copy by default too, and it was only measured on the Deck. Design rule 2 asks for the copy, so I think it's the right default, but it's worth knowing.

The title no longer matches the change. Since #2419 the HUD and lighting artifacts are gone, and what this fixes now is the Deck freezing in conversations. Something like "fix(vk): copy uniform buffers as submitted on integrated GPUs without mirrors" would read better in the changelog.

> [@Joey-Magnus](https://github.com/Joey-Magnus) approved on GitHub · 2026-11-12 14:32 UTC · [original](https://github.com/NotRexed/Hookshot/pull/1795#pullrequestreview-5876201520) My points are handled. The default now applies to Intel or VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU without mirrors, using main's pools_mirrored(), so GCN discrete cards keep main's behaviour. Both switches follow env_on's rule, a copy whose read failed isn't kept, and the event-wait commit is gone. Thanks for re-running on current main with the snapshot off, which answers whether the copy is still needed. I merged d89045e into current main (clean) and ran test_ub_snapshot_default and test_env_on, and both pass. The only red CI job is test_release_scripts, which is flaky. One thing for maintainers: INTEGRATED_GPU covers more than the Deck. AMD APUs on Windows (the Ally, 680M and 780M laptops) get the copy by default too, and it was only measured on the Deck. Design rule 2 asks for the copy, so I think it's the right default, but it's worth knowing. The title no longer matches the change. Since #2419 the HUD and lighting artifacts are gone, and what this fixes now is the Deck freezing in conversations. Something like "fix(vk): copy uniform buffers as submitted on integrated GPUs without mirrors" would read better in the changelog.

github-sync referenced this pull request

2026-11-12 14:43:22 +00:00

fix(vk): the uniform buffer snapshot keys on the binding's pool, not the device #2609

github-sync referenced this pull request

2026-11-12 15:49:18 +00:00

[Tracking] AMD handhelds and APUs (ROG Ally / Ally X, Legion Go, Radeon 780M/860M/890M): black screen, device lost, reboot #997

Commenting is not possible because the repository is archived.

No reviewers