[PATCH v3 0/7] drm: Clean up mmap for TTM-based GEM drivers

[PATCH v3 0/7] drm: Clean up mmap for TTM-based GEM drivers

[Thomas Zimmermann]

Implement mmap via struct drm_gem_object_functions.mmap in amdgpu,
radeon and nouveau. This allows for using common DRM helpers for
the mmap-related callbacks in struct file_operations and struct
drm_driver. The drivers have their own vm_ops, which are now set
automatically by the DRM core functions. The code in each driver's
verify_access becomes part of the driver's new mmap implementation.

With the GEM drivers converted, vmwgfx is the only user of
ttm_bo_mmap() and related infrastructure. So move everything into
vmwgfx and delete the rsp code from TTM.

This touches several drivers. Preferably everything would be merged
at once via drm-misc-next.

v3:
	* tidy up the new mmap functions in amdgpu and radeon (Christian)
v2:
	* removal of amdgpu fbdev mmap already merged (Christian)
	* rebase on top of amdgpu fixes [1] (Felix)
	* replace pr_err() with drm_err() in vmwgfx patch (Zack)
	* several typos

[1] https://patchwork.freedesktop.org/series/88822/

Thomas Zimmermann (7):
  drm/ttm: Don't override vm_ops callbacks, if set
  drm/amdgpu: Implement mmap as GEM object function
  drm/radeon: Implement mmap as GEM object function
  drm/nouveau: Implement mmap as GEM object function
  drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver
  drm/vmwgfx: Inline vmw_verify_access()
  drm/ttm: Remove ttm_bo_mmap() and friends

 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     | 44 ++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     | 64 --------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |  1 -
 drivers/gpu/drm/nouveau/nouveau_bo.c        | 10 ----
 drivers/gpu/drm/nouveau/nouveau_drm.c       |  3 +-
 drivers/gpu/drm/nouveau/nouveau_gem.c       | 36 ++++++++++++
 drivers/gpu/drm/nouveau/nouveau_ttm.c       | 49 ----------------
 drivers/gpu/drm/nouveau/nouveau_ttm.h       |  1 -
 drivers/gpu/drm/radeon/radeon_drv.c         |  3 +-
 drivers/gpu/drm/radeon/radeon_gem.c         | 49 ++++++++++++++++
 drivers/gpu/drm/radeon/radeon_ttm.c         | 65 ---------------------
 drivers/gpu/drm/radeon/radeon_ttm.h         |  1 -
 drivers/gpu/drm/ttm/ttm_bo_vm.c             | 60 ++-----------------
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c  |  9 ---
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c    | 53 ++++++++++++++++-
 include/drm/ttm/ttm_bo_api.h                | 13 -----
 include/drm/ttm/ttm_device.h                | 15 -----
 20 files changed, 191 insertions(+), 337 deletions(-)

--
2.31.1

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

[PATCH v3 1/7] drm/ttm: Don't override vm_ops callbacks, if set

[Thomas Zimmermann]

Drivers may want to set their own callbacks for a VM area. Only set
TTM's callbacks if the vm_ops field is clear.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ttm/ttm_bo_vm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index b31b18058965..bf4a213bc66c 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -534,7 +534,12 @@ static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_device *bdev,
 
 static void ttm_bo_mmap_vma_setup(struct ttm_buffer_object *bo, struct vm_area_struct *vma)
 {
-	vma->vm_ops = &ttm_bo_vm_ops;
+	/*
+	 * Drivers may want to override the vm_ops field. Otherwise we
+	 * use TTM's default callbacks.
+	 */
+	if (!vma->vm_ops)
+		vma->vm_ops = &ttm_bo_vm_ops;
 
 	/*
 	 * Note: We're transferring the bo reference to
-- 
2.31.1

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

[PATCH v3 2/7] drm/amdgpu: Implement mmap as GEM object function

[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 function amdgpu_verify_access() is
no longer being called and therefore removed by this patch.

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

v3:
	* rename mmap function to amdgpu_gem_object_mmap() (Christian)
	* remove unnecessary checks from mmap (Christian)
v2:
	* rename amdgpu_ttm_vm_ops and amdgpu_ttm_fault() to
	  amdgpu_gem_vm_ops and amdgpu_gem_fault() (Christian)
	* the check for kfd_bo has meanwhile been removed

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

cleanup mmap rename gem_object_mmap
---
 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     | 44 ++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     | 64 ---------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |  1 -
 6 files changed, 46 insertions(+), 115 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 80130c1c0c68..69d2485ec665 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1686,7 +1686,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
@@ -1749,7 +1749,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 311bcdc59eda..df78a06ef0c8 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_gem_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_gem_vm_ops = {
+	.fault = amdgpu_gem_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);
@@ -205,6 +235,18 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
 	ttm_eu_backoff_reservation(&ticket, &list);
 }
 
+static int amdgpu_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
+{
+	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
+
+	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
+		return -EPERM;
+	if (bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
+		return -EPERM;
+
+	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,
@@ -212,6 +254,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_object_mmap,
+	.vm_ops = &amdgpu_gem_vm_ops,
 };
 
 /*
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 1485f33c3cc7..d4083c19402b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -152,25 +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);
-
-	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
@@ -1522,7 +1503,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,
@@ -1897,50 +1877,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);
-- 
2.31.1

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

[PATCH v3 3/7] drm/radeon: Implement mmap as GEM object function

[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, radeon_ttm_vm_ops and radeon_ttm_fault() are now
implemented in amdgpu's GEM code.

v3:
	* remove unnecessary checks from mmap (Christian)
v2:
	* rename radeon_ttm_vm_ops and radeon_ttm_fault() to
	  radeon_gem_vm_ops and radeon_gem_fault() (Christian)
	* fix commit description (Alex)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/radeon/radeon_drv.c |  3 +-
 drivers/gpu/drm/radeon/radeon_gem.c | 49 ++++++++++++++++++++++
 drivers/gpu/drm/radeon/radeon_ttm.c | 65 -----------------------------
 drivers/gpu/drm/radeon/radeon_ttm.h |  1 -
 4 files changed, 51 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 8885e849717d..61793ffdfa1b 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -558,7 +558,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
@@ -633,6 +633,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..ff8849827d61 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_gem_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_gem_vm_ops = {
+	.fault = radeon_gem_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,17 @@ 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 (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 +283,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_gem_vm_ops,
 };
 
 /*
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 380b3007fd0b..8b6efcc4191c 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,
@@ -705,7 +694,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,
 };
@@ -802,59 +790,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__ */
-- 
2.31.1

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

[PATCH v3 4/7] drm/nouveau: Implement mmap as GEM object function

[Thomas Zimmermann]

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

The GEM object function is provided by GEM TTM helpers. Nouveau's
implementation of verify_access is unused and has been removed. Access
permissions are validated by the DRM helpers.

As a side effect, nouveau_ttm_vm_ops and nouveau_ttm_fault() are now
implemented in nouveau's GEM code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/nouveau/nouveau_bo.c  | 10 ------
 drivers/gpu/drm/nouveau/nouveau_drm.c |  3 +-
 drivers/gpu/drm/nouveau/nouveau_gem.c | 36 ++++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_ttm.c | 49 ---------------------------
 drivers/gpu/drm/nouveau/nouveau_ttm.h |  1 -
 5 files changed, 38 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 3e09df0472ce..bc67cbccc83b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -1051,15 +1051,6 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
 	return ret;
 }
 
