Hi

Am 06.04.21 um 11:43 schrieb Christian König:

Am 06.04.21 um 11:08 schrieb Thomas Zimmermann:

...

Remove an unused function. Mapping the fbdev framebuffer is apparently not supported.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Reviewed-by: Christian König christian.koenig@amd.com

Should I just upstream this through our internal branches?

I guess you can pick up this patch into your branches if that's easier for you. For the other patches, I'd like to merge them via drm-misc-next.

Best regards Thomas

Thanks, Christian.

...

---

drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 19 -------------------   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 --   2 files changed, 21 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index b99e9d8736c2..cfc89164dee8 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -1092,25 +1092,6 @@ void amdgpu_bo_fini(struct amdgpu_device *adev)       }   } -/**

• • amdgpu_bo_fbdev_mmap - mmap fbdev memory
• • @bo: &amdgpu_bo buffer object
• • @vma: vma as input from the fbdev mmap method
• • Calls ttm_fbdev_mmap() to mmap fbdev memory if it is backed by a bo.
• • Returns:
• • 0 for success or a negative error code on failure.
• */

-int amdgpu_bo_fbdev_mmap(struct amdgpu_bo *bo, -                 struct vm_area_struct *vma) -{ -    if (vma->vm_pgoff != 0) -        return -EACCES;

-    return ttm_bo_mmap_obj(vma, &bo->tbo); -}

/**    * amdgpu_bo_set_tiling_flags - set tiling flags    * @bo: &amdgpu_bo buffer object diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h index 54ceb065e546..46e94d413c5c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h @@ -268,8 +268,6 @@ void amdgpu_bo_unpin(struct amdgpu_bo *bo);   int amdgpu_bo_evict_vram(struct amdgpu_device *adev);   int amdgpu_bo_init(struct amdgpu_device *adev);   void amdgpu_bo_fini(struct amdgpu_device *adev); -int amdgpu_bo_fbdev_mmap(struct amdgpu_bo *bo, -                struct vm_area_struct *vma);   int amdgpu_bo_set_tiling_flags(struct amdgpu_bo *bo, u64 tiling_flags);   void amdgpu_bo_get_tiling_flags(struct amdgpu_bo *bo, u64 *tiling_flags);   int amdgpu_bo_set_metadata (struct amdgpu_bo *bo, void *metadata,

Am 06.04.21 um 11:08 schrieb Thomas Zimmermann:

Moving the driver-specific mmap code into a GEM object function allows for using DRM helpers for various mmap callbacks.

This change resolves several inconsistencies between regular mmap and prime-based mmap. The vm_ops field in vma is now set for all mmap'ed areas. Previously it way only set for regular mmap calls, prime-based mmap used TTM's default vm_ops. The check for kfd_bo has been taken from amdgpu_verify_access(), which is not called any longer and has been removed.

As a side effect, amdgpu_ttm_vm_ops and amdgpu_ttm_fault() are now implemented in amdgpu's GEM code.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 46 ------------- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h | 2 - drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 64 +++++++++++++++++++ drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 71 --------------------- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 1 - 6 files changed, 66 insertions(+), 122 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index e0c4f7c7f1b9..19c5ab08d9ec 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -42,52 +42,6 @@ #include <linux/pci-p2pdma.h> #include <linux/pm_runtime.h>

-/**

• • amdgpu_gem_prime_mmap - &drm_driver.gem_prime_mmap implementation
• • @obj: GEM BO
• • @vma: Virtual memory area
• • Sets up a userspace mapping of the BO's memory in the given
• • virtual memory area.
• • Returns:
• • 0 on success or a negative error code on failure.
• */

-int amdgpu_gem_prime_mmap(struct drm_gem_object *obj,

• 	  struct vm_area_struct *vma)
  

-{

• struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
• struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
• unsigned asize = amdgpu_bo_size(bo);
• int ret;
• if (!vma->vm_file)

• return -ENODEV;
  

• if (adev == NULL)

• return -ENODEV;
  

• /* Check for valid size. */
• if (asize < vma->vm_end - vma->vm_start)

• return -EINVAL;
  

• if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) ||

•    (bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)) {
  

• return -EPERM;
  

• }
• vma->vm_pgoff += amdgpu_bo_mmap_offset(bo) >> PAGE_SHIFT;
• /* prime mmap does not need to check access, so allow here */
• ret = drm_vma_node_allow(&obj->vma_node, vma->vm_file->private_data);
• if (ret)

• return ret;
  

• ret = ttm_bo_mmap(vma->vm_file, vma, &adev->mman.bdev);
• drm_vma_node_revoke(&obj->vma_node, vma->vm_file->private_data);
• return ret;

-}

• static int __dma_resv_make_exclusive(struct dma_resv *obj) {

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h index 39b5b9616fd8..3e93b9b407a9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h @@ -31,8 +31,6 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf); bool amdgpu_dmabuf_is_xgmi_accessible(struct amdgpu_device *adev, struct amdgpu_bo *bo); -int amdgpu_gem_prime_mmap(struct drm_gem_object *obj,

• 	  struct vm_area_struct *vma);
  
  

  extern const struct dma_buf_ops amdgpu_dmabuf_ops;

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 76f48f79c70b..e96d2758f4bb 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1656,7 +1656,7 @@ static const struct file_operations amdgpu_driver_kms_fops = { .flush = amdgpu_flush, .release = drm_release, .unlocked_ioctl = amdgpu_drm_ioctl,

• .mmap = amdgpu_mmap,

• .mmap = drm_gem_mmap, .poll = drm_poll, .read = drm_read, #ifdef CONFIG_COMPAT

@@ -1719,7 +1719,7 @@ static const struct drm_driver amdgpu_kms_driver = { .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_import = amdgpu_gem_prime_import,

• .gem_prime_mmap = amdgpu_gem_prime_mmap,

• .gem_prime_mmap = drm_gem_prime_mmap,

  .name = DRIVER_NAME, .desc = DRIVER_DESC,

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index fb7171e5507c..fe93faad05f2 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -41,6 +41,36 @@

static const struct drm_gem_object_funcs amdgpu_gem_object_funcs;

+static vm_fault_t amdgpu_ttm_fault(struct vm_fault *vmf)

Please name that function amdgpu_gem_fault or amdgpu_gem_object_fault

+{

• struct ttm_buffer_object *bo = vmf->vma->vm_private_data;
• vm_fault_t ret;
• ret = ttm_bo_vm_reserve(bo, vmf);
• if (ret)

• return ret;
  

• ret = amdgpu_bo_fault_reserve_notify(bo);
• if (ret)

• goto unlock;
  

• ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,

• 		       TTM_BO_VM_NUM_PREFAULT, 1);
  

• if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))

• return ret;
  

+unlock:

• dma_resv_unlock(bo->base.resv);
• return ret;

+}

+static const struct vm_operations_struct amdgpu_ttm_vm_ops = {

• .fault = amdgpu_ttm_fault,
• .open = ttm_bo_vm_open,
• .close = ttm_bo_vm_close,
• .access = ttm_bo_vm_access

+};

• static void amdgpu_gem_object_free(struct drm_gem_object *gobj) { struct amdgpu_bo *robj = gem_to_amdgpu_bo(gobj);

@@ -201,6 +231,38 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj, ttm_eu_backoff_reservation(&ticket, &list); }

+static int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) +{

• struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
• struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
• unsigned long asize = amdgpu_bo_size(bo);
• if (!vma->vm_file)

• return -ENODEV;
  

• if (!adev)

• return -ENODEV;
  

• /* Check for valid size. */
• if (asize < vma->vm_end - vma->vm_start)

• return -EINVAL;
  

• /*

• * Don't verify access for KFD BOs. They don't have a GEM
  

• * object associated with them.
  

• */
  

• if (bo->kfd_bo)

• goto out;
  

Who does the access verification now?

Christian.

• if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) ||

•    (bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)) {
  

• return -EPERM;
  

• }

+out:

• return drm_gem_ttm_mmap(obj, vma);

+}

• static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = { .free = amdgpu_gem_object_free, .open = amdgpu_gem_object_open,

@@ -208,6 +270,8 @@ static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = { .export = amdgpu_gem_prime_export, .vmap = drm_gem_ttm_vmap, .vunmap = drm_gem_ttm_vunmap,

• .mmap = amdgpu_gem_prime_mmap,

• .vm_ops = &amdgpu_ttm_vm_ops, };

  /*

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 1c6131489a85..d9de91a517c6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -152,32 +152,6 @@ static void amdgpu_evict_flags(struct ttm_buffer_object *bo, *placement = abo->placement; }

-/**

• • amdgpu_verify_access - Verify access for a mmap call
• • @bo: The buffer object to map
• • @filp: The file pointer from the process performing the mmap
• • This is called by ttm_bo_mmap() to verify whether a process
• • has the right to mmap a BO to their process space.
• */

-static int amdgpu_verify_access(struct ttm_buffer_object *bo, struct file *filp) -{

• struct amdgpu_bo *abo = ttm_to_amdgpu_bo(bo);
• /*

• * Don't verify access for KFD BOs. They don't have a GEM
  

• * object associated with them.
  

• */
  

• if (abo->kfd_bo)

• return 0;
  

• if (amdgpu_ttm_tt_get_usermm(bo->ttm))

• return -EPERM;
  

• return drm_vma_node_verify_access(&abo->tbo.base.vma_node,

• 			  filp->private_data);
  

-}

• /**
  • amdgpu_ttm_map_buffer - Map memory into the GART windows
  • @bo: buffer object to map

@@ -1531,7 +1505,6 @@ static struct ttm_device_funcs amdgpu_bo_driver = { .eviction_valuable = amdgpu_ttm_bo_eviction_valuable, .evict_flags = &amdgpu_evict_flags, .move = &amdgpu_bo_move,

• .verify_access = &amdgpu_verify_access, .delete_mem_notify = &amdgpu_bo_delete_mem_notify, .release_notify = &amdgpu_bo_release_notify, .io_mem_reserve = &amdgpu_ttm_io_mem_reserve,

@@ -1906,50 +1879,6 @@ void amdgpu_ttm_set_buffer_funcs_status(struct amdgpu_device *adev, bool enable) adev->mman.buffer_funcs_enabled = enable; }

-static vm_fault_t amdgpu_ttm_fault(struct vm_fault *vmf) -{

• struct ttm_buffer_object *bo = vmf->vma->vm_private_data;
• vm_fault_t ret;
• ret = ttm_bo_vm_reserve(bo, vmf);
• if (ret)

• return ret;
  

• ret = amdgpu_bo_fault_reserve_notify(bo);
• if (ret)

• goto unlock;
  

• ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,

• 		       TTM_BO_VM_NUM_PREFAULT, 1);
  

• if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))

• return ret;
  

-unlock:

• dma_resv_unlock(bo->base.resv);
• return ret;

-}

-static const struct vm_operations_struct amdgpu_ttm_vm_ops = {

• .fault = amdgpu_ttm_fault,
• .open = ttm_bo_vm_open,
• .close = ttm_bo_vm_close,
• .access = ttm_bo_vm_access

-};

-int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma) -{

• struct drm_file *file_priv = filp->private_data;
• struct amdgpu_device *adev = drm_to_adev(file_priv->minor->dev);
• int r;
• r = ttm_bo_mmap(filp, vma, &adev->mman.bdev);
• if (unlikely(r != 0))

• return r;
  

• vma->vm_ops = &amdgpu_ttm_vm_ops;
• return 0;

-}

• 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);

Hi Thomas,

Am 06.04.21 um 12:38 schrieb Thomas Zimmermann:

Hi

Am 06.04.21 um 11:35 schrieb Christian König:

...

Am 06.04.21 um 11:08 schrieb Thomas Zimmermann:

...

Moving the driver-specific mmap code into a GEM object function allows for using DRM helpers for various mmap callbacks.

This change resolves several inconsistencies between regular mmap and prime-based mmap. The vm_ops field in vma is now set for all mmap'ed areas. Previously it way only set for regular mmap calls, prime-based mmap used TTM's default vm_ops. The check for kfd_bo has been taken from amdgpu_verify_access(), which is not called any longer and has been removed.

As a side effect, amdgpu_ttm_vm_ops and amdgpu_ttm_fault() are now implemented in amdgpu's GEM code.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 46 -------------   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h |  2 -   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |  4 +-   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c     | 64 +++++++++++++++++++   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     | 71

---

drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |  1 -   6 files changed, 66 insertions(+), 122 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index e0c4f7c7f1b9..19c5ab08d9ec 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -42,52 +42,6 @@   #include <linux/pci-p2pdma.h>   #include <linux/pm_runtime.h> -/**

• • amdgpu_gem_prime_mmap - &drm_driver.gem_prime_mmap implementation
• • @obj: GEM BO
• • @vma: Virtual memory area
• • Sets up a userspace mapping of the BO's memory in the given
• • virtual memory area.
• • Returns:
• • 0 on success or a negative error code on failure.
• */

-int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, -              struct vm_area_struct *vma) -{ -    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); -    struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); -    unsigned asize = amdgpu_bo_size(bo); -    int ret;

