Re: [PATCH libdrm 4/4] amdgpu: add a function to create amdgpu bo internally

[Christian König]

Am 08.08.2018 um 09:12 schrieb Zhang, Jerry (Junwei):
On 08/08/2018 02:51 PM, Christian König wrote:
Am 08.08.2018 um 06:08 schrieb Junwei Zhang:
a helper function to create and initialize amdgpu bo

Can the new function be also used to initialize a BO structure during 
import?

Yeah, that's what I'm going to talk a bit more in this patch.
(actually it's a RFC patch)

When I'm working on it, find amdgpu_bo_import() holds the table lock 
through the function.
Wonder if it involves any potential issue, if I add the 
amdgpu_bo_create() at the end of function
out of the table lock?

If so, I would provide a amdgpu_bo_create_locked() function to insert 
the range of holding the lock.

Any background/insight could be shared?

Ah, yes of course. We need to hold the lock during import to make sure 
that we don't import things twice.

I would just move adding the handle to the table out of the function 
into the callers.

It's still a nice cleanup if we just have the structure initialization 
in one place.

Christian.


Regards,
Jerry


Apart from that it look like a nice cleanup to me,
Christian.


Signed-off-by: Junwei Zhang <jerry.zh...@amd.com>
---
  amdgpu/amdgpu_bo.c | 81 
++++++++++++++++++++++--------------------------------
  1 file changed, 33 insertions(+), 48 deletions(-)

diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
index a7f0662..59cba69 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -48,11 +48,39 @@ static void 
amdgpu_close_kms_handle(amdgpu_device_handle dev,
      drmIoctl(dev->fd, DRM_IOCTL_GEM_CLOSE, &args);
  }
+static int amdgpu_bo_create(amdgpu_device_handle dev,
+                uint64_t size,
+                uint32_t handle,
+                amdgpu_bo_handle *buf_handle)
+{
+    struct amdgpu_bo *bo;
+    int r = 0;
+
+    bo = calloc(1, sizeof(struct amdgpu_bo));
+    if (!bo)
+        return -ENOMEM;
+
+    atomic_set(&bo->refcount, 1);
+    bo->dev = dev;
+    bo->alloc_size = size;
+    bo->handle = handle;
+    pthread_mutex_init(&bo->cpu_access_mutex, NULL);
+
+    pthread_mutex_lock(&bo->dev->bo_table_mutex);
+    r = handle_table_insert(&bo->dev->bo_handles, bo->handle, bo);
+    pthread_mutex_unlock(&bo->dev->bo_table_mutex);
+    if (r)
+        amdgpu_bo_free(bo);
+    else
+        *buf_handle = bo;
+
+    return r;
+}
+
  int amdgpu_bo_alloc(amdgpu_device_handle dev,
              struct amdgpu_bo_alloc_request *alloc_buffer,
              amdgpu_bo_handle *buf_handle)
  {
-    struct amdgpu_bo *bo;
      union drm_amdgpu_gem_create args;
      unsigned heap = alloc_buffer->preferred_heap;
      int r = 0;
@@ -61,14 +89,6 @@ int amdgpu_bo_alloc(amdgpu_device_handle dev,
      if (!(heap & (AMDGPU_GEM_DOMAIN_GTT | AMDGPU_GEM_DOMAIN_VRAM)))
          return -EINVAL;
-    bo = calloc(1, sizeof(struct amdgpu_bo));
-    if (!bo)
-        return -ENOMEM;
-
-    atomic_set(&bo->refcount, 1);
-    bo->dev = dev;
-    bo->alloc_size = alloc_buffer->alloc_size;
-
      memset(&args, 0, sizeof(args));
      args.in.bo_size = alloc_buffer->alloc_size;
      args.in.alignment = alloc_buffer->phys_alignment;
@@ -80,25 +100,11 @@ int amdgpu_bo_alloc(amdgpu_device_handle dev,
      /* Allocate the buffer with the preferred heap. */
      r = drmCommandWriteRead(dev->fd, DRM_AMDGPU_GEM_CREATE,
                  &args, sizeof(args));
-    if (r) {
-        free(bo);
-        return r;
-    }
-
-    bo->handle = args.out.handle;
-
-    pthread_mutex_lock(&bo->dev->bo_table_mutex);
-    r = handle_table_insert(&bo->dev->bo_handles, bo->handle, bo);
-    pthread_mutex_unlock(&bo->dev->bo_table_mutex);
-
-    pthread_mutex_init(&bo->cpu_access_mutex, NULL);
-
      if (r)
-        amdgpu_bo_free(bo);
-    else
-        *buf_handle = bo;
+        return r;
-    return r;
+    return amdgpu_bo_create(dev, alloc_buffer->alloc_size, 
args.out.handle,
+                buf_handle);
  }
  int amdgpu_bo_set_metadata(amdgpu_bo_handle bo,
@@ -569,7 +575,6 @@ int 
amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
                      amdgpu_bo_handle *buf_handle)
  {
      int r;
-    struct amdgpu_bo *bo;
      struct drm_amdgpu_gem_userptr args;
      args.addr = (uintptr_t)cpu;
@@ -581,27 +586,7 @@ int 
amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
      if (r)
          return r;
-    bo = calloc(1, sizeof(struct amdgpu_bo));
-    if (!bo)
-        return -ENOMEM;
-
-    atomic_set(&bo->refcount, 1);
-    bo->dev = dev;
-    bo->alloc_size = size;
-    bo->handle = args.handle;
-
-    pthread_mutex_lock(&bo->dev->bo_table_mutex);
-    r = handle_table_insert(&bo->dev->bo_handles, bo->handle, bo);
-    pthread_mutex_unlock(&bo->dev->bo_table_mutex);
-
-    pthread_mutex_init(&bo->cpu_access_mutex, NULL);
-
-    if (r)
-        amdgpu_bo_free(bo);
-    else
-        *buf_handle = bo;
-
-    return r;
+    return amdgpu_bo_create(dev, size, args.handle, buf_handle);
  }
  int amdgpu_bo_list_create(amdgpu_device_handle dev,


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx