Repository navigation
fix(vk): cyan after a pause in the background: keep the mirrors of pools the GPU writes - #2154
Conversation
An idle pool's VRAM mirror is given back after kIdleEpochs submissions (at once past kLongIdle, or when a wanted mirror does not fit), and made again from guest memory when the pool is next bound. What a shader wrote through a bare entry, the pointer table or a wide binding is never copied back to guest memory (memory_note_gpu_write): it was in the old mirror alone, so the new one started from stale data and the GPU went on from that. On an RX 5500M, pausing for 1-2 minutes with the game in the background gave back the idle pools' mirrors (51 mirrors, 1136 MB in one pause), and every resume brought cyan for a few seconds. With RX_VK_MIRROR_KEEP=1 (no give-back) two such pauses resumed clean, at Auto and at 33% Render resolution. memory_note_gpu_write now marks the pools it reaches (Pool::gpu_only), the idle give-back leaves their mirrors, and a pool that takes others in keeps their mark. A pool refused a mirror for the budget meanwhile is read in system memory: slower, not wrong. The give-back's log line says how many MB of idle mirrors are kept this way. tests/test_mirror_gpu_only.py reads the source for the three (fails on main).
…carries over a merge nx2/test_import_windows.py compiles pool_lookup from gpu/vk/vk_memory.cpp against a Pool of its own; the merge now copies the taken-in pools' gpu_only mark, so that Pool needs the field (CI: 'struct Pool' has no member named 'gpu_only').
…oo, and log the ones still given back After a save reload, cyan showed in play and over the Start menu, with no pool written past the write-back given back: the mirrors still going were of pools the GPU writes through a normal binding (RX 5500M: three reloads clean with RX_VK_MIRROR_KEEP=1, cyan after each reload without it). Pool::gpu_written is set by gpu_buffer_bind for a written binding of the mirror, carried over a merge, and the idle give-back keeps those mirrors as it keeps gpu_only ones. The mirrors still given back are logged (the first 200): retired, retired and not inherited in time, idle, or idle for room, with how long since their sync and the pool's marks.
rakefire-impala
changed the title
hckrman101
left a comment
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, the diagnosis is clear and the first commit is a well-targeted fix: data that memory_note_gpu_write deliberately never copies back really does live only in the mirror, so giving that mirror back can't be right. CI is green.
My concern is the second commit, which you've flagged as untested in game.
VRAM / budget cost. The VRAM ceiling doesn't change: kept mirrors still count in mirror_bytes and mirror_make still refuses past mirror_budget(). What changes is who gets that budget. Both marks are sticky (never cleared, and ORed through merges). gpu_written is set by any written binding, and with RX_VK_WB_MAX_KB unset (the default) every one of those is queued for write-back, so its data is already in guest memory. In practice this probably marks most pools the game touches with compute. Once kept mirrors fill the budget, the "idle, for room" pass has nothing to free, and new or hot pools stay in system memory and are read over PCIe. That's the "stayed in system memory for good" behaviour the give-back was added to fix (see the comment above kIdleEpochs). The budget is small exactly on the cards this PR is about: on a 4 GB card it's VRAM/8 = 512 MB, or 256 MB with the Minimum preset. The first commit alone kept 112 MB. On 8 GB+ cards the budget is up to 3 GB, so long-idle mirrors that used to go after kLongIdle now stay resident.
Interaction with #2198. That PR turns the texture compaction on by default, and it holds textures at 85% of the allowance (80% for the reserve). Mirrors aren't something it can evict, so mirrors kept around permanently shrink the room left for textures, and the hold starts evicting textures sooner on cards that are already tight. Neither PR breaks the other in code (they don't touch the same files), but it's one more reason to measure the second commit before it ships. Could you post the MB of them idle and kept figure and any stays in system memory lines from a session with a few reloads, ideally on the 5500M?
Suggestions, any one would do:
- Let the "for room" pass free
gpu_written-only mirrors (keep onlygpu_onlyabsolute). Their data was written back, so remaking them from guest memory is correct, and only the long-idle free would be skipped. - Or split the second commit into a follow-up, once a run shows it matches
RX_VK_MIRROR_KEEP=1without pools being refused.
If the save-reload cyan does come from written-back pools, then the write-back or the remake from captures is missing something, and keeping the mirror would hide that rather than fix it. The new log line should show which.
Also, a small hole that stays open: a retired gpu_only pool whose successor makes no mirror within 600 submissions still loses its data (now logged as inherit_lost). That's fine to leave as it is, since the log will show whether it ever happens.
| } | ||
| if (p->synced_epoch != mirror_epoch || p->sync_seg > texture_segment()) mirror_sync(*p); | ||
| if (written) { | ||
| p->gpu_written = true; // (its mirror kept: mirrors_give_back) |
There was a problem hiding this comment.
This marks every pool with any written binding, including those whose write-back is queued just below (all of them with RX_VK_WB_MAX_KB unset). Their contents are in guest memory, so a mirror remade from it should be correct. Maybe only mark when the write-back is skipped (wb_max && size > wb_max), or let the for-room pass free these. See the review body.
| if (p.mstate == Pool::MIRROR_ON && mirror_epoch - p.synced_epoch > kIdleEpochs && p.last_value <= mem_completed) | ||
| idle.push_back(&p); | ||
| if (p.mstate == Pool::MIRROR_ON && mirror_epoch - p.synced_epoch > kIdleEpochs && p.last_value <= mem_completed) { | ||
| if (p.gpu_only || p.gpu_written) kept_bytes += p.hi - p.lo; |
There was a problem hiding this comment.
Kept pools are skipped by the for-room pass too, not just the long-idle one. Once they fill mirror_budget() (512 MB on a 4 GB card, 256 MB with the Minimum preset), every new pool is refused and read over PCIe. Keeping them from the kLongIdle free but still letting them go when a wanted mirror doesn't fit (at least the gpu_written-only ones) would avoid that.
| for (auto it = imported.upper_bound(guest + size - 1); it != imported.begin();) { | ||
| --it; | ||
| if (it->second.hi > guest) it->second.gpu_only = true; | ||
| else if (it->second.hi + (1ull << 30) < guest) break; |
There was a problem hiding this comment.
Nit: imported pools are disjoint (pool_lookup merges overlaps) and sorted by lo, so once hi <= guest nothing further down can reach guest, and the loop could break right there. As written it walks every pool within 1 GB below, while holding pools_mu, on every call.
…k that writes it again (#2125) > **EDIT (2026-11-10):** the first version of this PR said it fixes the cyan flashes and the UI thrown about from #1005. It doesn't. My runs with and without it also differed in Render resolution and session length, and the cyan that reproduces reliably turned out to come from VRAM mirrors given back while idle, after a pause with the game in the background and (mostly) after a save reload: #2154. What this PR does fix is below. ## What it fixes #296 made every CPU read of GPU results wait for the GPU work that writes them. Without that wait, OoT's exposure flashed for a frame (#296: "OoT exposure flashes"). With per-queue fences on (the default since #523) that guarantee has a hole. When the work that writes the range is stopped at a wait that is already met (for example a CPU event the game signalled, which is applied at the replay's next look at the queues), the read-back finds no pending write-back, waits for nothing, and reads the range as it was: an earlier frame's result. This PR closes that hole. Before the lookup the read-back lets that stopped work on, never forcing anything, so its write-back is known and the read waits for exactly that work. ## Why it's worth merging - **It restores #296's guarantee on the default path.** The hole looks accidental. #523 skipped the read-back's catch-up because a catch-up then pushed stopped work past its waits. #449 had already made the catch-up safe, 53 minutes before #523 merged, so the skip outlived its reason. - **It's cheap.** A read-back that has to wait now takes about 1 ms (0.65 ms median), the time for the GPU to do the write. `RX_VK_PER_QUEUE_FENCES=0`, the workaround in #1005, closes the same hole by waiting for every queue's work: 19–87 ms per read-back on the same card, 12.5 fps against 31.8 with this PR at the same settings. - **It's small and contained.** One `if` in `invalidate`, using the same `sync_settle()` the texture branch further down already calls. Nothing changes with `RX_VK_PER_QUEUE_FENCES=0`, `RX_VK_ASYNC=1` or `RX_VK_SYNC_WAITS=0`. - **It's covered.** `tests/test_readback_settle.py` fails on main and passes with this change. What I haven't seen is a visible change I can pin on it alone. It's a correctness fix for a known bug class (#296), not a fix for a reported picture problem. ## How the hole works `invalidate` looks up the range's write-back value, which is known only once the work that writes the range is recorded. #523 skipped the read-back's catch-up with per-queue fences on. So work stopped at a wait that was already met stayed unrecorded, its write-back was unknown, and the read went ahead with nothing to wait for. ## Why the read-back changes again The read-back in `invalidate` has changed several times, each step fixing the one before: 1. **#296** (evilmfkr): a CPU read of GPU results waits for the work that writes it back. Today each range remembers the submission that writes it back (`wb_pages_note`, noted when that work is recorded), and `invalidate` waits for that value. Before the lookup the replay is caught up, so work handed over can note its write-back first. 2. **The catch-up forced stopped work.** In the sync replay, that catch-up pushed work stopped at an event wait past its wait, ahead of the work it waits for: the fog and exposure of the frame before, for a frame. 3. **#449** (azlekayn, merged 2026-11-08 08:19 UTC): a catch-up lets on only the stopped work whose waits are met, and forces nothing (`wait_queue_empty` in the sync replay, `sync_settle`). 4. **#523** (hckrman101, merged 2026-11-08 09:12 UTC): per-queue fences. With them on, the read-back skips its catch-up (`!unforced`) and waits on the recordings' timeline instead. That skip was written while a catch-up still forced (step 2). It merged 53 minutes after #449 had made the catch-up safe, so the skip outlived its reason. 5. **#1276** (Odrannnn): the recordings' wait sends the held batch first (RADV). `RX_VK_PER_QUEUE_FENCES=0` goes back to the old path: a catch-up, then a wait on the shared timeline. That timeline is signalled only as far as no older submission of any queue is stopped, so each read-back also waits for stopped work whose waits are not met, which waits for other threads' submissions: tens of milliseconds. ## Change In `invalidate`'s read-back, with per-queue fences on, settle before the lookup: `sync_settle()` applies the CPU's pending signals and lets on the stopped work whose waits are met, never forcing anything (the same call the texture branch further down already makes). That work is recorded, its write-back value is known, and `wait_recorded` waits for exactly that recording, as before. Stopped work whose waits are not met is not waited for. - With `RX_VK_PER_QUEUE_FENCES=0`, `RX_VK_ASYNC=1` or `RX_VK_SYNC_WAITS=0` nothing changes: those still call `wait_queue_empty()` as before. - `RX_VK_READBACK_NO_CATCHUP` still turns the catch-up off. ## What it changes, measured RX 5500M (RDNA1, 4 GB), Windows 11, AMD driver, main `92907f7` with #1581. Read-back figures come from the `readbacks of GPU results: N of 3000 waited` lines. | | main | `RX_VK_PER_QUEUE_FENCES=0` | this PR | |---|---|---|---| | read-backs that waited, per 3000 | ~890 | 65–68 | ~710 | | time per wait | 0.07 ms (0.02–0.19) | 19–87 ms | 0.65 ms median, 1.28 mean | | main thread's time waiting, per 3000 reads | 24 ms | — | 499 ms | On main the waits are tiny because they usually wait for nothing: the work that writes the range has not been recorded yet. With this PR a read-back waits for the GPU to finish the write. On the main thread that is about 0.17 ms per read, roughly 24 ms in each second of play at that run's read rate (about 2.4% of the thread). Of that run's 122 read-back lines (3000 reads each), six averaged more than 5 ms a wait; the highest, 30 ms, was during an area load. Frame rate against `RX_VK_PER_QUEUE_FENCES=0`, the workaround in #1005, at the same settings (Render resolution 50%, Hyrule Market): | | `RX_VK_PER_QUEUE_FENCES=0` | this PR | |---|---|---| | 5-second stretches | 57 | 116 | | mean fps | 12.5 | 31.8 | | stretches under 8 fps | 29 (51%) | 4 (3%) | I did not run main and this PR at the same settings on the same route, so this is no claim that the PR is faster than main. ## Tests `tests/test_readback_settle.py` (new) reads the source and checks two things: the read-back settles before the write-back lookup, and it no longer skips its catch-up with per-queue fences. It fails on current main and passes with this change. `tests/test_recorded_wait_flush.py` still passes. `tests/run_all.py` on main `bdfe09c` with this change: 311 passed. The 6 that fail are the same 6 that fail on plain main on this PC. Rebased onto `fab8cb3` since, with no conflict, and the two tests above pass there. It also merges cleanly with #1905, which changes the same wait's batch flush. Related: #1005, #523, #449, #296.
Joey-Magnus
left a comment
Joey-Magnus
left a comment
There was a problem hiding this comment.
The first commit is right: data memory_note_gpu_write never copies back lives only in the VRAM mirror, so keeping gpu_only pools' mirrors fixes the cause.
The second commit is untested in game by your own note. It sets gpu_written on every written binding (vk_memory.cpp:2760) and never clears it, and mirrors_give_back then never frees those mirrors (:2901), not even in the "for room" pass. With write-back on by default, that data is already in guest memory. On a 4 GB card (512 MB mirror budget, 256 MB on Minimum) other pools get pushed into system memory for good. Either ship the first commit alone, or let the "for room" pass free gpu_written-only mirrors, and attach a 5500M log with save reloads.
Port of upstream NotRexed#2154. An idle pool's mirror could be given back though the GPU had written data there that is never copied back (guessed ranges, the pointer table, wide bindings) or through a normal mirror binding. Made again from guest memory, that data was gone and the picture showed cyan for a few seconds after a pause or a save reload. Such pools keep their mirrors; a pool that takes another in inherits both marks, and the give-back log says which mirrors still go and why.
Problem
Pause the game, leave it in the background for 1–2 minutes (another window or the Start menu in front), then come back and resume: for a few seconds the picture has cyan patches. On an RX 5500M (RDNA1, 4 GB, Windows 11, AMD driver) this happened on every resume.
While the game is paused, most of its memory pools are not bound. After
kIdleEpochs(600) submissions an unbound pool's VRAM mirror is idle, andmirrors_give_backgives it back once it has been idle pastkLongIdle(6000 submissions) or when a wanted mirror does not fit. In one such pause the running count in the log went from 35 to 51 (51 mirrors given back (1136 MB in all)). At the pool's next binding,mirror_makemakes a new mirror from guest memory (plus the CPU's newer writes from the captures).Most of what the GPU writes into a mirror is copied back to guest memory: a written binding queues a write-back. But some of it is never copied back, by design, and is noted only as a hazard by
memory_note_gpu_write: what a shader writes through a bare entry's guessed range, the pointer table's range or a wide binding. (Copying the whole guessed range back overwrote the CPU's own data next to it.) That data was in the old mirror alone. The new mirror starts without it, and the GPU goes on from stale data until it has rebuilt it, which is the few seconds of cyan.Change
Pool::gpu_only: set bymemory_note_gpu_writeon every imported pool its range reaches.mirrors_give_backleaves an idlegpu_onlypool's mirror alone. Its other idle mirrors go as before.pool_lookup's merge) keeps their mark: their mirrors' contents come with them.Pool::gpu_written, set bygpu_buffer_bindfor a written binding of the mirror, carried over a merge likegpu_only, and the idle give-back keeps those mirrors too.A pool that is refused a mirror for the budget meanwhile is read in system memory, which is slower but not wrong. Retired pools are unchanged: their GPU contents already go to the pool that took them in (
inherit_pending).Tested
RX 5500M, Windows 11, AMD driver, main
92907f7with #1581 and #2125, same settings throughout. Each pause was 1–2 minutes with the game in the background:92907f7+ #1581 + #2125)RX_VK_MIRROR_KEEP=1(no give-back at all)A very short black flash right on resume is still there at both resolutions. At Auto it has been there since the first builds on this card, including the
RX_VK_MIRROR_KEEP=1run, so it looks separate.Save reloads
With the first commit, cyan came back after reloading a save within a run (Hyrule Castle, Hyrule Field), in play and over the Start menu, with no pause and no
not importedpools in the log. (That run also hadRX_VK_FIFO=1, which turned out not to matter.) AgainstRX_VK_MIRROR_KEEP=1, which never gives a mirror back:RX_VK_MIRROR_KEEP=1So mirrors given back explain most of the cyan after a reload, but not all of it. The first commit keeps only pools the GPU wrote past the write-back. The pools whose mirrors still went are the ones the GPU writes through a normal binding, which the second commit keeps too.
Not tested in game yet: the second commit. I haven't played a build with it, so I don't know yet whether it matches
RX_VK_MIRROR_KEEP=1or what it costs in VRAM. Its log line is there to answer both, and to show which mirrors are still given back before any flash that's left.Tests
tests/test_mirror_gpu_only.py(new) reads the source and checks four things:memory_note_gpu_writemarks the pools it reaches,gpu_buffer_bindmarks a pool written through its mirror, the idle give-back leaves marked pools, and a merge keeps both marks. It fails on current main and passes with this change.nx2/test_import_windows.pybuildspool_lookupagainst aPoolof its own, which now has both fields.tests/run_all.pyon main4d68bc83with this branch merged: 311 passed. The 6 that fail are the same 6 that fail on plain main on this PC.Related: #1005 (cyan and UI flashes after focus loss), #2125.