CommandBuffer: have a state tracker per-pass

Also perform small code simplifications of the CommandBufferStateTracker
now that it only tracks aspects.

src/backend/CommandBuffer.cpp

@@ -278,8 +278,7 @@ namespace backend {
 
     // CommandBufferBuilder
 
-    CommandBufferBuilder::CommandBufferBuilder(DeviceBase* device)
+    CommandBufferBuilder::CommandBufferBuilder(DeviceBase* device) : Builder(device) {
-        : Builder(device), mState(std::make_unique<CommandBufferStateTracker>()) {
     }
 
     CommandBufferBuilder::~CommandBufferBuilder() {
@@ -392,6 +391,7 @@ namespace backend {
 
     MaybeError CommandBufferBuilder::ValidateComputePass() {
         PassResourceUsageTracker usageTracker;
+        CommandBufferStateTracker persistentState;
 
         Command type;
         while (mIterator.NextCommandId(&type)) {
@@ -401,20 +401,18 @@ namespace backend {
 
                     DAWN_TRY(usageTracker.ValidateUsages(PassType::Compute));
                     mPassResourceUsages.push_back(usageTracker.AcquireResourceUsage());
-
-                    mState->EndPass();
                     return {};
                 } break;
 
                 case Command::Dispatch: {
                     mIterator.NextCommand<DispatchCmd>();
-                    DAWN_TRY(mState->ValidateCanDispatch());
+                    DAWN_TRY(persistentState.ValidateCanDispatch());
                 } break;
 
                 case Command::SetComputePipeline: {
                     SetComputePipelineCmd* cmd = mIterator.NextCommand<SetComputePipelineCmd>();
                     ComputePipelineBase* pipeline = cmd->pipeline.Get();
-                    mState->SetComputePipeline(pipeline);
+                    persistentState.SetComputePipeline(pipeline);
                 } break;
 
                 case Command::SetPushConstants: {
@@ -433,7 +431,7 @@ namespace backend {
                     SetBindGroupCmd* cmd = mIterator.NextCommand<SetBindGroupCmd>();
 
                     TrackBindGroupResourceUsage(cmd->group.Get(), &usageTracker);
-                    mState->SetBindGroup(cmd->index, cmd->group.Get());
+                    persistentState.SetBindGroup(cmd->index, cmd->group.Get());
                 } break;
 
                 default:
@@ -446,6 +444,7 @@ namespace backend {
 
     MaybeError CommandBufferBuilder::ValidateRenderPass(RenderPassDescriptorBase* renderPass) {
         PassResourceUsageTracker usageTracker;
+        CommandBufferStateTracker persistentState;
 
         // Track usage of the render pass attachments
         for (uint32_t i : IterateBitSet(renderPass->GetColorAttachmentMask())) {
@@ -466,19 +465,17 @@ namespace backend {
 
                     DAWN_TRY(usageTracker.ValidateUsages(PassType::Render));
                     mPassResourceUsages.push_back(usageTracker.AcquireResourceUsage());
-
-                    mState->EndPass();
                     return {};
                 } break;
 
                 case Command::DrawArrays: {
                     mIterator.NextCommand<DrawArraysCmd>();
-                    DAWN_TRY(mState->ValidateCanDrawArrays());
+                    DAWN_TRY(persistentState.ValidateCanDrawArrays());
                 } break;
 
                 case Command::DrawElements: {
                     mIterator.NextCommand<DrawElementsCmd>();
-                    DAWN_TRY(mState->ValidateCanDrawElements());
+                    DAWN_TRY(persistentState.ValidateCanDrawElements());
                 } break;
 
                 case Command::SetRenderPipeline: {
@@ -489,7 +486,7 @@ namespace backend {
                         DAWN_RETURN_ERROR("Pipeline is incompatible with this render pass");
                     }
 
-                    mState->SetRenderPipeline(pipeline);
+                    persistentState.SetRenderPipeline(pipeline);
                 } break;
 
                 case Command::SetPushConstants: {
@@ -522,14 +519,14 @@ namespace backend {
                     SetBindGroupCmd* cmd = mIterator.NextCommand<SetBindGroupCmd>();
 
                     TrackBindGroupResourceUsage(cmd->group.Get(), &usageTracker);
-                    mState->SetBindGroup(cmd->index, cmd->group.Get());
+                    persistentState.SetBindGroup(cmd->index, cmd->group.Get());
                 } break;
 
                 case Command::SetIndexBuffer: {
                     SetIndexBufferCmd* cmd = mIterator.NextCommand<SetIndexBufferCmd>();
 
                     usageTracker.BufferUsedAs(cmd->buffer.Get(), dawn::BufferUsageBit::Index);
-                    DAWN_TRY(mState->SetIndexBuffer());
+                    persistentState.SetIndexBuffer();
                 } break;
 
                 case Command::SetVertexBuffers: {
@@ -539,8 +536,8 @@ namespace backend {
 
                     for (uint32_t i = 0; i < cmd->count; ++i) {
                         usageTracker.BufferUsedAs(buffers[i].Get(), dawn::BufferUsageBit::Vertex);
-                        DAWN_TRY(mState->SetVertexBuffer(cmd->startSlot + i));
                     }
+                    persistentState.SetVertexBuffer(cmd->startSlot, cmd->count);
                 } break;
 
                 default:

src/backend/CommandBuffer.h

@@ -31,7 +31,6 @@ namespace backend {
 
     class BindGroupBase;
     class BufferBase;
-    class CommandBufferStateTracker;
     class FramebufferBase;
     class DeviceBase;
     class PipelineBase;
@@ -138,7 +137,6 @@ namespace backend {
         MaybeError ValidateComputePass();
         MaybeError ValidateRenderPass(RenderPassDescriptorBase* renderPass);
 
-        std::unique_ptr<CommandBufferStateTracker> mState;
         CommandAllocator mAllocator;
         CommandIterator mIterator;
         bool mWasMovedToIterator = false;

src/backend/CommandBufferStateTracker.cpp

@@ -15,69 +15,124 @@
 #include "backend/CommandBufferStateTracker.h"
 
 #include "backend/BindGroup.h"
-#include "backend/BindGroupLayout.h"
-#include "backend/Buffer.h"
 #include "backend/ComputePipeline.h"
 #include "backend/Forward.h"
 #include "backend/InputState.h"
 #include "backend/PipelineLayout.h"
-#include "backend/RenderPassDescriptor.h"
 #include "backend/RenderPipeline.h"
-#include "backend/Texture.h"
 #include "common/Assert.h"
 #include "common/BitSetIterator.h"
 
 namespace backend {
 
-    MaybeError CommandBufferStateTracker::ValidateCanDispatch() {
+    enum ValidationAspect {
-        constexpr ValidationAspects requiredAspects =
+        VALIDATION_ASPECT_PIPELINE,
-            1 << VALIDATION_ASPECT_PIPELINE | 1 << VALIDATION_ASPECT_BIND_GROUPS;
+        VALIDATION_ASPECT_BIND_GROUPS,
-        if ((requiredAspects & ~mAspects).none()) {
+        VALIDATION_ASPECT_VERTEX_BUFFERS,
-            // Fast return-true path if everything is good
+        VALIDATION_ASPECT_INDEX_BUFFER,
-            return {};
-        }
 
-        if (!mAspects[VALIDATION_ASPECT_PIPELINE]) {
+        VALIDATION_ASPECT_COUNT
-            DAWN_RETURN_ERROR("No active compute pipeline");
+    };
-        }
+    static_assert(VALIDATION_ASPECT_COUNT == CommandBufferStateTracker::kNumAspects, "");
-        // Compute the lazily computed mAspects
+
-        if (!RecomputeHaveAspectBindGroups()) {
+    static constexpr CommandBufferStateTracker::ValidationAspects kDispatchAspects =
-            DAWN_RETURN_ERROR("Bind group state not valid");
+        1 << VALIDATION_ASPECT_PIPELINE | 1 << VALIDATION_ASPECT_BIND_GROUPS;
-        }
+
-        return {};
+    static constexpr CommandBufferStateTracker::ValidationAspects kDrawArraysAspects =
+        1 << VALIDATION_ASPECT_PIPELINE | 1 << VALIDATION_ASPECT_BIND_GROUPS |
+        1 << VALIDATION_ASPECT_VERTEX_BUFFERS;
+
+    static constexpr CommandBufferStateTracker::ValidationAspects kDrawElementsAspects =
+        1 << VALIDATION_ASPECT_PIPELINE | 1 << VALIDATION_ASPECT_BIND_GROUPS |
+        1 << VALIDATION_ASPECT_VERTEX_BUFFERS | 1 << VALIDATION_ASPECT_INDEX_BUFFER;
+
+    static constexpr CommandBufferStateTracker::ValidationAspects kLazyAspects =
+        1 << VALIDATION_ASPECT_BIND_GROUPS | 1 << VALIDATION_ASPECT_VERTEX_BUFFERS;
+
+    MaybeError CommandBufferStateTracker::ValidateCanDispatch() {
+        return ValidateOperation(kDispatchAspects);
     }
 
     MaybeError CommandBufferStateTracker::ValidateCanDrawArrays() {
-        constexpr ValidationAspects requiredAspects = 1 << VALIDATION_ASPECT_PIPELINE |
+        return ValidateOperation(kDrawArraysAspects);
-                                                      1 << VALIDATION_ASPECT_BIND_GROUPS |
-                                                      1 << VALIDATION_ASPECT_VERTEX_BUFFERS;
-        if ((requiredAspects & ~mAspects).none()) {
-            // Fast return-true path if everything is good
-            return {};
-        }
-
-        return RevalidateCanDraw();
     }
 
     MaybeError CommandBufferStateTracker::ValidateCanDrawElements() {
-        constexpr ValidationAspects requiredAspects =
+        return ValidateOperation(kDrawElementsAspects);
-            1 << VALIDATION_ASPECT_PIPELINE | 1 << VALIDATION_ASPECT_BIND_GROUPS |
+    }
-            1 << VALIDATION_ASPECT_VERTEX_BUFFERS | 1 << VALIDATION_ASPECT_INDEX_BUFFER;
+
-        if ((requiredAspects & ~mAspects).none()) {
+    MaybeError CommandBufferStateTracker::ValidateOperation(ValidationAspects requiredAspects) {
-            // Fast return-true path if everything is good
+        // Fast return-true path if everything is good
+        ValidationAspects missingAspects = requiredAspects & ~mAspects;
+        if (missingAspects.none()) {
             return {};
         }
 
-        if (!mAspects[VALIDATION_ASPECT_INDEX_BUFFER]) {
+        // Generate an error immediately if a non-lazy aspect is missing as computing lazy aspects
-            DAWN_RETURN_ERROR("Cannot DrawElements without index buffer set");
+        // requires the pipeline to be set.
+        if ((missingAspects & ~kLazyAspects).any()) {
+            return GenerateAspectError(missingAspects);
         }
-        return RevalidateCanDraw();
+
+        RecomputeLazyAspects(missingAspects);
+
+        missingAspects = requiredAspects & ~mAspects;
+        if (missingAspects.any()) {
+            return GenerateAspectError(missingAspects);
+        }
+
+        return {};
     }
 
-    void CommandBufferStateTracker::EndPass() {
+    void CommandBufferStateTracker::RecomputeLazyAspects(ValidationAspects aspects) {
-        mInputsSet.reset();
+        ASSERT(mAspects[VALIDATION_ASPECT_PIPELINE]);
-        mAspects = 0;
+        ASSERT((aspects & ~kLazyAspects).none());
-        mBindgroups.fill(nullptr);
+
+        if (aspects[VALIDATION_ASPECT_BIND_GROUPS]) {
+            bool matches = true;
+
+            for (uint32_t i : IterateBitSet(mLastPipelineLayout->GetBindGroupLayoutsMask())) {
+                if (mLastPipelineLayout->GetBindGroupLayout(i) != mBindgroups[i]->GetLayout()) {
+                    matches = false;
+                    break;
+                }
+            }
+
+            if (matches) {
+                mAspects.set(VALIDATION_ASPECT_BIND_GROUPS);
+            }
+        }
+
+        if (aspects[VALIDATION_ASPECT_VERTEX_BUFFERS]) {
+            ASSERT(mLastRenderPipeline != nullptr);
+
+            auto requiredInputs = mLastRenderPipeline->GetInputState()->GetInputsSetMask();
+            if ((mInputsSet & requiredInputs) == requiredInputs) {
+                mAspects.set(VALIDATION_ASPECT_VERTEX_BUFFERS);
+            }
+        }
+    }
+
+    MaybeError CommandBufferStateTracker::GenerateAspectError(ValidationAspects aspects) {
+        ASSERT(aspects.any());
+
+        if (aspects[VALIDATION_ASPECT_INDEX_BUFFER]) {
+            DAWN_RETURN_ERROR("Missing index buffer");
+        }
+
+        if (aspects[VALIDATION_ASPECT_VERTEX_BUFFERS]) {
+            DAWN_RETURN_ERROR("Missing vertex buffer");
+        }
+
+        if (aspects[VALIDATION_ASPECT_BIND_GROUPS]) {
+            DAWN_RETURN_ERROR("Missing bind group");
+        }
+
+        if (aspects[VALIDATION_ASPECT_PIPELINE]) {
+            DAWN_RETURN_ERROR("Missing pipeline");
+        }
+
+        UNREACHABLE();
     }
 
     void CommandBufferStateTracker::SetComputePipeline(ComputePipelineBase* pipeline) {
@@ -90,96 +145,26 @@ namespace backend {
     }
 
     void CommandBufferStateTracker::SetBindGroup(uint32_t index, BindGroupBase* bindgroup) {
-        mBindgroupsSet.set(index);
         mBindgroups[index] = bindgroup;
     }
 
-    MaybeError CommandBufferStateTracker::SetIndexBuffer() {
+    void CommandBufferStateTracker::SetIndexBuffer() {
-        if (!HavePipeline()) {
-            DAWN_RETURN_ERROR("Can't set the index buffer without a pipeline");
-        }
-
         mAspects.set(VALIDATION_ASPECT_INDEX_BUFFER);
-        return {};
     }
 
-    MaybeError CommandBufferStateTracker::SetVertexBuffer(uint32_t index) {
+    void CommandBufferStateTracker::SetVertexBuffer(uint32_t start, uint32_t count) {
-        if (!HavePipeline()) {
+        for (uint32_t i = 0; i < count; ++i) {
-            DAWN_RETURN_ERROR("Can't set vertex buffers without a pipeline");
+            mInputsSet.set(start + i);
         }
-
-        mInputsSet.set(index);
-        return {};
-    }
-
-    bool CommandBufferStateTracker::RecomputeHaveAspectBindGroups() {
-        if (mAspects[VALIDATION_ASPECT_BIND_GROUPS]) {
-            return true;
-        }
-        // Assumes we have a pipeline already
-        if (!mBindgroupsSet.all()) {
-            return false;
-        }
-        for (size_t i = 0; i < mBindgroups.size(); ++i) {
-            if (auto* bindgroup = mBindgroups[i]) {
-                // TODO(kainino@chromium.org): bind group compatibility
-                auto* pipelineBGL = mLastPipeline->GetLayout()->GetBindGroupLayout(i);
-                if (pipelineBGL && bindgroup->GetLayout() != pipelineBGL) {
-                    return false;
-                }
-            }
-        }
-        mAspects.set(VALIDATION_ASPECT_BIND_GROUPS);
-        return true;
-    }
-
-    bool CommandBufferStateTracker::RecomputeHaveAspectVertexBuffers() {
-        if (mAspects[VALIDATION_ASPECT_VERTEX_BUFFERS]) {
-            return true;
-        }
-        // Assumes we have a pipeline already
-        auto requiredInputs = mLastRenderPipeline->GetInputState()->GetInputsSetMask();
-        if ((mInputsSet & requiredInputs) == requiredInputs) {
-            mAspects.set(VALIDATION_ASPECT_VERTEX_BUFFERS);
-            return true;
-        }
-        return false;
-    }
-
-    bool CommandBufferStateTracker::HavePipeline() const {
-        return mAspects[VALIDATION_ASPECT_PIPELINE];
-    }
-
-    MaybeError CommandBufferStateTracker::RevalidateCanDraw() {
-        if (!mAspects[VALIDATION_ASPECT_PIPELINE]) {
-            DAWN_RETURN_ERROR("No active render pipeline");
-        }
-        // Compute the lazily computed mAspects
-        if (!RecomputeHaveAspectBindGroups()) {
-            DAWN_RETURN_ERROR("Bind group state not valid");
-        }
-        if (!RecomputeHaveAspectVertexBuffers()) {
-            DAWN_RETURN_ERROR("Some vertex buffers are not set");
-        }
-        return {};
     }
 
     void CommandBufferStateTracker::SetPipelineCommon(PipelineBase* pipeline) {
-        PipelineLayoutBase* layout = pipeline->GetLayout();
+        mLastPipelineLayout = pipeline->GetLayout();
 
         mAspects.set(VALIDATION_ASPECT_PIPELINE);
 
-        mAspects.reset(VALIDATION_ASPECT_BIND_GROUPS);
+        // Reset lazy aspects so they get recomputed on the next operation.
-        mAspects.reset(VALIDATION_ASPECT_VERTEX_BUFFERS);
+        mAspects &= ~kLazyAspects;
-        // Reset bindgroups but mark unused bindgroups as valid
-        mBindgroupsSet = ~layout->GetBindGroupLayoutsMask();
-
-        // Only bindgroups that were not the same layout in the last pipeline need to be set again.
-        if (mLastPipeline) {
-            mBindgroupsSet |= layout->InheritedGroupsMask(mLastPipeline->GetLayout());
-        }
-
-        mLastPipeline = pipeline;
     }
 
 }  // namespace backend

src/backend/CommandBufferStateTracker.h

@@ -28,45 +28,33 @@ namespace backend {
     class CommandBufferStateTracker {
       public:
         // Non-state-modifying validation functions
-        MaybeError ValidateCanCopy() const;
         MaybeError ValidateCanDispatch();
         MaybeError ValidateCanDrawArrays();
         MaybeError ValidateCanDrawElements();
 
         // State-modifying methods
-        void EndPass();
         void SetComputePipeline(ComputePipelineBase* pipeline);
         void SetRenderPipeline(RenderPipelineBase* pipeline);
         void SetBindGroup(uint32_t index, BindGroupBase* bindgroup);
-        MaybeError SetIndexBuffer();
+        void SetIndexBuffer();
-        MaybeError SetVertexBuffer(uint32_t index);
+        void SetVertexBuffer(uint32_t start, uint32_t count);
+
+        static constexpr size_t kNumAspects = 4;
+        using ValidationAspects = std::bitset<kNumAspects>;
 
       private:
-        enum ValidationAspect {
+        MaybeError ValidateOperation(ValidationAspects requiredAspects);
-            VALIDATION_ASPECT_PIPELINE,
+        void RecomputeLazyAspects(ValidationAspects aspects);
-            VALIDATION_ASPECT_BIND_GROUPS,
+        MaybeError GenerateAspectError(ValidationAspects aspects);
-            VALIDATION_ASPECT_VERTEX_BUFFERS,
-            VALIDATION_ASPECT_INDEX_BUFFER,
-
-            VALIDATION_ASPECT_COUNT
-        };
-        using ValidationAspects = std::bitset<VALIDATION_ASPECT_COUNT>;
-
-        // Queries for lazily evaluated aspects
-        bool RecomputeHaveAspectBindGroups();
-        bool RecomputeHaveAspectVertexBuffers();
-
-        bool HavePipeline() const;
-        MaybeError RevalidateCanDraw();
 
         void SetPipelineCommon(PipelineBase* pipeline);
 
         ValidationAspects mAspects;
 
-        std::bitset<kMaxBindGroups> mBindgroupsSet;
         std::array<BindGroupBase*, kMaxBindGroups> mBindgroups = {};
         std::bitset<kMaxVertexInputs> mInputsSet;
-        PipelineBase* mLastPipeline = nullptr;
+
+        PipelineLayoutBase* mLastPipelineLayout = nullptr;
         RenderPipelineBase* mLastRenderPipeline = nullptr;
     };