MagicBeanDev/Longshot
MagicBeanDev/Longshot
Watch
24
30
Fork
You've already forked Longshot
 43
Code Issues 62 Pull requests 127 Projects Releases 8 Packages Wiki Activity Actions 312

WIP: fix(vk): on Intel iGPUs a split submission's uniform updates are written by its last part #189

Draft
Guep wants to merge 4 commits from Guep/Longshot:fix-split-ub-flush into main
pull from: Guep/Longshot:fix-split-ub-flush
merge into: MagicBeanDev:main
Conversation 2 Commits 4 Files changed 4 +149 -2
Guep commented 2026-11-13 12:49:22 +00:00
First-time contributor

Why

On Intel iGPUs, Kokiri Forest shows big white glowing blocks and haze over the trees and houses, and save thumbnails come out white.

What changed

  • vk replay: maybe_split (gpu/vk/vk_replay.cpp) no longer calls flush_ub_updates at a part's end, nor moves ub_first_of_recording, on Intel GPUs whose buffer pools have no VRAM mirror (!pools_mirrored(): the iGPUs). A long submission is cut into parts (RX_VK_CHUNK); each part's end wrote the stream's uniform updates into guest memory halfway through the submission. Now the last part writes all of them, as an unsplit submission does in finish_recording, as the parallel replay does, and as RX_VK_CHUNK=0 does.
  • What reads the per-part write is not a uniform binding: one that overlaps an update gets an overlay copy built from the whole list (ub_overlay, from ub_updates[0..lim)), the same bytes either way. It is everything else that reads guest memory after a part's end: a later part's storage, vertex, index or texture reads of an updated range, and, without mirrors, the game's own CPU reads, since the GPU then writes the memory the game sees partway through its frame. Which of those draws the white blocks is not pinned down yet (no RX_VK_DUMP of a white frame); that is why the default stays where it was seen, Intel without mirrors, rather than every GPU without mirrors.
  • On the console UPDATE_UB lands in memory in stream order, so for such reads the per-part write is arguably closer to the console in timing; but no Longshot path other than the split ever showed a mid-submission write, and the unsplit path, the one every GPU runs most of the time, has always deferred. This change makes the split agree with it; it does not claim the deferral matches the console.
  • The cause was found by ploob-boop on an Arc 140V (NotRexed/Hookshot#999, "white flash every 8 frames"); this is the same three-line change, by default on Intel without mirrors only, with a switch.
  • tests: new tests/test_split_ub_flush.py (source checks on maybe_split, and the default compiled alone against a table: unset, off on Intel without mirrors, on for Intel with mirrors and elsewhere, mirrors or not; set, the switch on any GPU). No game or GPU needed.
  • docs: the switch in docs/development.md, a row in docs/tested-configurations.md.

Flags and settings

Flag What it does Default now Recommendation Helps on Doesn't help on Untested on
RX_VK_SPLIT_UBFLUSH (new) 1: each part of a split submission writes the uniform updates at its end (the old behaviour); 0: only the last part does 0 on Intel without VRAM mirrors (vendor 0x8086 and !pools_mirrored(): the iGPUs), 1 elsewhere, Arc discrete included On by default on Intel iGPUs now; on other GPUs after more testing (the Steam Deck and AMD APUs have no mirrors either) Intel Arc iGPU (Meteor Lake; Lunar Lake per NotRexed/Hookshot#999) - Arc discrete (unchanged unless =0), AMD APUs and the Steam Deck, NVIDIA, Linux

Measurements

main: 9a186f71 (2026-11-13)
this PR: ls/fix-split-ub-flush @ 0a5d21e6
Hardware: Lenovo 83DJ; Intel Arc Graphics (Core Ultra 7 155H iGPU, Meteor Lake), driver 32.0.101.9175; Core Ultra 7 155H; 32 GB; Windows 11 Pro (26300)
Scene: a save in Kokiri Forest, then the control bridge's teleport to Kakariko Village and to Death Mountain Trail, about 130 s in each; output 1920x1080, Render resolution 4, handheld profile
Method: 3 runs per side, interleaved with the other Intel PRs' builds (main, split, pcache, fadd, then again), RX_VK_NO_PREDICT=1 on every side. Per run and area: the median of the fps over the last 5 s windows (the first 4 after the teleport left out: the load) and of the GPU time ... ms a frame lines in longshot_log.txt; an area counts only with 5 windows or more. The table gives the median over the runs, n the runs that count, and the runs that ended in a device loss, with where
How to reproduce: a save outside Link's house, the options above, RX_VK_NO_PREDICT=1; teleport with the control bridge (cheats: teleport) to Kakariko Village, then Death Mountain Trail, about 130 s each; read the fps and GPU time lines of longshot_log.txt per area

main @ 9a186f71 this PR @ 0a5d21e6 Δ
Kokiri Forest: fps, median 5.5 (n=3) 5.4 (n=3) -0.1 fps (-2%)
Kokiri Forest: GPU time a frame, median 160.5 ms (n=3) 168.1 ms (n=3) +7.6 ms (+5%)
Kakariko Village: fps, median 7.4 (n=2) 8.1 (n=3) +0.7 fps (+9%)
Kakariko Village: GPU time a frame, median 120.4 ms (n=2) 107.9 ms (n=3) -12.5 ms (-10%)
Death Mountain Trail: fps, median 8.7 (n=1) 9.3 (n=1) +0.6 fps (+7%)
Death Mountain Trail: GPU time a frame, median 102.0 ms (n=1) 93.6 ms (n=1) -8.4 ms (-8%)
Runs measured 3 3
Runs that lost the device 2 of 3 (Kakariko Village x2) 2 of 3 (Kakariko Village x2)

The fps and GPU-time differences are within this series' run-to-run spread (3 runs a side, fewer where the device was lost): no cost, no gain claimed.

(tools/arm_medians.py over tools/route_report.py, which read the runs' longshot_log.txt)
main    runs 3  kokiri: fps  5.5 gpu  160.5 (n=3) | Kakariko: fps  7.4 gpu  120.4 (n=2) | DeathMou: fps  8.7 gpu  102.0 (n=1)  lost 2/3 n_main2:KakarikoVill n_main3:KakarikoVill
split   runs 3  kokiri: fps  5.4 gpu  168.1 (n=3) | Kakariko: fps  8.1 gpu  107.9 (n=3) | DeathMou: fps  9.3 gpu   93.6 (n=1)  lost 2/3 n_split1:KakarikoVill n_split3:KakarikoVill

Testing

Tested: Windows 11, Intel Arc iGPU (Meteor Lake): the route above, 3 runs per side interleaved with main and the two other Intel PRs' builds, a capture every 60 frames. White blocks (bright flat blocks per capture, the title screen left out): main 17 captures in 1 run of 3, up to 10.2 % of the picture; this PR 0 in 3 runs (the other two Intel PRs' builds 0 too: here only one run of main in three shows them, so this series alone is thin evidence; ploob-boop's Arc 140V runs and the game-free test are the rest). Device lost in Kakariko Village in 2 of 3 runs on both sides here.

The device losses of the earlier 300 s series (docs/tested-configurations.md): 2 of 8 runs with RX_VK_SPLIT_UBFLUSH=1 and 5 of 8 without, on one build that also had #190 and the 32-lane pin change. Those are the same compute programs (499dbfd0, a float add to a storage buffer; ab7699ee, likely one; aba5e6af, not classified) that lose the device with the float-add loop on main and on the other Intel builds: 12 of 18 runs in #191's series, 0 of 6 with #191's native add, and 0 in 6 route runs on 13/11 of a build with this change and #191 together. So they are most likely the loop's and not this change's, but 5 of 8 against 2 of 8 is not explained by that alone: it stays open until runs with #191 merged, on and off, say otherwise.

Not tested: AMD and NVIDIA, Arc discrete (all keep the old code unless RX_VK_SPLIT_UBFLUSH=0), the Steam Deck and AMD APUs (no mirrors, the same code path as Intel's iGPUs, but not seen there), Linux/Mesa ANV, Lunar Lake on Longshot's build.

Needs other testers:

  • Intel Arc discrete or Lunar Lake, Windows: Kokiri Forest outside Link's house, expecting no white blocks or haze, the same picture as with RX_VK_CHUNK=0 (request to open)
  • AMD or NVIDIA with RX_VK_SPLIT_UBFLUSH=0: the same spot, expecting the same picture and fps as without it, before the default can widen (request to open)
  • Steam Deck or an AMD APU with RX_VK_SPLIT_UBFLUSH=0 and =1: the same spot, expecting the same picture, before the default covers every GPU without mirrors (request to open)

Screenshots

To add: main and this PR at the same spot, from this series' captures.

Risk and rollback

On Intel iGPUs, a non-uniform read in a later part (storage, vertex, index, texture) of a range an earlier part's UPDATE_UB wrote now sees the value before the update, as an unsplit submission has always given it; none was seen to need the newer one. Uniform bindings are unchanged (the overlay). RX_VK_SPLIT_UBFLUSH=1 restores the old behaviour; reverting the commit is enough.

Reviewer notes

The device losses: see Testing. No textual conflict with #222 (vk_draw.cpp against vk_replay.cpp); 222's snapshot path is reached only for ranges no update overlaps and its reuse map is cleared per recording, so per part: the two do not interact in the code, though both gate on GPUs without mirrors.

Issues

See NotRexed/Hookshot#999

Checklist

For everyone

  • One topic.
  • The title is a Conventional Commit (type(scope): description).
  • Rebased on current main. (on fbd42024; measured on 9a186f71 before the rebase)
  • python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above). (run on 1c0e220a: 449 passed, 9 failed, 72 skipped; main fbd42024 fails 9 on this Windows PC the same way: test_bake_portability, test_bug_report, test_check_third_party, test_fpx_sticky, test_portable_builder_home, test_portable_builder_startup, test_prepare_at_build, test_release, test_spawn_emit (all but test_prepare_at_build fail here too); plus test_pipeline_cache_save, whose compile ran out of its 60 s while a 12-job game build ran beside it, in code this PR does not touch: to run again on an idle PC before ticking)
  • 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). (the test requests are still to open)
  • Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. (fewer than 2 runs that count: main Death Mountain Trail: 1 run; this PR Death Mountain Trail: 1 run)
  • 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).
  • Screenshots for a change that shows on screen. (to add: main and this PR at the same spot)
  • A row in docs/tested-configurations.md for each measured run. (the row there is the earlier 300 s series; the route series above still needs one)
  • A game-free test added for the fix, failing on main and passing here.
  • A check under tests/ that reads changed source updated in this PR, not weakened.
  • Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. (built on Windows only)
  • Docs updated.
## Why On Intel iGPUs, Kokiri Forest shows big white glowing blocks and haze over the trees and houses, and save thumbnails come out white. ## What changed - vk replay: `maybe_split` (`gpu/vk/vk_replay.cpp`) no longer calls `flush_ub_updates` at a part's end, nor moves `ub_first_of_recording`, on Intel GPUs whose buffer pools have no VRAM mirror (`!pools_mirrored()`: the iGPUs). A long submission is cut into parts (`RX_VK_CHUNK`); each part's end wrote the stream's uniform updates into guest memory halfway through the submission. Now the last part writes all of them, as an unsplit submission does in `finish_recording`, as the parallel replay does, and as `RX_VK_CHUNK=0` does. - What reads the per-part write is not a uniform binding: one that overlaps an update gets an overlay copy built from the whole list (`ub_overlay`, from `ub_updates[0..lim)`), the same bytes either way. It is everything else that reads guest memory after a part's end: a later part's storage, vertex, index or texture reads of an updated range, and, without mirrors, the game's own CPU reads, since the GPU then writes the memory the game sees partway through its frame. Which of those draws the white blocks is not pinned down yet (no `RX_VK_DUMP` of a white frame); that is why the default stays where it was seen, Intel without mirrors, rather than every GPU without mirrors. - On the console `UPDATE_UB` lands in memory in stream order, so for such reads the per-part write is arguably closer to the console in timing; but no Longshot path other than the split ever showed a mid-submission write, and the unsplit path, the one every GPU runs most of the time, has always deferred. This change makes the split agree with it; it does not claim the deferral matches the console. - The cause was found by ploob-boop on an Arc 140V ([NotRexed/Hookshot#999](https://github.com/NotRexed/Hookshot/issues/999), "white flash every 8 frames"); this is the same three-line change, by default on Intel without mirrors only, with a switch. - tests: new `tests/test_split_ub_flush.py` (source checks on `maybe_split`, and the default compiled alone against a table: unset, off on Intel without mirrors, on for Intel with mirrors and elsewhere, mirrors or not; set, the switch on any GPU). No game or GPU needed. - docs: the switch in `docs/development.md`, a row in `docs/tested-configurations.md`. ## Flags and settings | Flag | What it does | Default now | Recommendation | Helps on | Doesn't help on | Untested on | |---|---|---|---|---|---|---| | `RX_VK_SPLIT_UBFLUSH` (new) | `1`: each part of a split submission writes the uniform updates at its end (the old behaviour); `0`: only the last part does | `0` on Intel without VRAM mirrors (vendor 0x8086 and `!pools_mirrored()`: the iGPUs), `1` elsewhere, Arc discrete included | On by default on Intel iGPUs now; on other GPUs after more testing (the Steam Deck and AMD APUs have no mirrors either) | Intel Arc iGPU (Meteor Lake; Lunar Lake per [NotRexed/Hookshot#999](https://github.com/NotRexed/Hookshot/issues/999)) | - | Arc discrete (unchanged unless `=0`), AMD APUs and the Steam Deck, NVIDIA, Linux | ## Measurements main: 9a186f71 (2026-11-13) this PR: ls/fix-split-ub-flush @ 0a5d21e6 Hardware: Lenovo 83DJ; Intel Arc Graphics (Core Ultra 7 155H iGPU, Meteor Lake), driver 32.0.101.9175; Core Ultra 7 155H; 32 GB; Windows 11 Pro (26300) Scene: a save in Kokiri Forest, then the control bridge's teleport to Kakariko Village and to Death Mountain Trail, about 130 s in each; output 1920x1080, Render resolution 4, handheld profile Method: 3 runs per side, interleaved with the other Intel PRs' builds (main, split, pcache, fadd, then again), `RX_VK_NO_PREDICT=1` on every side. Per run and area: the median of the `fps over the last 5 s` windows (the first 4 after the teleport left out: the load) and of the `GPU time ... ms a frame` lines in `longshot_log.txt`; an area counts only with 5 windows or more. The table gives the median over the runs, n the runs that count, and the runs that ended in a device loss, with where How to reproduce: a save outside Link's house, the options above, `RX_VK_NO_PREDICT=1`; teleport with the control bridge (cheats: teleport) to Kakariko Village, then Death Mountain Trail, about 130 s each; read the fps and GPU time lines of `longshot_log.txt` per area | | main @ 9a186f71 | this PR @ 0a5d21e6 | Δ | |---|---|---|---| | Kokiri Forest: fps, median | 5.5 (n=3) | 5.4 (n=3) | -0.1 fps (-2%) | | Kokiri Forest: GPU time a frame, median | 160.5 ms (n=3) | 168.1 ms (n=3) | +7.6 ms (+5%) | | Kakariko Village: fps, median | 7.4 (n=2) | 8.1 (n=3) | +0.7 fps (+9%) | | Kakariko Village: GPU time a frame, median | 120.4 ms (n=2) | 107.9 ms (n=3) | -12.5 ms (-10%) | | Death Mountain Trail: fps, median | 8.7 (n=1) | 9.3 (n=1) | +0.6 fps (+7%) | | Death Mountain Trail: GPU time a frame, median | 102.0 ms (n=1) | 93.6 ms (n=1) | -8.4 ms (-8%) | | Runs measured | 3 | 3 | | | Runs that lost the device | 2 of 3 (Kakariko Village x2) | 2 of 3 (Kakariko Village x2) | | The fps and GPU-time differences are within this series' run-to-run spread (3 runs a side, fewer where the device was lost): no cost, no gain claimed. ``` (tools/arm_medians.py over tools/route_report.py, which read the runs' longshot_log.txt) main runs 3 kokiri: fps 5.5 gpu 160.5 (n=3) | Kakariko: fps 7.4 gpu 120.4 (n=2) | DeathMou: fps 8.7 gpu 102.0 (n=1) lost 2/3 n_main2:KakarikoVill n_main3:KakarikoVill split runs 3 kokiri: fps 5.4 gpu 168.1 (n=3) | Kakariko: fps 8.1 gpu 107.9 (n=3) | DeathMou: fps 9.3 gpu 93.6 (n=1) lost 2/3 n_split1:KakarikoVill n_split3:KakarikoVill ``` ## Testing **Tested:** Windows 11, Intel Arc iGPU (Meteor Lake): the route above, 3 runs per side interleaved with main and the two other Intel PRs' builds, a capture every 60 frames. White blocks (bright flat blocks per capture, the title screen left out): main 17 captures in 1 run of 3, up to 10.2 % of the picture; this PR 0 in 3 runs (the other two Intel PRs' builds 0 too: here only one run of main in three shows them, so this series alone is thin evidence; ploob-boop's Arc 140V runs and the game-free test are the rest). Device lost in Kakariko Village in 2 of 3 runs on both sides here. **The device losses of the earlier 300 s series** (`docs/tested-configurations.md`): 2 of 8 runs with `RX_VK_SPLIT_UBFLUSH=1` and 5 of 8 without, on one build that also had #190 and the 32-lane pin change. Those are the same compute programs (499dbfd0, a float add to a storage buffer; ab7699ee, likely one; aba5e6af, not classified) that lose the device with the float-add loop on main and on the other Intel builds: 12 of 18 runs in #191's series, 0 of 6 with #191's native add, and 0 in 6 route runs on 13/11 of a build with this change and #191 together. So they are most likely the loop's and not this change's, but 5 of 8 against 2 of 8 is not explained by that alone: it stays open until runs with #191 merged, on and off, say otherwise. **Not tested:** AMD and NVIDIA, Arc discrete (all keep the old code unless `RX_VK_SPLIT_UBFLUSH=0`), the Steam Deck and AMD APUs (no mirrors, the same code path as Intel's iGPUs, but not seen there), Linux/Mesa ANV, Lunar Lake on Longshot's build. **Needs other testers:** - Intel Arc discrete or Lunar Lake, Windows: Kokiri Forest outside Link's house, expecting no white blocks or haze, the same picture as with `RX_VK_CHUNK=0` (request to open) - AMD or NVIDIA with `RX_VK_SPLIT_UBFLUSH=0`: the same spot, expecting the same picture and fps as without it, before the default can widen (request to open) - Steam Deck or an AMD APU with `RX_VK_SPLIT_UBFLUSH=0` and `=1`: the same spot, expecting the same picture, before the default covers every GPU without mirrors (request to open) ## Screenshots To add: main and this PR at the same spot, from this series' captures. ## Risk and rollback On Intel iGPUs, a non-uniform read in a later part (storage, vertex, index, texture) of a range an earlier part's `UPDATE_UB` wrote now sees the value before the update, as an unsplit submission has always given it; none was seen to need the newer one. Uniform bindings are unchanged (the overlay). `RX_VK_SPLIT_UBFLUSH=1` restores the old behaviour; reverting the commit is enough. ## Reviewer notes The device losses: see Testing. No textual conflict with #222 (`vk_draw.cpp` against `vk_replay.cpp`); 222's snapshot path is reached only for ranges no update overlaps and its reuse map is cleared per recording, so per part: the two do not interact in the code, though both gate on GPUs without mirrors. ## Issues See [NotRexed/Hookshot#999](https://github.com/NotRexed/Hookshot/issues/999) ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main`. (on fbd42024; measured on 9a186f71 before the rebase) - [ ] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). (run on 1c0e220a: 449 passed, 9 failed, 72 skipped; main fbd42024 fails 9 on this Windows PC the same way: test_bake_portability, test_bug_report, test_check_third_party, test_fpx_sticky, test_portable_builder_home, test_portable_builder_startup, test_prepare_at_build, test_release, test_spawn_emit (all but test_prepare_at_build fail here too); plus test_pipeline_cache_save, whose compile ran out of its 60 s while a 12-job game build ran beside it, in code this PR does not touch: to run again on an idle PC before ticking) - [x] 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). (the test requests are still to open) - [ ] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. (fewer than 2 runs that count: main Death Mountain Trail: 1 run; this PR Death Mountain Trail: 1 run) - [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). - [ ] Screenshots for a change that shows on screen. (to add: main and this PR at the same spot) - [ ] A row in `docs/tested-configurations.md` for each measured run. (the row there is the earlier 300 s series; the route series above still needs one) - [x] A game-free test added for the fix, failing on `main` and passing here. - [x] A check under `tests/` that reads changed source updated in this PR, not weakened. - [ ] Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. (built on Windows only) - [x] Docs updated. 
Assisted-by: Claude Code
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Assisted-by: Claude Code
Co-Authored-By: Claude Opus 5.5 <[email protected]>
test(vk): a split submission's uniform updates, a source check and the decision against a table
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 3s
Details
Website / Website on Cloudflare (pull_request_target) Has been skipped
Details
CI / Conventional Commit title (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
Details
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Details
Third-party parts / As their notes say (pull_request) Has been cancelled
Details
Third-party parts / The same as upstream (pull_request) Has been cancelled
Details
0a5d21e661 
Assisted-by: Claude Code
Co-Authored-By: Claude Opus 5.5 <[email protected]>
HumanBean requested changes 2026-11-13 16:18:56 +00:00
HumanBean left a comment

The change itself looks right to me. With it, a split submission writes its uniform updates to guest memory the same way an unsplit one does (finish_recording), the same way the parallel replay does ("the uniform updates are written to guest memory at the submission's end"), and the same way RX_VK_CHUNK=0 does. I traced the deferral end to end and found no stale or out-of-order reads:

  • No uniform-buffer binding depends on the per-part write. ub_overlay and ub_read build every overlapping range from ub_updates[0..lim). They don't start at ub_first_of_recording (except with RX_VK_UB_CUR_ONLY=1, where keeping the index unmoved is the correct choice). So a draw in a later part that binds an updated range still gets its overlay copy, with the same bytes as before.
  • An earlier part can't see a later value. Nothing is written until the last part's end.
  • The only readers that change are non-uniform reads in a later part (storage, vertex, index, indirect, sampled) of a range an earlier part's UPDATE_UB wrote. They now see the pre-update value, which is what an unsplit submission has always given them.
  • Stops, bursts and failed submits stay consistent. A submission that stops at a wait after splitting still writes everything: finish_recording flushes from the unmoved ub_first_of_recording. A burst keeps one window. ub_prune can't drop the open submission's entries, because no part signals the timeline.
  • A failed non-final part no longer loses its updates. It's now the last part's submit that carries them, as in an unsplit submission.
  • Device loss is unaffected. device_lost() is [[noreturn]], so nothing after it runs either way.

The existing checks that cut maybe_split (test_parallel_small_cb.py) only read up to end_pass(cmd);, so the reshape doesn't break them. Main has moved on (it's 16 commits ahead, including #197 in vk_replay.cpp), but none of those commits touches maybe_split.

There's no textual conflict with #222: it touches vk_draw.cpp and this PR touches vk_replay.cpp. 222's snapshot path is only reached for ranges no update overlaps (the overlay path runs first), and its reuse map is cleared per recording, which means per part. So the two don't interact in the code. They do overlap in what they gate, though (below).

Must change

  • GitHub references aren't full URLs. NotRexed/Hookshot#999 appears in the docs/development.md row, in the docs/tested-configurations.md row (along with NotRexed/Hookshot#2564) and in the maybe_split comment. On Forgejo, owner/repo#N links to a repo on git.magicbean.dev, not to GitHub. Please use https://github.com/NotRexed/Hookshot/issues/999 (and the full URL for 2564).
  • The device losses need explaining in the description. The tested-configurations row says the device was lost in 2 of 8 old runs and 5 of 8 new runs, but the PR body only mentions the route series (2 of 3 each side). Either show that 5/8 is noise (more interleaved runs, or plain main's rate on the same build), or say plainly that it is open. Don't leave it only in the docs row.
  • The checklist boxes already unticked must become true before pr ready:
    • rebase onto current main;
    • run_all.py;
    • test requests;
    • screenshots;
    • a tested-configurations row for the route series;
    • a Linux build;
    • at least 2 counted runs for Death Mountain Trail, or drop that row.

Should change

  • Gate on what the bug depends on, not on the vendor. The flag table says the change "should help on any GPU whose buffer pools have no VRAM mirror". RX_VK_UB_SNAPSHOT (and 222's widened version of it) keys on !pools_mirrored(). This PR keys on vendorID == 0x8086, which has two effects:

    • It changes Intel Arc discrete cards, which nobody has tested. They have mirrors on: mirror_on() is on for discrete cards. There, gpu_write goes to the mirror, and the write-back happens at memory_end_submission of a different part than before.
    • It leaves out the Steam Deck and AMD APUs, which have no mirrors and are exactly where 222 sees in-place uniform trouble.

    Suggestion: !pools_mirrored() && vendor == Intel for now, or !pools_mirrored() once a Deck run is in. Either way, extend the test's table to cover both cases.

  • Say which reader actually changed. The comment says later parts' draws "read those ranges from guest memory". But a uniform-buffer binding that overlaps an update never binds guest memory: it gets an overlay copy built from the whole list. So the reader that saw the mid-submission write is not a uniform binding. It's a storage, vertex or texture read, or the game itself on a GPU without mirrors, whose memory the GPU overwrites partway through the frame. Naming it (for example from a capture or RX_VK_DUMP of a white-block frame) matters for two reasons:

    • It decides the gate.
    • The per-part write is arguably closer to the console for such reads. On the console, UPDATE_UB lands in memory in stream order, so the deferral may be hiding the real hazard rather than fixing it.

  • Take the opposite default to NVIDIA and AMD as a follow-up. Since the unsplit and parallel paths already defer, there's an argument for making 0 the default everywhere. The PR's "Needs other testers" item for AMD and NVIDIA is the right way to get there. Please open that test request.

The change itself looks right to me. With it, a split submission writes its uniform updates to guest memory the same way an unsplit one does (`finish_recording`), the same way the parallel replay does ("the uniform updates are written to guest memory at the submission's end"), and the same way `RX_VK_CHUNK=0` does. I traced the deferral end to end and found no stale or out-of-order reads: - **No uniform-buffer binding depends on the per-part write.** `ub_overlay` and `ub_read` build every overlapping range from `ub_updates[0..lim)`. They don't start at `ub_first_of_recording` (except with `RX_VK_UB_CUR_ONLY=1`, where keeping the index unmoved is the correct choice). So a draw in a later part that binds an updated range still gets its overlay copy, with the same bytes as before. - **An earlier part can't see a later value.** Nothing is written until the last part's end. - **The only readers that change** are non-uniform reads in a later part (storage, vertex, index, indirect, sampled) of a range an earlier part's `UPDATE_UB` wrote. They now see the pre-update value, which is what an unsplit submission has always given them. - **Stops, bursts and failed submits stay consistent.** A submission that stops at a wait after splitting still writes everything: `finish_recording` flushes from the unmoved `ub_first_of_recording`. A burst keeps one window. `ub_prune` can't drop the open submission's entries, because no part signals the timeline. - **A failed non-final part no longer loses its updates.** It's now the last part's submit that carries them, as in an unsplit submission. - **Device loss is unaffected.** `device_lost()` is `[[noreturn]]`, so nothing after it runs either way. The existing checks that cut `maybe_split` (`test_parallel_small_cb.py`) only read up to `end_pass(cmd);`, so the reshape doesn't break them. Main has moved on (it's 16 commits ahead, including #197 in `vk_replay.cpp`), but none of those commits touches `maybe_split`. There's no textual conflict with https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/222: it touches `vk_draw.cpp` and this PR touches `vk_replay.cpp`. 222's snapshot path is only reached for ranges no update overlaps (the overlay path runs first), and its reuse map is cleared per recording, which means per part. So the two don't interact in the code. They do overlap in what they gate, though (below). ## Must change - **GitHub references aren't full URLs.** `NotRexed/Hookshot#999` appears in the `docs/development.md` row, in the `docs/tested-configurations.md` row (along with `NotRexed/Hookshot#2564`) and in the `maybe_split` comment. On Forgejo, `owner/repo#N` links to a repo on git.magicbean.dev, not to GitHub. Please use https://github.com/NotRexed/Hookshot/issues/999 (and the full URL for 2564). - **The device losses need explaining in the description.** The `tested-configurations` row says the device was lost in 2 of 8 old runs and 5 of 8 new runs, but the PR body only mentions the route series (2 of 3 each side). Either show that 5/8 is noise (more interleaved runs, or plain main's rate on the same build), or say plainly that it is open. Don't leave it only in the docs row. - **The checklist boxes already unticked must become true before `pr ready`:** - rebase onto current main; - `run_all.py`; - test requests; - screenshots; - a `tested-configurations` row for the route series; - a Linux build; - at least 2 counted runs for Death Mountain Trail, or drop that row. ## Should change - **Gate on what the bug depends on, not on the vendor.** The flag table says the change "should help on any GPU whose buffer pools have no VRAM mirror". `RX_VK_UB_SNAPSHOT` (and 222's widened version of it) keys on `!pools_mirrored()`. This PR keys on `vendorID == 0x8086`, which has two effects: - **It changes Intel Arc discrete cards, which nobody has tested.** They have mirrors on: `mirror_on()` is on for discrete cards. There, `gpu_write` goes to the mirror, and the write-back happens at `memory_end_submission` of a different part than before. - **It leaves out the Steam Deck and AMD APUs**, which have no mirrors and are exactly where 222 sees in-place uniform trouble. Suggestion: `!pools_mirrored() && vendor == Intel` for now, or `!pools_mirrored()` once a Deck run is in. Either way, extend the test's table to cover both cases. - **Say which reader actually changed.** The comment says later parts' draws "read those ranges from guest memory". But a uniform-buffer binding that overlaps an update never binds guest memory: it gets an overlay copy built from the whole list. So the reader that saw the mid-submission write is not a uniform binding. It's a storage, vertex or texture read, or the game itself on a GPU without mirrors, whose memory the GPU overwrites partway through the frame. Naming it (for example from a capture or `RX_VK_DUMP` of a white-block frame) matters for two reasons: - **It decides the gate.** - **The per-part write is arguably closer to the console** for such reads. On the console, `UPDATE_UB` lands in memory in stream order, so the deferral may be hiding the real hazard rather than fixing it. - **Take the opposite default to NVIDIA and AMD as a follow-up.** Since the unsplit and parallel paths already defer, there's an argument for making `0` the default everywhere. The PR's "Needs other testers" item for AMD and NVIDIA is the right way to get there. Please open that test request.
docs/development.md Outdated
@ -209,6 +209,7 @@ Pipeline-cache file ownership, temporary storage and failure behavior are descri
| `RX_VK_FETCH_BOUNDS=0` or `1`, `RX_VK_NAN_BITS=0` or `1`, `RX_VK_GATHER_OFFSET=0` or `1` | The translation's vendor and driver workarounds (`gpu/vk/vk_vendor.h`) off or on for one run, whatever the GPU; the log's `vk: vendor fixes` line says which are on. By default: out-of-bounds texel fetches answered with 0 on every GPU but NVIDIA's; NaN tests on a value's bits on AMD's older Windows shader compiler (RX 5000 and RX 6000); gathers nudged by 1/512 of a texel on AMD's and Intel's drivers. They change the shaders' translation (one preparation of the shaders) |
| `RX_VK_NATIVE_FADD=0` or `1` | The shaders' float adds to storage buffers: 1 the GPU's own atomic where it has `VK_EXT_shader_atomic_float`'s `shaderBufferFloat32AtomicAdd`, 0 a compare-exchange loop on every GPU, the extension not enabled. By default the GPU's own on NVIDIA's driver only, the one it was tried on in the game; on AMD and Intel the loop until runs there show the native add works (the log's `vk: float atomic add on storage buffers:` line says which, and why). The loop gives up after 1024 attempts, and an add that gives up is lost |
| `RX_VK_UB_WORD_HASH=0` | A/B test: use the old low-address-bit index for cached uniform words (default: mix upper address bits to reduce aligned-buffer collisions) |
| `RX_VK_SPLIT_UBFLUSH=0` or `1` | A submission cut into parts (`RX_VK_CHUNK`): 1 each part writes the uniform updates recorded so far into game memory at its end, as before; 0 only the last part writes them all, as an unsplit submission does. Default 0 on Intel (white blocks and haze in Kokiri Forest otherwise, NotRexed/Hookshot#999), 1 elsewhere, where it was not measured |
HumanBean commented 2026-11-13 16:18:56 +00:00
Owner

Full URL for the issue, please (https://github.com/NotRexed/Hookshot/issues/999). Also, "Default 0 on Intel" covers Arc discrete, which has mirrors and wasn't measured.

Full URL for the issue, please (https://github.com/NotRexed/Hookshot/issues/999). Also, "Default 0 on Intel" covers Arc discrete, which has mirrors and wasn't measured.
docs/tested-configurations.md Outdated
@ -22,6 +22,7 @@ otherwise.
| 2026-11-12 | main 4dd972a8 + this change, `HOS_VIDEO_INPUT_LIMIT=0` (as main) against the default (16) | Linux (CachyOS, kernel 7.3.3), a private headless sway, scripted input, no sound device (`HOS_AUDIO=0`) | RTX 3090 24 GB, 620.70.06 | Ryzen 9 7900X, 30 GB (the PC was swapping in every run) | 2048x1152, default options, cap 60 | A new game from an empty data folder: the first movie (A after 11 s, about 820 pictures of it), then the second (9,254 pictures) to its end | Three runs each, interleaved. Pictures skipped, first movie: 244, 247, 251 without the limit; 8, 198, 1 with it. Second movie: 123, 700, 98 without; 0, 502, 0 with. In the second run of each side the PC swapped hard and frames were late (177 and 146 frames over 33 ms; 0 to 2 in the others): those skips are the late frames'. Waits of over 90 ms between two pictures in a movie's first 5 s: 44, 43, 43 and 20, 21, 18 without; 1, 0, 0 and 0, 9, 0 with. The second movie ends within 12 ms of its clock on both sides |
| 2026-11-13 | Hookshot fix/commit-limit 905cb5e1 (NotRexed/Hookshot#2972, ported here as fix/commit-limit), against Hookshot main e0e60920, alternated | Windows | RTX 4060 Ti, - | -, 31 GB (115 GB commit limit) | 3440x1440, muted, the commit rule idle (default `RX_VK_COMMIT_RESERVE_MB`) | Route run: Kokiri, Hyrule Field, Kakariko, 2 runs each | This change: 53 and 57 fps median, p99 40.5 and 36.1 ms, private commit at end 28.9 and 28.6 GB, no `commit limit near` line (the rule never fired). Main: 51 and 50 fps, p99 43.2 and 42.1 ms, 33.4 and 34.8 GB. Run-to-run variation, not the change: it costs nothing when idle |
| 2026-11-13 | as above, this change only | as above | as above | as above | as above, `RX_VK_COMMIT_RESERVE_MB=38397` (the rule forced, about 25 GB of the game's commit before it kicks in) | as above, 1 run | 54 fps median, p99 38.7 ms, max 881 ms (the run before, rule not firing: 58, 35.6, 881). 52 `commit limit near` lines, texture budget down by the shortfall each time (e.g. 3661 to 2812 MB), headroom 37.4 to 38.4 GB; private commit peak 26.7 GB (30.3 without the rule), VRAM blocks peak 9.0 GB (12.9). The 5 s wait at the limit itself not reached |
| 2026-11-13 | v2.1.0-rc.3-313-g9b6072c + this PR and two other Intel changes for NotRexed/Hookshot#999 (the pipeline cache fetch lock or the split-submission fix, and compute stages let go of the 32-lane pin as NotRexed/Hookshot#2564 does), the same patch rebased on main e0e6092 for the PR, built from source (llvm-mingw) | Windows 11 Pro (26300), 1920x1080, scripted input | Intel Arc Graphics (Core Ultra 7 155H iGPU, Meteor Lake), 32.0.101.9175 | Core Ultra 7 155H, 32 GB | Render resolution 4, handheld profile; `RX_VK_PASS_TIMING=2`, a screenshot every 60 frames; old/new alternated, the game closed normally; old: `RX_VK_SPLIT_UBFLUSH=1` | Kokiri Forest outside Link's house, standing, 300 s | White blocks over the forest in 9, 7 and 11 frames of the old runs that ran on, 0 in every new run (an earlier series of this change alone: 7, 1, 1 against 0, 0, 0); fps 4.7-5.7 old, 5.7-6.0 new. Device lost in compute programs (ab7699ee, aba5e6af, 499dbfd0) in 2 of 8 old and 5 of 8 new runs over both series; plain main loses it in the same programs |
HumanBean commented 2026-11-13 16:18:56 +00:00
Owner

Two things here:

  • NotRexed/Hookshot#999 and #2564 should be full URLs.
  • "Device lost ... in 2 of 8 old and 5 of 8 new runs" isn't mentioned in the PR description, which only gives the route series' 2/3 each. Please either show it's noise (plain main's rate on the same build, or more runs) or note it as open in the description.
Two things here: - `NotRexed/Hookshot#999` and `#2564` should be full URLs. - "Device lost ... in 2 of 8 old and 5 of 8 new runs" isn't mentioned in the PR description, which only gives the route series' 2/3 each. Please either show it's noise (plain main's rate on the same build, or more runs) or note it as open in the description.
gpu/vk/vk_replay.cpp Outdated
@ -1396,1 +1396,3 @@
flush_ub_updates(cmd);
// (a part's end does not write the stream's uniform updates recorded so far into guest memory, nor start a new window
// of them: the submission's last part writes them all, as an unsplit submission does (finish_recording). Written at
// each part's end, the draws of the later parts that read those ranges from guest memory saw a state an unsplit
HumanBean commented 2026-11-13 16:18:56 +00:00
Owner

"the draws of the later parts that read those ranges from guest memory": a uniform binding that overlaps an update never reads guest memory. ub_overlay builds a copy from ub_updates[0..lim), not from ub_first_of_recording, so its bytes are the same with or without the per-part flush. The reader that changed must be a non-uniform one (storage, vertex, texture), or the game's own memory on a GPU without mirrors. Could you name it here? It decides the gate, and whether deferring matches the console (where UPDATE_UB lands in stream order) or only matches the unsplit path.

"the draws of the later parts that read those ranges from guest memory": a uniform binding that overlaps an update never reads guest memory. `ub_overlay` builds a copy from `ub_updates[0..lim)`, not from `ub_first_of_recording`, so its bytes are the same with or without the per-part flush. The reader that changed must be a non-uniform one (storage, vertex, texture), or the game's own memory on a GPU without mirrors. Could you name it here? It decides the gate, and whether deferring matches the console (where `UPDATE_UB` lands in stream order) or only matches the unsplit path.
gpu/vk/vk_replay.cpp Outdated
@ -1397,0 +1397,4 @@
// of them: the submission's last part writes them all, as an unsplit submission does (finish_recording). Written at
// each part's end, the draws of the later parts that read those ranges from guest memory saw a state an unsplit
// submission never shows: on Intel, white blocks and haze at Kokiri, and a whole-screen white flash every 8 frames on
// the Arc 140V (NotRexed/Hookshot#999). Measured on Intel only, so the default there; RX_VK_SPLIT_UBFLUSH=1: written at each part's
HumanBean commented 2026-11-13 16:18:56 +00:00
Owner

NotRexed/Hookshot#999: please use the full URL, https://github.com/NotRexed/Hookshot/issues/999. On Forgejo, owner/repo#N resolves to git.magicbean.dev.

`NotRexed/Hookshot#999`: please use the full URL, https://github.com/NotRexed/Hookshot/issues/999. On Forgejo, `owner/repo#N` resolves to git.magicbean.dev.
gpu/vk/vk_replay.cpp Outdated
@ -1397,0 +1401,4 @@
// end, as before, =0: at the last part's on any GPU)
static const bool part_flush = [] {
const char* e = getenv("RX_VK_SPLIT_UBFLUSH");
return e ? env_on("RX_VK_SPLIT_UBFLUSH") : dev.props.vendorID != 0x8086;
HumanBean commented 2026-11-13 16:18:56 +00:00
Owner

This keys on the vendor. The bug as described (later parts reading guest memory the per-part flush wrote) depends on the pools having no VRAM mirror, as RX_VK_UB_SNAPSHOT does (!pools_mirrored()), and 222 widens that to integrated GPUs.

As written:

  • Intel Arc discrete (mirrors on, untested) changes too.
  • The Steam Deck and AMD APUs (no mirrors) don't.

Suggest dev.props.vendorID == 0x8086 && !pools_mirrored() (the flush on the default branch inverted), and add mirror cases to the test's table.

This keys on the vendor. The bug as described (later parts reading guest memory the per-part flush wrote) depends on the pools having no VRAM mirror, as `RX_VK_UB_SNAPSHOT` does (`!pools_mirrored()`), and 222 widens that to integrated GPUs. As written: - Intel Arc discrete (mirrors on, untested) changes too. - The Steam Deck and AMD APUs (no mirrors) don't. Suggest `dev.props.vendorID == 0x8086 && !pools_mirrored()` (the flush on the default branch inverted), and add mirror cases to the test's table.
HumanBean added this to the v3.0.0 milestone 2026-11-13 16:28:42 +00:00
Guep force-pushed fix-split-ub-flush from 0a5d21e661
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 3s
Details
Website / Website on Cloudflare (pull_request_target) Has been skipped
Details
CI / Conventional Commit title (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
Details
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Details
Third-party parts / As their notes say (pull_request) Has been cancelled
Details
Third-party parts / The same as upstream (pull_request) Has been cancelled
Details
to 1c0e220aa5
Some checks are pending
CI / Conventional Commit title (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions
Details
CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions
Details
Third-party parts / As their notes say (pull_request) Blocked by required conditions
Details
Third-party parts / The same as upstream (pull_request) Blocked by required conditions
Details
Website / Website on Cloudflare (pull_request) Blocked by required conditions
Details
PR description / Description follows the template (pull_request_target) Successful in 7s
Details
2026-11-13 18:40:49 +00:00 Compare
Guep changed title from WIP: fix(vk): on Intel a split submission's uniform updates are written by its last part to WIP: fix(vk): on Intel iGPUs a split submission's uniform updates are written by its last part 2026-11-13 18:41:25 +00:00
Guep commented 2026-11-13 18:41:31 +00:00
Author
First-time contributor

Thank you for tracing it end to end. Rebased on fbd42024; the changes are in 1c0e220a.

Must change

  • GitHub references are full URLs in the comment, docs/development.md and docs/tested-configurations.md.
  • The device losses are in the description now (Testing). The 5 of 8 against 2 of 8 of the 300 s series are in the same compute programs that lose the device with the float-add loop on main and the other Intel builds (12 of 18 in #191's series; 0 of 6 with #191's native add), and there were none in 6 route runs on 13/11 of a build with this change and #191 together. So most likely the loop's; but I say plainly that the 5 of 8 is not explained by that alone and stays open until runs with #191 merged, this switch on and off.
  • The unticked boxes: rebased, run_all.py below; the test requests, screenshots, the route row, a Linux build and a second Death Mountain Trail run are still to do before pr ready.

Should change

  • Gate: done as you suggested, dev.props.vendorID == 0x8086 && !pools_mirrored() (the per-part flush on everything else, Arc discrete included). The test's table covers mirrors and none on each vendor, 54 cases.
  • The reader: you are right, a uniform binding never reads guest memory here. The comment and the description now name the readers that change (a later part's storage, vertex, index or texture reads of an updated range, and without mirrors the game's own reads) and say that which one draws the white blocks is not pinned down yet: no RX_VK_DUMP of a white frame so far. That is why the default stays on Intel without mirrors, where it was seen, and the description no longer claims the deferral matches the console, only that it makes the split agree with the unsplit path.
  • AMD and NVIDIA, and the Steam Deck / AMD APUs: listed as test requests, to open with the others.

tests/run_all.py on 1c0e220a: the same failures as main on this PC, plus test_pipeline_cache_save, whose compile timed out beside a game build (named in the checklist), to run again on an idle PC.

Thank you for tracing it end to end. Rebased on fbd42024; the changes are in 1c0e220a. **Must change** - GitHub references are full URLs in the comment, `docs/development.md` and `docs/tested-configurations.md`. - The device losses are in the description now (Testing). The 5 of 8 against 2 of 8 of the 300 s series are in the same compute programs that lose the device with the float-add loop on main and the other Intel builds (12 of 18 in #191's series; 0 of 6 with #191's native add), and there were none in 6 route runs on 13/11 of a build with this change and #191 together. So most likely the loop's; but I say plainly that the 5 of 8 is not explained by that alone and stays open until runs with #191 merged, this switch on and off. - The unticked boxes: rebased, `run_all.py` below; the test requests, screenshots, the route row, a Linux build and a second Death Mountain Trail run are still to do before `pr ready`. **Should change** - Gate: done as you suggested, `dev.props.vendorID == 0x8086 && !pools_mirrored()` (the per-part flush on everything else, Arc discrete included). The test's table covers mirrors and none on each vendor, 54 cases. - The reader: you are right, a uniform binding never reads guest memory here. The comment and the description now name the readers that change (a later part's storage, vertex, index or texture reads of an updated range, and without mirrors the game's own reads) and say that which one draws the white blocks is not pinned down yet: no `RX_VK_DUMP` of a white frame so far. That is why the default stays on Intel without mirrors, where it was seen, and the description no longer claims the deferral matches the console, only that it makes the split agree with the unsplit path. - AMD and NVIDIA, and the Steam Deck / AMD APUs: listed as test requests, to open with the others. `tests/run_all.py` on 1c0e220a: the same failures as main on this PC, plus `test_pipeline_cache_save`, whose compile timed out beside a game build (named in the checklist), to run again on an idle PC.
HumanBean reviewed 2026-11-13 18:57:15 +00:00
HumanBean left a comment

Re-review at 1c0e220a. Code only: I didn't build it, run the tests or run the game.

Earlier points

Must change:

  • GitHub references as full URLs: addressed. The maybe_split comment, the test's docstring, docs/development.md and docs/tested-configurations.md all use https://github.com/NotRexed/Hookshot/issues/999 and https://github.com/NotRexed/Hookshot/pull/2564 now.
  • Device losses in the description: addressed. Testing gives the 300 s series' 2 of 8 against 5 of 8, ties them to the same compute programs as the float-add loop (#191's series), and says plainly that the gap stays open until there are runs with 191 merged. That's the "say it's open" option I asked for.
  • Unticked boxes before pr ready: partly done.
    • Done: the rebase (onto fbd42024) and the run_all.py run.
    • Still to do, as you list them: the test requests, screenshots, a tested-configurations row for the route series, a Linux build, and a second Death Mountain Trail run (or drop that row).

Should change:

  • Gate on mirrors, not on the vendor: addressed. The gate is vendorID != 0x8086 || pools_mirrored(), which matches RX_VK_UB_SNAPSHOT in vk_draw.cpp. pools_mirrored() is declared in vk_internal.h, which vk_replay.cpp includes. It's fixed for the session (mirror_on caches it), so the static const is safe. The test's table covers mirrors and no mirrors for each vendor. The case count is right (54).
  • Name the reader that changed: addressed. The comment and the description now say a uniform binding gets the same bytes through ub_overlay. They list the readers that do change, say no dump has pinned down which one draws the white blocks, and drop the claim that the deferral matches the console.
  • The opposite default on AMD and NVIDIA as a follow-up: listed under "Needs other testers". The request isn't open yet.

New since the last review

  • maybe_split is unchanged apart from the gate and the comment. On the Meteor Lake iGPU (no mirrors) the new gate gives the same decision as the old one. So the 9a186f71 / 0a5d21e6 numbers still hold for that PC. Please say so under Measurements, because pr status warns about the old base.
  • Main has moved on to d9648863 since fbd42024. #260 and #234 touch vk_replay.cpp, but neither touches maybe_split, flush_ub_updates, finish_recording or ub_first_of_recording. I expect no conflict. Rebase before pr ready so the box stays true.
  • Nits (inline), none blocking:
    • The comment says nothing was seen on Arc discrete, but it wasn't tested there.
    • The bare #190 and #191 in the docs row don't link from a file.
  • The run_all.py note reads oddly. "main fails 9 ... (all but test_prepare_at_build fail here too)", plus test_pipeline_cache_save, adds up to 9 on the PR only if test_prepare_at_build passes here. Please list the PR's 9 failures exactly when you run it again on an idle PC.

Verdict

Comment, not request changes. The code is right and every earlier must-change point is addressed. Before pr ready, these are left:

  • Rebase onto current main.
  • run_all.py on an idle PC, with test_pipeline_cache_save passing.
  • The three test requests opened.
  • Screenshots of main and this PR at the same spot.
  • A tested-configurations row for the route series.
  • A Linux build.
  • A second counted Death Mountain Trail run per side, or that row dropped.
  • A line under Measurements on why the 9a186f71 numbers still hold.
Re-review at 1c0e220a. Code only: I didn't build it, run the tests or run the game. ## Earlier points Must change: - **GitHub references as full URLs:** addressed. The `maybe_split` comment, the test's docstring, `docs/development.md` and `docs/tested-configurations.md` all use https://github.com/NotRexed/Hookshot/issues/999 and https://github.com/NotRexed/Hookshot/pull/2564 now. - **Device losses in the description:** addressed. Testing gives the 300 s series' 2 of 8 against 5 of 8, ties them to the same compute programs as the float-add loop (https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/191's series), and says plainly that the gap stays open until there are runs with 191 merged. That's the "say it's open" option I asked for. - **Unticked boxes before `pr ready`:** partly done. - Done: the rebase (onto fbd42024) and the `run_all.py` run. - Still to do, as you list them: the test requests, screenshots, a `tested-configurations` row for the route series, a Linux build, and a second Death Mountain Trail run (or drop that row). Should change: - **Gate on mirrors, not on the vendor:** addressed. The gate is `vendorID != 0x8086 || pools_mirrored()`, which matches `RX_VK_UB_SNAPSHOT` in `vk_draw.cpp`. `pools_mirrored()` is declared in `vk_internal.h`, which `vk_replay.cpp` includes. It's fixed for the session (`mirror_on` caches it), so the `static const` is safe. The test's table covers mirrors and no mirrors for each vendor. The case count is right (54). - **Name the reader that changed:** addressed. The comment and the description now say a uniform binding gets the same bytes through `ub_overlay`. They list the readers that do change, say no dump has pinned down which one draws the white blocks, and drop the claim that the deferral matches the console. - **The opposite default on AMD and NVIDIA as a follow-up:** listed under "Needs other testers". The request isn't open yet. ## New since the last review - `maybe_split` is unchanged apart from the gate and the comment. On the Meteor Lake iGPU (no mirrors) the new gate gives the same decision as the old one. So the 9a186f71 / 0a5d21e6 numbers still hold for that PC. Please say so under Measurements, because `pr status` warns about the old base. - Main has moved on to d9648863 since fbd42024. https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/260 and https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/234 touch `vk_replay.cpp`, but neither touches `maybe_split`, `flush_ub_updates`, `finish_recording` or `ub_first_of_recording`. I expect no conflict. Rebase before `pr ready` so the box stays true. - Nits (inline), none blocking: - The comment says nothing was seen on Arc discrete, but it wasn't tested there. - The bare #190 and #191 in the docs row don't link from a file. - The `run_all.py` note reads oddly. "main fails 9 ... (all but test_prepare_at_build fail here too)", plus `test_pipeline_cache_save`, adds up to 9 on the PR only if `test_prepare_at_build` passes here. Please list the PR's 9 failures exactly when you run it again on an idle PC. ## Verdict Comment, not request changes. The code is right and every earlier must-change point is addressed. Before `pr ready`, these are left: - Rebase onto current main. - `run_all.py` on an idle PC, with `test_pipeline_cache_save` passing. - The three test requests opened. - Screenshots of main and this PR at the same spot. - A `tested-configurations` row for the route series. - A Linux build. - A second counted Death Mountain Trail run per side, or that row dropped. - A line under Measurements on why the 9a186f71 numbers still hold.
docs/tested-configurations.md
@ -33,6 +33,7 @@ otherwise.
| 2026-11-12 | main 4dd972a8 + this change, `HOS_VIDEO_INPUT_LIMIT=0` (as main) against the default (16) | Linux (CachyOS, kernel 7.3.3), a private headless sway, scripted input, no sound device (`HOS_AUDIO=0`) | RTX 3090 24 GB, 620.70.06 | Ryzen 9 7900X, 30 GB (the PC was swapping in every run) | 2048x1152, default options, cap 60 | A new game from an empty data folder: the first movie (A after 11 s, about 820 pictures of it), then the second (9,254 pictures) to its end | Three runs each, interleaved. Pictures skipped, first movie: 244, 247, 251 without the limit; 8, 198, 1 with it. Second movie: 123, 700, 98 without; 0, 502, 0 with. In the second run of each side the PC swapped hard and frames were late (177 and 146 frames over 33 ms; 0 to 2 in the others): those skips are the late frames'. Waits of over 90 ms between two pictures in a movie's first 5 s: 44, 43, 43 and 20, 21, 18 without; 1, 0, 0 and 0, 9, 0 with. The second movie ends within 12 ms of its clock on both sides |
| 2026-11-13 | Hookshot fix/commit-limit 905cb5e1 (NotRexed/Hookshot#2972, ported here as fix/commit-limit), against Hookshot main e0e60920, alternated | Windows | RTX 4060 Ti, - | -, 31 GB (115 GB commit limit) | 3440x1440, muted, the commit rule idle (default `RX_VK_COMMIT_RESERVE_MB`) | Route run: Kokiri, Hyrule Field, Kakariko, 2 runs each | This change: 53 and 57 fps median, p99 40.5 and 36.1 ms, private commit at end 28.9 and 28.6 GB, no `commit limit near` line (the rule never fired). Main: 51 and 50 fps, p99 43.2 and 42.1 ms, 33.4 and 34.8 GB. Run-to-run variation, not the change: it costs nothing when idle |
| 2026-11-13 | as above, this change only | as above | as above | as above | as above, `RX_VK_COMMIT_RESERVE_MB=38397` (the rule forced, about 25 GB of the game's commit before it kicks in) | as above, 1 run | 54 fps median, p99 38.7 ms, max 881 ms (the run before, rule not firing: 58, 35.6, 881). 52 `commit limit near` lines, texture budget down by the shortfall each time (e.g. 3661 to 2812 MB), headroom 37.4 to 38.4 GB; private commit peak 26.7 GB (30.3 without the rule), VRAM blocks peak 9.0 GB (12.9). The 5 s wait at the limit itself not reached |
| 2026-11-13 | v2.1.0-rc.3-313-g9b6072c + this PR, the pipeline cache fetch lock (#190) and compute stages let go of the 32-lane pin as https://github.com/NotRexed/Hookshot/pull/2564 does, one build for every run, for https://github.com/NotRexed/Hookshot/issues/999; the same patch rebased on main e0e6092 for the PR, built from source (llvm-mingw) | Windows 11 Pro (26300), 1920x1080, scripted input | Intel Arc Graphics (Core Ultra 7 155H iGPU, Meteor Lake), 32.0.101.9175 | Core Ultra 7 155H, 32 GB | Render resolution 4, handheld profile; `RX_VK_PASS_TIMING=2`, a screenshot every 60 frames; old/new alternated, the game closed normally; old: the same build with `RX_VK_SPLIT_UBFLUSH=1` | Kokiri Forest outside Link's house, standing, 300 s | White blocks over the forest in 9, 7 and 11 frames of the old runs that ran on, 0 in every new run (an earlier series of this change alone: 7, 1, 1 against 0, 0, 0); fps 4.7-5.7 old, 5.7-6.0 new. Device lost in compute programs (ab7699ee, aba5e6af, 499dbfd0) in 2 of 8 old and 5 of 8 new runs over both series. The same programs lose it with the float-add loop on main and the other Intel builds (12 of 18 runs in #191's series; 499dbfd0 is a float add to a storage buffer, ab7699ee likely one), none with #191's native add (0 of 6), and none in 6 route runs of a build with this change and #191 together (13/11): most likely the loop's, not this change's, though 5 of 8 against 2 of 8 is not explained |
HumanBean commented 2026-11-13 18:57:15 +00:00
Owner

Nit: in a docs file, #190 and #191 don't link anywhere, and on GitHub they would point at Hookshot. Like the PsychoV35 row's link to pulls/108, use #190 and .../pulls/191.

Nit: in a docs file, #190 and #191 don't link anywhere, and on GitHub they would point at Hookshot. Like the PsychoV35 row's link to pulls/108, use https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/190 and .../pulls/191.
gpu/vk/vk_replay.cpp
@ -1595,0 +1600,4 @@
// through its frame. Written at each part's end, on Intel without mirrors: white blocks and haze at Kokiri, and a
// whole-screen white flash every 8 frames on the Arc 140V (https://github.com/NotRexed/Hookshot/issues/999).
// Measured on an Intel iGPU only (no mirrors), so the default there; with mirrors (Arc discrete) the write goes to the
// mirror and nothing was seen: left as before. RX_VK_SPLIT_UBFLUSH=1: written at each part's end, as before, =0: at the
HumanBean commented 2026-11-13 18:57:15 +00:00
Owner

Nit: "with mirrors (Arc discrete) the write goes to the mirror and nothing was seen". Nobody ran Arc discrete, so nothing could be seen. Suggest "... goes to the mirror, and it was not tested there: left as before", to match the docs row ("where it was not measured").

Nit: "with mirrors (Arc discrete) the write goes to the mirror and nothing was seen". Nobody ran Arc discrete, so nothing *could* be seen. Suggest "... goes to the mirror, and it was not tested there: left as before", to match the docs row ("where it was not measured").
Some checks are pending
CI / Conventional Commit title (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions
Details
CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions
Details
Third-party parts / As their notes say (pull_request) Blocked by required conditions
Details
Third-party parts / The same as upstream (pull_request) Blocked by required conditions
Details
Website / Website on Cloudflare (pull_request) Blocked by required conditions
Details
PR description / Description follows the template (pull_request_target) Successful in 7s
Details
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-split-ub-flush:Guep-fix-split-ub-flush'
git switch 'Guep-fix-split-ub-flush'
Sign in to join this conversation.
Reviewers
No reviewers
HumanBean
Labels
Clear labels   
area/accessibility
Barriers for people with disabilities

  
area/audio
Sound and music

  
area/input
Controllers, keyboard, mouse

  
area/install
Installing, updating, building from your game

  
area/menu
In-game menu and settings

  
area/renderer
Vulkan renderer, shaders, effects

  
area/saves
Save files

  
area/upscaling
DLSS, FSR, frame generation

  
good first issue
A good start for a new contributor

  
gpu/amd
AMD GPUs

  
gpu/igpu
Integrated GPUs

  
gpu/intel
Intel GPUs

  
gpu/nvidia
NVIDIA GPUs

  
help wanted
Extra hands or knowledge wanted

  
kind
bug
 Something doesn't work as it should

  
kind
chore
 Build, CI, dependencies, tidying

  
kind
docs
 Documentation

  
kind
feature
 A new feature or an improvement

  
kind
performance
 Frame rate, frame time, loading, memory

  
kind
question
 A question rather than a change

  
needs measurements
Waiting for before/after measurements

  
platform/linux
Linux only

  
platform/steam-deck
Steam Deck

  
platform/windows
Windows only

  
preview
Maintainers: deploy a preview of this PR's website (read its changes first). Removed after the run.

  
priority
critical
 Crashes, data loss, or blocks most players

  
priority
high
 Important: next in line

  
priority
low
 Nice to have

  
status
blocked
 Waits on something else first

  
status
confirmed
 Reproduced or agreed on

  
status
duplicate
 Already reported or proposed

  
status
invalid
 Not a problem in this project

  
status
needs-info
 Waiting for more information from the reporter

  
status
wontfix
 Will not be worked on

  
testing wanted
Wanted: a test on other hardware (posts a test request in #test-requests)

No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
Milestone
Clear milestone
No items
No milestone
v3.0.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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!189
No description provided.