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

Open

blessedplummer wants to merge 1 commit from blessedplummer/ub-snapshot-pr into main

blessedplummer commented

2026-11-12 01:49:42 +00:00

(Migrated from github.com)

Why

#1795's in-place hazard: a uniform buffer bound as the pool's guest memory is read by the GPU when the recording executes, and the game rewrites its constants right after the submit — the Steam Deck drew giant HUD hearts, slabs and stretched geometry from exactly this. The fix proposed there keys the snapshot globally (no mirrors on the device ⇒ copy every uniform buffer), which covers integrated GPUs, RX_VK_MIRROR=0 and GCN's default — but leaves the mixed states of a mirrored device exposed: a pool whose mirror the budget refused, or whose make is queued or deferred, binds the same guest memory with the same hazard while mirrors run everywhere else.

This keys the condition per binding instead of globally: the protection follows the hazard itself, so any mirror-less bind on any device is safe — including mixed states that don't exist yet.

What

gpu_buffer_bind tags the bindings that ARE the pool's guest memory (BufferRef::live, set by its three direct paths — the mirror-less states, and only where a recording will execute them), and the uniform-buffer path copies exactly those as submitted. A mirror's binding stays in place (it was synced at the submit); a bind outside a recording executes nothing.

The states (where behavior changes on unset)

