Thomas Zimmermann
2021-Apr-06 11:55 UTC
[Nouveau] [PATCH 3/8] drm/amdgpu: Implement mmap as GEM object function
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. 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-- 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/6c322c25/attachment.sig>
Christian König
2021-Apr-06 12:42 UTC
[Nouveau] [PATCH 3/8] drm/amdgpu: Implement mmap as GEM object function
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. 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 >