-static int
-nouveau_bo_verify_access(struct ttm_buffer_object *bo, struct file *filp)
-{
-	struct nouveau_bo *nvbo = nouveau_bo(bo);
-
-	return drm_vma_node_verify_access(&nvbo->bo.base.vma_node,
-					  filp->private_data);
-}
-
 static void
 nouveau_ttm_io_mem_free_locked(struct nouveau_drm *drm,
 			       struct ttm_resource *reg)
@@ -1332,7 +1323,6 @@ struct ttm_device_funcs nouveau_bo_driver = {
 	.evict_flags = nouveau_bo_evict_flags,
 	.delete_mem_notify = nouveau_bo_delete_mem_notify,
 	.move = nouveau_bo_move,
-	.verify_access = nouveau_bo_verify_access,
 	.io_mem_reserve = &nouveau_ttm_io_mem_reserve,
 	.io_mem_free = &nouveau_ttm_io_mem_free,
 };
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 3204fc0a90d2..a616cf4573b8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1179,7 +1179,7 @@ nouveau_driver_fops = {
 	.open = drm_open,
 	.release = drm_release,
 	.unlocked_ioctl = nouveau_drm_ioctl,
-	.mmap = nouveau_ttm_mmap,
+	.mmap = drm_gem_mmap,
 	.poll = drm_poll,
 	.read = drm_read,
 #if defined(CONFIG_COMPAT)
@@ -1212,6 +1212,7 @@ driver_stub = {
 	.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 = nouveau_gem_prime_import_sg_table,
+	.gem_prime_mmap = drm_gem_prime_mmap,
 
 	.dumb_create = nouveau_display_dumb_create,
 	.dumb_map_offset = drm_gem_ttm_dumb_map_offset,
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index c88cbb85f101..71dfac820c4d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -39,6 +39,40 @@
 #include <nvif/class.h>
 #include <nvif/push206e.h>
 
+static vm_fault_t nouveau_ttm_fault(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct ttm_buffer_object *bo = vma->vm_private_data;
+	pgprot_t prot;
+	vm_fault_t ret;
+
+	ret = ttm_bo_vm_reserve(bo, vmf);
+	if (ret)
+		return ret;
+
+	ret = nouveau_ttm_fault_reserve_notify(bo);
+	if (ret)
+		goto error_unlock;
+
+	nouveau_bo_del_io_reserve_lru(bo);
+	prot = vm_get_page_prot(vma->vm_flags);
+	ret = ttm_bo_vm_fault_reserved(vmf, prot, TTM_BO_VM_NUM_PREFAULT, 1);
+	nouveau_bo_add_io_reserve_lru(bo);
+	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
+		return ret;
+
+error_unlock:
+	dma_resv_unlock(bo->base.resv);
+	return ret;
+}
+
+static const struct vm_operations_struct nouveau_ttm_vm_ops = {
+	.fault = nouveau_ttm_fault,
+	.open = ttm_bo_vm_open,
+	.close = ttm_bo_vm_close,
+	.access = ttm_bo_vm_access
+};
+
 void
 nouveau_gem_object_del(struct drm_gem_object *gem)
 {
@@ -180,6 +214,8 @@ const struct drm_gem_object_funcs nouveau_gem_object_funcs = {
 	.get_sg_table = nouveau_gem_prime_get_sg_table,
 	.vmap = drm_gem_ttm_vmap,
 	.vunmap = drm_gem_ttm_vunmap,
+	.mmap = drm_gem_ttm_mmap,
+	.vm_ops = &nouveau_ttm_vm_ops,
 };
 
 int
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index b81ae90b8449..e511a26379da 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -127,55 +127,6 @@ const struct ttm_resource_manager_func nv04_gart_manager = {
 	.free = nouveau_manager_del,
 };
 
-static vm_fault_t nouveau_ttm_fault(struct vm_fault *vmf)
-{
-	struct vm_area_struct *vma = vmf->vma;
-	struct ttm_buffer_object *bo = vma->vm_private_data;
-	pgprot_t prot;
-	vm_fault_t ret;
-
-	ret = ttm_bo_vm_reserve(bo, vmf);
-	if (ret)
-		return ret;
-
-	ret = nouveau_ttm_fault_reserve_notify(bo);
-	if (ret)
-		goto error_unlock;
-
-	nouveau_bo_del_io_reserve_lru(bo);
-	prot = vm_get_page_prot(vma->vm_flags);
-	ret = ttm_bo_vm_fault_reserved(vmf, prot, TTM_BO_VM_NUM_PREFAULT, 1);
-	nouveau_bo_add_io_reserve_lru(bo);
-	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
-		return ret;
-
-error_unlock:
-	dma_resv_unlock(bo->base.resv);
-	return ret;
-}
-
-static const struct vm_operations_struct nouveau_ttm_vm_ops = {
-	.fault = nouveau_ttm_fault,
-	.open = ttm_bo_vm_open,
-	.close = ttm_bo_vm_close,
-	.access = ttm_bo_vm_access
-};
-
-int
-nouveau_ttm_mmap(struct file *filp, struct vm_area_struct *vma)
-{
-	struct drm_file *file_priv = filp->private_data;
-	struct nouveau_drm *drm = nouveau_drm(file_priv->minor->dev);
-	int ret;
-
-	ret = ttm_bo_mmap(filp, vma, &drm->ttm.bdev);
-	if (ret)
-		return ret;
-
-	vma->vm_ops = &nouveau_ttm_vm_ops;
-	return 0;
-}
-
 static int
 nouveau_ttm_init_host(struct nouveau_drm *drm, u8 kind)
 {
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.h b/drivers/gpu/drm/nouveau/nouveau_ttm.h
index dbf6dc238efd..2f0efda7ccdb 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.h
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.h
@@ -17,7 +17,6 @@ struct ttm_tt *nouveau_sgdma_create_ttm(struct ttm_buffer_object *bo,
 
 int  nouveau_ttm_init(struct nouveau_drm *drm);
 void nouveau_ttm_fini(struct nouveau_drm *drm);
-int  nouveau_ttm_mmap(struct file *, struct vm_area_struct *);
 
 int  nouveau_ttm_global_init(struct nouveau_drm *);
 void nouveau_ttm_global_release(struct nouveau_drm *);
-- 
2.31.1

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

[PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver

[Thomas Zimmermann]

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().

v2:
	* replace pr_err() with drm_err() (Zack)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Zack Rusin <zackr@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 56 ++++++++++++++++++++++--
 1 file changed, 53 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..c8b6543b4e39 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
@@ -27,6 +27,32 @@
 
 #include "vmwgfx_drv.h"
 
+static struct ttm_buffer_object *vmw_bo_vm_lookup(struct ttm_device *bdev,
+						  unsigned long offset,
+						  unsigned long pages)
+{
+	struct vmw_private *dev_priv = container_of(bdev, struct vmw_private, bdev);
+	struct drm_device *drm = &dev_priv->drm;
+	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)
+		drm_err(drm, "Could not find buffer object to map\n");
+
+	return bo;
+}
+
 int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
 {
 	static const struct vm_operations_struct vmw_vm_ops = {
@@ -41,10 +67,28 @@ 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);
-	int ret = ttm_bo_mmap(filp, vma, &dev_priv->bdev);
+	struct ttm_device *bdev = &dev_priv->bdev;
+	struct ttm_buffer_object *bo;
+	int ret;
+
+	if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET_START))
+		return -EINVAL;
+
+	bo = vmw_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma));
+	if (unlikely(!bo))
+		return -EINVAL;
 
-	if (ret)
-		return ret;
+	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;
+
+	ret = ttm_bo_mmap_obj(vma, bo);
+	if (unlikely(ret != 0))
+		goto out_unref;
 
 	vma->vm_ops = &vmw_vm_ops;
 
@@ -52,7 +96,13 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
 	if (!is_cow_mapping(vma->vm_flags))
 		vma->vm_flags = (vma->vm_flags & ~VM_MIXEDMAP) | VM_PFNMAP;
 
+	ttm_bo_put(bo); /* release extra ref taken by ttm_bo_mmap_obj() */
+
 	return 0;
+
+out_unref:
+	ttm_bo_put(bo);
+	return ret;
 }
 
 /* struct vmw_validation_mem callback */
-- 
2.31.1

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

Re: [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver

[Christian König]

Am 16.04.21 um 15:31 schrieb Thomas Zimmermann:
> 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().
>
> v2:
> 	* replace pr_err() with drm_err() (Zack)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Zack Rusin <zackr@vmware.com>
> ---
>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 56 ++++++++++++++++++++++--
>   1 file changed, 53 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..c8b6543b4e39 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> @@ -27,6 +27,32 @@
>   
>   #include "vmwgfx_drv.h"
>   
> +static struct ttm_buffer_object *vmw_bo_vm_lookup(struct ttm_device *bdev,
> +						  unsigned long offset,
> +						  unsigned long pages)
> +{
> +	struct vmw_private *dev_priv = container_of(bdev, struct vmw_private, bdev);
> +	struct drm_device *drm = &dev_priv->drm;
> +	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)
> +		drm_err(drm, "Could not find buffer object to map\n");
> +
> +	return bo;
> +}
> +
>   int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
>   {
>   	static const struct vm_operations_struct vmw_vm_ops = {
> @@ -41,10 +67,28 @@ 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);
> -	int ret = ttm_bo_mmap(filp, vma, &dev_priv->bdev);
> +	struct ttm_device *bdev = &dev_priv->bdev;
> +	struct ttm_buffer_object *bo;
> +	int ret;
> +
> +	if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET_START))
> +		return -EINVAL;
> +
> +	bo = vmw_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma));
> +	if (unlikely(!bo))
> +		return -EINVAL;
>   
> -	if (ret)
> -		return ret;
> +	if (unlikely(!bo->bdev->funcs->verify_access)) {
> +		ret = -EPERM;
> +		goto out_unref;
> +	}
> +	ret = bo->bdev->funcs->verify_access(bo, filp);

