perf(vk): the retirement sweep's texture write-backs in batches, one submission and one wait each #840

2026-11-08T22:54:59Z

SadTransGirl commented

2026-11-08 22:54:59 +00:00

(Migrated from github.com)

flush_texture is split into a GPU half (flush_record: the image's copies into a staging buffer, into rb_cmd) and a CPU half (flush_finish: the texels into the texture's memory), and flush_textures does a batch with one submission and one fence wait. The sweep chooses the textures to go first, writes back the flushable ones in batches of 64 or ~512 MB, then destroys them; its budget test counts the chosen ones as gone. Each write-back was a submit and a wait on its fence, a drain of the queue: a sweep of 500-600 textures at a scene load stalled for seconds (hookshot_log: 517 and 635 retired at once, 160 write-backs, submits of 4.7 and 7.7 s).

Tested: RTX 4070 SUPER, Windows 11, Kokiri Forest and the scene loads after it. Before, sweeps retiring 517 and 635 textures at once made single submits of 4.7 and 7.7 s (hookshot_log: 160 write-backs); after, the worst submit of a session with the same loads was 0.48 s, with no device loss.

flush_texture is split into a GPU half (flush_record: the image's copies into a staging buffer, into rb_cmd) and a CPU half (flush_finish: the texels into the texture's memory), and flush_textures does a batch with one submission and one fence wait. The sweep chooses the textures to go first, writes back the flushable ones in batches of 64 or ~512 MB, then destroys them; its budget test counts the chosen ones as gone. Each write-back was a submit and a wait on its fence, a drain of the queue: a sweep of 500-600 textures at a scene load stalled for seconds (hookshot_log: 517 and 635 retired at once, 160 write-backs, submits of 4.7 and 7.7 s). Tested: RTX 4070 SUPER, Windows 11, Kokiri Forest and the scene loads after it. Before, sweeps retiring 517 and 635 textures at once made single submits of 4.7 and 7.7 s (hookshot_log: 160 write-backs); after, the worst submit of a session with the same loads was 0.48 s, with no device loss.

lucasmazzoni45-del commented

2026-11-09 19:19:41 +00:00

(Migrated from github.com)

Data point for this PR from an AMD card, in case it helps the rebase: RX 7800 XT + Ryzen 7 5800X3D, Windows 11, v2.0.10, 2560x1440, frame generation x2, about 1.5 hours of normal play.

The retirement sweep is very busy on this card. From hookshot_log.txt of that one session:

2073 textures retired events, 108255 textures retired in all (about 50 a sweep, up to 130 in one: vk: 132 textures retired (1083 left, 3065 MB))
VRAM late in the session: 12.8 GB used of 15.4 GB, textures 7.1-8.1 GB against the 8192 MB budget, texture count going 4449 -> 3811 -> 3854

So the sweep runs constantly here, not just at a scene load. Each retired texture the GPU wrote goes through flush_texture, one submission and one fence wait each, so batching them as this PR does should matter on this card too. I have not measured it: I could not apply the PR as it is, because the eviction loop it changes was reworked in v2.0.8 (the dead-target handling: need_dead, drawn_over_shifted, the n_dead/b_dead counters). The merge conflicts are in exactly that loop.

Could the PR be rebased on v2.0.10 (or main)? I can test a rebased version on this machine (AMD, Windows) and report the effect on the frame times.

Related stutter data from the same card, for context: #995 (comment with the replay CPU times, v2.0.7 vs v2.0.10).

Data point for this PR from an AMD card, in case it helps the rebase: RX 7800 XT + Ryzen 7 5800X3D, Windows 11, v2.0.10, 2560x1440, frame generation x2, about 1.5 hours of normal play. The retirement sweep is very busy on this card. From `hookshot_log.txt` of that one session: - 2073 `textures retired` events, 108255 textures retired in all (about 50 a sweep, up to 130 in one: `vk: 132 textures retired (1083 left, 3065 MB)`) - VRAM late in the session: 12.8 GB used of 15.4 GB, textures 7.1-8.1 GB against the 8192 MB budget, texture count going 4449 -> 3811 -> 3854 So the sweep runs constantly here, not just at a scene load. Each retired texture the GPU wrote goes through `flush_texture`, one submission and one fence wait each, so batching them as this PR does should matter on this card too. I have not measured it: I could not apply the PR as it is, because the eviction loop it changes was reworked in v2.0.8 (the dead-target handling: `need_dead`, `drawn_over_shifted`, the `n_dead`/`b_dead` counters). The merge conflicts are in exactly that loop. Could the PR be rebased on v2.0.10 (or main)? I can test a rebased version on this machine (AMD, Windows) and report the effect on the frame times. Related stutter data from the same card, for context: #995 (comment with the replay CPU times, v2.0.7 vs v2.0.10).

