Vulkan:ImageHelper read combined DS textures

src/libANGLE/renderer/vulkan/TextureVk.cpp

@@ -969,11 +969,13 @@ angle::Result TextureVk::copyImageDataToBufferAndGetData(ContextVk *contextVk,
 
     gl::Box area(0, 0, 0, sourceArea.width, sourceArea.height, 1);
 
-    vk::BufferHelper *copyBuffer  = nullptr;
-    VkDeviceSize sourceCopyOffset = 0;
+    vk::BufferHelper *copyBuffer                   = nullptr;
+    vk::StagingBufferOffsetArray sourceCopyOffsets = {0, 0};
+    size_t bufferSize                              = 0;
 
     ANGLE_TRY(mImage->copyImageDataToBuffer(contextVk, sourceLevel, layerCount, 0, area,
-                                            &copyBuffer, &sourceCopyOffset, outDataPtr));
+                                            &copyBuffer, &bufferSize, &sourceCopyOffsets,
+                                            outDataPtr));
 
     // Explicitly finish. If new use cases arise where we don't want to block we can change this.
     ANGLE_TRY(contextVk->finishImpl());
@@ -1106,8 +1108,7 @@ angle::Result TextureVk::copyImageDataToStagingBuffer(ContextVk *contextVk,
                                                       uint32_t stagingDstMipLevel,
                                                       vk::BufferHelper **stagingBuffer)
 {
-    const gl::Extents &baseLevelExtents       = desc.size;
-    const gl::InternalFormat &baseLevelFormat = *desc.format.info;
+    const gl::Extents &baseLevelExtents = desc.size;
 
     VkExtent3D updatedExtents;
     VkOffset3D offset = {};
@@ -1123,18 +1124,18 @@ angle::Result TextureVk::copyImageDataToStagingBuffer(ContextVk *contextVk,
     }
 
     // Copy from the base level image to the staging buffer
-    VkDeviceSize stagingBufferOffset = 0;
+    vk::StagingBufferOffsetArray stagingBufferOffsets = {0, 0};
+    size_t bufferSize                                 = 0;
     ANGLE_TRY(mImage->copyImageDataToBuffer(contextVk, sourceMipLevel, layerCount, currentLayer,
-                                            area, stagingBuffer, &stagingBufferOffset, nullptr));
+                                            area, stagingBuffer, &bufferSize, &stagingBufferOffsets,
+                                            nullptr));
 
     // Stage an update to the new image
-    size_t bufferSize = updatedExtents.width * updatedExtents.height * updatedExtents.depth *
-                        baseLevelFormat.pixelBytes;
 
     ASSERT(*stagingBuffer);
     ANGLE_TRY(mImage->stageSubresourceUpdateFromBuffer(
         contextVk, bufferSize, stagingDstMipLevel, currentLayer, layerCount, updatedExtents, offset,
-        *stagingBuffer, stagingBufferOffset));
+        *stagingBuffer, stagingBufferOffsets));
 
     // Set up write dependency, we are writing to this buffer from the old image.
     (*stagingBuffer)->onWrite(contextVk, mImage, 0, VK_ACCESS_TRANSFER_WRITE_BIT);

src/libANGLE/renderer/vulkan/vk_helpers.cpp

@@ -1874,6 +1874,12 @@ VkImageAspectFlags ImageHelper::getAspectFlags() const
     return GetFormatAspectFlags(mFormat->actualImageFormat());
 }
 