Is there any reason we can't call vmw_verify_access() directly here?

Would allow us to completely nuke the verify_access callback as well as 
far as I can see.

Regards,
Christian.

> +	if (unlikely(ret != 0))
> +		goto out_unref;
> +
> +	ret = ttm_bo_mmap_obj(vma, bo);
> +	if (unlikely(ret != 0))
> +		goto out_unref;
>   
>   	vma->vm_ops = &vmw_vm_ops;
>   
> @@ -52,7 +96,13 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
>   	if (!is_cow_mapping(vma->vm_flags))
>   		vma->vm_flags = (vma->vm_flags & ~VM_MIXEDMAP) | VM_PFNMAP;
>   
> +	ttm_bo_put(bo); /* release extra ref taken by ttm_bo_mmap_obj() */
> +
>   	return 0;
> +
> +out_unref:
> +	ttm_bo_put(bo);
> +	return ret;
>   }
>   
>   /* struct vmw_validation_mem callback */

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

Re: [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver

[Christian König]

Am 16.04.21 um 15:46 schrieb Christian König:
> Am 16.04.21 um 15:31 schrieb Thomas Zimmermann:
>> 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().
>>
>> v2:
>>     * replace pr_err() with drm_err() (Zack)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Zack Rusin <zackr@vmware.com>
>> ---
>>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 56 ++++++++++++++++++++++--
>>   1 file changed, 53 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..c8b6543b4e39 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> @@ -27,6 +27,32 @@
>>     #include "vmwgfx_drv.h"
>>   +static struct ttm_buffer_object *vmw_bo_vm_lookup(struct 
>> ttm_device *bdev,
>> +                          unsigned long offset,
>> +                          unsigned long pages)
>> +{
>> +    struct vmw_private *dev_priv = container_of(bdev, struct 
>> vmw_private, bdev);
>> +    struct drm_device *drm = &dev_priv->drm;
>> +    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)
>> +        drm_err(drm, "Could not find buffer object to map\n");
>> +
>> +    return bo;
>> +}
>> +
>>   int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
>>   {
>>       static const struct vm_operations_struct vmw_vm_ops = {
>> @@ -41,10 +67,28 @@ 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);
>> -    int ret = ttm_bo_mmap(filp, vma, &dev_priv->bdev);
>> +    struct ttm_device *bdev = &dev_priv->bdev;
>> +    struct ttm_buffer_object *bo;
>> +    int ret;
>> +
>> +    if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET_START))
>> +        return -EINVAL;
>> +
>> +    bo = vmw_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma));
>> +    if (unlikely(!bo))
>> +        return -EINVAL;
>>   -    if (ret)
>> -        return ret;
>> +    if (unlikely(!bo->bdev->funcs->verify_access)) {
>> +        ret = -EPERM;
>> +        goto out_unref;
>> +    }
>> +    ret = bo->bdev->funcs->verify_access(bo, filp);
>
> Is there any reason we can't call vmw_verify_access() directly here?
>
> Would allow us to completely nuke the verify_access callback as well 
> as far as I can see.

