fix(vulkan): track writes from direct buffer bindings #1133
No reviewers
Labels
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
No milestone
No assignees
1 participant
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Hookshot!1133
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/direct-buffer-write-bookkeeping"
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?
With buffer mirrors disabled or unavailable, CPU readback can miss GPU-written values because an early direct binding does not record the write's pages or epoch. Record that bookkeeping on both direct returns in
gpu_buffer_bind, addressing the source gap reported in #1126. Credit to @drpowerup for the original observation.note_direct_writemarks the current mirror epoch and records the translated, pool-clamped guest range for writes up to 1 MiB. Read-only bindings, mirrored writebacks, the currentWB_MAXpolicy and upstreamshadow_forgetbehavior remain intact. This does not include the separate #1150 import policy.This refresh normally merges
mainat117eae79, including #1858's presentation link repair and #1793's shared source-check repairs, without rewriting history. The runner, API header and workflow are unchanged from that base; the test declares its ownRUN_ALL_ARGS. The fixture extracts the actual productionshadow_forgetand checks precisely which pool-relative shadow pages are invalidated. Every original case and assertion is preserved. All 15 extracted production inputs, both complete fixture files and the runner are byte-identical to the previously tested7ca77cbhead; upstream's imported-buffer destruction changes are outside those inputs.Validation on this refresh:
Historical hardware reports remain attributed to their authors and earlier code: @DarkGhostHunter reported no more blinking wrong textures on an RDNA3 iGPU. @djanice1980 reported steady Castle-gate ground on Radeon 8060S/RADV, with about 9 ms/frame of readback-wait cost; that test applied page notes without the epoch line.
All eight hosted checks now pass on current head
2babca1a: Windows/Linux source suites, Windows Builder packaging, Windows Vulkan, Linux AppImage, both native lowering checks and workflow lint. Release-upload checks are correctly skipped for a pull request. The normal merges incorporate #1858's presentation definitions and #1793's shared source-check repairs.Gaps: no full local suite, current-head GPU/game or timeline test, or new iGPU measurement. The fixture executes actual binding/page-table/shadow functions with authored pool, mirror, host and hazard boundaries; it does not allocate a real Vulkan device or access a game dump. Hardware reports above remain historical and attributed to the original testers.
The fix itself looks right to me. The early return in
gpu_buffer_bind()(mirrors off, outside a recording, refused pool) binds the pool's own import, so the GPU writes guest memory directly, the same as themstate != MIRROR_ONpath further down. That path already setsdirect_write_epochand notes the pages, and this one didn't. Same 1 MiB cap, same clamp to the pool end, andmem_rec_valueis the recording the write belongs to, so the readback wait in vk_replay.cpp waits for the right submission.wb_pages_notealready dropsvalue == 0andRX_VK_NO_READBACK_WAIT. Read bindings (memo/TLS shortcuts) aren't touched. Direct writes never go through a write-back, so the write-back diff and thecpu_wordscut don't apply here. Settingdirect_write_epochon this path can only stop a mirror being made later in the same part, which is the intended rule.CI: both "Python source checks" jobs fail only on
tests/test_program_answer_domains.py, the same failure as on main.test_direct_buffer_writes.pypasses on Linux and Windows.One blocker, though: that pass was on the merge with
8b2e1d3. Main is ~160 commits ahead and the test won't compile against it:std::mutex wb_pages_mu;tovoid wb_pages_note(now pulls instruct WbPages(chunked table, #1161 and the bookkeeping series), which needskPages,kChunkPagesandstd::unique_ptr. The test only defineskPageShift.gpu_buffer_bindnow callsrb_hazard_locked()in the mirrored write path (#1069), and the test has no stub for it.Since it's registered in run_all.py, it'll go red as soon as CI runs on current main. Please rebase and update the stubs/extraction (tests/test_wb_pages.py on main already extracts the new table, which could help). The
Common::FastHashshim can go too.Non-blocking:
RX_VK_MIRROR=0), every small written binding now takeswb_pages_muand sets up to 257 pages, per binding per draw. On main's chunked table that's cheap, but a CPU readback that touches those pages will now actually wait (that's the point). A quick frame-time A/B on an iGPU would be good to have before this ships.rb_direct_hit) that binds guest memory without this bookkeeping. If written bindings can reach it, it has the same gap. With three copies of these two lines, a smallnote_direct_write(p, off, size)helper would keep them in sync. #1184 also edits this function, so expect a trivial conflict.These two lines are now in two places (and #1150 adds a third early return). A tiny helper used by both paths would keep them from drifting apart.
@ -0,0 +48,4 @@dimensions.append("constexpr uint64_t %s = %s;" % (name, value[1]))types = "\n".join(dimensions) + "\n\n" + "\n\n".join([block(header, "struct BufferRef {", True), block(source, "struct Pool {", True),block(source, "struct TlPool {", True), block(source, "struct WriteBack {", True)])pages = source[source.index("std::mutex wb_pages_mu;"):source.index("void wb_pages_note(")]Against current main this slice includes
struct WbPages, which needskPages/kChunkPages/<memory>, and the extractedgpu_buffer_bindcallsrb_hazard_locked(), so the test stops compiling after a rebase. It passed in CI only because the merge base was8b2e1d3.The blocker is fixed. The test now reads kPageShift, kChunkPages and kPages from the production source, so the chunked WbPages slice compiles. It also stubs
rb_hazard_locked()and checks that the mirror_on case calls it once with the right range. The code change is the same 3 lines ingpu_buffer_bindas before, and it still looks right.Two small things, not blocking: the two copies of the direct-write lines could share a
note_direct_writehelper, and a frame-time check on a mirror-off iGPU would still be nice to have. CI is green.Can confirm it works on iGPU (RDNA3): no more blinking wrong textures. Had a very hard stop on entering the market but probably shaders, after a few seconds it run again like normal. Performance is acceptable, it seems there is a lot of work to do there.
Hardware test on an integrated GPU with the mirror off, as asked above. Radeon 8060S (RADV STRIX_HALO, Mesa driver 0x6802006), CachyOS, kernel 7.3.3. Built from main
5609af4plus the samewb_pages_notelines in the no-mirror return ofgpu_buffer_bind(without thedirect_write_epochline).Before: shaded ground at the Hyrule Castle gate flipped between two colors several times a second (stone floor mean sRGB 68/61/50 vs 62/76/92 in two screenshots; sunlit areas, sky and HUD within 3%). No "readbacks of GPU results" line in the log. After: steady.
Frame-time A/B, same save, standing still, about 1335 draws a frame, frame generation and frame pacing off, one build with switches:
So the change fixes the flicker and costs about 9 ms a frame here; the mirror costs about 6 ms. For comparison, an RTX 5070 Ti with RX_VK_MIRROR=0 also stopped flickering with the change, with waits of 0.24 ms each (0.13 ms with the mirror on). I can rerun anything that would help.
Still looks right after the refresh.
note_direct_write()is a straight extraction of the old lines (epoch, 1 MiB cap, clamp to the pool end,mem_rec_value), both direct returns ingpu_buffer_binduse it underpools_mu, and the new async-import branch and TLS shortcut on main only take read bindings, so no written binding skips the bookkeeping. The fixture asserting no async copy is a nice touch. CI is green.The iGPU numbers from djanice1980 are useful: ~9 ms/frame of readback waits with the mirror off vs ~6 ms with it on is a real cost, but steady ground beats flicker, and it's worth a follow-up on the wait side rather than holding this back. On mirror-off setups the epoch line is a no-op anyway (
mirror_on()is false), so that test covers what ships there.One thing before merging: main has moved again and this now conflicts. The only conflict I can see is
tests/run_all.py, where yourARGSentry and main's newtest_dlss_sdk_cache/test_builder_dlss_update/test_png_publicationentries all land right afterARGS = {. Keep both.vk_memory.cppmerges cleanly and main's changes there (submitted pages, the commit walk) are outside the slices the test extracts, so the fixture should still build. Could you merge main once more?Done with note_direct_write(), thanks.
Hardware test on a Steam Deck OLED (AMD Custom APU 0932, RADV VANGOGH, SteamOS, kernel 7.2.7-valve2), mirrors off as on every integrated GPU.
Not this branch itself: I tested the equivalent change on
maind8f0513(plus #1625 and #1574), awb_pages_notefor written bindings of 1 MB or less in the no-mirror early return ofgpu_buffer_bind, without thedirect_write_epochline.Before: in Kokiri Forest the whole picture flickered several times a second, all at once: foliage going from dark green to brightly lit and back. It looks like the exposure stepping, not textures going missing. The log had
VRAM mirror of buffer pools offand never printed areadbacks of GPU resultsline, over tens of thousands of frames.What pointed at the wait:
RX_VK_MIRROR=1removed the flicker, withRX_VK_MIRROR_MB=1024and with256.RX_VK_MIRROR=1withRX_VK_NO_READBACK_WAIT=1brought it back. So it is the readback wait that fixes it, and the mirrors only mattered because their write-backs fedwb_pages.RX_VK_WAIT_GPU=1alone did not help.After, mirrors off: no flicker in several runs of a few minutes each. The log now shows
readbacks of GPU results: ~930 of 3000 waited for their write-back, 1.4-1.8 ms each.Cost here: about 30-31 fps (flickering) to about 26.5 fps at ~2,000 draws. The GPU takes ~17 ms a frame on this APU, so each wait is long. With
RX_VK_LOG_READBACK_WAITS=1, two alternating ranges of0x3980bytes account for nearly all of it, about 5 ms a wait (up to 14 ms), onModuleSystemWorkerthreads. The other ranges wait under 0.6 ms. Compared with getting the same fix throughRX_VK_MIRROR=1, this is 1-2 fps faster and leaves about 1 GB more memory free.Heads-up: this conflicts with main now (a lot landed today). Could you merge main into the branch (or rebase) and resolve the conflicts? Thanks!
Thanks for merging main. The conflict is gone and GitHub reports the branch as clean. The
gpu_buffer_bindchange is the same as the version I approved:note_direct_write()is used on both direct returns, only for written bindings, with the 1 MiB cap and the pool-end clamp. The only changes since are merges of main, and CI is green on this head (Linux and Windows source checks, Windows Vulkan, AppImage, Builder).The two non-blocking points still stand: a frame-time check on a mirror-off iGPU (djanice1980's and hclaria's numbers help here), and the readback-wait cost as a follow-up. Neither should hold this back.
Tested on a Lenovo Legion Go (Ryzen Z1 Extreme, Radeon 780M iGPU), Windows. On v2.0.14 (which doesn't include this PR) I get constant white flashes in Kokiri Forest, and RX_VK_MIRROR=1 removes them completely, which points at the non-mirror path this PR fixes. I'll test 2.1.0-rc without the env switch and report back.
I tested 2.1.0-RC2 and I can confirm there are no white flashes, at least on Linux.