Skip to content

fix(vulkan): bind only bytes accessed by buffer-image copies - #1083

Open
Trycky64 wants to merge 2 commits into
NotRexed:mainfrom
Trycky64:resubmit/vulkan-copy-span
Open

Trycky64 wants to merge 2 commits into
NotRexed:mainfrom
Trycky64:resubmit/vulkan-copy-span

Conversation

@Trycky64

@Trycky64 Trycky64 commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Update

Rebased the buffer/image copy span fix onto current main (ac3bd4400e2e4eb82aaa18fd8207df642a37a6ed). The production change remains limited to gpu/vk/vk_xfer.cpp: the bound span includes row, layer, and depth-slice padding between accessed data, but stops after the final texel block Vulkan reads or writes. The #1082 and #1084 changes and tests remain in the base.

Following review feedback, the opt-in GPU fixture now selects a compatible Vulkan physical device by queue, format, and memory capabilities instead of requiring an AMD RX 9070. --gpu-device NAME remains available for diagnostics. A new runner-discovered compile-only check builds the GPU fixture when a compiler and Vulkan headers are available; it does not load Vulkan or run GPU work.

Validation

  • Windows targeted runner: 5 passed, 0 failed, 0 skipped (copy-span, GPU-fixture compile, partial-clear mip, and program-cache index/reader checks).
  • Linux targeted runner: 5 passed, 0 failed, 0 skipped for the same checks.
  • RX 9070 Vulkan probe after rebase: passed on Vulkan 1.4.359, driver 0x8001a0; four texels matched and padding remained untouched. Khronos validation layers were unavailable. An explicit RX 9070 selector also passed; a non-matching selector returned SKIP cleanly.
  • Full Windows Vulkan backend build: hookshot_vk.dll linked successfully from an isolated temporary build directory; 185 compile jobs, 0 failed. No game files or output directory were used.
  • Full Windows runner on current main: 156 checks, 128 passed, 5 failed, 23 skipped. On this branch: 156 checks, 126 passed, the same 5 failed, and 25 skipped. The failures were nx2/test_nup_read_name.py, nx2/test_nup_paths.py, tests/test_fps_overlay_order.py, tests/test_fpx_sticky.py, and tests/test_precompile_compute_guard.py; no additional failing check appeared on this branch. nx2/test_nup_paths.py also causes the runner's final Windows summary to raise UnicodeEncodeError while printing a replacement character, on both base and branch.

No in-game test was performed, and the real-device run covers only the RX 9070. This change does not claim to fix issues #696, #723, or #708.

This resubmission updates the existing PR #1083 after the reviewer requested portable GPU-device selection and automatic fixture compilation. It remains independent of the partial-clear mip fix.

@hckrman101 hckrman101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through the span maths and it checks out. With bufferRowLength = (row/bpb)bw and bufferImageHeight = hbbh, Vulkan's slice stride is hbrow and layers are addressed like depth slices, so the last block touched ends at row(hblayersd - 1) + wb*bpb. hb/layers/d are all rejected when zero in resolve(), so the -1 can't underflow, and the uint64 cast comes before the multiply. The base address doesn't change, so bufferOffset alignment is the same as before. Each call is a single region.

The smaller size also flows into memory_stamp_gpu / texture_gpu_wrote on the image->buffer path, which is what you want: write-back no longer covers the trailing stride padding past the last row. The CPU fallback (texture_read_texels) isn't affected.

The fixture is registered in run_all.py, uses the existing {vulkan_headers} placeholder, and builds from the production functions, so it can't drift from vk_xfer.cpp. The 8 cases cover the padded/unpadded, array, 3D, BC edge, truncated pool and bad stride paths, both directions.

Two small things, neither blocking:

  • vulkan_copy_span_gpu.cpp only runs with --gpu, so CI never compiles it and it can bitrot without anyone noticing. It also only accepts an RX 9070 (see inline).
  • #1084 also touches vk_xfer.cpp and run_all.py. Different hunks, but whichever lands second may need a small rebase on run_all.py.

Comment thread tests/vulkan_copy_span_gpu.cpp Outdated
VkPhysicalDevice physical{}; VkPhysicalDeviceProperties properties{};
for(auto candidate:devices) {
vkGetPhysicalDeviceProperties(candidate,&properties);
if(properties.vendorID==0x1002 && std::strstr(properties.deviceName,"RX 9070")) { physical=candidate;break; }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: as written, this fails with CHECK(physical) on any machine without an RX 9070. Nothing here looks AMD-specific, so maybe take the first device that has a graphics queue (with an optional name filter), or print a skip message when there's no match, so other people can run it with --gpu.

@Trycky64
Trycky64 force-pushed the resubmit/vulkan-copy-span branch from 5befd2f to 4d20a5f Compare November 9, 2026 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants