[PATCH libdrm 1/4] amdgpu: add bo from user memory to handle table

[PATCH libdrm 1/4] amdgpu: add bo from user memory to handle table

[Junwei Zhang]

When create bo from user memory, add it to handle table
for future query.

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 amdgpu/amdgpu_bo.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
index 422c7c9..b24e698 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -556,7 +556,16 @@ int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
 	bo->alloc_size = size;
 	bo->handle = args.handle;
 
-	*buf_handle = bo;
+	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;
 }
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 2/4] amdgpu: add a function to find bo by cpu mapping (v2)

[Junwei Zhang]

Userspace needs to know if the user memory is from BO or malloc.

v2: update mutex range and rebase

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 amdgpu/amdgpu.h    | 23 +++++++++++++++++++++++
 amdgpu/amdgpu_bo.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index be83b45..a8c353c 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -678,6 +678,29 @@ int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
 				    amdgpu_bo_handle *buf_handle);
 
 /**
+ * Validate if the user memory comes from BO
+ *
+ * \param dev - [in] Device handle. See #amdgpu_device_initialize()
+ * \param cpu - [in] CPU address of user allocated memory which we
+ * want to map to GPU address space (make GPU accessible)
+ * (This address must be correctly aligned).
+ * \param size - [in] Size of allocation (must be correctly aligned)
+ * \param buf_handle - [out] Buffer handle for the userptr memory
+ * if the user memory is not from BO, the buf_handle will be NULL.
+ * \param offset_in_bo - [out] offset in this BO for this user memory
+ *
+ *
+ * \return   0 on success\n
+ *          <0 - Negative POSIX Error code
+ *
+*/
+int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
+				  void *cpu,
+				  uint64_t size,
+				  amdgpu_bo_handle *buf_handle,
+				  uint64_t *offset_in_bo);
+
+/**
  * Free previosuly allocated memory
  *
  * \param   dev	       - \c [in] Device handle. See #amdgpu_device_initialize()
diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
index b24e698..a7f0662 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -529,6 +529,40 @@ int amdgpu_bo_wait_for_idle(amdgpu_bo_handle bo,
 	}
 }
 
+int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
+				  void *cpu,
+				  uint64_t size,
+				  amdgpu_bo_handle *buf_handle,
+				  uint64_t *offset_in_bo)
+{
+	int i;
+	struct amdgpu_bo *bo;
+
+	if (cpu == NULL || size == 0)
+		return -EINVAL;
+
+	pthread_mutex_lock(&dev->bo_table_mutex);
+	for (i = 0; i < dev->bo_handles.max_key; i++) {
+		bo = handle_table_lookup(&dev->bo_handles, i);
+		if (!bo || !bo->cpu_ptr || size > bo->alloc_size)
+			continue;
+		if (cpu >= bo->cpu_ptr && cpu < (bo->cpu_ptr + bo->alloc_size))
+			break;
+	}
+
+	if (i < dev->bo_handles.max_key) {
+		atomic_inc(&bo->refcount);
+		*buf_handle = bo;
+		*offset_in_bo = cpu - bo->cpu_ptr;
+	} else {
+		*buf_handle = NULL;
+		*offset_in_bo = 0;
+	}
+	pthread_mutex_unlock(&dev->bo_table_mutex);
+
+	return 0;
+}
+
 int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
 				    void *cpu,
 				    uint64_t size,
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 2/4] amdgpu: add a function to find bo by cpu mapping (v2)

[zhoucm1]

On 2018年08月08日 12:08, Junwei Zhang wrote:
> Userspace needs to know if the user memory is from BO or malloc.
>
> v2: update mutex range and rebase
>
> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
> ---
>   amdgpu/amdgpu.h    | 23 +++++++++++++++++++++++
>   amdgpu/amdgpu_bo.c | 34 ++++++++++++++++++++++++++++++++++
>   2 files changed, 57 insertions(+)
>
> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
> index be83b45..a8c353c 100644
> --- a/amdgpu/amdgpu.h
> +++ b/amdgpu/amdgpu.h
> @@ -678,6 +678,29 @@ int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>   				    amdgpu_bo_handle *buf_handle);
>   
>   /**
> + * Validate if the user memory comes from BO
> + *
> + * \param dev - [in] Device handle. See #amdgpu_device_initialize()
> + * \param cpu - [in] CPU address of user allocated memory which we
> + * want to map to GPU address space (make GPU accessible)
> + * (This address must be correctly aligned).
> + * \param size - [in] Size of allocation (must be correctly aligned)
> + * \param buf_handle - [out] Buffer handle for the userptr memory
> + * if the user memory is not from BO, the buf_handle will be NULL.
> + * \param offset_in_bo - [out] offset in this BO for this user memory
> + *
> + *
> + * \return   0 on success\n
> + *          <0 - Negative POSIX Error code
> + *
> +*/
> +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
> +				  void *cpu,
> +				  uint64_t size,
> +				  amdgpu_bo_handle *buf_handle,
> +				  uint64_t *offset_in_bo);
> +
> +/**
>    * Free previosuly allocated memory
>    *
>    * \param   dev	       - \c [in] Device handle. See #amdgpu_device_initialize()
> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
> index b24e698..a7f0662 100644
> --- a/amdgpu/amdgpu_bo.c
> +++ b/amdgpu/amdgpu_bo.c
> @@ -529,6 +529,40 @@ int amdgpu_bo_wait_for_idle(amdgpu_bo_handle bo,
>   	}
>   }
>   
> +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
> +				  void *cpu,
> +				  uint64_t size,
> +				  amdgpu_bo_handle *buf_handle,
> +				  uint64_t *offset_in_bo)
> +{
> +	int i;
> +	struct amdgpu_bo *bo;
> +
> +	if (cpu == NULL || size == 0)
> +		return -EINVAL;
> +
> +	pthread_mutex_lock(&dev->bo_table_mutex);
> +	for (i = 0; i < dev->bo_handles.max_key; i++) {
Hi Jerry,

As Christian catched before, iterating all BOs of device will introduce 
much CPU overhead, this isn't good direction.
Since cpu virtual address is per-process, you should go to kernel to 
find them from vm tree, which obviously takes less time.

Regards,
David Zhou
> +		bo = handle_table_lookup(&dev->bo_handles, i);
> +		if (!bo || !bo->cpu_ptr || size > bo->alloc_size)
> +			continue;
> +		if (cpu >= bo->cpu_ptr && cpu < (bo->cpu_ptr + bo->alloc_size))
> +			break;
> +	}
> +
> +	if (i < dev->bo_handles.max_key) {
> +		atomic_inc(&bo->refcount);
> +		*buf_handle = bo;
> +		*offset_in_bo = cpu - bo->cpu_ptr;
> +	} else {
> +		*buf_handle = NULL;
> +		*offset_in_bo = 0;
> +	}
> +	pthread_mutex_unlock(&dev->bo_table_mutex);
> +
> +	return 0;
> +}
> +
>   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>   				    void *cpu,
>   				    uint64_t size,

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 2/4] amdgpu: add a function to find bo by cpu mapping (v2)

[Christian König]

Am 08.08.2018 um 06:23 schrieb zhoucm1:
>
>
> On 2018年08月08日 12:08, Junwei Zhang wrote:
>> Userspace needs to know if the user memory is from BO or malloc.
>>
>> v2: update mutex range and rebase
>>
>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>> ---
>>   amdgpu/amdgpu.h    | 23 +++++++++++++++++++++++
>>   amdgpu/amdgpu_bo.c | 34 ++++++++++++++++++++++++++++++++++
>>   2 files changed, 57 insertions(+)
>>
>> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
>> index be83b45..a8c353c 100644
>> --- a/amdgpu/amdgpu.h
>> +++ b/amdgpu/amdgpu.h
>> @@ -678,6 +678,29 @@ int 
>> amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>>                       amdgpu_bo_handle *buf_handle);
>>     /**
>> + * Validate if the user memory comes from BO
>> + *
>> + * \param dev - [in] Device handle. See #amdgpu_device_initialize()
>> + * \param cpu - [in] CPU address of user allocated memory which we
>> + * want to map to GPU address space (make GPU accessible)
>> + * (This address must be correctly aligned).
>> + * \param size - [in] Size of allocation (must be correctly aligned)
>> + * \param buf_handle - [out] Buffer handle for the userptr memory
>> + * if the user memory is not from BO, the buf_handle will be NULL.
>> + * \param offset_in_bo - [out] offset in this BO for this user memory
>> + *
>> + *
>> + * \return   0 on success\n
>> + *          <0 - Negative POSIX Error code
>> + *
>> +*/
>> +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
>> +                  void *cpu,
>> +                  uint64_t size,
>> +                  amdgpu_bo_handle *buf_handle,
>> +                  uint64_t *offset_in_bo);
>> +
>> +/**
>>    * Free previosuly allocated memory
>>    *
>>    * \param   dev           - \c [in] Device handle. See 
>> #amdgpu_device_initialize()
>> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
>> index b24e698..a7f0662 100644
>> --- a/amdgpu/amdgpu_bo.c
>> +++ b/amdgpu/amdgpu_bo.c
>> @@ -529,6 +529,40 @@ int amdgpu_bo_wait_for_idle(amdgpu_bo_handle bo,
>>       }
>>   }
>>   +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
>> +                  void *cpu,
>> +                  uint64_t size,
>> +                  amdgpu_bo_handle *buf_handle,
>> +                  uint64_t *offset_in_bo)
>> +{
>> +    int i;
>> +    struct amdgpu_bo *bo;
>> +
>> +    if (cpu == NULL || size == 0)
>> +        return -EINVAL;
>> +
>> +    pthread_mutex_lock(&dev->bo_table_mutex);
>> +    for (i = 0; i < dev->bo_handles.max_key; i++) {
> Hi Jerry,
>
> As Christian catched before, iterating all BOs of device will 
> introduce much CPU overhead, this isn't good direction.
> Since cpu virtual address is per-process, you should go to kernel to 
> find them from vm tree, which obviously takes less time.

Yeah, but is also much more overhead to maintain.

Since this is only to fix the behavior of a single buggy application at 
least I'm fine to keep the workaround as simple as this.

If we find a wider use we can still start to use the kernel 
implementation again.

Regards,
Christian.

>
> Regards,
> David Zhou
>> +        bo = handle_table_lookup(&dev->bo_handles, i);
>> +        if (!bo || !bo->cpu_ptr || size > bo->alloc_size)
>> +            continue;
>> +        if (cpu >= bo->cpu_ptr && cpu < (bo->cpu_ptr + bo->alloc_size))
>> +            break;
>> +    }
>> +
>> +    if (i < dev->bo_handles.max_key) {
>> +        atomic_inc(&bo->refcount);
>> +        *buf_handle = bo;
>> +        *offset_in_bo = cpu - bo->cpu_ptr;
>> +    } else {
>> +        *buf_handle = NULL;
>> +        *offset_in_bo = 0;
>> +    }
>> +    pthread_mutex_unlock(&dev->bo_table_mutex);
>> +
>> +    return 0;
>> +}
>> +
>>   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>>                       void *cpu,
>>                       uint64_t size,
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 2/4] amdgpu: add a function to find bo by cpu mapping (v2)

[Zhang, Jerry (Junwei)]

On 08/08/2018 02:48 PM, Christian König wrote:
> Am 08.08.2018 um 06:23 schrieb zhoucm1:
>>
>>
>> On 2018年08月08日 12:08, Junwei Zhang wrote:
>>> Userspace needs to know if the user memory is from BO or malloc.
>>>
>>> v2: update mutex range and rebase
>>>
>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>> ---
>>>   amdgpu/amdgpu.h    | 23 +++++++++++++++++++++++
>>>   amdgpu/amdgpu_bo.c | 34 ++++++++++++++++++++++++++++++++++
>>>   2 files changed, 57 insertions(+)
>>>
>>> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
>>> index be83b45..a8c353c 100644
>>> --- a/amdgpu/amdgpu.h
>>> +++ b/amdgpu/amdgpu.h
>>> @@ -678,6 +678,29 @@ int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>>>                       amdgpu_bo_handle *buf_handle);
>>>     /**
>>> + * Validate if the user memory comes from BO
>>> + *
>>> + * \param dev - [in] Device handle. See #amdgpu_device_initialize()
>>> + * \param cpu - [in] CPU address of user allocated memory which we
>>> + * want to map to GPU address space (make GPU accessible)
>>> + * (This address must be correctly aligned).
>>> + * \param size - [in] Size of allocation (must be correctly aligned)
>>> + * \param buf_handle - [out] Buffer handle for the userptr memory
>>> + * if the user memory is not from BO, the buf_handle will be NULL.
>>> + * \param offset_in_bo - [out] offset in this BO for this user memory
>>> + *
>>> + *
>>> + * \return   0 on success\n
>>> + *          <0 - Negative POSIX Error code
>>> + *
>>> +*/
>>> +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
>>> +                  void *cpu,
>>> +                  uint64_t size,
>>> +                  amdgpu_bo_handle *buf_handle,
>>> +                  uint64_t *offset_in_bo);
>>> +
>>> +/**
>>>    * Free previosuly allocated memory
>>>    *
>>>    * \param   dev           - \c [in] Device handle. See #amdgpu_device_initialize()
>>> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
>>> index b24e698..a7f0662 100644
>>> --- a/amdgpu/amdgpu_bo.c
>>> +++ b/amdgpu/amdgpu_bo.c
>>> @@ -529,6 +529,40 @@ int amdgpu_bo_wait_for_idle(amdgpu_bo_handle bo,
>>>       }
>>>   }
>>>   +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
>>> +                  void *cpu,
>>> +                  uint64_t size,
>>> +                  amdgpu_bo_handle *buf_handle,
>>> +                  uint64_t *offset_in_bo)
>>> +{
>>> +    int i;
>>> +    struct amdgpu_bo *bo;
>>> +
>>> +    if (cpu == NULL || size == 0)
>>> +        return -EINVAL;
>>> +
>>> +    pthread_mutex_lock(&dev->bo_table_mutex);
>>> +    for (i = 0; i < dev->bo_handles.max_key; i++) {
>> Hi Jerry,
>>
>> As Christian catched before, iterating all BOs of device will introduce much CPU overhead, this isn't good direction.
>> Since cpu virtual address is per-process, you should go to kernel to find them from vm tree, which obviously takes less time.
>
> Yeah, but is also much more overhead to maintain.
>
> Since this is only to fix the behavior of a single buggy application at least I'm fine to keep the workaround as simple as this.
>
> If we find a wider use we can still start to use the kernel implementation again.

Thanks you all to clarify that.

May I get RB for this patch?
Then I could work on the patch 4 only.
Thanks.

Regards,
Jerry

>
> Regards,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>> +        bo = handle_table_lookup(&dev->bo_handles, i);
>>> +        if (!bo || !bo->cpu_ptr || size > bo->alloc_size)
>>> +            continue;
>>> +        if (cpu >= bo->cpu_ptr && cpu < (bo->cpu_ptr + bo->alloc_size))
>>> +            break;
>>> +    }
>>> +
>>> +    if (i < dev->bo_handles.max_key) {
>>> +        atomic_inc(&bo->refcount);
>>> +        *buf_handle = bo;
>>> +        *offset_in_bo = cpu - bo->cpu_ptr;
>>> +    } else {
>>> +        *buf_handle = NULL;
>>> +        *offset_in_bo = 0;
>>> +    }
>>> +    pthread_mutex_unlock(&dev->bo_table_mutex);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>>>                       void *cpu,
>>>                       uint64_t size,
>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 2/4] amdgpu: add a function to find bo by cpu mapping (v2)

[zhoucm1]

On 2018年08月08日 14:48, Christian König wrote:
> Am 08.08.2018 um 06:23 schrieb zhoucm1:
>>
>>
>> On 2018年08月08日 12:08, Junwei Zhang wrote:
>>> Userspace needs to know if the user memory is from BO or malloc.
>>>
>>> v2: update mutex range and rebase
>>>
>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>> ---
>>>   amdgpu/amdgpu.h    | 23 +++++++++++++++++++++++
>>>   amdgpu/amdgpu_bo.c | 34 ++++++++++++++++++++++++++++++++++
>>>   2 files changed, 57 insertions(+)
>>>
>>> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
>>> index be83b45..a8c353c 100644
>>> --- a/amdgpu/amdgpu.h
>>> +++ b/amdgpu/amdgpu.h
>>> @@ -678,6 +678,29 @@ int 
>>> amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>>>                       amdgpu_bo_handle *buf_handle);
>>>     /**
>>> + * Validate if the user memory comes from BO
>>> + *
>>> + * \param dev - [in] Device handle. See #amdgpu_device_initialize()
>>> + * \param cpu - [in] CPU address of user allocated memory which we
>>> + * want to map to GPU address space (make GPU accessible)
>>> + * (This address must be correctly aligned).
>>> + * \param size - [in] Size of allocation (must be correctly aligned)
>>> + * \param buf_handle - [out] Buffer handle for the userptr memory
>>> + * if the user memory is not from BO, the buf_handle will be NULL.
>>> + * \param offset_in_bo - [out] offset in this BO for this user memory
>>> + *
>>> + *
>>> + * \return   0 on success\n
>>> + *          <0 - Negative POSIX Error code
>>> + *
>>> +*/
>>> +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
>>> +                  void *cpu,
>>> +                  uint64_t size,
>>> +                  amdgpu_bo_handle *buf_handle,
>>> +                  uint64_t *offset_in_bo);
>>> +
>>> +/**
>>>    * Free previosuly allocated memory
>>>    *
>>>    * \param   dev           - \c [in] Device handle. See 
>>> #amdgpu_device_initialize()
>>> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
>>> index b24e698..a7f0662 100644
>>> --- a/amdgpu/amdgpu_bo.c
>>> +++ b/amdgpu/amdgpu_bo.c
>>> @@ -529,6 +529,40 @@ int amdgpu_bo_wait_for_idle(amdgpu_bo_handle bo,
>>>       }
>>>   }
>>>   +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
>>> +                  void *cpu,
>>> +                  uint64_t size,
>>> +                  amdgpu_bo_handle *buf_handle,
>>> +                  uint64_t *offset_in_bo)
>>> +{
>>> +    int i;
>>> +    struct amdgpu_bo *bo;
>>> +
>>> +    if (cpu == NULL || size == 0)
>>> +        return -EINVAL;
>>> +
>>> +    pthread_mutex_lock(&dev->bo_table_mutex);
>>> +    for (i = 0; i < dev->bo_handles.max_key; i++) {
>> Hi Jerry,
>>
>> As Christian catched before, iterating all BOs of device will 
>> introduce much CPU overhead, this isn't good direction.
>> Since cpu virtual address is per-process, you should go to kernel to 
>> find them from vm tree, which obviously takes less time.
>
> Yeah, but is also much more overhead to maintain.
>
> Since this is only to fix the behavior of a single buggy application 
> at least I'm fine to keep the workaround as simple as this.
I like 'workaround' expression, if Jerry adds 'workaround' comments 
here, I'm ok as well.

Regards,
David Zhou
>
> If we find a wider use we can still start to use the kernel 
> implementation again.
>
> Regards,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>> +        bo = handle_table_lookup(&dev->bo_handles, i);
>>> +        if (!bo || !bo->cpu_ptr || size > bo->alloc_size)
>>> +            continue;
>>> +        if (cpu >= bo->cpu_ptr && cpu < (bo->cpu_ptr + 
>>> bo->alloc_size))
>>> +            break;
>>> +    }
>>> +
>>> +    if (i < dev->bo_handles.max_key) {
>>> +        atomic_inc(&bo->refcount);
>>> +        *buf_handle = bo;
>>> +        *offset_in_bo = cpu - bo->cpu_ptr;
>>> +    } else {
>>> +        *buf_handle = NULL;
>>> +        *offset_in_bo = 0;
>>> +    }
>>> +    pthread_mutex_unlock(&dev->bo_table_mutex);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>>>                       void *cpu,
>>>                       uint64_t size,
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 2/4] amdgpu: add a function to find bo by cpu mapping (v2)

[Christian König]

Am 08.08.2018 um 10:43 schrieb zhoucm1:
>
>
> On 2018年08月08日 14:48, Christian König wrote:
>> Am 08.08.2018 um 06:23 schrieb zhoucm1:
>>>
>>>
>>> On 2018年08月08日 12:08, Junwei Zhang wrote:
>>>> Userspace needs to know if the user memory is from BO or malloc.
>>>>
>>>> v2: update mutex range and rebase
>>>>
>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>>> ---
>>>>   amdgpu/amdgpu.h    | 23 +++++++++++++++++++++++
>>>>   amdgpu/amdgpu_bo.c | 34 ++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 57 insertions(+)
>>>>
>>>> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
>>>> index be83b45..a8c353c 100644
>>>> --- a/amdgpu/amdgpu.h
>>>> +++ b/amdgpu/amdgpu.h
>>>> @@ -678,6 +678,29 @@ int 
>>>> amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>>>>                       amdgpu_bo_handle *buf_handle);
>>>>     /**
>>>> + * Validate if the user memory comes from BO
>>>> + *
>>>> + * \param dev - [in] Device handle. See #amdgpu_device_initialize()
>>>> + * \param cpu - [in] CPU address of user allocated memory which we
>>>> + * want to map to GPU address space (make GPU accessible)
>>>> + * (This address must be correctly aligned).
>>>> + * \param size - [in] Size of allocation (must be correctly aligned)
>>>> + * \param buf_handle - [out] Buffer handle for the userptr memory
>>>> + * if the user memory is not from BO, the buf_handle will be NULL.
>>>> + * \param offset_in_bo - [out] offset in this BO for this user memory
>>>> + *
>>>> + *
>>>> + * \return   0 on success\n
>>>> + *          <0 - Negative POSIX Error code
>>>> + *
>>>> +*/
>>>> +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
>>>> +                  void *cpu,
>>>> +                  uint64_t size,
>>>> +                  amdgpu_bo_handle *buf_handle,
>>>> +                  uint64_t *offset_in_bo);
>>>> +
>>>> +/**
>>>>    * Free previosuly allocated memory
>>>>    *
>>>>    * \param   dev           - \c [in] Device handle. See 
>>>> #amdgpu_device_initialize()
>>>> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
>>>> index b24e698..a7f0662 100644
>>>> --- a/amdgpu/amdgpu_bo.c
>>>> +++ b/amdgpu/amdgpu_bo.c
>>>> @@ -529,6 +529,40 @@ int amdgpu_bo_wait_for_idle(amdgpu_bo_handle bo,
>>>>       }
>>>>   }
>>>>   +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
>>>> +                  void *cpu,
>>>> +                  uint64_t size,
>>>> +                  amdgpu_bo_handle *buf_handle,
>>>> +                  uint64_t *offset_in_bo)
>>>> +{
>>>> +    int i;
>>>> +    struct amdgpu_bo *bo;
>>>> +
>>>> +    if (cpu == NULL || size == 0)
>>>> +        return -EINVAL;
>>>> +
>>>> +    pthread_mutex_lock(&dev->bo_table_mutex);
>>>> +    for (i = 0; i < dev->bo_handles.max_key; i++) {
>>> Hi Jerry,
>>>
>>> As Christian catched before, iterating all BOs of device will 
>>> introduce much CPU overhead, this isn't good direction.
>>> Since cpu virtual address is per-process, you should go to kernel to 
>>> find them from vm tree, which obviously takes less time.
>>
>> Yeah, but is also much more overhead to maintain.
>>
>> Since this is only to fix the behavior of a single buggy application 
>> at least I'm fine to keep the workaround as simple as this.
> I like 'workaround' expression, if Jerry adds 'workaround' comments 
> here, I'm ok as well.

Yeah, agree a code comment is probably a good idea. Something like

/* Workaround for a buggy application which tries to import previously 
exposed CPU pointers.
  * If we find a real world use case we should improve that by asking 
the kernel for the right handle.
  */

