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

Draft
dukenheimer wants to merge 3 commits from dukenheimer/Longshot:fix/igpu-ub-snapshot into main
First-time contributor

Why

On integrated GPUs without VRAM mirrors, a uniform buffer bound in place is the game's own memory, which the game rewrites right after the submit while the GPU still runs: HUD and lighting artifacts, and on the Steam Deck freezes. https://github.com/NotRexed/Hookshot/pull/2464 fixed that for Intel; this extends the same copy to the other integrated GPUs. Moved from https://github.com/NotRexed/Hookshot/pull/1795.

What changed

  • vk: gpu/vk/vk_draw.cpp, the uniform-buffer snapshot default: on when the pools have no mirror and the GPU is Intel (https://github.com/NotRexed/Hookshot/pull/2464's rule, kept) or integrated (VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU). Discrete GPUs without mirrors keep main's default (off) until someone measures them.
  • vk: a copied uniform buffer serves later bindings in the recording only while its capture view and the replay's CPU-write generation are unchanged. Failed reads are not cached. A range written by the GPU earlier in the same recording falls back to its in-place buffer, so a stale CPU copy cannot hide that write.
  • vk: reuse defaults on only for the newly covered mirrorless non-Intel integrated-GPU path. Intel keeps the prior copy-per-binding behavior unless RX_VK_UB_SNAPSHOT_REUSE explicitly enables reuse. The cache is reserved at recording start and cleared with the stream it points into.
  • docs/tests: both switches are in docs/development.md; tests/test_ub_snapshot_default.py covers the snapshot and reuse defaults, explicit on/off values, failed-copy/CPU-write/GPU-write cache guards, and per-recording clear/reserve.

Flags and settings

Flag What it does Default now Recommendation Helps on Doesn't help on Untested on
RX_VK_UB_SNAPSHOT copy uniform buffers as submitted on for Intel or integrated GPUs without mirrors keep Steam Deck (stability) discrete GPUs with mirrors (unused there) discrete GPUs without mirrors
RX_VK_UB_SNAPSHOT_REUSE reuse a submitted copy within a recording on for the newly covered mirrorless non-Intel integrated-GPU path; off for Intel keep for the new path; Intel unforced Steam Deck Intel keeps copy-per-binding unless forced all other integrated GPUs and Intel reuse

Measurements

main: Hookshot main 6749ba21 (the measurements below are from the Hookshot PR, before the move; to be measured again on Longshot main)
this PR: fix/igpu-ub-snapshot
Hardware: Steam Deck LCD, SteamOS, Game Mode
Scene: Kokiri Forest; 30 s frame logs, 80 screenshots a start; frame generation on, so FPS is presented (~14-15 real)
Method: repeated starts per arm, frame logs and screenshots
How to reproduce: start Kokiri Forest on a Steam Deck with each arm's switches

main @ 6749ba21 (snapshot off) this PR @ 6749ba21 + this
starts 3 ~25
presented FPS 30.1, 30.0, froze 28.7-30.1
p95 54 ms 64-66 ms
freezes 1 (a capture's and a readback's write-back never came) 0
From the Hookshot PR's Deck runs; Longshot numbers to follow. The displayed data shows a material tail-latency risk:
p95 moved from 54 ms to 64-66 ms (about 20%) while presented fps fell by only roughly 1 fps. Repeats on Longshot
must include RX_VK_UB_SNAPSHOT_REUSE=1 and =0 and report both fps/p95 and the log's "reused copies" count before
the reuse default is kept.

Testing

Tested: python3 -I -B tests/test_ub_snapshot_default.py --cxx g++ passes in WSL Ubuntu 24.04 on the review update. It compiles the actual snapshot/reuse selection and checks the cache's clear, reserve, CPU-write and GPU-write guards. The direct-buffer fixture and timed-out fixture arms from the full run pass when rerun serially. python -I -B tests/run_all.py was rerun on Windows with llvm-mingw: 439 passed, 15 failed, 73 skipped. The failures were missing local modules/tooling, unrelated link/test failures, or concurrency timeouts; the timed-out test_direct_buffer_writes.py, test_frame_wait_bounds.py, test_fallback_buffer_cleanup.py and test_pipeline_cache_save.py passed serially. The snapshot test is intentionally checked under WSL because it uses POSIX setenv/unsetenv.

On the old Hookshot branch, ~25 Deck starts in Kokiri Forest completed without the previous freeze.

Not tested: Longshot main on the Deck; other integrated GPUs; Intel reuse (off by default, explicitly left unmeasured); and discrete pools that have mirrors globally but are individually refused/awaiting a mirror and therefore still bind in place. Those mirror-pressure cases are outside this defaulting change.

Needs other testers:

  • Other integrated GPUs (AMD APUs on Windows or Linux): Kokiri Forest without HUD or lighting artifacts and without freezes, expecting the same as on the Deck (request to open)

Screenshots

Nothing new on screen: the change removes artifacts and freezes; with the snapshot off on current main the screenshots are clean too.

Risk and rollback

Integrated GPUs pay about 1 fps for the copied bytes in the old Deck runs, with the p95 cost above still to be explained and retested. A GPU-written uniform range in the current recording falls back to its in-place buffer, avoiding a stale CPU copy. RX_VK_UB_SNAPSHOT=0 turns snapshots off; RX_VK_UB_SNAPSHOT_REUSE=0 makes each binding copy independently. Discrete GPUs whose global mirror setting is on but whose individual pool is MIRROR_REFUSED or MIRROR_WANTED are not covered by this PR's snapshot default and retain the same in-place behavior; that is a follow-up rather than a claim of coverage.

Reviewer notes

Ported as the Hookshot branch's net diff onto Longshot main; the test's include follows the rename to longshot_env.h. WIP until the Deck runs are repeated on Longshot main.

Issues

See https://github.com/NotRexed/Hookshot/pull/2464

Checklist

For everyone

  • One topic.
  • The title is a Conventional Commit (type(scope): description).
  • Rebased on current main.
  • python -I -B tests/run_all.py run and passing (ran; local failures are named above, so the box remains open).
  • Tested in the game, or what wasn't is named under Testing.
  • Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests).
  • Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported.
  • No game files, dumps, build output or generated files.
  • third_party/ untouched (a new folder with a PROVENANCE.md instead).
  • The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

  • AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
  • Flags noted, and the switch reference updated (docs/development.md; the README's switch section points there).
  • A game-free test added for the fix, failing on main and passing here.
## Why On integrated GPUs without VRAM mirrors, a uniform buffer bound in place is the game's own memory, which the game rewrites right after the submit while the GPU still runs: HUD and lighting artifacts, and on the Steam Deck freezes. https://github.com/NotRexed/Hookshot/pull/2464 fixed that for Intel; this extends the same copy to the other integrated GPUs. Moved from https://github.com/NotRexed/Hookshot/pull/1795. ## What changed - vk: `gpu/vk/vk_draw.cpp`, the uniform-buffer snapshot default: on when the pools have no mirror and the GPU is Intel (https://github.com/NotRexed/Hookshot/pull/2464's rule, kept) or integrated (`VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU`). Discrete GPUs without mirrors keep main's default (off) until someone measures them. - vk: a copied uniform buffer serves later bindings in the recording only while its capture view and the replay's CPU-write generation are unchanged. Failed reads are not cached. A range written by the GPU earlier in the same recording falls back to its in-place buffer, so a stale CPU copy cannot hide that write. - vk: reuse defaults on only for the newly covered mirrorless non-Intel integrated-GPU path. Intel keeps the prior copy-per-binding behavior unless `RX_VK_UB_SNAPSHOT_REUSE` explicitly enables reuse. The cache is reserved at recording start and cleared with the stream it points into. - docs/tests: both switches are in `docs/development.md`; `tests/test_ub_snapshot_default.py` covers the snapshot and reuse defaults, explicit on/off values, failed-copy/CPU-write/GPU-write cache guards, and per-recording clear/reserve. ## Flags and settings | Flag | What it does | Default now | Recommendation | Helps on | Doesn't help on | Untested on | |---|---|---|---|---|---|---| | `RX_VK_UB_SNAPSHOT` | copy uniform buffers as submitted | on for Intel or integrated GPUs without mirrors | keep | Steam Deck (stability) | discrete GPUs with mirrors (unused there) | discrete GPUs without mirrors | | `RX_VK_UB_SNAPSHOT_REUSE` | reuse a submitted copy within a recording | on for the newly covered mirrorless non-Intel integrated-GPU path; off for Intel | keep for the new path; Intel unforced | Steam Deck | Intel keeps copy-per-binding unless forced | all other integrated GPUs and Intel reuse | ## Measurements main: Hookshot main `6749ba21` (the measurements below are from the Hookshot PR, before the move; to be measured again on Longshot main) this PR: fix/igpu-ub-snapshot Hardware: Steam Deck LCD, SteamOS, Game Mode Scene: Kokiri Forest; 30 s frame logs, 80 screenshots a start; frame generation on, so FPS is presented (~14-15 real) Method: repeated starts per arm, frame logs and screenshots How to reproduce: start Kokiri Forest on a Steam Deck with each arm's switches | | main @ 6749ba21 (snapshot off) | this PR @ 6749ba21 + this | |---|---|---| | starts | 3 | ~25 | | presented FPS | 30.1, 30.0, froze | 28.7-30.1 | | p95 | 54 ms | 64-66 ms | | freezes | 1 (a capture's and a readback's write-back never came) | 0 | ``` From the Hookshot PR's Deck runs; Longshot numbers to follow. The displayed data shows a material tail-latency risk: p95 moved from 54 ms to 64-66 ms (about 20%) while presented fps fell by only roughly 1 fps. Repeats on Longshot must include RX_VK_UB_SNAPSHOT_REUSE=1 and =0 and report both fps/p95 and the log's "reused copies" count before the reuse default is kept. ``` ## Testing **Tested:** `python3 -I -B tests/test_ub_snapshot_default.py --cxx g++` passes in WSL Ubuntu 24.04 on the review update. It compiles the actual snapshot/reuse selection and checks the cache's clear, reserve, CPU-write and GPU-write guards. The direct-buffer fixture and timed-out fixture arms from the full run pass when rerun serially. `python -I -B tests/run_all.py` was rerun on Windows with llvm-mingw: **439 passed, 15 failed, 73 skipped**. The failures were missing local modules/tooling, unrelated link/test failures, or concurrency timeouts; the timed-out `test_direct_buffer_writes.py`, `test_frame_wait_bounds.py`, `test_fallback_buffer_cleanup.py` and `test_pipeline_cache_save.py` passed serially. The snapshot test is intentionally checked under WSL because it uses POSIX `setenv`/`unsetenv`. On the old Hookshot branch, ~25 Deck starts in Kokiri Forest completed without the previous freeze. **Not tested:** Longshot main on the Deck; other integrated GPUs; Intel reuse (off by default, explicitly left unmeasured); and discrete pools that have mirrors globally but are individually refused/awaiting a mirror and therefore still bind in place. Those mirror-pressure cases are outside this defaulting change. **Needs other testers:** - Other integrated GPUs (AMD APUs on Windows or Linux): Kokiri Forest without HUD or lighting artifacts and without freezes, expecting the same as on the Deck (request to open) ## Screenshots Nothing new on screen: the change removes artifacts and freezes; with the snapshot off on current main the screenshots are clean too. ## Risk and rollback Integrated GPUs pay about 1 fps for the copied bytes in the old Deck runs, with the p95 cost above still to be explained and retested. A GPU-written uniform range in the current recording falls back to its in-place buffer, avoiding a stale CPU copy. `RX_VK_UB_SNAPSHOT=0` turns snapshots off; `RX_VK_UB_SNAPSHOT_REUSE=0` makes each binding copy independently. Discrete GPUs whose global mirror setting is on but whose individual pool is `MIRROR_REFUSED` or `MIRROR_WANTED` are not covered by this PR's snapshot default and retain the same in-place behavior; that is a follow-up rather than a claim of coverage. ## Reviewer notes Ported as the Hookshot branch's net diff onto Longshot main; the test's include follows the rename to `longshot_env.h`. WIP until the Deck runs are repeated on Longshot main. ## Issues See https://github.com/NotRexed/Hookshot/pull/2464 ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main`. - [ ] `python -I -B tests/run_all.py` run and passing (ran; local failures are named above, so the box remains open). - [ ] Tested in the game, or what wasn't is named under Testing. - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). - [ ] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] Flags noted, and the switch reference updated (`docs/development.md`; the README's switch section points there). - [x] A game-free test added for the fix, failing on `main` and passing here.
fix(vk): HUD and lighting artifacts on GPUs without buffer mirrors
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 4s
CI / Conventional Commit title (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Third-party parts / As their notes say (pull_request) Has been cancelled
Third-party parts / The same as upstream (pull_request) Has been cancelled
de88f4cf76
Ported from https://github.com/NotRexed/Hookshot/pull/1795 (the net diff of its
branch, onto Longshot main). Without buffer mirrors (integrated GPUs, Intel), the
replay copies uniform buffers as submitted by default and replays in submit order;
a submitted copy is reused within a recording. RX_VK_UB_SNAPSHOT=0/1 forces either
way, and RX_VK_UB_SNAPSHOT_REUSE reads as env_on reads a switch (unset: on).

Co-authored-by: Copilot App <[email protected]>
HumanBean added this to the v3.0.0 milestone 2026-11-13 16:21:11 +00:00
HumanBean left a comment

Code review only. I didn't build it or run the checks or the game.

What's good. The detection is narrow and easy to follow: !pools_mirrored() && (Intel || VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU), so Intel's default from https://github.com/NotRexed/Hookshot/pull/2464 is kept and GCN and RX_VK_MIRROR=0 discrete cards stay off. The log line saying which way it went will help with triage. The reuse cache lives as long as the copies it points to. It is per recording (RecordingRes, thread-local cur) and is cleared in draw_begin_recording. Each split part (maybe_split -> begin_recording) and each parallel segment starts with an empty cache. The copies stay in the slot's streams, which are kept until the slot's next recording, after recording_wait. Copies made before a stream grows stay valid, as c1_at's do. ub_overlay runs first, so ranges with stream uniform updates (flush_ub_updates ones included) never reach the cache. A read that fails isn't cached. The counters are now atomic, which they need to be with parallel segments. The new test extracts the real selection code and fails on main.

Must change

  1. The Intel path is not unchanged. RX_VK_UB_SNAPSHOT_REUSE is on wherever the snapshot is on, so Intel iGPUs (and Intel discrete with RX_VK_MIRROR=0) now reuse copies within a recording where they copied every binding before. The selection rule is the same, but what the copy does changed, and the Intel path hasn't been measured with it. Either limit the reuse to the newly covered GPUs until Intel is measured, or measure Intel. Then fix the Flags table ("Untested on: other integrated GPUs") and "Intel's rule is unchanged".
  2. False checklist box: "Flags noted, and the switch reference updated (docs/development.md, the README)". The PR changes neither file. docs/development.md says a PR that adds or changes a switch "documents it here". This one changes RX_VK_UB_SNAPSHOT's default and adds RX_VK_UB_SNAPSHOT_REUSE. Document both, or untick the box.
  3. The measurements show a p95 cost that the description doesn't mention. The Deck table has p95 going from 54 ms to 64-66 ms, about 20% worse, on a CPU-bound device. Risk and rollback only says "~1 fps". It's also unclear whether the reuse was in the Hookshot build that was measured. When it is measured on Longshot, please include the reuse on and off (RX_VK_UB_SNAPSHOT_REUSE=0), and the log's reused copies count next to copied, so we can see whether the reuse pays for itself.

Should change / questions

  • Reuse hit rate. The cache key is the capture view, and cap_view_gen is bumped by every memory_stamp_gpu. That includes every draw or dispatch that binds a written storage buffer (vk_draw.cpp:3623), every GPU buffer copy or clear, and every capture published or collection. In compute-heavy scenes it may hardly ever hit. The log counter will tell. If it rarely hits, the map is pure overhead: a node is allocated for every miss on the Deck's hottest path. If it stays, reserve() the map or use a small fixed table like ub_copies in vk_replay.cpp.
  • A stale reuse after in-stream CPU writes. guest_cpu_write (small NOP_CLEAR_BUF, counter reports, the CPU fallback of NOP_COPY_B2B outside the pools) doesn't bump the view. A range copied before such a write in the same recording is served from the old copy. Without the reuse, the later binding read the new bytes, at least for pages no capture holds. This is probably rare for uniform buffers, but it needs a one-line answer, or a check on memory_page_stamp / ub_list_epoch like ub_copy_hit has.
  • GPU-written uniform ranges (now affects every iGPU). The snapshot reads the bytes from the CPU when the draw is recorded, so it can't see a GPU write to that range earlier in the same submission (a buffer copy, a clear, a storage write). The in-place binding did see those. This was already true on Intel, but now covers the Deck and AMD APUs too. Have you checked that OoT never produces a uniform buffer on the GPU within a frame? If it can, a range whose page stamp moved during this recording could fall back to the in-place binding.
  • Discrete GPUs under VRAM pressure don't take this path. The decision is static and global (mirror_on()). On a discrete card with mirrors on, pools that are MIRROR_REFUSED (over 1 GB, or over the budget) or still MIRROR_WANTED are bound in place too (gpu_buffer_bind, vk_memory.cpp ~3121/3148), with the same rewrite race and the snapshot off. This is fine to leave out of this PR, but name it under Testing or Risk so it isn't assumed to be covered.
  • The test doesn't cover the reuse, which is the riskier half: the RX_VK_UB_SNAPSHOT_REUSE parsing, the view key, and the clear in draw_begin_recording. Text checks like tests/test_mem_trim.py's would do.
  • "Rebased on current main" was true when the PR opened. Since then main has cd906265 (#197, which touches draw() in vk_draw.cpp) and three more commits. There's no textual overlap with these hunks. Rebase before the Longshot runs and re-run tests/run_all.py, whose box is still unticked.
  • Bare GitHub numbers. The new and rewritten lines use bare #2419, #2343 and #2464 (vk_draw.cpp comment, the test's docstring and CASES), and so does the description ("#2464's rule"). Use full URLs.
  • Nits: (hookshot-deck) should be (longshot). return on; }(); has stray spaces. The test file has no final newline.
Code review only. I didn't build it or run the checks or the game. **What's good.** The detection is narrow and easy to follow: `!pools_mirrored() && (Intel || VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU)`, so Intel's default from https://github.com/NotRexed/Hookshot/pull/2464 is kept and GCN and `RX_VK_MIRROR=0` discrete cards stay off. The log line saying which way it went will help with triage. The reuse cache lives as long as the copies it points to. It is per recording (`RecordingRes`, thread-local `cur`) and is cleared in `draw_begin_recording`. Each split part (`maybe_split` -> `begin_recording`) and each parallel segment starts with an empty cache. The copies stay in the slot's streams, which are kept until the slot's next recording, after `recording_wait`. Copies made before a stream grows stay valid, as `c1_at`'s do. `ub_overlay` runs first, so ranges with stream uniform updates (`flush_ub_updates` ones included) never reach the cache. A read that fails isn't cached. The counters are now atomic, which they need to be with parallel segments. The new test extracts the real selection code and fails on main. ## Must change 1. **The Intel path is not unchanged.** `RX_VK_UB_SNAPSHOT_REUSE` is on wherever the snapshot is on, so Intel iGPUs (and Intel discrete with `RX_VK_MIRROR=0`) now reuse copies within a recording where they copied every binding before. The selection rule is the same, but what the copy does changed, and the Intel path hasn't been measured with it. Either limit the reuse to the newly covered GPUs until Intel is measured, or measure Intel. Then fix the Flags table ("Untested on: other integrated GPUs") and "Intel's rule is unchanged". 2. **False checklist box: "Flags noted, and the switch reference updated (docs/development.md, the README)".** The PR changes neither file. docs/development.md says a PR that adds or changes a switch "documents it here". This one changes `RX_VK_UB_SNAPSHOT`'s default and adds `RX_VK_UB_SNAPSHOT_REUSE`. Document both, or untick the box. 3. **The measurements show a p95 cost that the description doesn't mention.** The Deck table has p95 going from 54 ms to 64-66 ms, about 20% worse, on a CPU-bound device. Risk and rollback only says "~1 fps". It's also unclear whether the reuse was in the Hookshot build that was measured. When it is measured on Longshot, please include the reuse on and off (`RX_VK_UB_SNAPSHOT_REUSE=0`), and the log's `reused copies` count next to `copied`, so we can see whether the reuse pays for itself. ## Should change / questions - **Reuse hit rate.** The cache key is the capture view, and `cap_view_gen` is bumped by every `memory_stamp_gpu`. That includes every draw or dispatch that binds a written storage buffer (`vk_draw.cpp:3623`), every GPU buffer copy or clear, and every capture published or collection. In compute-heavy scenes it may hardly ever hit. The log counter will tell. If it rarely hits, the map is pure overhead: a node is allocated for every miss on the Deck's hottest path. If it stays, `reserve()` the map or use a small fixed table like `ub_copies` in `vk_replay.cpp`. - **A stale reuse after in-stream CPU writes.** `guest_cpu_write` (small `NOP_CLEAR_BUF`, counter reports, the CPU fallback of `NOP_COPY_B2B` outside the pools) doesn't bump the view. A range copied before such a write in the same recording is served from the old copy. Without the reuse, the later binding read the new bytes, at least for pages no capture holds. This is probably rare for uniform buffers, but it needs a one-line answer, or a check on `memory_page_stamp` / `ub_list_epoch` like `ub_copy_hit` has. - **GPU-written uniform ranges (now affects every iGPU).** The snapshot reads the bytes from the CPU when the draw is recorded, so it can't see a GPU write to that range earlier in the same submission (a buffer copy, a clear, a storage write). The in-place binding did see those. This was already true on Intel, but now covers the Deck and AMD APUs too. Have you checked that OoT never produces a uniform buffer on the GPU within a frame? If it can, a range whose page stamp moved during this recording could fall back to the in-place binding. - **Discrete GPUs under VRAM pressure don't take this path.** The decision is static and global (`mirror_on()`). On a discrete card with mirrors on, pools that are `MIRROR_REFUSED` (over 1 GB, or over the budget) or still `MIRROR_WANTED` are bound in place too (`gpu_buffer_bind`, vk_memory.cpp ~3121/3148), with the same rewrite race and the snapshot off. This is fine to leave out of this PR, but name it under Testing or Risk so it isn't assumed to be covered. - **The test doesn't cover the reuse**, which is the riskier half: the `RX_VK_UB_SNAPSHOT_REUSE` parsing, the view key, and the clear in `draw_begin_recording`. Text checks like `tests/test_mem_trim.py`'s would do. - **"Rebased on current main"** was true when the PR opened. Since then main has `cd906265` (https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/197, which touches `draw()` in vk_draw.cpp) and three more commits. There's no textual overlap with these hunks. Rebase before the Longshot runs and re-run `tests/run_all.py`, whose box is still unticked. - **Bare GitHub numbers.** The new and rewritten lines use bare `#2419`, `#2343` and `#2464` (vk_draw.cpp comment, the test's docstring and `CASES`), and so does the description ("#2464's rule"). Use full URLs. - Nits: `(hookshot-deck)` should be `(longshot)`. `return on; }();` has stray spaces. The test file has no final newline.
@ -406,6 +406,11 @@ struct RecordingRes {
uint64_t before = 0; // (what the recording used of the streams before this one)
uint32_t idle = 0; // (recordings of the slot that stayed in the first stream)
std::unordered_map<const void*, std::pair<VkBuffer, uint64_t>> c1_at; // (c1 of a stage: uploaded once a recording)
// (hookshot-deck) a uniform buffer copied as submitted (RX_VK_UB_SNAPSHOT), by its address and size: the copy serves the
Owner

nit: Longshot's tag is (longshot), not (hookshot-deck).

nit: Longshot's tag is `(longshot)`, not `(hookshot-deck)`.
@ -3050,1 +3057,4 @@
dev.props.deviceType == VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU);
logf("vk: uniform buffers %s (RX_VK_UB_SNAPSHOT=%d)", on ? "copied as submitted" : "bound in place", on ? 1 : 0);
return on; }();
BufferRef b = snapshot ? BufferRef{} : gpu_buffer_bind(u.addr, n, false);
Owner

