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 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,
>