-    if (!vma->vm_file) -        return -ENODEV;

-    if (adev == NULL) -        return -ENODEV;

-    /* Check for valid size. */ -    if (asize < vma->vm_end - vma->vm_start) -        return -EINVAL;

-    if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) || -        (bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)) { -        return -EPERM; -    } -    vma->vm_pgoff += amdgpu_bo_mmap_offset(bo) >> PAGE_SHIFT;

-    /* prime mmap does not need to check access, so allow here */ -    ret = drm_vma_node_allow(&obj->vma_node, vma->vm_file->private_data); -    if (ret) -        return ret;

-    ret = ttm_bo_mmap(vma->vm_file, vma, &adev->mman.bdev); -    drm_vma_node_revoke(&obj->vma_node, vma->vm_file->private_data);

-    return ret; -}

static int   __dma_resv_make_exclusive(struct dma_resv *obj)   { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h index 39b5b9616fd8..3e93b9b407a9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h @@ -31,8 +31,6 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,                           struct dma_buf *dma_buf);   bool amdgpu_dmabuf_is_xgmi_accessible(struct amdgpu_device *adev,                         struct amdgpu_bo *bo); -int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, -              struct vm_area_struct *vma);   extern const struct dma_buf_ops amdgpu_dmabuf_ops; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 76f48f79c70b..e96d2758f4bb 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1656,7 +1656,7 @@ static const struct file_operations amdgpu_driver_kms_fops = {       .flush = amdgpu_flush,       .release = drm_release,       .unlocked_ioctl = amdgpu_drm_ioctl, -    .mmap = amdgpu_mmap, +    .mmap = drm_gem_mmap,       .poll = drm_poll,       .read = drm_read,   #ifdef CONFIG_COMPAT @@ -1719,7 +1719,7 @@ static const struct drm_driver amdgpu_kms_driver = {       .prime_handle_to_fd = drm_gem_prime_handle_to_fd,       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,       .gem_prime_import = amdgpu_gem_prime_import, -    .gem_prime_mmap = amdgpu_gem_prime_mmap, +    .gem_prime_mmap = drm_gem_prime_mmap,       .name = DRIVER_NAME,       .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index fb7171e5507c..fe93faad05f2 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -41,6 +41,36 @@   static const struct drm_gem_object_funcs amdgpu_gem_object_funcs; +static vm_fault_t amdgpu_ttm_fault(struct vm_fault *vmf)

Please name that function amdgpu_gem_fault or amdgpu_gem_object_fault

...

+{ +    struct ttm_buffer_object *bo = vmf->vma->vm_private_data; +    vm_fault_t ret;

+    ret = ttm_bo_vm_reserve(bo, vmf); +    if (ret) +        return ret;

+    ret = amdgpu_bo_fault_reserve_notify(bo); +    if (ret) +        goto unlock;

+    ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot, +                       TTM_BO_VM_NUM_PREFAULT, 1); +    if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) +        return ret;

+unlock: +    dma_resv_unlock(bo->base.resv); +    return ret; +}

+static const struct vm_operations_struct amdgpu_ttm_vm_ops = { +    .fault = amdgpu_ttm_fault, +    .open = ttm_bo_vm_open, +    .close = ttm_bo_vm_close, +    .access = ttm_bo_vm_access +};

static void amdgpu_gem_object_free(struct drm_gem_object *gobj)   {       struct amdgpu_bo *robj = gem_to_amdgpu_bo(gobj); @@ -201,6 +231,38 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,       ttm_eu_backoff_reservation(&ticket, &list);   } +static int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) +{ +    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); +    struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); +    unsigned long asize = amdgpu_bo_size(bo);

+    if (!vma->vm_file) +        return -ENODEV;

+    if (!adev) +        return -ENODEV;

+    /* Check for valid size. */ +    if (asize < vma->vm_end - vma->vm_start) +        return -EINVAL;

...

+    /* +     * Don't verify access for KFD BOs. They don't have a GEM +     * object associated with them. +     */ +    if (bo->kfd_bo) +        goto out;

Who does the access verification now?

This is somewhat confusing.

I took this check as-is, including the comment, from amdgpu's verify_access function. The verify_access function was called by ttm_bo_mmap. It returned 0 and ttm_bo_mmap did the mapping.

...

Christian.

...

+    if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) || +        (bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)) { +        return -EPERM; +    }

+out: +    return drm_gem_ttm_mmap(obj, vma); +}