Forget what I said, couldn't see the next patch in my mailbox at time of 
writing.

Whole series is Reviewed-by: Christian König <christian.koenig@amd.com>

Thanks for the nice cleanup,
Christian.

>
> Regards,
> Christian.
>
>> +    if (unlikely(ret != 0))
>> +        goto out_unref;
>> +
>> +    ret = ttm_bo_mmap_obj(vma, bo);
>> +    if (unlikely(ret != 0))
>> +        goto out_unref;
>>         vma->vm_ops = &vmw_vm_ops;
>>   @@ -52,7 +96,13 @@ int vmw_mmap(struct file *filp, struct 
>> vm_area_struct *vma)
>>       if (!is_cow_mapping(vma->vm_flags))
>>           vma->vm_flags = (vma->vm_flags & ~VM_MIXEDMAP) | VM_PFNMAP;
>>   +    ttm_bo_put(bo); /* release extra ref taken by 
>> ttm_bo_mmap_obj() */
>> +
>>       return 0;
>> +
>> +out_unref:
>> +    ttm_bo_put(bo);
>> +    return ret;
>>   }
>>     /* struct vmw_validation_mem callback */
>

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

Re: [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 5098 bytes --]

Hi

Am 16.04.21 um 15:51 schrieb Christian König:
> Am 16.04.21 um 15:46 schrieb Christian König:
>> Am 16.04.21 um 15:31 schrieb Thomas Zimmermann:
>>> 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().
>>>
>>> v2:
>>>     * replace pr_err() with drm_err() (Zack)
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Reviewed-by: Zack Rusin <zackr@vmware.com>
>>> ---
>>>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 56 ++++++++++++++++++++++--
>>>   1 file changed, 53 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..c8b6543b4e39 100644
>>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>>> @@ -27,6 +27,32 @@
>>>     #include "vmwgfx_drv.h"
>>>   +static struct ttm_buffer_object *vmw_bo_vm_lookup(struct 
>>> ttm_device *bdev,
>>> +                          unsigned long offset,
>>> +                          unsigned long pages)
>>> +{
>>> +    struct vmw_private *dev_priv = container_of(bdev, struct 
>>> vmw_private, bdev);
>>> +    struct drm_device *drm = &dev_priv->drm;
>>> +    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)
>>> +        drm_err(drm, "Could not find buffer object to map\n");
>>> +
>>> +    return bo;
>>> +}
>>> +
>>>   int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
>>>   {
>>>       static const struct vm_operations_struct vmw_vm_ops = {
>>> @@ -41,10 +67,28 @@ 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);
>>> -    int ret = ttm_bo_mmap(filp, vma, &dev_priv->bdev);
>>> +    struct ttm_device *bdev = &dev_priv->bdev;
>>> +    struct ttm_buffer_object *bo;
>>> +    int ret;
>>> +
>>> +    if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET_START))
>>> +        return -EINVAL;
>>> +
>>> +    bo = vmw_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma));
>>> +    if (unlikely(!bo))
>>> +        return -EINVAL;
>>>   -    if (ret)
>>> -        return ret;
>>> +    if (unlikely(!bo->bdev->funcs->verify_access)) {
>>> +        ret = -EPERM;
>>> +        goto out_unref;
>>> +    }
>>> +    ret = bo->bdev->funcs->verify_access(bo, filp);
>>
>> Is there any reason we can't call vmw_verify_access() directly here?
>>
>> Would allow us to completely nuke the verify_access callback as well 
>> as far as I can see.
> 
> Forget what I said, couldn't see the next patch in my mailbox at time of 
> writing.
> 
> Whole series is Reviewed-by: Christian König <christian.koenig@amd.com>

