Vulkan: Defer the depthStencil buffer layout change to endRenderPass

Depth stencil layout may change while we build the render pass,
depending on the read/write access been made. Right now we are always
inserting a layout change barrier at the start of render pass.  Later on
when the read/write property changes, we insert another layout change
barrier. Similarly, we maintain the attachmentOps and
RenderPassDesc::mPackedColorAttachmentRangeAndDSAccess as we changes
read/write access. This makes code quite commplicated. This CL moves
mReadOnlyDepthStencilMode from FramebufferVK to CommandBufferHelper
object and we only maintain that boolean while we updating the
read/write access. Then at the end of render pass or when depthStencil
image is deleted, we update attachmentOps and mRenderPassDesc and layout
transition all at once and only done once. This simplifies the read only
depth stencil mode implementation a lot.

Bug: b/168953278
Change-Id: Ie263b4526c82a9858e5d1f141ea58f499187a3ca
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/2432075
Commit-Queue: Charlie Lao <cclao@google.com>
Reviewed-by: Tim Van Patten <timvp@google.com>
Reviewed-by: Jamie Madill <jmadill@chromium.org>

Author: cclao

src/libANGLE/renderer/vulkan/ContextVk.cpp

@@ -4042,16 +4042,15 @@ angle::Result ContextVk::updateActiveTextures(const gl::Context *context)
 
             if (hasStartedRenderPass())
             {
-                if (!mDrawFramebuffer->isReadOnlyDepthMode())
+                if (!mRenderPassCommands->isReadOnlyDepthMode())
                 {
                     // To enter depth feedback loop, we must flush and start a new renderpass.
                     // Otherwise it will stick with writable layout and cause validation error.
                     ANGLE_TRY(flushCommandsAndEndRenderPass());
                 }
                 else
                 {
-                    ANGLE_TRY(mDrawFramebuffer->updateRenderPassReadOnlyDepthMode(
-                        this, mRenderPassCommands));
+                    mDrawFramebuffer->updateRenderPassReadOnlyDepthMode(this, mRenderPassCommands);
                 }
             }
 
@@ -4573,13 +4572,6 @@ angle::Result ContextVk::onImageRead(VkImageAspectFlags aspectFlags,
     ASSERT(!image->isReleasedToExternal());
     ASSERT(image->getImageSerial().valid());
 
-    // Layout transitions for images used in the render pass are handled especially.  This function
-    // is only called when the image is used outside the render pass.  As such, if the image is used
-    // inside the render pass, its layout is necessarily different from imageLayout, and thus a
-    // layout transition is necessary (and the render pass has to break).
-    ASSERT(image->isReadBarrierNecessary(imageLayout) ||
-           !(mRenderPassCommands->started() && mRenderPassCommands->usesImageInRenderPass(*image)));
-
     // Note that different read methods are not compatible. A shader read uses a different layout
     // than a transfer read. So we cannot support simultaneous read usage as easily as for Buffers.
     // TODO: Don't close the render pass if the image was only used read-only in the render pass.
@@ -4636,22 +4628,18 @@ angle::Result ContextVk::startRenderPass(gl::Rectangle renderArea,
 {
     mGraphicsDirtyBits |= mNewGraphicsCommandBufferDirtyBits;
 
-    // We always start renderpass with proper depth stencil read only mode based on the current
-    // depth stencil state.
-    const gl::DepthStencilState &dsState = mState.getDepthStencilState();
-    vk::ResourceAccess depthAccess       = GetDepthAccess(dsState);
-    vk::ResourceAccess stencilAccess     = GetStencilAccess(dsState);
-    bool readOnlyDepthMode =
-        depthAccess != vk::ResourceAccess::Write && stencilAccess != vk::ResourceAccess::Write;
-
-    ANGLE_TRY(mDrawFramebuffer->startNewRenderPass(this, readOnlyDepthMode, renderArea,
-                                                   &mRenderPassCommandBuffer));
+    ANGLE_TRY(mDrawFramebuffer->startNewRenderPass(this, renderArea, &mRenderPassCommandBuffer));
 
     ANGLE_TRY(resumeOcclusionQueryIfActive());
 
+    const gl::DepthStencilState &dsState = mState.getDepthStencilState();
+    vk::ResourceAccess depthAccess       = GetDepthAccess(dsState);
+    vk::ResourceAccess stencilAccess     = GetStencilAccess(dsState);
     mRenderPassCommands->onDepthAccess(depthAccess);
     mRenderPassCommands->onStencilAccess(stencilAccess);
 
+    mDrawFramebuffer->updateRenderPassReadOnlyDepthMode(this, mRenderPassCommands);
+
     if (commandBufferOut)
     {
         *commandBufferOut = mRenderPassCommandBuffer;
@@ -5088,8 +5076,7 @@ angle::Result ContextVk::updateRenderPassDepthStencilAccess()
                 mDrawFramebuffer->restoreDepthStencilDefinedContents();
             }
 
-            ANGLE_TRY(
-                mDrawFramebuffer->updateRenderPassReadOnlyDepthMode(this, mRenderPassCommands));
+            mDrawFramebuffer->updateRenderPassReadOnlyDepthMode(this, mRenderPassCommands);
         }
     }
 

src/libANGLE/renderer/vulkan/ContextVk.h

@@ -553,6 +553,12 @@ class ContextVk : public ContextImpl, public vk::Context
                                         vk::AliasingMode::Allowed, image);
     }
 