With that added the patch is Reviewed-by: Christian König 
<christian.koenig@amd.com>.

Regards,
Christian.

>
> Regards,
> David Zhou
>>
>> If we find a wider use we can still start to use the kernel 
>> implementation again.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> David Zhou
>>>> +        bo = handle_table_lookup(&dev->bo_handles, i);
>>>> +        if (!bo || !bo->cpu_ptr || size > bo->alloc_size)
>>>> +            continue;
>>>> +        if (cpu >= bo->cpu_ptr && cpu < (bo->cpu_ptr + 
>>>> bo->alloc_size))
>>>> +            break;
>>>> +    }
>>>> +
>>>> +    if (i < dev->bo_handles.max_key) {
>>>> +        atomic_inc(&bo->refcount);
>>>> +        *buf_handle = bo;
>>>> +        *offset_in_bo = cpu - bo->cpu_ptr;
>>>> +    } else {
>>>> +        *buf_handle = NULL;
>>>> +        *offset_in_bo = 0;
>>>> +    }
>>>> +    pthread_mutex_unlock(&dev->bo_table_mutex);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>>>>                       void *cpu,
>>>>                       uint64_t size,
>>>
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 2/4] amdgpu: add a function to find bo by cpu mapping (v2)

[Zhang, Jerry (Junwei)]

