vulkan: Fix D24S8 depth-aspect buffer copy layout#162
Open
utzcoz wants to merge 1 commit into
Open
Conversation
The Vulkan spec ("Copying Data Between Buffers and Images",
depth/stencil aspect copy table) addresses buffer copies of the depth
aspect of VK_FORMAT_D24_UNORM_S8_UINT as VK_FORMAT_X8_D24_UNORM_PACK32:
one 32-bit word per texel with 24 depth bits and 8 unused bits. The
transfer info introduced by d60d345 ("vk-format: separate each
aspect in image copy operation") instead packed depth tightly at 3
bytes per texel, so spec-conformant drivers reading and writing 4-byte
texels returned corrupted color buffer contents.
Observed on llvmpipe: gallium spells VK_FORMAT_D24_UNORM_S8_UINT as
PIPE_FORMAT_Z24_UNORM_S8_UINT (vk_format_to_pipe_format() in
src/vulkan/util/vk_format.c), and lavapipe's copy_depth_rect() in
src/gallium/frontends/lavapipe/lvp_image.c copies its depth aspect via
util_format_z24_unorm_s8_uint_{pack,unpack}_z24() in
src/util/format/u_format_zs.c, which read and write the buffer as one
uint32_t per texel and zero the upper byte on readback:
https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/util/format/u_format_zs.c
Store the depth plane as one 32-bit word per texel in the staging
buffer, writing the unused upper byte as zero on upload and ignoring it
on readback. The transfer paths keep sizing reads and bounds checks
with the staging copy size, which for D24S8 is now larger than the
interleaved guest pixels: the guest-facing size checks stay
conservative, and whole-buffer readbacks return staging-sized contents
whose bytes past the interleaved pixels are zero.
Note: this changes the staging representation recorded by snapshots for
D24S8 images, so pre-fix snapshots holding such images will fail the
size check on load.
Also wires vk_format_utils_unittest into bazel and the presubmit
workflow so the new layout and pack/unpack round-trip unit tests run in
CI.
Test: bazel test //host/vulkan:vk_format_utils_tests
Test: bazel test //host/vulkan:vk_common_operations_tests
Test: bazel test --graphics_drivers=gles_angle_vulkan_swiftshader
//host/vulkan:vk_common_operations_tests
Test: bazel build //host/vulkan:vk_common_operations_tests &&
VK_DRIVER_FILES=/usr/share/vulkan/icd.d/lvp_icd.json
bazel-bin/host/vulkan/vk_common_operations_tests
Test: bazel test --graphics_drivers=gles_angle_vulkan_swiftshader
host:gfxstream_framebuffer_tests tests/end2end:gfxstream_end2end_tests
Change-Id: Icbf1a98bd197e3eb2f9bfc8452f3918d6813484f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Vulkan spec ("Copying Data Between Buffers and Images", depth/stencil aspect copy table) addresses buffer copies of the depth aspect of VK_FORMAT_D24_UNORM_S8_UINT as VK_FORMAT_X8_D24_UNORM_PACK32: one 32-bit word per texel with 24 depth bits and 8 unused bits. The transfer info introduced by d60d345 ("vk-format: separate each aspect in image copy operation") instead packed depth tightly at 3 bytes per texel, so spec-conformant drivers reading and writing 4-byte texels returned corrupted color buffer contents.
Observed on llvmpipe: gallium spells VK_FORMAT_D24_UNORM_S8_UINT as PIPE_FORMAT_Z24_UNORM_S8_UINT (vk_format_to_pipe_format() in src/vulkan/util/vk_format.c), and lavapipe's copy_depth_rect() in src/gallium/frontends/lavapipe/lvp_image.c copies its depth aspect via util_format_z24_unorm_s8_uint_{pack,unpack}_z24() in src/util/format/u_format_zs.c, which read and write the buffer as one uint32_t per texel and zero the upper byte on readback: https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/util/format/u_format_zs.c
Store the depth plane as one 32-bit word per texel in the staging buffer, writing the unused upper byte as zero on upload and ignoring it on readback. The transfer paths keep sizing reads and bounds checks with the staging copy size, which for D24S8 is now larger than the interleaved guest pixels: the guest-facing size checks stay conservative, and whole-buffer readbacks return staging-sized contents whose bytes past the interleaved pixels are zero.
Note: this changes the staging representation recorded by snapshots for D24S8 images, so pre-fix snapshots holding such images will fail the size check on load.
Also wires vk_format_utils_unittest into bazel and the presubmit workflow so the new layout and pack/unpack round-trip unit tests run in CI.
Test: bazel test //host/vulkan:vk_format_utils_tests Test: bazel test //host/vulkan:vk_common_operations_tests Test: bazel test --graphics_drivers=gles_angle_vulkan_swiftshader
//host/vulkan:vk_common_operations_tests
Test: bazel build //host/vulkan:vk_common_operations_tests &&
VK_DRIVER_FILES=/usr/share/vulkan/icd.d/lvp_icd.json
bazel-bin/host/vulkan/vk_common_operations_tests
Test: bazel test --graphics_drivers=gles_angle_vulkan_swiftshader
host:gfxstream_framebuffer_tests tests/end2end:gfxstream_end2end_tests
Change-Id: Icbf1a98bd197e3eb2f9bfc8452f3918d6813484f