+    void onDepthStencilDraw(vk::ImageHelper *image, vk::ImageHelper *resolveImage)
+    {
+        ASSERT(mRenderPassCommands->started());
+        mRenderPassCommands->depthStencilImagesDraw(&mResourceUseList, image, resolveImage);
+    }
+
     void onImageHelperRelease(const vk::ImageHelper *image)
     {
         if (mRenderPassCommands->started())

src/libANGLE/renderer/vulkan/FramebufferVk.cpp

@@ -309,7 +309,6 @@ FramebufferVk::FramebufferVk(RendererVk *renderer,
       mBackbuffer(backbuffer),
       mFramebuffer(nullptr),
       mActiveColorComponents(0),
-      mReadOnlyDepthStencilMode(false),
       mReadOnlyDepthFeedbackLoopMode(false)
 {
     mReadPixelBuffer.init(renderer, VK_BUFFER_USAGE_TRANSFER_DST_BIT, kReadPixelsBufferAlignment,
@@ -1749,9 +1748,6 @@ angle::Result FramebufferVk::syncState(const gl::Context *context,
         return angle::Result::Continue;
     }
 
-    // Default to writable depth on any Framebuffer change.
-    mReadOnlyDepthStencilMode = false;
-
     // The FBO's new attachment may have changed the renderable area
     const gl::State &glState = context->getState();
     ANGLE_TRY(contextVk->updateScissor(glState));
@@ -1819,12 +1815,9 @@ void FramebufferVk::updateRenderPassDesc()
     RenderTargetVk *depthStencilRenderTarget = getDepthStencilRenderTarget();
     if (depthStencilRenderTarget)
     {
-        vk::ResourceAccess dsAccess =
-            mReadOnlyDepthStencilMode ? vk::ResourceAccess::ReadOnly : vk::ResourceAccess::Write;
-
         mRenderPassDesc.packDepthStencilAttachment(
             depthStencilRenderTarget->getImageForRenderPass().getFormat().intendedFormatID,
-            dsAccess);
+            vk::ResourceAccess::Write);
 
         // Add the resolve attachment, if any.
         if (depthStencilRenderTarget->hasResolveAttachment())
@@ -2154,7 +2147,7 @@ angle::Result FramebufferVk::clearWithLoadOp(ContextVk *contextVk,
             clearValue.depthStencil = clearDepthStencilValue;
             commands.updateRenderPassDepthStencilClear(dsAspectFlags, clearValue);
             // If we were in depth read only mode, we must change to write mode
-            ANGLE_TRY(updateRenderPassReadOnlyDepthMode(contextVk, &commands));
+            updateRenderPassReadOnlyDepthMode(contextVk, &commands);
         }
     }
     else
@@ -2237,7 +2230,7 @@ angle::Result FramebufferVk::clearWithCommand(
         attachments.emplace_back(VkClearAttachment{dsAspectFlags, 0, dsClearValue});
         // Because we may have changed the depth stencil access mode, update read only depth mode
         // now.
-        ANGLE_TRY(updateRenderPassReadOnlyDepthMode(contextVk, renderpassCommands));
+        updateRenderPassReadOnlyDepthMode(contextVk, renderpassCommands);
     }
 
     VkClearRect rect                           = {};
@@ -2260,7 +2253,6 @@ angle::Result FramebufferVk::getSamplePosition(const gl::Context *context,
 }
 
 angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk,
-                                                bool readOnlyDepthMode,
                                                 const gl::Rectangle &renderArea,
                                                 vk::CommandBuffer **commandBufferOut)
 {
@@ -2466,22 +2458,6 @@ angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk,
         renderPassAttachmentOps.setOps(depthStencilAttachmentIndex, depthLoadOp, depthStoreOp);
         renderPassAttachmentOps.setStencilOps(depthStencilAttachmentIndex, stencilLoadOp,
                                               stencilStoreOp);
-
-        // We can only start the renderpass in read only mode if it is requested to be read only and
-        // we are not doing clear.
-        bool depthStencilReadOnly =
-            readOnlyDepthMode && !depthStencilRenderTarget->hasResolveAttachment() &&
-            renderPassAttachmentOps[depthStencilAttachmentIndex].loadOp !=
-                VK_ATTACHMENT_LOAD_OP_CLEAR &&
-            renderPassAttachmentOps[depthStencilAttachmentIndex].stencilLoadOp !=
-                VK_ATTACHMENT_LOAD_OP_CLEAR;
-        setReadOnlyDepthMode(depthStencilReadOnly);
-
-        vk::ImageLayout dsLayout = mReadOnlyDepthStencilMode
-                                       ? vk::ImageLayout::DepthStencilReadOnly
-                                       : vk::ImageLayout::DepthStencilAttachment;
-
-        renderPassAttachmentOps.setLayouts(depthStencilAttachmentIndex, dsLayout, dsLayout);
     }
 
     // If render pass description is changed, the previous render pass desc is no longer compatible.
@@ -2524,7 +2500,8 @@ angle::Result FramebufferVk::startNewRenderPass(ContextVk *contextVk,
         // tracking content valid very loosely here that as long as it is attached, it assumes will
         // have valid content. The only time it has undefined content is between swap and
         // startNewRenderPass
-        depthStencilRenderTarget->onDepthStencilDraw(contextVk, mReadOnlyDepthStencilMode);
+        // The actual layout determination will be deferred until endRenderPass time
+        depthStencilRenderTarget->onDepthStencilDraw(contextVk);
     }
 
     if (unresolveColorMask.any() || unresolveDepth || unresolveStencil)
@@ -2663,50 +2640,19 @@ angle::Result FramebufferVk::flushDeferredClears(ContextVk *contextVk,
     return contextVk->startRenderPass(renderArea, nullptr);
 }
 
-void FramebufferVk::setReadOnlyDepthMode(bool readOnlyDepthEnabled)
-{
-    if (mReadOnlyDepthStencilMode != readOnlyDepthEnabled)
-    {
-        mReadOnlyDepthStencilMode = readOnlyDepthEnabled;
-
-        ASSERT(getDepthStencilRenderTarget());
-        vk::ResourceAccess dsAccess =
-            isReadOnlyDepthMode() ? vk::ResourceAccess::ReadOnly : vk::ResourceAccess::Write;
-        ASSERT(mRenderPassDesc.hasDepthStencilAttachment());
-        mRenderPassDesc.updateDepthStencilAccess(dsAccess);
-    }
-}
-
-angle::Result FramebufferVk::updateRenderPassReadOnlyDepthMode(ContextVk *contextVk,
-                                                               vk::CommandBufferHelper *renderPass)
+void FramebufferVk::updateRenderPassReadOnlyDepthMode(ContextVk *contextVk,
+                                                      vk::CommandBufferHelper *renderPass)
 {
-    ASSERT(getDepthStencilRenderTarget());
-
-    bool readOnlyDepthStencil =
-        !getDepthStencilRenderTarget()->hasResolveAttachment() &&
+    bool readOnlyDepthStencilMode =
+        getDepthStencilRenderTarget() && !getDepthStencilRenderTarget()->hasResolveAttachment() &&
         (mReadOnlyDepthFeedbackLoopMode || !renderPass->hasDepthStencilWriteOrClear());
-    if (readOnlyDepthStencil == mReadOnlyDepthStencilMode)
-    {
-        return angle::Result::Continue;
-    }
 
     // If readOnlyDepthStencil is false, we are switching out of read only mode due to depth write.
     // We must not be in the read only feedback loop mode because the logic in
     // ContextVk::updateRenderPassDepthStencilAccess() should ensure we end the previous renderpass
     // and a new renderpass will start with feedback loop disabled.
-    ASSERT(readOnlyDepthStencil || !mReadOnlyDepthFeedbackLoopMode);
-
-    setReadOnlyDepthMode(readOnlyDepthStencil);
-
-    // When we toggle read/write mode, we must insert a layout transition.
-    getDepthStencilRenderTarget()->onDepthStencilDraw(contextVk, readOnlyDepthStencil);
+    ASSERT(readOnlyDepthStencilMode || !mReadOnlyDepthFeedbackLoopMode);
 
-    vk::Framebuffer *currentFramebuffer = nullptr;
-    ANGLE_TRY(getFramebuffer(contextVk, &currentFramebuffer, nullptr));
-
-    renderPass->updateStartedRenderPassWithDepthMode(*currentFramebuffer, mRenderPassDesc,
-                                                     readOnlyDepthStencil);
-
-    return angle::Result::Continue;
+    renderPass->updateStartedRenderPassWithDepthMode(readOnlyDepthStencilMode);
 }
 }  // namespace rx

src/libANGLE/renderer/vulkan/FramebufferVk.h

@@ -116,7 +116,6 @@ class FramebufferVk : public FramebufferImpl
     RenderTargetVk *getColorReadRenderTarget() const;
 
     angle::Result startNewRenderPass(ContextVk *contextVk,
-                                     bool readOnlyDepthMode,
                                      const gl::Rectangle &renderArea,
                                      vk::CommandBuffer **commandBufferOut);
     void restoreDepthStencilDefinedContents();
@@ -130,17 +129,15 @@ class FramebufferVk : public FramebufferImpl
                                  vk::Framebuffer **framebufferOut,
                                  const vk::ImageView *resolveImageViewIn);
 