On 08/08/2018 04:51 PM, Christian König wrote:
> Am 08.08.2018 um 10:43 schrieb zhoucm1:
>>
>>
>> On 2018年08月08日 14:48, Christian König wrote:
>>> Am 08.08.2018 um 06:23 schrieb zhoucm1:
>>>>
>>>>
>>>> On 2018年08月08日 12:08, Junwei Zhang wrote:
>>>>> Userspace needs to know if the user memory is from BO or malloc.
>>>>>
>>>>> v2: update mutex range and rebase
>>>>>
>>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>>>>> ---
>>>>>   amdgpu/amdgpu.h    | 23 +++++++++++++++++++++++
>>>>>   amdgpu/amdgpu_bo.c | 34 ++++++++++++++++++++++++++++++++++
>>>>>   2 files changed, 57 insertions(+)
>>>>>
>>>>> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
>>>>> index be83b45..a8c353c 100644
>>>>> --- a/amdgpu/amdgpu.h
>>>>> +++ b/amdgpu/amdgpu.h
>>>>> @@ -678,6 +678,29 @@ int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>>>>>                       amdgpu_bo_handle *buf_handle);
>>>>>     /**
>>>>> + * Validate if the user memory comes from BO
>>>>> + *
>>>>> + * \param dev - [in] Device handle. See #amdgpu_device_initialize()
>>>>> + * \param cpu - [in] CPU address of user allocated memory which we
>>>>> + * want to map to GPU address space (make GPU accessible)
>>>>> + * (This address must be correctly aligned).
>>>>> + * \param size - [in] Size of allocation (must be correctly aligned)
>>>>> + * \param buf_handle - [out] Buffer handle for the userptr memory
>>>>> + * if the user memory is not from BO, the buf_handle will be NULL.
>>>>> + * \param offset_in_bo - [out] offset in this BO for this user memory
>>>>> + *
>>>>> + *
>>>>> + * \return   0 on success\n
>>>>> + *          <0 - Negative POSIX Error code
>>>>> + *
>>>>> +*/
>>>>> +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
>>>>> +                  void *cpu,
>>>>> +                  uint64_t size,
>>>>> +                  amdgpu_bo_handle *buf_handle,
>>>>> +                  uint64_t *offset_in_bo);
>>>>> +
>>>>> +/**
>>>>>    * Free previosuly allocated memory
>>>>>    *
>>>>>    * \param   dev           - \c [in] Device handle. See #amdgpu_device_initialize()
>>>>> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
>>>>> index b24e698..a7f0662 100644
>>>>> --- a/amdgpu/amdgpu_bo.c
>>>>> +++ b/amdgpu/amdgpu_bo.c
>>>>> @@ -529,6 +529,40 @@ int amdgpu_bo_wait_for_idle(amdgpu_bo_handle bo,
>>>>>       }
>>>>>   }
>>>>>   +int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
>>>>> +                  void *cpu,
>>>>> +                  uint64_t size,
>>>>> +                  amdgpu_bo_handle *buf_handle,
>>>>> +                  uint64_t *offset_in_bo)
>>>>> +{
>>>>> +    int i;
>>>>> +    struct amdgpu_bo *bo;
>>>>> +
>>>>> +    if (cpu == NULL || size == 0)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    pthread_mutex_lock(&dev->bo_table_mutex);
>>>>> +    for (i = 0; i < dev->bo_handles.max_key; i++) {
>>>> Hi Jerry,
>>>>
>>>> As Christian catched before, iterating all BOs of device will introduce much CPU overhead, this isn't good direction.
>>>> Since cpu virtual address is per-process, you should go to kernel to find them from vm tree, which obviously takes less time.
>>>
>>> Yeah, but is also much more overhead to maintain.
>>>
>>> Since this is only to fix the behavior of a single buggy application at least I'm fine to keep the workaround as simple as this.
>> I like 'workaround' expression, if Jerry adds 'workaround' comments here, I'm ok as well.
>
> Yeah, agree a code comment is probably a good idea. Something like
>
> /* Workaround for a buggy application which tries to import previously exposed CPU pointers.
>   * If we find a real world use case we should improve that by asking the kernel for the right handle.
>   */
>
> With that added the patch is Reviewed-by: Christian König <christian.koenig@amd.com>.

