fix(vk): no mirror is made inside a parallel replay's segment #1184

Merged

t0masz407 merged 1 commit from fix/parallel-mirror-make into main

2026-11-09 18:57:24 +00:00

t0masz407 commented

2026-11-09 16:39:13 +00:00

(Migrated from github.com)

What this fixes

With RX_VK_PARALLEL, large structures go missing in play: houses, big rocks, bushes, the tree over the Kokiri shop. With small segments, geometry is torn into huge stretched triangles. Serial replay is complete. This is the RX_VK_PARALLEL part of #994.

gpu_buffer_bind makes a pool's VRAM mirror the first time the pool is bound (mirror_make):

It points the page table at the mirror at once.
It records the mirror's fill (the whole pool copied in) into the uploads of the current recording.

In a parallel replay that recording is the uploads of the segment whose worker bound the pool first. The workers record out of order, so this need not be the first segment in the queue that reads the pool. An earlier segment, submitted first, reads the mirror before it is filled.

This matches what was seen:

The more segment boundaries, the worse.
One segment is complete.
A structure that was missing comes back after a few minutes, once every pool in use is mirrored, and new pools after a scene change start it again.

Change

Inside a parallel segment (par_active), the first binding no longer makes the mirror. The pool is marked wanted, and the mirror is made at the next submission's start, as RX_VK_MIRROR_DEFER=1 already does for every pool. Serial replay is unchanged. The cost is that a pool first bound in a segment is read from guest memory for one more submission.

tests/test_parallel_mirror_make.py checks the first-binding condition: it fails on current main and passes with this change.

Tested

RX 7900 XTX, RADV (Mesa 26.2.6), Linux. Hookshot v2.0.7 plus #1069 and #1108 plus this change. Scripted runs: the same save in front of the Kokiri shop, 4 camera directions, 2 runs per setting.

setting	before	after
`RX_VK_PARALLEL=0`	complete	(unchanged)
`RX_VK_PARALLEL=4`	shop tree and other structures missing	complete, 8 of 8
`RX_VK_PARALLEL=4`, `RX_VK_PARALLEL_SEG=40`	torn geometry, huge triangles, 74-87 fps	complete, 8 of 8, 120 fps (frame generation)
`RX_VK_PARALLEL=4`, `RX_VK_PARALLEL_SEG=1000000` (one segment)	complete

Ruled out before this was found: #1069 (RX_VK_RB_REFRESH_EACH=1), RX_VK_NO_DESC_REUSE, RX_VK_NO_GRID_PATCH. #1108 alone was not tested without this change.

Not covered

Hardware tested on a v2.0.7 base, not a current-main binary; the changed code is identical on main.
Only AMD/RADV on Linux.
mirror_sync inside a segment (a pool synced by a later segment first) was already handled by sync_seg and is unchanged.

🤖 Generated with Claude Code

## What this fixes With `RX_VK_PARALLEL`, large structures go missing in play: houses, big rocks, bushes, the tree over the Kokiri shop. With small segments, geometry is torn into huge stretched triangles. Serial replay is complete. This is the `RX_VK_PARALLEL` part of #994. `gpu_buffer_bind` makes a pool's VRAM mirror the first time the pool is bound (`mirror_make`): - It points the page table at the mirror at once. - It records the mirror's fill (the whole pool copied in) into the uploads of the current recording. In a parallel replay that recording is the uploads of the segment whose worker bound the pool first. The workers record out of order, so this need not be the first segment in the queue that reads the pool. An earlier segment, submitted first, reads the mirror before it is filled. This matches what was seen: - The more segment boundaries, the worse. - One segment is complete. - A structure that was missing comes back after a few minutes, once every pool in use is mirrored, and new pools after a scene change start it again. ## Change Inside a parallel segment (`par_active`), the first binding no longer makes the mirror. The pool is marked wanted, and the mirror is made at the next submission's start, as `RX_VK_MIRROR_DEFER=1` already does for every pool. Serial replay is unchanged. The cost is that a pool first bound in a segment is read from guest memory for one more submission. `tests/test_parallel_mirror_make.py` checks the first-binding condition: it fails on current main and passes with this change. ## Tested RX 7900 XTX, RADV (Mesa 26.2.6), Linux. Hookshot v2.0.7 plus #1069 and #1108 plus this change. Scripted runs: the same save in front of the Kokiri shop, 4 camera directions, 2 runs per setting. | setting | before | after | |---|---|---| | `RX_VK_PARALLEL=0` | complete | (unchanged) | | `RX_VK_PARALLEL=4` | shop tree and other structures missing | complete, 8 of 8 | | `RX_VK_PARALLEL=4`, `RX_VK_PARALLEL_SEG=40` | torn geometry, huge triangles, 74-87 fps | complete, 8 of 8, 120 fps (frame generation) | | `RX_VK_PARALLEL=4`, `RX_VK_PARALLEL_SEG=1000000` (one segment) | complete | | Ruled out before this was found: #1069 (`RX_VK_RB_REFRESH_EACH=1`), `RX_VK_NO_DESC_REUSE`, `RX_VK_NO_GRID_PATCH`. #1108 alone was not tested without this change. ## Not covered - Hardware tested on a v2.0.7 base, not a current-main binary; the changed code is identical on main. - Only AMD/RADV on Linux. - `mirror_sync` inside a segment (a pool synced by a later segment first) was already handled by `sync_seg` and is unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

hckrman101 (Migrated from github.com) approved these changes

2026-11-09 18:53:49 +00:00

hckrman101 (Migrated from github.com) left a comment

Nice find, and the diagnosis checks out against the code.