Thanks a lot. If I'm not mistaken, the patches at [1] need to go in 
first. So it could take a a bit until this lands.

Otherwise, this series could go through the same tree as [1] if nouveau 
and vmwgfx devs don't mind.

Best regards
Thomas

[1] https://patchwork.freedesktop.org/series/88822/

> 
> Thanks for the nice cleanup,
> Christian.
> 
>>
>> Regards,
>> Christian.
>>
>>> +    if (unlikely(ret != 0))
>>> +        goto out_unref;
>>> +
>>> +    ret = ttm_bo_mmap_obj(vma, bo);
>>> +    if (unlikely(ret != 0))
>>> +        goto out_unref;
>>>         vma->vm_ops = &vmw_vm_ops;
>>>   @@ -52,7 +96,13 @@ int vmw_mmap(struct file *filp, struct 
>>> vm_area_struct *vma)
>>>       if (!is_cow_mapping(vma->vm_flags))
>>>           vma->vm_flags = (vma->vm_flags & ~VM_MIXEDMAP) | VM_PFNMAP;
>>>   +    ttm_bo_put(bo); /* release extra ref taken 
by 
>>> ttm_bo_mmap_obj() */
>>> +
>>>       return 0;
>>> +
>>> +out_unref:
>>> +    ttm_bo_put(bo);
>>> +    return ret;
>>>   }
>>>     /* struct vmw_validation_mem callback */
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver

[Daniel Vetter]

On Tue, Apr 20, 2021 at 09:51:27AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 16.04.21 um 15:51 schrieb Christian König:
> > Am 16.04.21 um 15:46 schrieb Christian König:
> > > Am 16.04.21 um 15:31 schrieb Thomas Zimmermann:
> > > > 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().
> > > > 
> > > > v2:
> > > >     * replace pr_err() with drm_err() (Zack)
> > > > 
> > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > > Reviewed-by: Zack Rusin <zackr@vmware.com>
> > > > ---
> > > >   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 56 ++++++++++++++++++++++--
> > > >   1 file changed, 53 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..c8b6543b4e39 100644
> > > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> > > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
> > > > @@ -27,6 +27,32 @@
> > > >     #include "vmwgfx_drv.h"
> > > >   +static struct ttm_buffer_object *vmw_bo_vm_lookup(struct
> > > > ttm_device *bdev,
> > > > +                          unsigned long offset,
> > > > +                          unsigned long pages)
> > > > +{
> > > > +    struct vmw_private *dev_priv = container_of(bdev, struct
> > > > vmw_private, bdev);
> > > > +    struct drm_device *drm = &dev_priv->drm;
> > > > +    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)
> > > > +        drm_err(drm, "Could not find buffer object to map\n");
> > > > +
> > > > +    return bo;
> > > > +}
> > > > +
> > > >   int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
> > > >   {
> > > >       static const struct vm_operations_struct vmw_vm_ops = {
> > > > @@ -41,10 +67,28 @@ 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);
> > > > -    int ret = ttm_bo_mmap(filp, vma, &dev_priv->bdev);
> > > > +    struct ttm_device *bdev = &dev_priv->bdev;
> > > > +    struct ttm_buffer_object *bo;
> > > > +    int ret;
> > > > +
> > > > +    if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET_START))
> > > > +        return -EINVAL;
> > > > +
> > > > +    bo = vmw_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma));
> > > > +    if (unlikely(!bo))
> > > > +        return -EINVAL;
> > > >   -    if (ret)
> > > > -        return ret;
> > > > +    if (unlikely(!bo->bdev->funcs->verify_access)) {
> > > > +        ret = -EPERM;
> > > > +        goto out_unref;
> > > > +    }
> > > > +    ret = bo->bdev->funcs->verify_access(bo, filp);
> > > 
> > > Is there any reason we can't call vmw_verify_access() directly here?
> > > 
> > > Would allow us to completely nuke the verify_access callback as well
> > > as far as I can see.
> > 
> > Forget what I said, couldn't see the next patch in my mailbox at time of
> > writing.
> > 
> > Whole series is Reviewed-by: Christian König <christian.koenig@amd.com>
> 
> Thanks a lot. If I'm not mistaken, the patches at [1] need to go in first.
> So it could take a a bit until this lands.
> 
> Otherwise, this series could go through the same tree as [1] if nouveau and
> vmwgfx devs don't mind.