Thanks you all, will add that comments.

BTW, Christian, SRDC still has no permission to get the new drm repo,
since git port is blocked by local network manager.

I could prepare those patches in gfx mail list, anyone will help to push them?

Regards,
Jerry

>
> Regards,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>>
>>> If we find a wider use we can still start to use the kernel implementation again.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Regards,
>>>> David Zhou
>>>>> +        bo = handle_table_lookup(&dev->bo_handles, i);
>>>>> +        if (!bo || !bo->cpu_ptr || size > bo->alloc_size)
>>>>> +            continue;
>>>>> +        if (cpu >= bo->cpu_ptr && cpu < (bo->cpu_ptr + bo->alloc_size))
>>>>> +            break;
>>>>> +    }
>>>>> +
>>>>> +    if (i < dev->bo_handles.max_key) {
>>>>> +        atomic_inc(&bo->refcount);
>>>>> +        *buf_handle = bo;
>>>>> +        *offset_in_bo = cpu - bo->cpu_ptr;
>>>>> +    } else {
>>>>> +        *buf_handle = NULL;
>>>>> +        *offset_in_bo = 0;
>>>>> +    }
>>>>> +    pthread_mutex_unlock(&dev->bo_table_mutex);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>>>>>                       void *cpu,
>>>>>                       uint64_t size,
>>>>
>>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 3/4] tests/amdgpu: add test for finding bo by CPU mapping

