MagicBeanDev/Hookshot

Archived

Watch

Fork 0

fix(vk): recover presentation after surface loss #1995

Merged

Arkex-ai merged 4 commits from fix/vulkan-surface-loss into main

2026-11-11 17:05:58 +00:00

Arkex-ai commented

2026-11-10 23:01:38 +00:00

(Migrated from github.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

Drain pending GPU work and retire the old swapchain, views and semaphores before destroying the surface.
Reuse the startup surface factory on retry. Wait for a drawable window, limit surface creation attempts, recheck presentation support and keep startup error messages out of repeated recovery attempts.
Track swapchain generation with deferred frames so an old frame cannot reach a replacement chain even if the driver reuses its handle.
Recheck spare-image capacity after replacement so a 2× deferred pair cannot acquire on a chain that only has room for ordinary presentation. Deferred out-of-date presents also no longer count as shown frames in the overlay or pacing history.

Validation

Current head: b52079d5, rebased onto main 49f75dc8 (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 main ebbba2ea. 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.

At 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 #2013.
The Windows and Linux regression fixtures inject surface loss during acquire, present and swapchain creation. They verify resource destruction order, reject deferred frames when the replacement swapchain reuses the old handle, and check recovery with a smaller replacement chain.
Native driver surface loss and Linux gameplay remain untested.

Earlier native smoke test and Linux surface probe

At 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.
The standalone WSLg probe recreated Vulkan surfaces 8 times on X11 and 8 times on Wayland using SDL2 and lavapipe. Each replacement supported presentation and returned valid capabilities. The probe did not exercise SDL3 or swapchain presentation.

Current-head workflow	Result
Linux AppImage	Needs attention
Game-free source regressions	Passed
PR title	Passed
Windows builder	Passed

## 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 - Drain pending GPU work and retire the old swapchain, views and semaphores before destroying the surface. - Reuse the startup surface factory on retry. Wait for a drawable window, limit surface creation attempts, recheck presentation support and keep startup error messages out of repeated recovery attempts. - Track swapchain generation with deferred frames so an old frame cannot reach a replacement chain even if the driver reuses its handle. - Recheck spare-image capacity after replacement so a 2× deferred pair cannot acquire on a chain that only has room for ordinary presentation. Deferred out-of-date presents also no longer count as shown frames in the overlay or pacing history. ## Validation Current head: `b52079d5`, rebased onto main `49f75dc8` (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 main `ebbba2ea`. 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. - At `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 #2013. - The Windows and Linux regression fixtures inject surface loss during acquire, present and swapchain creation. They verify resource destruction order, reject deferred frames when the replacement swapchain reuses the old handle, and check recovery with a smaller replacement chain. - Native driver surface loss and Linux gameplay remain untested. <details> <summary>Earlier native smoke test and Linux surface probe</summary> - At `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. - The standalone WSLg probe recreated Vulkan surfaces **8 times on X11 and 8 times on Wayland** using SDL2 and lavapipe. Each replacement supported presentation and returned valid capabilities. The probe did not exercise SDL3 or swapchain presentation. </details>  | Current-head workflow | Result | |---|---| | [Linux AppImage](https://github.com/NotRexed/Hookshot/actions/runs/43022789108) | Needs attention | | [Game-free source regressions](https://github.com/NotRexed/Hookshot/actions/runs/43022789062) | Passed | | [PR title](https://github.com/NotRexed/Hookshot/actions/runs/43022784549) | Passed | | [Windows builder](https://github.com/NotRexed/Hookshot/actions/runs/43022789141) | Passed |

hckrman101 (Migrated from github.com) reviewed

2026-11-10 23:18:37 +00:00

hckrman101 (Migrated from github.com) left a 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 takes present_queue_mu, so an in-flight present on the dedicated queue has returned). Then it drops the views and the swap_rendered semaphores, and destroys the chain. Only after that does invalidate_lost_surface() destroy the surface. In create_swapchain the old chain is still retired after vkCreateSwapchainKHR before 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.acquired unsignalled and f.done un-reset, so both can be reused. Every successful acquire still reaches the submit that waits on f.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 the swap_rendered semaphores 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_spare becoming atomic fixes the display thread's unlocked read. The < 2 recheck in present_image sits after the recreate, so a pair that was chosen against the old chain cannot over-acquire on a smaller one. That pair is dropped (used stays false) and the next set falls back to ordinary presents.
Locking. The lock order stays present_image_mu → present_mu → present_queue_mu. dev.surface and present_queue_supported are only touched in vk_main.cpp/vk_replay.cpp under present_mu, and nothing outside those files reads the surface or present_family. Not resetting present_queue/present_family in choose_present_queue is fine, because every user now goes through present_queue_supported.
Tests. The source slices in test_present_semaphores.py (acquire block up to #ifndef _WIN32, present block up to if (res < 0), test_antilag.py and test_vk_swapchain_compat.py match the head source. They compile the real retire_swapchain/invalidate_lost_surface/present_commit rather 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_KHR the 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=x11 with a remapped window. Linux hasn't been run in the game at all. Also, on Linux the recreate now calls create_surface from whichever thread presents (display/painter) rather than the thread that called start(). 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_commit now 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.

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 takes `present_queue_mu`, so an in-flight present on the dedicated queue has returned). Then it drops the views and the `swap_rendered` semaphores, and destroys the chain. Only after that does `invalidate_lost_surface()` destroy the surface. In `create_swapchain` the old chain is still retired after `vkCreateSwapchainKHR` before 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.acquired` unsignalled and `f.done` un-reset, so both can be reused. Every successful acquire still reaches the submit that waits on `f.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 the `swap_rendered` semaphores 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_spare` becoming atomic fixes the display thread's unlocked read. The `< 2` recheck in `present_image` sits after the recreate, so a pair that was chosen against the old chain cannot over-acquire on a smaller one. That pair is dropped (`used` stays false) and the next set falls back to ordinary presents. - **Locking.** The lock order stays `present_image_mu` → `present_mu` → `present_queue_mu`. `dev.surface` and `present_queue_supported` are only touched in vk_main.cpp/vk_replay.cpp under `present_mu`, and nothing outside those files reads the surface or `present_family`. Not resetting `present_queue`/`present_family` in `choose_present_queue` is fine, because every user now goes through `present_queue_supported`. - **Tests.** The source slices in `test_present_semaphores.py` (acquire block up to `#ifndef _WIN32`, present block up to `if (res < 0`), `test_antilag.py` and `test_vk_swapchain_compat.py` match the head source. They compile the real `retire_swapchain`/`invalidate_lost_surface`/`present_commit` rather than copies. Things to sort out before it leaves draft: 1. **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_KHR` the picture stopped for good: nothing replaced the surface (#1977)." Then the rest as it is. 2. **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=x11` with a remapped window. Linux hasn't been run in the game at all. Also, on Linux the recreate now calls `create_surface` from whichever thread presents (display/painter) rather than the thread that called `start()`. I haven't seen that fail with SDL on Wayland/X11, but a Linux run would cover it. 3. **Log spam** while the window can't be recreated (inline). 4. Small behaviour change, not a problem: `present_commit` now 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.

gpu/vk/vk_main.cpp

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()?

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()`?

gpu/vk/vk_replay.cpp

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.

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.

Arkex-ai commented

2026-11-10 23:29:53 +00:00

(Migrated from github.com)

Thanks for the review. c5aec41 keeps both surface startup diagnostics at startup only, so a failed 250 ms retry won't print them repeatedly. The existing capped surface-loss message remains. I also rewrote the opening, noted the deferred OUT_OF_DATE accounting, and clarified that the native resize smoke test exercised ordinary window transitions. Surface loss itself was injected. Linux gameplay and driver-reported native loss are still untested.

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.

Thanks for the review. `c5aec41` keeps both surface startup diagnostics at startup only, so a failed 250 ms retry won't print them repeatedly. The existing capped surface-loss message remains. I also rewrote the opening, noted the deferred OUT_OF_DATE accounting, and clarified that the native resize smoke test exercised ordinary window transitions. Surface loss itself was injected. Linux gameplay and driver-reported native loss are still untested. The previous Windows source run matched the [exact upstream base](https://github.com/NotRexed/Hookshot/actions/runs/42916504394): 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`.

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 02:16:08 +00:00

hckrman101 (Migrated from github.com) left a comment

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_surface running 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 when start() calls it. The 250 ms retry in create_swapchain passes false, so a window that can't be recreated stays quiet apart from the capped surface-lost line (8 at most). choose_present_queue still logs, but only after a surface was actually made, so that's once per recovery. test_vk_swapchain_compat.py now checks that both diagnostics are guarded and that the retry passes false.
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.

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: 1. **Description.** It now opens with the problem (the picture could stop after a lost surface). 2. **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_surface` running 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. 3. **Log spam.** `open_window_surface(bool log_failure)` logs only when `start()` calls it. The 250 ms retry in `create_swapchain` passes `false`, so a window that can't be recreated stays quiet apart from the capped surface-lost line (8 at most). `choose_present_queue` still logs, but only after a surface was actually made, so that's once per recovery. `test_vk_swapchain_compat.py` now checks that both diagnostics are guarded and that the retry passes `false`. 4. **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.

hckrman101 (Migrated from github.com) reviewed

2026-11-11 02:16:18 +00:00

gpu/vk/vk_main.cpp

Fixed in c5aec41 (now cda37c2): the retry passes log_failure=false, so only start() logs these, and the test checks it. Thanks.

Fixed in c5aec41 (now cda37c2): the retry passes `log_failure=false`, so only `start()` logs these, and the test checks it. Thanks.

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 16:49:49 +00:00

hckrman101 (Migrated from github.com) left a 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.

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.