I would land it all through drm-misc-next. Maybe check with Alex on irc
for an ack for merging that way, but I don't think this will cause issues
against the amdgpu tree. Lots of ttm cleanup has landed this way already
past few months. Otherwise you could create a small topic branch with
these patches here and send that to Alex, and he can sort out the
interaction with Felix' series.
-Daniel


> 
> Best regards
> Thomas
> 
> [1] https://patchwork.freedesktop.org/series/88822/
> 
> > 
> > Thanks for the nice cleanup,
> > Christian.
> > 
> > > 
> > > Regards,
> > > Christian.
> > > 
> > > > +    if (unlikely(ret != 0))
> > > > +        goto out_unref;
> > > > +
> > > > +    ret = ttm_bo_mmap_obj(vma, bo);
> > > > +    if (unlikely(ret != 0))
> > > > +        goto out_unref;
> > > >         vma->vm_ops = &vmw_vm_ops;
> > > >   @@ -52,7 +96,13 @@ int vmw_mmap(struct file *filp, struct
> > > > vm_area_struct *vma)
> > > >       if (!is_cow_mapping(vma->vm_flags))
> > > >           vma->vm_flags = (vma->vm_flags & ~VM_MIXEDMAP) | VM_PFNMAP;
> > > >   +    ttm_bo_put(bo); /* release extra ref taken
> by
> > > > ttm_bo_mmap_obj() */
> > > > +
> > > >       return 0;
> > > > +
> > > > +out_unref:
> > > > +    ttm_bo_put(bo);
> > > > +    return ret;
> > > >   }
> > > >     /* struct vmw_validation_mem callback */
> > > 
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@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
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver

[Felix Kuehling]

Am 2021-04-20 um 4:51 a.m. schrieb Daniel Vetter:
>>> Whole series is Reviewed-by: Christian König <christian.koenig@amd.com>
>> Thanks a lot. If I'm not mistaken, the patches at [1] need to go in first.
>> So it could take a a bit until this lands.
>>
>> Otherwise, this series could go through the same tree as [1] if nouveau and
>> vmwgfx devs don't mind.
> I would land it all through drm-misc-next. Maybe check with Alex on irc
> for an ack for merging that way, but I don't think this will cause issues
> against the amdgpu tree. Lots of ttm cleanup has landed this way already
> past few months. Otherwise you could create a small topic branch with
> these patches here and send that to Alex, and he can sort out the
> interaction with Felix' series.
> -Daniel

My patch series involved some pretty far-reaching changes in KFD
(renaming some variables in KFD and amdgpu, changing the KFD->amdgpu
interface). We already submitted other patches on top of it that have
dependencies on it. If we decide to deliver this through a different
tree and remove it from amd-staging-drm-next, there will be conflicts to
resolve when removing it from amd-staging-drm-next, and again the next
time you merge with amd-staging-drm-next.

Regards,
  Felix


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

