Repository navigation
fix(vulkan): track writes from direct buffer bindings - #1133
Conversation
Record the direct write epoch and small result pages when mirrors are disabled, unavailable or refused. Match the existing later direct path so CPU invalidation can recognize these GPU writes. Add twelve production-extraction cases for page registration, epoch-only writes and existing readonly/mirrored controls. Based on the source observation in issue NotRexed#1126 by drpowerup. Assisted-by: OpenAI Codex Co-Authored-By: OpenAI Codex <noreply@openai.com>
hckrman101
left a comment
There was a problem hiding this comment.
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 the mstate != MIRROR_ON path further down. That path already sets direct_write_epoch and notes the pages, and this one didn't. Same 1 MiB cap, same clamp to the pool end, and mem_rec_value is the recording the write belongs to, so the readback wait in vk_replay.cpp waits for the right submission. wb_pages_note already drops value == 0 and RX_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 the cpu_words cut don't apply here. Setting direct_write_epoch on 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.py passes 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:
- the slice from
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. - main's
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::FastHash shim can go too.
Non-blocking:
- On mirror-off setups (iGPUs by default, i.e. the #1126 hardware, or
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. - Overlap: #1150 adds another early return in this function (
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.
| if (!mirror_on() || !mirror_upload || p->mstate == Pool::MIRROR_REFUSED) { | ||
| if (written) p->direct_write_epoch = mirror_epoch; | ||
| if (written && size <= (1u << 20)) | ||
| wb_pages_note(p->lo + off, p->lo + std::min(off + size, p->hi - p->lo), mem_rec_value); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done with note_direct_write(), thanks.
| header = (ROOT / "gpu/vk/vk_internal.h").read_text(encoding="utf-8") | ||
| types = "\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(")] |
There was a problem hiding this comment.
Against current main this slice includes struct WbPages, which needs kPages/kChunkPages/<memory>, and the extracted gpu_buffer_bind calls rb_hazard_locked(), so the test stops compiling after a rebase. It passed in CI only because the merge base was 8b2e1d3.
Normally merge main 7dbdcdc, preserving its page-table representation, parallel mirror guard, mirrored-write hazard callback and all test entries. Keep the original early-direct write epoch/page-note fix unchanged. Extract current page-table dimensions, include unique_ptr support and record the existing hazard callback in the authored fixture. Preserve all twelve assertions. Current-main negative: seven failures and five controls; fixed Windows GNU/MSVC-target and Linux checks pass. Assisted-by: OpenAI Codex Co-Authored-By: OpenAI Codex <noreply@openai.com>
hckrman101
left a comment
There was a problem hiding this comment.
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 in gpu_buffer_bind as before, and it still looks right.
Two small things, not blocking: the two copies of the direct-write lines could share a note_direct_write helper, 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 5609af4 plus the same 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. |
Normally merge main 6add2f1 and preserve all upstream runner entries. Share epoch and small-result page tracking between the two direct paths through note_direct_write, without importing the draft readback policy. Adapt the actual-function fixture for main's read-only asynchronous-import branch. Preserve all twelve original cases and eighteen require checks; add an assertion that no asynchronous streaming copy occurs. The same final fixture fails seven intended cases on unmodified main and passes all twelve with the fix. Four focused strict checks pass on both Windows and Linux, sequentially. No GPU or game validation was performed. Assisted-by: OpenAI Codex Co-Authored-By: OpenAI Codex <noreply@openai.com>
hckrman101
left a comment
There was a problem hiding this comment.
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 in gpu_buffer_bind use it under pools_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 your ARGS entry and main's new test_dlss_sdk_cache / test_builder_dlss_update / test_png_publication entries all land right after ARGS = {. Keep both. vk_memory.cpp merges 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?
|
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 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 What pointed at the wait:
After, mirrors off: no flicker in several runs of a few minutes each. The log now shows 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 |
|
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! |
Keep upstream shadow invalidation and writeback policy, extract the actual shadow helper in the fixture, and verify precise page invalidation. Register the test through RUN_ALL_ARGS while preserving main runner/API/workflow. Assisted-by: OpenAI Codex Co-Authored-By: Codex <noreply@openai.com>
…nce/pr1133-current-main
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for merging main. The conflict is gone and GitHub reports the branch as clean. The gpu_buffer_bind change 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. |
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.