From cb2d6d8553852b10b94d101a7106bdb753c8dc7b Mon Sep 17 00:00:00 2001 From: Kai Ninomiya Date: Fri, 7 Jul 2017 16:06:14 -0700 Subject: [PATCH] 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 | 14 ++-- next.json | 3 +- src/backend/CommandBufferStateTracker.cpp | 82 ++++++++++++++--------- src/backend/CommandBufferStateTracker.h | 7 +- src/backend/Texture.cpp | 7 ++ src/backend/Texture.h | 2 + src/backend/d3d12/TextureD3D12.cpp | 28 ++++---- src/backend/metal/TextureMTL.mm | 2 +- src/tests/end2end/InputStateTests.cpp | 4 +- 9 files changed, 90 insertions(+), 59 deletions(-) diff --git a/examples/RenderToTexture.cpp b/examples/RenderToTexture.cpp index 1ceee3f874..aea96e1948 100644 --- a/examples/RenderToTexture.cpp +++ b/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) - .SetInitialUsage(nxt::TextureUsageBit::ColorAttachment) + .SetAllowedUsage(nxt::TextureUsageBit::OutputAttachment | nxt::TextureUsageBit::Sampled) + .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 - // ColorAttachment or InputAttachment. - .TransitionTextureUsage(renderTarget, nxt::TextureUsageBit::Sampled) + // renderTarget usage unlocked, but left in Attachment usage .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() diff --git a/next.json b/next.json index 718ed87197..b038e560c0 100644 --- a/next.json +++ b/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": 32, "name": "depth stencil attachment"} + {"value": 16, "name": "output attachment"} ] }, "texture view": { diff --git a/src/backend/CommandBufferStateTracker.cpp b/src/backend/CommandBufferStateTracker.cpp index 535700f877..d4ee50031d 100644 --- a/src/backend/CommandBufferStateTracker.cpp +++ b/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)) { - continue; - } - if (!texture->IsTransitionPossible(nxt::TextureUsageBit::ColorAttachment)) { - builder->HandleError("Can't transition attachment to ColorAttachment usage"); + if (!EnsureTextureUsage(texture, nxt::TextureUsageBit::OutputAttachment)) { + builder->HandleError("Unable to ensure texture has OutputAttachment usage"); return false; } - mostRecentTextureUsages[texture] = nxt::TextureUsageBit::ColorAttachment; - texturesTransitioned.insert(texture); + texturesAttached.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 { - const nxt::TextureUsageBit attachmentUsages = - nxt::TextureUsageBit::ColorAttachment | - nxt::TextureUsageBit::DepthStencilAttachment; + bool CommandBufferStateTracker::IsInternalTextureTransitionPossible(TextureBase* texture, nxt::TextureUsageBit usage) const { 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()) { - aspects.set(VALIDATION_ASPECT_BIND_GROUPS); - return true; + // Assumes we have a pipeline already + if (!bindgroupsSet.all()) { + 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); } } diff --git a/src/backend/CommandBufferStateTracker.h b/src/backend/CommandBufferStateTracker.h index 2e2a4f2a76..81d04c8c03 100644 --- a/src/backend/CommandBufferStateTracker.h +++ b/src/backend/CommandBufferStateTracker.h @@ -18,6 +18,7 @@ #include "backend/CommandBuffer.h" #include "common/Constants.h" +#include #include #include #include @@ -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 buffersTransitioned; std::set texturesTransitioned; + std::set 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 bindgroupsSet; + std::array bindgroups = {}; std::bitset inputsSet; PipelineBase* lastPipeline = nullptr; diff --git a/src/backend/Texture.cpp b/src/backend/Texture.cpp index 7f0d865748..b78a8ca99a 100644 --- a/src/backend/Texture.cpp +++ b/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"); diff --git a/src/backend/Texture.h b/src/backend/Texture.h index 4f40f30944..a6655b4f90 100644 --- a/src/backend/Texture.h +++ b/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 diff --git a/src/backend/d3d12/TextureD3D12.cpp b/src/backend/d3d12/TextureD3D12.cpp index 6ac66668ca..f17950b3be 100644 --- a/src/backend/d3d12/TextureD3D12.cpp +++ b/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 (usage & nxt::TextureUsageBit::DepthStencilAttachment) { - resourceState |= D3D12_RESOURCE_STATE_DEPTH_WRITE; + if (TextureBase::IsDepthFormat(format)) { + 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 (usage & nxt::TextureUsageBit::DepthStencilAttachment) { - flags |= D3D12_RESOURCE_FLAG_ALLOW_DEPTH_STENCIL; + if (TextureBase::IsDepthFormat(format)) { + 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 stateAfter = D3D12TextureUsage(targetUsage); + D3D12_RESOURCE_STATES stateBefore = D3D12TextureUsage(currentUsage, GetFormat()); + D3D12_RESOURCE_STATES stateAfter = D3D12TextureUsage(targetUsage, GetFormat()); if (stateBefore == stateAfter) { return false; diff --git a/src/backend/metal/TextureMTL.mm b/src/backend/metal/TextureMTL.mm index b00faf409f..842edc716c 100644 --- a/src/backend/metal/TextureMTL.mm +++ b/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; } diff --git a/src/tests/end2end/InputStateTests.cpp b/src/tests/end2end/InputStateTests.cpp index a99637981f..f6d1a1de0f 100644 --- a/src/tests/end2end/InputStateTests.cpp +++ b/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) - .SetInitialUsage(nxt::TextureUsageBit::ColorAttachment) + .SetAllowedUsage(nxt::TextureUsageBit::OutputAttachment | nxt::TextureUsageBit::TransferSrc) + .SetInitialUsage(nxt::TextureUsageBit::OutputAttachment) .GetResult(); renderTargetView = renderTarget.CreateTextureViewBuilder().GetResult();