Re: [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver

[Daniel Vetter]

On Tue, Apr 20, 2021 at 10:23 PM Felix Kuehling <felix.kuehling@amd.com> wrote:
>
>
> Am 2021-04-20 um 4:51 a.m. schrieb Daniel Vetter:
> >>> Whole series is Reviewed-by: Christian König <christian.koenig@amd.com>
> >> Thanks a lot. If I'm not mistaken, the patches at [1] need to go in first.
> >> So it could take a a bit until this lands.
> >>
> >> Otherwise, this series could go through the same tree as [1] if nouveau and
> >> vmwgfx devs don't mind.
> > I would land it all through drm-misc-next. Maybe check with Alex on irc
> > for an ack for merging that way, but I don't think this will cause issues
> > against the amdgpu tree. Lots of ttm cleanup has landed this way already
> > past few months. Otherwise you could create a small topic branch with
> > these patches here and send that to Alex, and he can sort out the
> > interaction with Felix' series.
> > -Daniel
>
> My patch series involved some pretty far-reaching changes in KFD
> (renaming some variables in KFD and amdgpu, changing the KFD->amdgpu
> interface). We already submitted other patches on top of it that have
> dependencies on it. If we decide to deliver this through a different
> tree and remove it from amd-staging-drm-next, there will be conflicts to
> resolve when removing it from amd-staging-drm-next, and again the next
> time you merge with amd-staging-drm-next.

Ah then the usual way is for Alex to assemble a topic pull request
(stable, non-rebasing) with those select patches, which then gets
merged into drm-misc-next. Or we smash it all into amdgpu-next. Or we
just wait until -rc2 when drm-next is back open for business.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver

[Christian König]

Am 20.04.21 um 22:53 schrieb Daniel Vetter:
> On Tue, Apr 20, 2021 at 10:23 PM Felix Kuehling <felix.kuehling@amd.com> wrote:
>>
>> Am 2021-04-20 um 4:51 a.m. schrieb Daniel Vetter:
>>>>> Whole series is Reviewed-by: Christian König <christian.koenig@amd.com>
>>>> Thanks a lot. If I'm not mistaken, the patches at [1] need to go in first.
>>>> So it could take a a bit until this lands.
>>>>
>>>> Otherwise, this series could go through the same tree as [1] if nouveau and
>>>> vmwgfx devs don't mind.
>>> I would land it all through drm-misc-next. Maybe check with Alex on irc
>>> for an ack for merging that way, but I don't think this will cause issues
>>> against the amdgpu tree. Lots of ttm cleanup has landed this way already
>>> past few months. Otherwise you could create a small topic branch with
>>> these patches here and send that to Alex, and he can sort out the
>>> interaction with Felix' series.
>>> -Daniel
>> My patch series involved some pretty far-reaching changes in KFD
>> (renaming some variables in KFD and amdgpu, changing the KFD->amdgpu
>> interface). We already submitted other patches on top of it that have
>> dependencies on it. If we decide to deliver this through a different
>> tree and remove it from amd-staging-drm-next, there will be conflicts to
>> resolve when removing it from amd-staging-drm-next, and again the next
>> time you merge with amd-staging-drm-next.
> Ah then the usual way is for Alex to assemble a topic pull request
> (stable, non-rebasing) with those select patches, which then gets
> merged into drm-misc-next. Or we smash it all into amdgpu-next. Or we
> just wait until -rc2 when drm-next is back open for business.

I need to double check, but if I'm not totally mistaken the changes in 
question should already be in drm-next.

But exactly that was the reason why I asked when the next backmerge from 
drm-next into drm-misc-next is planned :)

Christian.

> -Daniel

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

Re: [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver

[Daniel Vetter]

On Wed, Apr 21, 2021 at 09:01:00AM +0200, Christian König wrote:
> Am 20.04.21 um 22:53 schrieb Daniel Vetter:
> > On Tue, Apr 20, 2021 at 10:23 PM Felix Kuehling <felix.kuehling@amd.com> wrote:
> > > 
> > > Am 2021-04-20 um 4:51 a.m. schrieb Daniel Vetter:
> > > > > > Whole series is Reviewed-by: Christian König <christian.koenig@amd.com>
> > > > > Thanks a lot. If I'm not mistaken, the patches at [1] need to go in first.
> > > > > So it could take a a bit until this lands.
> > > > > 
> > > > > Otherwise, this series could go through the same tree as [1] if nouveau and
> > > > > vmwgfx devs don't mind.
> > > > I would land it all through drm-misc-next. Maybe check with Alex on irc
> > > > for an ack for merging that way, but I don't think this will cause issues
> > > > against the amdgpu tree. Lots of ttm cleanup has landed this way already
> > > > past few months. Otherwise you could create a small topic branch with
> > > > these patches here and send that to Alex, and he can sort out the
> > > > interaction with Felix' series.
> > > > -Daniel
> > > My patch series involved some pretty far-reaching changes in KFD
> > > (renaming some variables in KFD and amdgpu, changing the KFD->amdgpu
> > > interface). We already submitted other patches on top of it that have
> > > dependencies on it. If we decide to deliver this through a different
> > > tree and remove it from amd-staging-drm-next, there will be conflicts to
> > > resolve when removing it from amd-staging-drm-next, and again the next
> > > time you merge with amd-staging-drm-next.
> > Ah then the usual way is for Alex to assemble a topic pull request
> > (stable, non-rebasing) with those select patches, which then gets
> > merged into drm-misc-next. Or we smash it all into amdgpu-next. Or we
> > just wait until -rc2 when drm-next is back open for business.
> 
> I need to double check, but if I'm not totally mistaken the changes in
> question should already be in drm-next.
> 
> But exactly that was the reason why I asked when the next backmerge from
> drm-next into drm-misc-next is planned :)

Best way to make that happen isn't to ask when it will happen, but tell
drm-misc maintainers to do it and why (ideally with references to the
commits you need). Also I tend to do those requests over irc.

Backmerges are generally on demand, not on schedule. Hence you need to
demand one :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4489 bytes --]

Hi

Am 16.04.21 um 15:46 schrieb Christian König:
> Am 16.04.21 um 15:31 schrieb Thomas Zimmermann:
>> 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().
>>
>> v2:
>>     * replace pr_err() with drm_err() (Zack)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Zack Rusin <zackr@vmware.com>
>> ---
>>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 56 ++++++++++++++++++++++--
>>   1 file changed, 53 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..c8b6543b4e39 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
>> @@ -27,6 +27,32 @@
>>   #include "vmwgfx_drv.h"
>> +static struct ttm_buffer_object *vmw_bo_vm_lookup(struct ttm_device 
>> *bdev,
>> +                          unsigned long offset,
>> +                          unsigned long pages)
>> +{
>> +    struct vmw_private *dev_priv = container_of(bdev, struct 
>> vmw_private, bdev);
>> +    struct drm_device *drm = &dev_priv->drm;
>> +    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)
>> +        drm_err(drm, "Could not find buffer object to map\n");
>> +
>> +    return bo;
>> +}
>> +
>>   int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
>>   {
>>       static const struct vm_operations_struct vmw_vm_ops = {
>> @@ -41,10 +67,28 @@ 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);
>> -    int ret = ttm_bo_mmap(filp, vma, &dev_priv->bdev);
>> +    struct ttm_device *bdev = &dev_priv->bdev;
>> +    struct ttm_buffer_object *bo;
>> +    int ret;
>> +
>> +    if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET_START))
>> +        return -EINVAL;
>> +
>> +    bo = vmw_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma));
>> +    if (unlikely(!bo))
>> +        return -EINVAL;
>> -    if (ret)
>> -        return ret;
>> +    if (unlikely(!bo->bdev->funcs->verify_access)) {
>> +        ret = -EPERM;
>> +        goto out_unref;
>> +    }
>> +    ret = bo->bdev->funcs->verify_access(bo, filp);
> 
> Is there any reason we can't call vmw_verify_access() directly here?
> 
> Would allow us to completely nuke the verify_access callback as well as 

