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

["Zhang, Jerry (Junwei)" <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>]

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?

Regards,
Jerry

>
> Apart from that it look like a nice cleanup to me,
> Christian.
>
>>
>> Signed-off-by: Junwei Zhang <Jerry.Zhang@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