Skip to content

Commit

Permalink
vulkan: implicitly free command buffers (google#7167)
Browse files Browse the repository at this point in the history 
We were calling vkFreeCommandBuffers directly, but resetting
the buffers implicitly (when vkBeginCommandBuffer is called)
seems to be a lot more performant.

Also, cleaned up destructor for VkBuffer to no longer require
a separate terminate() method.
  • Loading branch information
@poweifeng @plepers
poweifeng authored and plepers committed Dec 9, 2023
1 parent d421996 commit 00bcfe8
Show file tree
Hide file tree
Showing 14 changed files with 76 additions and 84 deletions.
10 changes: 4 additions & 6 deletions filament/backend/src/vulkan/VulkanBlitter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ void VulkanBlitter::blitColor(BlitArgs args) {
}
#endif
VulkanCommandBuffer& commands = mCommands->get();
VkCommandBuffer const cmdbuffer = commands.cmdbuffer;
VkCommandBuffer const cmdbuffer = commands.buffer();
commands.acquire(src.texture);
commands.acquire(dst.texture);

Expand Down Expand Up @@ -184,7 +184,7 @@ void VulkanBlitter::blitDepth(BlitArgs args) {
}

VulkanCommandBuffer& commands = mCommands->get();
VkCommandBuffer const cmdbuffer = commands.cmdbuffer;
VkCommandBuffer const cmdbuffer = commands.buffer();
commands.acquire(src.texture);
commands.acquire(dst.texture);
blitFast(cmdbuffer, aspect, args.filter, args.srcTarget->getExtent(), src, dst, args.srcRectPair,
Expand All @@ -197,13 +197,11 @@ void VulkanBlitter::terminate() noexcept {
mDepthResolveProgram = nullptr;

if (mTriangleBuffer) {
mTriangleBuffer->terminate();
delete mTriangleBuffer;
mTriangleBuffer = nullptr;
}

if (mParamsBuffer) {
mParamsBuffer->terminate();
delete mParamsBuffer;
mParamsBuffer = nullptr;
}
Expand Down Expand Up @@ -257,7 +255,7 @@ void VulkanBlitter::lazyInit() noexcept {
};

VulkanCommandBuffer& commands = mCommands->get();
VkCommandBuffer const cmdbuffer = commands.cmdbuffer;
VkCommandBuffer const cmdbuffer = commands.buffer();

mTriangleBuffer = new VulkanBuffer(mAllocator, mStagePool, VK_BUFFER_USAGE_VERTEX_BUFFER_BIT,
sizeof(kTriangleVertices));
Expand All @@ -278,7 +276,7 @@ void VulkanBlitter::blitSlowDepth(VkFilter filter, const VkExtent2D srcExtent, V
lazyInit();

VulkanCommandBuffer* commands = &mCommands->get();
VkCommandBuffer const cmdbuffer = commands->cmdbuffer;
VkCommandBuffer const cmdbuffer = commands->buffer();
commands->acquire(src.texture);
commands->acquire(dst.texture);

Expand Down
7 changes: 0 additions & 7 deletions filament/backend/src/vulkan/VulkanBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,7 @@ VulkanBuffer::VulkanBuffer(VmaAllocator allocator, VulkanStagePool& stagePool,
}

VulkanBuffer::~VulkanBuffer() {
assert_invariant(mGpuMemory == VK_NULL_HANDLE);
assert_invariant(mGpuBuffer == VK_NULL_HANDLE);
}

void VulkanBuffer::terminate() {
vmaDestroyBuffer(mAllocator, mGpuBuffer, mGpuMemory);
mGpuMemory = VK_NULL_HANDLE;
mGpuBuffer = VK_NULL_HANDLE;
}

void VulkanBuffer::loadFromCpu(VkCommandBuffer cmdbuf, const void* cpuData, uint32_t byteOffset,
Expand Down
1 change: 0 additions & 1 deletion filament/backend/src/vulkan/VulkanBuffer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class VulkanBuffer {
VulkanBuffer(VmaAllocator allocator, VulkanStagePool& stagePool, VkBufferUsageFlags usage,
uint32_t numBytes);
~VulkanBuffer();
void terminate();
void loadFromCpu(VkCommandBuffer cmdbuf, const void* cpuData, uint32_t byteOffset,
uint32_t numBytes) const;
VkBuffer getGpuBuffer() const {
Expand Down
68 changes: 35 additions & 33 deletions filament/backend/src/vulkan/VulkanCommands.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,23 @@ VulkanCmdFence::~VulkanCmdFence() {
vkDestroyFence(device, fence, VKALLOC);
}

VulkanCommandBuffer::VulkanCommandBuffer(VulkanResourceAllocator* allocator, VkDevice device,
VkCommandPool pool)
: mResourceManager(allocator) {
// Create the low-level command buffer.
const VkCommandBufferAllocateInfo allocateInfo{
.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO,
.commandPool = pool,
.level = VK_COMMAND_BUFFER_LEVEL_PRIMARY,
.commandBufferCount = 1,
};

// The buffer allocated here will be implicitly reset when vkBeginCommandBuffer is called.
vkAllocateCommandBuffers(device, &allocateInfo, &mBuffer);

// We don't need to deallocate since destroying the pool will free all of the buffers.
}

CommandBufferObserver::~CommandBufferObserver() {}

static VkCommandPool createPool(VkDevice device, uint32_t queueFamilyIndex) {
Expand Down Expand Up @@ -130,7 +147,7 @@ VulkanCommands::VulkanCommands(VkDevice device, VkQueue queue, uint32_t queueFam
}

for (size_t i = 0; i < CAPACITY; ++i) {
mStorage[i] = std::make_unique<VulkanCommandBuffer>(allocator);
mStorage[i] = std::make_unique<VulkanCommandBuffer>(allocator, mDevice, mPool);
}
}

Expand All @@ -151,7 +168,7 @@ VulkanCommandBuffer& VulkanCommands::get() {
// If we ran out of available command buffers, stall until one finishes. This is very rare.
// It occurs only when Filament invokes commit() or endFrame() a large number of times without
// presenting the swap chain or waiting on a fence.
while (mAvailableCount == 0) {
while (mAvailableBufferCount == 0) {
#if VK_REPORT_STALLS
slog.i << "VulkanCommands has stalled. "
<< "If this occurs frequently, consider increasing VK_MAX_COMMAND_BUFFERS."
Expand All @@ -165,24 +182,15 @@ VulkanCommandBuffer& VulkanCommands::get() {
// Find an available slot.
for (size_t i = 0; i < CAPACITY; ++i) {
auto wrapper = mStorage[i].get();
if (wrapper->cmdbuffer == VK_NULL_HANDLE) {
if (wrapper->buffer() == VK_NULL_HANDLE) {
mCurrentCommandBufferIndex = static_cast<int8_t>(i);
currentbuf = wrapper;
break;
}
}

assert_invariant(currentbuf);
--mAvailableCount;

// Create the low-level command buffer.
const VkCommandBufferAllocateInfo allocateInfo {
.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO,
.commandPool = mPool,
.level = VK_COMMAND_BUFFER_LEVEL_PRIMARY,
.commandBufferCount = 1
};
vkAllocateCommandBuffers(mDevice, &allocateInfo, &currentbuf->cmdbuffer);
mAvailableBufferCount--;

// Note that the fence wrapper uses shared_ptr because a DriverAPI fence can also have ownership
// over it. The destruction of the low-level fence occurs either in VulkanCommands::gc(), or in
Expand All @@ -194,7 +202,7 @@ VulkanCommandBuffer& VulkanCommands::get() {
.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO,
.flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT,
};
vkBeginCommandBuffer(currentbuf->cmdbuffer, &binfo);
vkBeginCommandBuffer(currentbuf->buffer(), &binfo);

// Notify the observer that a new command buffer has been activated.
if (mObserver) {
Expand Down Expand Up @@ -235,7 +243,7 @@ bool VulkanCommands::flush() {
VulkanCommandBuffer const* currentbuf = mStorage[index].get();
VkSemaphore const renderingFinished = mSubmissionSignals[index];

vkEndCommandBuffer(currentbuf->cmdbuffer);
vkEndCommandBuffer(currentbuf->buffer());

// If the injected semaphore is an "image available" semaphore that has not yet been signaled,
// it is sometimes fine to start executing commands anyway, as along as we stall the GPU at the
Expand All @@ -253,13 +261,15 @@ bool VulkanCommands::flush() {
VK_NULL_HANDLE,
};

VkCommandBuffer const cmdbuffer = currentbuf->buffer();

VkSubmitInfo submitInfo {
.sType = VK_STRUCTURE_TYPE_SUBMIT_INFO,
.waitSemaphoreCount = 0,
.pWaitSemaphores = signals,
.pWaitDstStageMask = waitDestStageMasks,
.commandBufferCount = 1,
.pCommandBuffers = &currentbuf->cmdbuffer,
.pCommandBuffers = &cmdbuffer,
.signalSemaphoreCount = 1u,
.pSignalSemaphores = &renderingFinished,
};
Expand All @@ -278,7 +288,7 @@ bool VulkanCommands::flush() {
}

#if FILAMENT_VULKAN_VERBOSE
slog.i << "Submitting cmdbuffer=" << currentbuf->cmdbuffer
slog.i << "Submitting cmdbuffer=" << cmdbuffer
<< " wait=(" << signals[0] << ", " << signals[1] << ") "
<< " signal=" << renderingFinished
<< io::endl;
Expand Down Expand Up @@ -326,7 +336,7 @@ void VulkanCommands::wait() {
size_t count = 0;
for (size_t i = 0; i < CAPACITY; i++) {
auto wrapper = mStorage[i].get();
if (wrapper->cmdbuffer != VK_NULL_HANDLE
if (wrapper->buffer() != VK_NULL_HANDLE
&& mCurrentCommandBufferIndex != static_cast<int8_t>(i)) {
fences[count++] = wrapper->fence->fence;
}
Expand All @@ -337,33 +347,25 @@ void VulkanCommands::wait() {
}

void VulkanCommands::gc() {
VkCommandBuffer buffers[CAPACITY];
size_t count = 0;
for (size_t i = 0; i < CAPACITY; i++) {
auto wrapper = mStorage[i].get();
if (wrapper->cmdbuffer == VK_NULL_HANDLE) {
if (wrapper->buffer() == VK_NULL_HANDLE) {
continue;
}
VkResult const result = vkWaitForFences(mDevice, 1, &wrapper->fence->fence, VK_TRUE, 0);
if (result != VK_SUCCESS) {
continue;
}
buffers[count++] = wrapper->cmdbuffer;
wrapper->cmdbuffer = VK_NULL_HANDLE;
wrapper->fence->status.store(VK_SUCCESS);
wrapper->fence.reset();
wrapper->clearResources();
++mAvailableCount;
}
if (count > 0) {
vkFreeCommandBuffers(mDevice, mPool, count, buffers);
wrapper->reset();
mAvailableBufferCount++;
}
}

void VulkanCommands::updateFences() {
for (size_t i = 0; i < CAPACITY; i++) {
auto wrapper = mStorage[i].get();
if (wrapper->cmdbuffer != VK_NULL_HANDLE) {
if (wrapper->buffer() != VK_NULL_HANDLE) {
VulkanCmdFence* fence = wrapper->fence.get();
if (fence) {
VkResult status = vkGetFenceStatus(mDevice, fence->fence);
Expand All @@ -384,7 +386,7 @@ void VulkanCommands::pushGroupMarker(char const* str, VulkanGroupMarkers::Timest
#endif

// TODO: Add group marker color to the Driver API
const VkCommandBuffer cmdbuffer = get().cmdbuffer;
VkCommandBuffer const cmdbuffer = get().buffer();

if (!mGroupMarkers) {
mGroupMarkers = std::make_unique<VulkanGroupMarkers>();
Expand Down Expand Up @@ -412,7 +414,7 @@ void VulkanCommands::popGroupMarker() {
assert_invariant(mGroupMarkers);

if (!mGroupMarkers->empty()) {
const VkCommandBuffer cmdbuffer = get().cmdbuffer;
VkCommandBuffer const cmdbuffer = get().buffer();
#if FILAMENT_VULKAN_VERBOSE
auto const [marker, startTime] = mGroupMarkers->pop();
auto const endTime = std::chrono::high_resolution_clock::now();
Expand All @@ -437,7 +439,7 @@ void VulkanCommands::popGroupMarker() {
}

void VulkanCommands::insertEventMarker(char const* string, uint32_t len) {
VkCommandBuffer const cmdbuffer = get().cmdbuffer;
VkCommandBuffer const cmdbuffer = get().buffer();
if (mContext->isDebugUtilsSupported()) {
VkDebugUtilsLabelEXT labelInfo = {
.sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT,
Expand Down
20 changes: 14 additions & 6 deletions filament/backend/src/vulkan/VulkanCommands.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,10 @@ struct VulkanCmdFence {
// DriverApi fence object and should not be destroyed until both the DriverApi object is freed and
// we're done waiting on the most recent submission of the given command buffer.
struct VulkanCommandBuffer {
VulkanCommandBuffer(VulkanResourceAllocator* allocator)
: mResourceManager(allocator) {}
VulkanCommandBuffer(VulkanResourceAllocator* allocator, VkDevice device, VkCommandPool pool);

VulkanCommandBuffer(VulkanCommandBuffer const&) = delete;
VulkanCommandBuffer& operator=(VulkanCommandBuffer const&) = delete;
VkCommandBuffer cmdbuffer = VK_NULL_HANDLE;
std::shared_ptr<VulkanCmdFence> fence;

inline void acquire(VulkanResource* resource) {
mResourceManager.acquire(resource);
Expand All @@ -87,12 +84,23 @@ struct VulkanCommandBuffer {
mResourceManager.acquire(srcResources);
}

inline void clearResources() {
inline void reset() {
fence.reset();
mResourceManager.clear();
}

inline VkCommandBuffer buffer() const {
if (fence) {
return mBuffer;
}
return VK_NULL_HANDLE;
}

std::shared_ptr<VulkanCmdFence> fence;

private:
VulkanAcquireOnlyResourceManager mResourceManager;
VkCommandBuffer mBuffer;
};

// Allows classes to be notified after a new command buffer has been activated.
Expand Down Expand Up @@ -185,7 +193,7 @@ class VulkanCommands {
VkSemaphore mInjectedSignal = {};
utils::FixedCapacityVector<std::unique_ptr<VulkanCommandBuffer>> mStorage;
VkSemaphore mSubmissionSignals[CAPACITY] = {};
size_t mAvailableCount = CAPACITY;
uint8_t mAvailableBufferCount = CAPACITY;
CommandBufferObserver* mObserver = nullptr;

std::unique_ptr<VulkanGroupMarkers> mGroupMarkers;
Expand Down
7 changes: 4 additions & 3 deletions filament/backend/src/vulkan/VulkanContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,9 @@ void VulkanTimestamps::beginQuery(VulkanCommandBuffer const* commands,
VulkanTimerQuery* query) {
uint32_t const index = query->getStartingQueryIndex();

vkCmdResetQueryPool(commands->cmdbuffer, mPool, index, 2);
vkCmdWriteTimestamp(commands->cmdbuffer, VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT, mPool, index);
auto const cmdbuffer = commands->buffer();
vkCmdResetQueryPool(cmdbuffer, mPool, index, 2);
vkCmdWriteTimestamp(cmdbuffer, VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT, mPool, index);

// We stash this because getResult might come before the query is actually processed.
query->setFence(commands->fence);
Expand All @@ -127,7 +128,7 @@ void VulkanTimestamps::beginQuery(VulkanCommandBuffer const* commands,
void VulkanTimestamps::endQuery(VulkanCommandBuffer const* commands,
VulkanTimerQuery const* query) {
uint32_t const index = query->getStoppingQueryIndex();
vkCmdWriteTimestamp(commands->cmdbuffer, VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT, mPool, index);
vkCmdWriteTimestamp(commands->buffer(), VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT, mPool, index);
}

VulkanTimestamps::QueryResult VulkanTimestamps::getResult(VulkanTimerQuery const* query) {
Expand Down
14 changes: 7 additions & 7 deletions filament/backend/src/vulkan/VulkanDriver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -873,7 +873,7 @@ void VulkanDriver::updateIndexBuffer(Handle<HwIndexBuffer> ibh, BufferDescriptor
VulkanCommandBuffer& commands = mCommands->get();
auto ib = mResourceAllocator.handle_cast<VulkanIndexBuffer*>(ibh);
commands.acquire(ib);
ib->buffer.loadFromCpu(commands.cmdbuffer, p.buffer, byteOffset, p.size);
ib->buffer.loadFromCpu(commands.buffer(), p.buffer, byteOffset, p.size);

scheduleDestroy(std::move(p));
}
Expand All @@ -884,7 +884,7 @@ void VulkanDriver::updateBufferObject(Handle<HwBufferObject> boh, BufferDescript

auto bo = mResourceAllocator.handle_cast<VulkanBufferObject*>(boh);
commands.acquire(bo);
bo->buffer.loadFromCpu(commands.cmdbuffer, bd.buffer, byteOffset, bd.size);
bo->buffer.loadFromCpu(commands.buffer(), bd.buffer, byteOffset, bd.size);

scheduleDestroy(std::move(bd));
}
Expand All @@ -895,7 +895,7 @@ void VulkanDriver::updateBufferObjectUnsynchronized(Handle<HwBufferObject> boh,
auto bo = mResourceAllocator.handle_cast<VulkanBufferObject*>(boh);
commands.acquire(bo);
// TODO: implement unsynchronized version
bo->buffer.loadFromCpu(commands.cmdbuffer, bd.buffer, byteOffset, bd.size);
bo->buffer.loadFromCpu(commands.buffer(), bd.buffer, byteOffset, bd.size);
mResourceManager.acquire(bo);
scheduleDestroy(std::move(bd));
}
Expand Down Expand Up @@ -1021,7 +1021,7 @@ void VulkanDriver::beginRenderPass(Handle<HwRenderTarget> rth, const RenderPassP
// the non-sampling case.
bool samplingDepthAttachment = false;
VulkanCommandBuffer& commands = mCommands->get();
VkCommandBuffer const cmdbuffer = commands.cmdbuffer;
VkCommandBuffer const cmdbuffer = commands.buffer();

UTILS_NOUNROLL
for (uint8_t samplerGroupIdx = 0; samplerGroupIdx < Program::SAMPLER_BINDING_COUNT;
Expand Down Expand Up @@ -1247,7 +1247,7 @@ void VulkanDriver::beginRenderPass(Handle<HwRenderTarget> rth, const RenderPassP

void VulkanDriver::endRenderPass(int) {
VulkanCommandBuffer& commands = mCommands->get();
VkCommandBuffer cmdbuffer = commands.cmdbuffer;
VkCommandBuffer cmdbuffer = commands.buffer();
vkCmdEndRenderPass(cmdbuffer);

VulkanRenderTarget* rt = mCurrentRenderPass.renderTarget;
Expand Down Expand Up @@ -1299,7 +1299,7 @@ void VulkanDriver::nextSubpass(int) {
assert_invariant(renderTarget);
assert_invariant(mCurrentRenderPass.params.subpassMask);

vkCmdNextSubpass(mCommands->get().cmdbuffer, VK_SUBPASS_CONTENTS_INLINE);
vkCmdNextSubpass(mCommands->get().buffer(), VK_SUBPASS_CONTENTS_INLINE);

mPipelineCache.bindRenderPass(mCurrentRenderPass.renderPass,
++mCurrentRenderPass.currentSubpass);
Expand Down Expand Up @@ -1484,7 +1484,7 @@ void VulkanDriver::blit(TargetBufferFlags buffers, Handle<HwRenderTarget> dst, V
void VulkanDriver::draw(PipelineState pipelineState, Handle<HwRenderPrimitive> rph,
const uint32_t instanceCount) {
VulkanCommandBuffer* commands = &mCommands->get();
VkCommandBuffer cmdbuffer = commands->cmdbuffer;
VkCommandBuffer cmdbuffer = commands->buffer();
const VulkanRenderPrimitive& prim = *mResourceAllocator.handle_cast<VulkanRenderPrimitive*>(rph);

Handle<HwProgram> programHandle = pipelineState.program;
Expand Down
Loading

0 comments on commit 00bcfe8

Please sign in to comment.