static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {       .free = amdgpu_gem_object_free,       .open = amdgpu_gem_object_open, @@ -208,6 +270,8 @@ static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {       .export = amdgpu_gem_prime_export,       .vmap = drm_gem_ttm_vmap,       .vunmap = drm_gem_ttm_vunmap, +    .mmap = amdgpu_gem_prime_mmap, +    .vm_ops = &amdgpu_ttm_vm_ops,   };   /* diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 1c6131489a85..d9de91a517c6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -152,32 +152,6 @@ static void amdgpu_evict_flags(struct ttm_buffer_object *bo,       *placement = abo->placement;   } -/**

• • amdgpu_verify_access - Verify access for a mmap call
• • @bo:    The buffer object to map
• • @filp: The file pointer from the process performing the mmap
• • This is called by ttm_bo_mmap() to verify whether a process
• • has the right to mmap a BO to their process space.
• */

-static int amdgpu_verify_access(struct ttm_buffer_object *bo, struct file *filp) -{ -    struct amdgpu_bo *abo = ttm_to_amdgpu_bo(bo);

-    /* -     * Don't verify access for KFD BOs. They don't have a GEM -     * object associated with them. -     */ -    if (abo->kfd_bo) -        return 0;

-    if (amdgpu_ttm_tt_get_usermm(bo->ttm)) -        return -EPERM; -    return drm_vma_node_verify_access(&abo->tbo.base.vma_node, -                      filp->private_data); -}

Here's the orignal verification code. It gives a free pass to KFD.

...

...

/**    * amdgpu_ttm_map_buffer - Map memory into the GART windows    * @bo: buffer object to map @@ -1531,7 +1505,6 @@ static struct ttm_device_funcs amdgpu_bo_driver = {       .eviction_valuable = amdgpu_ttm_bo_eviction_valuable,       .evict_flags = &amdgpu_evict_flags,       .move = &amdgpu_bo_move, -    .verify_access = &amdgpu_verify_access,       .delete_mem_notify = &amdgpu_bo_delete_mem_notify,       .release_notify = &amdgpu_bo_release_notify,       .io_mem_reserve = &amdgpu_ttm_io_mem_reserve, @@ -1906,50 +1879,6 @@ void amdgpu_ttm_set_buffer_funcs_status(struct amdgpu_device *adev, bool enable)       adev->mman.buffer_funcs_enabled = enable;   } -static vm_fault_t amdgpu_ttm_fault(struct vm_fault *vmf) -{ -    struct ttm_buffer_object *bo = vmf->vma->vm_private_data; -    vm_fault_t ret;

-    ret = ttm_bo_vm_reserve(bo, vmf); -    if (ret) -        return ret;

-    ret = amdgpu_bo_fault_reserve_notify(bo); -    if (ret) -        goto unlock;

-    ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot, -                       TTM_BO_VM_NUM_PREFAULT, 1); -    if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) -        return ret;

-unlock: -    dma_resv_unlock(bo->base.resv); -    return ret; -}

-static const struct vm_operations_struct amdgpu_ttm_vm_ops = { -    .fault = amdgpu_ttm_fault, -    .open = ttm_bo_vm_open, -    .close = ttm_bo_vm_close, -    .access = ttm_bo_vm_access -};

-int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma) -{ -    struct drm_file *file_priv = filp->private_data; -    struct amdgpu_device *adev = drm_to_adev(file_priv->minor->dev); -    int r;

-    r = ttm_bo_mmap(filp, vma, &adev->mman.bdev); -    if (unlikely(r != 0)) -        return r;

-    vma->vm_ops = &amdgpu_ttm_vm_ops; -    return 0; -}

And this was the mmap callback in struct file_operations. It calls ttm_bo_mmap(), which skips verification for KFD BOs. To the best of my knowledge, there was no additional verification for these KFD BOs.

The original code in amdgpu_gem_prime_mmap() did seom verification, but didn't handle KFD specially. I guess, PRIME needs GEM and KFD BOs wouldn't quailify.

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.

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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

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#L... [2] https://elixir.bootlin.com/linux/v5.11.11/source/drivers/gpu/drm/amd/amdgpu/... [3] https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/gpu/drm/drm_gem.c#...

...

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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

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#L...

[2] https://elixir.bootlin.com/linux/v5.11.11/source/drivers/gpu/drm/amd/amdgpu/...

[3] https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/gpu/drm/drm_gem.c#...

...

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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Am 2021-04-06 um 6:38 a.m. schrieb Thomas Zimmermann:

Hi

Am 06.04.21 um 11:35 schrieb Christian König:

...

Am 06.04.21 um 11:08 schrieb Thomas Zimmermann:

...

Moving the driver-specific mmap code into a GEM object function allows for using DRM helpers for various mmap callbacks.

This change resolves several inconsistencies between regular mmap and prime-based mmap. The vm_ops field in vma is now set for all mmap'ed areas. Previously it way only set for regular mmap calls, prime-based mmap used TTM's default vm_ops. The check for kfd_bo has been taken from amdgpu_verify_access(), which is not called any longer and has been removed.

As a side effect, amdgpu_ttm_vm_ops and amdgpu_ttm_fault() are now implemented in amdgpu's GEM code.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 46 -------------   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h |  2 -   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |  4 +-   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c     | 64 +++++++++++++++++++   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     | 71

---

drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |  1 -   6 files changed, 66 insertions(+), 122 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index e0c4f7c7f1b9..19c5ab08d9ec 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -42,52 +42,6 @@   #include <linux/pci-p2pdma.h>   #include <linux/pm_runtime.h> -/**

• • amdgpu_gem_prime_mmap - &drm_driver.gem_prime_mmap implementation
• • @obj: GEM BO
• • @vma: Virtual memory area
• • Sets up a userspace mapping of the BO's memory in the given
• • virtual memory area.
• • Returns:
• • 0 on success or a negative error code on failure.
• */

-int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, -              struct vm_area_struct *vma) -{ -    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); -    struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); -    unsigned asize = amdgpu_bo_size(bo); -    int ret;

-    if (!vma->vm_file) -        return -ENODEV;

-    if (adev == NULL) -        return -ENODEV;

