v3dv: add a refcount mechanism to BOs

Until now we have lived without a refcount mechanism in the driver
because in Vulkan the user is responsible for handling the life
span of memory allocations for all Vulkan objects, however,
imported BOs are tricky because the kernel doesn't refcount
so user-space needs to make sure that:

1. When importing a BO into the same device used to create it
   (self-importing) it does not double free the same BO.
2. Frees imported BOs that were not allocated through the same
   device.

Our initial implementation always freed BOs when requested,
so we handled 2) correctly but not 1) and we would double-free
self-imported BOs. We tried to fix that in commit d809d9f3
but that broke 2) and we started to leak BOs for some imports.

This fixes the problem for good by adding refcounts to BOs
so that self-imported BOs have a refcnt > 1 and are only freed
when all references are freed.

Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/5769
Tested-by: Roman Stratiienko <r.stratiienko@gmail.com>
Reviewed-by: Juan A. Suarez <jasuarez@igalia.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14392>

src/broadcom/vulkan/v3dv_bo.c

@@ -117,8 +117,8 @@ bo_from_cache(struct v3dv_device *device, uint32_t size, const char *name)
       }
 
       bo_remove_from_cache(cache, bo);
-
       bo->name = name;
+      p_atomic_set(&bo->refcnt, 1);
    }
    mtx_unlock(&cache->lock);
    return bo;
@@ -131,12 +131,21 @@ bo_free(struct v3dv_device *device,
    if (!bo)
       return true;
 
+   assert(p_atomic_read(&bo->refcnt) == 0);
+
    if (bo->map)
       v3dv_bo_unmap(device, bo);
 
+   /* Our BO structs are stored in a sparse array in the physical device,
+    * so we don't want to free the BO pointer, instead we want to reset it
+    * to 0, to signal that array entry as being free.
+    */
+   uint32_t handle = bo->handle;
+   memset(bo, 0, sizeof(*bo));
+
    struct drm_gem_close c;
    memset(&c, 0, sizeof(c));
-   c.handle = bo->handle;
+   c.handle = handle;
    int ret = v3dv_ioctl(device->pdevice->render_fd, DRM_IOCTL_GEM_CLOSE, &c);
    if (ret != 0)
       fprintf(stderr, "close object %d: %s\n", bo->handle, strerror(errno));
@@ -152,8 +161,6 @@ bo_free(struct v3dv_device *device,
       bo_dump_stats(device);
    }
 
-   vk_free(&device->vk.alloc, bo);
-
    return ret == 0;
 }
 
@@ -183,6 +190,7 @@ v3dv_bo_init(struct v3dv_bo *bo,
              const char *name,
              bool private)
 {
+   p_atomic_set(&bo->refcnt, 1);
    bo->handle = handle;
    bo->handle_bit = 1ull << (handle % 64);
    bo->size = size;
@@ -218,14 +226,6 @@ v3dv_bo_alloc(struct v3dv_device *device,
       }
    }
 
-   bo = vk_alloc(&device->vk.alloc, sizeof(struct v3dv_bo), 8,
-                 VK_SYSTEM_ALLOCATION_SCOPE_DEVICE);
-
-   if (!bo) {
-      fprintf(stderr, "Failed to allocate host memory for BO\n");
-      return NULL;
-   }
-
  retry:
    ;
 
@@ -244,7 +244,6 @@ v3dv_bo_alloc(struct v3dv_device *device,
          goto retry;
       }
 
-      vk_free(&device->vk.alloc, bo);
       fprintf(stderr, "Failed to allocate device memory for BO\n");
       return NULL;
    }
