Repository navigation
Conversation
06afdd2 to
ce164ce
Compare
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for this, the fps numbers are interesting and the code is tidy. What checks out:
- Lock order is consistent:
ParLock->pools_mu->rb_direct_muinrb_direct_apply, the direct bind path,mirror_makeand the write-back, andmemory_rb_direct_noteonly takesrb_direct_mu. No inversion that I can see. - The append-only range list (entry written, then release store of the count, acquire load in
rb_direct_hit) is fine for lock-free readers. cut_rb_direct's page math handles runs that start or end mid-page, and the test covers that.mirror_makepointing the direct pages back at guest memory is the right spot, andRX_VK_RB_DIRECT=0is a clean way out.- CI is green, including the new test.
My main concern is that I think the speedup comes from removing the readback wait that fixed the exposure flashes, not from skipping the write-back copy:
- The readback wait is gone for these pages.
invalidate()waits onmemory_writeback_value(), which only knows pages fromwb_pages_note(). That's called from the write-back runs (aftercut_rb_direct, so never for direct pages) and from the small in-place writes path. The new direct path doesn't call it. So the CPU can now read OoT's exposure meter, and set it to 0, before the replay has even recorded that frame's adds. This is the 0 / 2x pattern described in the EvilBoyce comment ininvalidate(). Also, the PR body says the GPU still sees what the CPU wrote as of the submit, but that's not true for direct pages. The GPU reads live guest memory there, so the CPU's zero for frame N+1 can land before frame N's adds. The "big flashes" count may not catch the small 0.2 EV steps, so it would be worth checking exposure values directly (RX_VK_LOG_READBACK). - Any overlap makes the whole binding direct.
rb_direct_hitis a plain overlap test against page-rounded ranges. So a bigger SSBO or UBO binding that only touches a readback page gets bound entirely in guest memory. Its GPU writes outside the readback pages never reach the mirror, and later mirror-bound or page-table reads see stale data. Its reads see live CPU writes instead of the submit-time capture, which is the tearing the mirror exists to prevent. The 4 KB rounding also pulls in neighbouring data in the same pages. cut_rb_directonly ever drops real data. Direct bindings never add write-backs, so the only write-back runs that can cover a direct page come from mirror bindings recorded before that range's first invalidate.invalidate()runs on the game thread, ahead of the replay. Cutting those runs throws away the GPU's result for the first readback.
Smaller points: ranges and pages never expire, so memory reused after a scene change stays direct for good, and a game with ring-buffer readbacks fills the 512 entries quickly. The new test isn't in ARGS in tests/run_all.py (inline comment). This overlaps #1133, which tracks writes from direct bindings in gpu_buffer_bind (the new early return skips that), and #1184, which touches mirror_make and the parallel segments. #1109 and #1069 are merged now, so a re-test on current main would help.
Suggestion: keep the direct binding but call wb_pages_note(g0, g0 + size, mem_rec_value) for written direct bindings so the CPU still waits, only go direct when the binding sits fully inside direct pages, and drop the cut (or only cut pages that were direct when the submission started). Then measure again to see how much of the gain is left.
| const int64_t nb = ++st_rb_direct_binds; | ||
| st_rb_direct_bytes += (int64_t)size; | ||
| if (nb % 20000 == 1) logf("vk: read-back direct: %lld bindings, %.1f KB each", (long long)nb, st_rb_direct_bytes.load() / 1024.0 / nb); | ||
| return remember({p->buffer, g0 - p->lo, size, 0}); |
There was a problem hiding this comment.
The in-place path below calls wb_pages_note(...) for written bindings so invalidate() waits for the GPU. This path doesn't, and the write-back that used to note these pages is cut, so memory_writeback_value() never returns a pending value for them. Something like if (written) wb_pages_note(g0, g0 + size, mem_rec_value); before returning would keep the wait.
| if (rb_direct_on()) { | ||
| if (rb_direct_pending_n.load(std::memory_order_relaxed)) rb_direct_apply(); | ||
| const uint64_t g0 = guest; | ||
| if (rb_direct_hit(g0, size)) { |
There was a problem hiding this comment.
This sends the whole binding direct if it overlaps a readback range at all. GPU writes to the rest of the binding then only land in guest memory: they aren't in writebacks and the mirror keeps stale data. Reads of the rest see live CPU writes instead of the capture. I'd only go direct when [g0, g0 + size) sits entirely inside direct pages, and fall through otherwise.
| runs.push_back({w.lo, w.lo, w.hi - w.lo}); | ||
| } | ||
| cut_cpu_words(*p, runs); | ||
| cut_rb_direct(runs, p->lo); |
There was a problem hiding this comment.
Since every overlapping binding now goes direct, a write-back run can only cover a direct page if the GPU wrote it into the mirror, from bindings recorded before memory_rb_direct_note ran on the game thread. Cutting it drops that result, so the first readback after the switch reads stale data. I think this cut should go, or only apply to pages that were already direct when the submission started recording.
| void texture_created_of(const nvn_tex* t) { tex_pages_add(t, +1); } | ||
| uint64_t fence(); | ||
| void invalidate(uint64_t gpu, uint64_t size) { | ||
| memory_rb_direct_note(host->gpu_guest(gpu), size); |
There was a problem hiding this comment.
Pages added here are never removed, so a range stays direct after the game frees or reuses that memory (a scene change), and ring-style readbacks at moving addresses will fill the 512 slots. Maybe drop the ranges of a pool when it's retired?
| with tempfile.TemporaryDirectory() as d: | ||
| src, exe = Path(d) / 't.cpp', Path(d) / 't' | ||
| src.write_text(fixture, encoding='utf-8') | ||
| subprocess.run([args.cxx, '-std=c++20', '-O1', '-o', str(exe), str(src)], check=True) |
There was a problem hiding this comment.
Could you add "tests/test_rb_direct.py": ["--cxx", "{cxx}"], to ARGS in tests/run_all.py, like the other C++ fixture tests? Then a runner without g++ skips it instead of failing. Also, this only covers the run cutting. The changes in behaviour (binding choice, readback wait) aren't tested.
|
NVIDIA / Windows: a large slowdown here. RTX 3070 8 GB, Windows 11, driver 0x98960000, main 09e4eb5 merged with this PR (clean merge), default
The path is active: Where the time goes. I ran both builds with #1243 on top, which times a capture's GPU wait for its late pages apart from the rest: So on this card about every sixth capture waits ~135 ms for the GPU. That is ~22 ms a capture on average, about six captures a frame. The replay thread mostly sits in My reading: the directly bound read-back ranges are written by the GPU straight into imported guest memory. The pages they share with mirrored data then become "late", and the next captures wait for the submission that writes them. The same wait shows, much smaller, when mirrors are short (#995). On RADV the wait may be short enough not to show. On the NVIDIA Windows driver, GPU writes to imported host memory seem to make it long. No device lost, the picture matches main (exposure looked the same), just the frame rate. |
|
On Windows with NVIDIA this makes the GPU about 18 times slower in Kokiri Forest. Setup. RTX 4080 SUPER, driver 616.92, Windows 11, Ryzen 7 5800X3D. Built from GitHub's
The title screen and the load look normal (GPU 0.2-6 ms a frame). The slowdown starts once the scene is drawn and stays. Main without this PR runs the same scene at 7-12 ms of GPU a frame. My guess, not verified: on this setup guest memory is imported host memory, so every shader read of those ranges crosses PCIe. If something reads the exposure sums per pixel or per draw, that adds up fast, while it may be cheap on RADV. I have not run Not checked: the AMD/Linux case the PR measured, and whether only some of the bound ranges cause it. |
|
@itaskaev2's numbers match my logs, and they correct my reading above. In the same runs, the log's
So the cause is GPU-side: the work itself gets ~13x slower once those ranges are bound in imported host memory, plausibly the per-pixel or per-draw reads over PCIe that @itaskaev2 suggests. The capture's late-page wait I measured (22 ms) is just the replay thread waiting for that slow submission, not a separate cost. Sorry for the misleading framing. |
|
One data point from AMD / Windows, because the reports above are NVIDIA / Windows and show a large GPU slowdown: I do not see that slowdown on an RX 7800 XT. RX 7800 XT 16 GB, Ryzen 7 5800X3D, Windows 11, 2560x1440, frame generation x2, v2.0.11 with this PR merged on top (clean merge; the same build also has #1267 and some AMD changes of mine), normal play with a save, about 12000 presents. From the log's I have no controlled A/B (no scripted route, |
A range the game invalidates to read (nvnMemoryPoolInvalidateMappedRange, nvnBufferInvalidateMappedRange) holds GPU results. In a mirror they reach guest memory only by a write-back at the end of the submission. From a range's first invalidate on, a binding wholly inside it is bound in guest memory, with the guest page table pointing there too, so the GPU's writes land where the CPU reads them. The CPU's read still waits for the submission. Only ranges up to 64 KB go direct, and a retired pool's ranges are dropped. RX_VK_RB_DIRECT=0: as before. RX 7800 XT, RADV, ~2150-draw Kokiri Forest view: 50.9 -> 55.1 fps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ce164ce to
a5bee1b
Compare
|
Thanks all, this was very useful. hckrman101's main concern was right, and I've reworked the PR and put it in draft. Review points
Exposure: Result on RX 7800 XT / RADV: main 51.3, 51.6, 49.9 fps, this version 56.9, 54.2, 54.2. A 4-minute soak averaged 54.7. I also tried exact, unrounded ranges, and the gain went away (51.2-51.8 fps), so ranges stay page-rounded. NVIDIA: thanks Nitnek and itaskaev2 for the GPU time numbers. They point to the GPU work itself being slow in imported memory. Only ranges up to 64 KB now go direct, which brings the average direct binding from about 400 KB to 3.4 KB. I can't test NVIDIA here, and this may not be enough if the cost is per-pixel access to small ranges such as the exposure sums. If either of you has time, could you rerun with this version and post the Thanks also lucasmazzoni45-del for the AMD/Windows data point. 🤖 Generated with Claude Code |
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for the rework, this is much closer. Everything from my last review is addressed:
- Read-back wait: written direct bindings call
wb_pages_note, soinvalidate()waits again. UsingRX_VK_NO_READBACK_WAIT=1as a control for the spike count is a nice way to show the measurement can see the bug. - Partial overlap: only bindings wholly inside a range go direct now.
cut_rb_direct: gone, so mirror write-backs are kept.- Expiry: a retired pool drops its ranges and pages, and freed slots get reused.
- Tests: in
ARGS, and they cover the binding choice, with source checks for the wait, the cap and the retirement.
What's still open:
- NVIDIA. The first version made GPU time 13-18x worse on two NVIDIA/Windows setups, and nothing confirms yet that the 64 KB cap fixes it. I'd want a rerun from Nitnek or itaskaev2 before this leaves draft. If it's still slow, defaulting
RX_VK_RB_DIRECToff on NVIDIA (vendorID 0x10DE) would keep the RADV gain without the risk. - Mirror and direct access to the same pages (inline). Probably rare in OoT, but a counter would settle it.
- Merge conflict with main, in
tests/run_all.pyonly. YourARGSline is next to main's newtest_present_geometry_trace.pyentry, so keep both.vk_memory.cpp,vk_replay.cppandvk_internal.hmerge cleanly, including #1184'smirror_inheritchanges inmirror_make.
CI is green. I'm leaving this as a comment while it's a draft waiting on the NVIDIA retest. On the code side I'd approve once that's in, or once it's gated off on NVIDIA.
| if (rb_direct_on()) { | ||
| if (rb_direct_pending_n.load(std::memory_order_relaxed)) rb_direct_apply(); | ||
| const uint64_t g0 = guest; | ||
| if (rb_direct_inside(g0, size)) { |
There was a problem hiding this comment.
One coherence gap is left: the mirror's copy of a direct page goes stale. GPU writes here land in guest memory and don't bump the page generation, so mirror_sync never copies those pages again. The mirror keeps what it had at mirror_make or at the CPU's last write to the page. Two things follow:
- A binding that only partly overlaps a range stays mirrored, so it reads that stale copy instead of the GPU's results.
- If such a binding writes, its write-back copies mirror -> guest at the end of the submission. That lands after the direct writes, whatever the command order was. The diff copies only the words it changed, but a pool without a shadow (no shadow is made for write-backs under 256 KB) or
RX_VK_WB_DIFF=0copies the whole run and can wipe that submission's direct results.
The same thing happens once per range on the switch-over: bindings recorded before the first invalidate are mirrored and written back over the direct ones later in that submission. It happens again after a pool retirement drops the range. That costs one wrong read-back each time.
I doubt OoT hits the steady-state case. A cheap check would tell you: on the mirror path, a written binding that overlaps rb_direct_pages, logged once. If it does show up, sending that binding direct too, or keeping the range mirrored, would close it.
|
Tested on AMD / RADV. Setup: RX 9070 XT (RADV, Mesa, Vulkan 1.4), Ryzen 9 9950X3D, NixOS. Linux build of
No regression, no visible flashes, and a small fps gain here. |
|
Retest of the reworked 64 KiB-capped version: the large NVIDIA slowdown is still present on this RTX 3070. Tested exact head
The B logs confirm the direct path was exercised. A2 returned to the fast regime after 60.685 s of scene warm-up, with 62.294 s measured. Both OFF runs being fast, while both ON runs have much higher GPU time, supports the slowdown association in this scene even after the binding-size cap. Limits: A2 began 63.26 minutes after B2, with intervening activity/cache history: this was not a continuous ABBA. Sparse matched captures do not establish flash-free rendering or direct/mirror coherency. Initial preparation reported 83 failures, retained as a qualification limitation rather than erased by the later cached run. A2 was deliberately stopped at its bound (game exit 1); its native controller exit code was not recorded. These are performance observations, not a clean shader/correctness/stability pass or a claim about all NVIDIA cards/scenes. For this RTX 3070, the 64 KiB cap is not sufficient. I would keep this path disabled on NVIDIA pending further work; this does not invalidate the separate AMD results. No FPS gain is claimed from A2 versus A1: both are OFF controls. AI-assisted testing and analysis with Hermes Agent / Claude Code; measurements come from the recorded native runs. |
Joey-Magnus
left a comment
There was a problem hiding this comment.
Two things before this comes out of draft.
First, @Nitnek retested this exact head (a5bee1b) on an RTX 3070. GPU time still goes from about 13 ms to about 151 ms a frame with the direct path on. The path is on by default wherever mirrors are on: rb_direct_on() in gpu/vk/vk_memory.cpp:2010 returns true when RX_VK_RB_DIRECT is unset. Unless the cause is found, it should default to off on NVIDIA (vendorID 0x10de).
Second, that switch is read with atoi. Since #2042, switches go through env_value_off in gpu/vk/compat/hookshot_env.h.
The branch also conflicts with main in vk_memory.cpp, vk_replay.cpp and run_all.py. Since this branch's base, main has merged:
- #1133's direct-write bookkeeping (the direct_write_epoch line @mrdev023 mentioned is needed here);
- #2131's write-back observations;
- #2343, which turns mirrors off by default on GCN cards.
So the rebase needs care around the mirror and write-back paths. test_rb_direct.py passes on the head as it stands.
|
I rebased this onto current main and applied the review points on a branch in my fork: fix/pr1150-readback-direct.
Tests pass on Windows (llvm-mingw) apart from 4 failures that also fail on main here. Not measured: no in-game or NVIDIA run, so the NVIDIA default is untested on hardware. 🤖 Generated with Claude Code |
|
Update to my branch fix/pr1150-readback-direct: 🤖 Generated with Claude Code |
Draft: reworked after review. It still needs an NVIDIA retest before it can be considered.
What
A range the game invalidates to read (
nvnMemoryPoolInvalidateMappedRange,nvnBufferInvalidateMappedRange) holds GPU results. From a range's first invalidate on, a binding wholly inside it is bound in guest memory instead of the VRAM mirror, and the guest page table points there too. Ranges are rounded to pages.wb_pages_note, so the CPU's read still waits for that submission.RX_VK_RB_DIRECT=0restores the old behaviour. It only applies when mirrors are on.Measurements
RX 7800 XT, RADV (Mesa 26.2.6), Ryzen 7 7800X3D, Linux, ~2150-draw Kokiri Forest balcony view, default settings, base 7dbdcdc. Only the renderer
.sodiffers, runs were alternated, and frames were presented through a headless weston on the same GPU.RX_VK_RB_DIRECT=0)A 4-minute run averaged 54.7 fps over 13,146 frames with 0 flashes and no device loss. Direct bindings average 3.4 KB, against about 400 KB in the first version. The spike count takes every frame's mean brightness. With the read-back wait removed (
RX_VK_NO_READBACK_WAIT=1) it reports 270-360 spikes a minute, so it can see the bug.Limits
direct_write_epochupdate.Tests
tests/test_rb_direct.pychecks which bindings go direct, the range list's limits and reuse, and (from the source) the read-back wait, the size cap and the pool retirement.tests/run_all.pyhas the same two failures as main (nx2/test_nup_read_name.pyand the byte-compile step), plus this test passing.🤖 Generated with Claude Code