WIP: fix(vk): HUD and lighting artifacts on GPUs without buffer mirrors #222
No reviewers
Labels
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
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!222
Loading…
Reference in a new issue
No description provided.
Delete branch "dukenheimer/Longshot:fix/igpu-ub-snapshot"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
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.RX_VK_UB_SNAPSHOT_REUSEexplicitly enables reuse. The cache is reserved at recording start and cleared with the stream it points into.docs/development.md;tests/test_ub_snapshot_default.pycovers 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
RX_VK_UB_SNAPSHOTRX_VK_UB_SNAPSHOT_REUSEMeasurements
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
6749ba21(snapshot off)6749ba21+ thisTesting
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.pywas 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-outtest_direct_buffer_writes.py,test_frame_wait_bounds.py,test_fallback_buffer_cleanup.pyandtest_pipeline_cache_save.pypassed serially. The snapshot test is intentionally checked under WSL because it uses POSIXsetenv/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:
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=0turns snapshots off;RX_VK_UB_SNAPSHOT_REUSE=0makes each binding copy independently. Discrete GPUs whose global mirror setting is on but whose individual pool isMIRROR_REFUSEDorMIRROR_WANTEDare 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
type(scope): description).main.python -I -B tests/run_all.pyrun and passing (ran; local failures are named above, so the box remains open).third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.docs/development.md; the README's switch section points there).mainand passing here.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 andRX_VK_MIRROR=0discrete 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-localcur) and is cleared indraw_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, afterrecording_wait. Copies made before a stream grows stay valid, asc1_at's do.ub_overlayruns first, so ranges with stream uniform updates (flush_ub_updatesones 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
RX_VK_UB_SNAPSHOT_REUSEis on wherever the snapshot is on, so Intel iGPUs (and Intel discrete withRX_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".RX_VK_UB_SNAPSHOT's default and addsRX_VK_UB_SNAPSHOT_REUSE. Document both, or untick the box.RX_VK_UB_SNAPSHOT_REUSE=0), and the log'sreused copiescount next tocopied, so we can see whether the reuse pays for itself.Should change / questions
cap_view_genis bumped by everymemory_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 likeub_copiesinvk_replay.cpp.guest_cpu_write(smallNOP_CLEAR_BUF, counter reports, the CPU fallback ofNOP_COPY_B2Boutside 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 onmemory_page_stamp/ub_list_epochlikeub_copy_hithas.mirror_on()). On a discrete card with mirrors on, pools that areMIRROR_REFUSED(over 1 GB, or over the budget) or stillMIRROR_WANTEDare 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.RX_VK_UB_SNAPSHOT_REUSEparsing, the view key, and the clear indraw_begin_recording. Text checks liketests/test_mem_trim.py's would do.cd906265(#197, which touchesdraw()in vk_draw.cpp) and three more commits. There's no textual overlap with these hunks. Rebase before the Longshot runs and re-runtests/run_all.py, whose box is still unticked.#2419,#2343and#2464(vk_draw.cpp comment, the test's docstring andCASES), and so does the description ("#2464's rule"). Use full URLs.(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 thenit: 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);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'sBare GitHub numbers in a rewritten comment (
#2419here,#2343two 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);This only follows the global
mirror_on(). On a discrete card with mirrors on,MIRROR_REFUSEDpools (over 1 GB, or over the budget) andMIRROR_WANTEDones are also bound in place (vk_memory.cppgpu_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; }();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)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;The key is the capture view alone, and
cap_view_genis bumped by everymemory_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 thereused copiescount from the log next tocopied. Also,guest_cpu_write(smallNOP_CLEAR_BUF, counter reports, the CPU fallback ofNOP_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, butub_copy_hitalso checksub_list_epoch; would amemory_page_stampcheck 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};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 indraw_begin_recording, or use a small fixed table likeub_copiesin 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)This covers the default, but not the reuse, which is the riskier half: the
RX_VK_UB_SNAPSHOT_REUSEparsing, the view key, andr.ub_snap.clear()indraw_begin_recording. Text checks liketests/test_mem_trim.py's would cover it. Also,#2464and#2343(here and in the docstring) should be full GitHub URLs, and the file has no final newline.de88f4cf76a2ddd410b9Addressed the review and pushed
a2ddd41on the rebased branch (mainatd5fc892):RX_VK_UB_SNAPSHOT_REUSEnow 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.docs/development.md, and the PR description no longer claims an undocumented switch/reference update.reused copieslog count. I did not run the game for this update.guest_cpu_write/guest_cpu_wrotenow advances a dedicated generation. A cache entry includes that generation, so an in-recording CPU write cannot reuse an older copy.MIRROR_REFUSED/MIRROR_WANTEDdiscrete-pool cases under Risk; this PR still changes only the global mirrorless integrated-GPU default.(longshot), the closing-brace spacing, and the bare upstream issue/PR numbers; the branch is rebased on currentmain.Validation: the requested Windows
tests/run_all.pyrun 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.Follow-up
01e82d6: parallel recording writes are now included in the GPU-write fallback guard; the WSL snapshot fixture still passes.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.