[Junwei Zhang]

Add a test for API to query bo by CPU mapping

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 tests/amdgpu/bo_tests.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tests/amdgpu/bo_tests.c b/tests/amdgpu/bo_tests.c
index 9d4da4a..dc2de9b 100644
--- a/tests/amdgpu/bo_tests.c
+++ b/tests/amdgpu/bo_tests.c
@@ -27,6 +27,7 @@
 
 #include "amdgpu_test.h"
 #include "amdgpu_drm.h"
+#include "amdgpu_internal.h"
 
 #define BUFFER_SIZE (4*1024)
 #define BUFFER_ALIGN (4*1024)
@@ -44,6 +45,7 @@ static void amdgpu_bo_metadata(void);
 static void amdgpu_bo_map_unmap(void);
 static void amdgpu_memory_alloc(void);
 static void amdgpu_mem_fail_alloc(void);
+static void amdgpu_bo_find_by_cpu_mapping(void);
 
 CU_TestInfo bo_tests[] = {
 	{ "Export/Import",  amdgpu_bo_export_import },
@@ -51,6 +53,7 @@ CU_TestInfo bo_tests[] = {
 	{ "CPU map/unmap",  amdgpu_bo_map_unmap },
 	{ "Memory alloc Test",  amdgpu_memory_alloc },
 	{ "Memory fail alloc Test",  amdgpu_mem_fail_alloc },
+	{ "Find bo by CPU mapping",  amdgpu_bo_find_by_cpu_mapping },
 	CU_TEST_INFO_NULL,
 };
 
@@ -262,3 +265,33 @@ static void amdgpu_mem_fail_alloc(void)
 		CU_ASSERT_EQUAL(r, 0);
 	}
 }