MIRROR_REFUSED — the mirror budget's refusals (pools > 1 GB refused outright; budget-full): in-place at every read on a mirrored device. Previously exposed; now copied.
MIRROR_WANTED — a make queued or deferred: the first bind under RX_VK_MIRROR_DEFER=1, a parallel replay's segments (the make is skipped), a pool already written this submission, a failed make retried at submission start. In-place until made. Previously exposed; now copied.
Global-off degenerate (integrated GPUs, RX_VK_MIRROR=0, GCN's default): every pool binds guest memory ⇒ every uniform buffer copied — byte-identical to the keying #1795 proposed (live-measured below).

Unchanged on unset: mirror binds (submit-synced, in place), binds outside a recording, the async-import stream copies, unaligned/outside-pool binds (already copied), and a mirrored device with no refusals (live-measured: zero copies, as on main).

Switch semantics (a plain note, since one documented behavior is superseded)

On main, unset and RX_VK_UB_SNAPSHOT=0 both mean fully-off (in-place everywhere). After this, unset and =0 both mean the per-binding rule, and =1 still copies every uniform buffer without asking the pools. The in-place opt-out for a mirror-less device is RX_VK_MIRROR=1, whose submit-time sync makes it safe. #1795's "=0 restores the in-place binding" semantic is superseded by this. Storage buffers, vertex and index binds are out of scope, as in #1795.

Verification

Game-free tests: tests/test_ub_snapshot_per_pool.py compiles the production selection hunk (cut from vk_draw.cpp) with the bind stubbed — 7 scenarios: mirror bind in place; mirror-less bind copied; outside-pools copied; misaligned copied; =1 copies everything without asking the pools; =0 is the per-binding default. Plus source needles (the live tag, the three tagged direct returns count-checked, the !b.live condition). 7/7 pass.
Full suite vs pristine at base 6f7ab2a7: 440 passed / 1 failed / 33 skipped on this branch, pristine 439/1/33 — the 1 is upstream's own test_env_on failure (pre-existing on pristine, tracked in the thread); this branch adds exactly its 1 test. Build: 0 failed.
Boot smokes on device, 0 faults in all three modes: unset → 0 copies (mirrored dGPU, no hazard states reached at the title screen — unchanged as claimed); RX_VK_UB_SNAPSHOT=1 → 1,275,001 copies, never asking the pools; RX_VK_MIRROR=0 → 1,280,001 copies — the global-off degenerate case, byte-identical volume to the keying #1795 proposed.

Cost

Measured at parity on gameplay, not inferred: the RX_VK_DGPU_TRIALS=0 discriminator in the companion thread measured this snapshot path at +0.9% (inside run-to-run noise, adjacent warp-normalized pairs) while it was serving ~14M stream copies per run. Correctness fix, noise cost.

Provenance & scope

Based on upstream/main 6f7ab2a7. No policy machinery travels with this: no trials, no skip list, no counters, no RX_VK_DGPU gating — the diff reads as a refinement of #1795's own fix.
All measurements on one rig, named per the companion thread's discipline: RX 9070 XT (Navi 48), ASRock Steel Legend, RADV/Mesa on Linux, PCIe Gen5 x16, ReBAR enabled.
Extracted from the companion dGPU-policy PR #2338, where the hazard analysis, the parity-cost discriminator measurement, and the hardware identification were established; this PR carries only the correctness piece, independent of that PR's fate.

## Why #1795's in-place hazard: a uniform buffer bound as the pool's guest memory is read by the GPU when the recording executes, and the game rewrites its constants right after the submit — the Steam Deck drew giant HUD hearts, slabs and stretched geometry from exactly this. The fix proposed there keys the snapshot **globally** (no mirrors on the device ⇒ copy every uniform buffer), which covers integrated GPUs, `RX_VK_MIRROR=0` and GCN's default — but leaves the **mixed states of a mirrored device** exposed: a pool whose mirror the budget refused, or whose make is queued or deferred, binds the same guest memory with the same hazard while mirrors run everywhere else. This keys the condition **per binding** instead of globally: the protection follows the hazard itself, so any mirror-less bind on any device is safe — including mixed states that don't exist yet. ## What `gpu_buffer_bind` tags the bindings that ARE the pool's guest memory (`BufferRef::live`, set by its three direct paths — the mirror-less states, and only where a recording will execute them), and the uniform-buffer path copies exactly those as submitted. A mirror's binding stays in place (it was synced at the submit); a bind outside a recording executes nothing. ## The states (where behavior changes on unset) 1. **MIRROR_REFUSED** — the mirror budget's refusals (pools > 1 GB refused outright; budget-full): in-place at *every* read on a mirrored device. Previously exposed; now copied. 2. **MIRROR_WANTED** — a make queued or deferred: the first bind under `RX_VK_MIRROR_DEFER=1`, a parallel replay's segments (the make is skipped), a pool already written this submission, a failed make retried at submission start. In-place until made. Previously exposed; now copied. 3. **Global-off degenerate** (integrated GPUs, `RX_VK_MIRROR=0`, GCN's default): every pool binds guest memory ⇒ every uniform buffer copied — **byte-identical to the keying #1795 proposed** (live-measured below). **Unchanged on unset**: mirror binds (submit-synced, in place), binds outside a recording, the async-import stream copies, unaligned/outside-pool binds (already copied), and a mirrored device with no refusals (live-measured: zero copies, as on main). ## Switch semantics (a plain note, since one documented behavior is superseded) On main, unset **and** `RX_VK_UB_SNAPSHOT=0` both mean fully-off (in-place everywhere). After this, unset **and** `=0` both mean the per-binding rule, and `=1` still copies every uniform buffer without asking the pools. The in-place opt-out for a mirror-less device is `RX_VK_MIRROR=1`, whose submit-time sync makes it safe. #1795's "`=0` restores the in-place binding" semantic is superseded by this. Storage buffers, vertex and index binds are out of scope, as in #1795. ## Verification - Game-free tests: `tests/test_ub_snapshot_per_pool.py` compiles the **production selection hunk** (cut from `vk_draw.cpp`) with the bind stubbed — 7 scenarios: mirror bind in place; mirror-less bind copied; outside-pools copied; misaligned copied; `=1` copies everything without asking the pools; `=0` is the per-binding default. Plus source needles (the `live` tag, the three tagged direct returns count-checked, the `!b.live` condition). 7/7 pass. - Full suite vs pristine at base `6f7ab2a7`: **440 passed / 1 failed / 33 skipped** on this branch, pristine **439/1/33** — the 1 is upstream's own `test_env_on` failure (pre-existing on pristine, tracked in the thread); this branch adds exactly its 1 test. Build: 0 failed. - Boot smokes on device, 0 faults in all three modes: unset → **0 copies** (mirrored dGPU, no hazard states reached at the title screen — unchanged as claimed); `RX_VK_UB_SNAPSHOT=1` → 1,275,001 copies, never asking the pools; `RX_VK_MIRROR=0` → **1,280,001 copies** — the global-off degenerate case, byte-identical volume to the keying #1795 proposed. ## Cost Measured at parity on gameplay, not inferred: the `RX_VK_DGPU_TRIALS=0` discriminator in the companion thread measured this snapshot path at **+0.9%** (inside run-to-run noise, adjacent warp-normalized pairs) while it was serving ~14M stream copies per run. Correctness fix, noise cost. ## Provenance & scope - Based on upstream/main `6f7ab2a7`. No policy machinery travels with this: no trials, no skip list, no counters, no `RX_VK_DGPU` gating — the diff reads as a refinement of #1795's own fix. - All measurements on one rig, named per the companion thread's discipline: **RX 9070 XT (Navi 48), ASRock Steel Legend, RADV/Mesa on Linux, PCIe Gen5 x16, ReBAR enabled**. - Extracted from the companion dGPU-policy PR **#2338**, where the hazard analysis, the parity-cost discriminator measurement, and the hardware identification were established; this PR carries only the correctness piece, independent of that PR's fate.

github-sync referenced this pull request

2026-11-12 14:42:25 +00:00

perf(vk): a PC-like dGPU policy (RX_VK_DGPU=1) - direct guest-memory binds, mirrors only where a measured trial proves them #2338

github-sync commented

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

Owner

@Joey-Magnus reviewed on GitHub · 2026-11-12 14:33 UTC · original

This no longer merges. #2464 (018a1d2e) has since landed in the same uniform_range hunk of gpu/vk/vk_draw.cpp, and vk_memory.cpp conflicts too, so I couldn't run the tests on a merge. #2464 made the snapshot the default on Intel when the pools have no mirror. It deliberately left AMD APUs, GCN and RX_VK_MIRROR=0 off "until it is measured there". This PR turns it on per binding for every GPU, the Steam Deck included. The only cost figure is from a mirrored RX 9070 XT, where it copies nothing. The devices it actually changes, and where every uniform buffer would be copied, have no frame-time measurement. A rebase onto #2464 needs that measurement on an APU, or the Deck, in Ocarina of Time.
After this, RX_VK_UB_SNAPSHOT=0 no longer turns the snapshot off, which goes against the on/off rule the switches follow since #2651. Please keep =0 as off.
The mixed-state cases (a refused or wanted pool on a mirrored GPU) are a real gap and worth fixing. It also overlaps #1795 and #2338, which are both still open, so the maintainers should pick which way they want this to go.

> [@Joey-Magnus](https://github.com/Joey-Magnus) reviewed on GitHub · 2026-11-12 14:33 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2609#pullrequestreview-5876221265) This no longer merges. #2464 (018a1d2e) has since landed in the same uniform_range hunk of gpu/vk/vk_draw.cpp, and vk_memory.cpp conflicts too, so I couldn't run the tests on a merge. #2464 made the snapshot the default on Intel when the pools have no mirror. It deliberately left AMD APUs, GCN and RX_VK_MIRROR=0 off "until it is measured there". This PR turns it on per binding for every GPU, the Steam Deck included. The only cost figure is from a mirrored RX 9070 XT, where it copies nothing. The devices it actually changes, and where every uniform buffer would be copied, have no frame-time measurement. A rebase onto #2464 needs that measurement on an APU, or the Deck, in Ocarina of Time. After this, RX_VK_UB_SNAPSHOT=0 no longer turns the snapshot off, which goes against the on/off rule the switches follow since #2651. Please keep =0 as off. The mixed-state cases (a refused or wanted pool on a mirrored GPU) are a real gap and worth fixing. It also overlaps #1795 and #2338, which are both still open, so the maintainers should pick which way they want this to go.

github-sync referenced this pull request

2026-11-12 14:48:30 +00:00

perf(vk): sparse VRAM mirrors, memory only where a game pool is (RX_VK_MIRROR_SPARSE=1) #2794

Commenting is not possible because the repository is archived.

No reviewers