Question, now that this covers the Deck and AMD APUs: the snapshot reads the bytes from the CPU at record time, so a GPU write to the range earlier in the same submission (buffer copy, clear, storage write) is lost, where the in-place binding saw it. Has anyone checked that OoT never produces a uniform buffer on the GPU within a frame? If it can, a range whose page stamp moved during this recording could fall back to gpu_buffer_bind.

Question, now that this covers the Deck and AMD APUs: the snapshot reads the bytes from the CPU at record time, so a GPU write to the range earlier in the same submission (buffer copy, clear, storage write) is lost, where the in-place binding saw it. Has anyone checked that OoT never produces a uniform buffer on the GPU within a frame? If it can, a range whose page stamp moved during this recording could fall back to `gpu_buffer_bind`.
@ -3046,0 +3046,4 @@
// over the ones submitted (the per-object matrices are in memory it reuses frame after frame). On Intel the depth
// prepass drew the Kokiri ground with other objects' matrices, out of sight: the depth there stayed far, and the fog
// pass painted sky over the ground (the white forest on Intel Core Ultra iGPUs, still there after the split-submission
// fence fix #2419). On a Steam Deck on main 6749ba21, bound in place, 1 of 3 starts froze in a conversation (a capture's
Owner

Bare GitHub numbers in a rewritten comment (#2419 here, #2343 two lines down). Please use full URLs: https://github.com/NotRexed/Hookshot/pull/2419 and so on.

Bare GitHub numbers in a rewritten comment (`#2419` here, `#2343` two lines down). Please use full URLs: https://github.com/NotRexed/Hookshot/pull/2419 and so on.
@ -3050,0 +3054,4 @@
const char* e = getenv("RX_VK_UB_SNAPSHOT");
const bool on = e ? env_on("RX_VK_UB_SNAPSHOT")
: !pools_mirrored() && (dev.props.vendorID == 0x8086 ||
dev.props.deviceType == VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU);
Owner

This only follows the global mirror_on(). On a discrete card with mirrors on, MIRROR_REFUSED pools (over 1 GB, or over the budget) and MIRROR_WANTED ones are also bound in place (vk_memory.cpp gpu_buffer_bind), with the same rewrite race and the snapshot off. Fine as a follow-up, but please name it under Testing or Risk.

This only follows the global `mirror_on()`. On a discrete card with mirrors on, `MIRROR_REFUSED` pools (over 1 GB, or over the budget) and `MIRROR_WANTED` ones are also bound in place (vk_memory.cpp `gpu_buffer_bind`), with the same rewrite race and the snapshot off. Fine as a follow-up, but please name it under Testing or Risk.
@ -3050,0 +3056,4 @@
: !pools_mirrored() && (dev.props.vendorID == 0x8086 ||
dev.props.deviceType == VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU);
logf("vk: uniform buffers %s (RX_VK_UB_SNAPSHOT=%d)", on ? "copied as submitted" : "bound in place", on ? 1 : 0);
return on; }();
Owner

