Vulkan: Remove dead path in clear

src/libANGLE/renderer/vulkan/FramebufferVk.cpp

@@ -470,11 +470,17 @@ angle::Result FramebufferVk::clearImpl(const gl::Context *context,
     bool clearDepthWithDraw   = clearDepth && scissoredClear;
     bool clearStencilWithDraw = clearStencil && (maskedClearStencil || scissoredClear);
 
-    vk::Framebuffer *currentFramebuffer = nullptr;
-    ANGLE_TRY(getFramebuffer(contextVk, &currentFramebuffer, nullptr));
-    const bool isMidRenderPassClear =
-        contextVk->hasStartedRenderPassWithCommands() &&
-        contextVk->hasStartedRenderPassWithFramebuffer(currentFramebuffer);
+    const bool isMidRenderPassClear = contextVk->hasStartedRenderPassWithCommands();
+
+    if (isMidRenderPassClear)
+    {
+        // If a render pass is open with commands, it must be for this framebuffer.  Otherwise,
+        // either FramebufferVk::syncState() or ContextVk::syncState() would have closed it.
+        vk::Framebuffer *currentFramebuffer = nullptr;
+        ANGLE_TRY(getFramebuffer(contextVk, &currentFramebuffer, nullptr));
+        ASSERT(contextVk->hasStartedRenderPassWithFramebuffer(currentFramebuffer));
+    }
+
     const bool preferDrawOverClearAttachments =
         contextVk->getRenderer()->getFeatures().preferDrawClearOverVkCmdClearAttachments.enabled;
 
@@ -512,15 +518,15 @@ angle::Result FramebufferVk::clearImpl(const gl::Context *context,
                                       clearDepthStencilValue);
     }
 
