From b2b495b5aad2abccd2afdf9b46d32add610e413a Mon Sep 17 00:00:00 2001 From: Kai Ninomiya Date: Wed, 24 May 2017 13:07:30 -0700 Subject: [PATCH] Forbid explicit transitions to/from attachment usages (#18) --- src/backend/common/CommandBuffer.cpp | 87 ++++++++++++++++++---------- 1 file changed, 58 insertions(+), 29 deletions(-) diff --git a/src/backend/common/CommandBuffer.cpp b/src/backend/common/CommandBuffer.cpp index 2111bfc53c..bed17aa1e5 100644 --- a/src/backend/common/CommandBuffer.cpp +++ b/src/backend/common/CommandBuffer.cpp @@ -201,6 +201,22 @@ namespace backend { auto it = mostRecentTextureUsages.find(texture); return it != mostRecentTextureUsages.end() && (it->second & usage); }; + auto isTextureTransitionPossible = [&](TextureBase* texture, nxt::TextureUsageBit usage) -> bool { + const nxt::TextureUsageBit attachmentUsages = + nxt::TextureUsageBit::ColorAttachment | + nxt::TextureUsageBit::DepthStencilAttachment; + ASSERT(usage != nxt::TextureUsageBit::None && nxt::HasZeroOrOneBits(usage)); + if (usage & attachmentUsages) { + return false; + } + auto it = mostRecentTextureUsages.find(texture); + if (it != mostRecentTextureUsages.end()) { + if (it->second & attachmentUsages) { + return false; + } + } + return texture->IsTransitionPossible(usage); + }; auto validateBindGroupUsages = [&](BindGroupBase* group) -> bool { const auto& layoutInfo = group->GetLayout()->GetBindingInfo(); @@ -260,7 +276,8 @@ namespace backend { uint32_t currentSubpass = 0; auto beginSubpass = [&]() -> bool { auto& subpassInfo = currentRenderPass->GetSubpassInfo(currentSubpass); - for (auto attachmentSlot : subpassInfo.colorAttachments) { + for (auto location : IterateBitSet(subpassInfo.colorAttachmentsSet)) { + auto attachmentSlot = subpassInfo.colorAttachments[location]; auto* tv = currentFramebuffer->GetTextureView(attachmentSlot); // TODO(kainino@chromium.org): the TextureView can only be null // because of the null=backbuffer hack (null representing the @@ -278,11 +295,31 @@ namespace backend { HandleError("Can't transition attachment to ColorAttachment usage"); return false; } - mostRecentTextureUsages.erase(texture); + mostRecentTextureUsages[texture] = nxt::TextureUsageBit::ColorAttachment; texturesTransitioned.insert(texture); } return true; }; + auto endSubpass = [&]() { + auto& subpassInfo = currentRenderPass->GetSubpassInfo(currentSubpass); + for (auto location : IterateBitSet(subpassInfo.colorAttachmentsSet)) { + auto attachmentSlot = subpassInfo.colorAttachments[location]; + auto* tv = currentFramebuffer->GetTextureView(attachmentSlot); + // TODO(kainino@chromium.org): the TextureView can only be null + // because of the null=backbuffer hack (null representing the + // backbuffer). Once that hack is removed (once we have WSI) + // this check isn't needed. + if (tv == nullptr) { + continue; + } + + auto* texture = tv->GetTexture(); + if (texture->IsFrozen()) { + continue; + } + mostRecentTextureUsages[texture] = nxt::TextureUsageBit::None; + } + }; Command type; while(iterator.NextCommandId(&type)) { @@ -298,6 +335,8 @@ namespace backend { HandleError("Can't advance beyond the last subpass"); return false; } + + endSubpass(); currentSubpass += 1; if (!beginSubpass()) { return false; @@ -465,6 +504,7 @@ namespace backend { HandleError("Can't end a render pass before the last subpass"); return false; } + endSubpass(); currentRenderPass = nullptr; currentFramebuffer = nullptr; aspects.reset(VALIDATION_ASPECT_RENDER_PASS); @@ -578,8 +618,14 @@ namespace backend { auto buffer = cmd->buffer.Get(); auto usage = cmd->usage; - if (!cmd->buffer->IsTransitionPossible(cmd->usage)) { - HandleError("Buffer frozen or usage not allowed"); + if (!buffer->IsTransitionPossible(usage)) { + if (buffer->IsFrozen()) { + HandleError("Buffer transition not possible (usage is frozen)"); + } else if (!BufferBase::IsUsagePossible(buffer->GetAllowedUsage(), usage)) { + HandleError("Buffer transition not possible (usage not allowed)"); + } else { + HandleError("Buffer transition not possible"); + } return false; } @@ -595,32 +641,15 @@ namespace backend { auto texture = cmd->texture.Get(); auto usage = cmd->usage; - if (!cmd->texture->IsTransitionPossible(cmd->usage)) { - HandleError("Texture frozen or usage not allowed"); - return false; - } - - // TODO(kainino@chromium.org): We should be able to - // optimize this by tracking which textures are in use - // as attachments. Maybe it's possible that the - // XAttachment usages could mark this: disallow - // explicit transitions to XAttachment usages, and - // disallow explicit transitions of resources already - // in an XAttachment usage. - if (currentRenderPass) { - auto& info = currentRenderPass->GetSubpassInfo(currentSubpass); - for (uint32_t location = 0; location < info.colorAttachments.size(); ++location) { - if (!info.colorAttachmentsSet[location]) { - continue; - } - uint32_t attachmentSlot = info.colorAttachments[location]; - auto* tv = currentFramebuffer->GetTextureView(attachmentSlot); - // TODO(kainino@chromium.org): remove check for tv being non-null once the null=backbuffer hack is removed. - if (tv && tv->GetTexture() == texture) { - HandleError("Can't transition a texture while it's used as a color attachment"); - return false; - } + if (!isTextureTransitionPossible(texture, usage)) { + if (texture->IsFrozen()) { + HandleError("Texture transition not possible (usage is frozen)"); + } else if (!TextureBase::IsUsagePossible(texture->GetAllowedUsage(), usage)) { + HandleError("Texture transition not possible (usage not allowed)"); + } else { + HandleError("Texture transition not possible"); } + return false; } mostRecentTextureUsages[texture] = usage;