Refactor vulkan buffer mapping and unmapping

src/gpu/vk/GrVkBuffer.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "GrVkBuffer.h"
 #include "GrVkGpu.h"
 #include "GrVkMemory.h"
@@ skipping 103 matching lines @@
     VALIDATE();
 }
 
 void GrVkBuffer::vkAbandon() {
     fResource->unrefAndAbandon();
     fResource = nullptr;
     fMapPtr = nullptr;
     VALIDATE();
 }
 
-void* GrVkBuffer::vkMap(const GrVkGpu* gpu) {
+void* GrVkBuffer::vkMap(GrVkGpu* gpu) {
+    this->innerMap(gpu, fDesc.fSizeInBytes);
+    return fMapPtr;
+}
+
+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::innerMap(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,

[jvanverth1, 2016/09/19 14:44:11]

I believe we add barriers externally in some cases when we map/unmap, so you
might want to look for that and remove them.

+                                   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) {
+    this->innerUnmap(gpu, this->size());
+}
+
+void GrVkBuffer::innerUnmap(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 {
     VALIDATE();
     return SkToBool(fMapPtr);
 }
 
 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->innerMap(gpu, srcSizeInBytes, createdNewBuffer);

[jvanverth1, 2016/09/19 14:44:11]

I don't think that two new methods are needed, since vkMap and vkUnmap
essentially wrap them. You could just call vkMap here and vkUnmap below.

[egdaniel, 2016/09/19 15:17:24]

The main difference here is the size parameter is different for vkMap/Unmap vs
the one called from updateData

[jvanverth1, 2016/09/19 15:37:31]

Ah, I missed that. How about calling them internalMap/internalUnmap (fits naming
elsewhere in the code), and inlining them in the header for vkMap and vkUnmap?

[egdaniel, 2016/09/19 16:01:42]

Done.

+    if (!fMapPtr) {
         return false;
     }
 
-    memcpy(mapPtr, src, srcSizeInBytes);
+    memcpy(fMapPtr, src, srcSizeInBytes);
 
-    VK_CALL(gpu, UnmapMemory(gpu->device(), alloc.fMemory));
+    this->innerUnmap(gpu, srcSizeInBytes);
 
     return true;
 }
 
 void GrVkBuffer::validate() const {
     SkASSERT(!fResource || kVertex_Type == fDesc.fType || kIndex_Type == fDesc.fType
              || kCopyRead_Type == fDesc.fType || kCopyWrite_Type == fDesc.fType
              || kUniform_Type == fDesc.fType);
 }