nit: return on; }(); has stray spaces before the closing brace.

nit: `return on; }();` has stray spaces before the closing brace.
@ -3050,2 +3059,4 @@
return on; }();
BufferRef b = snapshot ? BufferRef{} : gpu_buffer_bind(u.addr, n, false);
if (b && b.offset % align == 0) return {b.buffer, b.offset, n};
static const bool snap_reuse = [] { // (unset: on; set, as env_on reads it)
Owner

This reuse is on wherever the snapshot is, Intel included. Intel used to copy every binding and now reuses copies within a recording, so the Intel path from https://github.com/NotRexed/Hookshot/pull/2464 has changed and hasn't been measured. Either limit the reuse to the newly covered GPUs until Intel is measured, or measure it there and update the Flags table and "Intel's rule is unchanged".

This reuse is on wherever the snapshot is, Intel included. Intel used to copy every binding and now reuses copies within a recording, so the Intel path from https://github.com/NotRexed/Hookshot/pull/2464 has changed and hasn't been measured. Either limit the reuse to the newly covered GPUs until Intel is measured, or measure it there and update the Flags table and "Intel's rule is unchanged".
@ -3052,0 +3064,4 @@
return !e || !env_value_off(e);
}();
const uint64_t snap_key = u.addr * 0x9E3779B97F4A7C15ull ^ n;
const uint64_t view = snapshot && snap_reuse ? memory_capture_view() : 0;
Owner

