From d42a809e8c7411f665041e83403018127bce111c Mon Sep 17 00:00:00 2001 From: Takahiro Date: Thu, 26 May 2022 01:26:34 +0000 Subject: [PATCH] Record zero-attribute vertex buffer when creating render pipeline Currently Dawn ignores all zero-attribute vertex buffer in the given pipeline descriptor when creating RenderPipelineBase because zero-attribute vertex buffer is treated as unused slot, however the spec doesn't state that zero-attribute vertex buffer should be ignored. To support zero-attribute vertex buffer, this commit has the following changes. 1. Add VertexBufferNotUsed enum value to wgpu::VertexStepMode to represent unused slots 2. Ignore VertexBufferNotUsed step mode buffers when creating RenderPipelineBase and add tests to check it 3. Record zero-attribute vertex buffers when creating RenderPipelineBase and add tests to check it 4. Fix VertexStateTest::LastAllowedVertexBuffer broken by the above changes Temporarily we set the enum value of wgpu::VertexStepMode::VertexBufferNotUsed to 0 to pass the CTS tests because currently empty vertex buffer slots step mode can be zero-initialized. We will make a CL to Blink to explicitly set wgpu::VertexStepMode::VertexBufferNotUsed for empty slots and change the enum value to 2. Bug: dawn:1000 Change-Id: Ibd4ab87f2c922e8e460f2311547f13d58f1d5611 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/89340 Reviewed-by: Corentin Wallez Kokoro: Kokoro Commit-Queue: Takahiro --- dawn.json | 5 +- src/dawn/native/RenderPipeline.cpp | 3 +- src/dawn/native/ShaderModule.cpp | 3 +- src/dawn/native/d3d12/RenderPipelineD3D12.cpp | 2 + src/dawn/native/metal/RenderPipelineMTL.mm | 2 + src/dawn/native/opengl/RenderPipelineGL.cpp | 2 + src/dawn/native/vulkan/RenderPipelineVk.cpp | 3 +- src/dawn/tests/end2end/VertexStateTests.cpp | 4 + ...VertexAndIndexBufferOOBValidationTests.cpp | 113 ++++++++++++++++++ 9 files changed, 132 insertions(+), 5 deletions(-) diff --git a/dawn.json b/dawn.json index 2b417fbd3b..f7fe018c17 100644 --- a/dawn.json +++ b/dawn.json @@ -1479,8 +1479,9 @@ "vertex step mode": { "category": "enum", "values": [ - {"value": 0, "name": "vertex"}, - {"value": 1, "name": "instance"} + {"value": 0, "name": "vertex buffer not used"}, + {"value": 1, "name": "vertex"}, + {"value": 2, "name": "instance"} ] }, "load op": { diff --git a/src/dawn/native/RenderPipeline.cpp b/src/dawn/native/RenderPipeline.cpp index 75f6420634..3ec6bdcd3e 100644 --- a/src/dawn/native/RenderPipeline.cpp +++ b/src/dawn/native/RenderPipeline.cpp @@ -498,7 +498,8 @@ RenderPipelineBase::RenderPipelineBase(DeviceBase* device, mVertexBufferCount = descriptor->vertex.bufferCount; const VertexBufferLayout* buffers = descriptor->vertex.buffers; for (uint8_t slot = 0; slot < mVertexBufferCount; ++slot) { - if (buffers[slot].attributeCount == 0) { + // Skip unused slots + if (buffers[slot].stepMode == wgpu::VertexStepMode::VertexBufferNotUsed) { continue; } diff --git a/src/dawn/native/ShaderModule.cpp b/src/dawn/native/ShaderModule.cpp index 43fb89e017..21edca997f 100644 --- a/src/dawn/native/ShaderModule.cpp +++ b/src/dawn/native/ShaderModule.cpp @@ -112,8 +112,9 @@ tint::transform::VertexStepMode ToTintVertexStepMode(wgpu::VertexStepMode mode) return tint::transform::VertexStepMode::kVertex; case wgpu::VertexStepMode::Instance: return tint::transform::VertexStepMode::kInstance; + case wgpu::VertexStepMode::VertexBufferNotUsed: + UNREACHABLE(); } - UNREACHABLE(); } ResultOrError TintPipelineStageToShaderStage(tint::ast::PipelineStage stage) { diff --git a/src/dawn/native/d3d12/RenderPipelineD3D12.cpp b/src/dawn/native/d3d12/RenderPipelineD3D12.cpp index 9f513cf76d..8980b30a81 100644 --- a/src/dawn/native/d3d12/RenderPipelineD3D12.cpp +++ b/src/dawn/native/d3d12/RenderPipelineD3D12.cpp @@ -106,6 +106,8 @@ D3D12_INPUT_CLASSIFICATION VertexStepModeFunction(wgpu::VertexStepMode mode) { return D3D12_INPUT_CLASSIFICATION_PER_VERTEX_DATA; case wgpu::VertexStepMode::Instance: return D3D12_INPUT_CLASSIFICATION_PER_INSTANCE_DATA; + case wgpu::VertexStepMode::VertexBufferNotUsed: + UNREACHABLE(); } } diff --git a/src/dawn/native/metal/RenderPipelineMTL.mm b/src/dawn/native/metal/RenderPipelineMTL.mm index 89548aec86..6e5afd54fe 100644 --- a/src/dawn/native/metal/RenderPipelineMTL.mm +++ b/src/dawn/native/metal/RenderPipelineMTL.mm @@ -98,6 +98,8 @@ MTLVertexStepFunction VertexStepModeFunction(wgpu::VertexStepMode mode) { return MTLVertexStepFunctionPerVertex; case wgpu::VertexStepMode::Instance: return MTLVertexStepFunctionPerInstance; + case wgpu::VertexStepMode::VertexBufferNotUsed: + UNREACHABLE(); } } diff --git a/src/dawn/native/opengl/RenderPipelineGL.cpp b/src/dawn/native/opengl/RenderPipelineGL.cpp index 6f93260e3b..54b6ffba66 100644 --- a/src/dawn/native/opengl/RenderPipelineGL.cpp +++ b/src/dawn/native/opengl/RenderPipelineGL.cpp @@ -276,6 +276,8 @@ void RenderPipeline::CreateVAOForVertexState() { case wgpu::VertexStepMode::Instance: gl.VertexAttribDivisor(glAttrib, 1); break; + case wgpu::VertexStepMode::VertexBufferNotUsed: + UNREACHABLE(); } } } diff --git a/src/dawn/native/vulkan/RenderPipelineVk.cpp b/src/dawn/native/vulkan/RenderPipelineVk.cpp index 8a808a1414..47c3c85076 100644 --- a/src/dawn/native/vulkan/RenderPipelineVk.cpp +++ b/src/dawn/native/vulkan/RenderPipelineVk.cpp @@ -39,8 +39,9 @@ VkVertexInputRate VulkanInputRate(wgpu::VertexStepMode stepMode) { return VK_VERTEX_INPUT_RATE_VERTEX; case wgpu::VertexStepMode::Instance: return VK_VERTEX_INPUT_RATE_INSTANCE; + case wgpu::VertexStepMode::VertexBufferNotUsed: + UNREACHABLE(); } - UNREACHABLE(); } VkFormat VulkanVertexFormat(wgpu::VertexFormat format) { diff --git a/src/dawn/tests/end2end/VertexStateTests.cpp b/src/dawn/tests/end2end/VertexStateTests.cpp index 2089cd6250..aea465d227 100644 --- a/src/dawn/tests/end2end/VertexStateTests.cpp +++ b/src/dawn/tests/end2end/VertexStateTests.cpp @@ -550,6 +550,10 @@ TEST_P(VertexStateTest, LastAllowedVertexBuffer) { vertexState.cAttributes[0].offset = 0; vertexState.cAttributes[0].format = VertexFormat::Float32x4; + for (uint32_t i = 0; i < kBufferIndex; i++) { + vertexState.cVertexBuffers[i].stepMode = VertexStepMode::VertexBufferNotUsed; + } + wgpu::RenderPipeline pipeline = MakeTestPipeline(vertexState, 1, {{0, VertexFormat::Float32x4, VertexStepMode::Vertex}}); diff --git a/src/dawn/tests/unittests/validation/DrawVertexAndIndexBufferOOBValidationTests.cpp b/src/dawn/tests/unittests/validation/DrawVertexAndIndexBufferOOBValidationTests.cpp index fa66bbaeec..4b91c27f2f 100644 --- a/src/dawn/tests/unittests/validation/DrawVertexAndIndexBufferOOBValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/DrawVertexAndIndexBufferOOBValidationTests.cpp @@ -440,6 +440,50 @@ TEST_F(DrawVertexAndIndexBufferOOBValidationTests, DrawVertexBufferOutOfBoundWit } } +// Verify zero-attribute vertex buffer OOB for non-instanced Draw are caught in command encoder +TEST_F(DrawVertexAndIndexBufferOOBValidationTests, ZeroAttribute) { + // Create a render pipeline with zero-attribute vertex buffer description + wgpu::RenderPipeline pipeline = + CreateRenderPipelineWithBufferDesc({{kFloat32x4Stride, wgpu::VertexStepMode::Vertex, {}}}); + + wgpu::Buffer vertexBuffer = CreateBuffer(3 * kFloat32x4Stride); + + { + // Valid + VertexBufferList vertexBufferList = {{0, vertexBuffer, 0, wgpu::kWholeSize}}; + TestRenderPassDraw(pipeline, vertexBufferList, 3, 1, 0, 0, true); + } + + { + // OOB + VertexBufferList vertexBufferList = {{0, vertexBuffer, 0, 0}}; + TestRenderPassDraw(pipeline, vertexBufferList, 3, 1, 0, 0, false); + } +} + +// Verify vertex buffers don't need to be set to unused (hole) slots for Draw +TEST_F(DrawVertexAndIndexBufferOOBValidationTests, UnusedSlots) { + // Set vertex buffer only to the second slot + wgpu::Buffer vertexBuffer = CreateBuffer(3 * kFloat32x4Stride); + VertexBufferList vertexBufferList = {{1, vertexBuffer, 0, wgpu::kWholeSize}}; + + { + // The first slot it unused so valid even if vertex buffer is not set to it. + wgpu::RenderPipeline pipeline = CreateRenderPipelineWithBufferDesc( + {{0, wgpu::VertexStepMode::VertexBufferNotUsed, {}}, + {kFloat32x4Stride, wgpu::VertexStepMode::Vertex, {}}}); + TestRenderPassDraw(pipeline, vertexBufferList, 3, 1, 0, 0, true); + } + + { + // The first slot it used so invalid if vertex buffer is not set to it. + wgpu::RenderPipeline pipeline = CreateRenderPipelineWithBufferDesc( + {{0, wgpu::VertexStepMode::Vertex, {}}, + {kFloat32x4Stride, wgpu::VertexStepMode::Vertex, {}}}); + TestRenderPassDraw(pipeline, vertexBufferList, 3, 1, 0, 0, false); + } +} + // Control case for DrawIndexed TEST_F(DrawVertexAndIndexBufferOOBValidationTests, DrawIndexedBasic) { wgpu::RenderPipeline pipeline = CreateBasicRenderPipeline(); @@ -556,6 +600,75 @@ TEST_F(DrawVertexAndIndexBufferOOBValidationTests, DrawIndexedVertexBufferOOB) { } } +// Verify zero-attribute instance step mode vertex buffer OOB for instanced DrawIndex +// are caught in command encoder +TEST_F(DrawVertexAndIndexBufferOOBValidationTests, DrawIndexedZeroAttribute) { + // Create a render pipeline with a vertex step mode and a zero-attribute + // instance step mode vertex buffer description + wgpu::RenderPipeline pipeline = CreateRenderPipelineWithBufferDesc( + {{kFloat32x4Stride, wgpu::VertexStepMode::Vertex, {{0, wgpu::VertexFormat::Float32x4}}}, + {kFloat32x2Stride, wgpu::VertexStepMode::Instance, {}}}); + + // Build index buffer for 12 indexes + wgpu::Buffer indexBuffer = CreateBuffer(12 * sizeof(uint32_t), wgpu::BufferUsage::Index); + IndexBufferDesc indexBufferDesc = {indexBuffer, wgpu::IndexFormat::Uint32}; + + // Build vertex buffer for 3 vertices + wgpu::Buffer vertexBuffer = CreateBuffer(3 * kFloat32x4Stride); + + // Build vertex buffer for 3 instances + wgpu::Buffer instanceBuffer = CreateBuffer(3 * kFloat32x2Stride); + + { + // Valid + VertexBufferList vertexBufferList = {{0, vertexBuffer, 0, wgpu::kWholeSize}, + {1, vertexBuffer, 0, wgpu::kWholeSize}}; + TestRenderPassDrawIndexed(pipeline, indexBufferDesc, vertexBufferList, 12, 3, 0, 0, 0, + true); + } + + { + // OOB + VertexBufferList vertexBufferList = {{0, vertexBuffer, 0, wgpu::kWholeSize}, + {1, vertexBuffer, 0, 0}}; + TestRenderPassDrawIndexed(pipeline, indexBufferDesc, vertexBufferList, 12, 3, 0, 0, 0, + false); + } +} + +// Verify vertex buffers don't need to be set to unused (hole) slots for DrawIndexed +TEST_F(DrawVertexAndIndexBufferOOBValidationTests, DrawIndexedUnusedSlots) { + // Build index buffer + wgpu::Buffer indexBuffer = CreateBuffer(12 * sizeof(uint32_t), wgpu::BufferUsage::Index); + IndexBufferDesc indexBufferDesc = {indexBuffer, wgpu::IndexFormat::Uint32}; + + // Set vertex buffers only to the second and third slots + wgpu::Buffer vertexBuffer = CreateBuffer(3 * kFloat32x4Stride); + wgpu::Buffer instanceBuffer = CreateBuffer(3 * kFloat32x2Stride); + VertexBufferList vertexBufferList = {{1, vertexBuffer, 0, wgpu::kWholeSize}, + {2, instanceBuffer, 0, wgpu::kWholeSize}}; + + { + // The first slot it unused so valid even if vertex buffer is not set to it. + wgpu::RenderPipeline pipeline = CreateRenderPipelineWithBufferDesc( + {{0, wgpu::VertexStepMode::VertexBufferNotUsed, {}}, + {kFloat32x4Stride, wgpu::VertexStepMode::Vertex, {}}, + {kFloat32x2Stride, wgpu::VertexStepMode::Instance, {}}}); + TestRenderPassDrawIndexed(pipeline, indexBufferDesc, vertexBufferList, 12, 3, 0, 0, 0, + true); + } + + { + // The first slot it used so invalid if vertex buffer is not set to it. + wgpu::RenderPipeline pipeline = CreateRenderPipelineWithBufferDesc( + {{0, wgpu::VertexStepMode::Vertex, {}}, + {kFloat32x4Stride, wgpu::VertexStepMode::Vertex, {}}, + {kFloat32x2Stride, wgpu::VertexStepMode::Instance, {}}}); + TestRenderPassDrawIndexed(pipeline, indexBufferDesc, vertexBufferList, 12, 3, 0, 0, 0, + false); + } +} + // Verify zero array stride vertex buffer OOB for Draw and DrawIndexed are caught in command encoder // This test only test cases that strideCount > 0. Cases of strideCount == 0 are tested in // ZeroStrideCountVertexBufferNeverOOB.