Repository navigation
fix(vk): DLSS comes back after a feature that was not made, and frame generation takes the multiplier its sets were made for - #2658
Conversation
… generation takes the multiplier its sets were made for - vk_framegen.cpp: framegen_record took the multiplier from the row again after ensure_size had made the sets; a step up in the menu (the display thread) in between copied the game's frame into an image that was never made. It takes fg.mult. - vk_dlss.cpp: a feature that was not made (NotRexed#2550) was asked for again only when the output, preset or HDR changed. The quality mode is in the key, release() clears it, and temporal_reset_failure clears it (dlss_retry). - vk_dlss.cpp: with retry (NotRexed#2574) a failing evaluation logged on every frame for good. Its line is spaced as the chain's reason is, and after 30 in a row the feature is made again once. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
A Windows test of the frame-generation race in the first part of this PR. Current
The three
With this PR, no crash: three rounds of 2x → 3x → 4x and back in the menu, in a window: 11 swapchain remakes (4, 5 and 6 images), the game ran on and exited normally. This PR also held up for the second case. At 4x before play, the game went from fullscreen to a window and back 25 times each ( These are small counts: one crash per case on Not tested: Linux, AMD and Intel; the DLSS retry part of this PR (no failed feature creation here). 🤖 Generated with Claude Code |
|
The race in the first part happens in the game: a crash on Windows when stepping the multiplier up, at the copy this PR fixes.
With the build's So it isn't only a window seen in reading: one step up was enough here. 🤖 Generated with Claude Code |
Joey-Magnus
left a comment
There was a problem hiding this comment.
I merged this into current main and ran the DLSS/framegen/present/temporal/HDR checks (run_all.py dlss framegen present temporal hdr upscale: 26 passed, 4 skipped for Vulkan headers). The test_hdr_encode failure on this head was main's own (kEngine's retry, fixed by #2648), so a re-run should clear it.
The framegen fix is right and matters. ensure_size builds the sets for fg.mult, and framegen_record re-read multiplier() afterwards, so a menu step on the display thread in between made the last vkCmdCopyImage target a frame that was never created. Taking fg.mult at gpu/vk/vk_framegen.cpp:791 closes that. Jiro131 and polackangry both reproduced the nvoglv64.dll read at 0x590 in framegen_record on main, and neither saw it with this change.
On DLSS: adding the quality mode to the refused key, clearing refused in release() (vk_dlss.cpp:481) and calling dlss_retry from temporal_reset_failure look correct. The key's mode bits (8-10) don't overlap the preset bits. Note that the release after 30 failed evaluations (:469) fires only once per run of the same error code, which I think is what you want. I found no open PR touching these same lines (#2788 and #2665 change other parts of these files).
Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, this is a good one. The frame generation half fixes a real crash, and the DLSS half is small and easy to follow.
Frame generation. Taking fg.mult in framegen_record is the right fix. ensure_size made the sets for that count, and nothing on the display thread reads multiplier() to size anything. Reading opts.framegen == 1 once also holds up: both branches are now bounded by fg.mult, so a switch between Auto and a fixed count between ensure_size and the record stays inside the sets that exist. Jiro131 (RTX 4090, Windows 11) and polackangry (RTX 5060 Ti, Windows 11) both crashed main with the same driver read inside framegen_record when stepping the multiplier, and neither crashed with this change. Together with your own Linux runs, that's the in-game evidence this needs.
DLSS retry. I checked that it can't turn into a retry every frame:
refusedis cleared in three places: when the key changes, inrelease(), and indlss_retry().release()only runs when the Upscaler switches engines, when the chain goes idle (and only once the row has left DLSS), or once after 30 failed evaluations.dlss_retry()only runs from the menu's DLSS, Render resolution and DLSS model rows. So a creation that keeps failing is tried again once per player change, not once per frame. While DLSS is off,preparereturns 0 with the same reason pointer, so the chain'sfail()doesn't log each frame either.- The 30-in-a-row release happens once for each run of the same error code (as Joey-Magnus noted): one new feature, then at most one log line every 600 frames. If that new creation fails,
refusedholds it off again. Therelease()insideevaluatedefers the handle throughtemporal_later, so the evaluation already recorded incmdkeeps a live feature. - While DLSS is off, the state stays consistent:
s.featureis null, the stalefeature_*sizes are only read behinds.feature,hdr()reports false, and the chain falls back to the game's upscale. dlss_retryruns under the chain's lock, asprepareandreleasedo, sorefusedhas a single owner. The non-DLSS stub is there too.
Two small notes, inline. Neither blocks.
You couldn't make a creation or an evaluation fail on a 16 GB card, so the DLSS retry is checked by reading only. That's fine here: the normal DLSS path ran in the game with no regression, and if the new branches misbehaved, the worst they could do is what main already does. If someone with an 8 GB card hits a its feature was not made line, a log showing feature N made (again after it was let go) after a row change would close it.
CI is green and the branch merges cleanly. I found no open PR that touches these lines. #2788 and #2665 edit other parts of vk_dlss.cpp / vk_framegen.cpp, so the merge order doesn't matter.
Approving.
| } | ||
| const uint64_t key = (uint64_t)ow << 44 ^ (uint64_t)oh << 24 ^ (uint64_t)preset << 1 ^ (hdr ? 1 : 0) ^ 1ull << 63; | ||
| // (the quality mode with them: the Render resolution row asks for another feature of the same output) | ||
| const uint64_t key = (uint64_t)ow << 44 ^ (uint64_t)oh << 24 ^ (uint64_t)((int)mode & 7) << 8 ^ (uint64_t)preset << 1 ^ |
There was a problem hiding this comment.
Nit, nothing to change unless you see it in practice: with the mode in the key, a refused feature is also retried whenever the game's dynamic resolution moves the render size into another mode's range. refused holds one key, so a size that goes back and forth across a range boundary tries again on each crossing. That's the same cost a working feature already pays (a new feature on each crossing), so I think it's fine. I'd keep the mode here, because it covers the case where the menu's dlss_retry is used up on a frame that still has the old render size.
| logf("vk: NVIDIA DLSS: its evaluation failed (%#x)", (unsigned)r); | ||
| // (the chain tries again at the next frame, kEngine's retry: one that keeps failing says so when its result changes | ||
| // and every 600th time, as the chain's own reason does, not on every frame) | ||
| if (r != s.eval_result || ++s.eval_repeats % 600 == 0) |
There was a problem hiding this comment.
Nit: if two error codes alternate (A, B, A, B...), r != s.eval_result is true every frame and this logs every frame again, and the 30-in-a-row release never fires. That seems unlikely from NGX, so it's fine as it is. Keeping a separate eval_fails count that only a success resets would cover it, if you want to.
After one failed DLSS feature creation, DLSS stays off until the player changes the DLSS model or the output size; and stepping the Frame generation multiplier up in the menu can make a frame be copied into an image that does not exist. Both are in code that #2550 and #2574 touched, found by reading the merged result.
What changed
Frame generation: the multiplier is read once (
vk_framegen.cpp).framegen_recordcalledmultiplier()again afterensure_sizehad made the sets forfg.mult. The menu runs on the display thread (menu_tickfrompresent_image), the record on the replay thread, with a wait for a free set in between. A step from 2x to 3x in that window givesm = fg.mult + 1, and the lastvkCmdCopyImagetargetsset->frame[fg.mult], a null image, which the display thread then presents. It takesfg.multnow, and reads the Auto test once for the same reason.DLSS:
refusedis cleared where a retry should happen (vk_dlss.cpp). Since #2550 a creation that fails returns 0 withs.refused = key, and the key is the output size, preset and HDR. The old feature is already let go, so from then on:release()leftrefusedset).#2574's
temporal_reset_failuredoes not help there: the chain'sfailedflag is not what holds DLSS off. Now the quality mode is part of the key,release()clearsrefused, andtemporal_reset_failureclears it too (dlss_retry, called with the chain's lock held, aspreparehas it).DLSS: a failing evaluation no longer logs on every frame (
vk_dlss.cpp). Withretry = true(#2574) an evaluation that fails every frame wroteits evaluation failed60 times a second for the rest of the run. The line is spaced like the chain's own reason (when the result changes, and every 600th time), and after 30 failures in a row the feature is let go once sopreparemakes a new one.How it was tested
tests/run_all.py: with fix: main's source checks pass again (15 red since #2574, #2617 and others), and the three bugs behind them #2656 (which makes main's checks pass) and this together, 468 passed, 0 failed, 22 skipped. On this branch alone the 15 checks that are red on main stay red and nothing else fails.test_present_source_crop's fixture setsfg.multin itsensure_sizestand-in, as the real one does.feature 4 made (again after it was let go ...).Not tested
Review notes that are not fixed here, for whoever owns them: FSR 4's engine still has the permanent latch #2574 removed for DLSS (
vk_fsr4.cpppreparereturns -1, noretry); with Native HDR, frame generation's images are RGBA8 (vk_framegen.cppensure_size), so the game's 10-bit PQ picture passes through 8 bits; DLSS-G's feature keeps the render size it was created with when the Render resolution changes.🤖 Generated with Claude Code