> far as I can see.

We have to spare something for patches 6 and 7. Otherwise they'd be 
empty. ;)

Best regards
Thomas

> 
> Regards,
> Christian.
> 
>> +    if (unlikely(ret != 0))
>> +        goto out_unref;
>> +
>> +    ret = ttm_bo_mmap_obj(vma, bo);
>> +    if (unlikely(ret != 0))
>> +        goto out_unref;
>>       vma->vm_ops = &vmw_vm_ops;
>> @@ -52,7 +96,13 @@ int vmw_mmap(struct file *filp, struct 
>> vm_area_struct *vma)
>>       if (!is_cow_mapping(vma->vm_flags))
>>           vma->vm_flags = 
(vma->vm_flags & ~VM_MIXEDMAP) | VM_PFNMAP;
>> +    ttm_bo_put(bo); /* release extra ref taken by ttm_bo_mmap_obj() */
>> +
>>       return 0;
>> +
>> +out_unref:
>> +    ttm_bo_put(bo);
>> +    return ret;
>>   }
>>   /* struct vmw_validation_mem callback */
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

[PATCH v3 6/7] drm/vmwgfx: Inline vmw_verify_access()

[Thomas Zimmermann]

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>
Reviewed-by: Zack Rusin <zackr@vmware.com>
---
 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 c8b6543b4e39..e6b1f98ec99f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
@@ -67,6 +67,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;
@@ -78,11 +79,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;
 
-- 
2.31.1

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

[PATCH v3 7/7] drm/ttm: Remove ttm_bo_mmap() and friends

[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.
 	 */
-- 
2.31.1

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

Re: [PATCH v3 0/7] drm: Clean up mmap for TTM-based GEM drivers

[Christian König]

Patches #1-#4 are Reviewed-by: Christian König <christian.koenig@amd.com>

One more comment on the vmw patches.

Regards,
Christian.

Am 16.04.21 um 15:31 schrieb Thomas Zimmermann:
> Implement mmap via struct drm_gem_object_functions.mmap in amdgpu,
> radeon and nouveau. This allows for using common DRM helpers for
> the mmap-related callbacks in struct file_operations and struct
> drm_driver. The drivers have their own vm_ops, which are now set
> automatically by the DRM core functions. The code in each driver's
> verify_access becomes part of the driver's new mmap implementation.
>
> With the GEM drivers converted, vmwgfx is the only user of
> ttm_bo_mmap() and related infrastructure. So move everything into
> vmwgfx and delete the rsp code from TTM.
>
> This touches several drivers. Preferably everything would be merged
> at once via drm-misc-next.
>
> v3:
> 	* tidy up the new mmap functions in amdgpu and radeon (Christian)
> v2:
> 	* removal of amdgpu fbdev mmap already merged (Christian)
> 	* rebase on top of amdgpu fixes [1] (Felix)
> 	* replace pr_err() with drm_err() in vmwgfx patch (Zack)
> 	* several typos
>
> [1] https://patchwork.freedesktop.org/series/88822/
>
> Thomas Zimmermann (7):
>    drm/ttm: Don't override vm_ops callbacks, if set
>    drm/amdgpu: Implement mmap as GEM object function
>    drm/radeon: Implement mmap as GEM object function
>    drm/nouveau: Implement mmap as GEM object function
>    drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver
>    drm/vmwgfx: Inline vmw_verify_access()
>    drm/ttm: Remove ttm_bo_mmap() and friends
>
>   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     | 44 ++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     | 64 --------------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |  1 -
>   drivers/gpu/drm/nouveau/nouveau_bo.c        | 10 ----
>   drivers/gpu/drm/nouveau/nouveau_drm.c       |  3 +-
>   drivers/gpu/drm/nouveau/nouveau_gem.c       | 36 ++++++++++++
>   drivers/gpu/drm/nouveau/nouveau_ttm.c       | 49 ----------------
>   drivers/gpu/drm/nouveau/nouveau_ttm.h       |  1 -
>   drivers/gpu/drm/radeon/radeon_drv.c         |  3 +-
>   drivers/gpu/drm/radeon/radeon_gem.c         | 49 ++++++++++++++++
>   drivers/gpu/drm/radeon/radeon_ttm.c         | 65 ---------------------
>   drivers/gpu/drm/radeon/radeon_ttm.h         |  1 -
>   drivers/gpu/drm/ttm/ttm_bo_vm.c             | 60 ++-----------------
>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c  |  9 ---
>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c    | 53 ++++++++++++++++-
>   include/drm/ttm/ttm_bo_api.h                | 13 -----
>   include/drm/ttm/ttm_device.h                | 15 -----
>   20 files changed, 191 insertions(+), 337 deletions(-)
>
> --
> 2.31.1
>
> _______________________________________________
> 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