perf(vk): a PC-like dGPU policy (RX_VK_DGPU=1) - direct guest-memory binds, mirrors only where a measured trial proves them #2338

github-sync · 2026-11-11T15:13:50Z

blessedplummer commented

2026-11-11 15:13:50 +00:00

(Migrated from github.com)

Why

4K is CPU-bound by the memory sync (#2099): at 3840x2160 on an RX 9070 XT (RADV) the game runs 35.6 fps where 1080p runs 60, with the GPU starved (9 ms of a ~14 ms frame). The cost is unified-memory emulation: every guest pool gets a VRAM mirror, kept coherent by captures, shadows and write-backs - the write-back diff alone compares ~3.1 GB per 600 submits and mirror syncs move ~137k dirty pages in the same window.

A dGPU does not need that emulation. This PR adds one opt-in switch that makes the replay treat it like a PC engine treats its GPU: the GPU works in its own memory, and the bus carries only what the CPU actually reads.

What `RX_VK_DGPU=1` changes (unset = stock exactly)

Direct by default: a pool's first binding makes no mirror (MIRROR_REFUSED, the existing state iGPU runs use today - and the state #2343 now uses for GCN by default). Reads take the lock-free TLS path; GPU reads/writes land in the imported guest memory, coherent by construction - no mirror sync, no write-back, no shadow/diff for that pool. Mirror-less pools read uniform buffers as submitted (#1795's per-pool form), so the artifact class that issue documents is covered under the switch too.
Mirrors are earned, by measurement: every 600-submit window, the heaviest guest-memory-read pool with >=1 MB of reads and not write-dominated runs a trial (120 frames direct -> 12 settle, held until the mirror is really on -> 120 frames mirrored). The verdict decides on present-to-present frame time and keeps the mirror only when the win is both >10% (RX_VK_DGPU_GAIN) and >0.25 ms (RX_VK_DGPU_FLOOR). A lost verdict appends the pool to hookshot_mirror_skip.txt (game folder; carries a device+driver header, so a file from another device or driver is ignored) so later runs do not re-trial it. Trials whose windows did not measure the same workload abort with no verdict and no skip line; a pool that aborts or is voided three times sits out the session. Trials do not run under RX_VK_PARALLEL or where mirrors cannot come on (iGPUs, GCN).
Knobs: RX_VK_DGPU_TRIALS=0 (pure direct), RX_VK_DGPU_GAIN (10), RX_VK_DGPU_FLOOR (0.25).

Default-off is exact: the added counters and logging are gated on the switch (found and fixed in review - the first cut taxed the unset fast path on iGPU tiers and logged on every stock run).

Measurement

Status: measured to finality. The trial machinery is proven end-to-end and the positive fps claim is measured to be not defensible on this hardware — the RX 9070 XT (Navi 48, RADV/Mesa, PCIe Gen5 x16, ReBAR on) that every measurement in this PR ran on; the RTX 3080 Ti numbers below in the thread are @cashcon57's rig, not ours. The reason is quantified, not assumed. On the fixed branch the trial engine works completely: the first keep was earned and held across 3/3 runs (pool 0x109a000000, 160 MB: 33.94 -> 21.80 / 36.41 -> 22.31 / 24.68 -> 22.03 ms in the trial windows - the mirror beats the policy's own direct mode by 10.7-39.7%, kept with 0 dirty pages synced per window), releases converge into the device-keyed skip list (0 -> 10 entries over a session), churn is bounded (23 -> 3 attempts/run), and the false-void signature is gone (0 voids across the run set). The headline heavy reader (0x1186000000, ~7 GB of guest reads/window) trialed at 21.70 -> 21.41 and was correctly released: ~7 GB/window of guest-memory reads costs 0.29 ms on this rig - ReBAR + Gen5 makes them nearly free, measured directly.

That is why the pairs still lose (P -4.1% / Q -1.3% / R -2.7%, median -2.7%, steady-state confirmed): the mirror can only climb back to near-stock, never past it, when stock's guest reads cost almost nothing and the UB snapshot costs parity (measured via the RX_VK_DGPU_TRIALS=0 discriminator). The value case is empirically confined to hardware where guest reads are NOT nearly free: small-BAR / Gen4-class / proprietary-driver rigs - exactly the matrix the review proposed (the independent RTX 3080 Ti datapoint in the thread is that class). Keep-as-opt-in-machinery vs withdraw-and-re-file-the-#1795-snapshot-standalone is put to the maintainers in the thread; both paths are held. The historical pairs below are from a pre-rework build on an older main (cross-era absolute fps is not comparable; adjacent pairs are the controlled evidence):

For the record, the pre-rework build's numbers (measured on an older main; cross-era absolute fps is not comparable, the adjacent pairs are the controlled evidence):

pair	OFF fps	ON fps	delta
E	70.2	78.0	+11.1%
F	71.0	77.0	+8.5%
G	59.6	76.6	+28.6%
median	70.2	77.0	+9.7%

An earlier +10.7% batch was retracted before this PR: kill-autosaves drift the save position between runs, so those pairs compared different scenes. All pairs warp-normalize.

Known limits (stated honestly)

A lighter scene (Kokiri bridge) measured ~parity on the pre-rework build - the win scales with coherence load.
The remaining "memory sync" ms under the policy is the submit-time capture of CPU-written pages (texture recreation correctness, ~30 MB/submit), which the policy deliberately does not touch: page-granular write-back culling is unsafe (skipping it reproduced a 3941-vs-1364 draw drift - the game reads GPU results outside the readback API), so gating is pool-level.
On this GPU's large BAR, PCIe reads are nearly free, so most pools rightly stay direct; small-BAR devices are where earned mirrors should matter most, and that is untested (one rig only).
#1795's event-waits fix is keyed globally, not per pool; under the switch event waits stay on on a dGPU. Whether that needs the same per-pool treatment is an open question flagged to the reviewers.

Tests and verification

New: tests/test_dgpu_policy.{py,cpp} (pure rules: state transitions, window boundaries, verdict rule incl. the measured 10.32->9.23 ms case as a CHECK, candidate threshold, void semantics, keep-line format) and a dgpu_default case in the direct-buffer-writes fixture asserting REFUSED, no mirror queued, no write-back, shadow pages forgotten.
Game-free checks: dgpu_policy, direct_buffer_writes, parallel_mirror_make, parallel_rb_refresh, wb_pages, bind_memo, draw_binds - all pass. Suite: 415 passed / 0 failed vs pristine 414 / 0 on the same base.
docs/development.md: switch rows added.
Independently reviewed (hckrman101, Joey-Magnus; all findings addressed) - and the review's hardening is exactly what the re-measurement caught misfiring, now being fixed with the re-run to follow.

Related: #2099, #995, #1150, #1377, #1629.

## Why 4K is CPU-bound by the memory sync (#2099): at 3840x2160 on an RX 9070 XT (RADV) the game runs 35.6 fps where 1080p runs 60, with the GPU starved (9 ms of a ~14 ms frame). The cost is unified-memory emulation: every guest pool gets a VRAM mirror, kept coherent by captures, shadows and write-backs - the write-back diff alone compares ~3.1 GB per 600 submits and mirror syncs move ~137k dirty pages in the same window. A dGPU does not need that emulation. This PR adds one opt-in switch that makes the replay treat it like a PC engine treats its GPU: the GPU works in its own memory, and the bus carries only what the CPU actually reads. ## What `RX_VK_DGPU=1` changes (unset = stock exactly) 1. **Direct by default**: a pool's first binding makes no mirror (`MIRROR_REFUSED`, the existing state iGPU runs use today - and the state #2343 now uses for GCN by default). Reads take the lock-free TLS path; GPU reads/writes land in the imported guest memory, coherent by construction - no mirror sync, no write-back, no shadow/diff for that pool. Mirror-less pools read uniform buffers as submitted (#1795's per-pool form), so the artifact class that issue documents is covered under the switch too. 2. **Mirrors are earned, by measurement**: every 600-submit window, the heaviest guest-memory-read pool with >=1 MB of reads and not write-dominated runs a trial (120 frames direct -> 12 settle, held until the mirror is really on -> 120 frames mirrored). The verdict decides on present-to-present frame time and keeps the mirror only when the win is both >10% (`RX_VK_DGPU_GAIN`) and >0.25 ms (`RX_VK_DGPU_FLOOR`). A lost verdict appends the pool to `hookshot_mirror_skip.txt` (game folder; carries a device+driver header, so a file from another device or driver is ignored) so later runs do not re-trial it. Trials whose windows did not measure the same workload abort with no verdict and no skip line; a pool that aborts or is voided three times sits out the session. Trials do not run under `RX_VK_PARALLEL` or where mirrors cannot come on (iGPUs, GCN). 3. **Knobs**: `RX_VK_DGPU_TRIALS=0` (pure direct), `RX_VK_DGPU_GAIN` (10), `RX_VK_DGPU_FLOOR` (0.25). Default-off is exact: the added counters and logging are gated on the switch (found and fixed in review - the first cut taxed the unset fast path on iGPU tiers and logged on every stock run). ## Measurement **Status: measured to finality. The trial machinery is proven end-to-end and the positive fps claim is measured to be not defensible on this hardware — the RX 9070 XT (Navi 48, RADV/Mesa, PCIe Gen5 x16, ReBAR on) that every measurement in this PR ran on; the RTX 3080 Ti numbers below in the thread are @cashcon57's rig, not ours. The reason is quantified, not assumed.** On the fixed branch the trial engine works completely: the first keep was earned and held across 3/3 runs (pool 0x109a000000, 160 MB: 33.94 -> 21.80 / 36.41 -> 22.31 / 24.68 -> 22.03 ms in the trial windows - the mirror beats the policy's own direct mode by 10.7-39.7%, kept with 0 dirty pages synced per window), releases converge into the device-keyed skip list (0 -> 10 entries over a session), churn is bounded (23 -> 3 attempts/run), and the false-void signature is gone (0 voids across the run set). The headline heavy reader (0x1186000000, ~7 GB of guest reads/window) trialed at 21.70 -> 21.41 and was correctly released: **~7 GB/window of guest-memory reads costs 0.29 ms on this rig** - ReBAR + Gen5 makes them nearly free, measured directly. That is why the pairs still lose (P -4.1% / Q -1.3% / R -2.7%, median -2.7%, steady-state confirmed): the mirror can only climb back to near-stock, never past it, when stock's guest reads cost almost nothing and the UB snapshot costs parity (measured via the RX_VK_DGPU_TRIALS=0 discriminator). The value case is empirically confined to hardware where guest reads are NOT nearly free: small-BAR / Gen4-class / proprietary-driver rigs - exactly the matrix the review proposed (the independent RTX 3080 Ti datapoint in the thread is that class). Keep-as-opt-in-machinery vs withdraw-and-re-file-the-#1795-snapshot-standalone is put to the maintainers in the thread; both paths are held. The historical pairs below are from a pre-rework build on an older main (cross-era absolute fps is not comparable; adjacent pairs are the controlled evidence): For the record, the pre-rework build's numbers (measured on an older main; cross-era absolute fps is not comparable, the adjacent pairs are the controlled evidence): | pair | OFF fps | ON fps | delta | |---|---|---|---| | E | 70.2 | 78.0 | +11.1% | | F | 71.0 | 77.0 | +8.5% | | G | 59.6 | 76.6 | +28.6% | | median | 70.2 | 77.0 | +9.7% | An earlier +10.7% batch was retracted before this PR: kill-autosaves drift the save position between runs, so those pairs compared different scenes. All pairs warp-normalize. ## Known limits (stated honestly) - A lighter scene (Kokiri bridge) measured ~parity on the pre-rework build - the win scales with coherence load. - The remaining "memory sync" ms under the policy is the submit-time capture of CPU-written pages (texture recreation correctness, ~30 MB/submit), which the policy deliberately does not touch: page-granular write-back culling is unsafe (skipping it reproduced a 3941-vs-1364 draw drift - the game reads GPU results outside the readback API), so gating is pool-level. - On this GPU's large BAR, PCIe reads are nearly free, so most pools rightly stay direct; small-BAR devices are where earned mirrors should matter most, and that is untested (one rig only). - #1795's event-waits fix is keyed globally, not per pool; under the switch event waits stay on on a dGPU. Whether that needs the same per-pool treatment is an open question flagged to the reviewers. ## Tests and verification - New: `tests/test_dgpu_policy.{py,cpp}` (pure rules: state transitions, window boundaries, verdict rule incl. the measured 10.32->9.23 ms case as a CHECK, candidate threshold, void semantics, keep-line format) and a `dgpu_default` case in the direct-buffer-writes fixture asserting REFUSED, no mirror queued, no write-back, shadow pages forgotten. - Game-free checks: `dgpu_policy`, `direct_buffer_writes`, `parallel_mirror_make`, `parallel_rb_refresh`, `wb_pages`, `bind_memo`, `draw_binds` - all pass. Suite: 415 passed / 0 failed vs pristine 414 / 0 on the same base. - `docs/development.md`: switch rows added. - Independently reviewed (hckrman101, Joey-Magnus; all findings addressed) - and the review's hardening is exactly what the re-measurement caught misfiring, now being fixed with the re-run to follow. Related: #2099, #995, #1150, #1377, #1629.