-    bool isReadOnlyDepthMode() const { return mReadOnlyDepthStencilMode; }
-
     bool hasDeferredClears() const { return !mDeferredClears.empty(); }
     angle::Result flushDeferredClears(ContextVk *contextVk, const gl::Rectangle &renderArea);
     void setReadOnlyDepthFeedbackLoopMode(bool readOnlyDepthFeedbackModeEnabled)
     {
         mReadOnlyDepthFeedbackLoopMode = readOnlyDepthFeedbackModeEnabled;
     }
     bool isReadOnlyDepthFeedbackLoopMode() const { return mReadOnlyDepthFeedbackLoopMode; }
-    angle::Result updateRenderPassReadOnlyDepthMode(ContextVk *contextVk,
-                                                    vk::CommandBufferHelper *renderPass);
+    void updateRenderPassReadOnlyDepthMode(ContextVk *contextVk,
+                                           vk::CommandBufferHelper *renderPass);
 
   private:
     FramebufferVk(RendererVk *renderer,
@@ -232,8 +229,6 @@ class FramebufferVk : public FramebufferImpl
                                       vk::ImageViewSubresourceSerial resolveImageViewSerial);
     void removeColorResolveAttachment(uint32_t colorIndexGL);
 
-    void setReadOnlyDepthMode(bool readOnlyDepthEnabled);
-
     WindowSurfaceVk *mBackbuffer;
 
     vk::RenderPassDesc mRenderPassDesc;