-    /* Check for valid size. */ -    if (asize < vma->vm_end - vma->vm_start) -        return -EINVAL;

-    if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) || -        (bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)) { -        return -EPERM; -    } -    vma->vm_pgoff += amdgpu_bo_mmap_offset(bo) >> PAGE_SHIFT;

-    /* prime mmap does not need to check access, so allow here */ -    ret = drm_vma_node_allow(&obj->vma_node, vma->vm_file->private_data); -    if (ret) -        return ret;

-    ret = ttm_bo_mmap(vma->vm_file, vma, &adev->mman.bdev); -    drm_vma_node_revoke(&obj->vma_node, vma->vm_file->private_data);

-    return ret; -}

static int   __dma_resv_make_exclusive(struct dma_resv *obj)   { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h index 39b5b9616fd8..3e93b9b407a9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h @@ -31,8 +31,6 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,                           struct dma_buf *dma_buf);   bool amdgpu_dmabuf_is_xgmi_accessible(struct amdgpu_device *adev,                         struct amdgpu_bo *bo); -int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, -              struct vm_area_struct *vma);   extern const struct dma_buf_ops amdgpu_dmabuf_ops; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 76f48f79c70b..e96d2758f4bb 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1656,7 +1656,7 @@ static const struct file_operations amdgpu_driver_kms_fops = {       .flush = amdgpu_flush,       .release = drm_release,       .unlocked_ioctl = amdgpu_drm_ioctl, -    .mmap = amdgpu_mmap, +    .mmap = drm_gem_mmap,       .poll = drm_poll,       .read = drm_read,   #ifdef CONFIG_COMPAT @@ -1719,7 +1719,7 @@ static const struct drm_driver amdgpu_kms_driver = {       .prime_handle_to_fd = drm_gem_prime_handle_to_fd,       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,       .gem_prime_import = amdgpu_gem_prime_import, -    .gem_prime_mmap = amdgpu_gem_prime_mmap, +    .gem_prime_mmap = drm_gem_prime_mmap,       .name = DRIVER_NAME,       .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index fb7171e5507c..fe93faad05f2 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -41,6 +41,36 @@   static const struct drm_gem_object_funcs amdgpu_gem_object_funcs; +static vm_fault_t amdgpu_ttm_fault(struct vm_fault *vmf)

Please name that function amdgpu_gem_fault or amdgpu_gem_object_fault

...

+{ +    struct ttm_buffer_object *bo = vmf->vma->vm_private_data; +    vm_fault_t ret;

+    ret = ttm_bo_vm_reserve(bo, vmf); +    if (ret) +        return ret;

+    ret = amdgpu_bo_fault_reserve_notify(bo); +    if (ret) +        goto unlock;

+    ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot, +                       TTM_BO_VM_NUM_PREFAULT, 1); +    if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) +        return ret;

+unlock: +    dma_resv_unlock(bo->base.resv); +    return ret; +}

+static const struct vm_operations_struct amdgpu_ttm_vm_ops = { +    .fault = amdgpu_ttm_fault, +    .open = ttm_bo_vm_open, +    .close = ttm_bo_vm_close, +    .access = ttm_bo_vm_access +};

static void amdgpu_gem_object_free(struct drm_gem_object *gobj)   {       struct amdgpu_bo *robj = gem_to_amdgpu_bo(gobj); @@ -201,6 +231,38 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,       ttm_eu_backoff_reservation(&ticket, &list);   } +static int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) +{ +    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); +    struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); +    unsigned long asize = amdgpu_bo_size(bo);

+    if (!vma->vm_file) +        return -ENODEV;

+    if (!adev) +        return -ENODEV;

+    /* Check for valid size. */ +    if (asize < vma->vm_end - vma->vm_start) +        return -EINVAL;

...

+    /* +     * Don't verify access for KFD BOs. They don't have a GEM +     * object associated with them. +     */ +    if (bo->kfd_bo) +        goto out;

Who does the access verification now?

This is somewhat confusing.

I took this check as-is, including the comment, from amdgpu's verify_access function. The verify_access function was called by ttm_bo_mmap. It returned 0 and ttm_bo_mmap did the mapping.

This is probably a left-over from when we mapped BOs using /dev/kfd. We changed this to use /dev/dri/renderD* a long time ago to fix CPU mapping invalidations on memory evictions. I think we can let GEM do the access check.

Regards,   Felix

...

Christian.

...

+    if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) || +        (bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)) { +        return -EPERM; +    }

+out: +    return drm_gem_ttm_mmap(obj, vma); +}

