Vulkan: Protect OneOff commands recording with pool mutex

Bug: angleproject:384940864
Change-Id: I518c54ae4b0fc5da0e58d330f8c531bc8d65529e
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/6108843
Commit-Queue: Igor Nazarov <i.nazarov@samsung.com>
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Reviewed-by: Charlie Lao <cclao@google.com>

Author: i-nazarov-ukraine

src/libANGLE/renderer/vulkan/CommandQueue.cpp

@@ -492,8 +492,8 @@ void CommandPoolAccess::destroyPrimaryCommandBuffer(VkDevice device,
                                                     PrimaryCommandBuffer *primaryCommands) const
 {
     ASSERT(primaryCommands->valid());
-    std::lock_guard<angle::SimpleMutex> lock(mCmdPoolMutex);
 
+    // Does not require a pool mutex lock.
     primaryCommands->destroy(device);
 }
 

src/libANGLE/renderer/vulkan/CommandQueue.h

@@ -543,6 +543,43 @@ class CleanUpThread : public Context
     std::condition_variable mWorkAvailableCondition;
     std::atomic<bool> mNeedCleanUp;
 };
+
+// Provides access to the PrimaryCommandBuffer while also locking the corresponding CommandPool
+class [[nodiscard]] ScopedPrimaryCommandBuffer final
+{
+  public:
+    explicit ScopedPrimaryCommandBuffer(VkDevice device) : mCommandBuffer(device) {}
+
+    void assign(std::unique_lock<angle::SimpleMutex> &&poolLock,
+                PrimaryCommandBuffer &&commandBuffer)
+    {
+        ASSERT(poolLock.owns_lock());
+        ASSERT(commandBuffer.valid());
+        ASSERT(mPoolLock.mutex() == nullptr);
+        ASSERT(!mCommandBuffer.get().valid());
+        mPoolLock            = std::move(poolLock);
+        mCommandBuffer.get() = std::move(commandBuffer);
+    }
+
+    PrimaryCommandBuffer &get()
+    {
+        ASSERT(mPoolLock.owns_lock());
+        ASSERT(mCommandBuffer.get().valid());
+        return mCommandBuffer.get();
+    }
+
+    DeviceScoped<PrimaryCommandBuffer> unlockAndRelease()
+    {
+        ASSERT(mCommandBuffer.get().valid() && mPoolLock.owns_lock() ||
+               !mCommandBuffer.get().valid() && mPoolLock.mutex() == nullptr);
+        mPoolLock = {};
+        return std::move(mCommandBuffer);
+    }
+
+  private:
+    std::unique_lock<angle::SimpleMutex> mPoolLock;
+    DeviceScoped<PrimaryCommandBuffer> mCommandBuffer;
+};
 }  // namespace vk
 
 }  // namespace rx

src/libANGLE/renderer/vulkan/ContextVk.cpp

@@ -3923,10 +3923,11 @@ angle::Result ContextVk::synchronizeCpuGpuTime()
         ANGLE_VK_TRY(this, gpuDone.get().reset(device));
 
         // Record the command buffer
-        vk::DeviceScoped<vk::PrimaryCommandBuffer> commandBatch(device);
-        vk::PrimaryCommandBuffer &commandBuffer = commandBatch.get();
+        vk::ScopedPrimaryCommandBuffer scopedCommandBuffer(device);
 
