WIP: fix(vk): Hookshot's renderer fixes merged after Longshot branched, restored #287
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
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/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!287
Loading…
Reference in a new issue
No description provided.
Delete branch "TrashPanda/Longshot:restore/renderer-fixes"
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?
Why
Renderer fixes Hookshot merged after Longshot branched: missing or stale textures after a failed upload, the FSR 4 tail
written out of order, Intel's split submissions flashing white, upscaler smears after every camera cut, the ultrawide
sides open during loading, FSR 3.1 losing HDR highlights, and juliotec's Vulkan robustness fixes. Good for rc.4, but they
need a run in the game first. Split out of #209.
What changed
Split out of #209 as its reviewer asked (review on #209, point 4, group d). Each commit is the merged Hookshot pull request under its original author, carried through Longshot's rename; a
Restored-from:trailer names it, and the bare Hookshot numbers in the messages and the restored text are now full URLs.Credit to the authors of the restored work: jarynclouatre, unixman69, NotRexed, Mathieu Sellier, juliotec.
unsupported texel-buffer formats skipped, the cooperative-matrix probe checked, sparse binds retried and flushed, the
read-back command buffer not reused while a timed-out submission holds it
texture_touch); a GPU buffer write into a texture's storage is found under alater, smaller write
dlss_math::camera_cut); FSR 3.1 on thechain keeps HDR (shader
fg_scale.compand its SPIR-V headers)MkwVkHost.wide_cover,MKWVK_API_VERSION23)networks are not shader failures
RX_VK_TEMPORAL_CUTandRX_VK_SPLIT_UBFLUSHin the switch referenceSplit siblings: tools, checks and docs #284, test-request pinning #285, rc.4 player fixes #286, new defaults and speed-ups #288.
Commits, in order:
98d7dc3ffix(fsr4): the tail is written before its reader also when the unrolled shader arrives between bind and dispatch (Hookshot 2952, by jarynclouatre)cb7fa397fix(vk): a background-prepared update makes the state list's pipelines behind the preparing screen too (Hookshot 2982, by jarynclouatre)28d15628fix(vk): a half-written last record is cut off before the pipeline lists are added to (Hookshot 2983, by jarynclouatre)0c340b48fix(vk): the networks a card leaves out are not logged or counted as shader failures (Hookshot 2987, by jarynclouatre)602e4ec4fix(vk): a GPU buffer write into a texture's storage is found under a later, smaller write (Hookshot 3002, by jarynclouatre)0031584ffix(vk): a texture whose upload failed is tried again, not left empty or stale (Hookshot 3003, by jarynclouatre)2ce58147feat(vk): FSR 3.1 on the chain keeps the game's native HDR highlights (Hookshot 2614, by unixman69)24dac2f9fix(ultrawide): the sides stay closed while the loading cover shows in the middle 16:9 (Hookshot 2939, by jarynclouatre)fe326b54fix(vk): a camera cut starts a new upscaler history (DLSS, FSR, their frame generation) (Hookshot 2927, by NotRexed)4d9be5ecfix(vk): on Intel a split submission's uniform updates are written by its last part (Hookshot 3000, by Mathieu Sellier)1f494405fix(vk): bind a sparse texture's queued blocks when its upload runs out of staging (Hookshot 3031, by juliotec)7877b7f1fix(vk): do not reuse the read-back command buffer while a timed-out submission holds it (Hookshot 3032, by juliotec)0e9e458efix(vk): validate texture blits before vkCmdBlitImage (Hookshot 2691, by juliotec)cf9ebf7ffix(vk): skip clears of textures kept in ASTC blocks (Hookshot 2718, by juliotec)bd4d9ebbfix(vk): keep mesh indirect argument ranges from overlapping (Hookshot 2719, by juliotec)1fed49dffix(vk): skip texel buffer views of formats the device cannot use (Hookshot 2741, by juliotec)7b1ed3c9fix(vk): check the cooperative matrix probe's mapping, fence and timeout (Hookshot 2742, by juliotec)747dd3ccfix(vk): retry sparse blocks whose bind failed (Hookshot 2743, by juliotec)bdd0a85dfix(vk): MKWVK_API_VERSION 23 for MkwVkHost.wide_cover (mine)42af7869test: a compile_all stand-in in the check of the two step sequences (mine)c6601f6fdocs: the restored Hookshot references as full URLs (mine)a7b863c5docs(development): RX_VK_TEMPORAL_CUT and RX_VK_SPLIT_UBFLUSH in the switches (mine)Flags and settings
RX_VK_TEMPORAL_CUT=0RX_VK_SPLIT_UBFLUSH=1/=0=0RX_VK_FSR3_HDR=0RX_VK_IRMeasurements
No numbers of my own: each fix was measured or shown upstream (commit messages and its tested-configuration rows).
Here: game-free checks and builds, main and this PR on the same PC.
main:
d9648863(2026-11-13)this PR: restore/renderer-fixes @ 0e45532f (the runs; now
a7b863c5, after the docs-only rebase onto main's514591a3)Hardware: RTX 4070 Ti (driver 595.99.02), Ryzen 7 9800X3D, Ubuntu 26.04.1
Scene: none (no game run)
Method:
python3 -I -B tests/run_all.py --jobs 6, once on each; the builds belowHow to reproduce: the same command on each commit
d9648863run_all.py: passed / failed / skippedThe numbers still hold on
514591a3: main's #274 changed a Discord link in 11 docs files and nothing these checks or builds compile; the checks that read those docs were run again on this head.Testing
Tested:
python3 -I -B tests/run_all.py --jobs 6on Linux (Ubuntu 26.04.1, Python 3.14, Vulkan and SPIR-V headers found): this PR @0e45532f: 524 passed, 2 failed, 18 skipped (test_lift_cache.py, test_menu_i18n.py); main @d9648863on the same PC: 513 passed, 2 failed, 18 skipped (test_lift_cache.py, test_menu_i18n.py).test_lift_cachefails on main the same way (the lift's scripts do not import here), andtest_menu_i18nfails on that main too. Main then gained514591a3(#274, a Discord link in 11 docs files only); after the rebase onto it, the checks that read those docs passed again ata7b863c5.Built at
0e45532fwith no game, on Linux (system clang 21, lld) and for Windows (cross-built here with tools.lock's llvm-mingw 20261104 and its windows-x64 CPM cache, which has FFmpeg and dav1d): (before the docs-only rebase onto #274)nx2/build.py --standalonewith an empty lift compiled everything in the repository that the game links, the host, its libraries and the Vulkan back end (406 files on Linux, 405 for Windows), with no failure, and its link stopped only at the symbols the game's lifted code provides (18 on Linux, 19 on Windows: the same list as main @d9648863).RX_VK_ONLY=1 RX_RELEASE=1linkedlongshot_vk.soandlongshot_vk.dll, andbuilder/setup/setup.clinked asLongshotBuilder-Setup.exewith build-windows.yml's command. The five split branches merged together locally (the API version taken as 23) compiled and linked the same way, so they also build together.Not tested: a run in the game: the game build needs the game's lifted code, and I had no run here.
Needs other testers:
DLSS or FSR, expecting no smear after the cut, no missing textures and no device lost (T-TQY6, claimable once a maintainer adds the
testing wantedlabel)Screenshots
None: no run in the game yet (the ultrawide sides and the camera-cut smears show on screen).
Risk and rollback
Rendering could go wrong on a GPU the upstream runs did not cover; each commit reverts on its own, and the new behaviours
have switches above.
MKWVK_API_VERSIONis 23 here, assuming the rc.4 fixes (#286, 22) merge first; should this mergefirst, it takes 22 and the other 23.
Reviewer notes
Please merge with rebase, not squash, so that each author keeps their commit. Two commits are mine: the API version,
and a
compile_allstand-in in the check that https://github.com/NotRexed/Hookshot/pull/2982 extends.Issues
See #209
Checklist
For everyone
type(scope): description).main.python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above).third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.docs/tested-configurations.mdfor each measured run. (The restored rows, from the upstream runs.)MKWVK_API_VERSIONbumped (gpu/vk/vk_api.h'sMkwVkHostorMkwVkRendererchanged).fg_scale.compas it is here.)tests/that reads changed source updated in this PR, not weakened.### The bug `upload_inner` (gpu/vk/vk_texture.cpp) uploads a texture in runs of rows. For a sparse texture, each run first queues the binds of the tiles it touches with `texture_resident_queue`: the tiles are marked bound at once, and the binds go into `sparse_binds`, one list shared by every texture. `texture_resident_flush` later binds the whole list with `vkQueueBindSparse`, into one image only: `VkSparseImageMemoryBindInfo` takes a single `image` for the list. Each run takes its staging memory with `staging()`. When that failed part-way, the code was: ```cpp if (!st.buffer) return false; ``` so it returned with the binds of the earlier rows still in `sparse_binds`. Step by step: 1. Texture A (sparse) uploads. Rows 0..N queue the binds of their tiles; those tiles are marked bound. 2. The staging for row N+1 fails, and `upload_inner` returns false. A's binds are still in `sparse_binds` and were never made. 3. Texture B (sparse) uploads. Its `texture_resident_flush` binds the whole list into **B's** image, including A's binds. The memory meant for A's tiles goes to the same coordinates of B's image. 4. A's tiles stay marked bound, so no later write queues them again. A's later copies into those tiles land in unbacked memory, and B gets memory it did not ask for. The other exits of that loop (a new staging arena, the end of the upload) already call `texture_resident_flush(&tex)` first. Only the staging-failure exit did not. ### What changed Only that exit, in `upload_inner`: ```cpp if (!st.buffer) { texture_resident_flush(&tex); return false; } ``` The binds the earlier rows queued are now made into A's own image before giving up, as the other exits do. The success path is unchanged. A texture is only sparse when it is a 2D array over `RX_VK_TEX_MAX_MB` (1.5 GB) on a device with sparse support, which is MKW's course-map case. ### Test `tests/test_sparse_staging_failure.py` is a source check on `upload_inner`. In the part of the loop after the first `staging()` call and before `texture_resident_queue`, every `return false;` must have a `texture_resident_flush(&tex);` in the same block. The old `if (!st.buffer) return false;` must be gone. On main it fails with "a staging failure returns without binding the sparse blocks queued before it"; with the fix it passes. ### How tested - The new test fails on main and passes with the fix. - The tests that reference vk_texture.cpp pass. - Release build OK. - Zelda OoT on Radeon RX 9070 XT (Windows, Adrenalin), nightly with this change, about 40 minutes, log clean. This checks that non-sparse uploads are unaffected; the sparse path itself is not reached there. Assisted-by: GitHub Copilot Restored-from: https://github.com/NotRexed/Hookshot/pull/3031### The bug Read-backs (texture write-backs to guest memory) use their own command pool `rb_pool`, command buffer `rb_cmd` and fence `rb_fence` (gpu/vk/vk_texture.cpp): - `readback_begin` resets the pool and begins `rb_cmd`. - `readback_submit_wait` resets `rb_fence`, submits `rb_cmd` with it, and waits up to 5 s. When that wait timed out, the function returned false, but nothing remembered that the submission was still running. Step by step: 1. A read-back is submitted. The GPU is slow (heavy load, a long batch before it), and `vkWaitForFences` returns `VK_TIMEOUT` after 5 s. `rb_cmd` is still in the pending state and `rb_fence` is still tied to it. 2. The next read-back calls `readback_begin`: - `vkResetCommandPool(rb_pool)` while `rb_cmd` is pending. This violates **VUID-vkResetCommandPool-commandPool-00040**: "All VkCommandBuffer objects allocated from commandPool must not be in the pending state". - `vkBeginCommandBuffer(rb_cmd)` while it is pending. This violates **VUID-vkBeginCommandBuffer-commandBuffer-00049**: "commandBuffer must not be in the pending state". 3. `readback_submit_wait` then calls `vkResetFences(rb_fence)` while it is still tied to the unfinished submission. This violates **VUID-vkResetFences-pFences-01123**: "Each element of pFences must not be currently associated with any queue command that has not yet completed execution on that queue". This is undefined behaviour: the GPU can be running commands that the CPU is rewriting. There was a second problem in the creation. `readback_begin` makes the pool, then the command buffer, then the fence, all only when `rb_pool` is null. If `vkAllocateCommandBuffers` or `vkCreateFence` failed, `rb_pool` stayed set with `rb_cmd` or `rb_fence` null. Every later call then skipped the creation and used a null command buffer or fence. ### What changed - New `bool rb_pending`. `readback_submit_wait` sets it when its wait does not succeed: ```cpp rb_pending = vkWaitForFences(dev.device, 1, &rb_fence, VK_TRUE, 5000000000ull) != VK_SUCCESS; return !rb_pending; ``` - `readback_begin` first waits for a pending submission, before any reset. If it is still not done, this read-back is skipped (returns false), as a failed read-back already was: ```cpp if (rb_pending) { if (vkWaitForFences(dev.device, 1, &rb_fence, VK_TRUE, 5000000000ull) != VK_SUCCESS) return false; rb_pending = false; } ``` - If the command buffer or the fence cannot be made, the pool is destroyed and all three handles go back to null, so the next call makes them again instead of using them half made. When no wait times out, the behaviour is the same as before. ### Test `tests/test_readback_pending.py` is a source check on `readback_begin` and `readback_submit_wait`. It checks that: - `rb_pending` exists. - In `readback_begin`, `if (rb_pending)` and its `vkWaitForFences` on `rb_fence` come **before** `vkResetCommandPool`. - The failure branch of the creation destroys `rb_pool` and sets it back to null. - `readback_submit_wait` stores the result of its wait in `rb_pending`. On main it fails ("a timed-out read-back is not remembered"); with the fix it passes. ### How tested - The new test fails on main and passes with the fix. - The tests that reference vk_texture.cpp pass. - Release build OK. - Zelda OoT on Radeon RX 9070 XT (Windows, Adrenalin), nightly with this change, about 40 minutes, log clean. Read-backs run normally there, but none timed out, so the timeout path itself was not reached. Assisted-by: GitHub Copilot Restored-from: https://github.com/NotRexed/Hookshot/pull/3032### The bug `copy_t2t` (gpu/vk/vk_xfer.cpp) makes a texture-to-texture copy in one of three ways: 1. `vkCmdResolveImage`: a multisampled source into a single-sampled destination of the same size. 2. `vkCmdCopyImage`: same size, same block size, same sample count. 3. `vkCmdBlitImage`: everything else, which means scaled or converting copies. Branch 3 called `vkCmdBlitImage` without checking any of the rules the spec puts on it. Step by step, with one example: 1. The game copies a texture into another of a different size or a different format, for example an unsigned-integer texture into a normalized-colour one. 2. Neither the resolve nor the copy branch applies, so the copy goes to `vkCmdBlitImage`. Linear is used when the game asks for it, or when the destination keeps its level at a smaller size than the source (`d.down > s.down`). 4. When one of those rules is broken, the command is invalid usage and the result is undefined. Some drivers tolerate it; others may write garbage or lose the device. The rules, with their exact text from the spec (`validusage.json`): | VUID | Text | |---|---| | VUID-vkCmdBlitImage-srcImage-01999 | The format features of srcImage must contain VK_FORMAT_FEATURE_BLIT_SRC_BIT | | VUID-vkCmdBlitImage-dstImage-02000 | The format features of dstImage must contain VK_FORMAT_FEATURE_BLIT_DST_BIT | | VUID-vkCmdBlitImage-filter-02001 | If filter is VK_FILTER_LINEAR, then the format features of srcImage must contain VK_FORMAT_FEATURE_SAMPLED_IMAGE_FILTER_LINEAR_BIT | | VUID-vkCmdBlitImage-srcImage-00229 | If either of srcImage or dstImage was created with a signed integer VkFormat, the other must also have been created with a signed integer VkFormat | | VUID-vkCmdBlitImage-srcImage-00230 | If either of srcImage or dstImage was created with an unsigned integer color VkFormat, the other must also have been created with an unsigned integer color VkFormat | | VUID-vkCmdBlitImage-srcImage-00231 | If either of srcImage or dstImage was created with a depth/stencil format and the maintenance12 feature is not enabled the other must have exactly the same format | | VUID-vkCmdBlitImage-srcImage-00233 | srcImage must have been created with a samples value of VK_SAMPLE_COUNT_1_BIT | | VUID-vkCmdBlitImage-dstImage-00234 | dstImage must have been created with a samples value of VK_SAMPLE_COUNT_1_BIT | Hookshot does not enable maintenance12, so -00231 applies as written. The images are created with `VK_IMAGE_TILING_OPTIMAL`, so the format features that matter are `optimalTilingFeatures`. ### What changed (gpu/vk/vk_xfer.cpp) Before, branch 3 went straight to the blit: ```cpp } else { // scaled or converting VkImageBlit bl{}; ... const bool linear = ((p.flags & NVN_COPY_FLAGS_LINEAR_FILTER_BIT) || d.down > s.down) && color; vkCmdBlitImage(..., linear ? VK_FILTER_LINEAR : VK_FILTER_NEAREST); } ``` Now it first checks the rules above, and skips a blit that breaks any of them: ```cpp if (!(tiling_features(s.tex->format) & VK_FORMAT_FEATURE_BLIT_SRC_BIT) || // -01999 !(tiling_features(d.tex->format) & VK_FORMAT_FEATURE_BLIT_DST_BIT) || // -02000 format_class(s.tex->format) != format_class(d.tex->format) || // -00229, -00230 (!color && s.tex->format != d.tex->format) || // -00231 s.tex->samples != VK_SAMPLE_COUNT_1_BIT || d.tex->samples != VK_SAMPLE_COUNT_1_BIT) { // -00233, -00234 warn(13, "texture blit the device cannot make: skipped"); full_barrier(cmd); return; } ``` Linear filtering is only used when the source format supports it (-02001), and only for float or normalized formats: ```cpp const bool linear = ((p.flags & NVN_COPY_FLAGS_LINEAR_FILTER_BIT) || d.down > s.down) && color && (tiling_features(s.tex->format) & VK_FORMAT_FEATURE_SAMPLED_IMAGE_FILTER_LINEAR_BIT) && format_class(s.tex->format) == 0; ``` Two new helpers in the same file: - `format_class(VkFormat)` returns: - 1 for the unsigned-integer formats, - 2 for the signed-integer formats, - 0 for everything else (float, normalized, sRGB). - `tiling_features(VkFormat)` returns the format's `optimalTilingFeatures`: - For the core formats (values below 256), it asks the driver once per format and keeps the answer in a lock-free `std::atomic<uint64_t>` table. Bit 63 marks a format as already asked. - Formats from extensions (256 and above) are asked each time. - A scaled copy checks both sides, so the cache avoids querying the driver twice per copy on the replay thread. The depth/stencil condition is already caught earlier in `copy_t2t`, by warn 7 ("texture copy between unlike depth formats: skipped"), so it never fires here. It is kept in the check so the blit's own rules stay in one place. `warn` logs each key at most 3 times. ### Behaviour - A blit that follows the rules goes through exactly as before. - A blit that breaks a rule is skipped, and its destination is left as it was. Before, it was undefined: it may have looked right on some drivers, for example an integer/float mix or a multisampled image on a driver that tolerates it. Now the destination is stale instead. - This matters most on devices with a narrower format list (mobile and integrated GPUs, layered drivers such as MoltenVK), and for games or scenes that blit less common formats. ### Changes after review - The format features come from the `tiling_features` cache, so they are not queried twice per scaled copy. - The first push of that change left one use of the old local variable in the linear-filter check. The vk_xfer tests don't compile that code; the full release build caught it, and it is fixed. ### Test There is no dedicated unit test for these rules. The change is reviewed against the VUIDs above. The existing checks that compile or read vk_xfer.cpp pass (6, including `test_vulkan_copy_span_gpu_compile`). ### How tested - Release build (`nx2/build.py --standalone`, Windows, llvm-mingw). - The 6 vk_xfer.cpp checks pass. - Zelda OoT on Radeon RX 9070 XT (Windows, Adrenalin): - Two full sessions on build g54d01844. - One session of about 40 minutes on nightly g57471723, which includes this change. - No visible regression, no device loss. - "texture blit the device cannot make" does not appear in any of those logs. Every blit OoT makes on this device follows the rules, so the skip path is not reached there. This change guards against invalid usage; it does not fix a visible glitch in OoT. Assisted-by: GitHub Copilot Restored-from: https://github.com/NotRexed/Hookshot/pull/2691### The bug ASTC textures are kept in one of three ways, depending on the GPU: - **Decoded to RGBA8** on most GPUs. - **Re-encoded as BC3** for some big textures. - **Kept as ASTC blocks** when the device samples ASTC natively (`textureCompressionASTC_LDR`, `astc_native()`). The image then has an ASTC `VkFormat`. `clear_tex` (gpu/vk/vk_xfer.cpp) clears a texture in one of two ways: 1. **Whole level**: when the clear covers the whole level, the image is not 3D, and the format is not BC (`raw`), it uses `vkCmdClearColorImage` for colour, or `vkCmdClearDepthStencilImage`. 2. **Region**: otherwise, a render pass over that level and layer with `vkCmdClearAttachments`. For BC images (`raw`) the rectangle is converted to blocks. `raw` is computed only for the BC range, so an image kept in ASTC blocks was treated as uncompressed. Step by step: 1. On a device with native ASTC, the game clears an ASTC texture. 2. If the clear covers the whole level, `clear_tex` calls `vkCmdClearColorImage` on the ASTC image. This breaks **VUID-vkCmdClearColorImage-image-00007**: "image must not have a compressed or depth/stencil format". 3. If it covers part of the level, the region path below only converts the rectangle to blocks for BC formats, not for ASTC. 4. Either way, a colour written over ASTC blocks would be garbage, since the blocks are compressed data, not texels. ### What changed (gpu/vk/vk_xfer.cpp, clear_tex) Before, no ASTC check: ```cpp full_barrier(cmd); const bool volume = s.tex->type == VK_IMAGE_TYPE_3D; const bool raw = s.tex->format >= VK_FORMAT_BC1_RGB_UNORM_BLOCK && s.tex->format <= VK_FORMAT_BC7_SRGB_BLOCK; ``` After: ```cpp full_barrier(cmd); if (s.tex->format >= VK_FORMAT_ASTC_4x4_UNORM_BLOCK && s.tex->format <= VK_FORMAT_ASTC_12x12_SRGB_BLOCK) { warn(14, "clear of an ASTC texture: skipped"); return; } const bool volume = s.tex->type == VK_IMAGE_TYPE_3D; ``` A clear of an image whose format is ASTC is skipped, and the image is left as it was. `warn` logs it at most 3 times. The early return also skips `texture_written` at the end of `clear_tex`, so the texture is not marked as GPU-written. That is on purpose, because nothing was written to it. ASTC textures decoded to RGBA8, ASTC kept as BC3, and every other format are cleared exactly as before. ### Who reaches this Only devices that expose `textureCompressionASTC_LDR` and sample ASTC natively, such as Intel iGPUs. AMD and NVIDIA desktop drivers don't expose it, so on them the image is never in an ASTC format and the new check never matches. ### Changes after review - The description no longer claims that drivers hang. The rule comes from the spec (the VUID above). - Skipping `texture_written` was confirmed as intended. ### Test There is no dedicated unit test. In the review run, `test_bc_raw_targets.py` and `test_vulkan_clear_mip.py` pass with this change. ### How tested - Release build. - Zelda OoT on Radeon RX 9070 XT (Windows, Adrenalin), including about 40 minutes on nightly g57471723, which includes this change. Log clean, and "clear of an ASTC texture" does not appear. - This GPU has no native ASTC, so the new path was not run. The test only checks that a GPU without native ASTC is unaffected. Assisted-by: GitHub Copilot Restored-from: https://github.com/NotRexed/Hookshot/pull/2718### The bug `texel_buffer_view` (gpu/vk/vk_texture.cpp) creates the `VkBufferView` for a texture the game binds as a texel buffer: - as a **uniform** texel buffer when it is sampled, - as a **storage** texel buffer when it is written. The view's format is the one the texture maps to (`vk_format(nf).format`). The only check was that the format is not `VK_FORMAT_UNDEFINED`. Nothing checked whether the device supports texel buffers of that format. Step by step: 1. The game binds a texture as a texel buffer, in a format such as `VK_FORMAT_R32G32B32_SFLOAT`. 2. The device does not list `VK_FORMAT_FEATURE_UNIFORM_TEXEL_BUFFER_BIT` (or `STORAGE_TEXEL_BUFFER_BIT`) for it in the format's `bufferFeatures`. Which formats are listed varies by driver. 3. `texel_buffer_view` creates the view anyway. This is invalid usage: | VUID | Text | |---|---| | VUID-VkBufferViewCreateInfo-format-08778 | If the buffer view usage contains VK_BUFFER_USAGE_UNIFORM_TEXEL_BUFFER_BIT, then format features of format must contain VK_FORMAT_FEATURE_UNIFORM_TEXEL_BUFFER_BIT | | VUID-VkBufferViewCreateInfo-format-08779 | If the buffer view usage contains VK_BUFFER_USAGE_STORAGE_TEXEL_BUFFER_BIT, then format features of format must contain VK_FORMAT_FEATURE_STORAGE_TEXEL_BUFFER_BIT | 4. What the shader then reads or writes through that view is undefined. ### What changed (gpu/vk/vk_texture.cpp, texel_buffer_view) A check right after the format is known, before anything is created, looked up, or marked as written: ```cpp VkFormat f = vk_format(nf).format; if (f == VK_FORMAT_UNDEFINED) return VK_NULL_HANDLE; // new: if (!(format_props(f).bufferFeatures & (storage ? VK_FORMAT_FEATURE_STORAGE_TEXEL_BUFFER_BIT : VK_FORMAT_FEATURE_UNIFORM_TEXEL_BUFFER_BIT))) { static std::atomic<int> logged{0}; if (logged.fetch_add(1, std::memory_order_relaxed) < 8) logf("vk: texel buffer of format %d (%s) not supported by the device: unbound", (int)f, storage ? "storage" : "uniform"); return VK_NULL_HANDLE; } ``` - A format without the needed feature gets no view, and the slot stays unbound. Callers already handle a null view; it is the same result as the existing `VK_FORMAT_UNDEFINED` case. - Because it returns before `texture_gpu_wrote`, a storage bind that is refused does not mark the texture as written by the GPU. - The answer comes from the existing `format_props` cache, which takes one short lock (`fmt_mu`) per texel-buffer bind but does not query the driver again. - The message is logged at most 8 times. - Supported formats follow the same path as before. ### Test `tests/test_texel_buffer_ownership.py` cuts `texel_buffer_view` out of vk_texture.cpp and compiles it against the stubs in `tests/texel_buffer_ownership_test.cpp`: - The fixture gets a `format_props` stub. It reports both texel-buffer features for every format except the one mapped from NVN format 44 (`VK_FORMAT_R32G32B32_SFLOAT`). - A new case, `unsupported`, binds format 44 as uniform and as storage. It checks that: - neither returns a view; - no view is created (`creates == 0`); - no buffer is looked up (`lookups == 0`); - the texture is not marked as written (`writes == 0`). - All 24 cases pass. On main, the case list has 23. ### Changes after review - The first version did not add the stub, so the test failed to compile ("'format_props' was not declared in this scope"). The stub and the `unsupported` case were added. ### How tested - Release build (`nx2/build.py --standalone`, Windows, llvm-mingw). - `tests/test_texel_buffer_ownership.py`: 24 cases pass. - Zelda OoT on Radeon RX 9070 XT (Windows, Adrenalin): - Two full sessions on build g54d01844. - One session of about 40 minutes on nightly g57471723, which includes this change. - No regression. - "texel buffer of format" does not appear in any of those logs. Every texel-buffer format OoT uses is supported on this device, so the new path is not reached there. This change guards against invalid usage; it does not fix a visible glitch in OoT. Assisted-by: GitHub Copilot Restored-from: https://github.com/NotRexed/Hookshot/pull/2741### The bug At startup, `coopmat_init` (gpu/vk/vk_coopmat.cpp) checks whether native matrix multiplies can be used with cooperative matrices. `measure_order` and `test_multiply` each run a small compute shader through `run_shader`. `run_shader` does the following: 1. Creates a host-visible buffer and its memory, maps it, and copies the input into it. 2. Creates the pipeline, a descriptor pool, a command pool and a fence. 3. Submits one dispatch, waits for it for up to 5 s, and reads the result back. 4. Destroys everything. It had three problems. **1. The mapping was not checked.** ```cpp uint32_t* words = nullptr; vkMapMemory(dev.device, memory, 0, size, 0, (void**)&words); std::memcpy(words, data.data(), size); ``` If `vkMapMemory` failed (for example, out of host memory), `words` stayed null and the `memcpy` crashed the emulator at startup. **2. A dispatch that did not finish was destroyed under the GPU.** ```cpp ok = vkQueueSubmit2(dev.queue, 1, &si, fence) == VK_SUCCESS && vkWaitForFences(dev.device, 1, &fence, VK_TRUE, 5000000000ull) == VK_SUCCESS; ... vkDestroyFence(...); vkDestroyCommandPool(...); vkDestroyDescriptorPool(...); vkDestroyPipeline(...); // and at the end of the function: vkDestroyBuffer(...); vkFreeMemory(...); ``` When the submit succeeded but the wait timed out (a slow or hung driver), the code still destroyed everything while the GPU could still be running the dispatch. Each of those calls breaks a rule: | VUID | Text | |---|---| | VUID-vkDestroyFence-fence-01120 | All queue submission commands that refer to fence must have completed execution | | VUID-vkDestroyCommandPool-commandPool-00041 | All VkCommandBuffer objects allocated from commandPool must not be in the pending state | | VUID-vkDestroyDescriptorPool-descriptorPool-00303 | All submitted commands that refer to descriptorPool (via any allocated descriptor sets) must have completed execution | | VUID-vkDestroyPipeline-pipeline-00765 | All submitted commands that refer to pipeline must have completed execution | | VUID-vkDestroyBuffer-buffer-00922 | All submitted commands that refer to buffer, either directly or via a VkBufferView, must have completed execution | | VUID-vkFreeMemory-memory-00677 | All submitted commands that refer to memory (via images or buffers) must have completed execution | **3. The fence was created unchecked.** If `vkCreateFence` failed, the fence was a null handle, and the code still submitted with it and then waited on it. `vkWaitForFences` requires valid handles (VUID-vkWaitForFences-pFences-parameter: "pFences must be a valid pointer to an array of fenceCount valid VkFence handles"). ### What changed (gpu/vk/vk_coopmat.cpp, run_shader) - A failed mapping now logs "cooperative matrix test: the buffer could not be mapped", frees the buffer and memory, and fails the probe: ```cpp if (vkMapMemory(dev.device, memory, 0, size, 0, (void**)&words) != VK_SUCCESS || !words) { logf("vk: cooperative matrix test: the buffer could not be mapped"); vkDestroyBuffer(dev.device, buffer, nullptr); vkFreeMemory(dev.device, memory, nullptr); return false; } ``` - A fence that cannot be created makes the probe skip the submit. Nothing was queued, so everything is freed as usual and the probe fails: ```cpp if (vkCreateFence(dev.device, &fci, nullptr, &fence) != VK_SUCCESS) fence = VK_NULL_HANDLE; const bool submitted = fence && vkQueueSubmit2(dev.queue, 1, &si, fence) == VK_SUCCESS; ok = submitted && vkWaitForFences(dev.device, 1, &fence, VK_TRUE, 5000000000ull) == VK_SUCCESS; ``` - A dispatch that was submitted but did not complete leaves the fence, command pool, descriptor pool, pipeline, buffer and memory alive, instead of destroying them under the GPU. Only the shader module is destroyed, which is allowed once the pipeline exists. This is a one-time leak at startup; on a device that really hung, later submits would queue behind it anyway. ```cpp if (submitted && !ok) { // (it may still be running: what it uses is left alive rather than destroyed under it) if (module) vkDestroyShaderModule(dev.device, module, nullptr); return false; } ``` The probe's result and every successful path are unchanged. A probe that fails falls back to software matrix multiplies, as before. ### Note for https://github.com/NotRexed/Hookshot/pull/1975 https://github.com/NotRexed/Hookshot/pull/1975 (cooperative matrices for the AA networks) changes `run_shader`'s signature. It merges cleanly with this PR, but whichever lands second should check that the new early returns still free what https://github.com/NotRexed/Hookshot/pull/1975 adds. ### Test There is no dedicated unit test. The failure paths need a driver that fails `vkMapMemory` or `vkCreateFence`, or a dispatch that takes more than 5 s. ### How tested - Release build. - Zelda OoT on Radeon RX 9070 XT (Windows, Adrenalin), including about 40 minutes on nightly g57471723, which includes this change. Log clean. - On this GPU, `coopmat_init` stops before the probe: the log says "matrix multiplies in software (the cooperative matrices lack 16x8x16 f16)". So `run_shader` is not reached there, and none of the changed paths ran. The test only shows that startup is unaffected. Assisted-by: GitHub Copilot Restored-from: https://github.com/NotRexed/Hookshot/pull/2742### The bug A sparse texture gets device memory per block (tile) as it is written, instead of all at once (gpu/vk/vk_texture.cpp): - `sparse_queue` walks the tiles a write reaches. For each tile that has no memory yet, it takes memory with `sparse_take`, appends a bind to `sparse_binds`, and **marks the tile bound at once** (`bound = 1`). The mip tail works the same way, with `sparse_opaque` and `tail_bound`. - `sparse_flush` then binds the whole list with `vkQueueBindSparse` and waits on a fence. When the bind failed, `sparse_flush` only logged it: ```cpp res = vkQueueBindSparse(dev.queue, 1, &bi, fence); ... if (res == VK_SUCCESS) { vkWaitForFences(...); vkResetFences(...); } else { logf("vk: sparse texture %#llx: binding failed (%d)", ...); } ... sparse_binds.clear(); sparse_opaque.clear(); return res == VK_SUCCESS; ``` Step by step: 1. A write to a sparse texture queues the binds of tiles A, B and C. Their `tiles[...]` entries are set to 1. 2. `vkQueueBindSparse` fails (for example, out of memory). The list is cleared, but the tiles stay marked bound. 3. The upload's copies go ahead and write tiles A, B and C, which have no memory behind them. 4. Every later write to those tiles sees them marked bound, so it never queues them again. Those tiles stay unbacked for the life of the texture, and every write to them is lost. Two more problems were in the same function: - The fence was created without checking. If `vkCreateFence` failed, every bind was submitted and waited on with a null fence. `vkWaitForFences` requires valid handles (VUID-vkWaitForFences-pFences-parameter: "pFences must be a valid pointer to an array of fenceCount valid VkFence handles"). - The memory `sparse_take` gave for the failed binds was simply lost to that texture. ### What changed (gpu/vk/vk_texture.cpp and vk_texture.h) - **New `sparse_unqueue(tex)`.** When the binds are not made, it: - marks every queued tile unbound again (`map[i] = 0`), finding the tile with the same math `sparse_queue` uses; - marks every queued mip tail unbound again (`tail_bound[layer] = 0`); - puts the memory those binds had taken on the texture's free list; - clears the queue, and counts `sparse_added` as `sparse_flush` does. The next write to those tiles queues them again, so the bind is retried. - **`sparse_flush` calls it in both failure cases**, and returns false: ```cpp if (vkCreateFence(dev.device, &fi, nullptr, &fence) != VK_SUCCESS) { fence = VK_NULL_HANDLE; static int logged; if (logged++ < 8) logf("vk: sparse texture %#llx: no fence for its binds", ...); sparse_unqueue(tex); return false; } ... } else { static int failed; if (failed++ < 8) logf("vk: sparse texture %#llx: binding failed (%d)", ...); sparse_unqueue(tex); return false; } ``` - **New per-texture free list** (`Texture::sparse_free`, with memory, offset and size). `sparse_take` gives it out before taking new chunk memory. A bind that keeps failing reuses the same memory instead of taking new memory on every retry. The list only points at the texture's own chunks, which are freed with the texture. - **Both messages are capped at 8.** A failing bind is now retried on later writes, so without the cap it would log on every write to that texture. The success path is unchanged. ### Who reaches this A texture is only sparse when it is a 2D array larger than `RX_VK_TEX_MAX_MB` (1.5 GB) on a device with sparse support. That is MKW's course-map case. OoT likely never makes one. ### Changes after review - The "binding failed" and "no fence for its binds" messages are capped at 8, like the other sparse messages. - The description now says when a texture is sparse, and that the OoT test does not reach this path. ### Test There is no dedicated unit test. The failure needs `vkQueueBindSparse` or `vkCreateFence` to fail on a sparse texture. ### How tested - Release build. - Zelda OoT on Radeon RX 9070 XT (Windows, Adrenalin), including about 40 minutes on nightly g57471723, which includes this change. Log clean, with no "sparse texture" line. - The sparse path itself was not run. The test only shows that non-sparse textures are unaffected. Assisted-by: GitHub Copilot Restored-from: https://github.com/NotRexed/Hookshot/pull/2743View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.