From fbe4ac0957231c1f154e4d699020d6b3c9e51800 Mon Sep 17 00:00:00 2001 From: Loko Kung Date: Tue, 29 Nov 2022 05:29:27 +0000 Subject: [PATCH] Fixes Vulkan command buffer leaks when an error occurs. - Uses an anonymous function to delete command pool/buffers. - Shuffles the code around a bit so that the CommandPoolAndBuffer are clearly next to the EncodingContext stuff to make it clear that we may be able to consolidate them in the future. Bug: chromium:1372772 Change-Id: I92a1d0333b7a85d439b5963a58db69ac685c03a4 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/112181 Reviewed-by: Austin Eng Kokoro: Kokoro Commit-Queue: Loko Kung --- .../native/vulkan/CommandRecordingContext.h | 10 +++ src/dawn/native/vulkan/DeviceVk.cpp | 61 +++++++++---------- src/dawn/native/vulkan/DeviceVk.h | 5 -- 3 files changed, 39 insertions(+), 37 deletions(-) diff --git a/src/dawn/native/vulkan/CommandRecordingContext.h b/src/dawn/native/vulkan/CommandRecordingContext.h index 7948e3b19d..7bd762f69c 100644 --- a/src/dawn/native/vulkan/CommandRecordingContext.h +++ b/src/dawn/native/vulkan/CommandRecordingContext.h @@ -19,11 +19,21 @@ #include "dawn/common/vulkan_platform.h" #include "dawn/native/vulkan/BufferVk.h" +#include "dawn/native/vulkan/VulkanFunctions.h" namespace dawn::native::vulkan { class Texture; +// Wrapping class that currently associates a command buffer to it's corresponding pool. +// TODO(dawn:1601) Revisit this structure since it is where the 1:1 mapping is implied. +// Also consider reusing this in CommandRecordingContext below instead of +// flattening the pool and command buffer again. +struct CommandPoolAndBuffer { + VkCommandPool pool = VK_NULL_HANDLE; + VkCommandBuffer commandBuffer = VK_NULL_HANDLE; +}; + // Used to track operations that are handled after recording. // Currently only tracks semaphores, but may be used to do barrier coalescing in the future. struct CommandRecordingContext { diff --git a/src/dawn/native/vulkan/DeviceVk.cpp b/src/dawn/native/vulkan/DeviceVk.cpp index fed1cb9244..63c94a2fc7 100644 --- a/src/dawn/native/vulkan/DeviceVk.cpp +++ b/src/dawn/native/vulkan/DeviceVk.cpp @@ -69,6 +69,23 @@ class ScopedSignalSemaphore : public NonMovable { VkSemaphore mSemaphore = VK_NULL_HANDLE; }; +// Destroys command pool/buffer. +// TODO(dawn:1601) Revisit this and potentially bake into pool/buffer objects instead. +void DestroyCommandPoolAndBuffer(const VulkanFunctions& fn, + VkDevice device, + const CommandPoolAndBuffer& commands) { + // The VkCommandBuffer memory should be wholly owned by the pool and freed when it is + // destroyed, but that's not the case in some drivers and they leak memory. So we call + // FreeCommandBuffers before DestroyCommandPool to be safe. + // TODO(enga): Only do this on a known list of bad drivers. + if (commands.pool != VK_NULL_HANDLE) { + if (commands.commandBuffer != VK_NULL_HANDLE) { + fn.FreeCommandBuffers(device, commands.pool, 1, &commands.commandBuffer); + } + fn.DestroyCommandPool(device, commands.pool, nullptr); + } +} + } // namespace // static @@ -745,7 +762,7 @@ MaybeError Device::SplitRecordingContext(CommandRecordingContext* recordingConte return {}; } -ResultOrError Device::BeginVkCommandBuffer() { +ResultOrError Device::BeginVkCommandBuffer() { CommandPoolAndBuffer commands; // First try to recycle unused command pools. @@ -754,19 +771,7 @@ ResultOrError Device::BeginVkCommandBuffer() { mUnusedCommands.pop_back(); DAWN_TRY_WITH_CLEANUP( CheckVkSuccess(fn.ResetCommandPool(mVkDevice, commands.pool, 0), "vkResetCommandPool"), - { - // vkResetCommandPool failed (it may return out-of-memory). - // Free the commands in the cleanup step before returning to - // reclaim memory. - - // The VkCommandBuffer memory should be wholly owned by the - // pool and freed when it is destroyed, but that's not the - // case in some drivers and they leak memory. So we call - // FreeCommandBuffers before DestroyCommandPool to be safe. - // TODO(enga): Only do this on a known list of bad drivers. - fn.FreeCommandBuffers(mVkDevice, commands.pool, 1, &commands.commandBuffer); - fn.DestroyCommandPool(mVkDevice, commands.pool, nullptr); - }); + { DestroyCommandPoolAndBuffer(fn, mVkDevice, commands); }); } else { // Create a new command pool for our commands and allocate the command buffer. VkCommandPoolCreateInfo createInfo; @@ -786,9 +791,10 @@ ResultOrError Device::BeginVkCommandBuffer() { allocateInfo.level = VK_COMMAND_BUFFER_LEVEL_PRIMARY; allocateInfo.commandBufferCount = 1; - DAWN_TRY(CheckVkSuccess( - fn.AllocateCommandBuffers(mVkDevice, &allocateInfo, &commands.commandBuffer), - "vkAllocateCommandBuffers")); + DAWN_TRY_WITH_CLEANUP(CheckVkSuccess(fn.AllocateCommandBuffers(mVkDevice, &allocateInfo, + &commands.commandBuffer), + "vkAllocateCommandBuffers"), + { DestroyCommandPoolAndBuffer(fn, mVkDevice, commands); }); } // Start the recording of commands in the command buffer. @@ -798,8 +804,9 @@ ResultOrError Device::BeginVkCommandBuffer() { beginInfo.flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT; beginInfo.pInheritanceInfo = nullptr; - DAWN_TRY(CheckVkSuccess(fn.BeginCommandBuffer(commands.commandBuffer, &beginInfo), - "vkBeginCommandBuffer")); + DAWN_TRY_WITH_CLEANUP(CheckVkSuccess(fn.BeginCommandBuffer(commands.commandBuffer, &beginInfo), + "vkBeginCommandBuffer"), + { DestroyCommandPoolAndBuffer(fn, mVkDevice, commands); }); return commands; } @@ -1127,13 +1134,8 @@ void Device::DestroyImpl() { // Immediately tag the recording context as unused so we don't try to submit it in Tick. mRecordingContext.needsSubmit = false; if (mRecordingContext.commandPool != VK_NULL_HANDLE) { - // The VkCommandBuffer memory should be wholly owned by the pool and freed when it is - // destroyed, but that's not the case in some drivers and the leak memory. - // So we call FreeCommandBuffers before DestroyCommandPool to be safe. - // TODO(enga): Only do this on a known list of bad drivers. - fn.FreeCommandBuffers(mVkDevice, mRecordingContext.commandPool, 1, - &mRecordingContext.commandBuffer); - fn.DestroyCommandPool(mVkDevice, mRecordingContext.commandPool, nullptr); + DestroyCommandPoolAndBuffer( + fn, mVkDevice, {mRecordingContext.commandPool, mRecordingContext.commandBuffer}); } for (VkSemaphore semaphore : mRecordingContext.waitSemaphores) { @@ -1147,12 +1149,7 @@ void Device::DestroyImpl() { ASSERT(mCommandsInFlight.Empty()); for (const CommandPoolAndBuffer& commands : mUnusedCommands) { - // The VkCommandBuffer memory should be wholly owned by the pool and freed when it is - // destroyed, but that's not the case in some drivers and the leak memory. - // So we call FreeCommandBuffers before DestroyCommandPool to be safe. - // TODO(enga): Only do this on a known list of bad drivers. - fn.FreeCommandBuffers(mVkDevice, commands.pool, 1, &commands.commandBuffer); - fn.DestroyCommandPool(mVkDevice, commands.pool, nullptr); + DestroyCommandPoolAndBuffer(fn, mVkDevice, commands); } mUnusedCommands.clear(); diff --git a/src/dawn/native/vulkan/DeviceVk.h b/src/dawn/native/vulkan/DeviceVk.h index d9c01db213..2b49a2ed4b 100644 --- a/src/dawn/native/vulkan/DeviceVk.h +++ b/src/dawn/native/vulkan/DeviceVk.h @@ -210,11 +210,6 @@ class Device final : public DeviceBase { const std::string mDebugPrefix; std::vector mDebugMessages; - struct CommandPoolAndBuffer { - VkCommandPool pool = VK_NULL_HANDLE; - VkCommandBuffer commandBuffer = VK_NULL_HANDLE; - }; - MaybeError PrepareRecordingContext(); ResultOrError BeginVkCommandBuffer(); void RecycleCompletedCommands();