Freeze texture transitions while attached; consolidate OutputAttachment usage (#67)

* lock usages for attachments during subpasses
* refactor IsTextureTransitionPossible
* change attachment usages to OutputAttachment
* make SetBindGroup validation lazier

examples/RenderToTexture.cpp

@@ -59,8 +59,8 @@ void initTextures() {
         .SetExtent(640, 480, 1)
         .SetFormat(nxt::TextureFormat::R8G8B8A8Unorm)
         .SetMipLevels(1)
-        .SetAllowedUsage(nxt::TextureUsageBit::ColorAttachment | nxt::TextureUsageBit::Sampled)
+        .SetAllowedUsage(nxt::TextureUsageBit::OutputAttachment | nxt::TextureUsageBit::Sampled)
-        .SetInitialUsage(nxt::TextureUsageBit::ColorAttachment)
+        .SetInitialUsage(nxt::TextureUsageBit::OutputAttachment)
         .GetResult();
     renderTargetView = renderTarget.CreateTextureViewBuilder().GetResult();
 
@@ -185,20 +185,18 @@ void frame() {
     static const uint32_t vertexBufferOffsets[1] = {0};
     nxt::CommandBuffer commands = device.CreateCommandBufferBuilder()
         .BeginRenderPass(renderpass, framebuffer)
-            // renderTarget is not transitioned here because it's implicit in
-            // BeginRenderPass or AdvanceSubpass.
             .BeginRenderSubpass()
+                // renderTarget implicitly locked to to Attachment usage (if not already frozen)
                 .SetPipeline(pipeline)
                 .SetVertexBuffers(0, 1, &vertexBuffer, vertexBufferOffsets)
                 .DrawArrays(3, 1, 0, 0)
             .EndRenderSubpass()
-            // renderTarget must be transitioned here because it's Sampled, not
+            // renderTarget usage unlocked, but left in Attachment usage
-            // ColorAttachment or InputAttachment.
-            .TransitionTextureUsage(renderTarget, nxt::TextureUsageBit::Sampled)
             .BeginRenderSubpass()
                 .SetPipeline(pipelinePost)
-                .SetBindGroup(0, bindGroup)
                 .SetVertexBuffers(0, 1, &vertexBufferQuad, vertexBufferOffsets)
+                .TransitionTextureUsage(renderTarget, nxt::TextureUsageBit::Sampled)
+                .SetBindGroup(0, bindGroup)
                 .DrawArrays(6, 1, 0, 0)
             .EndRenderSubpass()
         .EndRenderPass()

next.json

@@ -921,8 +921,7 @@
             {"value": 2, "name": "transfer dst"},
             {"value": 4, "name": "sampled"},
             {"value": 8, "name": "storage"},
-            {"value": 16, "name": "color attachment"},
+            {"value": 16, "name": "output attachment"}
-            {"value": 32, "name": "depth stencil attachment"}
         ]
     },
     "texture view": {

src/backend/CommandBufferStateTracker.cpp

@@ -74,7 +74,7 @@ namespace backend {
         }
         // Compute the lazily computed aspects
         if (!RecomputeHaveAspectBindGroups()) {
-            builder->HandleError("Some bind groups are not set");
+            builder->HandleError("Bind group state not valid");
             return false;
         }
         return true;
@@ -148,15 +148,11 @@ namespace backend {
             }
 
             auto* texture = tv->GetTexture();
-            if (texture->HasFrozenUsage(nxt::TextureUsageBit::ColorAttachment)) {
+            if (!EnsureTextureUsage(texture, nxt::TextureUsageBit::OutputAttachment)) {
-                continue;
+                builder->HandleError("Unable to ensure texture has OutputAttachment usage");
-            }
-            if (!texture->IsTransitionPossible(nxt::TextureUsageBit::ColorAttachment)) {
-                builder->HandleError("Can't transition attachment to ColorAttachment usage");
                 return false;
             }
-            mostRecentTextureUsages[texture] = nxt::TextureUsageBit::ColorAttachment;
+            texturesAttached.insert(texture);
-            texturesTransitioned.insert(texture);
         }
 
         aspects.set(VALIDATION_ASPECT_RENDER_SUBPASS);
@@ -186,9 +182,9 @@ namespace backend {
             if (texture->IsFrozen()) {
                 continue;
             }
-
-            mostRecentTextureUsages[texture] = nxt::TextureUsageBit::None;
         }
+        // Everything in texturesAttached should be for the current render subpass.
+        texturesAttached.clear();
 
         currentSubpass += 1;
         aspects.reset(VALIDATION_ASPECT_RENDER_SUBPASS);
@@ -274,14 +270,11 @@ namespace backend {
     }
 
     bool CommandBufferStateTracker::SetBindGroup(uint32_t index, BindGroupBase* bindgroup) {
-        if (bindgroup->GetLayout() != lastPipeline->GetLayout()->GetBindGroupLayout(index)) {
-            builder->HandleError("Bind group layout mismatch");
-            return false;
-        }
         if (!ValidateBindGroupUsages(bindgroup)) {
             return false;
         }
         bindgroupsSet.set(index);
+        bindgroups[index] = bindgroup;
 
         return true;
     }
@@ -336,11 +329,13 @@ namespace backend {
     }
 
     bool CommandBufferStateTracker::TransitionTextureUsage(TextureBase* texture, nxt::TextureUsageBit usage) {
-        if (!IsTextureTransitionPossible(texture, usage)) {
+        if (!IsExplicitTextureTransitionPossible(texture, usage)) {
             if (texture->IsFrozen()) {
                 builder->HandleError("Texture transition not possible (usage is frozen)");
             } else if (!TextureBase::IsUsagePossible(texture->GetAllowedUsage(), usage)) {
                 builder->HandleError("Texture transition not possible (usage not allowed)");
+            } else if (texturesAttached.find(texture) != texturesAttached.end()) {
+                builder->HandleError("Texture transition not possible (texture is in use as a framebuffer attachment)");
             } else {
                 builder->HandleError("Texture transition not possible");
             }
@@ -352,6 +347,18 @@ namespace backend {
         return true;
     }
 
+    bool CommandBufferStateTracker::EnsureTextureUsage(TextureBase* texture, nxt::TextureUsageBit usage) {
+        if (texture->HasFrozenUsage(nxt::TextureUsageBit::OutputAttachment)) {
+            return true;
+        }
+        if (!IsInternalTextureTransitionPossible(texture, nxt::TextureUsageBit::OutputAttachment)) {
+            return false;
+        }
+        mostRecentTextureUsages[texture] = nxt::TextureUsageBit::OutputAttachment;
+        texturesTransitioned.insert(texture);
+        return true;
+    }
+
     bool CommandBufferStateTracker::BufferHasGuaranteedUsageBit(BufferBase* buffer, nxt::BufferUsageBit usage) const {
         ASSERT(usage != nxt::BufferUsageBit::None && nxt::HasZeroOrOneBits(usage));
         if (buffer->HasFrozenUsage(usage)) {
@@ -370,38 +377,50 @@ namespace backend {
         return it != mostRecentTextureUsages.end() && (it->second & usage);
     };
 
-    bool CommandBufferStateTracker::IsTextureTransitionPossible(TextureBase* texture, nxt::TextureUsageBit usage) const {
+    bool CommandBufferStateTracker::IsInternalTextureTransitionPossible(TextureBase* texture, nxt::TextureUsageBit usage) const {
-        const nxt::TextureUsageBit attachmentUsages =
-            nxt::TextureUsageBit::ColorAttachment |
-            nxt::TextureUsageBit::DepthStencilAttachment;
         ASSERT(usage != nxt::TextureUsageBit::None && nxt::HasZeroOrOneBits(usage));
-        if (usage & attachmentUsages) {
+        if (texturesAttached.find(texture) != texturesAttached.end()) {
             return false;
         }
-        auto it = mostRecentTextureUsages.find(texture);
-        if (it != mostRecentTextureUsages.end()) {
-            if (it->second & attachmentUsages) {
-                return false;
-            }
-        }
         return texture->IsTransitionPossible(usage);
     };
 
+    bool CommandBufferStateTracker::IsExplicitTextureTransitionPossible(TextureBase* texture, nxt::TextureUsageBit usage) const {
+        const nxt::TextureUsageBit attachmentUsages =
+            nxt::TextureUsageBit::OutputAttachment;
+        if (usage & attachmentUsages) {
+            return false;
+        }
+        return IsInternalTextureTransitionPossible(texture, usage);
+    }
+
     bool CommandBufferStateTracker::RecomputeHaveAspectBindGroups() {
         if (aspects[VALIDATION_ASPECT_BIND_GROUPS]) {
             return true;
         }
-        if (bindgroupsSet.all()) {
+        // Assumes we have a pipeline already
-            aspects.set(VALIDATION_ASPECT_BIND_GROUPS);
+        if (!bindgroupsSet.all()) {
-            return true;
+            printf("%s:%d\n", __FUNCTION__, __LINE__);
+            return false;
         }
-        return false;
+        for (size_t i = 0; i < bindgroups.size(); ++i) {
+            if (auto* bindgroup = bindgroups[i]) {
+                // TODO(kainino@chromium.org): bind group compatibility
+                if (bindgroup->GetLayout() != lastPipeline->GetLayout()->GetBindGroupLayout(i)) {
+                    printf("%s:%d: i=%zu\n", __FUNCTION__, __LINE__, i);
+                    return false;
+                }
+            }
+        }
+        aspects.set(VALIDATION_ASPECT_BIND_GROUPS);
+        return true;
     }
 
     bool CommandBufferStateTracker::RecomputeHaveAspectVertexBuffers() {
         if (aspects[VALIDATION_ASPECT_VERTEX_BUFFERS]) {
             return true;
         }
+        // Assumes we have a pipeline already
         auto requiredInputs = lastPipeline->GetInputState()->GetInputsSetMask();
         if ((inputsSet & ~requiredInputs).none()) {
             aspects.set(VALIDATION_ASPECT_VERTEX_BUFFERS);
@@ -476,7 +495,7 @@ namespace backend {
         }
         // Compute the lazily computed aspects
         if (!RecomputeHaveAspectBindGroups()) {
-            builder->HandleError("Some bind groups are not set");
+            builder->HandleError("Bind group state not valid");
             return false;
         }
         if (!RecomputeHaveAspectVertexBuffers()) {
@@ -494,5 +513,6 @@ namespace backend {
               1 << VALIDATION_ASPECT_VERTEX_BUFFERS |
               1 << VALIDATION_ASPECT_INDEX_BUFFER);
         aspects &= pipelineDependentAspectsInverse;
+        bindgroups.fill(nullptr);
     }
 }

src/backend/CommandBufferStateTracker.h

@@ -18,6 +18,7 @@
 #include "backend/CommandBuffer.h"
 #include "common/Constants.h"
 
+#include <array>
 #include <bitset>
 #include <map>
 #include <set>
@@ -48,12 +49,14 @@ namespace backend {
             bool SetVertexBuffer(uint32_t index, BufferBase* buffer);
             bool TransitionBufferUsage(BufferBase* buffer, nxt::BufferUsageBit usage);
             bool TransitionTextureUsage(TextureBase* texture, nxt::TextureUsageBit usage);
+            bool EnsureTextureUsage(TextureBase* texture, nxt::TextureUsageBit usage);
 
             // These collections are copied to the CommandBuffer at build time.
             // These pointers will remain valid since they are referenced by
             // the bind groups which are referenced by this command buffer.
             std::set<BufferBase*> buffersTransitioned;
             std::set<TextureBase*> texturesTransitioned;
+            std::set<TextureBase*> texturesAttached;
 
         private:
             enum ValidationAspect {
@@ -71,7 +74,8 @@ namespace backend {
             // Usage helper functions
             bool BufferHasGuaranteedUsageBit(BufferBase* buffer, nxt::BufferUsageBit usage) const;
             bool TextureHasGuaranteedUsageBit(TextureBase* texture, nxt::TextureUsageBit usage) const;
-            bool IsTextureTransitionPossible(TextureBase* texture, nxt::TextureUsageBit usage) const;
+            bool IsInternalTextureTransitionPossible(TextureBase* texture, nxt::TextureUsageBit usage) const;
+            bool IsExplicitTextureTransitionPossible(TextureBase* texture, nxt::TextureUsageBit usage) const;
 
             // Queries for lazily evaluated aspects
             bool RecomputeHaveAspectBindGroups();
@@ -88,6 +92,7 @@ namespace backend {
             ValidationAspects aspects;
 
             std::bitset<kMaxBindGroups> bindgroupsSet;
+            std::array<BindGroupBase*, kMaxBindGroups> bindgroups = {};
             std::bitset<kMaxVertexInputs> inputsSet;
             PipelineBase* lastPipeline = nullptr;
 

src/backend/Texture.cpp

@@ -92,6 +92,13 @@ namespace backend {
         currentUsage = usage;
     }
 
+    bool TextureBase::IsDepthFormat(nxt::TextureFormat format) {
+        switch (format) {
+            case nxt::TextureFormat::R8G8B8A8Unorm:
+                return false;
+        }
+    }
+
     void TextureBase::TransitionUsage(nxt::TextureUsageBit usage) {
         if (!IsTransitionPossible(usage)) {
             device->HandleError("Texture frozen or usage not allowed");

src/backend/Texture.h

@@ -43,6 +43,8 @@ namespace backend {
             bool IsTransitionPossible(nxt::TextureUsageBit usage) const;
             void UpdateUsageInternal(nxt::TextureUsageBit usage);
 
+            static bool IsDepthFormat(nxt::TextureFormat format);
+
             DeviceBase* GetDevice();
 
             // NXT API

src/backend/d3d12/TextureD3D12.cpp

@@ -21,7 +21,7 @@ namespace backend {
 namespace d3d12 {
 
     namespace {
-        D3D12_RESOURCE_STATES D3D12TextureUsage(nxt::TextureUsageBit usage) {
+        D3D12_RESOURCE_STATES D3D12TextureUsage(nxt::TextureUsageBit usage, nxt::TextureFormat format) {
             D3D12_RESOURCE_STATES resourceState = D3D12_RESOURCE_STATE_COMMON;
 
             if (usage & nxt::TextureUsageBit::TransferSrc) {
@@ -36,27 +36,27 @@ namespace d3d12 {
             if (usage & nxt::TextureUsageBit::Storage) {
                 resourceState |= D3D12_RESOURCE_STATE_UNORDERED_ACCESS;
             }
-            if (usage & nxt::TextureUsageBit::ColorAttachment) {
+            if (usage & nxt::TextureUsageBit::OutputAttachment) {
                 resourceState |= D3D12_RESOURCE_STATE_RENDER_TARGET;
-            }
+                if (TextureBase::IsDepthFormat(format)) {
-            if (usage & nxt::TextureUsageBit::DepthStencilAttachment) {
+                    resourceState |= D3D12_RESOURCE_STATE_DEPTH_WRITE;
-                resourceState |= D3D12_RESOURCE_STATE_DEPTH_WRITE;
+                }
             }
 
             return resourceState;
         }
 
-        D3D12_RESOURCE_FLAGS D3D12ResourceFlags(nxt::TextureUsageBit usage) {
+        D3D12_RESOURCE_FLAGS D3D12ResourceFlags(nxt::TextureUsageBit usage, nxt::TextureFormat format) {
             D3D12_RESOURCE_FLAGS flags = D3D12_RESOURCE_FLAG_NONE;
 
             if (usage & nxt::TextureUsageBit::Storage) {
                 flags |= D3D12_RESOURCE_FLAG_ALLOW_UNORDERED_ACCESS;
             }
-            if (usage & nxt::TextureUsageBit::ColorAttachment) {
+            if (usage & nxt::TextureUsageBit::OutputAttachment) {
                 flags |= D3D12_RESOURCE_FLAG_ALLOW_RENDER_TARGET;
-            }
+                if (TextureBase::IsDepthFormat(format)) {
-            if (usage & nxt::TextureUsageBit::DepthStencilAttachment) {
+                    flags |= D3D12_RESOURCE_FLAG_ALLOW_DEPTH_STENCIL;
-                flags |= D3D12_RESOURCE_FLAG_ALLOW_DEPTH_STENCIL;
+                }
             }
 
             return flags;
@@ -91,9 +91,9 @@ namespace d3d12 {
         resourceDescriptor.SampleDesc.Count = 1;
         resourceDescriptor.SampleDesc.Quality = 0;
         resourceDescriptor.Layout = D3D12_TEXTURE_LAYOUT_UNKNOWN;
-        resourceDescriptor.Flags = D3D12ResourceFlags(GetUsage());
+        resourceDescriptor.Flags = D3D12ResourceFlags(GetUsage(), GetFormat());
 
-        resource = device->GetResourceAllocator()->Allocate(D3D12_HEAP_TYPE_DEFAULT, resourceDescriptor, D3D12TextureUsage(GetUsage()));
+        resource = device->GetResourceAllocator()->Allocate(D3D12_HEAP_TYPE_DEFAULT, resourceDescriptor, D3D12TextureUsage(GetUsage(), GetFormat()));
     }
 
     Texture::~Texture() {
@@ -109,8 +109,8 @@ namespace d3d12 {
     }
 
     bool Texture::GetResourceTransitionBarrier(nxt::TextureUsageBit currentUsage, nxt::TextureUsageBit targetUsage, D3D12_RESOURCE_BARRIER* barrier) {
-        D3D12_RESOURCE_STATES stateBefore = D3D12TextureUsage(currentUsage);
+        D3D12_RESOURCE_STATES stateBefore = D3D12TextureUsage(currentUsage, GetFormat());
-        D3D12_RESOURCE_STATES stateAfter = D3D12TextureUsage(targetUsage);
+        D3D12_RESOURCE_STATES stateAfter = D3D12TextureUsage(targetUsage, GetFormat());
 
         if (stateBefore == stateAfter) {
             return false;

src/backend/metal/TextureMTL.mm

@@ -38,7 +38,7 @@ namespace metal {
                 result |= MTLTextureUsageShaderRead;
             }
 
-            if (usage & (nxt::TextureUsageBit::ColorAttachment | nxt::TextureUsageBit::DepthStencilAttachment)) {
+            if (usage & (nxt::TextureUsageBit::OutputAttachment)) {
                 result |= MTLTextureUsageRenderTarget;
             }
 

src/tests/end2end/InputStateTests.cpp

@@ -48,8 +48,8 @@ class InputStateTest : public NXTTest {
                 .SetExtent(kRTSize, kRTSize, 1)
                 .SetFormat(nxt::TextureFormat::R8G8B8A8Unorm)
                 .SetMipLevels(1)
-                .SetAllowedUsage(nxt::TextureUsageBit::ColorAttachment | nxt::TextureUsageBit::TransferSrc)
+                .SetAllowedUsage(nxt::TextureUsageBit::OutputAttachment | nxt::TextureUsageBit::TransferSrc)
-                .SetInitialUsage(nxt::TextureUsageBit::ColorAttachment)
+                .SetInitialUsage(nxt::TextureUsageBit::OutputAttachment)
                 .GetResult();
 
             renderTargetView = renderTarget.CreateTextureViewBuilder().GetResult();