Walked through the failure on main: segment k's worker binds a pool first, mirror_make records the whole-pool fill into segment k's upload and sets synced_gen = gen_now, sync_seg = k. Then a worker for an earlier segment j < k binds the same pool, takes the slow path, sees sync_seg > texture_segment() and calls mirror_sync, which copies nothing (no page is newer than synced_gen) and hands back p->mirror. Segment j runs before k's upload in the queue, so it reads an empty mirror. That lines up with what you saw (worse with more segments, fine with one, self-healing once everything is mirrored).

What I checked with !par:

A pool first bound in a segment goes to MIRROR_WANTED and every binding of it (both the lock-free tl_pools path and the locked path, any worker) gets p->buffer, so no segment can end up bound to a mirror that doesn't exist or isn't filled yet. The page table isn't touched either, since guest_pages_set only happens in mirror_make.
mirror_wanted.push_back and the mstate change happen under pools_mu, and the mirror is made at memory_begin_submission on the replay thread, outside par_active.
par_active is only set while workers record segments (the replay thread just waits and submits), and it's set before the jobs are pushed under par_pool.mu, so the relaxed load in the workers is fine.
The two mirror_make call sites are this one and memory_begin_submission, so nothing else makes a mirror inside a segment. Pools already mirrored are still covered by memory_sync_mirrors_before_segments.
The serial path is unchanged.

The trade-off is that a pool first bound inside a segment is read from guest memory for one submission. That's the case the make-at-first-binding was added for (the shadow line flicker), so it could come back briefly for those pools under RX_VK_PARALLEL. That's much better than missing geometry and it's documented, so I'm fine with it. If it ever shows up, a follow-up could make those mirrors on the replay thread before the split.

Test: tests/test_parallel_mirror_make.py gets picked up by run_all.py's glob and doesn't need an ARGS entry. Its checks hold on the head source: one mirror_make(*p) line in gpu_buffer_bind, and that line contains !par. It's a source-shape check, which is how the repo's other tests do it. No CI checks have run on this head yet.

Overlaps: #1133 and #1150 both edit gpu_buffer_bind near this line (the direct-binding branch, and a new read-back-direct early return), and #1127 changes the inherit step in mirror_make. I don't see any conflict in what they do, just nearby hunks that might need a small rebase depending on merge order. The branch is also a bit behind main (the RX_VK_WB_DIFF default), but GitHub reports it mergeable.

LGTM.

Nice find, and the diagnosis checks out against the code. Walked through the failure on main: segment k's worker binds a pool first, `mirror_make` records the whole-pool fill into segment k's upload and sets `synced_gen = gen_now`, `sync_seg = k`. Then a worker for an earlier segment j < k binds the same pool, takes the slow path, sees `sync_seg > texture_segment()` and calls `mirror_sync`, which copies nothing (no page is newer than `synced_gen`) and hands back `p->mirror`. Segment j runs before k's upload in the queue, so it reads an empty mirror. That lines up with what you saw (worse with more segments, fine with one, self-healing once everything is mirrored). What I checked with `!par`: - A pool first bound in a segment goes to `MIRROR_WANTED` and every binding of it (both the lock-free `tl_pools` path and the locked path, any worker) gets `p->buffer`, so no segment can end up bound to a mirror that doesn't exist or isn't filled yet. The page table isn't touched either, since `guest_pages_set` only happens in `mirror_make`. - `mirror_wanted.push_back` and the `mstate` change happen under `pools_mu`, and the mirror is made at `memory_begin_submission` on the replay thread, outside `par_active`. - `par_active` is only set while workers record segments (the replay thread just waits and submits), and it's set before the jobs are pushed under `par_pool.mu`, so the relaxed load in the workers is fine. - The two `mirror_make` call sites are this one and `memory_begin_submission`, so nothing else makes a mirror inside a segment. Pools already mirrored are still covered by `memory_sync_mirrors_before_segments`. - The serial path is unchanged. The trade-off is that a pool first bound inside a segment is read from guest memory for one submission. That's the case the make-at-first-binding was added for (the shadow line flicker), so it could come back briefly for those pools under `RX_VK_PARALLEL`. That's much better than missing geometry and it's documented, so I'm fine with it. If it ever shows up, a follow-up could make those mirrors on the replay thread before the split. Test: `tests/test_parallel_mirror_make.py` gets picked up by `run_all.py`'s glob and doesn't need an `ARGS` entry. Its checks hold on the head source: one `mirror_make(*p)` line in `gpu_buffer_bind`, and that line contains `!par`. It's a source-shape check, which is how the repo's other tests do it. No CI checks have run on this head yet. Overlaps: #1133 and #1150 both edit `gpu_buffer_bind` near this line (the direct-binding branch, and a new read-back-direct early return), and #1127 changes the inherit step in `mirror_make`. I don't see any conflict in what they do, just nearby hunks that might need a small rebase depending on merge order. The branch is also a bit behind main (the `RX_VK_WB_DIFF` default), but GitHub reports it mergeable. LGTM.

gpu/vk/vk_memory.cpp

Checked: with !par the pool falls straight into the MIRROR_WANTED branch below and binds p->buffer, and the lock-free path above already returns the guest buffer for MIRROR_WANTED. So once one worker marks the pool wanted, every segment binds guest memory consistently for the rest of the submission, which is what we want.

Checked: with `!par` the pool falls straight into the `MIRROR_WANTED` branch below and binds `p->buffer`, and the lock-free path above already returns the guest buffer for `MIRROR_WANTED`. So once one worker marks the pool wanted, every segment binds guest memory consistently for the rest of the submission, which is what we want.

Commenting is not possible because the repository is archived.

No reviewers

hckrman101