static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {       .free = amdgpu_gem_object_free,       .open = amdgpu_gem_object_open, @@ -208,6 +270,8 @@ static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {       .export = amdgpu_gem_prime_export,       .vmap = drm_gem_ttm_vmap,       .vunmap = drm_gem_ttm_vunmap, +    .mmap = amdgpu_gem_prime_mmap, +    .vm_ops = &amdgpu_ttm_vm_ops,   };   /* diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 1c6131489a85..d9de91a517c6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -152,32 +152,6 @@ static void amdgpu_evict_flags(struct ttm_buffer_object *bo,       *placement = abo->placement;   } -/**

• • amdgpu_verify_access - Verify access for a mmap call
• • @bo:    The buffer object to map
• • @filp: The file pointer from the process performing the mmap
• • This is called by ttm_bo_mmap() to verify whether a process
• • has the right to mmap a BO to their process space.
• */

-static int amdgpu_verify_access(struct ttm_buffer_object *bo, struct file *filp) -{ -    struct amdgpu_bo *abo = ttm_to_amdgpu_bo(bo);

-    /* -     * Don't verify access for KFD BOs. They don't have a GEM -     * object associated with them. -     */ -    if (abo->kfd_bo) -        return 0;

-    if (amdgpu_ttm_tt_get_usermm(bo->ttm)) -        return -EPERM; -    return drm_vma_node_verify_access(&abo->tbo.base.vma_node, -                      filp->private_data); -}

Here's the orignal verification code. It gives a free pass to KFD.

...

...

/**    * amdgpu_ttm_map_buffer - Map memory into the GART windows    * @bo: buffer object to map @@ -1531,7 +1505,6 @@ static struct ttm_device_funcs amdgpu_bo_driver = {       .eviction_valuable = amdgpu_ttm_bo_eviction_valuable,       .evict_flags = &amdgpu_evict_flags,       .move = &amdgpu_bo_move, -    .verify_access = &amdgpu_verify_access,       .delete_mem_notify = &amdgpu_bo_delete_mem_notify,       .release_notify = &amdgpu_bo_release_notify,       .io_mem_reserve = &amdgpu_ttm_io_mem_reserve, @@ -1906,50 +1879,6 @@ void amdgpu_ttm_set_buffer_funcs_status(struct amdgpu_device *adev, bool enable)       adev->mman.buffer_funcs_enabled = enable;   } -static vm_fault_t amdgpu_ttm_fault(struct vm_fault *vmf) -{ -    struct ttm_buffer_object *bo = vmf->vma->vm_private_data; -    vm_fault_t ret;

-    ret = ttm_bo_vm_reserve(bo, vmf); -    if (ret) -        return ret;

-    ret = amdgpu_bo_fault_reserve_notify(bo); -    if (ret) -        goto unlock;

-    ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot, -                       TTM_BO_VM_NUM_PREFAULT, 1); -    if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) -        return ret;

-unlock: -    dma_resv_unlock(bo->base.resv); -    return ret; -}

-static const struct vm_operations_struct amdgpu_ttm_vm_ops = { -    .fault = amdgpu_ttm_fault, -    .open = ttm_bo_vm_open, -    .close = ttm_bo_vm_close, -    .access = ttm_bo_vm_access -};

-int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma) -{ -    struct drm_file *file_priv = filp->private_data; -    struct amdgpu_device *adev = drm_to_adev(file_priv->minor->dev); -    int r;

-    r = ttm_bo_mmap(filp, vma, &adev->mman.bdev); -    if (unlikely(r != 0)) -        return r;

-    vma->vm_ops = &amdgpu_ttm_vm_ops; -    return 0; -}

And this was the mmap callback in struct file_operations. It calls ttm_bo_mmap(), which skips verification for KFD BOs. To the best of my knowledge, there was no additional verification for these KFD BOs.

The original code in amdgpu_gem_prime_mmap() did seom verification, but didn't handle KFD specially. I guess, PRIME needs GEM and KFD BOs wouldn't quailify.

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.

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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Am 06.04.21 um 11:08 schrieb Thomas Zimmermann:

Moving the driver-specific mmap code into a GEM object function allows for using DRM helpers for various mmap callbacks.

This change also allows to support prime-based mmap via DRM's helper drm_gem_prime_mmap().

Permission checks are implemented by drm_gem_mmap(), with an additional check for radeon_ttm_tt_has_userptr() in the GEM object function. The function radeon_verify_access() is now unused and has thus been removed.

As a side effect, amdgpu_ttm_vm_ops and amdgpu_ttm_fault() are now implemented in amdgpu's GEM code.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/radeon/radeon_drv.c | 3 +- drivers/gpu/drm/radeon/radeon_gem.c | 52 +++++++++++++++++++++++ drivers/gpu/drm/radeon/radeon_ttm.c | 65 ----------------------------- drivers/gpu/drm/radeon/radeon_ttm.h | 1 - 4 files changed, 54 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index efeb115ae70e..4039b6d71aa2 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -557,7 +557,7 @@ static const struct file_operations radeon_driver_kms_fops = { .open = drm_open, .release = drm_release, .unlocked_ioctl = radeon_drm_ioctl,

• .mmap = radeon_mmap,

• .mmap = drm_gem_mmap, .poll = drm_poll, .read = drm_read, #ifdef CONFIG_COMPAT

@@ -632,6 +632,7 @@ static const struct drm_driver kms_driver = { .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_import_sg_table = radeon_gem_prime_import_sg_table,

• .gem_prime_mmap = drm_gem_prime_mmap,

  .name = DRIVER_NAME, .desc = DRIVER_DESC,

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 05ea2f39f626..71e8737bce01 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -44,6 +44,42 @@ void radeon_gem_prime_unpin(struct drm_gem_object *obj);

const struct drm_gem_object_funcs radeon_gem_object_funcs;

+static vm_fault_t radeon_ttm_fault(struct vm_fault *vmf)

Please name this radeon_gem_fault or radeon_gem_object_fault.

Apart from that looks good to me.

Christian.

+{

• struct ttm_buffer_object *bo = vmf->vma->vm_private_data;
• struct radeon_device *rdev = radeon_get_rdev(bo->bdev);
• vm_fault_t ret;
• down_read(&rdev->pm.mclk_lock);
• ret = ttm_bo_vm_reserve(bo, vmf);
• if (ret)

• goto unlock_mclk;
  

• ret = radeon_bo_fault_reserve_notify(bo);
• if (ret)

• goto unlock_resv;
  

• ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,

• 		       TTM_BO_VM_NUM_PREFAULT, 1);
  

• if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))

• goto unlock_mclk;
  

+unlock_resv:

• dma_resv_unlock(bo->base.resv);

+unlock_mclk:

• up_read(&rdev->pm.mclk_lock);
• return ret;

+}

+static const struct vm_operations_struct radeon_ttm_vm_ops = {

• .fault = radeon_ttm_fault,
• .open = ttm_bo_vm_open,
• .close = ttm_bo_vm_close,
• .access = ttm_bo_vm_access

+};

• static void radeon_gem_object_free(struct drm_gem_object *gobj) { struct radeon_bo *robj = gem_to_radeon_bo(gobj);

@@ -226,6 +262,20 @@ static int radeon_gem_handle_lockup(struct radeon_device *rdev, int r) return r; }

+static int radeon_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) +{

• struct radeon_bo *bo = gem_to_radeon_bo(obj);
• struct radeon_device *rdev = radeon_get_rdev(bo->tbo.bdev);
• if (!rdev)

• return -EINVAL;
  

• if (radeon_ttm_tt_has_userptr(rdev, bo->tbo.ttm))

• return -EPERM;
  

• return drm_gem_ttm_mmap(obj, vma);

+}

• const struct drm_gem_object_funcs radeon_gem_object_funcs = { .free = radeon_gem_object_free, .open = radeon_gem_object_open,

@@ -236,6 +286,8 @@ const struct drm_gem_object_funcs radeon_gem_object_funcs = { .get_sg_table = radeon_gem_prime_get_sg_table, .vmap = drm_gem_ttm_vmap, .vunmap = drm_gem_ttm_vunmap,

• .mmap = radeon_gem_object_mmap,

• .vm_ops = &radeon_ttm_vm_ops, };

  /*

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 476ce9c24b9f..a5ce43a909a2 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -136,17 +136,6 @@ static void radeon_evict_flags(struct ttm_buffer_object *bo, *placement = rbo->placement; }

-static int radeon_verify_access(struct ttm_buffer_object *bo, struct file *filp) -{

• struct radeon_bo *rbo = container_of(bo, struct radeon_bo, tbo);
• struct radeon_device *rdev = radeon_get_rdev(bo->bdev);
• if (radeon_ttm_tt_has_userptr(rdev, bo->ttm))

• return -EPERM;
  

• return drm_vma_node_verify_access(&rbo->tbo.base.vma_node,

• 			  filp->private_data);
  

-}

• static int radeon_move_blit(struct ttm_buffer_object *bo, bool evict, struct ttm_resource *new_mem,

@@ -704,7 +693,6 @@ static struct ttm_device_funcs radeon_bo_driver = { .eviction_valuable = ttm_bo_eviction_valuable, .evict_flags = &radeon_evict_flags, .move = &radeon_bo_move,

• .verify_access = &radeon_verify_access, .delete_mem_notify = &radeon_bo_delete_mem_notify, .io_mem_reserve = &radeon_ttm_io_mem_reserve, };

@@ -801,59 +789,6 @@ void radeon_ttm_set_active_vram_size(struct radeon_device *rdev, u64 size) man->size = size >> PAGE_SHIFT; }

-static vm_fault_t radeon_ttm_fault(struct vm_fault *vmf) -{

• struct ttm_buffer_object *bo = vmf->vma->vm_private_data;
• struct radeon_device *rdev = radeon_get_rdev(bo->bdev);
• vm_fault_t ret;
• down_read(&rdev->pm.mclk_lock);
• ret = ttm_bo_vm_reserve(bo, vmf);
• if (ret)

• goto unlock_mclk;
  

• ret = radeon_bo_fault_reserve_notify(bo);
• if (ret)

• goto unlock_resv;
  

• ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,

• 		       TTM_BO_VM_NUM_PREFAULT, 1);
  

• if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))

• goto unlock_mclk;
  

-unlock_resv:

• dma_resv_unlock(bo->base.resv);

-unlock_mclk:

• up_read(&rdev->pm.mclk_lock);
• return ret;

-}

-static const struct vm_operations_struct radeon_ttm_vm_ops = {

• .fault = radeon_ttm_fault,
• .open = ttm_bo_vm_open,
• .close = ttm_bo_vm_close,
• .access = ttm_bo_vm_access

-};

-int radeon_mmap(struct file *filp, struct vm_area_struct *vma) -{

• int r;
• struct drm_file *file_priv = filp->private_data;
• struct radeon_device *rdev = file_priv->minor->dev->dev_private;
• if (rdev == NULL)

• return -EINVAL;
  

• r = ttm_bo_mmap(filp, vma, &rdev->mman.bdev);
• if (unlikely(r != 0))

• return r;
  

• vma->vm_ops = &radeon_ttm_vm_ops;
• return 0;

-}

• #if defined(CONFIG_DEBUG_FS)

  static int radeon_mm_vram_dump_table_show(struct seq_file *m, void *unused)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.h b/drivers/gpu/drm/radeon/radeon_ttm.h index 4d7b90ee2774..91ea7141bc81 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.h +++ b/drivers/gpu/drm/radeon/radeon_ttm.h @@ -32,6 +32,5 @@ struct radeon_device;

int radeon_ttm_init(struct radeon_device *rdev); void radeon_ttm_fini(struct radeon_device *rdev); -int radeon_mmap(struct file *filp, struct vm_area_struct *vma);

#endif /* __RADEON_TTM_H__ */

On 4/6/21 5:09 AM, Thomas Zimmermann wrote:

The vmwgfx driver is the only remaining user of ttm_bo_mmap(). Inline the code. The internal helper ttm_bo_vm_lookup() is now also part of vmwgfx as vmw_bo_vm_lookup().

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 54 ++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c index cb9975889e2f..3eaad00668f2 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c @@ -27,6 +27,30 @@

#include "vmwgfx_drv.h"

+static struct ttm_buffer_object *vmw_bo_vm_lookup(struct ttm_device *bdev,

• 				  unsigned long offset,
  

• 				  unsigned long pages)
  

+{

• struct drm_vma_offset_node *node;
• struct ttm_buffer_object *bo = NULL;
• drm_vma_offset_lock_lookup(bdev->vma_manager);
• node = drm_vma_offset_lookup_locked(bdev->vma_manager, offset, pages);
• if (likely(node)) {

• bo = container_of(node, struct ttm_buffer_object,
  

• 		  base.vma_node);
  

• bo = ttm_bo_get_unless_zero(bo);
  

• }
• drm_vma_offset_unlock_lookup(bdev->vma_manager);
• if (!bo)

• pr_err("Could not find buffer object to map\n");
  

It's not a big deal and I know it's been in the original, but since you're already in there if you could change this to DRM_ERR that'd be great. Either way: Reviewed-by: Zack Rusin zackr@vmware.com

z

On 4/6/21 5:09 AM, Thomas Zimmermann wrote:

Vmwgfx is the only user of the TTM's verify_access callback. Inline the call and avoid the indirection through the function pointer.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 9 --------- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 7 ++----- 2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c index 2dc031fe4a90..a079734f9d68 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c @@ -658,14 +658,6 @@ static void vmw_evict_flags(struct ttm_buffer_object *bo, *placement = vmw_sys_placement; }

-static int vmw_verify_access(struct ttm_buffer_object *bo, struct file *filp) -{

• struct ttm_object_file *tfile =

• vmw_fpriv((struct drm_file *)filp->private_data)->tfile;
  

• return vmw_user_bo_verify_access(bo, tfile);

-}

• static int vmw_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *mem) { struct vmw_private *dev_priv = container_of(bdev, struct vmw_private, bdev);

@@ -768,7 +760,6 @@ struct ttm_device_funcs vmw_bo_driver = { .eviction_valuable = ttm_bo_eviction_valuable, .evict_flags = vmw_evict_flags, .move = vmw_move,

• .verify_access = vmw_verify_access, .swap_notify = vmw_swap_notify, .io_mem_reserve = &vmw_ttm_io_mem_reserve, };

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c index 3eaad00668f2..2574d4707407 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c @@ -65,6 +65,7 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma) }; struct drm_file *file_priv = filp->private_data; struct vmw_private *dev_priv = vmw_priv(file_priv->minor->dev);

• struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile; struct ttm_device *bdev = &dev_priv->bdev; struct ttm_buffer_object *bo; int ret;

@@ -76,11 +77,7 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma) if (unlikely(!bo)) return -EINVAL;

• if (unlikely(!bo->bdev->funcs->verify_access)) {

• ret = -EPERM;
  

• goto out_unref;
  

• }
• ret = bo->bdev->funcs->verify_access(bo, filp);

• ret = vmw_user_bo_verify_access(bo, tfile); if (unlikely(ret != 0)) goto out_unref;

Looks great.

Reviewed-by: Zack Rusin zackr@vmware.com

z

Am 06.04.21 um 11:09 schrieb Thomas Zimmermann:

The function ttm_bo_mmap is unused. Remove it and it's helpers; including the verify_access callback in struct ttm_device_funcs.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Reviewed-by: Christian König christian.koenig@amd.com

---

drivers/gpu/drm/ttm/ttm_bo_vm.c | 53 --------------------------------- include/drm/ttm/ttm_bo_api.h | 13 -------- include/drm/ttm/ttm_device.h | 15 ---------- 3 files changed, 81 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index bf4a213bc66c..6cd352399941 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -508,30 +508,6 @@ static const struct vm_operations_struct ttm_bo_vm_ops = { .access = ttm_bo_vm_access, };