hckrman101 (Migrated from github.com) reviewed

2026-11-09 19:31:38 +00:00

hckrman101 (Migrated from github.com) left a comment

Nice one, the split into flush_record / flush_finish is clean and the numbers in the description are convincing.

This conflicts with main and needs a rebase. The flush helpers themselves haven't changed on main, so that half should apply as is. The conflict is in the texture_frame eviction loop, which main has reworked since:

the break condition is now frame_no - last < need_dead, followed by the drawn_over_shifted / need_unused check (dead transient targets)
there's a new keep-unflushable continue (issue 993)
per-eviction bookkeeping now sits between the flush and destroy_later: n_dead/b_dead for shifted targets, and the evicted_from_memory record for uploaded textures

When you rebase, those last two should go into the destroy loop. The easiest way is probably to carry shifted in Evict. The pending_bytes change also needs to go onto main's new break line. Side note: main now runs the dead-target sweep every 60 presents instead of every 600, so batching helps there too. The failing test_mem_trim check ("a BC3 upload uses a scratch it did not ask for") comes from main's newer test running against this old base, not from this change.

What I checked and think is right:

every texture in a batch gets its own staging buffer, with 16-byte-aligned part offsets, the same as before
one full_barrier before all copies covers every image (they're all in GENERAL)
the host barrier and fence wait run before any flush_finish
if one texture's flush_record fails, only that texture is skipped, and a failed readback_begin is handled
flush_finish runs in oldest-first order, so overlapping write-backs land in the same order as before, and the memory_written_runs checks see the same earlier writes
deferring destroy_later/forget_gpu until after the selection doesn't change any drawn_over/keep_finalized decision, because a texture evicted earlier in order can't have last_written after a later one's last_frame
pending_bytes uses the same bytes + pad that destroy_later subtracts
last_value > completed is still checked, image destruction is still deferred, and it's still the same thread and the same rb_cmd use as before

Two smaller things inline. With a rebase I'd be happy with this.

Nice one, the split into flush_record / flush_finish is clean and the numbers in the description are convincing. **This conflicts with main and needs a rebase.** The flush helpers themselves haven't changed on main, so that half should apply as is. The conflict is in the `texture_frame` eviction loop, which main has reworked since: - the break condition is now `frame_no - last < need_dead`, followed by the `drawn_over_shifted` / `need_unused` check (dead transient targets) - there's a new keep-unflushable `continue` (issue 993) - per-eviction bookkeeping now sits between the flush and `destroy_later`: `n_dead`/`b_dead` for shifted targets, and the `evicted_from_memory` record for uploaded textures When you rebase, those last two should go into the destroy loop. The easiest way is probably to carry `shifted` in `Evict`. The `pending_bytes` change also needs to go onto main's new break line. Side note: main now runs the dead-target sweep every 60 presents instead of every 600, so batching helps there too. The failing `test_mem_trim` check ("a BC3 upload uses a scratch it did not ask for") comes from main's newer test running against this old base, not from this change. What I checked and think is right: - every texture in a batch gets its own staging buffer, with 16-byte-aligned part offsets, the same as before - one `full_barrier` before all copies covers every image (they're all in GENERAL) - the host barrier and fence wait run before any `flush_finish` - if one texture's `flush_record` fails, only that texture is skipped, and a failed `readback_begin` is handled - `flush_finish` runs in oldest-first order, so overlapping write-backs land in the same order as before, and the `memory_written_runs` checks see the same earlier writes - deferring `destroy_later`/`forget_gpu` until after the selection doesn't change any `drawn_over`/`keep_finalized` decision, because a texture evicted earlier in `order` can't have `last_written` after a later one's `last_frame` - `pending_bytes` uses the same `bytes + pad` that `destroy_later` subtracts - `last_value > completed` is still checked, image destruction is still deferred, and it's still the same thread and the same `rb_cmd` use as before Two smaller things inline. With a rebase I'd be happy with this.

gpu/vk/vk_texture.cpp

					
				@ -2525,0 +2558,4 @@

				        // that failed. The copies may still be writing into the staging buffers, so they are not freed here: they go the way a

				        // destroyed image does, once a submission made after this one is done. The batch's write-backs are lost, as a single

				        // one's was before)

				        static int logged;

Already true before this PR, but it matters more now. If this 5 s wait times out (it also covers whatever queue_flush_locked just submitted, and at a scene load that can be a lot), flush_finish frees up to 64 staging buffers that the copies may still be writing into, and the whole batch's write-backs are lost at once instead of one. On a non-success result, could the buffers be kept alive (wait again / wait for idle, or hand them to the deferred-destroy list) instead of freed straight away?

Already true before this PR, but it matters more now. If this 5 s wait times out (it also covers whatever `queue_flush_locked` just submitted, and at a scene load that can be a lot), `flush_finish` frees up to 64 staging buffers that the copies may still be writing into, and the whole batch's write-backs are lost at once instead of one. On a non-success result, could the buffers be kept alive (wait again / wait for idle, or hand them to the deferred-destroy list) instead of freed straight away?

gpu/vk/vk_texture.cpp

Small robustness point: destroy_later clamps with std::min(total_bytes, ...), which suggests total_bytes can drift below the sum of the textures' sizes. If it does, total_bytes - pending_bytes wraps around, the budget test never breaks, and everything old enough goes. Maybe pending_bytes >= total_bytes || total_bytes - pending_bytes <= budget / 8 * 7? (This line is also where the rebase conflicts with main's need_dead change.)

Small robustness point: `destroy_later` clamps with `std::min(total_bytes, ...)`, which suggests `total_bytes` can drift below the sum of the textures' sizes. If it does, `total_bytes - pending_bytes` wraps around, the budget test never breaks, and everything old enough goes. Maybe `pending_bytes >= total_bytes || total_bytes - pending_bytes <= budget / 8 * 7`? (This line is also where the rebase conflicts with main's `need_dead` change.)

