[kafasam]

What

On Windows, AMD and Intel kept every retired texture pool's import for the whole session. This gives them back after an idle wait instead, so they stop accumulating. Nothing changes off Windows or on Windows NVIDIA.

Why

main's default per OS and vendor for a retired pool's import:

	main	this PR
Linux, any vendor	given back as soon as the GPU is past it (issues 312, 413)	same
Windows NVIDIA	given back as soon as the GPU is past it (issue 1057)	same
Windows AMD, Intel	kept for the session (device losses right after a give-back: issues 17, 309, 571)	given back after a wait

Kept, those retired pools only grow: multi-GB in a long session, until the system heap's budget is passed and new imports fail. RX_VK_KEEP_RETIRED=0 gives them back with no wait at all, which is the behaviour the device losses were reported against.

Changes

• The wait (Windows AMD and Intel only). A retired pool's import goes back once the GPU is past it (same gates as before) and it has been retired for RX_VK_RETIRE_SECS seconds (default 30), oldest first. This happens whatever the memory pressure.
• Pressure only shortens the wait. While the system heap is past RX_VK_IMPORT_HIGH percent of its budget (default 85, 0 turns it off), the wait is RX_VK_RETIRE_MIN_SECS seconds (default 2), until enough is queued to come down to RX_VK_IMPORT_LOW percent (75). Unlike imports_give_back's valve this one is on by default on Windows and is not limited to budgets under 12 GB, as it only frees pools nothing uses any more; the comments in both places say so.
• RX_VK_KEEP_RETIRED: 1 keeps them for the session; 0, off, false or no gives them back as soon as the GPU is past them; unset or empty is the default in the table. Applies to every OS and vendor, as before.
• The two valves no longer act on the same overshoot. What is queued in dead_imports is still in the driver's figure until the GPU is past it, so both valves count it as given back (only where retired imports wait; elsewhere imports_give_back reads what it read on main).
• The numbers are read with env_int, the on/off switch with env_on.
• The rule is in gpu/vk/retired_give_back.h with a check that needs no game or GPU: tests/test_retired_give_back.py.

Testing

• tests/test_retired_give_back.py passes (the defaults by OS and vendor, the wait and its floor, the watermarks, a 20-area tour kept / at once / waited, the two valves not asking twice); gpu/vk/vk_memory.cpp compiles for Windows with llvm-mingw.
• An earlier revision of the wait was run on an RX 6700 XT (12 GB) on Windows: 10 retired imports (704 MB) given back mid-session where main gave back none, no device loss. That was before this revision's changes and has not been repeated on it.
• Not tested: Windows Intel Arc, the AMD parts that lost the device (Ally X, RX 6600), and the pressure path (never crossed 85% on that machine). Linux is not expected to change, and has not been run.

AI assistance disclosure: the design and fix were developed with AI assistance.

[hckrman101]

Thanks for picking this up, the session-long accumulation on Windows is real and a grace period is a reasonable idea. A few things before it can go in:

The branch carries two other PRs. The diff has 3 commits: #1439 (FSR 1 EASU+RCAS, the taau shaders and the vk_draw.cpp changes) and #1905 (FR_READBACK, vk_replay.cpp), with this PR's own commit 8fb0290 on top. The description says one file, 44/10, but the PR is +1200/-69 over 7 files, and it is 921 commits behind main and conflicts (#1439 and #1905 are also dirty). Please rebase onto current main with only the vk_memory.cpp commit so it can be reviewed and merged on its own. CI hasn't run on it either.

Linux changes behaviour too, and the description doesn't mention it. On main, Linux already gives retired imports back as soon as the GPU is past them (keep is false there). That was the fix for the amdgpu userptr submit cost (issues 312/413: every retired import checked at each CS, 3-4 fps in Kokiri). With this PR Linux waits 30 s, so the retired pools from a burst of merges stay in every submission's BO list for that long again. I'd keep Linux at once by default and put the grace period on the Windows path only.

The NVIDIA part is already fixed on main. The description and the new comment say every vendor leaked, but main's Windows default already gives retired imports back on NVIDIA (vendorID != 0x10DE, issue 1057, the RTX 3070 case). What this PR really changes is the Windows default for AMD and Intel, where give-back was turned off on purpose after device losses (Arc B390 entering Kokiri every time, issue 17; Ally X 309; RX 6600 571). The idea that those losses came from giving back too soon is plausible, but nothing has tested it yet: the one test is an RX 6700 XT (which OS? on Linux main would already show retired imports given back, so I guess Windows), and the pressure path, which drops the wait to 2 s and is close to the old behaviour, was never exercised. This needs runs on Windows Intel Arc and on the AMD parts that lost the device before the default flips there, and I'd leave that call to NotRexed.

The pressure valve and imports_give_back fire on the same submission and both act on the same overshoot (inline). Freeing retired imports doesn't lower the heap usage until dead_imports is reaped, so imports_give_back reads the same used a moment later and evicts live idle pools for the same deficit. On Linux with a budget under 12 GB both valves are on by default.