-static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_device *bdev,

• 				  unsigned long offset,
  

• 				  unsigned long pages)
  

-{

• struct drm_vma_offset_node *node;
• struct ttm_buffer_object *bo = NULL;
• drm_vma_offset_lock_lookup(bdev->vma_manager);
• node = drm_vma_offset_lookup_locked(bdev->vma_manager, offset, pages);
• if (likely(node)) {

• bo = container_of(node, struct ttm_buffer_object,
  

• 		  base.vma_node);
  

• bo = ttm_bo_get_unless_zero(bo);
  

• }
• drm_vma_offset_unlock_lookup(bdev->vma_manager);
• if (!bo)

• pr_err("Could not find buffer object to map\n");
  

• return bo;

-}

• static void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma) { /*

@@ -559,35 +535,6 @@ static void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_s vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP; }

-int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,

• struct ttm_device *bdev)
  

-{

• struct ttm_buffer_object *bo;
• int ret;
• if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET_START))

• return -EINVAL;
  

• bo = ttm_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma));
• if (unlikely(!bo))

• return -EINVAL;
  

• if (unlikely(!bo->bdev->funcs->verify_access)) {

• ret = -EPERM;
  

• goto out_unref;
  

• }
• ret = bo->bdev->funcs->verify_access(bo, filp);
• if (unlikely(ret != 0))

• goto out_unref;
  

• ttm_bo_mmap_vma_setup(bo, vma);
• return 0;

-out_unref:

• ttm_bo_put(bo);
• return ret;

-} -EXPORT_SYMBOL(ttm_bo_mmap);

• int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo) { ttm_bo_get(bo);

diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 2155e2e38aec..6e35680ac01b 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -522,19 +522,6 @@ void ttm_bo_vunmap(struct ttm_buffer_object *bo, struct dma_buf_map *map); */ int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo);

-/**

• • ttm_bo_mmap - mmap out of the ttm device address space.
• • @filp: filp as input from the mmap method.
• • @vma: vma as input from the mmap method.
• • @bdev: Pointer to the ttm_device with the address space manager.
• • This function is intended to be called by the device mmap method.
• • if the device address space is to be backed by the bo manager.
• */

-int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,

• struct ttm_device *bdev);
  

• /**
  • ttm_bo_io

diff --git a/include/drm/ttm/ttm_device.h b/include/drm/ttm/ttm_device.h index 7c8f87bd52d3..cd592f8e941b 100644 --- a/include/drm/ttm/ttm_device.h +++ b/include/drm/ttm/ttm_device.h @@ -161,21 +161,6 @@ struct ttm_device_funcs { struct ttm_resource *new_mem, struct ttm_place *hop);

• /**

• * struct ttm_bo_driver_member verify_access
  

• *
  

• * @bo: Pointer to a buffer object.
  

• * @filp: Pointer to a struct file trying to access the object.
  

• *
  

• * Called from the map / write / read methods to verify that the
  

• * caller is permitted to access the buffer object.
  

• * This member may be set to NULL, which will refuse this kind of
  

• * access for all buffer objects.
  

• * This function should return 0 if access is granted, -EPERM otherwise.
  

• */
  

• int (*verify_access)(struct ttm_buffer_object *bo,

• 	     struct file *filp);
  

• /**
  • Hook to notify driver about a resource delete.
  */