gpu/vk/vk_texture.cpp

Nit: tex->bytes is the image's size. The staging buffer is f.total, which is the guest-format size, so the 512 MB cap is only approximate. With kFlushMax at 16 MB the 64-texture cap bounds it anyway (1 GB worst case), so this is fine. Counting e.tex->guest_size would just make the cap mean what the comment says.

Nit: `tex->bytes` is the image's size. The staging buffer is `f.total`, which is the guest-format size, so the 512 MB cap is only approximate. With `kFlushMax` at 16 MB the 64-texture cap bounds it anyway (1 GB worst case), so this is fine. Counting `e.tex->guest_size` would just make the cap mean what the comment says.

SadTransGirl commented

2026-11-09 22:38:25 +00:00

(Migrated from github.com)

@hckrman101 thanks for the careful read. Main is merged into the branch now (6ff5fc8), resolved the way you laid out:

main's selection logic is untouched; the chosen textures' bytes count as gone on the new break line (total_bytes - pending_bytes);
shifted is carried in Evict, and n_dead/b_dead, the evicted_from_memory record and the remade log run in the destroy loop, after the batches;
the keep-unflushable continue stays where main has it, before a texture is chosen;
flush_texture had not changed on main, so the split into flush_record / flush_finish / flush_textures applies as it was.

GitHub shows it mergeable, and test_mem_trim passes on the merge. It is a merge commit rather than a rebase because I could not force-push from my setup.

@lucasmazzoni45-del it applies to main now, so it builds on top of v2.0.11. If you can run a session with it, the lines to compare are the textures retired sweeps and the slow submit lines around them.