-    // If any deferred clears, we can further defer them, modify the current render pass' loadOps,
-    // or clear them with vkCmdClearAttachments.
+    // If any deferred clears, we can further defer them, clear them with vkCmdClearAttachments or
+    // flush them if necessary.
     if (mDeferredClears.any())
     {
         const bool clearAnyWithDraw =
             clearColorWithDraw || clearDepthWithDraw || clearStencilWithDraw;
 
         // If we are in an active renderpass that has recorded commands and the framebuffer hasn't
-        // changed, inline the clear
+        // changed, inline the clear.
         if (isMidRenderPassClear)
         {
             ANGLE_PERF_WARNING(
@@ -536,20 +542,19 @@ angle::Result FramebufferVk::clearImpl(const gl::Context *context,
         }
         else
         {
+            ASSERT(!contextVk->hasStartedRenderPass());
+
             // This path will defer the current clears along with deferred clears.  This won't work
             // if any attachment needs to be subsequently cleared with a draw call.  In that case,
             // flush deferred clears, which will start a render pass with deferred clear values.
             // The subsequent draw call will then operate on the cleared attachments.
-            if (!contextVk->hasStartedRenderPass() && clearAnyWithDraw)
+            if (clearAnyWithDraw)
             {
                 ANGLE_TRY(flushDeferredClears(contextVk, scissoredRenderArea));
             }
             else
             {
-                // clearWithLoadOp will operate on deferred clears.  Either these will affect the
-                // currently open render pass' loadOps, or will be re-staged.  The latter will
-                // further defer these clears.
-                ANGLE_TRY(clearWithLoadOp(contextVk));
+                redeferClears(contextVk);
             }
         }
 
@@ -2111,8 +2116,10 @@ VkClearValue FramebufferVk::getCorrectedColorClearValue(size_t colorIndexGL,
     return clearValue;
 }
 
-angle::Result FramebufferVk::clearWithLoadOp(ContextVk *contextVk)
+void FramebufferVk::redeferClears(ContextVk *contextVk)
 {
+    ASSERT(!contextVk->hasStartedRenderPass());
+
     // Set the appropriate loadOp and clear values for depth and stencil.
     VkImageAspectFlags dsAspectFlags = 0;
     VkClearValue dsClearValue;
@@ -2131,56 +2138,26 @@ angle::Result FramebufferVk::clearWithLoadOp(ContextVk *contextVk)
         mDeferredClears.reset(vk::kUnpackedStencilIndex);
     }
 
-    // If the render pass has started, modify its ops directly.  Note that the render pass should
-    // otherwise not have any commands in it.
-    if (contextVk->hasStartedRenderPass())
+    // Go through deferred clears and stage the clears for future.
+    for (size_t colorIndexGL : mDeferredClears.getColorMask())
     {
-        vk::CommandBufferHelper &commands = contextVk->getStartedRenderPassCommands();
-
-        ASSERT(commands.getCommandBuffer().empty());
-
-        // Go through deferred clears and update render pass loadOp and clear colors.
-        vk::PackedAttachmentIndex colorIndexVk(0);
-        for (size_t colorIndexGL : mDeferredClears.getColorMask())
-        {
-            ASSERT(mState.getEnabledDrawBuffers().test(colorIndexGL));
-
-            commands.updateRenderPassColorClear(colorIndexVk, mDeferredClears[colorIndexGL]);
-            mDeferredClears.reset(colorIndexGL);
-            ++colorIndexVk;
-        }
+        ASSERT(mState.getEnabledDrawBuffers().test(colorIndexGL));
 
-        if (dsAspectFlags)
-        {
-            commands.updateRenderPassDepthStencilClear(dsAspectFlags, dsClearValue);
-            // If we were in depth read only mode, we must change to write mode
-            updateRenderPassReadOnlyDepthMode(contextVk, &commands);
-        }
+        RenderTargetVk *renderTarget = getColorDrawRenderTarget(colorIndexGL);
+        gl::ImageIndex imageIndex    = renderTarget->getImageIndex();
+        renderTarget->getImageForWrite().stageClear(imageIndex, VK_IMAGE_ASPECT_COLOR_BIT,
+                                                    mDeferredClears[colorIndexGL]);
+        mDeferredClears.reset(colorIndexGL);
     }
-    else
-    {
-        // Go through deferred clears and stage the clears for future.
-        for (size_t colorIndexGL : mDeferredClears.getColorMask())
-        {
-            ASSERT(mState.getEnabledDrawBuffers().test(colorIndexGL));
 
-            RenderTargetVk *renderTarget = getColorDrawRenderTarget(colorIndexGL);
-            gl::ImageIndex imageIndex    = renderTarget->getImageIndex();
-            renderTarget->getImageForWrite().stageClear(imageIndex, VK_IMAGE_ASPECT_COLOR_BIT,
-                                                        mDeferredClears[colorIndexGL]);
-            mDeferredClears.reset(colorIndexGL);
-        }
-
-        if (dsAspectFlags)
-        {
-            RenderTargetVk *renderTarget = getDepthStencilRenderTarget();
-            ASSERT(renderTarget);
+    if (dsAspectFlags)
+    {
+        RenderTargetVk *renderTarget = getDepthStencilRenderTarget();
+        ASSERT(renderTarget);
 
-            gl::ImageIndex imageIndex = renderTarget->getImageIndex();
-            renderTarget->getImageForWrite().stageClear(imageIndex, dsAspectFlags, dsClearValue);
-        }
+        gl::ImageIndex imageIndex = renderTarget->getImageIndex();
+        renderTarget->getImageForWrite().stageClear(imageIndex, dsAspectFlags, dsClearValue);
     }
-    return angle::Result::Continue;
 }
 
 angle::Result FramebufferVk::clearWithCommand(ContextVk *contextVk,

src/libANGLE/renderer/vulkan/FramebufferVk.h

@@ -186,7 +186,7 @@ class FramebufferVk : public FramebufferImpl
                                 uint8_t stencilMask,
                                 const VkClearColorValue &clearColorValue,
                                 const VkClearDepthStencilValue &clearDepthStencilValue);
-    angle::Result clearWithLoadOp(ContextVk *contextVk);
+    void redeferClears(ContextVk *contextVk);
     angle::Result clearWithCommand(ContextVk *contextVk,
                                    vk::CommandBufferHelper *renderpassCommands,
                                    const gl::Rectangle &scissoredRenderArea);

src/libANGLE/renderer/vulkan/README.md

@@ -76,6 +76,7 @@ More implementation details can be found in the `doc` directory:
 - [Shader Module Compilation](doc/ShaderModuleCompilation.md)
 - [OpenGL Line Segment Rasterization](doc/OpenGLLineSegmentRasterization.md)
 - [Format Tables and Emulation](doc/FormatTablesAndEmulation.md)
+- [Deferred Clears](doc/DeferredClears.md)
 
 [VkDevice]: https://www.khronos.org/registry/vulkan/specs/1.1-extensions/man/html/VkDevice.html
 [VkQueue]: https://www.khronos.org/registry/vulkan/specs/1.1-extensions/man/html/VkQueue.html

src/libANGLE/renderer/vulkan/doc/DeferredClears.md

+# Deferred Clears
+
+Take the following scenario:
+
+1. Application binds and clears FBO1
+2. Application binds FBO2 and renders to it
+3. Application binds FBO1 again and renders to it
+
+Steps 2 and 3 each require a render pass for rendering. The clear in step 1 can potentially be done
+through `loadOp` of the render pass for step 3, assuming step 2 doesn't use the attachments of FBO1.
+This optimization is achieved in ANGLE by deferring clears.
+
+When a clear is issued, one of the following happens:
+
+- If a render pass is already open, the framebuffer is cleared inline (using
+  `vkCmdClearAttachments`)
+- If the clear is not to the whole attachment (i.e. is scissored, or masked), a draw call is used to
+  perform the clear.
+- Otherwise the clear is deferred.
+
+Deferring a clear is done by staging a `Clear` update in the `vk::ImageHelper` corresponding to the
+attachment being cleared.
+
+There are two possibilities at this point:
+
+1. The `vk::ImageHelper` is used in any way other than as a framebuffer attachment (for example it's
+   sampled from), or
+2. It's used as a framebuffer attachment and rendering is done.
+
+In scenario 1, the staged updates in the `vk::ImageHelper` are flushed. That includes the `Clear`
+updates which will be done with an out-of-render-pass `vkCmdClear*Image` call.
+
+In scenario 2, `FramebufferVk::syncState` is responsible for extracting the staged `Clear` updates,
+assuming there are no subsequent updates to that subresource of the image, and keep them as
+_deferred clears_. The `FramebufferVk` call that immediately follows must handle these clears one
+way or another. In most cases, this implies starting a new render pass and using `loadOp`s to
+perform the clear before the actual operation in that function is performed. This also implies that
+the front-end must always follow a `syncState` call with another call (and for example cannot decide
+to no-op the call in between).
+
+If the subsequent call itself is a clear operation, there could be further optimizations. In
+particular, the previously deferred clears and can be overridden by and/or re-deferred along with
+the new clears.