Vulkan: Properly handle errors for fences and commands

This commit makes the following changes:
 - Unused fences are reset when they will next be used so there is a
   single place where error handling is needed, either for resetting
   an existing fence, or for creating a new fence.
 - All accesses to VkCommandBuffer are moved to using the
   CommandRecordingContext and GetPendingCommandBuffer is removed.
 - Instead of tracking both a current (VkCommandBuffer + Pool) and
   a command recording context that contains the same VkCommandBuffer,
   the RecordingContext now holds the pool too, as well as a tag to
   know if it was ever queried (meaning it contains commands).
 - mRecordingContext is now always valid, such that
   GetRecordingContext() doesn't need to return an error.

BUG=dawn:19

Change-Id: I853d4ecdc6905b66e842688f39d863e362f59b66
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/12022
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

src/dawn_native/vulkan/CommandRecordingContext.h

@@ -33,6 +33,10 @@ namespace dawn_native { namespace vulkan {
         // The internal buffers used in the workaround of texture-to-texture copies with compressed
         // formats.
         std::vector<Ref<Buffer>> tempBuffers;
+
+        // For Device state tracking only.
+        VkCommandPool commandPool = VK_NULL_HANDLE;
+        bool used = false;
     };
 
 }}  // namespace dawn_native::vulkan

src/dawn_native/vulkan/DeviceVk.cpp