One honest note from my side: on current main I have not re-measured the effect. A ten-minute run here (RTX 4070 SUPER, Windows 11, main at 7dbdcdc with this branch) had 84 sweeps, the largest retiring 553 textures at once, and none of them had anything to write back, so the batches were not exercised; no device loss. The multi-second sweeps in the description were measured on the v2.0.5 base. A card where the sweep writes back often, like yours, is the better test.

@hckrman101 thanks for the careful read. Main is merged into the branch now (6ff5fc8), resolved the way you laid out: - main's selection logic is untouched; the chosen textures' bytes count as gone on the new break line (`total_bytes - pending_bytes`); - `shifted` is carried in `Evict`, and `n_dead`/`b_dead`, the `evicted_from_memory` record and the remade log run in the destroy loop, after the batches; - the keep-unflushable `continue` stays where main has it, before a texture is chosen; - `flush_texture` had not changed on main, so the split into `flush_record` / `flush_finish` / `flush_textures` applies as it was. GitHub shows it mergeable, and `test_mem_trim` passes on the merge. It is a merge commit rather than a rebase because I could not force-push from my setup. @lucasmazzoni45-del it applies to main now, so it builds on top of v2.0.11. If you can run a session with it, the lines to compare are the `textures retired` sweeps and the `slow submit` lines around them. One honest note from my side: on current main I have not re-measured the effect. A ten-minute run here (RTX 4070 SUPER, Windows 11, main at 7dbdcdc with this branch) had 84 sweeps, the largest retiring 553 textures at once, and none of them had anything to write back, so the batches were not exercised; no device loss. The multi-second sweeps in the description were measured on the v2.0.5 base. A card where the sweep writes back often, like yours, is the better test.

hckrman101 (Migrated from github.com) reviewed