+
+static void amdgpu_bo_find_by_cpu_mapping(void)
+{
+	amdgpu_bo_handle bo_handle, find_bo_handle;
+	amdgpu_va_handle va_handle;
+	void *bo_cpu;
+	uint64_t bo_mc_address;
+	uint64_t offset;
+	int r;
+
+	r = amdgpu_bo_alloc_and_map(device_handle, 4096, 4096,
+				    AMDGPU_GEM_DOMAIN_GTT, 0,
+				    &bo_handle, &bo_cpu,
+				    &bo_mc_address, &va_handle);
+	CU_ASSERT_EQUAL(r, 0);
+
+	r = amdgpu_find_bo_by_cpu_mapping(device_handle,
+					  bo_cpu,
+					  4096,
+					  &find_bo_handle,
+					  &offset);
+	CU_ASSERT_EQUAL(r, 0);
+	CU_ASSERT_EQUAL(offset, 0);
+	CU_ASSERT_EQUAL(bo_handle->handle, find_bo_handle->handle);
+
+	atomic_dec(&find_bo_handle->refcount, 1);
+	r = amdgpu_bo_unmap_and_free(bo_handle, va_handle,
+				     bo_mc_address, 4096);
+	CU_ASSERT_EQUAL(r, 0);
+}
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Junwei Zhang]

a helper function to create and initialize amdgpu bo

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Christian König]

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?

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

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

[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?

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

Re: [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@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