+bool ImageHelper::isCombinedDepthStencilFormat() const
+{
+    return ((VK_IMAGE_ASPECT_DEPTH_BIT | VK_IMAGE_ASPECT_STENCIL_BIT) & getAspectFlags()) ==
+           (VK_IMAGE_ASPECT_DEPTH_BIT | VK_IMAGE_ASPECT_STENCIL_BIT);
+}
+
 VkImageLayout ImageHelper::getCurrentLayout() const
 {
     return kImageMemoryBarrierData[mCurrentLayout].layout;
@@ -2463,27 +2469,40 @@ angle::Result ImageHelper::stageSubresourceUpdateFromBuffer(ContextVk *contextVk
                                                             const VkExtent3D &extent,
                                                             const VkOffset3D &offset,
                                                             BufferHelper *bufferHelper,
-                                                            VkDeviceSize stagingOffset)
+                                                            StagingBufferOffsetArray stagingOffsets)
 {
     // This function stages an update from explicitly provided handle and offset
     // It is used when the texture base level has changed, and we need to propagate data
 
-    VkBufferImageCopy copy               = {};
-    copy.bufferOffset                    = stagingOffset;
-    copy.bufferRowLength                 = extent.width;
-    copy.bufferImageHeight               = extent.height;
-    copy.imageSubresource.aspectMask     = getAspectFlags();
-    copy.imageSubresource.mipLevel       = mipLevel;
-    copy.imageSubresource.baseArrayLayer = baseArrayLayer;
-    copy.imageSubresource.layerCount     = layerCount;
-    copy.imageOffset                     = offset;
-    copy.imageExtent                     = extent;
-
-    ASSERT(getAspectFlags() == VK_IMAGE_ASPECT_COLOR_BIT ||
-           getAspectFlags() == VK_IMAGE_ASPECT_DEPTH_BIT ||
-           getAspectFlags() == VK_IMAGE_ASPECT_STENCIL_BIT);
-
-    mSubresourceUpdates.emplace_back(bufferHelper, copy);
+    VkBufferImageCopy copy[2]               = {};
+    copy[0].bufferOffset                    = stagingOffsets[0];
+    copy[0].bufferRowLength                 = extent.width;
+    copy[0].bufferImageHeight               = extent.height;
+    copy[0].imageSubresource.aspectMask     = getAspectFlags();
+    copy[0].imageSubresource.mipLevel       = mipLevel;
+    copy[0].imageSubresource.baseArrayLayer = baseArrayLayer;
+    copy[0].imageSubresource.layerCount     = layerCount;
+    copy[0].imageOffset                     = offset;
+    copy[0].imageExtent                     = extent;
+
+    if (isCombinedDepthStencilFormat())
+    {
+        // Force aspect to depth for first copy
+        copy[0].imageSubresource.aspectMask = VK_IMAGE_ASPECT_DEPTH_BIT;
+        // Copy stencil aspect separately
+        copy[1].bufferOffset                    = stagingOffsets[1];
+        copy[1].bufferRowLength                 = extent.width;
+        copy[1].bufferImageHeight               = extent.height;
+        copy[1].imageSubresource.aspectMask     = VK_IMAGE_ASPECT_STENCIL_BIT;
+        copy[1].imageSubresource.mipLevel       = mipLevel;
+        copy[1].imageSubresource.baseArrayLayer = baseArrayLayer;
+        copy[1].imageSubresource.layerCount     = layerCount;
+        copy[1].imageOffset                     = offset;
+        copy[1].imageExtent                     = extent;
+        mSubresourceUpdates.emplace_back(bufferHelper, copy[1]);
+    }
+
+    mSubresourceUpdates.emplace_back(bufferHelper, copy[0]);
 
     return angle::Result::Continue;
 }
