Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

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

On 07/31/2018 04:13 PM, Christian König wrote:
> Am 31.07.2018 um 10:05 schrieb Zhang, Jerry (Junwei):
>> On 07/31/2018 03:03 PM, Christian König wrote:
>>> Am 31.07.2018 um 08:58 schrieb Zhang, Jerry (Junwei):
>>>> On 07/30/2018 06:47 PM, Christian König wrote:
>>>>> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>>>>>> From: Chunming Zhou <David1.Zhou@amd.com>
>>>>>>
>>>>>> v2: get original gem handle from gobj
>>>>>> v3: update find bo data structure as union(in, out)
>>>>>>      simply some code logic
>>>>>
>>>>> Do we now have an open source user for this, so that we can upstream it? One more point below.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>>> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 +++++++++++++++++++++++++++++++++
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>>>>>>   include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>>>>>>   4 files changed, 88 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> index 4cd20e7..46c370b 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device *dev, void *data,
>>>>>>                 struct drm_file *filp);
>>>>>>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>>>>>               struct drm_file *filp);
>>>>>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
>>>>>> +                        struct drm_file *filp);
>>>>>>   int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>>>>>>                 struct drm_file *filp);
>>>>>>   int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> index 71792d8..bae8417 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>>>>>       return 0;
>>>>>>   }
>>>>>> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
>>>>>> +                         struct drm_gem_object *obj)
>>>>>> +{
>>>>>> +    int i;
>>>>>> +    struct drm_gem_object *tmp;
>>>>>> +
>>>>>> +    spin_lock(&filp->table_lock);
>>>>>> +    idr_for_each_entry(&filp->object_idr, tmp, i) {
>>>>>> +        if (obj == tmp) {
>>>>>> +            drm_gem_object_reference(obj);
>>>>>> +            spin_unlock(&filp->table_lock);
>>>>>> +            return i;
>>>>>> +        }
>>>>>> +    }
>>>>>
>>>>> Please double check if that is still up to date.
>>>>
>>>> We may have to replace drm_gem_object_reference() with drm_gem_object_get().
>>>>
>>>> On 2nd thought, do we really need to do reference every time?
>>>
>>> Yes, that's a must have. Otherwise the handle could be freed and reused already when we return.
>>>
>>>> if UMD find the same gem object for 3 times, it also need to explicitly free(put) that object for 3 times?
>>>
>>> Correct yes. Thinking more about this the real problem is to translate the handle into a structure in libdrm.
>>>
>>> Here we are back to the problem Marek and Michel has been working on for a while that we always need to be able to translate a handle into a bo structure.....
>>>
>>> So that needs to be solved before we can upstream the changes.
>>
>> Thanks for your info.
>> It's better to fix that before upstream.
>
> Thinking more about this the hash currently used in libdrm is not adequate any more.
>
> E.g. we now need to be able to find all BOs based on their handle. Since the handles are dense either an r/b tree or a radix tree now sounds like the best approach to me.

Not sure the exact reason that we added hash table in libdrm.
But it really costs much less time than calling IOCTL to find BO by their handles.

In this case, UMD seems not to be able to get BO handle and try to verify it by cpu address then.
In another word, UMD would like to find if the memory is created as BO or system memory, I suppose.

Regards,
Jerry


>
> Christian.
>
>>
>> Regards,
>> Jerry
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx