Add a size argument to Set[Vertex|Index]Buffer

Bug: dawn:22
Change-Id: I06a4fc2b651e5a4692893f710ca11785976c1fa4
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/20200
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

dawn.json

@@ -1001,14 +1001,16 @@
                 "args": [
                     {"name": "slot", "type": "uint32_t"},
                     {"name": "buffer", "type": "buffer"},
-                    {"name": "offset", "type": "uint64_t", "default": "0"}
+                    {"name": "offset", "type": "uint64_t", "default": "0"},
+                    {"name": "size", "type": "uint64_t", "default": "0"}
                 ]
             },
             {
                 "name": "set index buffer",
                 "args": [
                     {"name": "buffer", "type": "buffer"},
-                    {"name": "offset", "type": "uint64_t", "default": "0"}
+                    {"name": "offset", "type": "uint64_t", "default": "0"},
+                    {"name": "size", "type": "uint64_t", "default": "0"}
                 ]
             },
             {
@@ -1186,14 +1188,16 @@
                 "args": [
                     {"name": "slot", "type": "uint32_t"},
                     {"name": "buffer", "type": "buffer"},
-                    {"name": "offset", "type": "uint64_t", "default": "0"}
+                    {"name": "offset", "type": "uint64_t", "default": "0"},
+                    {"name": "size", "type": "uint64_t", "default": "0"}
                 ]
             },
             {
                 "name": "set index buffer",
                 "args": [
                     {"name": "buffer", "type": "buffer"},
-                    {"name": "offset", "type": "uint64_t", "default": "0"}
+                    {"name": "offset", "type": "uint64_t", "default": "0"},
+                    {"name": "size", "type": "uint64_t", "default": "0"}
                 ]
             },
             {

src/dawn_native/Commands.h

@@ -218,12 +218,14 @@ namespace dawn_native {
     struct SetIndexBufferCmd {
         Ref<BufferBase> buffer;
         uint64_t offset;
+        uint64_t size;
     };
 
     struct SetVertexBufferCmd {
         uint32_t slot;
         Ref<BufferBase> buffer;
         uint64_t offset;
+        uint64_t size;
     };
 
     // This needs to be called before the CommandIterator is freed so that the Ref<> present in

src/dawn_native/RenderEncoderBase.cpp

@@ -135,14 +135,29 @@ namespace dawn_native {
         });
     }
 
