From 927ac9c532084c816801434340383e4bd822c726 Mon Sep 17 00:00:00 2001 From: egdaniel Date: Mon, 19 Sep 2016 09:32:09 -0700 Subject: [PATCH] Refactor vulkan buffer mapping and unmapping A lot of this is so we don't have duplicated code in both the map/unmap and updateData functions of GrVkBuffer. Also there were slightly differences in how we handled various things in the two cases that this now unifies. Also I added a barrier after the vkUpdateBuffer call which I believe was missing. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2344323002 Review-Url: https://codereview.chromium.org/2344323002 --- src/gpu/vk/GrVkBuffer.cpp | 86 ++++++++++++++++++++++++------------------ src/gpu/vk/GrVkBuffer.h | 12 +++++- src/gpu/vk/GrVkGpu.cpp | 8 ---- src/gpu/vk/GrVkUniformBuffer.h | 2 +- 4 files changed, 60 insertions(+), 48 deletions(-) diff --git a/src/gpu/vk/GrVkBuffer.cpp b/src/gpu/vk/GrVkBuffer.cpp index a03ea0c..b8efb39 100644 --- a/src/gpu/vk/GrVkBuffer.cpp +++ b/src/gpu/vk/GrVkBuffer.cpp @@ -121,43 +121,76 @@ void GrVkBuffer::vkAbandon() { VALIDATE(); } -void* GrVkBuffer::vkMap(const GrVkGpu* gpu) { +VkAccessFlags buffer_type_to_access_flags(GrVkBuffer::Type type) { + switch (type) { + case GrVkBuffer::kIndex_Type: + return VK_ACCESS_INDEX_READ_BIT; + case GrVkBuffer::kVertex_Type: + return VK_ACCESS_VERTEX_ATTRIBUTE_READ_BIT; + default: + // This helper is only called for static buffers so we should only ever see index or + // vertex buffers types + SkASSERT(false); + return 0; + } +} + +void GrVkBuffer::internalMap(GrVkGpu* gpu, size_t size, bool* createdNewBuffer) { VALIDATE(); SkASSERT(!this->vkIsMapped()); + if (!fResource->unique()) { - // in use by the command buffer, so we need to create a new one - fResource->unref(gpu); - fResource = Create(gpu, fDesc); + if (fDesc.fDynamic) { + // in use by the command buffer, so we need to create a new one + fResource->recycle(gpu); + fResource = this->createResource(gpu, fDesc); + if (createdNewBuffer) { + *createdNewBuffer = true; + } + } else { + SkASSERT(fMapPtr); + this->addMemoryBarrier(gpu, + buffer_type_to_access_flags(fDesc.fType), + VK_ACCESS_TRANSFER_WRITE_BIT, + VK_PIPELINE_STAGE_VERTEX_INPUT_BIT, + VK_PIPELINE_STAGE_TRANSFER_BIT, + false); + } } if (fDesc.fDynamic) { const GrVkAlloc& alloc = this->alloc(); VkResult err = VK_CALL(gpu, MapMemory(gpu->device(), alloc.fMemory, alloc.fOffset + fOffset, - fDesc.fSizeInBytes, 0, &fMapPtr)); + size, 0, &fMapPtr)); if (err) { fMapPtr = nullptr; } } else { - fMapPtr = new unsigned char[this->size()]; + if (!fMapPtr) { + fMapPtr = new unsigned char[this->size()]; + } } VALIDATE(); - return fMapPtr; } -void GrVkBuffer::vkUnmap(GrVkGpu* gpu) { +void GrVkBuffer::internalUnmap(GrVkGpu* gpu, size_t size) { VALIDATE(); SkASSERT(this->vkIsMapped()); if (fDesc.fDynamic) { VK_CALL(gpu, UnmapMemory(gpu->device(), this->alloc().fMemory)); + fMapPtr = nullptr; } else { - gpu->updateBuffer(this, fMapPtr, this->offset(), this->size()); - delete [] (unsigned char*)fMapPtr; + gpu->updateBuffer(this, fMapPtr, this->offset(), size); + this->addMemoryBarrier(gpu, + VK_ACCESS_TRANSFER_WRITE_BIT, + buffer_type_to_access_flags(fDesc.fType), + VK_PIPELINE_STAGE_TRANSFER_BIT, + VK_PIPELINE_STAGE_VERTEX_INPUT_BIT, + false); } - - fMapPtr = nullptr; } bool GrVkBuffer::vkIsMapped() const { @@ -167,39 +200,18 @@ bool GrVkBuffer::vkIsMapped() const { bool GrVkBuffer::vkUpdateData(GrVkGpu* gpu, const void* src, size_t srcSizeInBytes, bool* createdNewBuffer) { - SkASSERT(!this->vkIsMapped()); - VALIDATE(); if (srcSizeInBytes > fDesc.fSizeInBytes) { return false; } - // TODO: update data based on buffer offset - if (!fDesc.fDynamic) { - return gpu->updateBuffer(this, src, fOffset, srcSizeInBytes); - } - - if (!fResource->unique()) { - // in use by the command buffer, so we need to create a new one - fResource->recycle(gpu); - fResource = this->createResource(gpu, fDesc); - if (createdNewBuffer) { - *createdNewBuffer = true; - } - } - - void* mapPtr; - const GrVkAlloc& alloc = this->alloc(); - VkResult err = VK_CALL(gpu, MapMemory(gpu->device(), alloc.fMemory, - alloc.fOffset + fOffset, - srcSizeInBytes, 0, &mapPtr)); - - if (VK_SUCCESS != err) { + this->internalMap(gpu, srcSizeInBytes, createdNewBuffer); + if (!fMapPtr) { return false; } - memcpy(mapPtr, src, srcSizeInBytes); + memcpy(fMapPtr, src, srcSizeInBytes); - VK_CALL(gpu, UnmapMemory(gpu->device(), alloc.fMemory)); + this->internalUnmap(gpu, srcSizeInBytes); return true; } diff --git a/src/gpu/vk/GrVkBuffer.h b/src/gpu/vk/GrVkBuffer.h index 473a837..e58d5e4 100644 --- a/src/gpu/vk/GrVkBuffer.h +++ b/src/gpu/vk/GrVkBuffer.h @@ -23,6 +23,7 @@ public: virtual ~GrVkBuffer() { // either release or abandon should have been called by the owner of this object. SkASSERT(!fResource); + delete [] (unsigned char*)fMapPtr; } VkBuffer buffer() const { return fResource->fBuffer; } @@ -83,8 +84,12 @@ protected: : fDesc(desc), fResource(resource), fOffset(0), fMapPtr(nullptr) { } - void* vkMap(const GrVkGpu* gpu); - void vkUnmap(GrVkGpu* gpu); + void* vkMap(GrVkGpu* gpu) { + this->internalMap(gpu, fDesc.fSizeInBytes); + return fMapPtr; + } + void vkUnmap(GrVkGpu* gpu) { this->internalUnmap(gpu, this->size()); } + // If the caller passes in a non null createdNewBuffer, this function will set the bool to true // if it creates a new VkBuffer to upload the data to. bool vkUpdateData(GrVkGpu* gpu, const void* src, size_t srcSizeInBytes, @@ -99,6 +104,9 @@ private: return Create(gpu, descriptor); } + void internalMap(GrVkGpu* gpu, size_t size, bool* createdNewBuffer = nullptr); + void internalUnmap(GrVkGpu* gpu, size_t size); + void validate() const; bool vkIsMapped() const; diff --git a/src/gpu/vk/GrVkGpu.cpp b/src/gpu/vk/GrVkGpu.cpp index a339377..82e7394 100644 --- a/src/gpu/vk/GrVkGpu.cpp +++ b/src/gpu/vk/GrVkGpu.cpp @@ -601,14 +601,6 @@ bool GrVkGpu::uploadTexDataOptimal(GrVkTexture* tex, transferBuffer->unmap(); - // make sure the unmap has finished - transferBuffer->addMemoryBarrier(this, - VK_ACCESS_HOST_WRITE_BIT, - VK_ACCESS_TRANSFER_READ_BIT, - VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, - VK_PIPELINE_STAGE_TRANSFER_BIT, - false); - // Change layout of our target so it can be copied to tex->setImageLayout(this, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, diff --git a/src/gpu/vk/GrVkUniformBuffer.h b/src/gpu/vk/GrVkUniformBuffer.h index cfde125..2535e0c 100644 --- a/src/gpu/vk/GrVkUniformBuffer.h +++ b/src/gpu/vk/GrVkUniformBuffer.h @@ -19,7 +19,7 @@ public: static const GrVkResource* CreateResource(GrVkGpu* gpu, size_t size); static const size_t kStandardSize = 256; - void* map(const GrVkGpu* gpu) { + void* map(GrVkGpu* gpu) { return this->vkMap(gpu); } void unmap(GrVkGpu* gpu) { -- 2.7.4