[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.Zhang at 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,
>>