blessedplummer commented

2026-11-11 15:48:08 +00:00

(Migrated from github.com)

The three failing checks (appimage, SPIR-V fixtures, Ubuntu Python checks) are runner-side DNS outages, not PR failures: all three lost archive.ubuntu.com / security.ubuntu.com resolution in the System packages step across three run windows (15:14, 15:20, 15:41 UTC) while the Windows jobs (which don't use apt) pass every time. The code itself is verified locally: full suite 352 passed / 0 failed vs pristine 351/0, and the seven game-free checks over the changed code all pass. I'll leave these to rerun once the resolver recovers; happy to re-trigger if preferred.

hckrman101 (Migrated from github.com) reviewed

2026-11-11 19:57:33 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks for this, and for the careful write-up. The measurements are clear, the retracted batch is a good sign, and keeping the decision logic in a pure header with its own test makes it easy to check. I read the whole diff against the head and the code around it. Some notes, then a few questions for you.

What I checked and found fine

With RX_VK_DGPU unset, behaviour matches main. Every new branch is gated on dgpu_on(). Moving !par later in the make condition doesn't change anything, because the terms before mirror_make have no side effects. gpu_ns_total always accumulates, but nothing reads it when the switch is off.
Direct binds use the same import as the iGPU / RX_VK_MIRROR=0 path. Host-pointer imports only take a HOST_VISIBLE|HOST_COHERENT type, so no flush or invalidate is missing. (The dma-buf path's fallback to any memory type is on main already, not from this PR.) GPU writes are seen by the CPU once the submission completes, the same as the refused state today.
The tests need no game. test_dgpu_policy.py has RUN_ALL_ARGS and run_all finds it. It passed on Ubuntu and Windows CI, and the new dgpu_default case is listed in CASES.
CI: the only red job is Windows part 2, on test_native_astc_texture_count.py. That's main's own scanf deprecation failure, fixed on main by #2397, so a rebase clears it. There's no conflict with #2198 (it touches other files), and GitHub reports the PR as mergeable.

Correctness: in-place reads of CPU-rewritten memory
Under the policy, every pool, uniform buffers included, is bound in guest memory. The GPU then reads the game's memory when it executes, not when the work was submitted. #1795 (open) documents what that does on mirror-less GPUs: the game rewrites constants right after the submit, and when the GPU runs behind, draws read the next frame's data. On a Steam Deck that showed up as a giant HUD heart, slabs and stretched geometry. On dGPUs today the mirror is synced at submit, which hides this. The policy takes that protection away. #1795's fix is keyed on mirrors being off, which RX_VK_DGPU=1 doesn't set, so it wouldn't cover this either. Draw-count parity doesn't catch this kind of bug. A flicker/screenshot check in a GPU-bound scene is needed (see question 3). One option is to have the policy turn on the UB snapshot for direct pools.

The trial measures the wrong cost
gpu_time_total_ns() adds TOP..BOTTOM timestamps written into r.cmd only. mirror_sync and mirror_make record their copies into upload_command_buffer(), which isn't timed, and the CPU-side capture, diff and write-back work isn't counted at all. So the mirrored window leaves out exactly the costs this PR exists to remove, and verdicts lean towards keeping mirrors. Your own diagnosis is that the frame is CPU-bound, so a GPU-only verdict can keep a mirror that loses on frame time. It would be worth timing the upload buffer too, or deciding on present-to-present frame time or the replay's own ms.

Smaller points (inline)

The par_active guard in dgpu_pick never triggers. par_active is only true while segments are being recorded and is false again before memory_end_submission. So trials do run under RX_VK_PARALLEL. The docs row and the PR description both say they don't, and that parallel keeps the stock all-mirrored policy. Under the policy, parallel runs also mark pools refused. In practice the deferred make goes through mirror_wanted and the settle waits for MIRROR_ON, so it probably works. The claim and the code just need to agree.
On the fast path, p->dgpu_binds++ isn't gated on !par. Parallel segment workers increment it concurrently without a lock: a data race, and lost counts feed the 2x bind-volume check.
An abort is retried forever. With RX_VK_DGPU=1 on an iGPU (mirror_on() false), or when a dGPU's mirror budget is full, the same heaviest pool starts a trial every window and aborts 132 frames later, logging each time. It might be worth requiring mirror_on() in dgpu_on(), or giving up on a pool after N aborts.
The skip list is keyed only on the guest pool start, and a verdict lasts forever. One 120-frame window in one scene decides it for good, even across a GPU swap, a driver update or a different resolution. It might be worth recording the device or driver in the file, or letting entries expire.
Nit: RUN_ALL_ARGS = ["--cxx", "g++"] hard-codes g++. Neighbouring tests use "{cxx}", so a clang-only machine still runs them.

Questions for you

Do you recommend turning this on by default, either on dGPUs or everywhere? Why or why not? Your own numbers show parity in lighter scenes and a win that grows with coherence load, so I'd like to hear where you think the line is.
If we do turn it on, which settings and thresholds should we use? The 120/12/120-frame trial windows, the 1 MB candidate floor, RX_VK_DGPU_GAIN and RX_VK_DGPU_FLOOR, and RX_VK_DGPU_TRIALS: should trials run at all by default, or is pure direct enough? And should the skip list ship enabled?
What testing do we need before flipping it, and what have you already tested? I'm thinking of NVIDIA (proprietary on Windows and Linux), AMD (RADV and the Windows driver, including small-BAR / non-ReBAR systems, where PCIe reads aren't nearly free), Intel Arc, and iGPUs / UMA, on both Linux and Windows. For scenes, a few heavy and light ones (Hyrule Field, Kokiri, Kakariko, a dungeon, menus and pause, save reload) at 1080p and 4K, capped and uncapped. For each: fps and frame-time percentiles, flicker/artifact screenshots (the #1795 failure mode), and correctness (draw parity, readbacks, no cyan after a pause as in #2154). So far I only see RX 9070 XT / RADV at 4K.

Related open PRs in this area to coordinate with: #1795 (UB snapshot when mirrors are off), #2154 (keeps mirrors of pools the GPU writes), #1377 (mirror the pools the GPU reads most) and #2343 (GCN: mirrors off by default).

Thanks for this, and for the careful write-up. The measurements are clear, the retracted batch is a good sign, and keeping the decision logic in a pure header with its own test makes it easy to check. I read the whole diff against the head and the code around it. Some notes, then a few questions for you. **What I checked and found fine** - With `RX_VK_DGPU` unset, behaviour matches main. Every new branch is gated on `dgpu_on()`. Moving `!par` later in the make condition doesn't change anything, because the terms before `mirror_make` have no side effects. `gpu_ns_total` always accumulates, but nothing reads it when the switch is off. - Direct binds use the same import as the iGPU / `RX_VK_MIRROR=0` path. Host-pointer imports only take a `HOST_VISIBLE|HOST_COHERENT` type, so no flush or invalidate is missing. (The dma-buf path's fallback to any memory type is on main already, not from this PR.) GPU writes are seen by the CPU once the submission completes, the same as the refused state today. - The tests need no game. `test_dgpu_policy.py` has `RUN_ALL_ARGS` and run_all finds it. It passed on Ubuntu and Windows CI, and the new `dgpu_default` case is listed in `CASES`. - CI: the only red job is Windows part 2, on `test_native_astc_texture_count.py`. That's main's own `scanf` deprecation failure, fixed on main by #2397, so a rebase clears it. There's no conflict with #2198 (it touches other files), and GitHub reports the PR as mergeable. **Correctness: in-place reads of CPU-rewritten memory** Under the policy, every pool, uniform buffers included, is bound in guest memory. The GPU then reads the game's memory when it *executes*, not when the work was submitted. #1795 (open) documents what that does on mirror-less GPUs: the game rewrites constants right after the submit, and when the GPU runs behind, draws read the next frame's data. On a Steam Deck that showed up as a giant HUD heart, slabs and stretched geometry. On dGPUs today the mirror is synced at submit, which hides this. The policy takes that protection away. #1795's fix is keyed on mirrors being off, which `RX_VK_DGPU=1` doesn't set, so it wouldn't cover this either. Draw-count parity doesn't catch this kind of bug. A flicker/screenshot check in a GPU-bound scene is needed (see question 3). One option is to have the policy turn on the UB snapshot for direct pools. **The trial measures the wrong cost** `gpu_time_total_ns()` adds TOP..BOTTOM timestamps written into `r.cmd` only. `mirror_sync` and `mirror_make` record their copies into `upload_command_buffer()`, which isn't timed, and the CPU-side capture, diff and write-back work isn't counted at all. So the mirrored window leaves out exactly the costs this PR exists to remove, and verdicts lean towards keeping mirrors. Your own diagnosis is that the frame is CPU-bound, so a GPU-only verdict can keep a mirror that loses on frame time. It would be worth timing the upload buffer too, or deciding on present-to-present frame time or the replay's own ms. **Smaller points (inline)** - The `par_active` guard in `dgpu_pick` never triggers. `par_active` is only true while segments are being recorded and is false again before `memory_end_submission`. So trials *do* run under `RX_VK_PARALLEL`. The docs row and the PR description both say they don't, and that parallel keeps the stock all-mirrored policy. Under the policy, parallel runs also mark pools refused. In practice the deferred make goes through `mirror_wanted` and the settle waits for `MIRROR_ON`, so it probably works. The claim and the code just need to agree. - On the fast path, `p->dgpu_binds++` isn't gated on `!par`. Parallel segment workers increment it concurrently without a lock: a data race, and lost counts feed the 2x bind-volume check. - An abort is retried forever. With `RX_VK_DGPU=1` on an iGPU (`mirror_on()` false), or when a dGPU's mirror budget is full, the same heaviest pool starts a trial every window and aborts 132 frames later, logging each time. It might be worth requiring `mirror_on()` in `dgpu_on()`, or giving up on a pool after N aborts. - The skip list is keyed only on the guest pool start, and a verdict lasts forever. One 120-frame window in one scene decides it for good, even across a GPU swap, a driver update or a different resolution. It might be worth recording the device or driver in the file, or letting entries expire. - Nit: `RUN_ALL_ARGS = ["--cxx", "g++"]` hard-codes g++. Neighbouring tests use `"{cxx}"`, so a clang-only machine still runs them. **Questions for you** 1. Do you recommend turning this on by default, either on dGPUs or everywhere? Why or why not? Your own numbers show parity in lighter scenes and a win that grows with coherence load, so I'd like to hear where you think the line is. 2. If we do turn it on, which settings and thresholds should we use? The 120/12/120-frame trial windows, the 1 MB candidate floor, `RX_VK_DGPU_GAIN` and `RX_VK_DGPU_FLOOR`, and `RX_VK_DGPU_TRIALS`: should trials run at all by default, or is pure direct enough? And should the skip list ship enabled? 3. What testing do we need before flipping it, and what have you already tested? I'm thinking of NVIDIA (proprietary on Windows and Linux), AMD (RADV and the Windows driver, including small-BAR / non-ReBAR systems, where PCIe reads aren't nearly free), Intel Arc, and iGPUs / UMA, on both Linux and Windows. For scenes, a few heavy and light ones (Hyrule Field, Kokiri, Kakariko, a dungeon, menus and pause, save reload) at 1080p and 4K, capped and uncapped. For each: fps and frame-time percentiles, flicker/artifact screenshots (the #1795 failure mode), and correctness (draw parity, readbacks, no cyan after a pause as in #2154). So far I only see RX 9070 XT / RADV at 4K. Related open PRs in this area to coordinate with: #1795 (UB snapshot when mirrors are off), #2154 (keeps mirrors of pools the GPU writes), #1377 (mirror the pools the GPU reads most) and #2343 (GCN: mirrors off by default).

docs/development.md

"trials run on a serial replay only (RX_VK_PARALLEL keeps the stock all-mirrored policy ...)": as written, the code doesn't do this. Under RX_VK_PARALLEL with RX_VK_DGPU=1, pools are still marked refused at first bind, and trials still start (see the par_active comment). Either the code or this row needs to change.

"trials run on a serial replay only (RX_VK_PARALLEL keeps the stock all-mirrored policy ...)": as written, the code doesn't do this. Under `RX_VK_PARALLEL` with `RX_VK_DGPU=1`, pools are still marked refused at first bind, and trials still start (see the `par_active` comment). Either the code or this row needs to change.

gpu/vk/vk_memory.cpp

					
				@ -2666,0 +2791,4 @@

				// only a clear, repeatable win keeps the mirror (dgpu::mirror_worth). A candidate is not write-dominated: a pool

				// the GPU renders into lives best in guest memory (its writes land in the pool the CPU reads, and out of the

				// mirrors such pools cost only GPU write time, ~1 ms a frame, and gave nothing - Triforce's demotion measured),

				// while the diff around a mirror would buy the write-back's compare back. RX_VK_DGPU_TRIALS=0: none (pure direct,

dgpu_read is guarded by !par, but this increment isn't. Parallel segment workers take this lock-free path concurrently, so dgpu_binds is a plain non-atomic ++ from several threads: a data race, and lost counts feed the 2x bind-volume check. Guard it with !par too, or use a relaxed atomic.

`dgpu_read` is guarded by `!par`, but this increment isn't. Parallel segment workers take this lock-free path concurrently, so `dgpu_binds` is a plain non-atomic `++` from several threads: a data race, and lost counts feed the 2x bind-volume check. Guard it with `!par` too, or use a relaxed atomic.

gpu/vk/vk_memory.cpp

This is the key behaviour change. Uniform and other read buffers of every pool are now read in place, when the GPU executes. #1795 shows that on mirror-less GPUs the game rewrites constants right after the submit, and draws read the next frame's data when the GPU runs behind. The dGPU mirror, synced at submit, hides that today. RX_VK_DGPU=1 keeps mirror_on() true, so a fix keyed on mirrors being off (like #1795's) wouldn't apply here.

This is the key behaviour change. Uniform and other read buffers of every pool are now read in place, when the GPU executes. #1795 shows that on mirror-less GPUs the game rewrites constants right after the submit, and draws read the next frame's data when the GPU runs behind. The dGPU mirror, synced at submit, hides that today. `RX_VK_DGPU=1` keeps `mirror_on()` true, so a fix keyed on mirrors being off (like #1795's) wouldn't apply here.

gpu/vk/vk_memory.cpp

par_active is set only while the segments are recorded (vk_replay.cpp sets it true, then false after par_pool.done), so it's always false here in memory_end_submission. Trials do start under RX_VK_PARALLEL, which contradicts the docs row and the PR description. If parallel should really be excluded, check the configured thread count (the RX_VK_PARALLEL value) instead.

`par_active` is set only while the segments are recorded (vk_replay.cpp sets it true, then false after `par_pool.done`), so it's always false here in `memory_end_submission`. Trials do start under `RX_VK_PARALLEL`, which contradicts the docs row and the PR description. If parallel should really be excluded, check the configured thread count (the `RX_VK_PARALLEL` value) instead.

gpu/vk/vk_memory.cpp

gpu_time_total_ns() only covers r.cmd (gpu_time_begin/end), but mirror_sync/mirror_make record into upload_command_buffer(). The mirrored window therefore doesn't count its own sync copies, or any CPU-side capture/diff/write-back, and the verdict leans towards keeping the mirror. Since the 4K frame is CPU-bound by your own diagnosis, it may be better to decide on frame time or replay ms, or at least to time the upload buffer as well.

`gpu_time_total_ns()` only covers `r.cmd` (gpu_time_begin/end), but `mirror_sync`/`mirror_make` record into `upload_command_buffer()`. The mirrored window therefore doesn't count its own sync copies, or any CPU-side capture/diff/write-back, and the verdict leans towards keeping the mirror. Since the 4K frame is CPU-bound by your own diagnosis, it may be better to decide on frame time or replay ms, or at least to time the upload buffer as well.

gpu/vk/vk_memory.cpp

One 120-frame window decides this for good, keyed only by guest address. A later GPU or driver swap, a resolution change, or a trial run in a scene that happens to be light keeps the pool out of trials permanently, unless the user knows to delete the file. Could the file record the device/driver (and maybe the resolution), or could entries expire?

gpu/vk/vk_memory.cpp

An abort doesn't mark the pool, so the next window picks the same heaviest pool again. With RX_VK_DGPU=1 on an iGPU (mirror_on() false, so the mirror never comes on), or with the mirror budget full, this repeats every window forever: a 132-frame trial plus a log line each time. Requiring mirror_on() in dgpu_on(), or giving up on a pool after a couple of aborts, would avoid it.

An abort doesn't mark the pool, so the next window picks the same heaviest pool again. With `RX_VK_DGPU=1` on an iGPU (`mirror_on()` false, so the mirror never comes on), or with the mirror budget full, this repeats every window forever: a 132-frame trial plus a log line each time. Requiring `mirror_on()` in `dgpu_on()`, or giving up on a pool after a couple of aborts, would avoid it.

tests/test_dgpu_policy.py

					
				@ -0,0 +1,188 @@

				"""Compile and run the dGPU policy's rules (gpu/vk/dgpu_policy.h; no game/GPU required), and check that vk_memory.cpp

Nit: the neighbouring tests use RUN_ALL_ARGS = ["--cxx", "{cxx}"], so run_all picks the compiler it found (clang++ first). Hard-coding g++ fails on a machine that only has clang.

Nit: the neighbouring tests use `RUN_ALL_ARGS = ["--cxx", "{cxx}"]`, so run_all picks the compiler it found (clang++ first). Hard-coding `g++` fails on a machine that only has clang.

blessedplummer commented

2026-11-11 20:25:32 +00:00

(Migrated from github.com)

Thanks for the thorough review — this is exactly the pass the PR needed. All findings accepted; fixes are in flight, ordered by your priority:

#1795 in-place reads — agreed this is the one that matters: draw parity cannot catch it, and our pool-level gate protects CPU-read pools, not the CPU-rewrite-while-GPU-runs-behind class. Plan: extend #1795's fix keying from global mirror-off to the per-pool condition (mirror-less at the read), so refused pools get the same protection mirror-less GPUs get on main and trial-promoted pools keep it via their mirror. I'll report what the fix needs once it's traced against the actual code.
Verdict timing — agreed, GPU-only time biases toward mirrors precisely when the frame is CPU-bound; moving to a frame-relevant metric.
Parallel — agreed, the claims and the code must agree and the ungated dgpu_binds++ is a race; making them agree with no race.
Abort-forever — agreed on both halves (mirror_on() in the candidate gate; an abort cap).
Skip list — agreed on device/driver keying.
g++ nit — fixing.
Windows CI: that's main's own scanf deprecation, #2397 merged upstream 19:14 UTC after our rebase base — the rebase onto current main clears it.

I'll answer your three questions against the updated code rather than pre-answering, since Q1/Q2 depend on what the #1795 extension requires — but the short forms: we currently recommend opt-in (one GPU, one scene class measured; the matrix you list is the right gate for a default flip), and trials should stay on where they're allowed since they're the mechanism that rejects what doesn't pay. Full answers coming with the fixes.

Thanks for the thorough review — this is exactly the pass the PR needed. All findings accepted; fixes are in flight, ordered by your priority: - **#1795 in-place reads** — agreed this is the one that matters: draw parity cannot catch it, and our pool-level gate protects CPU-*read* pools, not the CPU-rewrite-while-GPU-runs-behind class. Plan: extend #1795's fix keying from global mirror-off to the per-pool condition (mirror-less at the read), so refused pools get the same protection mirror-less GPUs get on main and trial-promoted pools keep it via their mirror. I'll report what the fix needs once it's traced against the actual code. - **Verdict timing** — agreed, GPU-only time biases toward mirrors precisely when the frame is CPU-bound; moving to a frame-relevant metric. - **Parallel** — agreed, the claims and the code must agree and the ungated `dgpu_binds++` is a race; making them agree with no race. - **Abort-forever** — agreed on both halves (`mirror_on()` in the candidate gate; an abort cap). - **Skip list** — agreed on device/driver keying. - **g++ nit** — fixing. - Windows CI: that's main's own scanf deprecation, #2397 merged upstream 19:14 UTC after our rebase base — the rebase onto current main clears it. I'll answer your three questions against the updated code rather than pre-answering, since Q1/Q2 depend on what the #1795 extension requires — but the short forms: we currently recommend opt-in (one GPU, one scene class measured; the matrix you list is the right gate for a default flip), and trials should stay on where they're allowed since they're the mechanism that rejects what doesn't pay. Full answers coming with the fixes.

Joey-Magnus (Migrated from github.com) reviewed

2026-11-11 21:26:33 +00:00

Joey-Magnus (Migrated from github.com) left a comment

Opt-in, so nothing changes for anyone until RX_VK_DGPU is set. The red Windows part 2 job is main's old scanf/ASTC failure (#2397). Waiting on the fix push for the accepted review points: #1795 in-place reads, trial timing GPU work only, the dgpu_binds++ race, the abort retried forever, and skip-list keying. hookshot_mirror_skip.txt should go under cache/ if #2317 lands first.

Opt-in, so nothing changes for anyone until `RX_VK_DGPU` is set. The red Windows part 2 job is main's old `scanf`/ASTC failure (#2397). Waiting on the fix push for the accepted review points: #1795 in-place reads, trial timing GPU work only, the `dgpu_binds++` race, the abort retried forever, and skip-list keying. `hookshot_mirror_skip.txt` should go under `cache/` if #2317 lands first.

blessedplummer commented

2026-11-11 22:15:28 +00:00

(Migrated from github.com)

The fix push is up (head 64117673): reworked branch, one commit, on top of current main (30c2ebd3). Per-item against your review, then the three questions.

All six findings, fixed:

#1795 in-place reads (your critical one) — #1795's fix is now keyed per pool, not on global mirror-off: a binding that is a pool's guest memory during an executing recording (BufferRef::live) marks the read as in-place, and the UB snapshot engages for exactly those bindings (dgpu_ub_in_place()). Refused pools read uniforms as submitted; trial-won pools keep the in-place mirror binds, which are submit-time synced, so nothing changes for them. Unset is untouched. Live-verified on the 9070 XT: a boot run with the snapshot engaged did 1.26M UB stream copies, zero faults. Two honest open items below (event waits; snapshot cost).
Verdict timing — you were right that GPU-only time biases toward mirrors exactly when the frame is CPU-bound. Verdicts now decide on present-to-present frame time (the replay's own counter), not gpu_time_total_ns. The mirror's cost is CPU work + upload-buffer recording, invisible to GPU timestamps, and its PCIe benefit only shows frame-wise — a CPU-only metric would release mirrors that small-BAR devices need. WHY comment in place; 120/12/120 kept; settle still holds until the mirror is really on.
Parallel — code and docs now agree the simple way: the policy runs trial-less under RX_VK_PARALLEL (pure direct, documented), the fast-path counters are gated off parallel, and the never-true par_active guard is gone.
Abort-forever — the candidate gate now requires mirror_on() (iGPUs: no trials at all — direct is right for UMA, and on GCN after #2343 this also means the switch runs trial-less there), and a pool that aborts 3 times without a verdict sits out the session.
Skip list — the file now carries a device+driver header (AMD Radeon RX 9070 XT (RADV GFX1201), driver 109056099, written on the live run); a foreign-device or header-less file is ignored and its pools re-trialled rather than trusted. Placement: beside the game's own state files, moving under #2317's per-user cache dir if that lands first (per Joey-Magnus's note — one WHY comment at the path site, no speculative restructuring).
{cxx} — RUN_ALL_ARGS uses it now.

Rebase notes: main's #2343 (GCN mirror-less by default) composes unchanged — on GCN, mirror_on() is false so the trials never start; the switch runs pure direct plus the UB snapshot, i.e. their new default plus hazard protection. #2131's new test_wb_observation_lifetime compiles a slice of vk_memory.cpp alone and our trial functions sat inside the span — they moved just outside it; that check passes untouched. No mechanism duplication found with #2131: it measures how much copied data changed; our per-pool counters feed trial candidacy.

Your three questions:

Q1 (default-on?) — Opt-in for now, and #2343 actually strengthens that answer rather than complicating it: upstream has already accepted device-conditional mirror defaults (mirror-less by default for GCN, from measured RX 580 data). This PR is the per-pool, measured version of the same judgment — instead of deciding once per device class, a pool earns its mirror only when a trial proves it pays and loses it when one proves it doesn't. The trial supplies the measurement instead of the vendor family supplying the verdict. What keeps it opt-in is hardware breadth, not a known defect: our only rig is RX 9070 XT / RADV / Linux, and the remaining reason is exactly the matrix you listed. A default flip (dGPUs first) becomes defensible after that matrix runs clean — including artifact screenshots in a GPU-bound scene — and after the UB snapshot's cost has an A/B on our rig (it is new in this push; engagement and safety are proven, cost is not yet).

Q2 (settings and shipping defaults) — Ship what the switch means today: RX_VK_DGPU=1, trials ON by default (RX_VK_DGPU_TRIALS=0 exists for pure-direct A/Bs — trials are the mechanism that earns heavily-read pools their mirrors back on small-BAR, so they are the policy's point). Trial structure 120/12/120; candidate floor ≥1 MB of guest-memory reads per 600-submit window and not write-dominated (render targets stay direct); verdict keeps the mirror only when faster by BOTH 10% (RX_VK_DGPU_GAIN) and 0.25 ms/frame (RX_VK_DGPU_FLOOR), on frame time; skip list ships enabled (it is what makes convergence stick across sessions) with the device+driver header. Trials don't run under RX_VK_PARALLEL or where mirrors cannot come on (iGPUs/GCN).

Q3 (testing) — What exists: RX 9070 XT / RADV / Linux / 4K, warp-normalized adjacent A/B pairs in real 3D gameplay (Hyrule Field, Kokiri Forest), three pairs +8.5% to +28.6% (median +9.7%) with per-pair draw-count parity and the OFF side reproducing the issue's fingerprint (wb-diff ~3.1 GB/600 submits, >130k mirror-sync pages → ~0); GPU never saturated, so the win is CPU-side as diagnosed. This push adds the UB snapshot, frame-time verdicts, the parallel/iGPU/abort gates and the device-keyed skip list — seven game-free checks pass, suite 415/0 vs pristine 414/0 on the same base, and a boot-level smoke ran both modes fault-free. What's needed before any default flip — we agree with your matrix and can supply the AMD/RADV/Linux column: NVIDIA proprietary (your RTX 4060 Ti offer is the small-BAR case that most needs the trials to earn mirrors back), AMD Windows, Intel Arc, iGPU/UMA — and now GCN as its own row, where the policy composes with #2343's default (trials off; cheap to verify). Scenes: Kakariko, a dungeon, menus/pause, save reload, plus our two; 1080p and 4K, capped and uncapped (at a cap the trials will keep nothing — frame time cannot show a sub-cap win; expected, worth observing); frame-time percentiles; artifact screenshots in a GPU-bound scene (the #1795 failure mode, which draw parity cannot catch); correctness incl. the #2154 cyan case and readbacks.

Two open items we flag rather than paper over: #1795's event-waits commit is keyed globally (mirrors off), not per pool — under this policy event waits stay on on a dGPU, and we'd value your read on whether that needs the same per-pool treatment. And the UB snapshot's frame-time cost on our rig is unmeasured beyond the boot smoke; we'll run the A/B when the matrix work starts.

Thanks to @Joey-Magnus for the second read and the cache/ note.

The fix push is up (head `64117673`): reworked branch, one commit, on top of current main (30c2ebd3). Per-item against your review, then the three questions. **All six findings, fixed:** - **#1795 in-place reads (your critical one)** — #1795's fix is now keyed per pool, not on global mirror-off: a binding that *is* a pool's guest memory during an executing recording (`BufferRef::live`) marks the read as in-place, and the UB snapshot engages for exactly those bindings (`dgpu_ub_in_place()`). Refused pools read uniforms as submitted; trial-won pools keep the in-place mirror binds, which are submit-time synced, so nothing changes for them. Unset is untouched. Live-verified on the 9070 XT: a boot run with the snapshot engaged did 1.26M UB stream copies, zero faults. Two honest open items below (event waits; snapshot cost). - **Verdict timing** — you were right that GPU-only time biases toward mirrors exactly when the frame is CPU-bound. Verdicts now decide on **present-to-present frame time** (the replay's own counter), not `gpu_time_total_ns`. The mirror's cost is CPU work + upload-buffer recording, invisible to GPU timestamps, and its PCIe benefit only shows frame-wise — a CPU-only metric would release mirrors that small-BAR devices need. WHY comment in place; 120/12/120 kept; settle still holds until the mirror is really on. - **Parallel** — code and docs now agree the simple way: the policy runs trial-less under `RX_VK_PARALLEL` (pure direct, documented), the fast-path counters are gated off parallel, and the never-true `par_active` guard is gone. - **Abort-forever** — the candidate gate now requires `mirror_on()` (iGPUs: no trials at all — direct is right for UMA, and on GCN after #2343 this also means the switch runs trial-less there), and a pool that aborts 3 times without a verdict sits out the session. - **Skip list** — the file now carries a device+driver header (`AMD Radeon RX 9070 XT (RADV GFX1201), driver 109056099`, written on the live run); a foreign-device or header-less file is ignored and its pools re-trialled rather than trusted. Placement: beside the game's own state files, moving under #2317's per-user cache dir if that lands first (per Joey-Magnus's note — one WHY comment at the path site, no speculative restructuring). - **`{cxx}`** — `RUN_ALL_ARGS` uses it now. **Rebase notes:** main's #2343 (GCN mirror-less by default) composes unchanged — on GCN, `mirror_on()` is false so the trials never start; the switch runs pure direct plus the UB snapshot, i.e. their new default plus hazard protection. #2131's new `test_wb_observation_lifetime` compiles a slice of vk_memory.cpp alone and our trial functions sat inside the span — they moved just outside it; that check passes untouched. No mechanism duplication found with #2131: it measures how much copied data changed; our per-pool counters feed trial candidacy. **Your three questions:** **Q1 (default-on?)** — Opt-in for now, and #2343 actually strengthens that answer rather than complicating it: upstream has already accepted device-conditional mirror defaults (mirror-less by default for GCN, from measured RX 580 data). This PR is the per-pool, *measured* version of the same judgment — instead of deciding once per device class, a pool earns its mirror only when a trial proves it pays and loses it when one proves it doesn't. The trial supplies the measurement instead of the vendor family supplying the verdict. What keeps it opt-in is hardware breadth, not a known defect: our only rig is RX 9070 XT / RADV / Linux, and the remaining reason is exactly the matrix you listed. A default flip (dGPUs first) becomes defensible after that matrix runs clean — including artifact screenshots in a GPU-bound scene — and after the UB snapshot's cost has an A/B on our rig (it is new in this push; engagement and safety are proven, cost is not yet). **Q2 (settings and shipping defaults)** — Ship what the switch means today: `RX_VK_DGPU=1`, trials ON by default (`RX_VK_DGPU_TRIALS=0` exists for pure-direct A/Bs — trials are the mechanism that earns heavily-read pools their mirrors back on small-BAR, so they are the policy's point). Trial structure 120/12/120; candidate floor ≥1 MB of guest-memory reads per 600-submit window and not write-dominated (render targets stay direct); verdict keeps the mirror only when faster by BOTH 10% (`RX_VK_DGPU_GAIN`) and 0.25 ms/frame (`RX_VK_DGPU_FLOOR`), on frame time; skip list ships enabled (it is what makes convergence stick across sessions) with the device+driver header. Trials don't run under `RX_VK_PARALLEL` or where mirrors cannot come on (iGPUs/GCN). **Q3 (testing)** — What exists: RX 9070 XT / RADV / Linux / 4K, warp-normalized adjacent A/B pairs in real 3D gameplay (Hyrule Field, Kokiri Forest), three pairs +8.5% to +28.6% (median +9.7%) with per-pair draw-count parity and the OFF side reproducing the issue's fingerprint (wb-diff ~3.1 GB/600 submits, >130k mirror-sync pages → ~0); GPU never saturated, so the win is CPU-side as diagnosed. This push adds the UB snapshot, frame-time verdicts, the parallel/iGPU/abort gates and the device-keyed skip list — seven game-free checks pass, suite 415/0 vs pristine 414/0 on the same base, and a boot-level smoke ran both modes fault-free. What's needed before any default flip — we agree with your matrix and can supply the AMD/RADV/Linux column: NVIDIA proprietary (your RTX 4060 Ti offer is the small-BAR case that most needs the trials to earn mirrors back), AMD Windows, Intel Arc, iGPU/UMA — and now GCN as its own row, where the policy composes with #2343's default (trials off; cheap to verify). Scenes: Kakariko, a dungeon, menus/pause, save reload, plus our two; 1080p and 4K, capped and uncapped (at a cap the trials will keep nothing — frame time cannot show a sub-cap win; expected, worth observing); frame-time percentiles; artifact screenshots in a GPU-bound scene (the #1795 failure mode, which draw parity cannot catch); correctness incl. the #2154 cyan case and readbacks. **Two open items we flag rather than paper over:** #1795's event-waits commit is keyed globally (mirrors off), not per pool — under this policy event waits stay on on a dGPU, and we'd value your read on whether that needs the same per-pool treatment. And the UB snapshot's frame-time cost on our rig is unmeasured beyond the boot smoke; we'll run the A/B when the matrix work starts. Thanks to @Joey-Magnus for the second read and the cache/ note.

blessedplummer commented

2026-11-11 22:46:53 +00:00

(Migrated from github.com)

Re-measurement result on the fix push, disclosed rather than discovered by you: the +9.7% fps claim does not hold on the current code and is withdrawn from the PR body.

We re-ran the same warp-normalized Hyrule Field pairs (H/I/J, same recipe as E/F/G) on the reworked branch. Median is currently -1.9% (47.5 -> 46.6), ON losing all three pairs. The mechanisms are identified from the run logs, not guessed:

Our review-round hardening misfires on success. The bind-drift void check (added per the accepted review point on scene changes) voids exactly the trials where the mirror works — a working mirror collapses the pool's guest-read volume ~70x (one pool: 12,420 -> 180 reads between windows), which the check reads as scene-change drift. So only pools the mirror doesn't help ever reach a verdict (0 keeps, 1 release across the run), 42-44 trials void per run, and the churn never converges. Fix in flight: compare the bound-pool set between windows (a mirror changes served volume, not which pools bind) and cap voids like aborts.
The UB snapshot's cost is now measured in gameplay: ~+14M stream copies per run vs stock, submit +2 ms (recording), GPU +1.2-2.1 ms (executing). Caveat: with the livelock fixed, the heavily-read pools should earn mirrors back (they were precisely the voided ones), and mirrored pools use submit-synced binds — so part of this term may self-resolve; the re-run will say.

What held in the re-measurement: mirror-sync dirty pages ~0 under the switch (131k+ stock — the issue's fingerprint), draw parity 3/3, unset = stock exactly, zero faults. The correctness work from your review stands; it's the performance claim that isn't currently true.

We'll post the fix and the re-run numbers here when they land. The verdict metric change (frame time, per your point) is not implicated — it correctly released the one pool that reached a verdict (21.42 vs 22.08 ms) — the failure is the void check upstream of it.

Re-measurement result on the fix push, disclosed rather than discovered by you: **the +9.7% fps claim does not hold on the current code and is withdrawn from the PR body.** We re-ran the same warp-normalized Hyrule Field pairs (H/I/J, same recipe as E/F/G) on the reworked branch. Median is currently **-1.9%** (47.5 -> 46.6), ON losing all three pairs. The mechanisms are identified from the run logs, not guessed: 1. **Our review-round hardening misfires on success.** The bind-drift void check (added per the accepted review point on scene changes) voids exactly the trials where the mirror *works* — a working mirror collapses the pool's guest-read volume ~70x (one pool: 12,420 -> 180 reads between windows), which the check reads as scene-change drift. So only pools the mirror doesn't help ever reach a verdict (0 keeps, 1 release across the run), 42-44 trials void per run, and the churn never converges. Fix in flight: compare the bound-pool *set* between windows (a mirror changes served volume, not which pools bind) and cap voids like aborts. 2. **The UB snapshot's cost is now measured in gameplay**: ~+14M stream copies per run vs stock, submit +2 ms (recording), GPU +1.2-2.1 ms (executing). Caveat: with the livelock fixed, the heavily-read pools should earn mirrors back (they were precisely the voided ones), and mirrored pools use submit-synced binds — so part of this term may self-resolve; the re-run will say. What held in the re-measurement: mirror-sync dirty pages ~0 under the switch (131k+ stock — the issue's fingerprint), draw parity 3/3, unset = stock exactly, zero faults. The correctness work from your review stands; it's the performance claim that isn't currently true. We'll post the fix and the re-run numbers here when they land. The verdict metric change (frame time, per your point) is not implicated — it correctly released the one pool that reached a verdict (21.42 vs 22.08 ms) — the failure is the void check upstream of it.

hckrman101 commented

2026-11-11 22:55:49 +00:00

(Migrated from github.com)

This now conflicts with main in gpu/vk/vk_replay.cpp. Could you merge main into the branch (or rebase) when you get a chance? I'll take another look once it's up to date.

This now conflicts with main in `gpu/vk/vk_replay.cpp`. Could you merge main into the branch (or rebase) when you get a chance? I'll take another look once it's up to date.

cashcon57 commented

2026-11-11 23:16:38 +00:00

(Migrated from github.com)

An NVIDIA data point, in case it helps scope the policy: on this card RX_VK_DGPU=1 went the other way.

RTX 3080 Ti, driver 615.78.08, Ryzen 7 5800X3D, Linux (CachyOS), headless gamescope at 3840x2160, Hyrule Field, 4K output with DLSS Quality (Resolution 67%), RX_VK_RECORDER=thread. Build: main 49f75dc8 + this PR (plus #2314), the same binary with and without the switch, 2 rounds alternating:

run	fps	GPU time	replay memory sync	frames over 33 ms / min
off 1	45.8	13.3 ms	4.35 ms	8.3
on 1	33.9	17.3 ms	6.00 ms	345.7
off 2	46.6	13.3 ms	4.40 ms	5.7
on 2	39.3	17.2 ms	5.84 ms	184.3

GPU time rises by about 4 ms a frame with the pools bound in guest memory, presumably the GPU reading host memory over PCIe where the mirrors had kept it in VRAM, and the memory sync didn't drop either. No errors in any run. So on NVIDIA it may want to stay off, or be gated by vendor, if the RADV gain doesn't carry over. Happy to run another configuration if useful.

An NVIDIA data point, in case it helps scope the policy: on this card `RX_VK_DGPU=1` went the other way. RTX 3080 Ti, driver 615.78.08, Ryzen 7 5800X3D, Linux (CachyOS), headless gamescope at 3840x2160, Hyrule Field, 4K output with DLSS Quality (Resolution 67%), `RX_VK_RECORDER=thread`. Build: main 49f75dc8 + this PR (plus #2314), the same binary with and without the switch, 2 rounds alternating: | run | fps | GPU time | replay memory sync | frames over 33 ms / min | |---|---|---|---|---| | off 1 | 45.8 | 13.3 ms | 4.35 ms | 8.3 | | on 1 | 33.9 | 17.3 ms | 6.00 ms | 345.7 | | off 2 | 46.6 | 13.3 ms | 4.40 ms | 5.7 | | on 2 | 39.3 | 17.2 ms | 5.84 ms | 184.3 | GPU time rises by about 4 ms a frame with the pools bound in guest memory, presumably the GPU reading host memory over PCIe where the mirrors had kept it in VRAM, and the memory sync didn't drop either. No errors in any run. So on NVIDIA it may want to stay off, or be gated by vendor, if the RADV gain doesn't carry over. Happy to run another configuration if useful.

blessedplummer commented

2026-11-11 23:58:59 +00:00

(Migrated from github.com)

Rebased now — head 41dd125d, on top of main at fce53453, MERGEABLE. The conflict you flagged was your #2325 presenter-thread extraction: the block the PR's present-to-present tick lived in became present_finish(), so the tick moved into it. Verified with WHY comments in-code: present_finish is the one function every made present passes exactly once in either threading mode (the game thread's path and the presenter thread's; the hand-off branch returns before it, so no double-count), the mode is run-static so the tick's statics only ever touch the presenting thread, and a headless twin was added beside frame_log_present's offscreen call so headless runs stay measurable, symmetric with #2354.

Two notes on the current CI run:

The three red Python checks (test_env_on) are main's own, not this PR: upstream's new HOS_FPS_LOG getenv at host/oot_high_fps.c:15 reads set-means-on ("set to 0 they are on"), which the #2042 env_on rule and the PR's own test_env_on reject. It fails identically on pristine main at this tip (pristine: 436 passed / 1 failed; this branch: 437 / 1 — the delta is the PR's one new test). Nothing for this branch to fix, but flagging so it isn't mistaken for PR breakage.
Separately, per my comment above: the +9.7% fps claim is withdrawn pending re-measurement — the reworked branch currently measures slightly negative in the warp pairs (the review-round hardening's void check was voiding successful trials; fix landed, pairs re-running now). The correctness results (sync ~0, draw parity, unset=stock) held throughout.

Also — a second NVIDIA datapoint just arrived below from @cashcon57 (RTX 3080 Ti), which will feed the hardware matrix question from your review.

Rebased now — head `41dd125d`, on top of main at fce53453, `MERGEABLE`. The conflict you flagged was your #2325 presenter-thread extraction: the block the PR's present-to-present tick lived in became `present_finish()`, so the tick moved into it. Verified with WHY comments in-code: `present_finish` is the one function every *made* present passes exactly once in either threading mode (the game thread's path and the presenter thread's; the hand-off branch returns before it, so no double-count), the mode is run-static so the tick's statics only ever touch the presenting thread, and a headless twin was added beside `frame_log_present`'s offscreen call so headless runs stay measurable, symmetric with #2354. Two notes on the current CI run: - The three red Python checks (`test_env_on`) are **main's own**, not this PR: upstream's new `HOS_FPS_LOG` getenv at `host/oot_high_fps.c:15` reads set-means-on ("set to 0 they are on"), which the #2042 `env_on` rule and the PR's own `test_env_on` reject. It fails identically on pristine main at this tip (pristine: 436 passed / 1 failed; this branch: 437 / 1 — the delta is the PR's one new test). Nothing for this branch to fix, but flagging so it isn't mistaken for PR breakage. - Separately, per my comment above: the +9.7% fps claim is withdrawn pending re-measurement — the reworked branch currently measures slightly negative in the warp pairs (the review-round hardening's void check was voiding successful trials; fix landed, pairs re-running now). The correctness results (sync ~0, draw parity, unset=stock) held throughout. Also — a second NVIDIA datapoint just arrived below from @cashcon57 (RTX 3080 Ti), which will feed the hardware matrix question from your review.

blessedplummer commented

2026-11-11 23:59:00 +00:00

(Migrated from github.com)

Thanks — this is exactly the datapoint the hardware matrix needed, and the first external one. Before drawing conclusions from it, one crucial question: which head of this PR did you build? (you mention main 49f75dc8 + the PR + #2314 — the PR has been reworked substantially since its first push).

The reason it matters: between the fix push and last night we found and fixed a defect that would reproduce your numbers almost exactly. The review round added a bind-drift "void" check to invalidate trials that straddle a scene change — and it misfired on success: a working mirror collapses a pool's guest-read volume ~70x, which the check read as drift, so it voided 42-44 trials per run (mirrors repeatedly made, uploaded, voided, freed — which would show up as memory sync not dropping, even rising), only pools the mirror didn't help ever reached a verdict, and with zero mirrors kept every pool stayed bound in guest memory — where the #1795 uniform-buffer snapshot (a correctness fix, ~+14M stream copies per run, GPU +1.2-2.1 ms on RADV) applies to everything. Your +4 ms GPU time and un-dropped sync are consistent with that state.

The fix is landed on the current head (the drift rule now compares which pools bind between windows — a mirror changes served volume, not the pool set — plus a cap so churn cannot repeat), and the warp pairs are being re-run on RADV right now to re-establish the honest number.

So, two asks if you're willing:

If you can, re-run your same recipe on the current head (41dd125d's tree — happy to point you at the exact commit) — on the fixed code, your two heavily-read pools should either earn mirrors back (trials decide on frame time, which is what your table measures) or be correctly refused with the churn gone.
One extra run with RX_VK_DGPU_TRIALS=0 (pure direct, no trials at all) would isolate the snapshot cost from the trial machinery — if GPU time is still +4 ms there on NVIDIA, that's the real small-BAR PCIe story: guest-memory reads are not nearly free the way they are on this AMD's large BAR, and the trials are what should be earning those pools their mirrors back.

Your core observation stands regardless: if guest-memory binds cost GPU time on NVIDIA proprietary that they don't cost on large-BAR AMD, then the policy's value is conditional on hardware — which is exactly what the opt-in switch and the trial mechanism are for (a pool whose mirror wins on frame time keeps it; one that loses doesn't), and it's a strong argument for why the hardware matrix in the review should gate any default-on flip. Vendor-conditional behavior may well be the honest end state, as you suggest — but the fixed trials, not a static vendor gate, are the version of that we can actually measure.

Thanks — this is exactly the datapoint the hardware matrix needed, and the first external one. Before drawing conclusions from it, one crucial question: **which head of this PR did you build?** (you mention main `49f75dc8` + the PR + #2314 — the PR has been reworked substantially since its first push). The reason it matters: between the fix push and last night we found and fixed a defect that would reproduce your numbers almost exactly. The review round added a bind-drift "void" check to invalidate trials that straddle a scene change — and it misfired on success: a working mirror collapses a pool's guest-read volume ~70x, which the check read as drift, so it voided 42-44 trials *per run* (mirrors repeatedly made, uploaded, voided, freed — which would show up as *memory sync not dropping, even rising*), only pools the mirror didn't help ever reached a verdict, and with zero mirrors kept every pool stayed bound in guest memory — where the #1795 uniform-buffer snapshot (a correctness fix, ~+14M stream copies per run, GPU +1.2-2.1 ms on RADV) applies to everything. Your +4 ms GPU time and un-dropped sync are consistent with that state. The fix is landed on the current head (the drift rule now compares *which pools bind* between windows — a mirror changes served volume, not the pool set — plus a cap so churn cannot repeat), and the warp pairs are being re-run on RADV right now to re-establish the honest number. So, two asks if you're willing: 1. If you can, re-run your same recipe on the current head (`41dd125d`'s tree — happy to point you at the exact commit) — on the fixed code, your two heavily-read pools should either earn mirrors back (trials decide on frame time, which is what your table measures) or be correctly refused with the churn gone. 2. One extra run with `RX_VK_DGPU_TRIALS=0` (pure direct, no trials at all) would isolate the snapshot cost from the trial machinery — if GPU time is still +4 ms there on NVIDIA, that's the real small-BAR PCIe story: guest-memory reads are not nearly free the way they are on this AMD's large BAR, and the trials are what should be earning those pools their mirrors back. Your core observation stands regardless: if guest-memory binds cost GPU time on NVIDIA proprietary that they don't cost on large-BAR AMD, then the policy's value is *conditional on hardware* — which is exactly what the opt-in switch and the trial mechanism are for (a pool whose mirror wins on frame time keeps it; one that loses doesn't), and it's a strong argument for why the hardware matrix in the review should gate any default-on flip. Vendor-conditional behavior may well be the honest end state, as you suggest — but the fixed trials, not a static vendor gate, are the version of that we can actually measure.

blessedplummer commented

2026-11-12 00:03:34 +00:00

(Migrated from github.com)

One more variable worth recording for this datapoint (and the matrix generally): the PCIe link itself.

This rig's 9070 XT runs Gen 5 x16 (32 GT/s). Your 3080 Ti on the 5800X3D platform is AM4 — Gen 4 x16 at best (16 GT/s, half the bandwidth), and Gen 3 (8 GT/s, a quarter) if the board negotiates down. The policy's whole trade is "more GPU reads served over PCIe" vs "no mirror coherence on the CPU" — so the read side's cost scales directly with link generation and width, and a 2-4x link difference is exactly the kind of thing that could flip a GPU-time delta between vendors independent of driver behavior or BAR size.

If you re-run, could you record the link state alongside the fps numbers? Either is fine:

nvidia-smi --query-gpu=pcieLinkGenCurrent,pcieLinkWidthCurrent --format=csv
lspci -vv -s <gpu> | grep -E "LnkSta|LnkCap"

Also worth checking whether Resizable BAR is actually enabled for the card (nvidia-smi -q | grep -i bar or the BIOS setting) — large BAR is what makes guest-memory reads tolerable, and a 3080 Ti with ReBAR off would explain a lot of a +4 ms GPU delta by itself.

We'll record our own link state (Gen 5 x16) in the matrix baseline so cross-rig comparisons carry it. The honest framing either way: the trials measure frame time on your hardware, so a policy that correctly keeps mirrors where PCIe reads are expensive and refuses them where they're free should degrade gracefully across link generations — the re-run on the fixed head will show whether it actually does.

One more variable worth recording for this datapoint (and the matrix generally): the PCIe link itself. This rig's 9070 XT runs **Gen 5 x16** (32 GT/s). Your 3080 Ti on the 5800X3D platform is AM4 — Gen 4 x16 at best (16 GT/s, half the bandwidth), and Gen 3 (8 GT/s, a quarter) if the board negotiates down. The policy's whole trade is "more GPU reads served over PCIe" vs "no mirror coherence on the CPU" — so the read side's cost scales directly with link generation and width, and a 2-4x link difference is exactly the kind of thing that could flip a GPU-time delta between vendors independent of driver behavior or BAR size. If you re-run, could you record the link state alongside the fps numbers? Either is fine: ``` nvidia-smi --query-gpu=pcieLinkGenCurrent,pcieLinkWidthCurrent --format=csv lspci -vv -s <gpu> | grep -E "LnkSta|LnkCap" ``` Also worth checking whether Resizable BAR is actually enabled for the card (`nvidia-smi -q | grep -i bar` or the BIOS setting) — large BAR is what makes guest-memory reads tolerable, and a 3080 Ti with ReBAR off would explain a lot of a +4 ms GPU delta by itself. We'll record our own link state (Gen 5 x16) in the matrix baseline so cross-rig comparisons carry it. The honest framing either way: the trials measure frame time on *your* hardware, so a policy that correctly keeps mirrors where PCIe reads are expensive and refuses them where they're free should degrade gracefully across link generations — the re-run on the fixed head will show whether it actually does.

blessedplummer commented

2026-11-12 00:18:09 +00:00

(Migrated from github.com)

Re-run numbers, as promised. Verdict: still slightly negative with trials running (median -1.5%, pairs K/L/M), parity with them off (+0.9%, RX_VK_DGPU_TRIALS=0) — but the run isolates everything precisely, and one finding changes the PR's context:

The counting half of the fix is proven. The H/I/J failure signature (bind volume collapsing 70x on success, read as drift) is gone — every void now shows before≈after binds (23,814→24,009 on the repeatedly-voided pool). Voids dropped 42-44 → 15/run, all post-warp, none at title.

The scene-set rule is now the one blocker. It voids every gameplay trial in a streaming scene: each void cites "1 pool left the scene, 2 entered" — streaming pools legitimately cross the 64-bind anchor floor every window, so the set always moves and no verdict can ever be reached (0 keeps, 0 releases, 0 verdicts across the run; the 23-attempt cycle repeats identically). The floor was calibrated at the title screen and didn't survive contact with streaming gameplay — our calibration mistake, found exactly as designed (the cap bounded the churn; the logs named the cause). Also observed: pool recycling defeats id-keyed set comparison (the 0x109a range rebounded under a new id, 0x7fd3, with stable binds). Fix in flight: tolerate straddlers — fraction-threshold on the anchored set and overlap-aware matching, not any-movement-voids.

The snapshot cost question is answered, and the #1795 fix is vindicated: the TRIALS=0 discriminator pair measured parity (+0.9%) with the snapshot at full volume (16.4M as-submitted copies vs 2.2M stock) — its frame-level cost in this scene is noise. What costs is the trial engine's churn (mirror-carrying windows: make/upload/free ≈ 1 fps), and that is bounded and fixable.

The strategic finding: the win the +9.7%-era pairs measured has largely dried up on main. Those pairs won by freeing memory-sync CPU time (OFF side was 3.24 ms/frame); after two rebases, current main's OFF-side memory sync is already 0.46-0.50 ms — upstream's own work (split-submission fences #2419, presenter thread #2325, the GCN work) absorbed most of that cost. What remains under this PR is the part main does not do: earned mirrors for heavily-read pools (the heavyweight here reads 6.7 GB/window — that is real PCIe traffic even at Gen5 x16) plus the #1795 per-pool protection. The remaining upside is unmeasurable until the set rule tolerates straddlers; the next re-run measures exactly that.

Coherence held everywhere: sync pages ~0 under the switch (133k+ stock), wb confined to trial windows, draw parity ≤1.9%, unset=stock exact, 0 faults in 9/9 runs including the discriminator.

Re-run numbers, as promised. Verdict: **still slightly negative with trials running (median -1.5%, pairs K/L/M), parity with them off (+0.9%, `RX_VK_DGPU_TRIALS=0`)** — but the run isolates everything precisely, and one finding changes the PR's context: **The counting half of the fix is proven.** The H/I/J failure signature (bind volume collapsing 70x on success, read as drift) is gone — every void now shows before≈after binds (23,814→24,009 on the repeatedly-voided pool). Voids dropped 42-44 → 15/run, all post-warp, none at title. **The scene-set rule is now the one blocker.** It voids every gameplay trial in a streaming scene: each void cites "1 pool left the scene, 2 entered" — streaming pools legitimately cross the 64-bind anchor floor every window, so the set *always* moves and no verdict can ever be reached (0 keeps, 0 releases, 0 verdicts across the run; the 23-attempt cycle repeats identically). The floor was calibrated at the title screen and didn't survive contact with streaming gameplay — our calibration mistake, found exactly as designed (the cap bounded the churn; the logs named the cause). Also observed: pool recycling defeats id-keyed set comparison (the 0x109a range rebounded under a new id, 0x7fd3, with stable binds). Fix in flight: tolerate straddlers — fraction-threshold on the anchored set and overlap-aware matching, not any-movement-voids. **The snapshot cost question is answered, and the #1795 fix is vindicated**: the `TRIALS=0` discriminator pair measured **parity (+0.9%) with the snapshot at full volume** (16.4M as-submitted copies vs 2.2M stock) — its frame-level cost in this scene is noise. What costs is the trial engine's churn (mirror-carrying windows: make/upload/free ≈ 1 fps), and that is bounded and fixable. **The strategic finding**: the win the +9.7%-era pairs measured has largely **dried up on main**. Those pairs won by freeing memory-sync CPU time (OFF side was 3.24 ms/frame); after two rebases, current main's OFF-side memory sync is already 0.46-0.50 ms — upstream's own work (split-submission fences #2419, presenter thread #2325, the GCN work) absorbed most of that cost. What remains under this PR is the part main does not do: **earned mirrors for heavily-read pools** (the heavyweight here reads 6.7 GB/window — that is real PCIe traffic even at Gen5 x16) plus the #1795 per-pool protection. The remaining upside is unmeasurable until the set rule tolerates straddlers; the next re-run measures exactly that. Coherence held everywhere: sync pages ~0 under the switch (133k+ stock), wb confined to trial windows, draw parity ≤1.9%, unset=stock exact, 0 faults in 9/9 runs including the discriminator.

blessedplummer commented

2026-11-12 01:07:06 +00:00

(Migrated from github.com)

P/Q/R numbers are in, and this round closes the measurement question with a clean split: the machinery is now proven end-to-end, and the positive fps claim is measured to be not defensible on this hardware — with the reason quantified rather than assumed.

What works (first time in the campaign's history):

The first keep ever earned: pool 0x109a000000 (160 MB) measured 33.94 → 21.80 ms in the trial's windows (−35.7%), 36.41 → 22.31, 24.68 → 22.03 across the three pairs — kept every session, and nearly free to maintain (0 dirty pages synced per window for 160 MB of VRAM). This is the same pool that was recycled across H/I/J and voided 16–17×/run; the anchor tolerance and range matching handled it.
Releases converge into the skip list: 0 → 6 → 8 → 10 entries across the session, with the device+driver header, exactly as designed. Churn attempts 23 → 3 per run. Voids: zero — the "1 pool left, 2 entered" false-void signature is gone entirely, no caps fired, all streaming transitions tolerated.
The headline pool released, correctly, with numbers: 0x1186000000 (the 144 MB pool reading ~7 GB/window) trialed at 21.70 → 21.41 — a 0.29 ms (1.3%) gain, below the 10% AND-bar, released. ~7 GB/window of guest reads costs 0.29 ms on this rig: that is the ReBAR/Gen5 PCIe story measured directly.

Why the pairs still lose: the mirror beats the policy's own direct mode by 10.7–39.7% in the trial windows — the trial metric is truthful and the keep is legitimate. But stock runs the same scene at 20.4–21.0 ms: on this ReBAR/Gen5 x16 rig, guest-memory GPU reads are nearly free (the 0.29 ms/7 GB number is the proof), the #1795 UB snapshot converts some reads into stream copies (its cost: parity, per the TRIALS=0 discriminator last round), and a mirror can only climb back to near-stock, never past it. Best policy state: −2.7% median (P −4.1 / Q −1.3 / R −2.7), steady-state confirmed post-keep, not a trial-window artifact.

So the fork, put to the maintainers rather than assumed:

(a) Keep the PR open as opt-in machinery with no performance claim on this hardware — the value case is now empirically confined to hardware where guest reads are not nearly free: small-BAR / Gen4-class / proprietary-driver rigs (the @cashcon57 3080 Ti datapoint is exactly that class; @hckrman101's matrix predicted it). The trials would do the work there that they demonstrably do here — just with a different verdict landscape.

(b) Withdraw, and re-file the #1795 per-pool UB snapshot standalone — it is correctness-only, measured at parity cost, keyed per pool (main's keying is global mirror-off, which leaves mixed states unprotected), fully tested, and lands independent of this PR's fate.

We hold both paths ready and will follow the maintainers' preference; either way, no more GPU-side churn from this branch until the direction is set. The correctness results held through every round: sync pages ~0 under the switch (131k+ stock), wb confined to mirror windows, draw parity, unset=stock exact, 0 faults in 7/7.

For what it's worth: the review's hardware matrix framing turned out to be the precise predictor of this outcome — the rig where the win was measured turned out to be the rig where the win can't exist, and the numbers now say so explicitly instead of by omission.

P/Q/R numbers are in, and this round closes the measurement question with a clean split: **the machinery is now proven end-to-end, and the positive fps claim is measured to be not defensible on this hardware — with the reason quantified rather than assumed.** **What works (first time in the campaign's history):** - **The first keep ever earned**: pool `0x109a000000` (160 MB) measured 33.94 → 21.80 ms in the trial's windows (−35.7%), 36.41 → 22.31, 24.68 → 22.03 across the three pairs — kept every session, and nearly free to maintain (0 dirty pages synced per window for 160 MB of VRAM). This is the same pool that was recycled across H/I/J and voided 16–17×/run; the anchor tolerance and range matching handled it. - **Releases converge into the skip list**: 0 → 6 → 8 → 10 entries across the session, with the device+driver header, exactly as designed. Churn attempts 23 → 3 per run. Voids: **zero** — the "1 pool left, 2 entered" false-void signature is gone entirely, no caps fired, all streaming transitions tolerated. - **The headline pool released, correctly, with numbers**: `0x1186000000` (the 144 MB pool reading ~7 GB/window) trialed at 21.70 → 21.41 — a 0.29 ms (1.3%) gain, below the 10% AND-bar, released. ~7 GB/window of guest reads costs 0.29 ms on this rig: that is the ReBAR/Gen5 PCIe story measured directly. **Why the pairs still lose**: the mirror beats the *policy's own direct mode* by 10.7–39.7% in the trial windows — the trial metric is truthful and the keep is legitimate. But stock runs the same scene at 20.4–21.0 ms: on this ReBAR/Gen5 x16 rig, guest-memory GPU reads are nearly free (the 0.29 ms/7 GB number is the proof), the #1795 UB snapshot converts some reads into stream copies (its cost: parity, per the `TRIALS=0` discriminator last round), and a mirror can only climb back to near-stock, never past it. Best policy state: **−2.7% median** (P −4.1 / Q −1.3 / R −2.7), steady-state confirmed post-keep, not a trial-window artifact. **So the fork, put to the maintainers rather than assumed:** (a) **Keep the PR open as opt-in machinery with no performance claim on this hardware** — the value case is now empirically confined to hardware where guest reads are *not* nearly free: small-BAR / Gen4-class / proprietary-driver rigs (the @cashcon57 3080 Ti datapoint is exactly that class; @hckrman101's matrix predicted it). The trials would do the work there that they demonstrably do here — just with a different verdict landscape. (b) **Withdraw, and re-file the #1795 per-pool UB snapshot standalone** — it is correctness-only, measured at parity cost, keyed per pool (main's keying is global mirror-off, which leaves mixed states unprotected), fully tested, and lands independent of this PR's fate. We hold both paths ready and will follow the maintainers' preference; either way, no more GPU-side churn from this branch until the direction is set. The correctness results held through every round: sync pages ~0 under the switch (131k+ stock), wb confined to mirror windows, draw parity, unset=stock exact, 0 faults in 7/7. For what it's worth: the review's hardware matrix framing turned out to be the precise predictor of this outcome — the rig where the win was measured turned out to be the rig where the win can't exist, and the numbers now say so explicitly instead of by omission.

blessedplummer commented

2026-11-12 01:19:04 +00:00

(Migrated from github.com)

Hardware identification, to remove an ambiguity several messages back could leave: every measurement we have posted in this PR — pairs E through R, the RX_VK_DGPU_TRIALS=0 discriminator, the coherence counters — ran on one rig: RX 9070 XT (Navi 48), ASRock Steel Legend, RADV/Mesa on Linux, PCIe Gen5 x16 with ReBAR enabled. The RTX 3080 Ti (driver 615.78.08, CachyOS) numbers are @cashcon57's separate rig, measured independently — the two are never mixed in any row above. Where our comments say "this rig" or "this hardware", they mean the 9070 XT; where the 3080 Ti is meant, it is named.

Hardware identification, to remove an ambiguity several messages back could leave: **every measurement we have posted in this PR — pairs E through R, the `RX_VK_DGPU_TRIALS=0` discriminator, the coherence counters — ran on one rig: RX 9070 XT (Navi 48), ASRock Steel Legend, RADV/Mesa on Linux, PCIe Gen5 x16 with ReBAR enabled.** The RTX 3080 Ti (driver 615.78.08, CachyOS) numbers are @cashcon57's separate rig, measured independently — the two are never mixed in any row above. Where our comments say "this rig" or "this hardware", they mean the 9070 XT; where the 3080 Ti is meant, it is named.

blessedplummer commented

2026-11-12 01:49:54 +00:00

(Migrated from github.com)

Following up on the fork question above, one piece is now independent of this PR's fate: the #1795 per-pool uniform-buffer snapshot has been extracted into its own PR — #2609 (based on current main, one commit, 4 files +209/−5).

What it is, in one line: the snapshot condition re-keyed from global ("no mirrors on the device") to per binding ("this bind is the pool's guest memory and that pool is mirror-less"), so the mirrored device's own mirror-less states are covered — the budget's refusals (pools > 1 GB, budget-full), the queued/deferred-make windows (RX_VK_MIRROR_DEFER, parallel segments, written-this-submission, failed-make retry) — which the global keying left exposed. Global-off (iGPU / RX_VK_MIRROR=0 / GCN default) is the degenerate case and behaves byte-identically to the keying #1795 proposed (measured: 1.28M copies on the 9070 XT with RX_VK_MIRROR=0). One documented behavior is superseded and stated plainly in the body: RX_VK_UB_SNAPSHOT=0 now means the per-binding rule rather than fully-off.

Cost, measured not inferred: parity in gameplay (+0.9%, inside noise) via the RX_VK_DGPU_TRIALS=0 discriminator above, while serving ~14M copies/run. Provenance and the hazard analysis trace back to this thread; the PR body links here.

No policy machinery travels with it — no trials, no skip list, no RX_VK_DGPU gating. This PR's fork question stays with the maintainers as posed; #2609 simply lets the correctness piece land whenever it's ready, whichever way (a) or (b) goes.

Following up on the fork question above, one piece is now independent of this PR's fate: **the #1795 per-pool uniform-buffer snapshot has been extracted into its own PR — #2609** (based on current main, one commit, 4 files +209/−5). What it is, in one line: the snapshot condition re-keyed from *global* ("no mirrors on the device") to **per binding** ("this bind is the pool's guest memory and that pool is mirror-less"), so the mirrored device's own mirror-less states are covered — the budget's refusals (pools > 1 GB, budget-full), the queued/deferred-make windows (`RX_VK_MIRROR_DEFER`, parallel segments, written-this-submission, failed-make retry) — which the global keying left exposed. Global-off (iGPU / `RX_VK_MIRROR=0` / GCN default) is the degenerate case and behaves byte-identically to the keying #1795 proposed (measured: 1.28M copies on the 9070 XT with `RX_VK_MIRROR=0`). One documented behavior is superseded and stated plainly in the body: `RX_VK_UB_SNAPSHOT=0` now means the per-binding rule rather than fully-off. Cost, measured not inferred: parity in gameplay (+0.9%, inside noise) via the `RX_VK_DGPU_TRIALS=0` discriminator above, while serving ~14M copies/run. Provenance and the hazard analysis trace back to this thread; the PR body links here. No policy machinery travels with it — no trials, no skip list, no `RX_VK_DGPU` gating. This PR's fork question stays with the maintainers as posed; #2609 simply lets the correctness piece land whenever it's ready, whichever way (a) or (b) goes.

github-sync referenced this pull request from a commit

2026-11-12 06:19:00 +00:00

fix(vk): capture switches follow the on/off rule (#2651)

github-sync referenced this pull request

2026-11-12 06:23:34 +00:00

fix(vk): count BC textures by blocks in the texture budget #2624

github-sync referenced this pull request

2026-11-12 11:36:25 +00:00

perf(vk): RX_VK_WB_MAX_KB writes large GPU-written bindings back only where the CPU reads or writes them (opt-in) #2731

github-sync commented

2026-11-12 14:42:25 +00:00

@Joey-Magnus reviewed on GitHub · 2026-11-12 14:32 UTC · original

The review points are addressed on 71add749: dgpu_binds is counted only with !par on the lock-free path (gpu/vk/vk_memory.cpp:2872, :2881), a pool gives up after three aborts, and the skip list records device and driver. But it now conflicts with main in vk_memory.cpp, vk_draw.cpp, docs/development.md and tests/direct_buffer_writes_test.cpp.

More to the point, your P/Q/R runs answer the question: on the RX 9070 XT the best policy state is 2.7% slower than main, whose memory sync is already about 0.5 ms a frame, and cashcon57's RTX 3080 Ti went from 46 to 34-39 fps with 4 ms more GPU time. No measured hardware comes out ahead; the small-BAR case is a guess. The correctness piece, the snapshot keyed on the binding's pool, is now #2609, the part worth landing.

I'd close this in favour of #2609 (your option b) and reopen if someone measures a win on a small-BAR or older-PCIe card. If it comes back, hookshot_mirror_skip.txt (:2735) belongs under cache/ once #2317 is in. Thanks for measuring and reporting this so carefully.

> [@Joey-Magnus](https://github.com/Joey-Magnus) reviewed on GitHub · 2026-11-12 14:32 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2338#pullrequestreview-5876209944) The review points are addressed on 71add749: `dgpu_binds` is counted only with `!par` on the lock-free path (`gpu/vk/vk_memory.cpp:2872`, `:2881`), a pool gives up after three aborts, and the skip list records device and driver. But it now conflicts with main in `vk_memory.cpp`, `vk_draw.cpp`, `docs/development.md` and `tests/direct_buffer_writes_test.cpp`. More to the point, your P/Q/R runs answer the question: on the RX 9070 XT the best policy state is 2.7% slower than main, whose memory sync is already about 0.5 ms a frame, and cashcon57's RTX 3080 Ti went from 46 to 34-39 fps with 4 ms more GPU time. No measured hardware comes out ahead; the small-BAR case is a guess. The correctness piece, the snapshot keyed on the binding's pool, is now #2609, the part worth landing. I'd close this in favour of #2609 (your option b) and reopen if someone measures a win on a small-BAR or older-PCIe card. If it comes back, `hookshot_mirror_skip.txt` (`:2735`) belongs under `cache/` once #2317 is in. Thanks for measuring and reporting this so carefully.

github-sync referenced this pull request

2026-11-12 14:43:22 +00:00

fix(vk): the uniform buffer snapshot keys on the binding's pool, not the device #2609

github-sync referenced this pull request

2026-11-12 14:48:30 +00:00

perf(vk): sparse VRAM mirrors, memory only where a game pool is (RX_VK_MIRROR_SPARSE=1) #2794

github-sync referenced this pull request from a commit

2026-11-12 16:54:32 +00:00

fix(vk): count BC textures by blocks in the texture budget (#2624)

Commenting is not possible because the repository is archived.

No reviewers