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