fix(vk): a recording without a draw stream is submitted instead of crashing #552
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
beanstalk: approved @ 1697603
beanstalk: approved @ 2c5a62e
beanstalk: approved @ 4fd82b7
beanstalk: approved @ 64d6d09
beanstalk: approved @ 6f91fae
beanstalk: approved @ 70a78f9
beanstalk: approved @ 8d361dc
beanstalk: approved @ 9d8356e
beanstalk: approved @ abf2227
beanstalk: approved @ d82f202
beanstalk: approved @ e5f1e40
beanstalk: approved @ ee8ab5f
ci: approved
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/macos
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
regression
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
ci: approved
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!552
Loading…
Reference in a new issue
No description provided.
Delete branch "zeldaootr/Longshot:draw-flush-empty-stream"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Type: fix · Risk: low · Tested on: Linux x86-64 (game-free); Steam Deck OLED, SteamOS 3.9.5, RADV (a tester's run) · Needs: none
Why
When the GPU's host-visible memory ran out right at the start of a frame's recording, the renderer then read past an empty list while finishing that recording and could crash the game.
How to reproduce
Seen on: Linux x86-64, game-free (the real
draw_flush_recordingwith a stand-in buffer maker); not reproducible on demand in the game, since it needs host-visible memory to run out at a recording's startSettings and switches: defaults
Steps: on
mainaa95d35fwith this PR's test case added (d6ec2f17), runpython -I -B tests/test_draw_stream_visibility.py --cxx g++ --headers <Vulkan-Headers/include>: itsfirst-stream-failurecase makes the slot's first stream buffer failExpected: the recording is flushed with nothing to flush and submitted
Actual:
fixture.exe first-stream-failure coherent died with <Signals.SIGSEGV: 11>: the flush readsr.streams[0]of an empty vectorWhat changed
Finishing a recording now flushes only the streams the slot has, so a recording whose first stream could not be made is submitted without its draws instead of crashing.
draw_flush_recording()ingpu/vk/vk_draw.cppstops at the streams the slot has: whendraw_begin_recordingcould not make the slot's first stream, there are none, and the loop used to readr.streams[0]of an empty vector.tests/draw_stream_visibility_test.cppgets afirst-stream-failurecase (the first stream's buffer cannot be made; the recording must submit with nothing to flush), andtests/test_draw_stream_visibility.pyruns it.Measurements
Verdict: fixed in the game-free test (crashes on
main, passes with this PR, two interleaved rounds); no regression on a tester's Steam Deck (no crash or fault, fps 20.6 and 21.2 against main's 19.9 and 20.2).Hardware: Linux x86-64 server (Ubuntu 24.04), no GPU or game used; gcc 13.3, clang 23.1.5
Scene: no game: the game-free check
Method:
python3 -I -B tests/test_draw_stream_visibility.py --cxx g++ --headers <Vulkan-Headers/include>at the test-only commitd6ec2f17(mainaa95d35fplus the test) and at21b94e1a, twice each, alternatingHow to reproduce: check out each commit and run the command above
aa95d35f(with the test,d6ec2f17)21b94e1afirst-stream-failurecase (both rounds the same)In the game, dukenheimer's Steam Deck run for test request T-E85L (pass; #552 (comment)): this PR's head
d9edb33f(the same change, on main7ff5256e, in a build with #550 to #558 merged) against main7ff5256e, Steam Deck OLED (SteamOS 3.9.5, Mesa 26.2.0-devel, Game Mode, docked, stock clocks), two 5-minute runs each through Kokiri Forest, Hyrule Field and Kokiri again. No crash, device loss, fault or GPU stall in any run; fps 20.6 and 21.2 with it against 19.9 and 20.2 on main; the arrival pictures match main's, nothing missing. The failure itself needs memory to run out, and nothing in these runs shows whether a first upload stream ever failed (the change logs nothing); that Deck was short of memory in every run (about 4 GB swapped, MemAvailable down to 280 to 880 MB) and nothing crashed. So this is a non-regression result; the fix itself is shown by the game-free test.Flags and settings
None
Testing
Tested: Linux x86-64, game-free. The new case fails (SIGSEGV) on the test-only commit
d6ec2f17and passes at21b94e1a(this branch rebased ontomainaa95d35f), twice each, alternating, and undertests/run_all.pywith g++ 13.3 and with clang 23.1.5 at both commits. At the tip, the 109 checks that readgpu/vk/vk_draw.cppor the changed test: 106 passed, 1 skipped, and 2 failed that fail or are skipped by OS onmainaa95d35ftoo (test_pipeline_tour;test_shader_module_guard, a Windows-only check run here by name). Before the rebase the same at509f122fandd9edb33f(main7ff5256e). Steam Deck: dukenheimer's run for T-E85L, in Measurements.Not tested: The out-of-memory case in the game (not reproducible on demand). Windows in the game.
Must be tested: None
Risks
What could break: one bound added to a loop; a slot with streams flushes exactly as before.
Turning it off: revert the commit
Compatibility: Nothing changes
Issues
None filed.
Checklist
Everyone
type(scope): description) of the status line's type.main. Onto mainaa95d35f, no conflict; the test and fix commits are the same changes as before (git patch-id).python -I -B tests/run_all.pypasses (any failure that also fails onmainis named). The full suite on mainaa95d35f(Linux, g++ 13.3, the Vulkan headers set): 528 passed, 15 failed, 50 skipped; on this branch the new test and the 109 checks that read the changed files ran, with no failure thatmaindoes not have (see Testing). The 15 that fail on main:test_appimage_player_build,test_bake_portability,test_cpm_discovery,test_crash_dialog,test_env_on,test_lift_cache,test_lift_cleanup_error,test_make_game_cache,test_mods_size_table,test_options_budget_once,test_pipeline_tour,test_readback_late,test_vk_pgo,test_walk_keyandnx2/test_texture_init_barrier.py.third_party/untouched (a new folder with aPROVENANCE.mdinstead).When it applies
Assisted-by:/Co-Authored-By:trailers.mainand passing here.tests/that reads changed source updated in this PR, not weakened.gpu/vk/vk_draw.cppat21b94e1achecked whole (-fsyntax-only -Wall -Wextra, the build's defines) with clang 23.1.5 for Linux and llvm-mingw 20261104 for Windows: the same diagnostics asmainaa95d35f's (102 and 103), none new; on Linux both stop at the same narrowing error in a line this PR does not touch. Before the rebase, the combined branch 27553c83 linkedlongshot_vk.dll(llvm-mingw 20261104,RX_VK_ONLY=1 RX_RELEASE=1 RX_TARGET=windows) andlongshot_vk.so.draw_flush_recordingflushes no stream when the slot has noneFilled in: the full game-free suite on the combined branch, the Windows and Linux builds, and the test request T-E85L. Still work in progress until a Steam Deck run is in.
Test request T-E85L: pass
Built from main
7ff5256ewith #550 to #558 merged in order (all clean); this PR at its headd9edb33f. The comparison is main7ff5256e.Setup and method below; the runs are #551's series, where the combined build ran third and fourth.
Result
run.
As the request says, the failure itself needs memory to run out, and nothing here shows whether a first upload stream ever failed:
the change logs nothing. This Deck was short of memory in every run at these settings (about 4 GB swapped, MemAvailable down to
280-880 MB), and nothing crashed.
Setup
kernel 7.2.7-valve2. Game Mode, docked, stock clocks.
7ff5256e; main + this PR alone (the renderer built from the PR's head); and a combined build of main + #550 to #558,merged in order, all clean. All built for x86-64-v3 with the release play profile and LLVM 23.1.5. One test folder (warm caches) with
the binaries swapped; the shader caches' stamp is the same for all three.
1280x720, Render resolution 50% (640x360), the game's own upscaler, frame generation off, VRAM budget 2.2 GB.
HyruleField_Child (the Lost Woods bridge), 110 s, the teleport back to KokiriForest_Child, 50 s.
off events that blocked the teleports.
arrival.
The arrival pictures, all six runs (Kokiri, the Field, Kokiri again):
Thank you for the Deck run, dukenheimer: your result is in the description as you reported it, including its limits. Marking this ready for review.
WIP: fix(vk): a recording without a draw stream is submitted instead of crashingto fix(vk): a recording without a draw stream is submitted instead of crashingRebased onto
mainaa95d35fwith no conflict (the same two changes,git patch-id; commits now under the noreply identity) and the description moved to the current template.The test was run again on the new base (SIGSEGV at the test-only commit, pass at the tip, two rounds, g++ and clang); dukenheimer's T-E85L Deck run still holds as a non-regression result:
mainchangedvk_draw.cppsince7ff5256e(#404, #608, #721, #415, #729, #761) but notdraw_flush_recording(), the one function this PR touches.d9edb33f7921b94e1ab2