From 310d86f4a0a13c084329923977574d5aa52b692b Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Fri, 22 Jan 2021 09:57:38 +0000 Subject: [PATCH] dawn_native: Move pass validation of buffer usages into the encoder. In a future CL the PassResourceUsage structure will become a SyncScopeResourceUsage and will be used to validate at each synchronization scope. For separation of concerns, the validation that resource have the correct usage shouldn't be done at the sync scope level but at each entrypoint that uses the resource. The validation tests had no coverage of usage validation for pass usage so validation tests are added for Indirct/Index/Vertex usages. (Uniform and Storage are validated at bindgroup creation and already had validation tests) Bug: dawn:635 Change-Id: I5058ad30eb041809f0f60d9403f3cc2d5d7e7c96 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/38380 Reviewed-by: Austin Eng Reviewed-by: Corentin Wallez Commit-Queue: Corentin Wallez Auto-Submit: Corentin Wallez --- src/dawn_native/CommandValidation.cpp | 6 --- src/dawn_native/ComputePassEncoder.cpp | 1 + src/dawn_native/RenderEncoderBase.cpp | 5 ++ .../ComputeIndirectValidationTests.cpp | 14 +++++- .../DrawIndirectValidationTests.cpp | 20 +++++++- .../validation/IndexBufferValidationTests.cpp | 42 +++++++++++++++++ .../VertexBufferValidationTests.cpp | 46 ++++++++++++++++++- 7 files changed, 123 insertions(+), 11 deletions(-) diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp index 6d33f0ae2d..390d16d9cb 100644 --- a/src/dawn_native/CommandValidation.cpp +++ b/src/dawn_native/CommandValidation.cpp @@ -312,13 +312,7 @@ namespace dawn_native { MaybeError ValidatePassResourceUsage(const PassResourceUsage& pass) { // Buffers can only be used as single-write or multiple read. for (size_t i = 0; i < pass.buffers.size(); ++i) { - const BufferBase* buffer = pass.buffers[i]; wgpu::BufferUsage usage = pass.bufferUsages[i]; - - if (usage & ~buffer->GetUsage()) { - return DAWN_VALIDATION_ERROR("Buffer missing usage for the pass"); - } - bool readOnly = IsSubset(usage, kReadOnlyBufferUsages); bool singleUse = wgpu::HasZeroOrOneBits(usage); diff --git a/src/dawn_native/ComputePassEncoder.cpp b/src/dawn_native/ComputePassEncoder.cpp index afa9df0a38..39af70f7c5 100644 --- a/src/dawn_native/ComputePassEncoder.cpp +++ b/src/dawn_native/ComputePassEncoder.cpp @@ -69,6 +69,7 @@ namespace dawn_native { void ComputePassEncoder::DispatchIndirect(BufferBase* indirectBuffer, uint64_t indirectOffset) { mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { DAWN_TRY(GetDevice()->ValidateObject(indirectBuffer)); + DAWN_TRY(ValidateCanUseAs(indirectBuffer, wgpu::BufferUsage::Indirect)); // Indexed dispatches need a compute-shader based validation to check that the dispatch // sizes aren't too big. Disallow them as unsafe until the validation is implemented. diff --git a/src/dawn_native/RenderEncoderBase.cpp b/src/dawn_native/RenderEncoderBase.cpp index 2bc3c8d374..fac7822cc4 100644 --- a/src/dawn_native/RenderEncoderBase.cpp +++ b/src/dawn_native/RenderEncoderBase.cpp @@ -17,6 +17,7 @@ #include "common/Constants.h" #include "dawn_native/Buffer.h" #include "dawn_native/CommandEncoder.h" +#include "dawn_native/CommandValidation.h" #include "dawn_native/Commands.h" #include "dawn_native/Device.h" #include "dawn_native/RenderPipeline.h" @@ -87,6 +88,7 @@ namespace dawn_native { void RenderEncoderBase::DrawIndirect(BufferBase* indirectBuffer, uint64_t indirectOffset) { mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { DAWN_TRY(GetDevice()->ValidateObject(indirectBuffer)); + DAWN_TRY(ValidateCanUseAs(indirectBuffer, wgpu::BufferUsage::Indirect)); if (indirectOffset % 4 != 0) { return DAWN_VALIDATION_ERROR("Indirect offset must be a multiple of 4"); @@ -111,6 +113,7 @@ namespace dawn_native { uint64_t indirectOffset) { mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { DAWN_TRY(GetDevice()->ValidateObject(indirectBuffer)); + DAWN_TRY(ValidateCanUseAs(indirectBuffer, wgpu::BufferUsage::Indirect)); // Indexed indirect draws need a compute-shader based validation check that the range of // indices is contained inside the index buffer on Metal. Disallow them as unsafe until @@ -167,6 +170,7 @@ namespace dawn_native { uint64_t size) { mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { DAWN_TRY(GetDevice()->ValidateObject(buffer)); + DAWN_TRY(ValidateCanUseAs(buffer, wgpu::BufferUsage::Index)); DAWN_TRY(ValidateIndexFormat(format)); if (format == wgpu::IndexFormat::Undefined) { @@ -206,6 +210,7 @@ namespace dawn_native { uint64_t size) { mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError { DAWN_TRY(GetDevice()->ValidateObject(buffer)); + DAWN_TRY(ValidateCanUseAs(buffer, wgpu::BufferUsage::Vertex)); if (slot >= kMaxVertexBuffers) { return DAWN_VALIDATION_ERROR("Vertex buffer slot out of bounds"); diff --git a/src/tests/unittests/validation/ComputeIndirectValidationTests.cpp b/src/tests/unittests/validation/ComputeIndirectValidationTests.cpp index 06693df7d0..32248190f4 100644 --- a/src/tests/unittests/validation/ComputeIndirectValidationTests.cpp +++ b/src/tests/unittests/validation/ComputeIndirectValidationTests.cpp @@ -46,9 +46,10 @@ class ComputeIndirectValidationTest : public ValidationTest { void TestIndirectOffset(utils::Expectation expectation, std::initializer_list bufferList, - uint64_t indirectOffset) { + uint64_t indirectOffset, + wgpu::BufferUsage usage = wgpu::BufferUsage::Indirect) { wgpu::Buffer indirectBuffer = - utils::CreateBufferFromData(device, wgpu::BufferUsage::Indirect, bufferList); + utils::CreateBufferFromData(device, usage, bufferList); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); wgpu::ComputePassEncoder pass = encoder.BeginComputePass(); @@ -85,3 +86,12 @@ TEST_F(ComputeIndirectValidationTest, IndirectOffsetBounds) { uint64_t offset = std::numeric_limits::max(); TestIndirectOffset(utils::Expectation::Failure, {1, 2, 3, 4, 5, 6}, offset); } + +// Check that the buffer must have the indirect usage +TEST_F(ComputeIndirectValidationTest, IndirectUsage) { + // Control case: using a buffer with the indirect usage is valid. + TestIndirectOffset(utils::Expectation::Success, {1, 2, 3}, 0, wgpu::BufferUsage::Indirect); + + // Error case: using a buffer with the vertex usage is an error. + TestIndirectOffset(utils::Expectation::Failure, {1, 2, 3}, 0, wgpu::BufferUsage::Vertex); +} diff --git a/src/tests/unittests/validation/DrawIndirectValidationTests.cpp b/src/tests/unittests/validation/DrawIndirectValidationTests.cpp index c030202d7b..9d1894af0b 100644 --- a/src/tests/unittests/validation/DrawIndirectValidationTests.cpp +++ b/src/tests/unittests/validation/DrawIndirectValidationTests.cpp @@ -69,9 +69,10 @@ class DrawIndirectValidationTest : public ValidationTest { void TestIndirectOffset(utils::Expectation expectation, std::initializer_list bufferList, uint64_t indirectOffset, - bool indexed) { + bool indexed, + wgpu::BufferUsage usage = wgpu::BufferUsage::Indirect) { wgpu::Buffer indirectBuffer = - utils::CreateBufferFromData(device, wgpu::BufferUsage::Indirect, bufferList); + utils::CreateBufferFromData(device, usage, bufferList); DummyRenderPass renderPass(device); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); @@ -146,3 +147,18 @@ TEST_F(DrawIndirectValidationTest, DrawIndexedIndirectOffsetBounds) { TestIndirectOffsetDrawIndexed(utils::Expectation::Failure, {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}, offset); } + +// Check that the buffer must have the indirect usage +TEST_F(DrawIndirectValidationTest, IndirectUsage) { + // Control cases: using a buffer with the indirect usage is valid. + TestIndirectOffset(utils::Expectation::Success, {1, 2, 3, 4}, 0, false, + wgpu::BufferUsage::Indirect); + TestIndirectOffset(utils::Expectation::Success, {1, 2, 3, 4, 5}, 0, true, + wgpu::BufferUsage::Indirect); + + // Error cases: using a buffer with the vertex usage is an error. + TestIndirectOffset(utils::Expectation::Failure, {1, 2, 3, 4}, 0, false, + wgpu::BufferUsage::Vertex); + TestIndirectOffset(utils::Expectation::Failure, {1, 2, 3, 4, 5}, 0, true, + wgpu::BufferUsage::Vertex); +} diff --git a/src/tests/unittests/validation/IndexBufferValidationTests.cpp b/src/tests/unittests/validation/IndexBufferValidationTests.cpp index d849a29c7a..8213baf2c2 100644 --- a/src/tests/unittests/validation/IndexBufferValidationTests.cpp +++ b/src/tests/unittests/validation/IndexBufferValidationTests.cpp @@ -184,3 +184,45 @@ TEST_F(IndexBufferValidationTest, IndexBufferFormatMatchesPipelineStripFormat) { encoder.Finish(); } } + +// Check that the index buffer must have the Index usage. +TEST_F(IndexBufferValidationTest, InvalidUsage) { + wgpu::Buffer indexBuffer = + utils::CreateBufferFromData(device, wgpu::BufferUsage::Index, {0, 1, 2}); + wgpu::Buffer copyBuffer = + utils::CreateBufferFromData(device, wgpu::BufferUsage::CopySrc, {0, 1, 2}); + + DummyRenderPass renderPass(device); + // Control case: using the index buffer is valid. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); + pass.SetIndexBuffer(indexBuffer, wgpu::IndexFormat::Uint32); + pass.EndPass(); + encoder.Finish(); + } + // Error case: using the copy buffer is an error. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); + pass.SetIndexBuffer(copyBuffer, wgpu::IndexFormat::Uint32); + pass.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + utils::ComboRenderBundleEncoderDescriptor renderBundleDesc = {}; + renderBundleDesc.colorFormatsCount = 1; + renderBundleDesc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm; + // Control case: using the index buffer is valid. + { + wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc); + encoder.SetIndexBuffer(indexBuffer, wgpu::IndexFormat::Uint32); + encoder.Finish(); + } + // Error case: using the copy buffer is an error. + { + wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc); + encoder.SetIndexBuffer(copyBuffer, wgpu::IndexFormat::Uint32); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } +} diff --git a/src/tests/unittests/validation/VertexBufferValidationTests.cpp b/src/tests/unittests/validation/VertexBufferValidationTests.cpp index 50e0f6226f..3015774994 100644 --- a/src/tests/unittests/validation/VertexBufferValidationTests.cpp +++ b/src/tests/unittests/validation/VertexBufferValidationTests.cpp @@ -83,6 +83,7 @@ class VertexBufferValidationTest : public ValidationTest { wgpu::ShaderModule fsModule; }; +// Check that vertex buffers still count as bound if we switch the pipeline. TEST_F(VertexBufferValidationTest, VertexBuffersInheritedBetweenPipelines) { DummyRenderPass renderPass(device); wgpu::ShaderModule vsModule2 = MakeVertexShader(2); @@ -119,7 +120,8 @@ TEST_F(VertexBufferValidationTest, VertexBuffersInheritedBetweenPipelines) { encoder.Finish(); } -TEST_F(VertexBufferValidationTest, VertexBuffersNotInheritedBetweenRendePasses) { +// Check that vertex buffers that are set are reset between render passes. +TEST_F(VertexBufferValidationTest, VertexBuffersNotInheritedBetweenRenderPasses) { DummyRenderPass renderPass(device); wgpu::ShaderModule vsModule2 = MakeVertexShader(2); wgpu::ShaderModule vsModule1 = MakeVertexShader(1); @@ -168,6 +170,7 @@ TEST_F(VertexBufferValidationTest, VertexBuffersNotInheritedBetweenRendePasses) ASSERT_DEVICE_ERROR(encoder.Finish()); } +// Check validation of the vertex buffer slot for OOB. TEST_F(VertexBufferValidationTest, VertexBufferSlotValidation) { wgpu::Buffer buffer = MakeVertexBuffer(); @@ -281,3 +284,44 @@ TEST_F(VertexBufferValidationTest, VertexBufferOffsetOOBValidation) { ASSERT_DEVICE_ERROR(encoder.Finish()); } } + +// Check that the vertex buffer must have the Vertex usage. +TEST_F(VertexBufferValidationTest, InvalidUsage) { + wgpu::Buffer vertexBuffer = MakeVertexBuffer(); + wgpu::Buffer indexBuffer = + utils::CreateBufferFromData(device, wgpu::BufferUsage::Index, {0, 0, 0}); + + DummyRenderPass renderPass(device); + // Control case: using the vertex buffer is valid. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); + pass.SetVertexBuffer(0, vertexBuffer); + pass.EndPass(); + encoder.Finish(); + } + // Error case: using the index buffer is an error. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass); + pass.SetVertexBuffer(0, indexBuffer); + pass.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + utils::ComboRenderBundleEncoderDescriptor renderBundleDesc = {}; + renderBundleDesc.colorFormatsCount = 1; + renderBundleDesc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm; + // Control case: using the vertex buffer is valid. + { + wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc); + encoder.SetVertexBuffer(0, vertexBuffer); + encoder.Finish(); + } + // Error case: using the index buffer is an error. + { + wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&renderBundleDesc); + encoder.SetVertexBuffer(0, indexBuffer); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } +}