From a31ab4ba9b3fea5bb4e16ae1a9d5a2e7e905ac6d Mon Sep 17 00:00:00 2001 From: Greg Daniel Date: Wed, 11 Mar 2020 13:59:38 -0400 Subject: [PATCH] Have GrVkUniformBuffers own their descriptor sets. Locally this looks to have a perf win especially on Android. We no longer have to update the values in the uniform descriptor every draw. Also since we essentially have one uniform buffer per draw already, we were making a descriptor set per draw as well. Now we just tie them to the uniform buffers and we should end up with about the same amount. Change-Id: I65c6a8b2ad5c631c4dbb7ea50068f71a0ed4c894 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/276476 Reviewed-by: Jim Van Verth Commit-Queue: Greg Daniel --- src/gpu/vk/GrVkBuffer.h | 3 +- src/gpu/vk/GrVkPipelineState.cpp | 45 ++------------------------- src/gpu/vk/GrVkPipelineState.h | 4 --- src/gpu/vk/GrVkResourceProvider.cpp | 16 +++++----- src/gpu/vk/GrVkUniformBuffer.cpp | 47 +++++++++++++++++++++++++++-- src/gpu/vk/GrVkUniformBuffer.h | 18 +++++++++-- 6 files changed, 73 insertions(+), 60 deletions(-) diff --git a/src/gpu/vk/GrVkBuffer.h b/src/gpu/vk/GrVkBuffer.h index 89f97087df..110bd41811 100644 --- a/src/gpu/vk/GrVkBuffer.h +++ b/src/gpu/vk/GrVkBuffer.h @@ -68,9 +68,10 @@ protected: GrVkAlloc fAlloc; Type fType; - private: + protected: void freeGPUData(GrGpu* gpu) const override; + private: void onRecycle(GrGpu* gpu) const override { this->unref(gpu); } typedef GrRecycledResource INHERITED; diff --git a/src/gpu/vk/GrVkPipelineState.cpp b/src/gpu/vk/GrVkPipelineState.cpp index b4797ba511..8cf121896a 100644 --- a/src/gpu/vk/GrVkPipelineState.cpp +++ b/src/gpu/vk/GrVkPipelineState.cpp @@ -39,7 +39,6 @@ GrVkPipelineState::GrVkPipelineState( std::unique_ptr[]> fragmentProcessors, int fragmentProcessorCnt) : fPipeline(pipeline) - , fUniformDescriptorSet(nullptr) , fSamplerDSHandle(samplerDSHandle) , fBuiltinUniformHandles(builtinUniformHandles) , fGeometryProcessor(std::move(geometryProcessor)) @@ -73,11 +72,6 @@ void GrVkPipelineState::freeGPUResources(GrVkGpu* gpu) { fUniformBuffer->release(gpu); fUniformBuffer.reset(); } - - if (fUniformDescriptorSet) { - fUniformDescriptorSet->recycle(const_cast(gpu)); - fUniformDescriptorSet = nullptr; - } } bool GrVkPipelineState::setAndBindUniforms(GrVkGpu* gpu, @@ -105,22 +99,10 @@ bool GrVkPipelineState::setAndBindUniforms(GrVkGpu* gpu, // Get new descriptor set if (fUniformBuffer) { - if (fDataManager.uploadUniformBuffers(gpu, fUniformBuffer.get()) || - !fUniformDescriptorSet) { - if (fUniformDescriptorSet) { - fUniformDescriptorSet->recycle(gpu); - } - fUniformDescriptorSet = gpu->resourceProvider().getUniformDescriptorSet(); - if (!fUniformDescriptorSet) { - return false; - } - this->writeUniformBuffers(gpu); - } + fDataManager.uploadUniformBuffers(gpu, fUniformBuffer.get()); static const int kUniformDSIdx = GrVkUniformHandler::kUniformBufferDescSet; commandBuffer->bindDescriptorSets(gpu, this, fPipeline->layout(), kUniformDSIdx, 1, - fUniformDescriptorSet->descriptorSet(), 0, nullptr); - SkASSERT(fUniformDescriptorSet); - commandBuffer->addRecycledResource(fUniformDescriptorSet); + fUniformBuffer->descriptorSet(), 0, nullptr); commandBuffer->addRecycledResource(fUniformBuffer->resource()); } return true; @@ -267,29 +249,6 @@ void set_uniform_descriptor_writes(VkWriteDescriptorSet* descriptorWrite, descriptorWrite->pTexelBufferView = nullptr; } -void GrVkPipelineState::writeUniformBuffers(const GrVkGpu* gpu) { - VkWriteDescriptorSet descriptorWrites[3]; - VkDescriptorBufferInfo bufferInfos[3]; - - uint32_t writeCount = 0; - - if (fUniformBuffer.get()) { - SkASSERT(fUniformDescriptorSet); - set_uniform_descriptor_writes(&descriptorWrites[writeCount], - &bufferInfos[writeCount], - fUniformBuffer.get(), - *fUniformDescriptorSet->descriptorSet()); - ++writeCount; - } - - if (writeCount) { - GR_VK_CALL(gpu->vkInterface(), UpdateDescriptorSets(gpu->device(), - writeCount, - descriptorWrites, - 0, nullptr)); - } -} - void GrVkPipelineState::setRenderTargetState(const GrRenderTarget* rt, GrSurfaceOrigin origin) { // Load the RT height uniform if it is needed to y-flip gl_FragCoord. diff --git a/src/gpu/vk/GrVkPipelineState.h b/src/gpu/vk/GrVkPipelineState.h index 8862305930..04aaee8542 100644 --- a/src/gpu/vk/GrVkPipelineState.h +++ b/src/gpu/vk/GrVkPipelineState.h @@ -69,8 +69,6 @@ public: void freeGPUResources(GrVkGpu* gpu); private: - void writeUniformBuffers(const GrVkGpu* gpu); - /** * We use the RT's size and origin to adjust from Skia device space to vulkan normalized device * space and to make device space positions have the correct origin for processors that require @@ -113,8 +111,6 @@ private: // GrManagedResources GrVkPipeline* fPipeline; - const GrVkDescriptorSet* fUniformDescriptorSet; - const GrVkDescriptorSetManager::Handle fSamplerDSHandle; SkSTArray<4, const GrVkSampler*> fImmutableSamplers; diff --git a/src/gpu/vk/GrVkResourceProvider.cpp b/src/gpu/vk/GrVkResourceProvider.cpp index 198a94611e..2e4c886e5b 100644 --- a/src/gpu/vk/GrVkResourceProvider.cpp +++ b/src/gpu/vk/GrVkResourceProvider.cpp @@ -418,19 +418,21 @@ void GrVkResourceProvider::destroyResources(bool deviceLost) { } fAvailableCommandPools.reset(); - // We must release/destroy all command buffers and pipeline states before releasing the - // GrVkDescriptorSetManagers - for (int i = 0; i < fDescriptorSetManagers.count(); ++i) { - fDescriptorSetManagers[i]->release(fGpu); - } - fDescriptorSetManagers.reset(); - // release our uniform buffers for (int i = 0; i < fAvailableUniformBufferResources.count(); ++i) { SkASSERT(fAvailableUniformBufferResources[i]->unique()); fAvailableUniformBufferResources[i]->unref(fGpu); } fAvailableUniformBufferResources.reset(); + + // We must release/destroy all command buffers and pipeline states before releasing the + // GrVkDescriptorSetManagers. Additionally, we must release all uniform buffers since they hold + // refs to GrVkDescriptorSets. + for (int i = 0; i < fDescriptorSetManagers.count(); ++i) { + fDescriptorSetManagers[i]->release(fGpu); + } + fDescriptorSetManagers.reset(); + } void GrVkResourceProvider::backgroundReset(GrVkCommandPool* pool) { diff --git a/src/gpu/vk/GrVkUniformBuffer.cpp b/src/gpu/vk/GrVkUniformBuffer.cpp index 2626fa9efb..bbb770d031 100644 --- a/src/gpu/vk/GrVkUniformBuffer.cpp +++ b/src/gpu/vk/GrVkUniformBuffer.cpp @@ -5,9 +5,11 @@ * found in the LICENSE file. */ -#include "src/gpu/vk/GrVkGpu.h" #include "src/gpu/vk/GrVkUniformBuffer.h" +#include "src/gpu/vk/GrVkDescriptorSet.h" +#include "src/gpu/vk/GrVkGpu.h" + #define VK_CALL(GPU, X) GR_VK_CALL(GPU->vkInterface(), X) GrVkUniformBuffer* GrVkUniformBuffer::Create(GrVkGpu* gpu, size_t size) { @@ -69,16 +71,43 @@ const GrManagedResource* GrVkUniformBuffer::CreateResource(GrVkGpu* gpu, size_t kUniform_Type, true, // dynamic &alloc)) { + VK_CALL(gpu, DestroyBuffer(gpu->device(), buffer, nullptr)); return nullptr; } - const GrManagedResource* resource = new GrVkUniformBuffer::Resource(buffer, alloc); - if (!resource) { + const GrVkDescriptorSet* descriptorSet = gpu->resourceProvider().getUniformDescriptorSet(); + if (!descriptorSet) { VK_CALL(gpu, DestroyBuffer(gpu->device(), buffer, nullptr)); GrVkMemory::FreeBufferMemory(gpu, kUniform_Type, alloc); return nullptr; } + VkDescriptorBufferInfo bufferInfo; + memset(&bufferInfo, 0, sizeof(VkDescriptorBufferInfo)); + bufferInfo.buffer = buffer; + bufferInfo.offset = 0; + bufferInfo.range = size; + + VkWriteDescriptorSet descriptorWrite; + memset(&descriptorWrite, 0, sizeof(VkWriteDescriptorSet)); + descriptorWrite.sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET; + descriptorWrite.pNext = nullptr; + descriptorWrite.dstSet = *descriptorSet->descriptorSet(); + descriptorWrite.dstBinding = GrVkUniformHandler::kUniformBinding; + descriptorWrite.dstArrayElement = 0; + descriptorWrite.descriptorCount = 1; + descriptorWrite.descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER; + descriptorWrite.pImageInfo = nullptr; + descriptorWrite.pBufferInfo = &bufferInfo; + descriptorWrite.pTexelBufferView = nullptr; + + GR_VK_CALL(gpu->vkInterface(), UpdateDescriptorSets(gpu->device(), + 1, + &descriptorWrite, + 0, nullptr)); + + const GrManagedResource* resource = new GrVkUniformBuffer::Resource(buffer, alloc, + descriptorSet); return resource; } @@ -102,3 +131,15 @@ void GrVkUniformBuffer::Resource::onRecycle(GrGpu* gpu) const { this->unref(gpu); } } + +void GrVkUniformBuffer::Resource::freeGPUData(GrGpu* gpu) const { + if (fDescriptorSet) { + fDescriptorSet->recycle(gpu); + } + INHERITED::freeGPUData(gpu); +} + +const VkDescriptorSet* GrVkUniformBuffer::Resource::descriptorSet() const { + return fDescriptorSet->descriptorSet(); +} + diff --git a/src/gpu/vk/GrVkUniformBuffer.h b/src/gpu/vk/GrVkUniformBuffer.h index db9350ae53..d50e3deab5 100644 --- a/src/gpu/vk/GrVkUniformBuffer.h +++ b/src/gpu/vk/GrVkUniformBuffer.h @@ -11,6 +11,7 @@ #include "include/gpu/vk/GrVkTypes.h" #include "src/gpu/vk/GrVkBuffer.h" +class GrVkDescriptorSet; class GrVkGpu; class GrVkUniformBuffer : public GrVkBuffer { @@ -34,15 +35,28 @@ public: } void release(const GrVkGpu* gpu) { this->vkRelease(gpu); } + const VkDescriptorSet* descriptorSet() const { + const Resource* resource = static_cast(this->resource()); + return resource->descriptorSet(); + } + private: class Resource : public GrVkBuffer::Resource { public: - Resource(VkBuffer buf, const GrVkAlloc& alloc) - : INHERITED(buf, alloc, kUniform_Type) {} + Resource(VkBuffer buf, const GrVkAlloc& alloc, const GrVkDescriptorSet* descSet) + : INHERITED(buf, alloc, kUniform_Type) + , fDescriptorSet(descSet) {} + + void freeGPUData(GrGpu* gpu) const override; void onRecycle(GrGpu* gpu) const override; + const VkDescriptorSet* descriptorSet() const; + typedef GrVkBuffer::Resource INHERITED; + + private: + const GrVkDescriptorSet* fDescriptorSet; }; const GrVkBuffer::Resource* createResource(GrVkGpu* gpu,