From 6ca021b9de97cf6ee088bf00ba9bcbdc62368cbf Mon Sep 17 00:00:00 2001 From: utzcoz Date: Fri, 10 Jul 2026 01:15:39 +0800 Subject: [PATCH] vulkan: Fix D24S8 depth-aspect buffer copy layout 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 d60d3457ac1f ("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 --- .github/workflows/presubmit_bazel.yml | 1 + host/vulkan/BUILD.bazel | 15 ++++ host/vulkan/vk_common_operations_tests.cpp | 80 ++++++++++++++++++- host/vulkan/vk_format_utils.cpp | 22 +++--- host/vulkan/vk_format_utils_unittest.cpp | 91 ++++++++++++++++++++++ 5 files changed, 199 insertions(+), 10 deletions(-) diff --git a/.github/workflows/presubmit_bazel.yml b/.github/workflows/presubmit_bazel.yml index dd89c314f..9889bcab5 100644 --- a/.github/workflows/presubmit_bazel.yml +++ b/.github/workflows/presubmit_bazel.yml @@ -72,5 +72,6 @@ jobs: host/vulkan:gfxstream_compositorvk_tests \ host/vulkan:gfxstream_emulatedphysicalmemory_tests \ host/vulkan:vk_common_operations_tests \ + host/vulkan:vk_format_utils_tests \ tests/end2end:gfxstream_end2end_tests \ ${{ inputs.additional-bazel-args }} diff --git a/host/vulkan/BUILD.bazel b/host/vulkan/BUILD.bazel index e97a8916a..b48c2a8a7 100644 --- a/host/vulkan/BUILD.bazel +++ b/host/vulkan/BUILD.bazel @@ -244,6 +244,21 @@ cc_test( ], ) +cc_test( + name = "vk_format_utils_tests", + srcs = [ + "vk_format_utils_unittest.cpp", + ], + copts = GFXSTREAM_HOST_COPTS, + deps = [ + ":gfxstream_vulkan_server", + "//common/base:gfxstream_common_base", + "//host:gfxstream_backend_static", + "@com_google_googletest//:gtest", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "vk_common_operations_tests", srcs = [ diff --git a/host/vulkan/vk_common_operations_tests.cpp b/host/vulkan/vk_common_operations_tests.cpp index d02fbfed6..a4dab8f15 100644 --- a/host/vulkan/vk_common_operations_tests.cpp +++ b/host/vulkan/vk_common_operations_tests.cpp @@ -17,11 +17,13 @@ #include #include +#include #include #include #include "gfxstream/common/testing/graphics_test_environment.h" #include "gfxstream/host/features.h" +#include "vulkan/vk_decoder_global_state.h" #include "vulkan/vulkan_dispatch.h" namespace gfxstream { @@ -51,9 +53,16 @@ class VkEmulationBufferTransferTest : public ::testing::Test { mVkEmu = VkEmulation::create(vk, {}, features); ASSERT_NE(mVkEmu, nullptr); + + // The color buffer update path consults VkDecoderGlobalState; initialize it as + // FrameBuffer does. + VkDecoderGlobalState::initialize(mVkEmu.get()); } - void TearDown() override { mVkEmu.reset(); } + void TearDown() override { + VkDecoderGlobalState::reset(); + mVkEmu.reset(); + } std::unique_ptr mVkEmu; }; @@ -121,6 +130,75 @@ TEST_F(VkEmulationBufferTransferTest, RejectsUndersizedDepthStencilColorBufferTr mVkEmu->teardownVkColorBuffer(kColorBufferHandle); } +// The packed depth/stencil formats share the plane pack/unpack machinery; iterate over +// the subset the driver supports. +constexpr GfxstreamFormat kPackedDepthStencilFormats[] = { + GfxstreamFormat::D24_UNORM_S8_UINT, + GfxstreamFormat::D32_FLOAT_S8_UINT, +}; + +TEST_F(VkEmulationBufferTransferTest, RoundTripsDepthStencilColorBufferContents) { + constexpr uint32_t kWidth = 8; + constexpr uint32_t kHeight = 8; + + uint32_t nextColorBufferHandle = 9; + size_t testedFormats = 0; + for (const GfxstreamFormat format : kPackedDepthStencilFormats) { + SCOPED_TRACE(ToString(format)); + if (!mVkEmu->isFormatSupported(format)) { + continue; + } + ++testedFormats; + + const uint32_t colorBufferHandle = nextColorBufferHandle++; + ASSERT_TRUE(mVkEmu->createVkColorBuffer(kWidth, kHeight, format, colorBufferHandle, + /*vulkanOnly=*/true, + VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT, + /*mipLevels=*/1)); + + std::vector full; + ASSERT_TRUE(mVkEmu->readColorBufferToBytes(colorBufferHandle, &full)); + + // Update packs interleaved pixels into per-aspect planes and read unpacks them + // back; they must be exact inverses. Float depth must hold in-range [0.0, 1.0] + // values (out-of-range is undefined; observed zeroed on RADV) that are exact + // binary fractions so readback is bit-identical; 24-bit UNORM depth accepts any + // bit pattern. The i * 17 + 3 byte sequence keeps adjacent bytes distinct and + // repeats only every 256 bytes, so shifted or dropped bytes fail the comparison. + std::vector pattern(full.size()); + if (format == GfxstreamFormat::D32_FLOAT_S8_UINT) { + // Interleaved D32S8 pixel: 4-byte float depth, then 1 stencil byte. + ASSERT_EQ(pattern.size() % 5, 0u); + const size_t pixelCount = pattern.size() / 5; + for (size_t i = 0; i < pixelCount; ++i) { + const float depth = static_cast(i) / static_cast(pixelCount); + std::memcpy(&pattern[i * 5], &depth, sizeof(depth)); + pattern[i * 5 + 4] = static_cast(i * 17 + 3); // stencil byte + } + } else { + // Interleaved D24S8 pixels are 4 bytes each. Whole-buffer reads return the + // larger staging-sized buffer, whose bytes past the interleaved pixels do + // not round-trip (they read back as zero), so leave them zero. + const size_t pixelBytes = kWidth * kHeight * 4; + ASSERT_LE(pixelBytes, pattern.size()); + for (size_t i = 0; i < pixelBytes; ++i) { + pattern[i] = static_cast(i * 17 + 3); + } + } + ASSERT_TRUE(mVkEmu->updateColorBufferFromBytes(colorBufferHandle, pattern)); + + std::vector readback; + ASSERT_TRUE(mVkEmu->readColorBufferToBytes(colorBufferHandle, &readback)); + EXPECT_EQ(readback, pattern); + + mVkEmu->teardownVkColorBuffer(colorBufferHandle); + } + + if (testedFormats == 0) { + GTEST_SKIP() << "No packed depth/stencil format supported by this driver"; + } +} + } // namespace } // namespace vk } // namespace host diff --git a/host/vulkan/vk_format_utils.cpp b/host/vulkan/vk_format_utils.cpp index 5319a34ad..03026e13c 100644 --- a/host/vulkan/vk_format_utils.cpp +++ b/host/vulkan/vk_format_utils.cpp @@ -71,32 +71,36 @@ void UnpackD16S8(const VkExtent3D& extent, const uint8_t* src, uint8_t* dst) { } } +// Per the Vulkan spec, buffer copies of the depth aspect of D24 formats use +// VK_FORMAT_X8_D24_UNORM_PACK32: one 32-bit word per texel, upper 8 bits unused. void PackD24S8(const VkExtent3D& extent, const uint8_t* src, uint8_t* dst) { const uint32_t numPixels = extent.width * extent.height * extent.depth; - const uint32_t plane0Size = numPixels * 3; + const uint32_t plane0Size = numPixels * 4; const uint32_t plane1Offset = alignToPower2(plane0Size, 4); const uint8_t* srcPixels = src; uint8_t* dstDepth = dst; uint8_t* dstStencil = dst + plane1Offset; for (uint32_t i = 0; i < numPixels; ++i) { - dstDepth[i * 3 + 0] = srcPixels[i * 4 + 0]; - dstDepth[i * 3 + 1] = srcPixels[i * 4 + 1]; - dstDepth[i * 3 + 2] = srcPixels[i * 4 + 2]; + dstDepth[i * 4 + 0] = srcPixels[i * 4 + 0]; + dstDepth[i * 4 + 1] = srcPixels[i * 4 + 1]; + dstDepth[i * 4 + 2] = srcPixels[i * 4 + 2]; + // Unused X8 byte; zero it so the staging contents are deterministic. + dstDepth[i * 4 + 3] = 0; dstStencil[i] = srcPixels[i * 4 + 3]; } } void UnpackD24S8(const VkExtent3D& extent, const uint8_t* src, uint8_t* dst) { const uint32_t numPixels = extent.width * extent.height * extent.depth; - const uint32_t plane0Size = numPixels * 3; + const uint32_t plane0Size = numPixels * 4; const uint32_t plane1Offset = alignToPower2(plane0Size, 4); const uint8_t* srcDepth = src; const uint8_t* srcStencil = src + plane1Offset; uint8_t* dstPixels = dst; for (uint32_t i = 0; i < numPixels; ++i) { - dstPixels[i * 4 + 0] = srcDepth[i * 3 + 0]; - dstPixels[i * 4 + 1] = srcDepth[i * 3 + 1]; - dstPixels[i * 4 + 2] = srcDepth[i * 3 + 2]; + dstPixels[i * 4 + 0] = srcDepth[i * 4 + 0]; + dstPixels[i * 4 + 1] = srcDepth[i * 4 + 1]; + dstPixels[i * 4 + 2] = srcDepth[i * 4 + 2]; dstPixels[i * 4 + 3] = srcStencil[i]; } } @@ -247,7 +251,7 @@ const std::unordered_map& getFormatPlaneLayoutsMap { .horizontalSubsampling = 1, .verticalSubsampling = 1, - .sampleIncrementBytes = 3, + .sampleIncrementBytes = 4, .aspectMask = VK_IMAGE_ASPECT_DEPTH_BIT, }, { diff --git a/host/vulkan/vk_format_utils_unittest.cpp b/host/vulkan/vk_format_utils_unittest.cpp index bf6ba522e..6ec7322a8 100644 --- a/host/vulkan/vk_format_utils_unittest.cpp +++ b/host/vulkan/vk_format_utils_unittest.cpp @@ -29,6 +29,7 @@ using ::testing::ExplainMatchResult; using ::testing::Field; using ::testing::IsFalse; using ::testing::IsTrue; +using ::testing::NotNull; MATCHER_P(EqsVkExtent3D, expected, "") { return ExplainMatchResult(AllOf(Field("width", &VkExtent3D::width, Eq(expected.width)), @@ -254,6 +255,96 @@ TEST(VkFormatUtilsTest, GetTransferInfoYV12OrYV21) { }))); } +TEST(VkFormatUtilsTest, GetTransferInfoD24S8) { + const VkFormat format = VK_FORMAT_D24_UNORM_S8_UINT; + const uint32_t width = 16; + const uint32_t height = 16; + + TransferInfo transferInfo; + ASSERT_THAT(getFormatTransferInfo(format, {width, height, 1}, &transferInfo), IsTrue()); + // Staging stores a 4-byte-per-texel depth plane plus a 1-byte-per-texel stencil + // plane. + EXPECT_THAT(transferInfo.stagingBufferCopySize, Eq(1280)); + ASSERT_THAT(transferInfo.bufferImageCopies, + ElementsAre(EqsVkBufferImageCopy(VkBufferImageCopy{ + .bufferOffset = 0, + .bufferRowLength = 16, + .bufferImageHeight = 0, + .imageSubresource = + { + .aspectMask = VK_IMAGE_ASPECT_DEPTH_BIT, + .mipLevel = 0, + .baseArrayLayer = 0, + .layerCount = 1, + }, + .imageOffset = + { + .x = 0, + .y = 0, + .z = 0, + }, + .imageExtent = + { + .width = 16, + .height = 16, + .depth = 1, + }, + }), + EqsVkBufferImageCopy(VkBufferImageCopy{ + .bufferOffset = 1024, + .bufferRowLength = 16, + .bufferImageHeight = 0, + .imageSubresource = + { + .aspectMask = VK_IMAGE_ASPECT_STENCIL_BIT, + .mipLevel = 0, + .baseArrayLayer = 0, + .layerCount = 1, + }, + .imageOffset = + { + .x = 0, + .y = 0, + .z = 0, + }, + .imageExtent = + { + .width = 16, + .height = 16, + .depth = 1, + }, + }))); +} + +TEST(VkFormatUtilsTest, PackUnpackD24S8RoundTrip) { + const VkFormat format = VK_FORMAT_D24_UNORM_S8_UINT; + const VkExtent3D extent = {4, 4, 1}; + + TransferInfo transferInfo; + ASSERT_THAT(getFormatTransferInfo(format, extent, &transferInfo), IsTrue()); + ASSERT_THAT(transferInfo.packFunction, NotNull()); + ASSERT_THAT(transferInfo.unpackFunction, NotNull()); + + // Interleaved D24S8 pixels are 4 bytes each. + std::vector natural(extent.width * extent.height * extent.depth * 4); + for (size_t i = 0; i < natural.size(); ++i) { + natural[i] = static_cast(i * 7 + 1); + } + + std::vector staging(transferInfo.stagingBufferCopySize, 0xaa); + transferInfo.packFunction(extent, natural.data(), staging.data()); + + // The upper byte of each 32-bit depth word must be written as zero. + const size_t pixelCount = extent.width * extent.height; + for (size_t i = 0; i < pixelCount; ++i) { + EXPECT_THAT(staging[i * 4 + 3], Eq(0)) << "depth word " << i; + } + + std::vector roundTripped(natural.size(), 0); + transferInfo.unpackFunction(extent, staging.data(), roundTripped.data()); + EXPECT_THAT(roundTripped, Eq(natural)); +} + TEST(VkFormatUtilsTest, GetTransferInfoDepthStencilWithDepth) { const VkFormat format = VK_FORMAT_D16_UNORM_S8_UINT; const uint32_t width = 16;