Repository navigation
fix(vk): recover presentation after surface loss - #1995
Conversation
hckrman101
left a comment
There was a problem hiding this comment.
Thanks. This is still a draft, so I'm leaving a comment rather than an approval. I read it at d526a52. There are no earlier reviews or comments.
The recovery logic looks right to me. I checked these points:
- Destroy order.
retire_swapchain()drains the device (device_wait_idle_locked, which also takespresent_queue_mu, so an in-flight present on the dedicated queue has returned). Then it drops the views and theswap_renderedsemaphores, and destroys the chain. Only after that doesinvalidate_lost_surface()destroy the surface. Increate_swapchainthe old chain is still retired aftervkCreateSwapchainKHRbefore the surface goes, so the spec's "swapchains before their surface" holds on every path, including loss during creation. - Semaphores. A lost acquire leaves
f.acquiredunsignalled andf.doneun-reset, so both can be reused. Every successful acquire still reaches the submit that waits onf.acquired. I found no new early return between acquire and submit. The new swap_spare return is before the acquire. A present that fails with SURFACE_LOST still runs its semaphore wait, and theswap_renderedsemaphores of a rejected deferred frame belong to the chain being retired, so they are destroyed after the idle. - Frame generation. The generation counter closes the handle-reuse hole in
present_commit.swap_sparebecoming atomic fixes the display thread's unlocked read. The< 2recheck inpresent_imagesits after the recreate, so a pair that was chosen against the old chain cannot over-acquire on a smaller one. That pair is dropped (usedstays false) and the next set falls back to ordinary presents. - Locking. The lock order stays
present_image_mu→present_mu→present_queue_mu.dev.surfaceandpresent_queue_supportedare only touched in vk_main.cpp/vk_replay.cpp underpresent_mu, and nothing outside those files reads the surface orpresent_family. Not resettingpresent_queue/present_familyinchoose_present_queueis fine, because every user now goes throughpresent_queue_supported. - Tests. The source slices in
test_present_semaphores.py(acquire block up to#ifndef _WIN32, present block up toif (res < 0),test_antilag.pyandtest_vk_swapchain_compat.pymatch the head source. They compile the realretire_swapchain/invalidate_lost_surface/present_commitrather than copies.
Things to sort out before it leaves draft:
- Description. It opens with what the change does, not what goes wrong today. Please start with the problem in one plain sentence, e.g. "After
VK_ERROR_SURFACE_LOST_KHRthe picture stopped for good: nothing replaced the surface (#1977)." Then the rest as it is. - Reach and testing. Neither Windows minimize/monitor changes nor a live Wayland/X11 window normally produces SURFACE_LOST; they give OUT_OF_DATE or a zero extent, which main already handled. So the in-game run (maximize/restore/minimize) shows that the refactor didn't break the usual paths, not that recovery works. That's fine for a low-reach S1, but please say it plainly in the description. If you can trigger a real loss somewhere, that would be worth a line: on Linux, e.g. a compositor restart under KWin's Wayland session recovery, or
SDL_VIDEODRIVER=x11with a remapped window. Linux hasn't been run in the game at all. Also, on Linux the recreate now callscreate_surfacefrom whichever thread presents (display/painter) rather than the thread that calledstart(). I haven't seen that fail with SDL on Wayland/X11, but a Linux run would cover it. - Log spam while the window can't be recreated (inline).
- Small behaviour change, not a problem:
present_commitnow returns false for an OUT_OF_DATE present too, so the frame overlay and pacing stats leave those frames out. I think that's more correct, but it's worth a line in the description.
CI: everything has passed except Python source checks (windows-2022), which is still pending. The branch is 2 commits behind main and merges cleanly. Nothing in vk_api.h changed, so no API bump is needed.
| #else | ||
| const MkwVkWindow* w = prep_window; | ||
| if (!w || !w->create_surface(w->window, (void*)dev.instance, (void*)&surface)) { | ||
| logf("vk: SDL_Vulkan_CreateSurface failed; check SDL_VIDEODRIVER and the Vulkan window-system extensions"); |
There was a problem hiding this comment.
During recovery this runs every 250 ms from create_swapchain for as long as the surface can't be made, e.g. a window that is going away, or a compositor that's gone. This log and the present-support one below would then fill the log at 4 lines a second. Could they be capped like invalidate_lost_surface's (logged++ < 8), or logged only from start()?
| if (!qk.owns_lock()) qk.lock(); | ||
| create_swapchain(dev.swap_extent.width, dev.swap_extent.height); | ||
| } | ||
| return res == VK_SUCCESS || res == VK_SUBOPTIMAL_KHR; |
There was a problem hiding this comment.
Nit: this also returns false for VK_ERROR_OUT_OF_DATE_KHR. Before, present_commit returned true there, so the deferred path now leaves an out-of-date present out of the overlay counts and hist. That's probably what we want (the frame wasn't shown), just worth noting in the description since it isn't about surface loss.
d526a52 to
c5aec41
Compare
|
Thanks for the review. The previous Windows source run matched the exact upstream base: 322 passed, 2 failed, 21 skipped, with the same log timing failure and skeleton bake timeout. The three new fork workflows are linked in the Validation table for |
c5aec41 to
cda37c2
Compare
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, I re-read it at cda37c2. The branch was rebased onto main, so I compared the old and new patches file by file. The only code change since d526a52 is the log fix. The rest of the patch is the same as what I checked last time.
All my earlier points are sorted:
- Description. It now opens with the problem (the picture could stop after a lost surface).
- Reach and testing. It now says that the maximize/restore/minimize run only exercised ordinary window transitions, that surface loss was injected, and that native driver loss and Linux gameplay are untested. That's the honest framing for a low-reach S1. The Linux point about
create_surfacerunning on the presenting thread is still only covered by the WSLg probe, not by a game run. It isn't a blocker, but it would be the first thing to look at if someone on Linux reports a problem after this lands. - Log spam.
open_window_surface(bool log_failure)logs only whenstart()calls it. The 250 ms retry increate_swapchainpassesfalse, so a window that can't be recreated stays quiet apart from the capped surface-lost line (8 at most).choose_present_queuestill logs, but only after a surface was actually made, so that's once per recovery.test_vk_swapchain_compat.pynow checks that both diagnostics are guarded and that the retry passesfalse. - OUT_OF_DATE accounting. This is noted in the description.
CI is green on the head, including both Python source runs. It merges cleanly and vk_api.h is untouched. Looks good to me.
01147b2 to
8c6b593
Compare
Refs NotRexed#1977 Assisted-by: GPT-6 (Codex)
Refs NotRexed#1977 Assisted-by: GPT-6 (Codex)
Refs NotRexed#1977 Assisted-by: GPT-6 (Codex)
Assisted-by: GPT-6 (OpenAI)
8c6b593 to
b52079d
Compare
hckrman101
left a comment
There was a problem hiding this comment.
Re-read at b52079d. This is a rebase onto main (49f75dc) with no code changes: I compared the old and new patches file by file against cda37c2, and all ten files are identical. My approval at cda37c2 still holds. The log spam fix, the problem-first description, the honest testing scope and the OUT_OF_DATE accounting note are all still in place. The test_recorder_sync.py hunk still applies on top of main's ordering fix.
CI: the source checks (Linux and Windows, both parts), the Windows builder and the title check pass. The AppImage job was cancelled at the 1 h limit in its "System packages" step, before anything was built. That is the runner, not this PR, and main's AppImage runs are red/cancelled right now too. A re-run should clear it. It merges cleanly and vk_api.h is untouched.
Same caveat as before: on Linux, recovery calls create_surface from the presenting thread, and that is only covered by the WSLg probe. Looks good to me.
…#2354) **What this is for:** frame times from headless runs, the kind scripted tests and measurements use. `RX_VK_FRAME_LOG=<csv>` ("A CSV line per present: frame, seconds, frame time in ms", docs/development.md) writes nothing in a headless run (`HOS_HEADLESS`): the file is never even created. Fixes #2353. **What is measured:** on Linux (RTX 4090, NVIDIA 615.78.08) a headless run with the change wrote 6480 lines over 108.9 s, and its first interval equals the game's own present trace of the same run; none of 23 headless runs of main left a file; a game-free test that fails on main; the whole of the checks. **Not measured:** a windowed run with the change (that path calls the same code at the same place as before), Windows game runtime, AMD, the Steam Deck under gamescope. ## What changed - `gpu/vk/vk_replay.cpp`: the writer was only in `present_out`, after `vkQueuePresentKHR`. Without a window a present copies the picture into the offscreen image and `do_present` returns before `present_image`, so it never got there. The writer is now `frame_log_present()`, unchanged (same file, header, columns and flush every 60 lines), called where it was and at the end of a headless `do_present`, once the copy into the offscreen image is submitted. - Frame generation is off without a window (`framegen_on`), so a headless run writes one line per game frame, as a window does without frame generation. - The times are the same steady clock as before. A headless run has no swapchain to wait on, so they show the game's and the replay's pace without the display's (V-Sync, the compositor). - `tests/test_frame_log_headless.py` (game-free): compiles the actual `frame_log_present` and checks its CSV header, sequence numbers, elapsed/interval times, and disabled output. It does not infer execution from call-site spelling. - `docs/development.md`: documents headless CPU intervals and the absence of swapchain/display waits. ## How it was tested Latest refresh, 2026-11-11: head `e3119ce0` on `47ed9f14`, including the merged #2397 Windows ASTC fixture repair. The `present_native_pq` conflict was resolved by retaining main’s moved declaration and this PR’s logger. The compiled CSV writer passes with GCC and Clang. No new game run or full local suite was made for this refresh; [Source-regression CI](https://github.com/NotRexed/Hookshot/actions/runs/43093270209) now passes on this head: both Windows and both Ubuntu shards. RTX 4090, NVIDIA 615.78.08, Arch Linux; headless (`HOS_HEADLESS=1`, the log says `vk: no window: presents render offscreen (1920x1080)`), from a fresh profile, no input, 120 s, muted, `RX_VK_FRAME_LOG` and `HOS_PRESENT_TRACE` set. - Main: none of 23 such headless runs left a frame log (the switch was set in every one). - With this change: `frames.csv` holds the header and 6480 lines, frames 0 to 6479 over 108.9 s, times rising from 0; the frame times average 16.80 ms (59.5 fps; the last 30 s 59.9 fps) with the game at its 60 cap through the intro movie and the title screen (pictures of the run looked at). The game's own present trace of the same run has the same first interval (43.28 ms) and 6432 presents over 108.1 s. (Not a frame-rate claim: other work ran beside it.) - That run was of this change on main bdfe09c (commit 7da36252). Rebased onto 49f75dc the change is the same apart from one blank line. Added 2026-11-11 16:27 UTC: the rebased build (cc36308 on 49f75dc, `hookshot v2.1.0-rc.3-54-gcc363089`) ran the same way, 150 s: `frames.csv` holds the header and 5880 lines, frames 0 to 5879 over 98.4 s, times rising from 0; its first interval (32.909 ms) equals the game's own present trace of that run (32,908,677 ns); no FATAL or device-lost line, no driver line in the kernel log. (Load average 22 falling to 9: not a frame-rate claim.) - Updated 2026-11-11: rebased onto `7759a6f3`, head `5ce5970f`. #1995 changed `present_out` to return `bool`; the old test pinned its full signature. The source-text call-site assertions are removed rather than repinned. The compiled writer check passes with GCC 16.2.1 and Clang 23.1.3: `PASS: frame log CSV header, sequence, elapsed/interval times, and disabled output`. Against main it fails because main has no extracted `frame_log_present` helper; that is not a headless integration test. Headless integration evidence is the two older game runs above. - On `5ce5970f`, `tests/run_all.py`: `385 passed, 2 failed, 10 skipped in 141 s`. `test_updater_asset_digest.py` cannot resolve the .NET 8 reference packs on this machine. `nx2/test_texture_phi_dominance.py` had a compiler killed by SIGKILL; its narrow rerun alone passed (`1 passed, 0 failed, 0 skipped in 20 s`). No second full suite was run. - Earlier suite on `cc363089` (load 57 to 113), `python -I -B tests/run_all.py`: `362 passed, 3 failed, 10 skipped in 193 s (linux, 32 at once)`. Two of the failures are this machine's and fail on main as well, each with a pull request of its own: `tests/test_updater_asset_digest.py` (only a .NET 10 SDK here, #2294) and `tests/test_wayland_libdecor_hint.py` (Clang 23, #2290). The third, `tests/test_display_pointer.py`, failed under that load at `reads > 0` (its reader thread had made no read yet when the check ended) and passes when run alone (3 of 3, at load 55). Added 2026-11-11: [Windows part 2/2](https://github.com/NotRexed/Hookshot/actions/runs/43072060527/job/130702262513) passes `tests/test_frame_log_headless.py`. The job fails only on the unrelated ASTC fixture’s `scanf` deprecation, fixed by #2397, which has now merged and is included in the latest refresh. This is Windows coverage of the compiled writer, not a game run. ## Not tested - No new game build/run after the subsequent rebases, including `47ed9f14`; the game runs above name their earlier heads. The renderer patch remains the same; #1995's surface-loss return stays before the existing window log position. - A windowed run of this branch: the window's path calls the same function at the same place as before; not run here. - Windows game runtime, AMD, the Steam Deck under gamescope (the switch's first use). ## Risks - A windowed run writes exactly what it wrote: the same function is called at the same place in `present_out`. - A headless line is written when the offscreen copy is submitted, not when a display shows it: there is no display. Its times are the game's and the replay's pace. Nobody should read them as presentation times, which the description of the switch does not promise either. - The file is opened at the first present, as before; a run without the switch pays one `getenv` at its first present and a null check after that. ## Overlaps Updated 2026-11-11 after the refresh: 49 open PRs touch one of this branch’s three files; one additional text conflict was reported, and its hunk was read. - #2325 (frame critical-path recorder / asynchronous presenter): its presenter-specific surface-loss and deferred-resize handling meets the old inline frame logger in `present_out`. Keep that handling and call the extracted logger once at the corresponding present boundary; do not restore a second inline writer. No combined runtime test was made. - The earlier `present_native_pq` declaration conflict is resolved in this rebase. Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <noreply@openai.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: GPT-6 Astra <noreply@openai.com>
Summary
After Vulkan reported a lost window surface, the picture could stop indefinitely. The renderer now retires the old swapchain and retries with a replacement surface after loss during acquire, present or swapchain setup.
Closes #1977.
Technical changes
Validation
Current head:
b52079d5, rebased onto main49f75dc8(recorder test ordering fix). All authored commits are unchanged by range-diff. Fresh hosted checks run on this head; the local and native results below remain tied to their earlier tested heads.Earlier local and native evidence:
Earlier head:
8c6b593d, rebased onto mainebbba2ea. Focused Windows checks: 4 passed / 1 failed. Swapchain recovery, presentation semaphores, Anti-Lag and the new FPS statistics pass. The recorder check fails its existing destructor negative control on both this head and the exact clean base. Earlier native evidence remains labeled below.cda37c2, the upstream source suite passed with 323 passed / 0 failed / 24 skipped on Windows and 337 / 0 / 10 on Linux. Windows includes the temporary bake skips from test: log_batch "off" takes the fastest of three; bake tests skip on Windows CI (600 s limit) #2013.Earlier native smoke test and Linux surface probe
cc80fde(API 14), a Windows RTX 5070 Ti run logged 6,688 presents at 2× frame generation with pacing off. The Kokiri scene returned after maximize, restore and minimize, and the game exited normally.