Polish 2D texture copy splitter on D3D12

In every copy region, its bufferSize.height minus rowsPerImage
can be texture format's blockInfo.height (it is 1 for uncompressed
formats) at most, and it appears only if bytesPerRow is 256 and
copySize on height is a full copy.

This change will benefit 3D texture copy splitter via removing
unwanted empty rows issues. Because empty rows in 3D copy splitter
may lead to recompute/modify copy regions and there might be new
copy regions added.

The removed empty row situations are:
  1) Partial copy on height: copySize.height < rowsPerImage *
  blockInfo.height
  2) bytesPerRow is greater than 512. For example, if bytesPerRow
  is 512 and data in one row straddles two rows and there is no
  empty row at the first part. The second part will have a fake
  empty row if we don't recompute its alignedOffset.
  3) There are two empty rows in a copy region. For example:
  if data in one row straddles two rows and there is an empty row
  in the first copy region. Then there will be two empty rows in
  the copy region of the second part if we don't recompute the
  alignedOffset.

This change also fixes an issue found by Corentin that copy related
argument "rowsPerImage" should not take effect when we are copying
one single depth or array slice.

Bug: dawn:547

Change-Id: I603291d559de1d05e420e5ed1f4cabf53de5a93f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/52680
Reviewed-by: Corentin Wallez <cwallez@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Yunchao He <yunchao.he@intel.com>

src/dawn_native/d3d12/TextureCopySplitter.cpp