@@ -75,12 +75,15 @@ namespace dawn_native { namespace vulkan {
         mExternalMemoryService = std::make_unique<external_memory::Service>(this);
         mExternalSemaphoreService = std::make_unique<external_semaphore::Service>(this);
 
+        DAWN_TRY(PrepareRecordingContext());
+
         return {};
     }
 
     Device::~Device() {
-        // Immediately forget about all pending commands so we don't try to submit them in Tick
+        // Immediately tag the recording context as unused so we don't try to submit it in Tick.
-        FreeCommands(&mPendingCommands);
+        mRecordingContext.used = false;
+        fn.DestroyCommandPool(mVkDevice, mRecordingContext.commandPool, nullptr);
 
         if (fn.QueueWaitIdle(mQueue) != VK_SUCCESS) {
             ASSERT(false);
@@ -110,8 +113,8 @@ namespace dawn_native { namespace vulkan {
         Tick();
 
         ASSERT(mCommandsInFlight.Empty());
-        for (auto& commands : mUnusedCommands) {
+        for (const CommandPoolAndBuffer& commands : mUnusedCommands) {
-            FreeCommands(&commands);
+            fn.DestroyCommandPool(mVkDevice, commands.pool, nullptr);
         }
         mUnusedCommands.clear();
 
@@ -221,7 +224,7 @@ namespace dawn_native { namespace vulkan {
 
         mDeleter->Tick(mCompletedSerial);
 
-        if (mPendingCommands.pool != VK_NULL_HANDLE) {
+        if (mRecordingContext.used) {
             DAWN_TRY(SubmitPendingCommands());
         } else if (mCompletedSerial == mLastSubmittedSerial) {
             // If there's no GPU work in flight we still need to artificially increment the serial
@@ -268,38 +271,18 @@ namespace dawn_native { namespace vulkan {
         return mRenderPassCache.get();
     }
 
-    VkCommandBuffer Device::GetPendingCommandBuffer() {
-        if (mPendingCommands.pool == VK_NULL_HANDLE) {
-            mPendingCommands = GetUnusedCommands();
-
-            VkCommandBufferBeginInfo beginInfo;
-            beginInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO;
-            beginInfo.pNext = nullptr;
-            beginInfo.flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT;
-            beginInfo.pInheritanceInfo = nullptr;
-
-            if (fn.BeginCommandBuffer(mPendingCommands.commandBuffer, &beginInfo) != VK_SUCCESS) {
-                ASSERT(false);
-            }
-        }
-
-        return mPendingCommands.commandBuffer;
-    }
-
     CommandRecordingContext* Device::GetPendingRecordingContext() {
-        if (mRecordingContext.commandBuffer == VK_NULL_HANDLE) {
+        ASSERT(mRecordingContext.commandBuffer != VK_NULL_HANDLE);
-            mRecordingContext.commandBuffer = GetPendingCommandBuffer();
+        mRecordingContext.used = true;
-        }
-
         return &mRecordingContext;
     }
 
     MaybeError Device::SubmitPendingCommands() {
-        if (mPendingCommands.pool == VK_NULL_HANDLE) {
+        if (!mRecordingContext.used) {
             return {};
         }
 
-        DAWN_TRY(CheckVkSuccess(fn.EndCommandBuffer(mPendingCommands.commandBuffer),
+        DAWN_TRY(CheckVkSuccess(fn.EndCommandBuffer(mRecordingContext.commandBuffer),
                                 "vkEndCommandBuffer"));
 
         std::vector<VkPipelineStageFlags> dstStageMasks(mRecordingContext.waitSemaphores.size(),
@@ -313,19 +296,24 @@ namespace dawn_native { namespace vulkan {
         submitInfo.pWaitSemaphores = mRecordingContext.waitSemaphores.data();
         submitInfo.pWaitDstStageMask = dstStageMasks.data();
         submitInfo.commandBufferCount = 1;
-        submitInfo.pCommandBuffers = &mPendingCommands.commandBuffer;
+        submitInfo.pCommandBuffers = &mRecordingContext.commandBuffer;
         submitInfo.signalSemaphoreCount =
             static_cast<uint32_t>(mRecordingContext.signalSemaphores.size());
         submitInfo.pSignalSemaphores = mRecordingContext.signalSemaphores.data();
 
-        VkFence fence = GetUnusedFence();
+        VkFence fence = VK_NULL_HANDLE;
+        DAWN_TRY_ASSIGN(fence, GetUnusedFence());
         DAWN_TRY(CheckVkSuccess(fn.QueueSubmit(mQueue, 1, &submitInfo, fence), "vkQueueSubmit"));
 
         mLastSubmittedSerial++;
-        mCommandsInFlight.Enqueue(mPendingCommands, mLastSubmittedSerial);
-        mPendingCommands = CommandPoolAndBuffer();
         mFencesInFlight.emplace(fence, mLastSubmittedSerial);
 
+        CommandPoolAndBuffer submittedCommands = {mRecordingContext.commandPool,
+                                                  mRecordingContext.commandBuffer};
+        mCommandsInFlight.Enqueue(submittedCommands, mLastSubmittedSerial);
+        mRecordingContext = CommandRecordingContext();
+        DAWN_TRY(PrepareRecordingContext());
+
         for (VkSemaphore semaphore : mRecordingContext.waitSemaphores) {
             mDeleter->DeleteWhenUnused(semaphore);
         }
@@ -334,8 +322,6 @@ namespace dawn_native { namespace vulkan {
             mDeleter->DeleteWhenUnused(semaphore);
         }
 
-        mRecordingContext = CommandRecordingContext();
-
         return {};
     }
 
@@ -461,9 +447,11 @@ namespace dawn_native { namespace vulkan {
         return const_cast<VulkanFunctions*>(&fn);
     }
 
-    VkFence Device::GetUnusedFence() {
+    ResultOrError<VkFence> Device::GetUnusedFence() {
         if (!mUnusedFences.empty()) {
             VkFence fence = mUnusedFences.back();
+            DAWN_TRY(CheckVkSuccess(fn.ResetFences(mVkDevice, 1, &fence), "vkResetFences"));
+
             mUnusedFences.pop_back();
             return fence;
         }
@@ -474,9 +462,8 @@ namespace dawn_native { namespace vulkan {
         createInfo.flags = 0;
 
         VkFence fence = VK_NULL_HANDLE;
-        if (fn.CreateFence(mVkDevice, &createInfo, nullptr, &fence) != VK_SUCCESS) {
+        DAWN_TRY(CheckVkSuccess(fn.CreateFence(mVkDevice, &createInfo, nullptr, &fence),
-            ASSERT(false);
+                                "vkCreateFence"));
-        }
 
         return fence;
     }
@@ -495,11 +482,7 @@ namespace dawn_native { namespace vulkan {
                 return;
             }
 
-            if (fn.ResetFences(mVkDevice, 1, &fence) != VK_SUCCESS) {
-                ASSERT(false);
-            }
             mUnusedFences.push_back(fence);
-
             mFencesInFlight.pop();
 
             ASSERT(fenceSerial > mCompletedSerial);
@@ -507,60 +490,62 @@ namespace dawn_native { namespace vulkan {
         }
     }
 
-    Device::CommandPoolAndBuffer Device::GetUnusedCommands() {
+    MaybeError Device::PrepareRecordingContext() {
+        ASSERT(!mRecordingContext.used);
+        ASSERT(mRecordingContext.commandBuffer == VK_NULL_HANDLE);
+        ASSERT(mRecordingContext.commandPool == VK_NULL_HANDLE);
+
+        // First try to recycle unused command pools.
         if (!mUnusedCommands.empty()) {
             CommandPoolAndBuffer commands = mUnusedCommands.back();
             mUnusedCommands.pop_back();
-            return commands;
+            DAWN_TRY(CheckVkSuccess(fn.ResetCommandPool(mVkDevice, commands.pool, 0),
+                                    "vkResetCommandPool"));
+
+            mRecordingContext.commandBuffer = commands.commandBuffer;
+            mRecordingContext.commandPool = commands.pool;
+        } else {
+            // Create a new command pool for our commands and allocate the command buffer.
+            VkCommandPoolCreateInfo createInfo;
+            createInfo.sType = VK_STRUCTURE_TYPE_COMMAND_POOL_CREATE_INFO;
+            createInfo.pNext = nullptr;
+            createInfo.flags = VK_COMMAND_POOL_CREATE_TRANSIENT_BIT;
+            createInfo.queueFamilyIndex = mQueueFamily;
+
+            DAWN_TRY(CheckVkSuccess(fn.CreateCommandPool(mVkDevice, &createInfo, nullptr,
+                                                         &mRecordingContext.commandPool),
+                                    "vkCreateCommandPool"));
+
+            VkCommandBufferAllocateInfo allocateInfo;
+            allocateInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO;
+            allocateInfo.pNext = nullptr;
+            allocateInfo.commandPool = mRecordingContext.commandPool;
+            allocateInfo.level = VK_COMMAND_BUFFER_LEVEL_PRIMARY;
+            allocateInfo.commandBufferCount = 1;
+
+            DAWN_TRY(CheckVkSuccess(fn.AllocateCommandBuffers(mVkDevice, &allocateInfo,
+                                                              &mRecordingContext.commandBuffer),
+                                    "vkAllocateCommandBuffers"));
         }
 
-        CommandPoolAndBuffer commands;
+        // Start the recording of commands in the command buffer.
+        VkCommandBufferBeginInfo beginInfo;
+        beginInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO;
+        beginInfo.pNext = nullptr;
+        beginInfo.flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT;
+        beginInfo.pInheritanceInfo = nullptr;
 
-        VkCommandPoolCreateInfo createInfo;
+        return CheckVkSuccess(fn.BeginCommandBuffer(mRecordingContext.commandBuffer, &beginInfo),
-        createInfo.sType = VK_STRUCTURE_TYPE_COMMAND_POOL_CREATE_INFO;
+                              "vkBeginCommandBuffer");
-        createInfo.pNext = nullptr;
-        createInfo.flags = VK_COMMAND_POOL_CREATE_TRANSIENT_BIT;
-        createInfo.queueFamilyIndex = mQueueFamily;
-
-        if (fn.CreateCommandPool(mVkDevice, &createInfo, nullptr, &commands.pool) != VK_SUCCESS) {
-            ASSERT(false);
-        }
-
-        VkCommandBufferAllocateInfo allocateInfo;
-        allocateInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO;
-        allocateInfo.pNext = nullptr;
-        allocateInfo.commandPool = commands.pool;
-        allocateInfo.level = VK_COMMAND_BUFFER_LEVEL_PRIMARY;
-        allocateInfo.commandBufferCount = 1;
-
-        if (fn.AllocateCommandBuffers(mVkDevice, &allocateInfo, &commands.commandBuffer) !=
-            VK_SUCCESS) {
-            ASSERT(false);
-        }
-
-        return commands;
     }
 
     void Device::RecycleCompletedCommands() {
         for (auto& commands : mCommandsInFlight.IterateUpTo(mCompletedSerial)) {
-            if (fn.ResetCommandPool(mVkDevice, commands.pool, 0) != VK_SUCCESS) {
-                ASSERT(false);
-            }
             mUnusedCommands.push_back(commands);
         }
         mCommandsInFlight.ClearUpTo(mCompletedSerial);
     }
 
-    void Device::FreeCommands(CommandPoolAndBuffer* commands) {
-        if (commands->pool != VK_NULL_HANDLE) {
-            fn.DestroyCommandPool(mVkDevice, commands->pool, nullptr);
-            commands->pool = VK_NULL_HANDLE;
-        }
-
-        // Command buffers are implicitly destroyed when the command pool is.
-        commands->commandBuffer = VK_NULL_HANDLE;
-    }
-
     ResultOrError<std::unique_ptr<StagingBufferBase>> Device::CreateStagingBuffer(size_t size) {
         std::unique_ptr<StagingBufferBase> stagingBuffer =
             std::make_unique<StagingBuffer>(size, this);
@@ -573,6 +558,8 @@ namespace dawn_native { namespace vulkan {
                                                BufferBase* destination,
                                                uint64_t destinationOffset,
                                                uint64_t size) {
+        CommandRecordingContext* recordingContext = GetPendingRecordingContext();
+
         // Insert memory barrier to ensure host write operations are made visible before
         // copying from the staging buffer. However, this barrier can be removed (see note below).
         //
@@ -582,15 +569,15 @@ namespace dawn_native { namespace vulkan {
 
         // Insert pipeline barrier to ensure correct ordering with previous memory operations on the
         // buffer.
-        ToBackend(destination)
+        ToBackend(destination)->TransitionUsageNow(recordingContext, dawn::BufferUsage::CopyDst);
-            ->TransitionUsageNow(GetPendingRecordingContext(), dawn::BufferUsage::CopyDst);
 
         VkBufferCopy copy;
         copy.srcOffset = sourceOffset;
         copy.dstOffset = destinationOffset;
         copy.size = size;
 
-        this->fn.CmdCopyBuffer(GetPendingCommandBuffer(), ToBackend(source)->GetBufferHandle(),
+        this->fn.CmdCopyBuffer(recordingContext->commandBuffer,
+                               ToBackend(source)->GetBufferHandle(),
                                ToBackend(destination)->GetHandle(), 1, &copy);
 
         return {};

src/dawn_native/vulkan/DeviceVk.h

@@ -64,7 +64,6 @@ namespace dawn_native { namespace vulkan {
         MemoryAllocator* GetMemoryAllocator() const;
         RenderPassCache* GetRenderPassCache() const;
 
-        VkCommandBuffer GetPendingCommandBuffer();
         CommandRecordingContext* GetPendingRecordingContext();
         Serial GetPendingCommandSerial() const override;
         MaybeError SubmitPendingCommands();
@@ -144,7 +143,7 @@ namespace dawn_native { namespace vulkan {
         std::unique_ptr<external_memory::Service> mExternalMemoryService;
         std::unique_ptr<external_semaphore::Service> mExternalSemaphoreService;
 
-        VkFence GetUnusedFence();
+        ResultOrError<VkFence> GetUnusedFence();
         void CheckPassedFences();
 
         // We track which operations are in flight on the GPU with an increasing serial.
@@ -152,22 +151,22 @@ namespace dawn_native { namespace vulkan {
         // to a serial and a fence, such that when the fence is "ready" we know the operations
         // have finished.
         std::queue<std::pair<VkFence, Serial>> mFencesInFlight;
+        // Fences in the unused list aren't reset yet.
         std::vector<VkFence> mUnusedFences;
         Serial mCompletedSerial = 0;
         Serial mLastSubmittedSerial = 0;
 
+        MaybeError PrepareRecordingContext();
+        void RecycleCompletedCommands();
+
         struct CommandPoolAndBuffer {
             VkCommandPool pool = VK_NULL_HANDLE;
             VkCommandBuffer commandBuffer = VK_NULL_HANDLE;
         };
-
-        CommandPoolAndBuffer GetUnusedCommands();
-        void RecycleCompletedCommands();
-        void FreeCommands(CommandPoolAndBuffer* commands);
-
         SerialQueue<CommandPoolAndBuffer> mCommandsInFlight;
+        // Command pools in the unused list haven't been reset yet.
         std::vector<CommandPoolAndBuffer> mUnusedCommands;
-        CommandPoolAndBuffer mPendingCommands;
+        // There is always a valid recording context stored in mRecordingContext
         CommandRecordingContext mRecordingContext;
 
         MaybeError ImportExternalImage(const ExternalImageDescriptor* descriptor,

src/dawn_native/vulkan/NativeSwapChainImplVk.cpp

@@ -132,7 +132,7 @@ namespace dawn_native { namespace vulkan {
 
         // Do the initial layout transition for all these images from an undefined layout to
         // present so that it matches the "present" usage after the first GetNextTexture.
-        VkCommandBuffer commands = mDevice->GetPendingCommandBuffer();
+        CommandRecordingContext* recordingContext = mDevice->GetPendingRecordingContext();
         for (VkImage image : mSwapChainImages) {
             VkImageMemoryBarrier barrier;
             barrier.sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER;
@@ -150,9 +150,9 @@ namespace dawn_native { namespace vulkan {
             barrier.subresourceRange.baseArrayLayer = 0;
             barrier.subresourceRange.layerCount = 1;
 
-            mDevice->fn.CmdPipelineBarrier(commands, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT,
+            mDevice->fn.CmdPipelineBarrier(
-                                           VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, 0, 0, nullptr, 0,
+                recordingContext->commandBuffer, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT,
-                                           nullptr, 1, &barrier);
+                VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, 0, 0, nullptr, 0, nullptr, 1, &barrier);
         }
 
         if (oldSwapchain != VK_NULL_HANDLE) {