Check the draw type when grouping indirect batches
Presently, the first stage in the indirect draw validation
procedure attempts to optimize the number of passes by grouping
together batches that share the same input indirect buffer.
However, each pass only operates on a single draw type input.
If an indirect buffer is used for both Indexed and NonIndexed
draws, then it is possible to have batches included in a pass
with a mismatching draw type.
Change-Id: If61ce91b0b6a27511e4420051f14cdd3e633dddd
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/148440
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Loko Kung <lokokung@google.com>
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
diff --git a/src/dawn/native/IndirectDrawValidationEncoder.cpp b/src/dawn/native/IndirectDrawValidationEncoder.cpp
index 95851b8..9549541 100644
--- a/src/dawn/native/IndirectDrawValidationEncoder.cpp
+++ b/src/dawn/native/IndirectDrawValidationEncoder.cpp
@@ -261,6 +261,7 @@
struct Pass {
uint32_t flags;
BufferBase* inputIndirectBuffer;
+ IndirectDrawMetadata::DrawType drawType;
uint64_t outputParamsSize = 0;
uint64_t batchDataSize = 0;
std::unique_ptr<void, void (*)(void*)> batchData{nullptr, std::free};
@@ -269,8 +270,9 @@
// First stage is grouping all batches into passes. We try to pack as many batches into a
// single pass as possible. Batches can be grouped together as long as they're validating
- // data from the same indirect buffer, but they may still be split into multiple passes if
- // the number of draw calls in a pass would exceed some (very high) upper bound.
+ // data from the same indirect buffer and draw type, but they may still be split into
+ // multiple passes if the number of draw calls in a pass would exceed some (very high)
+ // upper bound.
uint64_t outputParamsSize = 0;
std::vector<Pass> passes;
IndirectDrawMetadata::IndexedIndirectBufferValidationInfoMap& bufferInfoMap =
@@ -315,7 +317,8 @@
}
Pass* currentPass = passes.empty() ? nullptr : &passes.back();
- if (currentPass && currentPass->inputIndirectBuffer == config.inputIndirectBuffer) {
+ if (currentPass && currentPass->inputIndirectBuffer == config.inputIndirectBuffer &&
+ currentPass->drawType == config.drawType) {
uint64_t nextBatchDataOffset =
Align(currentPass->batchDataSize, minStorageBufferOffsetAlignment);
uint64_t newPassBatchDataSize = nextBatchDataOffset + newBatch.dataSize;
@@ -333,6 +336,7 @@
Pass newPass{};
newPass.inputIndirectBuffer = config.inputIndirectBuffer;
+ newPass.drawType = config.drawType;
newPass.batchDataSize = newBatch.dataSize;
newPass.batches.push_back(newBatch);
newPass.flags = 0;
diff --git a/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp b/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp
index 8275245..b3ac146 100644
--- a/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp
+++ b/src/dawn/tests/end2end/DrawIndexedIndirectTests.cpp
@@ -464,6 +464,52 @@
EXPECT_PIXEL_RGBA8_EQ(filled, renderPass.color, 3, 1);
}
+TEST_P(DrawIndexedIndirectTest, ValidateEncodeMultipleMixedDrawsOneIndirectBufferThenSubmitAtOnce) {
+ // TODO(crbug.com/dawn/789): Test is failing after a roll on SwANGLE on Windows only.
+ DAWN_SUPPRESS_TEST_IF(IsANGLE() && IsWindows());
+
+ // TODO(crbug.com/dawn/1292): Some Intel OpenGL drivers don't seem to like
+ // the offsets that Tint/GLSL produces.
+ DAWN_SUPPRESS_TEST_IF(IsIntel() && IsOpenGL() && IsLinux());
+
+ // It's necessary to for this feature to be disabled so that validation layers
+ // can reject non-indexed indirect draws that use a nonzero firstInstance.
+ DAWN_SUPPRESS_TEST_IF(device.HasFeature(wgpu::FeatureName::IndirectFirstInstance));
+
+ utils::RGBA8 filled(0, 255, 0, 255);
+ utils::RGBA8 notFilled(0, 0, 0, 0);
+
+ // Use the same indirect buffer for both Indexed and non-Indexed draws
+ //
+ // Note: Indexed's vertexOffset and non-Indexed's firstInstance share the same offset.
+ //
+ // If the Indexed draw command (vertexOffset = 4) is correctly interpreted as an Indexed
+ // draw command, then the first 3 vertices of the second quad (top right triangle) will be
+ // drawn.
+ //
+ // Otherwise, if the Indexed draw command is incorrectly interpreted as a non-Indexed
+ // draw command (firstInstance = 4), then it won't be drawn since the validation procedure
+ // will reject draws with non-zero firstInstance (firstInstance = 4).
+ wgpu::Buffer indirectBuffer = CreateIndirectBuffer({0, 0, 0, 0, 0, // Non-Indexed
+ 3, 1, 0, 4, 0}); // Indexed
+
+ wgpu::Buffer indexBuffer = CreateIndexBuffer({0, 1, 2});
+
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo);
+ pass.SetPipeline(pipeline);
+ pass.SetVertexBuffer(0, vertexBuffer);
+ pass.SetIndexBuffer(indexBuffer, wgpu::IndexFormat::Uint32, 0);
+ pass.DrawIndirect(indirectBuffer, 0);
+ pass.DrawIndexedIndirect(indirectBuffer, 20);
+ pass.End();
+ wgpu::CommandBuffer commands = encoder.Finish();
+ queue.Submit(1, &commands);
+
+ EXPECT_PIXEL_RGBA8_EQ(notFilled, renderPass.color, 1, 3);
+ EXPECT_PIXEL_RGBA8_EQ(filled, renderPass.color, 3, 1);
+}
+
TEST_P(DrawIndexedIndirectTest, ValidateEncodeMultipleThenSubmitOutOfOrder) {
// TODO(crbug.com/dawn/789): Test is failing under SwANGLE on Windows only.
DAWN_SUPPRESS_TEST_IF(IsANGLE() && IsWindows());