-        ANGLE_TRY(mRenderer->getCommandBufferOneOff(this, getProtectionType(), &commandBuffer));
+        ANGLE_TRY(
+            mRenderer->getCommandBufferOneOff(this, getProtectionType(), &scopedCommandBuffer));
+        vk::PrimaryCommandBuffer &commandBuffer = scopedCommandBuffer.get();
 
         commandBuffer.setEvent(gpuReady.get().getHandle(), VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT);
         commandBuffer.waitEvents(1, cpuReady.get().ptr(), VK_PIPELINE_STAGE_HOST_BIT,
@@ -3941,8 +3942,9 @@ angle::Result ContextVk::synchronizeCpuGpuTime()
         QueueSerial submitSerial;
         // vkEvent's are externally synchronized, therefore need work to be submitted before calling
         // vkGetEventStatus
-        ANGLE_TRY(mRenderer->queueSubmitOneOff(this, std::move(commandBuffer), getProtectionType(),
-                                               mContextPriority, VK_NULL_HANDLE, 0, &submitSerial));
+        ANGLE_TRY(mRenderer->queueSubmitOneOff(this, std::move(scopedCommandBuffer),
+                                               getProtectionType(), mContextPriority,
+                                               VK_NULL_HANDLE, 0, &submitSerial));
 
         // Track it with the submitSerial.
         timestampQuery.setQueueSerial(submitSerial);
@@ -7973,17 +7975,17 @@ angle::Result ContextVk::getTimestamp(uint64_t *timestampOut)
     ANGLE_TRY(timestampQueryPool.get().allocateQuery(this, &timestampQuery, 1));
 
     // Record the command buffer
-    vk::DeviceScoped<vk::PrimaryCommandBuffer> commandBatch(device);
-    vk::PrimaryCommandBuffer &commandBuffer = commandBatch.get();
+    vk::ScopedPrimaryCommandBuffer scopedCommandBuffer(device);
 
-    ANGLE_TRY(mRenderer->getCommandBufferOneOff(this, getProtectionType(), &commandBuffer));
+    ANGLE_TRY(mRenderer->getCommandBufferOneOff(this, getProtectionType(), &scopedCommandBuffer));
+    vk::PrimaryCommandBuffer &commandBuffer = scopedCommandBuffer.get();
 
     timestampQuery.writeTimestampToPrimary(this, &commandBuffer);
     ANGLE_VK_TRY(this, commandBuffer.end());
 
     QueueSerial submitQueueSerial;
-    ANGLE_TRY(mRenderer->queueSubmitOneOff(this, std::move(commandBuffer), getProtectionType(),
-                                           mContextPriority, VK_NULL_HANDLE, 0,
+    ANGLE_TRY(mRenderer->queueSubmitOneOff(this, std::move(scopedCommandBuffer),
+                                           getProtectionType(), mContextPriority, VK_NULL_HANDLE, 0,
                                            &submitQueueSerial));
     // Track it with the submitSerial.
     timestampQuery.setQueueSerial(submitQueueSerial);

src/libANGLE/renderer/vulkan/SurfaceVk.cpp

@@ -2881,12 +2881,13 @@ VkResult WindowSurfaceVk::postProcessUnlockedAcquire(vk::Context *context)
     {
         ASSERT(image.image->valid() &&
                image.image->getCurrentImageLayout() != vk::ImageLayout::SharedPresent);
-        rx::vk::Renderer *renderer = context->getRenderer();
-        rx::vk::PrimaryCommandBuffer primaryCommandBuffer;
+        vk::Renderer *renderer = context->getRenderer();
+        vk::ScopedPrimaryCommandBuffer scopedCommandBuffer(renderer->getDevice());
         auto protectionType = vk::ConvertProtectionBoolToType(mState.hasProtectedContent());
-        if (renderer->getCommandBufferOneOff(context, protectionType, &primaryCommandBuffer) ==
+        if (renderer->getCommandBufferOneOff(context, protectionType, &scopedCommandBuffer) ==
             angle::Result::Continue)
         {
+            vk::PrimaryCommandBuffer &primaryCommandBuffer = scopedCommandBuffer.get();
             VkSemaphore semaphore;
             // Note return errors is early exit may leave new Image and Swapchain in unknown state.
             image.image->recordWriteBarrierOneOff(context, vk::ImageLayout::SharedPresent,
@@ -2898,8 +2899,8 @@ VkResult WindowSurfaceVk::postProcessUnlockedAcquire(vk::Context *context)
                 return VK_ERROR_OUT_OF_DATE_KHR;
             }
             QueueSerial queueSerial;
-            if (renderer->queueSubmitOneOff(context, std::move(primaryCommandBuffer),
-                                            protectionType, egl::ContextPriority::Medium, semaphore,
+            if (renderer->queueSubmitOneOff(context, std::move(scopedCommandBuffer), protectionType,
+                                            egl::ContextPriority::Medium, semaphore,
                                             VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT,
                                             &queueSerial) != angle::Result::Continue)
             {

src/libANGLE/renderer/vulkan/vk_helpers.cpp

@@ -5555,23 +5555,24 @@ angle::Result BufferHelper::initializeNonZeroMemory(Context *context,
         StagingBuffer stagingBuffer;
         ANGLE_TRY(stagingBuffer.init(context, size, StagingUsage::Both));
 
-        PrimaryCommandBuffer commandBuffer;
-        ANGLE_TRY(
-            renderer->getCommandBufferOneOff(context, ProtectionType::Unprotected, &commandBuffer));
-
         // Queue a DMA copy.
         VkBufferCopy copyRegion = {};
         copyRegion.srcOffset    = 0;
         copyRegion.dstOffset    = getOffset();
         copyRegion.size         = size;
 
+        ScopedPrimaryCommandBuffer scopedCommandBuffer(renderer->getDevice());
+        ANGLE_TRY(renderer->getCommandBufferOneOff(context, ProtectionType::Unprotected,
+                                                   &scopedCommandBuffer));
+        PrimaryCommandBuffer &commandBuffer = scopedCommandBuffer.get();
+
         commandBuffer.copyBuffer(stagingBuffer.getBuffer(), getBuffer(), 1, &copyRegion);
 
         ANGLE_VK_TRY(context, commandBuffer.end());
 
         QueueSerial queueSerial;
         ANGLE_TRY(renderer->queueSubmitOneOff(
-            context, std::move(commandBuffer), ProtectionType::Unprotected,
+            context, std::move(scopedCommandBuffer), ProtectionType::Unprotected,
             egl::ContextPriority::Medium, VK_NULL_HANDLE, 0, &queueSerial));
 
         stagingBuffer.collectGarbage(renderer, queueSerial);
@@ -6172,9 +6173,10 @@ angle::Result ImageHelper::copyToBufferOneOff(Context *context,
                                               VkBufferImageCopy copyRegion)
 {
     Renderer *renderer = context->getRenderer();
-    PrimaryCommandBuffer commandBuffer;
-    ANGLE_TRY(
-        renderer->getCommandBufferOneOff(context, ProtectionType::Unprotected, &commandBuffer));
+    ScopedPrimaryCommandBuffer scopedCommandBuffer(renderer->getDevice());
+    ANGLE_TRY(renderer->getCommandBufferOneOff(context, ProtectionType::Unprotected,
+                                               &scopedCommandBuffer));
+    PrimaryCommandBuffer &commandBuffer = scopedCommandBuffer.get();
 
     VkSemaphore acquireNextImageSemaphore;
     barrierImpl(context, getAspectFlags(), ImageLayout::TransferDst,
@@ -6185,10 +6187,10 @@ angle::Result ImageHelper::copyToBufferOneOff(Context *context,
     ANGLE_VK_TRY(context, commandBuffer.end());
 
     QueueSerial submitQueueSerial;
-    ANGLE_TRY(
-        renderer->queueSubmitOneOff(context, std::move(commandBuffer), ProtectionType::Unprotected,
-                                    egl::ContextPriority::Medium, acquireNextImageSemaphore,
-                                    kSwapchainAcquireImageWaitStageFlags, &submitQueueSerial));
+    ANGLE_TRY(renderer->queueSubmitOneOff(
+        context, std::move(scopedCommandBuffer), ProtectionType::Unprotected,
+        egl::ContextPriority::Medium, acquireNextImageSemaphore,
+        kSwapchainAcquireImageWaitStageFlags, &submitQueueSerial));
 
     return renderer->finishQueueSerial(context, submitQueueSerial);
 }
@@ -6605,9 +6607,10 @@ angle::Result ImageHelper::initializeNonZeroMemory(Context *context,
     // setEvent.
     ASSERT(!mCurrentEvent.valid());
 
-    PrimaryCommandBuffer commandBuffer;
+    ScopedPrimaryCommandBuffer scopedCommandBuffer(renderer->getDevice());
     auto protectionType = ConvertProtectionBoolToType(hasProtectedContent);
-    ANGLE_TRY(renderer->getCommandBufferOneOff(context, protectionType, &commandBuffer));
+    ANGLE_TRY(renderer->getCommandBufferOneOff(context, protectionType, &scopedCommandBuffer));
+    PrimaryCommandBuffer &commandBuffer = scopedCommandBuffer.get();
 
     // Queue a DMA copy.
     VkSemaphore acquireNextImageSemaphore;
@@ -6693,7 +6696,7 @@ angle::Result ImageHelper::initializeNonZeroMemory(Context *context,
     ANGLE_VK_TRY(context, commandBuffer.end());
 
     QueueSerial queueSerial;
-    ANGLE_TRY(renderer->queueSubmitOneOff(context, std::move(commandBuffer), protectionType,
+    ANGLE_TRY(renderer->queueSubmitOneOff(context, std::move(scopedCommandBuffer), protectionType,
                                           egl::ContextPriority::Medium, VK_NULL_HANDLE, 0,
                                           &queueSerial));
 
@@ -10856,11 +10859,12 @@ angle::Result ImageHelper::copySurfaceImageToBuffer(DisplayVk *displayVk,
 
     // We may have a valid event here but we do not have a collector to collect it. Release the
     // event here to force pipelineBarrier.
-    mCurrentEvent.release(displayVk->getRenderer());
+    mCurrentEvent.release(renderer);
 
-    PrimaryCommandBuffer primaryCommandBuffer;
+    ScopedPrimaryCommandBuffer scopedCommandBuffer(renderer->getDevice());
     ANGLE_TRY(renderer->getCommandBufferOneOff(displayVk, ProtectionType::Unprotected,
-                                               &primaryCommandBuffer));
+                                               &scopedCommandBuffer));
+    PrimaryCommandBuffer &primaryCommandBuffer = scopedCommandBuffer.get();
 
     VkSemaphore acquireNextImageSemaphore;
     barrierImpl(displayVk, getAspectFlags(), ImageLayout::TransferSrc,
@@ -10873,7 +10877,7 @@ angle::Result ImageHelper::copySurfaceImageToBuffer(DisplayVk *displayVk,
 
     QueueSerial submitQueueSerial;
     ANGLE_TRY(renderer->queueSubmitOneOff(
-        displayVk, std::move(primaryCommandBuffer), ProtectionType::Unprotected,
+        displayVk, std::move(scopedCommandBuffer), ProtectionType::Unprotected,
         egl::ContextPriority::Medium, acquireNextImageSemaphore,
         kSwapchainAcquireImageWaitStageFlags, &submitQueueSerial));
 
@@ -10910,9 +10914,10 @@ angle::Result ImageHelper::copyBufferToSurfaceImage(DisplayVk *displayVk,
     // event here to force pipelineBarrier.
     mCurrentEvent.release(displayVk->getRenderer());
 
-    PrimaryCommandBuffer commandBuffer;
-    ANGLE_TRY(
-        renderer->getCommandBufferOneOff(displayVk, ProtectionType::Unprotected, &commandBuffer));
+    ScopedPrimaryCommandBuffer scopedCommandBuffer(renderer->getDevice());
+    ANGLE_TRY(renderer->getCommandBufferOneOff(displayVk, ProtectionType::Unprotected,
+                                               &scopedCommandBuffer));
+    PrimaryCommandBuffer &commandBuffer = scopedCommandBuffer.get();
 
     VkSemaphore acquireNextImageSemaphore;
     barrierImpl(displayVk, getAspectFlags(), ImageLayout::TransferDst,
@@ -10925,7 +10930,7 @@ angle::Result ImageHelper::copyBufferToSurfaceImage(DisplayVk *displayVk,
 
     QueueSerial submitQueueSerial;
     ANGLE_TRY(renderer->queueSubmitOneOff(
-        displayVk, std::move(commandBuffer), ProtectionType::Unprotected,
+        displayVk, std::move(scopedCommandBuffer), ProtectionType::Unprotected,
         egl::ContextPriority::Medium, acquireNextImageSemaphore,
         kSwapchainAcquireImageWaitStageFlags, &submitQueueSerial));
 

src/libANGLE/renderer/vulkan/vk_renderer.cpp

@@ -1672,16 +1672,18 @@ void OneOffCommandPool::destroy(VkDevice device)
 }
 
 angle::Result OneOffCommandPool::getCommandBuffer(vk::Context *context,
-                                                  vk::PrimaryCommandBuffer *commandBufferOut)
+                                                  vk::ScopedPrimaryCommandBuffer *commandBufferOut)
 {
     std::unique_lock<angle::SimpleMutex> lock(mMutex);
 
     if (!mPendingCommands.empty() &&
         context->getRenderer()->hasResourceUseFinished(mPendingCommands.front().use))
     {
-        *commandBufferOut = std::move(mPendingCommands.front().commandBuffer);
+        commandBufferOut->assign(std::move(lock),
+                                 std::move(mPendingCommands.front().commandBuffer));
         mPendingCommands.pop_front();
-        ANGLE_VK_TRY(context, commandBufferOut->reset());
+        // No need to explicitly call reset() on |commandBufferOut|, since the begin() call below
+        // will do it implicitly.
     }
     else
     {
@@ -1707,14 +1709,16 @@ angle::Result OneOffCommandPool::getCommandBuffer(vk::Context *context,
         allocInfo.commandBufferCount          = 1;
         allocInfo.commandPool                 = mCommandPool.getHandle();
 
-        ANGLE_VK_TRY(context, commandBufferOut->init(context->getDevice(), allocInfo));
+        PrimaryCommandBuffer newCommandBuffer;
+        ANGLE_VK_TRY(context, newCommandBuffer.init(context->getDevice(), allocInfo));
+        commandBufferOut->assign(std::move(lock), std::move(newCommandBuffer));
     }
 
     VkCommandBufferBeginInfo beginInfo = {};
     beginInfo.sType                    = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO;
     beginInfo.flags                    = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT;
     beginInfo.pInheritanceInfo         = nullptr;
-    ANGLE_VK_TRY(context, commandBufferOut->begin(beginInfo));
+    ANGLE_VK_TRY(context, commandBufferOut->get().begin(beginInfo));
 
     return angle::Result::Continue;
 }
@@ -6201,14 +6205,17 @@ void Renderer::outputVmaStatString()
 }
 
 angle::Result Renderer::queueSubmitOneOff(vk::Context *context,
-                                          vk::PrimaryCommandBuffer &&primary,
+                                          vk::ScopedPrimaryCommandBuffer &&scopedCommandBuffer,
                                           vk::ProtectionType protectionType,
                                           egl::ContextPriority priority,
                                           VkSemaphore waitSemaphore,
                                           VkPipelineStageFlags waitSemaphoreStageMasks,
                                           QueueSerial *queueSerialOut)
 {
     ANGLE_TRACE_EVENT0("gpu.angle", "Renderer::queueSubmitOneOff");
+    DeviceScoped<PrimaryCommandBuffer> commandBuffer = scopedCommandBuffer.unlockAndRelease();
+    PrimaryCommandBuffer &primary                    = commandBuffer.get();
+
     // Allocate a one off SerialIndex and generate a QueueSerial and then use it and release the
     // index.
     vk::ScopedQueueSerialIndex index;

src/libANGLE/renderer/vulkan/vk_renderer.h

@@ -119,7 +119,7 @@ class OneOffCommandPool : angle::NonCopyable
     OneOffCommandPool();
     void init(vk::ProtectionType protectionType);
     angle::Result getCommandBuffer(vk::Context *context,
-                                   vk::PrimaryCommandBuffer *commandBufferOut);
+                                   vk::ScopedPrimaryCommandBuffer *commandBufferOut);
     void releaseCommandBuffer(const QueueSerial &submitQueueSerial,
                               vk::PrimaryCommandBuffer &&primary);
     void destroy(VkDevice device);
@@ -307,15 +307,15 @@ class Renderer : angle::NonCopyable
     // This command buffer should be submitted immediately via queueSubmitOneOff.
     angle::Result getCommandBufferOneOff(vk::Context *context,
                                          vk::ProtectionType protectionType,
-                                         vk::PrimaryCommandBuffer *commandBufferOut)
+                                         vk::ScopedPrimaryCommandBuffer *commandBufferOut)
     {
         return mOneOffCommandPoolMap[protectionType].getCommandBuffer(context, commandBufferOut);
     }
 
     // Fire off a single command buffer immediately with default priority.
     // Command buffer must be allocated with getCommandBufferOneOff and is reclaimed.
     angle::Result queueSubmitOneOff(vk::Context *context,
-                                    vk::PrimaryCommandBuffer &&primary,
+                                    vk::ScopedPrimaryCommandBuffer &&scopedCommandBuffer,
                                     vk::ProtectionType protectionType,
                                     egl::ContextPriority priority,
                                     VkSemaphore waitSemaphore,

src/libANGLE/renderer/vulkan/vk_utils.h

@@ -633,7 +633,8 @@ template <typename T>
 class [[nodiscard]] DeviceScoped final : angle::NonCopyable
 {
   public:
-    DeviceScoped(VkDevice device) : mDevice(device) {}
+    explicit DeviceScoped(VkDevice device) : mDevice(device) {}
+    DeviceScoped(DeviceScoped &&other) : mDevice(other.mDevice), mVar(std::move(other.mVar)) {}
     ~DeviceScoped() { mVar.destroy(mDevice); }
 
     const T &get() const { return mVar; }

src/libANGLE/renderer/vulkan/vk_wrapper.h

@@ -172,6 +172,7 @@ class CommandBuffer : public WrappedObject<CommandBuffer, VkCommandBuffer>
     VkCommandBuffer releaseHandle();
 
     // This is used for normal pool allocated command buffers. It reset the handle.
+    // Note: this method does not require pool synchronization (locking the pool mutex).
     void destroy(VkDevice device);
 
     // This is used in conjunction with VK_COMMAND_POOL_CREATE_RESET_COMMAND_BUFFER_BIT.
@@ -910,6 +911,8 @@ ANGLE_INLINE void CommandBuffer::imageWaitEvent(const VkEvent &event,
 
 ANGLE_INLINE void CommandBuffer::destroy(VkDevice device)
 {
+    // Note: do not add code that may access the pool in any way, because this method may be called
+    // without taking the pool mutex lock.
     releaseHandle();
 }