2026-11-09 22:43:01 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks for the merge, this now applies cleanly and I read the resolved eviction loop against main's: selection is unchanged, shifted rides in Evict, and n_dead/b_dead, the evicted_from_memory record and the remade log run in the destroy loop after the batches. The keep-unflushable continue still comes before a texture is chosen. CI is green. The flush_record / flush_finish / flush_textures code is the same as I reviewed before, and I still think it's correct (also checked: if every flush_record fails, rb_cmd is left recording but the next readback_begin resets the pool, so that's fine).

Two small points from my first review are still open, neither blocking. Replying on the old threads:

the 5 s fence timeout frees the batch's staging buffers while the copies may still be writing into them
total_bytes - pending_bytes can wrap if total_bytes has drifted below the sum (destroy_later clamps with std::min, so the code already expects that)

The tex->bytes vs guest_size thing is only a nit. I'm fine with this going in as is.

Thanks for the merge, this now applies cleanly and I read the resolved eviction loop against main's: selection is unchanged, `shifted` rides in `Evict`, and `n_dead`/`b_dead`, the `evicted_from_memory` record and the remade log run in the destroy loop after the batches. The keep-unflushable `continue` still comes before a texture is chosen. CI is green. The `flush_record` / `flush_finish` / `flush_textures` code is the same as I reviewed before, and I still think it's correct (also checked: if every `flush_record` fails, `rb_cmd` is left recording but the next `readback_begin` resets the pool, so that's fine). Two small points from my first review are still open, neither blocking. Replying on the old threads: - the 5 s fence timeout frees the batch's staging buffers while the copies may still be writing into them - `total_bytes - pending_bytes` can wrap if `total_bytes` has drifted below the sum (`destroy_later` clamps with `std::min`, so the code already expects that) The `tex->bytes` vs `guest_size` thing is only a nit. I'm fine with this going in as is.

hckrman101 (Migrated from github.com) reviewed

2026-11-09 22:43:10 +00:00

gpu/vk/vk_texture.cpp

					
				@ -2525,0 +2558,4 @@

				        // that failed. The copies may still be writing into the staging buffers, so they are not freed here: they go the way a

				        // destroyed image does, once a submission made after this one is done. The batch's write-backs are lost, as a single

				        // one's was before)

				        static int logged;

Still open at 6ff5fc8: on a non-success readback_submit_wait, flush_finish still destroys the staging buffers straight away. The risk is only a 5 s timeout, so a follow-up is fine.

Still open at 6ff5fc8: on a non-success `readback_submit_wait`, `flush_finish` still destroys the staging buffers straight away. The risk is only a 5 s timeout, so a follow-up is fine.

hckrman101 (Migrated from github.com) reviewed

2026-11-09 22:43:11 +00:00

gpu/vk/vk_texture.cpp

Still there on the new break line (total_bytes - pending_bytes). A guard like pending_bytes >= total_bytes || would cover it. Cheap, but not a blocker.

Still there on the new break line (`total_bytes - pending_bytes`). A guard like `pending_bytes >= total_bytes ||` would cover it. Cheap, but not a blocker.

lucasmazzoni45-del commented

2026-11-09 22:45:03 +00:00

(Migrated from github.com)

Thanks @SadTransGirl, and a correction to my own first comment here, because I ran this branch's idea on my machine and the data does not support what I wrote.

I wrote that each retired texture the GPU wrote goes through flush_texture one at a time, so batching "should matter on this card too". That was an assumption from the sweep counts, not something I had checked in the log. I now have four sessions on an RX 7800 XT / Windows 11 / v2.0.11 with a hand port of this PR in my build (flush_record / flush_finish / flush_textures, the Evict struct carrying shifted, the n_dead / b_dead bookkeeping in the destroy loop, the review's two points below), about 18 to 60 minutes each:

the sweeps are frequent: one session has 2539 textures retired lines, 163574 textures in all, the largest 1365 in one sweep (the others: 374 sweeps / 9607 textures / max 614; 158 / 4594 / max 709; 115 / 15863 / max 824);
none of them wrote anything back: the textures the GPU drew, written back into their memory before eviction: N so far line, which prints at the 1st write-back and every 50th, never appears in any of the four logs. So, like your 4070 SUPER run, the batches were never exercised here either, and I cannot say anything about their effect. My guess that this sweep was behind my stutter was wrong.
no did not complete in time batch timeout (the log line I added for the first review point), no device lost.

I also have no slow submit lines to compare, because I did not run with RX_VK_SLOW_LOG; I can do that if it helps.

On the two points @hckrman101 says are still open, in case my port is useful as a reference (small, builds and runs here, though as said the batch path itself has not been hit):

break line: if ((!sweep && (pending_bytes >= total_bytes || total_bytes - pending_bytes <= budget / 8 * 7)) || frame_no - last < need_dead) break;
timeout: in flush_textures, after const bool gpu_ok = any && readback_submit_wait();, if any && !gpu_ok I log once and set f.buf = VK_NULL_HANDLE, f.alloc = nullptr for every Flush in the batch before flush_finish, so the staging buffers are not freed while the copies may still write into them (they stay allocated, a rare small leak).

I would not spend more of your time on this one on my account; if a card that writes back often shows up, that is the test.

Thanks @SadTransGirl, and a correction to my own first comment here, because I ran this branch's idea on my machine and the data does not support what I wrote. I wrote that each retired texture the GPU wrote goes through `flush_texture` one at a time, so batching "should matter on this card too". That was an assumption from the sweep counts, not something I had checked in the log. I now have four sessions on an RX 7800 XT / Windows 11 / v2.0.11 with a hand port of this PR in my build (`flush_record` / `flush_finish` / `flush_textures`, the `Evict` struct carrying `shifted`, the `n_dead` / `b_dead` bookkeeping in the destroy loop, the review's two points below), about 18 to 60 minutes each: - the sweeps are frequent: one session has 2539 `textures retired` lines, 163574 textures in all, the largest 1365 in one sweep (the others: 374 sweeps / 9607 textures / max 614; 158 / 4594 / max 709; 115 / 15863 / max 824); - **none of them wrote anything back**: the `textures the GPU drew, written back into their memory before eviction: N so far` line, which prints at the 1st write-back and every 50th, never appears in any of the four logs. So, like your 4070 SUPER run, the batches were never exercised here either, and I cannot say anything about their effect. My guess that this sweep was behind my stutter was wrong. - no `did not complete in time` batch timeout (the log line I added for the first review point), no device lost. I also have no `slow submit` lines to compare, because I did not run with `RX_VK_SLOW_LOG`; I can do that if it helps. On the two points @hckrman101 says are still open, in case my port is useful as a reference (small, builds and runs here, though as said the batch path itself has not been hit): - **break line:** `if ((!sweep && (pending_bytes >= total_bytes || total_bytes - pending_bytes <= budget / 8 * 7)) || frame_no - last < need_dead) break;` - **timeout:** in `flush_textures`, after `const bool gpu_ok = any && readback_submit_wait();`, if `any && !gpu_ok` I log once and set `f.buf = VK_NULL_HANDLE, f.alloc = nullptr` for every `Flush` in the batch before `flush_finish`, so the staging buffers are not freed while the copies may still write into them (they stay allocated, a rare small leak). I would not spend more of your time on this one on my account; if a card that writes back often shows up, that is the test.

SadTransGirl commented

2026-11-09 23:00:49 +00:00

(Migrated from github.com)

@hckrman101 the two open points and the nit are in 8a453c4:

Timeout: when the batch's read-back does not complete, the staging buffers are no longer freed in flush_finish. They go on the deferred-destroy list (dead, with rec_value + 1), so they are freed once a submission made after this one is done, the way a destroyed image is. A log line says the batch was not written back.
Wrap: the break line is now pending_bytes >= total_bytes || total_bytes - pending_bytes <= budget / 8 * 7.
Nit: the 512 MB cap counts guest_size.

vk_texture.cpp compiles and test_mem_trim, test_nvn_texture_handle and test_texture_init_barrier pass here.

@lucasmazzoni45-del thank you for running four sessions with a port and for writing up that the batches never ran. That matches my run, and I agree: the measured benefit is from the v2.0.5 base, and on current main neither of us has a sweep that writes back. Your two snippets are what went in, except that the buffers are handed to the deferred-destroy list rather than leaked.

@hckrman101 the two open points and the nit are in 8a453c4: - **Timeout:** when the batch's read-back does not complete, the staging buffers are no longer freed in `flush_finish`. They go on the deferred-destroy list (`dead`, with `rec_value + 1`), so they are freed once a submission made after this one is done, the way a destroyed image is. A log line says the batch was not written back. - **Wrap:** the break line is now `pending_bytes >= total_bytes || total_bytes - pending_bytes <= budget / 8 * 7`. - **Nit:** the 512 MB cap counts `guest_size`. `vk_texture.cpp` compiles and `test_mem_trim`, `test_nvn_texture_handle` and `test_texture_init_barrier` pass here. @lucasmazzoni45-del thank you for running four sessions with a port and for writing up that the batches never ran. That matches my run, and I agree: the measured benefit is from the v2.0.5 base, and on current main neither of us has a sweep that writes back. Your two snippets are what went in, except that the buffers are handed to the deferred-destroy list rather than leaked.

hckrman101 (Migrated from github.com) approved these changes

2026-11-09 23:08:38 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks, both open points and the nit are fixed in 8a453c4.

Timeout: handing the staging buffers to dead with rec_value + 1 is sound. texture_frame runs inside the present recording, so rec_value is a recording that hasn't been submitted yet. It goes onto the same queue after the read-back, and rec_value + 1 comes after that. A signal operation's first scope covers everything earlier in submission order, so the timeline reaching that value means the read-back's copies are done. If the read-back is really hung, nothing later ever signals and the buffers just stay allocated. That's a leak, not a use-after-free, which is the right way to fail. Nothing else is left half-done on timeout: the copies don't change layouts (GENERAL in, GENERAL out), and flush_finish with gpu_ok == false doesn't invalidate, unswizzle or gpu_write anything, so guest memory and the written-page state are untouched. The batch's textures are lost, same as a single one before.
Wrap: pending_bytes >= total_bytes || covers it.
Nit: the cap counts guest_size now.

One related gap (it was there before this PR too, so it doesn't block). The image the timed-out copies read from isn't deferred the same way. The sweep only picks textures with last_value <= completed, and destroy_later queues the image at tex.last_value, so the next texture_frame destroys it while the hung copy may still be reading it. Inline comment with a one-liner below.

Also pre-existing: after a timeout, the next readback_begin resets rb_pool and rb_fence while that submission may still be pending, which the spec doesn't allow. Same 5 s case, so it's for a follow-up.

On coverage: no test reaches flush_textures or the timeout branch, and from the comments neither the 4070 SUPER run nor lucasmazzoni45-del's four RX 7800 XT sessions wrote anything back during a sweep. So the batch path is reviewed but hasn't really been run on current main. CI is green. Approving.

Thanks, both open points and the nit are fixed in 8a453c4. - **Timeout:** handing the staging buffers to `dead` with `rec_value + 1` is sound. `texture_frame` runs inside the present recording, so `rec_value` is a recording that hasn't been submitted yet. It goes onto the same queue after the read-back, and `rec_value + 1` comes after that. A signal operation's first scope covers everything earlier in submission order, so the timeline reaching that value means the read-back's copies are done. If the read-back is really hung, nothing later ever signals and the buffers just stay allocated. That's a leak, not a use-after-free, which is the right way to fail. Nothing else is left half-done on timeout: the copies don't change layouts (GENERAL in, GENERAL out), and `flush_finish` with `gpu_ok == false` doesn't invalidate, unswizzle or `gpu_write` anything, so guest memory and the written-page state are untouched. The batch's textures are lost, same as a single one before. - **Wrap:** `pending_bytes >= total_bytes ||` covers it. - **Nit:** the cap counts `guest_size` now. One related gap (it was there before this PR too, so it doesn't block). The *image* the timed-out copies read from isn't deferred the same way. The sweep only picks textures with `last_value <= completed`, and `destroy_later` queues the image at `tex.last_value`, so the next `texture_frame` destroys it while the hung copy may still be reading it. Inline comment with a one-liner below. Also pre-existing: after a timeout, the next `readback_begin` resets `rb_pool` and `rb_fence` while that submission may still be pending, which the spec doesn't allow. Same 5 s case, so it's for a follow-up. On coverage: no test reaches `flush_textures` or the timeout branch, and from the comments neither the 4070 SUPER run nor lucasmazzoni45-del's four RX 7800 XT sessions wrote anything back during a sweep. So the batch path is reviewed but hasn't really been run on current main. CI is green. Approving.

gpu/vk/vk_texture.cpp

Suggestion for a follow-up (or here if it's easy): the textures in this batch also need to outlive the copies. They go through destroy_later with tex.last_value, which the sweep already checked is <= completed, so their images (and memory) get destroyed at the next texture_frame while the timed-out copy may still read them. Bumping it here would defer them to the same point as the buffers:

for (Flush& f : fs)
    if (f.ok) f.t->last_value = std::max(f.t->last_value, rec_value + 1);

(texture_flush_to_guest keeps its texture, so a higher last_value there just makes it look recently used.)

Suggestion for a follow-up (or here if it's easy): the textures in this batch also need to outlive the copies. They go through `destroy_later` with `tex.last_value`, which the sweep already checked is `<= completed`, so their images (and memory) get destroyed at the next `texture_frame` while the timed-out copy may still read them. Bumping it here would defer them to the same point as the buffers: ```cpp for (Flush& f : fs) if (f.ok) f.t->last_value = std::max(f.t->last_value, rec_value + 1); ``` (`texture_flush_to_guest` keeps its texture, so a higher `last_value` there just makes it look recently used.)

hckrman101 (Migrated from github.com) reviewed

2026-11-09 23:08:53 +00:00

gpu/vk/vk_texture.cpp

					
				@ -2525,0 +2558,4 @@

				        // that failed. The copies may still be writing into the staging buffers, so they are not freed here: they go the way a

				        // destroyed image does, once a submission made after this one is done. The batch's write-backs are lost, as a single

				        // one's was before)

				        static int logged;

Fixed in 8a453c4: the buffers go to dead at rec_value + 1, which is safe given queue order. The source image is still not deferred, see the new inline comment.

Fixed in 8a453c4: the buffers go to `dead` at `rec_value + 1`, which is safe given queue order. The source image is still not deferred, see the new inline comment.