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

Zhang, Jerry (Junwei) Jerry.Zhang at amd.com
Thu Aug 9 08:45:00 UTC 2018


On 08/09/2018 03:40 PM, Yu, Qiang wrote:
> Comments inline.
>
> Regards,
> Qiang
> ________________________________________
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> on behalf of Junwei Zhang <Jerry.Zhang at amd.com>
> Sent: Thursday, August 9, 2018 11:19 AM
> To: amd-gfx at lists.freedesktop.org
> Cc: Zhang, Jerry; Koenig, Christian
> Subject: [PATCH libdrm] amdgpu: add a function to create amdgpu bo internally
>
> a helper function to create and initialize amdgpu bo
>
> Signed-off-by: Junwei Zhang <Jerry.Zhang at amd.com>
> ---
>   amdgpu/amdgpu_bo.c | 184 ++++++++++++++++++++++++-----------------------------
>   1 file changed, 84 insertions(+), 100 deletions(-)
>
> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
> index b790e9b..2cca40d 100644
> --- a/amdgpu/amdgpu_bo.c
> +++ b/amdgpu/amdgpu_bo.c
> @@ -48,11 +48,32 @@ 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;
> [yuq] not used.
>
> +
> +       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);
> +
> +       *buf_handle = bo;
> +       return 0;
> +}
> +
>   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 +82,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,24 +93,21 @@ 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)
> +               goto out;
>
> +       r = amdgpu_bo_create(dev, alloc_buffer->alloc_size, args.out.handle,
> +                            buf_handle);
>          if (r)
> -               amdgpu_bo_free(bo);
> -       else
> -               *buf_handle = bo;
> +               goto out;
> [yuq] the handle is not freed.

Do you mean to free buf_handle?
in the bo_create, the failure means bo is failed to create where there is no bo created actually.

>
> +       pthread_mutex_lock(&dev->bo_table_mutex);
> +       r = handle_table_insert(&dev->bo_handles, (*buf_handle)->handle,
> +                               *buf_handle);
> +       pthread_mutex_unlock(&dev->bo_table_mutex);
> +       if (r)
> +               amdgpu_bo_free(*buf_handle);
> +out:
>          return r;
>   }
>
> @@ -256,7 +266,9 @@ int amdgpu_bo_import(amdgpu_device_handle dev,
>   {
>          struct drm_gem_open open_arg = {};
>          struct amdgpu_bo *bo = NULL;
> -       int r;
> +       uint32_t handle = 0, flink_name = 0;
> +       uint64_t alloc_size = 0;
> +       int r = 0;
>          int dma_fd;
>          uint64_t dma_buf_size = 0;
>
> @@ -266,22 +278,18 @@ int amdgpu_bo_import(amdgpu_device_handle dev,
>
>          /* Convert a DMA buf handle to a KMS handle now. */
>          if (type == amdgpu_bo_handle_type_dma_buf_fd) {
> -               uint32_t handle;
>                  off_t size;
>
>                  /* Get a KMS handle. */
>                  r = drmPrimeFDToHandle(dev->fd, shared_handle, &handle);
> -               if (r) {
> -                       pthread_mutex_unlock(&dev->bo_table_mutex);
> -                       return r;
> -               }
> +               if (r)
> +                       goto unlock;
>
>                  /* Query the buffer size. */
>                  size = lseek(shared_handle, 0, SEEK_END);
>                  if (size == (off_t)-1) {
> -                       pthread_mutex_unlock(&dev->bo_table_mutex);
> -                       amdgpu_close_kms_handle(dev, handle);
> -                       return -errno;
> +                       r = -errno;
> +                       goto unlock;
>                  }
>                  lseek(shared_handle, 0, SEEK_SET);
>
> @@ -320,58 +328,32 @@ int amdgpu_bo_import(amdgpu_device_handle dev,
>                  return 0;
>          }
>
> -       bo = calloc(1, sizeof(struct amdgpu_bo));
> -       if (!bo) {
> -               pthread_mutex_unlock(&dev->bo_table_mutex);
> -               if (type == amdgpu_bo_handle_type_dma_buf_fd) {
> -                       amdgpu_close_kms_handle(dev, shared_handle);
> -               }
> -               return -ENOMEM;
> -       }
> -
>          /* Open the handle. */
>          switch (type) {
>          case amdgpu_bo_handle_type_gem_flink_name:
>                  open_arg.name = shared_handle;
>                  r = drmIoctl(dev->flink_fd, DRM_IOCTL_GEM_OPEN, &open_arg);
> -               if (r) {
> -                       free(bo);
> -                       pthread_mutex_unlock(&dev->bo_table_mutex);
> -                       return r;
> -               }
> +               if (r)
> +                       goto unlock;
>
> -               bo->handle = open_arg.handle;
> +               flink_name = shared_handle;
> +               handle = open_arg.handle;
> +               alloc_size = open_arg.size;
>                  if (dev->flink_fd != dev->fd) {
> -                       r = drmPrimeHandleToFD(dev->flink_fd, bo->handle, DRM_CLOEXEC, &dma_fd);
> -                       if (r) {
> -                               free(bo);
> -                               pthread_mutex_unlock(&dev->bo_table_mutex);
> -                               return r;
> -                       }
> -                       r = drmPrimeFDToHandle(dev->fd, dma_fd, &bo->handle );
> -
> +                       r = drmPrimeHandleToFD(dev->flink_fd, handle,
> +                                              DRM_CLOEXEC, &dma_fd);
> +                       if (r)
> +                               goto unlock;
> +                       r = drmPrimeFDToHandle(dev->fd, dma_fd, &handle);
>                          close(dma_fd);
> -
> -                       if (r) {
> -                               free(bo);
> -                               pthread_mutex_unlock(&dev->bo_table_mutex);
> -                               return r;
> -                       }
> -               }
> -               bo->flink_name = shared_handle;
> -               bo->alloc_size = open_arg.size;
> -               r = handle_table_insert(&dev->bo_flink_names, shared_handle,
> -                                       bo);
> -               if (r) {
> -                       pthread_mutex_unlock(&dev->bo_table_mutex);
> -                       amdgpu_bo_free(bo);
> -                       return r;
> +                       if (r)
> +                               goto unlock;
>                  }
>                  break;
>
>          case amdgpu_bo_handle_type_dma_buf_fd:
> -               bo->handle = shared_handle;
> -               bo->alloc_size = dma_buf_size;
> +               handle = shared_handle;
> +               alloc_size = dma_buf_size;
>                  break;
>
>          case amdgpu_bo_handle_type_kms:
> @@ -380,16 +362,28 @@ int amdgpu_bo_import(amdgpu_device_handle dev,
>          }
>
>          /* Initialize it. */
> -       atomic_set(&bo->refcount, 1);
> -       bo->dev = dev;
> -       pthread_mutex_init(&bo->cpu_access_mutex, NULL);
> -
> -       handle_table_insert(&dev->bo_handles, bo->handle, bo);
> -       pthread_mutex_unlock(&dev->bo_table_mutex);
> +       amdgpu_bo_create(dev, alloc_size, handle, &bo);
> [yuq] no check return value.

good catch, will update that, thanks.

Regards,
Jerry

>
> +       if (flink_name) {
> +               r = handle_table_insert(&dev->bo_flink_names, flink_name,
> +                                       bo);
> +               if (r) {
> +                       amdgpu_bo_free(bo);
> +                       goto unlock;
> +               }
> +       }
> +       r = handle_table_insert(&dev->bo_handles, bo->handle, bo);
> +       if (r) {
> +               amdgpu_bo_free(bo);
> +               goto unlock;
> +       }
>
>          output->buf_handle = bo;
>          output->alloc_size = bo->alloc_size;
> -       return 0;
> +unlock:
> +       pthread_mutex_unlock(&dev->bo_table_mutex);
> +       if (r && type == amdgpu_bo_handle_type_dma_buf_fd && handle)
> +               amdgpu_close_kms_handle(dev, handle);
> +       return r;
>   }
>
>   int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
> @@ -574,7 +568,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;
> @@ -584,28 +577,19 @@ int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>          r = drmCommandWriteRead(dev->fd, DRM_AMDGPU_GEM_USERPTR,
>                                  &args, sizeof(args));
>          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);
> +               goto out;
>
> +       r = amdgpu_bo_create(dev, size, args.handle, buf_handle);
>          if (r)
> -               amdgpu_bo_free(bo);
> -       else
> -               *buf_handle = bo;
> +               goto out;
> [yuq] handle is not freed.
>
> +       pthread_mutex_lock(&dev->bo_table_mutex);
> +       r = handle_table_insert(&dev->bo_handles, (*buf_handle)->handle,
> +                               *buf_handle);
> +       pthread_mutex_unlock(&dev->bo_table_mutex);
> +       if (r)
> +               amdgpu_bo_free(*buf_handle);
> +out:
>          return r;
>   }
>
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>


More information about the amd-gfx mailing list