@@ -37,8 +37,7 @@ namespace dawn_native { namespace d3d12 {
                                                            Extent3D copySize,
                                                            const TexelBlockInfo& blockInfo,
                                                            uint64_t offset,
-                                                           uint32_t bytesPerRow,
+                                                           uint32_t bytesPerRow) {
-                                                           uint32_t rowsPerImage) {
         TextureCopySubresource copy;
 
         ASSERT(bytesPerRow % blockInfo.byteSize == 0);
@@ -88,11 +87,11 @@ namespace dawn_native { namespace d3d12 {
         Origin3D texelOffset = ComputeTexelOffsets(
             blockInfo, static_cast<uint32_t>(offset - alignedOffset), bytesPerRow);
 
+        ASSERT(texelOffset.y <= blockInfo.height);
         ASSERT(texelOffset.z == 0);
 
         uint32_t copyBytesPerRowPitch = copySize.width / blockInfo.width * blockInfo.byteSize;
         uint32_t byteOffsetInRowPitch = texelOffset.x / blockInfo.width * blockInfo.byteSize;
-        uint32_t rowsPerImageInTexels = rowsPerImage * blockInfo.height;
         if (copyBytesPerRowPitch + byteOffsetInRowPitch <= bytesPerRow) {
             // The region's rows fit inside the bytes per row. In this case, extend the width of the
             // PlacedFootprint and copy the buffer with an offset location
@@ -125,7 +124,7 @@ namespace dawn_native { namespace d3d12 {
             copy.copies[0].bufferOffset = texelOffset;
 
             copy.copies[0].bufferSize.width = copySize.width + texelOffset.x;
-            copy.copies[0].bufferSize.height = rowsPerImageInTexels + texelOffset.y;
+            copy.copies[0].bufferSize.height = copySize.height + texelOffset.y;
             copy.copies[0].bufferSize.depthOrArrayLayers = copySize.depthOrArrayLayers;
 
             return copy;
@@ -178,10 +177,20 @@ namespace dawn_native { namespace d3d12 {
 
         copy.copies[0].bufferOffset = texelOffset;
         copy.copies[0].bufferSize.width = texelsPerRow;
-        copy.copies[0].bufferSize.height = rowsPerImageInTexels + texelOffset.y;
+        copy.copies[0].bufferSize.height = copySize.height + texelOffset.y;
         copy.copies[0].bufferSize.depthOrArrayLayers = copySize.depthOrArrayLayers;
 
-        copy.copies[1].alignedOffset = alignedOffset;
+        uint64_t offsetForCopy1 =
+            offset + copy.copies[0].copySize.width / blockInfo.width * blockInfo.byteSize;
+        uint64_t alignedOffsetForCopy1 =
+            offsetForCopy1 & ~static_cast<uint64_t>(D3D12_TEXTURE_DATA_PLACEMENT_ALIGNMENT - 1);
+        Origin3D texelOffsetForCopy1 = ComputeTexelOffsets(
+            blockInfo, static_cast<uint32_t>(offsetForCopy1 - alignedOffsetForCopy1), bytesPerRow);
+
+        ASSERT(texelOffsetForCopy1.y <= blockInfo.height);
+        ASSERT(texelOffsetForCopy1.z == 0);
+
+        copy.copies[1].alignedOffset = alignedOffsetForCopy1;
         copy.copies[1].textureOffset.x = origin.x + copy.copies[0].copySize.width;
         copy.copies[1].textureOffset.y = origin.y;
         copy.copies[1].textureOffset.z = origin.z;
@@ -191,11 +200,9 @@ namespace dawn_native { namespace d3d12 {
         copy.copies[1].copySize.height = copySize.height;
         copy.copies[1].copySize.depthOrArrayLayers = copySize.depthOrArrayLayers;
 
-        copy.copies[1].bufferOffset.x = 0;
+        copy.copies[1].bufferOffset = texelOffsetForCopy1;
-        copy.copies[1].bufferOffset.y = texelOffset.y + blockInfo.height;
+        copy.copies[1].bufferSize.width = copy.copies[1].copySize.width + texelOffsetForCopy1.x;
-        copy.copies[1].bufferOffset.z = 0;
+        copy.copies[1].bufferSize.height = copySize.height + texelOffsetForCopy1.y;
-        copy.copies[1].bufferSize.width = copy.copies[1].copySize.width;
-        copy.copies[1].bufferSize.height = rowsPerImageInTexels + texelOffset.y + blockInfo.height;
         copy.copies[1].bufferSize.depthOrArrayLayers = copySize.depthOrArrayLayers;
 
         return copy;
@@ -228,7 +235,7 @@ namespace dawn_native { namespace d3d12 {
         copyFirstLayerOrigin.z = 0;
 
         copies.copySubresources[0] = Compute2DTextureCopySubresource(
-            copyFirstLayerOrigin, copyOneLayerSize, blockInfo, offset, bytesPerRow, rowsPerImage);
+            copyFirstLayerOrigin, copyOneLayerSize, blockInfo, offset, bytesPerRow);
 
         // When the copy only refers one texture 2D array layer,
         // copies.copySubresources[1] will never be used so we can safely early return here.
@@ -244,7 +251,7 @@ namespace dawn_native { namespace d3d12 {
             const uint64_t bufferOffsetNextLayer = offset + bytesPerLayer;
             copies.copySubresources[1] =
                 Compute2DTextureCopySubresource(copyFirstLayerOrigin, copyOneLayerSize, blockInfo,
-                                                bufferOffsetNextLayer, bytesPerRow, rowsPerImage);
+                                                bufferOffsetNextLayer, bytesPerRow);
         }
 
         return copies;
@@ -274,9 +281,10 @@ namespace dawn_native { namespace d3d12 {
 
         // Call Compute2DTextureCopySubresource and get copy regions. This function has already
         // forwarded "copySize.depthOrArrayLayers" to all depth slices.
-        TextureCopySubresource copySubresource = Compute2DTextureCopySubresource(
+        TextureCopySubresource copySubresource =
-            origin, copySize, blockInfo, offset, bytesPerRow, rowsPerImage);
+            Compute2DTextureCopySubresource(origin, copySize, blockInfo, offset, bytesPerRow);
 
+        ASSERT(copySubresource.count <= 2);
         // If copySize.depth is 1, we can return copySubresource. Because we don't need to extend
         // the copy region(s) to other depth slice(s).
         if (copySize.depthOrArrayLayers == 1) {
@@ -284,10 +292,18 @@ namespace dawn_native { namespace d3d12 {
         }
 
         bool needRecompute = false;
+        uint32_t rowsPerImageInTexels = rowsPerImage * blockInfo.height;
         for (uint32_t i = 0; i < copySubresource.count; ++i) {
-            if (copySubresource.copies[i].bufferSize.height > rowsPerImage) {
+            // There can be one empty row at most in a copy region.
+            ASSERT(copySubresource.copies[i].bufferSize.height <= rowsPerImage + blockInfo.height);
+            Extent3D& bufferSize = copySubresource.copies[i].bufferSize;
+            if (bufferSize.height > rowsPerImageInTexels) {
                 needRecompute = true;
-                break;
+            } else if (bufferSize.height < rowsPerImageInTexels) {
+                // If we are copying multiple depth slices, we should skip rowsPerImageInTexels rows
+                // at least for each slice even though we only copy partial rows in each slice
+                // sometimes.
+                bufferSize.height = rowsPerImageInTexels;
             }
         }
 
@@ -297,6 +313,7 @@ namespace dawn_native { namespace d3d12 {
 
         // TODO(yunchao.he@intel.com): recompute copy regions for special cases for 3D textures,
         // and return the revised copy regions.
+        ASSERT(bytesPerRow == D3D12_TEXTURE_DATA_PITCH_ALIGNMENT);
         return copySubresource;
     }
 }}  // namespace dawn_native::d3d12

src/dawn_native/d3d12/TextureCopySplitter.h

@@ -59,10 +59,14 @@ namespace dawn_native { namespace d3d12 {
     // D3D12 driver may report validation errors when we call CopyTextureRegion. Some important
     // invariants are listed below. For more details
     // of these invariants, see src/tests/unittests/d3d12/CopySplitTests.cpp.
-    //   - Inside each copy region, its buffer offset plus copy size should be less than its buffer
+    //   - Inside each copy region: 1) its buffer offset plus copy size should be less than its
-    //     size.
+    //     buffer size, 2) its buffer offset on y-axis should be less than copy format's
-    //   - each region has an offset (aka alignedOffset) aligned to
+    //     blockInfo.height, 3) its buffer offset on z-axis should be 0.
+    //   - Each copy region has an offset (aka alignedOffset) aligned to
     //     D3D12_TEXTURE_DATA_PLACEMENT_ALIGNMENT
+    //   - The buffer footprint of each copy region should be entirely within the copied buffer,
+    //     which means that the last "texel" of the buffer footprint doesn't go past the end of
+    //     the buffer even though the last "texel" might not be copied.
     //   - If there are multiple copy regions, each copy region should not overlap with the others.
     //   - Copy region(s) combined should exactly be equivalent to the texture region to be copied.
     //   - Every pixel accessed by every copy region should not be out of the bound of the copied
@@ -71,8 +75,7 @@ namespace dawn_native { namespace d3d12 {
                                                            Extent3D copySize,
                                                            const TexelBlockInfo& blockInfo,
                                                            uint64_t offset,
-                                                           uint32_t bytesPerRow,
+                                                           uint32_t bytesPerRow);
-                                                           uint32_t rowsPerImage);
 
     TextureCopySplits Compute2DTextureCopySplits(Origin3D origin,
                                                  Extent3D copySize,

src/dawn_native/d3d12/TextureD3D12.cpp

@@ -983,10 +983,8 @@ namespace dawn_native { namespace d3d12 {
                     // compute d3d12 texture copy locations for texture and buffer
                     Extent3D copySize = GetMipLevelPhysicalSize(level);
 
-                    uint32_t rowsPerImage = copySize.height / blockInfo.height;
                     TextureCopySubresource copySplit = Compute2DTextureCopySubresource(
-                        {0, 0, 0}, copySize, blockInfo, uploadHandle.startOffset, bytesPerRow,
+                        {0, 0, 0}, copySize, blockInfo, uploadHandle.startOffset, bytesPerRow);
-                        rowsPerImage);
 
                     for (uint32_t layer = range.baseArrayLayer;
                          layer < range.baseArrayLayer + range.layerCount; ++layer) {

src/tests/end2end/CopyTests.cpp

@@ -863,6 +863,47 @@ TEST_P(CopyTests_T2B, StrideSpecialCases) {
     }
 }
 
+// Test copying a single slice with rowsPerImage larger than copy height and rowsPerImage will not
+// take effect. If rowsPerImage takes effect, it looks like the copy may go past the end of the
+// buffer.
+TEST_P(CopyTests_T2B, RowsPerImageShouldNotCauseBufferOOBIfDepthOrArrayLayersIsOne) {
+    // Check various offsets to cover each code path in the 2D split code in TextureCopySplitter.
+    for (uint32_t offset : {0, 4, 64}) {
+        constexpr uint32_t kWidth = 250;
+        constexpr uint32_t kHeight = 3;
+
+        TextureSpec textureSpec;
+        textureSpec.textureSize = {kWidth, kHeight, 1};
+
+        BufferSpec bufferSpec = MinimumBufferSpec(kWidth, kHeight);
+        bufferSpec.rowsPerImage = 2 * kHeight;
+        bufferSpec.offset = offset;
+        bufferSpec.size += offset;
+        DoTest(textureSpec, bufferSpec, {kWidth, kHeight, 1});
+        DoTest(textureSpec, bufferSpec, {kWidth, kHeight, 1}, wgpu::TextureDimension::e3D);
+    }
+}
+
+// Test copying a single row with bytesPerRow larger than copy width and bytesPerRow will not
+// take effect. If bytesPerRow takes effect, it looks like the copy may go past the end of the
+// buffer.
+TEST_P(CopyTests_T2B, BytesPerRowShouldNotCauseBufferOOBIfCopyHeightIsOne) {
+    // Check various offsets to cover each code path in the 2D split code in TextureCopySplitter.
+    for (uint32_t offset : {0, 4, 100}) {
+        constexpr uint32_t kWidth = 250;
+
+        TextureSpec textureSpec;
+        textureSpec.textureSize = {kWidth, 1, 1};
+
+        BufferSpec bufferSpec = MinimumBufferSpec(kWidth, 1);
+        bufferSpec.bytesPerRow = 1280;  // the default bytesPerRow is 1024.
+        bufferSpec.offset = offset;
+        bufferSpec.size += offset;
+        DoTest(textureSpec, bufferSpec, {kWidth, 1, 1});
+        DoTest(textureSpec, bufferSpec, {kWidth, 1, 1}, wgpu::TextureDimension::e3D);
+    }
+}
+
 // Test that copying whole texture 2D array layers in one texture-to-buffer-copy works.
 TEST_P(CopyTests_T2B, Texture2DArrayFull) {
     constexpr uint32_t kWidth = 256;

src/tests/unittests/d3d12/CopySplitTests.cpp

@@ -20,6 +20,7 @@
 #include "dawn_native/Format.h"
 #include "dawn_native/d3d12/TextureCopySplitter.h"
 #include "dawn_native/d3d12/d3d12_platform.h"
+#include "utils/TestUtils.h"
 
 using namespace dawn_native::d3d12;
 
@@ -31,7 +32,7 @@ namespace {
         uint32_t z;
         uint32_t width;
         uint32_t height;
-        uint32_t depth;
+        uint32_t depthOrArrayLayers;
         uint32_t texelBlockSizeInBytes;
         uint32_t blockWidth = 1;
         uint32_t blockHeight = 1;
@@ -44,13 +45,50 @@ namespace {
     };
 
     // Check that each copy region fits inside the buffer footprint
-    void ValidateFootprints(const TextureCopySubresource& copySplit) {
+    void ValidateFootprints(const TextureSpec& textureSpec,
+                            const BufferSpec& bufferSpec,
+                            const TextureCopySubresource& copySplit) {
         for (uint32_t i = 0; i < copySplit.count; ++i) {
             const auto& copy = copySplit.copies[i];
             ASSERT_LE(copy.bufferOffset.x + copy.copySize.width, copy.bufferSize.width);
             ASSERT_LE(copy.bufferOffset.y + copy.copySize.height, copy.bufferSize.height);
             ASSERT_LE(copy.bufferOffset.z + copy.copySize.depthOrArrayLayers,
                       copy.bufferSize.depthOrArrayLayers);
+
+            // If there are multiple layers, 2D texture splitter actually splits each layer
+            // independently. See the details in Compute2DTextureCopySplits(). As a result,
+            // if we simply expand a copy region generated by 2D texture splitter to all
+            // layers, the copy region might be OOB. But that is not the approach that the current
+            // 2D texture splitter is doing, although Compute2DTextureCopySubresource forwards
+            // "copySize.depthOrArrayLayers" to the copy region it generated. So skip the test
+            // below for 2D textures with multiple layers.
+            // TODO(yunchao.he@intel.com): add the test below for 3D texture with multiple slices.
+            if (textureSpec.depthOrArrayLayers <= 1) {
+                uint32_t widthInBlocks = textureSpec.width / textureSpec.blockWidth;
+                uint32_t heightInBlocks = textureSpec.height / textureSpec.blockHeight;
+                uint64_t minimumRequiredBufferSize =
+                    bufferSpec.offset +
+                    utils::RequiredBytesInCopy(bufferSpec.bytesPerRow, bufferSpec.rowsPerImage,
+                                               widthInBlocks, heightInBlocks,
+                                               textureSpec.depthOrArrayLayers,
+                                               textureSpec.texelBlockSizeInBytes);
+
+                ASSERT(copy.bufferSize.width % textureSpec.blockWidth == 0);
+                uint32_t widthInBlocksForFootprint = copy.bufferSize.width / textureSpec.blockWidth;
+                ASSERT(copy.bufferSize.height % textureSpec.blockHeight == 0);
+                uint32_t heightInBlocksForFootprint =
+                    copy.bufferSize.height / textureSpec.blockHeight;
+                uint64_t bufferSizeForFootprint =
+                    copy.alignedOffset + utils::RequiredBytesInCopy(
+                                             bufferSpec.bytesPerRow, bufferSpec.rowsPerImage,
+                                             widthInBlocksForFootprint, heightInBlocksForFootprint,
+                                             copy.bufferSize.depthOrArrayLayers,
+                                             textureSpec.texelBlockSizeInBytes);
+
+                // The buffer footprint for copy should not exceed the minimum required buffer
+                // size. Otherwise, pixels accessed by copy may be OOB.
+                ASSERT_LE(bufferSizeForFootprint, minimumRequiredBufferSize);
+            }
         }
     }
 
@@ -115,7 +153,7 @@ namespace {
         ASSERT_EQ(minZ, textureSpec.z);
         ASSERT_EQ(maxX, textureSpec.x + textureSpec.width);
         ASSERT_EQ(maxY, textureSpec.y + textureSpec.height);
-        ASSERT_EQ(maxZ, textureSpec.z + textureSpec.depth);
+        ASSERT_EQ(maxZ, textureSpec.z + textureSpec.depthOrArrayLayers);
     }
 
     // Validate that the number of pixels copied is exactly equal to the number of pixels in the
@@ -127,7 +165,7 @@ namespace {
             const auto& copy = copySplit.copies[i];
             count += copy.copySize.width * copy.copySize.height * copy.copySize.depthOrArrayLayers;
         }
-        ASSERT_EQ(count, textureSpec.width * textureSpec.height * textureSpec.depth);
+        ASSERT_EQ(count, textureSpec.width * textureSpec.height * textureSpec.depthOrArrayLayers);
     }
 
     // Check that every buffer offset is at the correct pixel location
@@ -149,6 +187,7 @@ namespace {
                 copy.bufferOffset.x / textureSpec.blockWidth * texelsPerBlock +
                 copy.bufferOffset.y / textureSpec.blockHeight * bytesPerRowInTexels;
 
+            ASSERT_LE(copy.bufferOffset.y, textureSpec.blockHeight);
             ASSERT_EQ(copy.bufferOffset.z, 0u);
 
             ASSERT(absoluteTexelOffset >=
@@ -170,7 +209,7 @@ namespace {
     void ValidateCopySplit(const TextureSpec& textureSpec,
                            const BufferSpec& bufferSpec,
                            const TextureCopySubresource& copySplit) {
-        ValidateFootprints(copySplit);
+        ValidateFootprints(textureSpec, bufferSpec, copySplit);
         ValidateOffset(copySplit);
         ValidateDisjoint(copySplit);
         ValidateTextureBounds(textureSpec, copySplit);
@@ -181,8 +220,8 @@ namespace {
     std::ostream& operator<<(std::ostream& os, const TextureSpec& textureSpec) {
         os << "TextureSpec("
            << "[(" << textureSpec.x << ", " << textureSpec.y << ", " << textureSpec.z << "), ("
-           << textureSpec.width << ", " << textureSpec.height << ", " << textureSpec.depth << ")], "
+           << textureSpec.width << ", " << textureSpec.height << ", "
-           << textureSpec.texelBlockSizeInBytes << ")";
+           << textureSpec.depthOrArrayLayers << ")], " << textureSpec.texelBlockSizeInBytes << ")";
         return os;
     }
 
@@ -212,10 +251,19 @@ namespace {
     constexpr TextureSpec kBaseTextureSpecs[] = {
         {0, 0, 0, 1, 1, 1, 4},
         {0, 0, 0, 64, 1, 1, 4},
+        {0, 0, 0, 128, 1, 1, 4},
+        {0, 0, 0, 192, 1, 1, 4},
         {31, 16, 0, 1, 1, 1, 4},
         {64, 16, 0, 1, 1, 1, 4},
         {64, 16, 8, 1, 1, 1, 4},
 
+        {0, 0, 0, 64, 2, 1, 4},
+        {0, 0, 0, 64, 2, 2, 4},
+        {0, 0, 0, 128, 2, 1, 4},
+        {0, 0, 0, 128, 2, 2, 4},
+        {0, 0, 0, 192, 2, 1, 4},
+        {0, 0, 0, 192, 2, 2, 4},
+
         {0, 0, 0, 1024, 1024, 1, 4},
         {256, 512, 0, 1024, 1024, 1, 4},
         {64, 48, 0, 1024, 1024, 1, 4},
@@ -246,7 +294,7 @@ namespace {
 
     // Define base buffer sizes to work with: some offsets aligned, some unaligned. bytesPerRow is
     // the minimum required
-    std::array<BufferSpec, 14> BaseBufferSpecs(const TextureSpec& textureSpec) {
+    std::array<BufferSpec, 15> BaseBufferSpecs(const TextureSpec& textureSpec) {
         uint32_t bytesPerRow = Align(textureSpec.texelBlockSizeInBytes * textureSpec.width,
                                      kTextureBytesPerRowAlignment);
 
@@ -257,6 +305,8 @@ namespace {
         return {
             BufferSpec{alignNonPow2(0, textureSpec.texelBlockSizeInBytes), bytesPerRow,
                        textureSpec.height},
+            BufferSpec{alignNonPow2(256, textureSpec.texelBlockSizeInBytes), bytesPerRow,
+                       textureSpec.height},
             BufferSpec{alignNonPow2(512, textureSpec.texelBlockSizeInBytes), bytesPerRow,
                        textureSpec.height},
             BufferSpec{alignNonPow2(1024, textureSpec.texelBlockSizeInBytes), bytesPerRow,
@@ -307,8 +357,8 @@ class CopySplitTest : public testing::Test {
         blockInfo.byteSize = textureSpec.texelBlockSizeInBytes;
         TextureCopySubresource copySplit = Compute2DTextureCopySubresource(
             {textureSpec.x, textureSpec.y, textureSpec.z},
-            {textureSpec.width, textureSpec.height, textureSpec.depth}, blockInfo,
+            {textureSpec.width, textureSpec.height, textureSpec.depthOrArrayLayers}, blockInfo,
-            bufferSpec.offset, bufferSpec.bytesPerRow, bufferSpec.rowsPerImage);
+            bufferSpec.offset, bufferSpec.bytesPerRow);
         ValidateCopySplit(textureSpec, bufferSpec, copySplit);
         return copySplit;
     }