@@ -2669,11 +2688,11 @@ angle::Result ImageHelper::allocateStagingMemory(ContextVk *contextVk,
                                                  size_t sizeInBytes,
                                                  uint8_t **ptrOut,
                                                  BufferHelper **bufferOut,
-                                                 VkDeviceSize *offsetOut,
+                                                 StagingBufferOffsetArray *offsetOut,
                                                  bool *newBufferAllocatedOut)
 {
     VkBuffer handle;
-    ANGLE_TRY(mStagingBuffer.allocate(contextVk, sizeInBytes, ptrOut, &handle, offsetOut,
+    ANGLE_TRY(mStagingBuffer.allocate(contextVk, sizeInBytes, ptrOut, &handle, &(*offsetOut)[0],
                                       newBufferAllocatedOut));
     *bufferOut = mStagingBuffer.getCurrentBuffer();
     return angle::Result::Continue;
@@ -2887,33 +2906,44 @@ angle::Result ImageHelper::copyImageDataToBuffer(ContextVk *contextVk,
                                                  uint32_t baseLayer,
                                                  const gl::Box &sourceArea,
                                                  BufferHelper **bufferOut,
-                                                 VkDeviceSize *bufferOffsetOut,
+                                                 size_t *bufferSize,
+                                                 StagingBufferOffsetArray *bufferOffsetsOut,
                                                  uint8_t **outDataPtr)
 {
     ANGLE_TRACE_EVENT0("gpu.angle", "ImageHelper::copyImageDataToBuffer");
 
     const angle::Format &imageFormat = mFormat->actualImageFormat();
-    size_t sourceCopyAllocationSize  = sourceArea.width * sourceArea.height * sourceArea.depth *
-                                      imageFormat.pixelBytes * layerCount;
+
+    // Two VK formats (one depth-only, one combined depth/stencil) use an extra byte for depth.
+    // From https://www.khronos.org/registry/vulkan/specs/1.1/html/vkspec.html#VkBufferImageCopy:
+    //  data copied to or from the depth aspect of a VK_FORMAT_X8_D24_UNORM_PACK32 or
+    //  VK_FORMAT_D24_UNORM_S8_UINT format is packed with one 32-bit word per texel...
+    // So make sure if we hit the depth/stencil format that we have 5 bytes per pixel (4 for depth
+    //  data, 1 for stencil). NOTE that depth-only VK_FORMAT_X8_D24_UNORM_PACK32 already has 4 bytes
+    //  per pixel which is sufficient to contain its depth aspect (no stencil aspect).
+    uint32_t pixelBytes         = imageFormat.pixelBytes;
+    uint32_t depthBytesPerPixel = imageFormat.depthBits >> 3;
+    if (mFormat->vkImageFormat == VK_FORMAT_D24_UNORM_S8_UINT)
+    {
+        pixelBytes         = 5;
+        depthBytesPerPixel = 4;
+    }
+
+    *bufferSize = sourceArea.width * sourceArea.height * sourceArea.depth * pixelBytes * layerCount;
 
     CommandBuffer *commandBuffer = nullptr;
     ANGLE_TRY(recordCommands(contextVk, &commandBuffer));
 
-    //  http://anglebug.com/3949: Need to handle DS combined aspect, will require copying D & S
-    //   separately. See ImageHelper::stageSubresourceUpdate for DS copy buff->image example.
-    ASSERT(getAspectFlags() == VK_IMAGE_ASPECT_COLOR_BIT ||
-           getAspectFlags() == VK_IMAGE_ASPECT_DEPTH_BIT ||
-           getAspectFlags() == VK_IMAGE_ASPECT_STENCIL_BIT);
-
     // Transition the image to readable layout
-    changeLayout(getAspectFlags(), ImageLayout::TransferSrc, commandBuffer);
+    const VkImageAspectFlags aspectFlags = getAspectFlags();
+    changeLayout(aspectFlags, ImageLayout::TransferSrc, commandBuffer);
 
     VkImageMemoryBarrier barrier            = {};
     barrier.sType                           = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER;
     barrier.image                           = mImage.getHandle();
     barrier.srcQueueFamilyIndex             = VK_QUEUE_FAMILY_IGNORED;
     barrier.dstQueueFamilyIndex             = VK_QUEUE_FAMILY_IGNORED;
-    barrier.subresourceRange.aspectMask     = getAspectFlags();
+    barrier.subresourceRange.aspectMask     = aspectFlags;
     barrier.subresourceRange.baseArrayLayer = baseLayer;
     barrier.subresourceRange.layerCount     = layerCount;
     barrier.subresourceRange.levelCount     = 1;
@@ -2927,26 +2957,61 @@ angle::Result ImageHelper::copyImageDataToBuffer(ContextVk *contextVk,
                                 &barrier);
 
     // Allocate staging buffer data
-    ANGLE_TRY(allocateStagingMemory(contextVk, sourceCopyAllocationSize, outDataPtr, bufferOut,
-                                    bufferOffsetOut, nullptr));
-
-    VkBufferImageCopy region               = {};
-    region.bufferOffset                    = *bufferOffsetOut;
-    region.bufferRowLength                 = 0;
-    region.bufferImageHeight               = 0;
-    region.imageExtent.width               = sourceArea.width;
-    region.imageExtent.height              = sourceArea.height;
-    region.imageExtent.depth               = sourceArea.depth;
-    region.imageOffset.x                   = sourceArea.x;
-    region.imageOffset.y                   = sourceArea.y;
-    region.imageOffset.z                   = sourceArea.z;
-    region.imageSubresource.aspectMask     = getAspectFlags();
-    region.imageSubresource.baseArrayLayer = baseLayer;
-    region.imageSubresource.layerCount     = layerCount;
-    region.imageSubresource.mipLevel       = static_cast<uint32_t>(sourceLevel);
+    ANGLE_TRY(allocateStagingMemory(contextVk, *bufferSize, outDataPtr, bufferOut, bufferOffsetsOut,
+                                    nullptr));
+
+    VkBufferImageCopy regions[2] = {};
+    // Default to non-combined DS case
+    regions[0].bufferOffset                    = (*bufferOffsetsOut)[0];
+    regions[0].bufferRowLength                 = 0;
+    regions[0].bufferImageHeight               = 0;
+    regions[0].imageExtent.width               = sourceArea.width;
+    regions[0].imageExtent.height              = sourceArea.height;
+    regions[0].imageExtent.depth               = sourceArea.depth;
+    regions[0].imageOffset.x                   = sourceArea.x;
+    regions[0].imageOffset.y                   = sourceArea.y;
+    regions[0].imageOffset.z                   = sourceArea.z;
+    regions[0].imageSubresource.aspectMask     = aspectFlags;
+    regions[0].imageSubresource.baseArrayLayer = baseLayer;
+    regions[0].imageSubresource.layerCount     = layerCount;
+    regions[0].imageSubresource.mipLevel       = static_cast<uint32_t>(sourceLevel);
+
+    if (isCombinedDepthStencilFormat())
+    {
+        // For combined DS image we'll copy depth and stencil aspects separately
+        // Depth aspect comes first in buffer and can use most settings from above
+        regions[0].imageSubresource.aspectMask = VK_IMAGE_ASPECT_DEPTH_BIT;
+
+        // Get depth data size since stencil data immediately follows depth data in buffer
+        const VkDeviceSize depthSize = depthBytesPerPixel * sourceArea.width * sourceArea.height *
+                                       sourceArea.depth * layerCount;
+
+        // Double-check that we allocated enough buffer space (always 1 byte per stencil)
+        ASSERT(*bufferSize >= (depthSize + (sourceArea.width * sourceArea.height *
+                                            sourceArea.depth * layerCount)));
+
+        // Copy stencil data into buffer immediately following the depth data
+        const VkDeviceSize stencilOffset           = (*bufferOffsetsOut)[0] + depthSize;
+        (*bufferOffsetsOut)[1]                     = stencilOffset;
+        regions[1].bufferOffset                    = stencilOffset;
+        regions[1].bufferRowLength                 = 0;
+        regions[1].bufferImageHeight               = 0;
+        regions[1].imageExtent.width               = sourceArea.width;
+        regions[1].imageExtent.height              = sourceArea.height;
+        regions[1].imageExtent.depth               = sourceArea.depth;
+        regions[1].imageOffset.x                   = sourceArea.x;
+        regions[1].imageOffset.y                   = sourceArea.y;
+        regions[1].imageOffset.z                   = sourceArea.z;
+        regions[1].imageSubresource.aspectMask     = VK_IMAGE_ASPECT_STENCIL_BIT;
+        regions[1].imageSubresource.baseArrayLayer = baseLayer;
+        regions[1].imageSubresource.layerCount     = layerCount;
+        regions[1].imageSubresource.mipLevel       = static_cast<uint32_t>(sourceLevel);
+        commandBuffer->copyImageToBuffer(mImage, getCurrentLayout(),
+                                         (*bufferOut)->getBuffer().getHandle(), 1, &regions[1]);
+    }
 
     commandBuffer->copyImageToBuffer(mImage, getCurrentLayout(),
-                                     (*bufferOut)->getBuffer().getHandle(), 1, &region);
+                                     (*bufferOut)->getBuffer().getHandle(), 1, regions);
 
     barrier.oldLayout = VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL;
     barrier.newLayout = VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL;

src/libANGLE/renderer/vulkan/vk_helpers.h

@@ -35,6 +35,8 @@ constexpr VkBufferUsageFlags kStagingBufferFlags =
     VK_BUFFER_USAGE_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_TRANSFER_DST_BIT;
 constexpr size_t kStagingBufferSize = 1024 * 16;
 
+using StagingBufferOffsetArray = std::array<VkDeviceSize, 2>;
+
 struct TextureUnit final
 {
     TextureVk *texture;
@@ -731,6 +733,8 @@ class ImageHelper final : public CommandGraphResource
     bool valid() const { return mImage.valid(); }
 
     VkImageAspectFlags getAspectFlags() const;
+    // True if image contains both depth & stencil aspects
+    bool isCombinedDepthStencilFormat() const;
     void destroy(VkDevice device);
 
     void init2DWeakReference(VkImage handle,
@@ -806,7 +810,7 @@ class ImageHelper final : public CommandGraphResource
                                                    const VkExtent3D &extent,
                                                    const VkOffset3D &offset,
                                                    BufferHelper *stagingBuffer,
-                                                   VkDeviceSize stagingOffset);
+                                                   StagingBufferOffsetArray stagingOffsets);
 
     angle::Result stageSubresourceUpdateFromFramebuffer(const gl::Context *context,
                                                         const gl::ImageIndex &index,
@@ -839,7 +843,7 @@ class ImageHelper final : public CommandGraphResource
                                         size_t sizeInBytes,
                                         uint8_t **ptrOut,
                                         BufferHelper **bufferOut,
-                                        VkDeviceSize *offsetOut,
+                                        StagingBufferOffsetArray *offsetOut,
                                         bool *newBufferAllocatedOut);
 
     // Flushes staged updates to a range of levels and layers from start to (but not including) end.
@@ -888,7 +892,8 @@ class ImageHelper final : public CommandGraphResource
                                         uint32_t baseLayer,
                                         const gl::Box &sourceArea,
                                         BufferHelper **bufferOut,
-                                        VkDeviceSize *bufferOffsetOut,
+                                        size_t *bufferSize,
+                                        StagingBufferOffsetArray *bufferOffsetsOut,
                                         uint8_t **outDataPtr);
 
     static angle::Result GetReadPixelsParams(ContextVk *contextVk,

src/tests/deqp_support/deqp_gles31_test_expectations.txt

@@ -637,7 +637,7 @@
 3683 VULKAN PIXEL2ORXL : dEQP-GLES31.functional.stencil_texturing.misc.compare_mode_effect = FAIL
 
 // Need to support non-color when staging image updates
-3949 VULKAN : dEQP-GLES31.functional.stencil_texturing.misc.base_level = SKIP
+4080 ANDROID VULKAN : dEQP-GLES31.functional.stencil_texturing.misc.base_level = SKIP
 
 // Multisampled textures:
 3565 VULKAN : dEQP-GLES31.functional.texture.multisample.* = SKIP