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 <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Auto-Submit: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/dawn_native/CommandValidation.cpp b/src/dawn_native/CommandValidation.cpp
index 6d33f0a..390d16d 100644
--- a/src/dawn_native/CommandValidation.cpp
+++ b/src/dawn_native/CommandValidation.cpp
@@ -312,13 +312,7 @@
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 afa9df0..39af70f 100644
--- a/src/dawn_native/ComputePassEncoder.cpp
+++ b/src/dawn_native/ComputePassEncoder.cpp
@@ -69,6 +69,7 @@
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 2bc3c8d..fac7822 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 @@
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 @@
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 @@
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 @@
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 06693df..3224819 100644
--- a/src/tests/unittests/validation/ComputeIndirectValidationTests.cpp
+++ b/src/tests/unittests/validation/ComputeIndirectValidationTests.cpp
@@ -46,9 +46,10 @@
void TestIndirectOffset(utils::Expectation expectation,
std::initializer_list<uint32_t> bufferList,
- uint64_t indirectOffset) {
+ uint64_t indirectOffset,
+ wgpu::BufferUsage usage = wgpu::BufferUsage::Indirect) {
wgpu::Buffer indirectBuffer =
- utils::CreateBufferFromData<uint32_t>(device, wgpu::BufferUsage::Indirect, bufferList);
+ utils::CreateBufferFromData<uint32_t>(device, usage, bufferList);
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
@@ -85,3 +86,12 @@
uint64_t offset = std::numeric_limits<uint64_t>::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 c030202..9d1894a 100644
--- a/src/tests/unittests/validation/DrawIndirectValidationTests.cpp
+++ b/src/tests/unittests/validation/DrawIndirectValidationTests.cpp
@@ -69,9 +69,10 @@
void TestIndirectOffset(utils::Expectation expectation,
std::initializer_list<uint32_t> bufferList,
uint64_t indirectOffset,
- bool indexed) {
+ bool indexed,
+ wgpu::BufferUsage usage = wgpu::BufferUsage::Indirect) {
wgpu::Buffer indirectBuffer =
- utils::CreateBufferFromData<uint32_t>(device, wgpu::BufferUsage::Indirect, bufferList);
+ utils::CreateBufferFromData<uint32_t>(device, usage, bufferList);
DummyRenderPass renderPass(device);
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
@@ -146,3 +147,18 @@
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 d849a29..8213baf 100644
--- a/src/tests/unittests/validation/IndexBufferValidationTests.cpp
+++ b/src/tests/unittests/validation/IndexBufferValidationTests.cpp
@@ -184,3 +184,45 @@
encoder.Finish();
}
}
+
+// Check that the index buffer must have the Index usage.
+TEST_F(IndexBufferValidationTest, InvalidUsage) {
+ wgpu::Buffer indexBuffer =
+ utils::CreateBufferFromData<uint32_t>(device, wgpu::BufferUsage::Index, {0, 1, 2});
+ wgpu::Buffer copyBuffer =
+ utils::CreateBufferFromData<uint32_t>(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 50e0f62..3015774 100644
--- a/src/tests/unittests/validation/VertexBufferValidationTests.cpp
+++ b/src/tests/unittests/validation/VertexBufferValidationTests.cpp
@@ -83,6 +83,7 @@
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 @@
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 @@
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 @@
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<uint32_t>(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());
+ }
+}
