From: "Zhang, Jerry (Junwei)" <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>
To: "Christian König" <christian.koenig-5C7GfCeVMHo@public.gmane.org>,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH libdrm] amdgpu: add a function to create amdgpu bo internally (v4)
Date: Tue, 14 Aug 2018 10:59:43 +0800 [thread overview]
Message-ID: <5B72459F.8040309@amd.com> (raw)
In-Reply-To: <e5ed0c82-74ad-f7ea-58af-84f3dee3e090-5C7GfCeVMHo@public.gmane.org>
On 08/13/2018 06:14 PM, Christian König wrote:
> Am 13.08.2018 um 12:06 schrieb Junwei Zhang:
>> a helper function to create and initialize amdgpu bo
>>
>> v2: update error handling: add label and free bo
>> v3: update error handling: separate each error label
>> v4: update error handling and free flink bo in bo import
>>
>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>
> A separate patch for the fix to flink handle handling would be nice, but patch is Reviewed-by: Christian König <christian.koenig@amd.com> anyway.
Thanks, yes.
I will prepare the fix patch for original code base, then it could be ported for legacy libdrm.
This patch could be rebased as final version.
Jerry
>
> Regards,
> Christian.
>
>> ---
>> amdgpu/amdgpu_bo.c | 208 +++++++++++++++++++++++++++--------------------------
>> 1 file changed, 107 insertions(+), 101 deletions(-)
>>
>> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
>> index b790e9b..ad72f09 100644
>> --- a/amdgpu/amdgpu_bo.c
>> +++ b/amdgpu/amdgpu_bo.c
>> @@ -48,11 +48,31 @@ 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;
>> +
>> + 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 +81,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 +92,23 @@ 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)
>> + goto out;
>> +
>> + r = amdgpu_bo_create(dev, alloc_buffer->alloc_size, args.out.handle,
>> + buf_handle);
>> if (r) {
>> - free(bo);
>> - return r;
>> + amdgpu_close_kms_handle(dev, args.out.handle);
>> + goto out;
>> }
>> - 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);
>> -
>> + 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(bo);
>> - else
>> - *buf_handle = bo;
>> -
>> + amdgpu_bo_free(*buf_handle);
>> +out:
>> return r;
>> }
>> @@ -255,8 +266,11 @@ int amdgpu_bo_import(amdgpu_device_handle dev,
>> struct amdgpu_bo_import_result *output)
>> {
>> struct drm_gem_open open_arg = {};
>> + struct drm_gem_close close_args = {};
>> 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 +280,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 free_bo_handle;
>> }
>> lseek(shared_handle, 0, SEEK_SET);
>> @@ -302,12 +312,12 @@ int amdgpu_bo_import(amdgpu_device_handle dev,
>> case amdgpu_bo_handle_type_kms:
>> case amdgpu_bo_handle_type_kms_noimport:
>> /* Importing a KMS handle in not allowed. */
>> - pthread_mutex_unlock(&dev->bo_table_mutex);
>> - return -EPERM;
>> + r = -EPERM;
>> + goto unlock;
>> default:
>> - pthread_mutex_unlock(&dev->bo_table_mutex);
>> - return -EINVAL;
>> + r = -EINVAL;
>> + goto unlock;
>> }
>> if (bo) {
>> @@ -320,58 +330,37 @@ 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 free_bo_handle;
>> + 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 free_bo_handle;
>> + close_args.handle = open_arg.handle;
>> + r = drmIoctl(dev->flink_fd, DRM_IOCTL_GEM_CLOSE,
>> + &close_args);
>> + if (r)
>> + goto free_bo_handle;
>> }
>> 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 +369,41 @@ 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);
>> + r = amdgpu_bo_create(dev, alloc_size, handle, &bo);
>> + if (r)
>> + goto free_bo_handle;
>> - handle_table_insert(&dev->bo_handles, bo->handle, bo);
>> - pthread_mutex_unlock(&dev->bo_table_mutex);
>> + r = handle_table_insert(&dev->bo_handles, bo->handle, bo);
>> + if (r)
>> + goto free_bo_handle;
>> + if (flink_name) {
>> + bo->flink_name = flink_name;
>> + r = handle_table_insert(&dev->bo_flink_names, flink_name,
>> + bo);
>> + if (r)
>> + goto remove_handle;
>> +
>> + }
>> output->buf_handle = bo;
>> output->alloc_size = bo->alloc_size;
>> + pthread_mutex_unlock(&dev->bo_table_mutex);
>> return 0;
>> +
>> +remove_handle:
>> + handle_table_remove(&dev->bo_handles, bo->handle);
>> +free_bo_handle:
>> + if (flink_name && !close_args.handle && open_arg.handle) {
>> + close_args.handle = open_arg.handle;
>> + drmIoctl(dev->flink_fd, DRM_IOCTL_GEM_CLOSE, &close_args);
>> + }
>> + if (bo)
>> + amdgpu_bo_free(bo);
>> + else
>> + amdgpu_close_kms_handle(dev, handle);
>> +unlock:
>> + pthread_mutex_unlock(&dev->bo_table_mutex);
>> + return r;
>> }
>> int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
>> @@ -574,7 +588,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 +597,21 @@ 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;
>> + goto out;
>> - 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);
>> + r = amdgpu_bo_create(dev, size, args.handle, buf_handle);
>> + if (r) {
>> + amdgpu_close_kms_handle(dev, args.handle);
>> + goto out;
>> + }
>> + 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(bo);
>> - else
>> - *buf_handle = bo;
>> -
>> + amdgpu_bo_free(*buf_handle);
>> +out:
>> return r;
>> }
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
prev parent reply other threads:[~2018-08-14 2:59 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-13 10:06 [PATCH libdrm] amdgpu: add a function to create amdgpu bo internally (v4) Junwei Zhang
[not found] ` <1534154763-2287-1-git-send-email-Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>
2018-08-13 10:14 ` Christian König
[not found] ` <e5ed0c82-74ad-f7ea-58af-84f3dee3e090-5C7GfCeVMHo@public.gmane.org>
2018-08-14 2:59 ` Zhang, Jerry (Junwei) [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5B72459F.8040309@amd.com \
--to=jerry.zhang-5c7gfcevmho@public.gmane.org \
--cc=amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
--cc=christian.koenig-5C7GfCeVMHo@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.