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 <enga@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Loko Kung <lokokung@google.com>
diff --git a/src/dawn/native/vulkan/CommandRecordingContext.h b/src/dawn/native/vulkan/CommandRecordingContext.h
index 7948e3b..7bd762f 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 fed1cb9..63c94a2 100644
--- a/src/dawn/native/vulkan/DeviceVk.cpp
+++ b/src/dawn/native/vulkan/DeviceVk.cpp
@@ -69,6 +69,23 @@
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 @@
return {};
}
-ResultOrError<Device::CommandPoolAndBuffer> Device::BeginVkCommandBuffer() {
+ResultOrError<CommandPoolAndBuffer> Device::BeginVkCommandBuffer() {
CommandPoolAndBuffer commands;
// First try to recycle unused command pools.
@@ -754,19 +771,7 @@
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 @@
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 @@
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 @@
// 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 @@
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 d9c01db..2b49a2e 100644
--- a/src/dawn/native/vulkan/DeviceVk.h
+++ b/src/dawn/native/vulkan/DeviceVk.h
@@ -210,11 +210,6 @@
const std::string mDebugPrefix;
std::vector<std::string> mDebugMessages;
- struct CommandPoolAndBuffer {
- VkCommandPool pool = VK_NULL_HANDLE;
- VkCommandBuffer commandBuffer = VK_NULL_HANDLE;
- };
-
MaybeError PrepareRecordingContext();
ResultOrError<CommandPoolAndBuffer> BeginVkCommandBuffer();
void RecycleCompletedCommands();