The key is the capture view alone, and cap_view_gen is bumped by every memory_stamp_gpu: every draw or dispatch that binds a written storage buffer (line 3623), every GPU buffer copy or clear, and every capture or collection. How often does this hit on the Deck? Please post the reused copies count from the log next to copied. Also, guest_cpu_write (small NOP_CLEAR_BUF, counter reports, the CPU fallback of NOP_COPY_B2B) doesn't bump the view, so a copy taken before such a write in the same recording is reused afterwards. Before this change the later binding read the new bytes (for pages no capture holds). Probably rare for uniform buffers, but ub_copy_hit also checks ub_list_epoch; would a memory_page_stamp check be enough here?

The key is the capture view alone, and `cap_view_gen` is bumped by every `memory_stamp_gpu`: every draw or dispatch that binds a written storage buffer (line 3623), every GPU buffer copy or clear, and every capture or collection. How often does this hit on the Deck? Please post the `reused copies` count from the log next to `copied`. Also, `guest_cpu_write` (small `NOP_CLEAR_BUF`, counter reports, the CPU fallback of `NOP_COPY_B2B`) doesn't bump the view, so a copy taken before such a write in the same recording is reused afterwards. Before this change the later binding read the new bytes (for pages no capture holds). Probably rare for uniform buffers, but `ub_copy_hit` also checks `ub_list_epoch`; would a `memory_page_stamp` check be enough here?
@ -3059,2 +3080,2 @@
logf("vk: uniform buffer %#llx +%#llx copied into the stream (%s; %lld so)", (unsigned long long)u.addr,
(unsigned long long)n, b ? "unaligned" : "outside the pools", (long long)copied);
// (a copy that could not be read is not kept for the recording's later bindings: each gets its own, as before)
if (memory_read_as_submitted(u.addr, cur->mapped + at, n) && view) cur->ub_snap[snap_key] = {u.addr, n, view, cur->stream, at};
Owner

