Nouveau - Apr 2021 - [PATCH 3/8] drm/amdgpu: Implement mmap as GEM object function

Hi

Am 06.04.21 um 14:42 schrieb Christian K?nig:
> Hi Thomas,
> 
> Am 06.04.21 um 13:55 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 06.04.21 um 12:56 schrieb Christian K?nig:
>>>>
>>>> In the end I went with the semantics I found in amdgpu_mmap()
and
>>>> handled KFD specially. Let me know if this requires to be
changed.
>>>
>>> Well the question is where is the call to 
>>> drm_vma_node_verify_access() now? Cause that needs to be skipped
for
>>> KFD BOs.
>>
>> I see. It's now drm_vma_node_is_allowed(); called by
drm_gem_mmap().
>> [1] So drm_gem_mmap() cannot be used by amdgpu.
>>
>> If I understand the code at [2] correctly, KFD objects don't use
the
>> GEM ioctl interfaces, but they still use the internal GEM object that 
>> is part of the TTM BO. In this case, amdgpu could have its own version 
>> of drm_gem_mmap(), which calls drm_gem_mmap_obj(), [3] which in turn 
>> handles the mmap details via GEM object functions.
> 
> Correct, well we could cleanup the KFD to use the GEM functions as well.
The KFD code already calls amdgpu_gem_object_create(). It should have 
the object-functions pointer set for use with mmap. Not sure what the 
use of drm_vma_node_is_allowed() would involve.

Best regards
Thomas
> 
> Felix what exactly was your objections to using this?
> 
> Regards,
> Christian.
> 
>>
>> drm_gem_prime_mmap() doesn't do any additional verification.
>>
>> Best regards
>> Thomas
>>
>> [1] 
>>
https://elixir.bootlin.com/linux/v5.11.11/source/drivers/gpu/drm/drm_gem.c#L1156
>>
>> [2] 
>>
https://elixir.bootlin.com/linux/v5.11.11/source/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c#L1224
>>
>> [3] 
>>
https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/gpu/drm/drm_gem.c#L1053
>>
>>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>> -
>>>>>> ? int amdgpu_copy_buffer(struct amdgpu_ring *ring,
uint64_t
>>>>>> src_offset,
>>>>>> ???????????????? uint64_t dst_offset, uint32_t
byte_count,
>>>>>> ???????????????? struct dma_resv *resv,
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>>>> index dec0db8b0b13..6e51faad7371 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>>>> @@ -146,7 +146,6 @@ int amdgpu_fill_buffer(struct
amdgpu_bo *bo,
>>>>>> ????????????? struct dma_resv *resv,
>>>>>> ????????????? struct dma_fence **fence);
>>>>>> -int amdgpu_mmap(struct file *filp, struct
vm_area_struct *vma);
>>>>>> ? int amdgpu_ttm_alloc_gart(struct ttm_buffer_object
*bo);
>>>>>> ? int amdgpu_ttm_recover_gart(struct ttm_buffer_object
*tbo);
>>>>>> ? uint64_t amdgpu_ttm_domain_start(struct amdgpu_device
*adev,
>>>>>> uint32_t type);
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 N?rnberg, Germany
(HRB 36809, AG N?rnberg)
Gesch?ftsf?hrer: Felix Imend?rffer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20210406/762bc74c/attachment.sig>

Am 06.04.21 um 14:52 schrieb Thomas Zimmermann:
> Hi
>
> Am 06.04.21 um 14:42 schrieb Christian K?nig:
>> Hi Thomas,
>>
>> Am 06.04.21 um 13:55 schrieb Thomas Zimmermann:
>>> Hi
>>>
>>> Am 06.04.21 um 12:56 schrieb Christian K?nig:
>>>>>
>>>>> In the end I went with the semantics I found in
amdgpu_mmap() and
>>>>> handled KFD specially. Let me know if this requires to be
changed.
>>>>
>>>> Well the question is where is the call to 
>>>> drm_vma_node_verify_access() now? Cause that needs to be
skipped
>>>> for KFD BOs.
>>>
>>> I see. It's now drm_vma_node_is_allowed(); called by
drm_gem_mmap().
>>> [1] So drm_gem_mmap() cannot be used by amdgpu.
>>>
>>> If I understand the code at [2] correctly, KFD objects don't
use the
>>> GEM ioctl interfaces, but they still use the internal GEM object 
>>> that is part of the TTM BO. In this case, amdgpu could have its own
>>> version of drm_gem_mmap(), which calls drm_gem_mmap_obj(), [3]
which
>>> in turn handles the mmap details via GEM object functions.
>>
>> Correct, well we could cleanup the KFD to use the GEM functions as
well.
>
> The KFD code already calls amdgpu_gem_object_create(). It should have 
> the object-functions pointer set for use with mmap. Not sure what the 
> use of drm_vma_node_is_allowed() would involve.
The KFD allows BOs to be mmaped with different offsets than what's used 
in the DRM node.

So drm_vma_node_is_allowed() would return false as far as I know.

Regards,
Christian.
>
> Best regards
> Thomas
>
>>
>> Felix what exactly was your objections to using this?
>>
>> Regards,
>> Christian.
>>
>>>
>>> drm_gem_prime_mmap() doesn't do any additional verification.
>>>
>>> Best regards
>>> Thomas
>>>
>>> [1] 
>>>
https://elixir.bootlin.com/linux/v5.11.11/source/drivers/gpu/drm/drm_gem.c#L1156
>>>
>>> [2] 
>>>
https://elixir.bootlin.com/linux/v5.11.11/source/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c#L1224
>>>
>>> [3] 
>>>
https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/gpu/drm/drm_gem.c#L1053
>>>
>>>
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>>>> -
>>>>>>> ? int amdgpu_copy_buffer(struct amdgpu_ring *ring,
uint64_t
>>>>>>> src_offset,
>>>>>>> ???????????????? uint64_t dst_offset, uint32_t
byte_count,
>>>>>>> ???????????????? struct dma_resv *resv,
>>>>>>> diff --git
a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>>>>> index dec0db8b0b13..6e51faad7371 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>>>>> @@ -146,7 +146,6 @@ int amdgpu_fill_buffer(struct
amdgpu_bo *bo,
>>>>>>> ????????????? struct dma_resv *resv,
>>>>>>> ????????????? struct dma_fence **fence);
>>>>>>> -int amdgpu_mmap(struct file *filp, struct
vm_area_struct *vma);
>>>>>>> ? int amdgpu_ttm_alloc_gart(struct
ttm_buffer_object *bo);
>>>>>>> ? int amdgpu_ttm_recover_gart(struct
ttm_buffer_object *tbo);
>>>>>>> ? uint64_t amdgpu_ttm_domain_start(struct
amdgpu_device *adev,
>>>>>>> uint32_t type);
>>>>>>
>>>>>> _______________________________________________
>>>>>> dri-devel mailing list
>>>>>> dri-devel at lists.freedesktop.org
>>>>>>
https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>