@@ -252,6 +251,9 @@ v3dv_bo_alloc(struct v3dv_device *device,
    assert(create.offset % page_align == 0);
    assert((create.offset & 0xffffffff) == create.offset);
 
+   bo = v3dv_device_lookup_bo(device->pdevice, create.handle);
+   assert(bo && bo->handle == 0);
+
    v3dv_bo_init(bo, create.handle, size, create.offset, name, private);
 
    device->bo_count++;
@@ -455,6 +457,9 @@ v3dv_bo_free(struct v3dv_device *device,
    if (!bo)
       return true;
 
+   if (!p_atomic_dec_zero(&bo->refcnt))
+      return true;
+
    struct timespec time;
    struct v3dv_bo_cache *cache = &device->bo_cache;
    uint32_t page_index = bo->size / 4096 - 1;

src/broadcom/vulkan/v3dv_bo.h

@@ -57,6 +57,8 @@ struct v3dv_bo {
     * handle of the dumb BO on that device.
     */
    int32_t dumb_handle;
+
+   int32_t refcnt;
 };
 
 void v3dv_bo_init(struct v3dv_bo *bo, uint32_t handle, uint32_t size, uint32_t offset, const char *name, bool private);

src/broadcom/vulkan/v3dv_device.c

@@ -267,6 +267,8 @@ physical_device_finish(struct v3dv_physical_device *device)
    v3dv_physical_device_free_disk_cache(device);
    v3d_compiler_free(device->compiler);
 
+   util_sparse_array_finish(&device->bo_map);
+
    close(device->render_fd);
    if (device->display_fd >= 0)
       close(device->display_fd);
@@ -822,6 +824,9 @@ physical_device_init(struct v3dv_physical_device *device,
       VK_MEMORY_PROPERTY_HOST_COHERENT_BIT;
    mem->memoryTypes[0].heapIndex = 0;
 
+   /* Initialize sparse array for refcounting imported BOs */
+   util_sparse_array_init(&device->bo_map, sizeof(struct v3dv_bo), 512);
+
    device->options.merge_jobs = getenv("V3DV_NO_MERGE_JOBS") == NULL;
 
    result = v3dv_wsi_init(device);
@@ -1912,15 +1917,11 @@ device_free(struct v3dv_device *device, struct v3dv_device_memory *mem)
     * display device to free the allocated dumb BO.
     */
    if (mem->is_for_wsi) {
-      assert(mem->has_bo_ownership);
       device_free_wsi_dumb(device->instance->physicalDevice.display_fd,
                            mem->bo->dumb_handle);
    }
 
-   if (mem->has_bo_ownership)
-      v3dv_bo_free(device, mem->bo);
-   else if (mem->bo)
-      vk_free(&device->vk.alloc, mem->bo);
+   v3dv_bo_free(device, mem->bo);
 }
 
 static void
@@ -1965,21 +1966,12 @@ device_import_bo(struct v3dv_device *device,
                  int fd, uint64_t size,
                  struct v3dv_bo **bo)
 {
-   VkResult result;
-
-   *bo = vk_alloc2(&device->vk.alloc, pAllocator, sizeof(struct v3dv_bo), 8,
-                   VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
-   if (*bo == NULL) {
-      result = VK_ERROR_OUT_OF_HOST_MEMORY;
-      goto fail;
-   }
+   *bo = NULL;
 
    off_t real_size = lseek(fd, 0, SEEK_END);
    lseek(fd, 0, SEEK_SET);
-   if (real_size < 0 || (uint64_t) real_size < size) {
-      result = VK_ERROR_INVALID_EXTERNAL_HANDLE;
-      goto fail;
-   }
+   if (real_size < 0 || (uint64_t) real_size < size)
+      return VK_ERROR_INVALID_EXTERNAL_HANDLE;
 
    int render_fd = device->pdevice->render_fd;
    assert(render_fd >= 0);
@@ -1987,31 +1979,26 @@ device_import_bo(struct v3dv_device *device,
    int ret;
    uint32_t handle;
    ret = drmPrimeFDToHandle(render_fd, fd, &handle);
-   if (ret) {
-      result = VK_ERROR_INVALID_EXTERNAL_HANDLE;
-      goto fail;
-   }
+   if (ret)
+      return VK_ERROR_INVALID_EXTERNAL_HANDLE;
 
    struct drm_v3d_get_bo_offset get_offset = {
       .handle = handle,
    };
    ret = v3dv_ioctl(render_fd, DRM_IOCTL_V3D_GET_BO_OFFSET, &get_offset);
-   if (ret) {
-      result = VK_ERROR_INVALID_EXTERNAL_HANDLE;
-      goto fail;
-   }
+   if (ret)
+      return VK_ERROR_INVALID_EXTERNAL_HANDLE;
    assert(get_offset.offset != 0);
 
-   v3dv_bo_init(*bo, handle, size, get_offset.offset, "import", false);
+   *bo = v3dv_device_lookup_bo(device->pdevice, handle);
+   assert(*bo);
+
+   if ((*bo)->refcnt == 0)
+      v3dv_bo_init(*bo, handle, size, get_offset.offset, "import", false);
+   else
+      p_atomic_inc(&(*bo)->refcnt);
 
    return VK_SUCCESS;
-
-fail:
-   if (*bo) {
-      vk_free2(&device->vk.alloc, pAllocator, *bo);
-      *bo = NULL;
-   }
-   return result;
 }
 
 static VkResult
@@ -2102,7 +2089,6 @@ v3dv_AllocateMemory(VkDevice _device,
 
    assert(pAllocateInfo->memoryTypeIndex < pdevice->memory.memoryTypeCount);
    mem->type = &pdevice->memory.memoryTypes[pAllocateInfo->memoryTypeIndex];
-   mem->has_bo_ownership = true;
    mem->is_for_wsi = false;
 
    const struct wsi_memory_allocate_info *wsi_info = NULL;
@@ -2154,7 +2140,6 @@ v3dv_AllocateMemory(VkDevice _device,
                 fd_info->handleType == VK_EXTERNAL_MEMORY_HANDLE_TYPE_DMA_BUF_BIT_EXT);
          result = device_import_bo(device, pAllocator,
                                    fd_info->fd, alloc_size, &mem->bo);
-         mem->has_bo_ownership = false;
          if (result == VK_SUCCESS)
             close(fd_info->fd);
       } else {

src/broadcom/vulkan/v3dv_private.h

@@ -73,6 +73,7 @@
 #include "vk_debug_report.h"
 #include "util/set.h"
 #include "util/hash_table.h"
+#include "util/sparse_array.h"
 #include "util/xmlconfig.h"
 #include "u_atomic.h"
 
@@ -153,6 +154,23 @@ struct v3dv_physical_device {
    const struct v3d_compiler *compiler;
    uint32_t next_program_id;
 
+   /* This array holds all our 'struct v3dv_bo' allocations. We use this
+    * so we can add a refcount to our BOs and check if a particular BO
+    * was already allocated in this device using its GEM handle. This is
+    * necessary to properly manage BO imports, because the kernel doesn't
+    * refcount the underlying BO memory.
+    *
+    * Specifically, when self-importing (i.e. importing a BO into the same
+    * device that created it), the kernel will give us the same BO handle
+    * for both BOs and we must only free it once when  both references are
+    * freed. Otherwise, if we are not self-importing, we get two differnt BO
+    * handles, and we want to free each one individually.
+    *
+    * The BOs in this map all have a refcnt with the referece counter and
+    * only self-imported BOs will ever have a refcnt > 1.
+    */
+   struct util_sparse_array bo_map;
+
    struct {
       bool merge_jobs;
    } options;
@@ -162,6 +180,12 @@ VkResult v3dv_physical_device_acquire_display(struct v3dv_instance *instance,
                                               struct v3dv_physical_device *pdevice,
                                               VkIcdSurfaceBase *surface);
 
+static inline struct v3dv_bo *
+v3dv_device_lookup_bo(struct v3dv_physical_device *device, uint32_t handle)
+{
+   return (struct v3dv_bo *) util_sparse_array_get(&device->bo_map, handle);
+}
+
 VkResult v3dv_wsi_init(struct v3dv_physical_device *physical_device);
 void v3dv_wsi_finish(struct v3dv_physical_device *physical_device);
 struct v3dv_image *v3dv_wsi_get_image_from_swapchain(VkSwapchainKHR swapchain,
@@ -487,7 +511,6 @@ struct v3dv_device_memory {
 
    struct v3dv_bo *bo;
    const VkMemoryType *type;
-   bool has_bo_ownership;
    bool is_for_wsi;
 };