vulkan: Fix D24S8 depth-aspect buffer copy layout

.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 }}

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 = [

host/vulkan/vk_common_operations_tests.cpp

@@ -17,11 +17,13 @@
 #include <gtest/gtest.h>
 
 #include <cstdint>
+#include <cstring>
 #include <memory>
 #include <vector>
 
 #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<VkEmulation> 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<uint8_t> 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<uint8_t> 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<float>(i) / static_cast<float>(pixelCount);
+                std::memcpy(&pattern[i * 5], &depth, sizeof(depth));
+                pattern[i * 5 + 4] = static_cast<uint8_t>(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<uint8_t>(i * 17 + 3);
+            }
+        }
+        ASSERT_TRUE(mVkEmu->updateColorBufferFromBytes(colorBufferHandle, pattern));
+
+        std::vector<uint8_t> 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

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<VkFormat, FormatPlaneLayouts>& getFormatPlaneLayoutsMap
                          {
                              .horizontalSubsampling = 1,
                              .verticalSubsampling = 1,
-                             .sampleIncrementBytes = 3,
+                             .sampleIncrementBytes = 4,
                              .aspectMask = VK_IMAGE_ASPECT_DEPTH_BIT,
                          },
                          {

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<uint8_t> natural(extent.width * extent.height * extent.depth * 4);
+    for (size_t i = 0; i < natural.size(); ++i) {
+        natural[i] = static_cast<uint8_t>(i * 7 + 1);
+    }
+
+    std::vector<uint8_t> 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<uint8_t> 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;