@@ -257,8 +252,6 @@ class FramebufferVk : public FramebufferImpl
 
     vk::ClearValuesArray mDeferredClears;
 
-    // True if depth stencil buffer is read only.
-    bool mReadOnlyDepthStencilMode;
     // Tracks if we are in depth feedback loop. Depth read only feedback loop is a special kind of
     // depth stencil read only mode. When we are in feedback loop, we must flush renderpass to exit
     // the loop instead of update the layout.

src/libANGLE/renderer/vulkan/RenderTargetVk.cpp

@@ -107,29 +107,12 @@ void RenderTargetVk::onColorDraw(ContextVk *contextVk)
     mContentDefined = true;
 }
 
-void RenderTargetVk::onDepthStencilDraw(ContextVk *contextVk, bool isReadOnly)
+void RenderTargetVk::onDepthStencilDraw(ContextVk *contextVk)
 {
     const angle::Format &format = mImage->getFormat().actualImageFormat();
     ASSERT(format.hasDepthOrStencilBits());
-    VkImageAspectFlags aspectFlags = vk::GetDepthStencilAspectFlags(format);
-
-    if (isReadOnly)
-    {
-        ASSERT(!mResolveImage);
-        contextVk->onImageRenderPassRead(aspectFlags, vk::ImageLayout::DepthStencilReadOnly,
-                                         mImage);
-    }
-    else
-    {
-        contextVk->onImageRenderPassWrite(aspectFlags, vk::ImageLayout::DepthStencilAttachment,
-                                          mImage);
-        if (mResolveImage)
-        {
-            contextVk->onImageRenderPassWrite(
-                aspectFlags, vk::ImageLayout::DepthStencilResolveAttachment, mResolveImage);
-        }
-    }
 
+    contextVk->onDepthStencilDraw(mImage, mResolveImage);
     retainImageViews(contextVk);
 
     mContentDefined = true;

src/libANGLE/renderer/vulkan/RenderTargetVk.h

@@ -67,7 +67,7 @@ class RenderTargetVk final : public FramebufferAttachmentRenderTarget
 
     // Note: RenderTargets should be called in order, with the depth/stencil onRender last.
     void onColorDraw(ContextVk *contextVk);
-    void onDepthStencilDraw(ContextVk *contextVk, bool isReadOnly);
+    void onDepthStencilDraw(ContextVk *contextVk);
 
     vk::ImageHelper &getImageForRenderPass();
     const vk::ImageHelper &getImageForRenderPass() const;

src/libANGLE/renderer/vulkan/UtilsVk.cpp

@@ -1413,7 +1413,7 @@ angle::Result UtilsVk::clearFramebuffer(ContextVk *contextVk,
 
         // We may have changed depth stencil access mode, so update read only depth stencil mode
         // here.
-        ANGLE_TRY(framebuffer->updateRenderPassReadOnlyDepthMode(contextVk, renderpassCommands));
+        framebuffer->updateRenderPassReadOnlyDepthMode(contextVk, renderpassCommands);
     }
 
     ImageClearShaderParams shaderParams;
@@ -1666,7 +1666,7 @@ angle::Result UtilsVk::blitResolveImpl(ContextVk *contextVk,
     pipelineDesc.setScissor(gl_vk::GetRect(params.blitArea));
 
     vk::CommandBuffer *commandBuffer;
-    ANGLE_TRY(framebuffer->startNewRenderPass(contextVk, false, params.blitArea, &commandBuffer));
+    ANGLE_TRY(framebuffer->startNewRenderPass(contextVk, params.blitArea, &commandBuffer));
     contextVk->onImageRenderPassRead(src->getAspectFlags(), vk::ImageLayout::FragmentShaderReadOnly,
                                      src);