Nits: the comment in imports_give_back ("Off by default on Windows, as giving retired imports back is") is out of date after this. RX_VK_IMPORT_HIGH now defaults to 85 for the retired valve on Windows and 0 for the live one, and the retired valve skips the budget < 12 GB gate. That may be what you want, but say so in the comment.

[hckrman101]

On Linux this used to be the whole default: keep false, so given back as soon as the GPU is past (issues 312/413, the amdgpu userptr CS cost). With idle_ms applied below, Linux now waits 30 s too. Suggest at_once defaulting to true off Windows, unless you've measured that the 30 s don't bring back the submit stall on RADV.

[hckrman101]

"every vendor leaked by it": on main NVIDIA was already excluded from keep (issue 1057), so only AMD and Intel on Windows leaked. Worth saying, since those are the vendors whose device losses the old default was protecting against.

[hckrman101]

This counter and imports_give_back's both run once per submission and reach % 16 on the same submission. What is pushed to dead_imports here isn't freed until the GPU is past it, so system_heap_usage returns the same used to imports_give_back right after, and it evicts live idle pools for the same want. Maybe pass the bytes queued here to imports_give_back and subtract them, or only probe in one place.

[kafasam]

You're right, and it was worse than one overshoot counted twice. Walking through it at 42ae0cd:

• memory_begin_submission runs reap_mirrors(), then retired_imports_give_back(), then imports_give_back(), all under pools_mu, once per submission.
• Each valve has its own static uint64_t calls, and each only counts when its own valve is enabled. Both conditions are fixed for the run (statics read once), so when both valves are on they count from the same first submission and stay in phase: they always hit % 16 == 0 on the same submission, not just sometimes.
• The retired valve moved the pool's buffer and memory into dead_imports with value = max(mem_rec_value, last_value). Nothing is freed there. vkFreeMemory only happens in reap_mirrors() once mem_completed reaches that value, and reap_mirrors() has already run for this submission, so the earliest free is the next submission, and only if the GPU has finished this one.
• system_heap_usage is the driver's figure (VK_EXT_memory_budget), so it cannot drop before that free. imports_give_back read the same used a few lines later, computed the same want, and evicted live idle pools for bytes that were already on their way out.
• The same staleness also hit the retired valve itself: if the queued imports were not reaped within 16 submissions (a long GPU frame), its next probe asked for the same bytes again and shortened the wait for more pools than needed.

And as you said, at 42ae0cd this was on by default on Linux under a 12 GB budget, since the retired valve had no OS split.

What the new head (4808b1c) does, taking your first suggestion:

• dead_import_bytes counts what sits in dead_imports: added at the push, subtracted in reap_mirrors() at the actual free.
• The retired valve probes want(settled(used, dead_import_bytes), ...), so what it queued earlier counts as given back until it really is.
• imports_give_back does used = settled(used, dead_import_bytes) before its watermark test, so it only evicts live pools for what the queued retired imports will not cover.
• Off Windows the retired valve no longer probes at all (retired imports go back at every submission again, as on main), and imports_give_back skips the subtraction there, so Linux reads exactly what it read on main. The interplay is left only on Windows AMD/Intel with RX_VK_IMPORT_HIGH set by hand (the live valve defaults to off there), and that is the case the subtraction covers.

I kept two probes rather than one because the two valves have different gates on purpose (the live one is off on Windows and limited to budgets under 12 GB, the retired one is neither; the comments in both now say so). tests/retired_give_back_test.cpp has the case: 1 GB over, three pools queued, and the next probe with the same used asks for nothing.

[kafasam]

Rebased as a single commit off current main (42ae0cd), unstacked per review: only gpu/vk/vk_memory.cpp and the leak's test fixture. Recap of the eviction policy: retired pools return to the driver only when the GPU has been quiet, the pool has been idle at least 2 s (RX_VK_RETIRE_MIN_SECS, default 2000 ms), and committed memory pressure has crossed 85% (relaxing back to 75% once under, so pools don't ping-pong).

[kafasam]

That rebase landed as of this morning: the branch is now a single commit (42ae0cd) off current main — only gpu/vk/vk_memory.cpp and the leak's test fixture, +44/-10. The two stacked PRs you saw are gone (they live in #1439, itself rebased to main, and #1905 which closed in favour of #1276). The hysteresis numbers in the recap comment are corrected too: idle floor 2 s (RX_VK_RETIRE_MIN_SECS, default 2000 ms), pressure 85% with the 75% relax.

[hckrman101]

Thanks for the rebase. The branch is now the one vk_memory.cpp commit on top of main and mergeable, so the stacking point is fixed.

The code itself is unchanged, though: the vk_memory.cpp diff at 42ae0cd is line for line the same as 8fb0290, so the rest of the last review still applies:

• Linux now waits 30 s before giving retired imports back, where main gives them back once the GPU is past them (the amdgpu userptr submit cost, issues 312/413). Still open, see the inline thread: I'd keep at_once the default off Windows.
• The two pressure valves act on the same overshoot on the same 16th submission (imports_give_back reads the same used before dead_imports is reaped and evicts live idle pools too). Still open, inline thread.
• NVIDIA was already given back on main (issue 1057), so the real change is the Windows default on AMD and Intel, where give-back was turned off after device losses (issues 17, 309, 571). That still has no run on Windows Arc or on those AMD parts, and the pressure path was never exercised. I'd still leave flipping that default to @NotRexed.
• Nits from last time (the stale "Off by default on Windows" comment in imports_give_back, the RX_VK_IMPORT_HIGH default split and the 12 GB gate) are untouched.

New:

• The comments here don't describe the code. The recap says a retired pool goes back only when the heap has crossed 85%; the code gives every retired pool back after 30 s whatever the pressure, and 85% only shortens that wait to 2 s. The commit message says "the keep= default stays true" and "idle >5s", but the code drops the Windows keep default and uses 2 s. You also mention a test fixture for the leak, but the PR has only vk_memory.cpp. Please make the description and the commit message match what the code does, and add the test if you meant to.
• CI hasn't run. The build and test workflows are waiting for a maintainer to approve them (first-time contributor), and the PR title check fails: it needs a conventional type, e.g. fix(vk): give retired pools back after an idle grace period.
• Overlap: #2154 also edits vk_memory.cpp (new fields next to retired_epoch in struct Pool, and mirrors_give_back), so whichever lands second needs a small rebase. #2198, #2368 and #2401 are in the texture cache and replay, not this path.

[Joey-Magnus]

I checked the head (42ae0cd) against main's retired_imports_give_back in gpu/vk/vk_memory.cpp. The code is the same as in hckrman101's last two reviews, so those points are all still open:

• Linux changes too. On main, keep is false off Windows, so retired imports go back as soon as the GPU is past them (the amdgpu submit cost, issues 312/413). Here at_once is true only with RX_VK_KEEP_RETIRED=0, so by default Linux now waits 30 s.
• The real change is the Windows default on AMD and Intel, where give-back was turned off after device losses (issues 17, 309, 571). There's still no run on Arc or on those AMD parts, and the 2 s pressure path has never run.
• The commit message ("keep= default stays true", "idle >5s") and the recap comment don't match the code. The test fixture the comment mentions isn't in the PR.

Smaller things: the three new number switches use atoi(getenv(...)). Main has env_int in gpu/vk/compat/hookshot_env.h for this. at_once only recognises a literal 0, so RX_VK_KEEP_RETIRED=off keeps the 30 s wait. The title still needs a conventional type, e.g. fix(vk): ....

[kafasam]

Pushed 4808b1c: still one commit, now on current main (ef81aee). This time the code changed, going through both reviews:

Linux is back to main's behaviour. Off Windows a retired import goes back as soon as the GPU is past it, by default, exactly as on main: same gates, no wait, and the pressure probe is not even run there. The wait applies only where main's default was to keep them, Windows AMD and Intel. Windows NVIDIA stays at once too (issue 1057).

RX_VK_KEEP_RETIRED is read with env_on: 1 keeps for the session; 0, off, false or no gives back at once; unset or empty is the OS/vendor default. The numbers (RX_VK_RETIRE_SECS, RX_VK_RETIRE_MIN_SECS, RX_VK_IMPORT_HIGH, RX_VK_IMPORT_LOW) are read with env_int.

The two valves and the same overshoot (inline thread, answered there): what is queued in dead_imports is now counted as given back by both valves until the GPU is past it, so imports_give_back no longer evicts live pools for the same bytes.

Comments, commit message, title and description now say what the code does. Only AMD and Intel on Windows leaked (NVIDIA was already excluded on main); a retired pool goes back after 30 s whatever the pressure, and 85% only shortens that to 2 s (not 5); the Windows keep default is what goes away for those two vendors. The stale "Off by default on Windows, as giving retired imports back is" comment in imports_give_back is rewritten, and both places now say that the retired valve defaults to 85 on Windows and skips the 12 GB gate, and why. Title: fix(vk): .... My two earlier recap comments here were wrong on these points; the description is the accurate one now.

The test fixture. My earlier comment mentioned one that was never in the PR, and there was none to bring over. It is written now: the rule moved to gpu/vk/retired_give_back.h, checked by tests/test_retired_give_back.py + tests/retired_give_back_test.cpp (no game or GPU).

Diff against main: 4 files, +288/-13 (vk_memory.cpp +62/-13, the rest is the header and the test).

Still not done, and unchanged by this push: no run on Windows Intel Arc or on the AMD parts that lost the device (Ally X, RX 6600), and the pressure path has not fired on real hardware. The RX 6700 XT run in the description was of the earlier revision. Whether the default should flip for those two vendors without those runs is yours and @NotRexed's call; RX_VK_KEEP_RETIRED=1 gives main's behaviour back.

Overlap with #2154 noted: whichever lands second, I'll rebase.