Each miss allocates a node in the unordered_map, on the hottest path of a CPU-bound device. If the hit rate justifies keeping the reuse, reserve() the map in draw_begin_recording, or use a small fixed table like ub_copies in vk_replay.cpp.

Each miss allocates a node in the unordered_map, on the hottest path of a CPU-bound device. If the hit rate justifies keeping the reuse, `reserve()` the map in `draw_begin_recording`, or use a small fixed table like `ub_copies` in vk_replay.cpp.
@ -0,0 +51,4 @@
'''
# (mirrors, integrated or discrete, vendor, RX_VK_UB_SNAPSHOT, expected)
CASES = [("0", "i", "1002", "unset", "copy"), # (an AMD APU, the Steam Deck: no mirrors)
Owner

This covers the default, but not the reuse, which is the riskier half: the RX_VK_UB_SNAPSHOT_REUSE parsing, the view key, and r.ub_snap.clear() in draw_begin_recording. Text checks like tests/test_mem_trim.py's would cover it. Also, #2464 and #2343 (here and in the docstring) should be full GitHub URLs, and the file has no final newline.

This covers the default, but not the reuse, which is the riskier half: the `RX_VK_UB_SNAPSHOT_REUSE` parsing, the view key, and `r.ub_snap.clear()` in `draw_begin_recording`. Text checks like `tests/test_mem_trim.py`'s would cover it. Also, `#2464` and `#2343` (here and in the docstring) should be full GitHub URLs, and the file has no final newline.
dukenheimer force-pushed fix/igpu-ub-snapshot from de88f4cf76
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 4s
CI / Conventional Commit title (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Third-party parts / As their notes say (pull_request) Has been cancelled
Third-party parts / The same as upstream (pull_request) Has been cancelled
to a2ddd410b9
Some checks failed
Website / Website on Cloudflare (pull_request) Blocked by required conditions
PR description / Description follows the template (pull_request_target) Successful in 18s
CI / Conventional Commit title (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Third-party parts / As their notes say (pull_request) Has been cancelled
Third-party parts / The same as upstream (pull_request) Has been cancelled
2026-11-13 16:45:31 +00:00
Compare
Author
First-time contributor

Addressed the review and pushed a2ddd41 on the rebased branch (main at d5fc892):

  1. RX_VK_UB_SNAPSHOT_REUSE now defaults off on Intel, preserving Intel's copy-per-binding behavior. Explicit values still force it either way; the default is on only for the newly covered mirrorless, non-Intel integrated-GPU path.
  2. Both switches are documented in docs/development.md, and the PR description no longer claims an undocumented switch/reference update.
  3. The description now calls out the 54 ms to 64-66 ms p95 increase and requires Longshot measurements with reuse on and off plus the reused copies log count. I did not run the game for this update.
  4. The per-recording cache is reserved when the recording begins, continues to be cleared with it, and the existing 5,000-copy log reports both copies and reused hits. A failed snapshot read is still not retained.
  5. Replay-side guest_cpu_write/guest_cpu_wrote now advances a dedicated generation. A cache entry includes that generation, so an in-recording CPU write cannot reuse an older copy.
  6. GPU writes are tracked by guest range for the current recording. A later uniform binding that overlaps one bypasses the CPU snapshot and binds the in-place buffer, preserving the earlier GPU write. The 128-range guard falls back conservatively if that small table fills.
  7. The PR description now names the uncovered MIRROR_REFUSED / MIRROR_WANTED discrete-pool cases under Risk; this PR still changes only the global mirrorless integrated-GPU default.
  8. The test now covers reuse defaults and all forced on/off spellings, CPU/GPU cache invalidation invariants, failed-read retention behavior, and per-recording clear/reserve. It passes under WSL Ubuntu 24.04 with g++.
  9. Fixed (longshot), the closing-brace spacing, and the bare upstream issue/PR numbers; the branch is rebased on current main.

Validation: the requested Windows tests/run_all.py run completed with 439 passed, 15 failed, 73 skipped. The failures were missing local dependencies/tooling, unrelated link/test failures, and parallel compile/test timeouts; the timed-out direct-buffer, frame-wait, fallback-cleanup and pipeline-cache fixtures all pass when rerun serially. The UB snapshot fixture passes in WSL and the source checks pass. The PR remains WIP because no Longshot game measurement was possible here.

Addressed the review and pushed `a2ddd41` on the rebased branch (`main` at `d5fc892`): 1. `RX_VK_UB_SNAPSHOT_REUSE` now defaults off on Intel, preserving Intel's copy-per-binding behavior. Explicit values still force it either way; the default is on only for the newly covered mirrorless, non-Intel integrated-GPU path. 2. Both switches are documented in `docs/development.md`, and the PR description no longer claims an undocumented switch/reference update. 3. The description now calls out the 54 ms to 64-66 ms p95 increase and requires Longshot measurements with reuse on and off plus the `reused copies` log count. I did not run the game for this update. 4. The per-recording cache is reserved when the recording begins, continues to be cleared with it, and the existing 5,000-copy log reports both copies and reused hits. A failed snapshot read is still not retained. 5. Replay-side `guest_cpu_write`/`guest_cpu_wrote` now advances a dedicated generation. A cache entry includes that generation, so an in-recording CPU write cannot reuse an older copy. 6. GPU writes are tracked by guest range for the current recording. A later uniform binding that overlaps one bypasses the CPU snapshot and binds the in-place buffer, preserving the earlier GPU write. The 128-range guard falls back conservatively if that small table fills. 7. The PR description now names the uncovered `MIRROR_REFUSED` / `MIRROR_WANTED` discrete-pool cases under Risk; this PR still changes only the global mirrorless integrated-GPU default. 8. The test now covers reuse defaults and all forced on/off spellings, CPU/GPU cache invalidation invariants, failed-read retention behavior, and per-recording clear/reserve. It passes under WSL Ubuntu 24.04 with g++. 9. Fixed `(longshot)`, the closing-brace spacing, and the bare upstream issue/PR numbers; the branch is rebased on current `main`. Validation: the requested Windows `tests/run_all.py` run completed with 439 passed, 15 failed, 73 skipped. The failures were missing local dependencies/tooling, unrelated link/test failures, and parallel compile/test timeouts; the timed-out direct-buffer, frame-wait, fallback-cleanup and pipeline-cache fixtures all pass when rerun serially. The UB snapshot fixture passes in WSL and the source checks pass. The PR remains WIP because no Longshot game measurement was possible here.
fix(vk): cover parallel uniform writes
Some checks are pending
CI / Conventional Commit title (pull_request) Blocked by required conditions
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions
CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions
Third-party parts / As their notes say (pull_request) Blocked by required conditions
Third-party parts / The same as upstream (pull_request) Blocked by required conditions
PR description / Description follows the template (pull_request_target) Successful in 27s
Website / Website on Cloudflare (pull_request_target) Has been skipped
01e82d6cab
Co-authored-by: Copilot App <[email protected]>
Author
First-time contributor

Follow-up 01e82d6: parallel recording writes are now included in the GPU-write fallback guard; the WSL snapshot fixture still passes.

Follow-up 01e82d6: parallel recording writes are now included in the GPU-write fallback guard; the WSL snapshot fixture still passes.
Some checks are pending
CI / Conventional Commit title (pull_request) Blocked by required conditions
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions
CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions
Third-party parts / As their notes say (pull_request) Blocked by required conditions
Third-party parts / The same as upstream (pull_request) Blocked by required conditions
PR description / Description follows the template (pull_request_target) Successful in 27s
Website / Website on Cloudflare (pull_request_target) Has been skipped
This pull request is marked as a work in progress.
Some workflows are waiting to be reviewed.
This branch is out-of-date with the base branch
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u 'fix/igpu-ub-snapshot:dukenheimer-fix/igpu-ub-snapshot'
git switch 'dukenheimer-fix/igpu-ub-snapshot'
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
MagicBeanDev/Longshot!222
No description provided.