-    void RenderEncoderBase::SetIndexBuffer(BufferBase* buffer, uint64_t offset) {
+    void RenderEncoderBase::SetIndexBuffer(BufferBase* buffer, uint64_t offset, uint64_t size) {
         mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError {
             DAWN_TRY(GetDevice()->ValidateObject(buffer));
 
+            uint64_t bufferSize = buffer->GetSize();
+            if (offset > bufferSize) {
+                return DAWN_VALIDATION_ERROR("Offset larger than the buffer size");
+            }
+            uint64_t remainingSize = bufferSize - offset;
+
+            if (size == 0) {
+                size = remainingSize;
+            } else {
+                if (size > remainingSize) {
+                    return DAWN_VALIDATION_ERROR("Size + offset larger than the buffer size");
+                }
+            }
+
             SetIndexBufferCmd* cmd =
                 allocator->Allocate<SetIndexBufferCmd>(Command::SetIndexBuffer);
             cmd->buffer = buffer;
             cmd->offset = offset;
+            cmd->size = size;
 
             mUsageTracker.BufferUsedAs(buffer, wgpu::BufferUsage::Index);
 
@@ -150,7 +165,10 @@ namespace dawn_native {
         });
     }
 
-    void RenderEncoderBase::SetVertexBuffer(uint32_t slot, BufferBase* buffer, uint64_t offset) {
+    void RenderEncoderBase::SetVertexBuffer(uint32_t slot,
+                                            BufferBase* buffer,
+                                            uint64_t offset,
+                                            uint64_t size) {
         mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError {
             DAWN_TRY(GetDevice()->ValidateObject(buffer));
 
@@ -158,11 +176,26 @@ namespace dawn_native {
                 return DAWN_VALIDATION_ERROR("Vertex buffer slot out of bounds");
             }
 
+            uint64_t bufferSize = buffer->GetSize();
+            if (offset > bufferSize) {
+                return DAWN_VALIDATION_ERROR("Offset larger than the buffer size");
+            }
+            uint64_t remainingSize = bufferSize - offset;
+
+            if (size == 0) {
+                size = remainingSize;
+            } else {
+                if (size > remainingSize) {
+                    return DAWN_VALIDATION_ERROR("Size + offset larger than the buffer size");
+                }
+            }
+
             SetVertexBufferCmd* cmd =
                 allocator->Allocate<SetVertexBufferCmd>(Command::SetVertexBuffer);
             cmd->slot = slot;
             cmd->buffer = buffer;
             cmd->offset = offset;
+            cmd->size = size;
 
             mUsageTracker.BufferUsedAs(buffer, wgpu::BufferUsage::Vertex);
 

src/dawn_native/RenderEncoderBase.h

@@ -39,8 +39,8 @@ namespace dawn_native {
 
         void SetPipeline(RenderPipelineBase* pipeline);
 
-        void SetVertexBuffer(uint32_t slot, BufferBase* buffer, uint64_t offset);
+        void SetVertexBuffer(uint32_t slot, BufferBase* buffer, uint64_t offset, uint64_t size);
-        void SetIndexBuffer(BufferBase* buffer, uint64_t offset);
+        void SetIndexBuffer(BufferBase* buffer, uint64_t offset, uint64_t size);
 
       protected:
         // Construct an "error" render encoder base.

src/dawn_native/d3d12/CommandBufferD3D12.cpp

@@ -298,13 +298,13 @@ namespace dawn_native { namespace d3d12 {
     namespace {
         class VertexBufferTracker {
           public:
-            void OnSetVertexBuffer(uint32_t slot, Buffer* buffer, uint64_t offset) {
+            void OnSetVertexBuffer(uint32_t slot, Buffer* buffer, uint64_t offset, uint64_t size) {
                 mStartSlot = std::min(mStartSlot, slot);
                 mEndSlot = std::max(mEndSlot, slot + 1);
 
                 auto* d3d12BufferView = &mD3D12BufferViews[slot];
                 d3d12BufferView->BufferLocation = buffer->GetVA() + offset;
-                d3d12BufferView->SizeInBytes = buffer->GetSize() - offset;
+                d3d12BufferView->SizeInBytes = size;
                 // The bufferView stride is set based on the vertex state before a draw.
             }
 
@@ -360,9 +360,9 @@ namespace dawn_native { namespace d3d12 {
 
         class IndexBufferTracker {
           public:
-            void OnSetIndexBuffer(Buffer* buffer, uint64_t offset) {
+            void OnSetIndexBuffer(Buffer* buffer, uint64_t offset, uint64_t size) {
                 mD3D12BufferView.BufferLocation = buffer->GetVA() + offset;
-                mD3D12BufferView.SizeInBytes = buffer->GetSize() - offset;
+                mD3D12BufferView.SizeInBytes = size;
 
                 // We don't need to dirty the state unless BufferLocation or SizeInBytes
                 // change, but most of the time this will always be the case.
@@ -1113,7 +1113,8 @@ namespace dawn_native { namespace d3d12 {
                 case Command::SetIndexBuffer: {
                     SetIndexBufferCmd* cmd = iter->NextCommand<SetIndexBufferCmd>();
 
-                    indexBufferTracker.OnSetIndexBuffer(ToBackend(cmd->buffer.Get()), cmd->offset);
+                    indexBufferTracker.OnSetIndexBuffer(ToBackend(cmd->buffer.Get()), cmd->offset,
+                                                        cmd->size);
                     break;
                 }
 
@@ -1121,7 +1122,7 @@ namespace dawn_native { namespace d3d12 {
                     SetVertexBufferCmd* cmd = iter->NextCommand<SetVertexBufferCmd>();
 
                     vertexBufferTracker.OnSetVertexBuffer(cmd->slot, ToBackend(cmd->buffer.Get()),
-                                                          cmd->offset);
+                                                          cmd->offset, cmd->size);
                     break;
                 }
 

src/tests/BUILD.gn

@@ -159,6 +159,7 @@ test("dawn_unittests") {
     "unittests/validation/ErrorScopeValidationTests.cpp",
     "unittests/validation/FenceValidationTests.cpp",
     "unittests/validation/GetBindGroupLayoutValidationTests.cpp",
+    "unittests/validation/IndexBufferValidationTests.cpp",
     "unittests/validation/QueueSubmitValidationTests.cpp",
     "unittests/validation/RenderBundleValidationTests.cpp",
     "unittests/validation/RenderPassDescriptorValidationTests.cpp",

src/tests/unittests/validation/IndexBufferValidationTests.cpp

@@ -0,0 +1,95 @@
+// Copyright 2020 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "tests/unittests/validation/ValidationTest.h"
+
+#include "utils/ComboRenderBundleEncoderDescriptor.h"
+
+class IndexBufferValidationTest : public ValidationTest {};
+
+// Test that for OOB validation of index buffer offset and size.
+TEST_F(IndexBufferValidationTest, IndexBufferOffsetOOBValidation) {
+    wgpu::BufferDescriptor bufferDesc = {
+        .usage = wgpu::BufferUsage::Index,
+        .size = 256,
+    };
+    wgpu::Buffer buffer = device.CreateBuffer(&bufferDesc);
+
+    DummyRenderPass renderPass(device);
+    // Control case, using the full buffer, with or without an explicit size is valid.
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        // Explicit size
+        pass.SetIndexBuffer(buffer, 0, 256);
+        // Implicit size
+        pass.SetIndexBuffer(buffer, 0, 0);
+        pass.SetIndexBuffer(buffer, 256 - 4, 0);
+        pass.SetIndexBuffer(buffer, 4, 0);
+        // Implicit size of zero
+        pass.SetIndexBuffer(buffer, 256, 0);
+        pass.EndPass();
+        encoder.Finish();
+    }
+
+    // Bad case, offset + size is larger than the buffer
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetIndexBuffer(buffer, 4, 256);
+        pass.EndPass();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    // Bad case, size is 0 but the offset is larger than the buffer
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetIndexBuffer(buffer, 256 + 4, 0);
+        pass.EndPass();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    utils::ComboRenderBundleEncoderDescriptor renderBundleDesc = {};
+    renderBundleDesc.colorFormatsCount = 1;
+    renderBundleDesc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm;
+
+    // Control case, using the full buffer, with or without an explicit size is valid.
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        // Explicit size
+        encoder.SetIndexBuffer(buffer, 0, 256);
+        // Implicit size
+        encoder.SetIndexBuffer(buffer, 0, 0);
+        encoder.SetIndexBuffer(buffer, 256 - 4, 0);
+        encoder.SetIndexBuffer(buffer, 4, 0);
+        // Implicit size of zero
+        encoder.SetIndexBuffer(buffer, 256, 0);
+        encoder.Finish();
+    }
+
+    // Bad case, offset + size is larger than the buffer
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetIndexBuffer(buffer, 4, 256);
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    // Bad case, size is 0 but the offset is larger than the buffer
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetIndexBuffer(buffer, 256 + 4, 0);
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+}

src/tests/unittests/validation/VertexBufferValidationTests.cpp

@@ -211,3 +211,75 @@ TEST_F(VertexBufferValidationTest, VertexBufferSlotValidation) {
         ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 }
+
+// Test that for OOB validation of vertex buffer offset and size.
+TEST_F(VertexBufferValidationTest, VertexBufferOffsetOOBValidation) {
+    wgpu::Buffer buffer = MakeVertexBuffer();
+
+    DummyRenderPass renderPass(device);
+    // Control case, using the full buffer, with or without an explicit size is valid.
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        // Explicit size
+        pass.SetVertexBuffer(0, buffer, 0, 256);
+        // Implicit size
+        pass.SetVertexBuffer(0, buffer, 0, 0);
+        pass.SetVertexBuffer(0, buffer, 256 - 4, 0);
+        pass.SetVertexBuffer(0, buffer, 4, 0);
+        // Implicit size of zero
+        pass.SetVertexBuffer(0, buffer, 256, 0);
+        pass.EndPass();
+        encoder.Finish();
+    }
+
+    // Bad case, offset + size is larger than the buffer
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetVertexBuffer(0, buffer, 4, 256);
+        pass.EndPass();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    // Bad case, size is 0 but the offset is larger than the buffer
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+        pass.SetVertexBuffer(0, buffer, 256 + 4, 0);
+        pass.EndPass();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    utils::ComboRenderBundleEncoderDescriptor renderBundleDesc = {};
+    renderBundleDesc.colorFormatsCount = 1;
+    renderBundleDesc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm;
+
+    // Control case, using the full buffer, with or without an explicit size is valid.
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        // Explicit size
+        encoder.SetVertexBuffer(0, buffer, 0, 256);
+        // Implicit size
+        encoder.SetVertexBuffer(0, buffer, 0, 0);
+        encoder.SetVertexBuffer(0, buffer, 256 - 4, 0);
+        encoder.SetVertexBuffer(0, buffer, 4, 0);
+        // Implicit size of zero
+        encoder.SetVertexBuffer(0, buffer, 256, 0);
+        encoder.Finish();
+    }
+
+    // Bad case, offset + size is larger than the buffer
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetVertexBuffer(0, buffer, 4, 256);
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+
+    // Bad case, size is 0 but the offset is larger than the buffer
+    {
+        wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc);
+        encoder.SetVertexBuffer(0, buffer, 256 + 4, 0);
+        ASSERT_DEVICE_ERROR(encoder.Finish());
+    }
+}