[PATCH v6 01/26] drm/gpuvm: Pass map arguments through a struct

[PATCH v6 01/26] drm/gpuvm: Pass map arguments through a struct

[Himal Prasad Ghimiray]

From: Boris Brezillon <boris.brezillon@collabora.com>

We are about to pass more arguments to drm_gpuvm_sm_map[_ops_create](),
so, before we do that, let's pass arguments through a struct instead
of changing each call site every time a new optional argument is added.

v5
 - Use drm_gpuva_op_map—same as drm_gpuvm_map_req
 - Rebase changes for drm_gpuvm_sm_map_exec_lock()
 - Fix kernel-docs

v6
 - Use drm_gpuvm_map_req (Danilo/Matt)

Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Brendan King <Brendan.King@imgtec.com>
Cc: Boris Brezillon <bbrezillon@kernel.org>
Cc: Caterina Shablia <caterina.shablia@collabora.com>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: <dri-devel@lists.freedesktop.org>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Caterina Shablia <caterina.shablia@collabora.com>
Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
---
 drivers/gpu/drm/drm_gpuvm.c            | 105 ++++++++++---------------
 drivers/gpu/drm/imagination/pvr_vm.c   |  15 ++--
 drivers/gpu/drm/msm/msm_gem_vma.c      |  25 ++++--
 drivers/gpu/drm/nouveau/nouveau_uvmm.c |  11 ++-
 drivers/gpu/drm/panthor/panthor_mmu.c  |  13 ++-
 drivers/gpu/drm/xe/xe_vm.c             |  13 ++-
 include/drm/drm_gpuvm.h                |  20 +++--
 7 files changed, 114 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index bbc7fecb6f4a..b3a01c40001b 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -486,13 +486,18 @@
  *				  u64 addr, u64 range,
  *				  struct drm_gem_object *obj, u64 offset)
  *	{
+ *		struct drm_gpuvm_map_req map_req = {
+ *		        .op_map.va.addr = addr,
+ *	                .op_map.va.range = range,
+ *	                .op_map.gem.obj = obj,
+ *	                .op_map.gem.offset = offset,
+ *	           };
  *		struct drm_gpuva_ops *ops;
  *		struct drm_gpuva_op *op
  *		struct drm_gpuvm_bo *vm_bo;
  *
  *		driver_lock_va_space();
- *		ops = drm_gpuvm_sm_map_ops_create(gpuvm, addr, range,
- *						  obj, offset);
+ *		ops = drm_gpuvm_sm_map_ops_create(gpuvm, &map_req);
  *		if (IS_ERR(ops))
  *			return PTR_ERR(ops);
  *
@@ -2054,16 +2059,15 @@ EXPORT_SYMBOL_GPL(drm_gpuva_unmap);
 
 static int
 op_map_cb(const struct drm_gpuvm_ops *fn, void *priv,
-	  u64 addr, u64 range,
-	  struct drm_gem_object *obj, u64 offset)
+	  const struct drm_gpuvm_map_req *req)
 {
 	struct drm_gpuva_op op = {};
 
 	op.op = DRM_GPUVA_OP_MAP;
-	op.map.va.addr = addr;
-	op.map.va.range = range;
-	op.map.gem.obj = obj;
-	op.map.gem.offset = offset;
+	op.map.va.addr = req->op_map.va.addr;
+	op.map.va.range = req->op_map.va.range;
+	op.map.gem.obj = req->op_map.gem.obj;
+	op.map.gem.offset = req->op_map.gem.offset;
 
 	return fn->sm_step_map(&op, priv);
 }
@@ -2102,17 +2106,16 @@ op_unmap_cb(const struct drm_gpuvm_ops *fn, void *priv,
 static int
 __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 		   const struct drm_gpuvm_ops *ops, void *priv,
-		   u64 req_addr, u64 req_range,
-		   struct drm_gem_object *req_obj, u64 req_offset)
+		   const struct drm_gpuvm_map_req *req)
 {
 	struct drm_gpuva *va, *next;
-	u64 req_end = req_addr + req_range;
+	u64 req_end = req->op_map.va.addr + req->op_map.va.range;
 	int ret;
 
-	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req_addr, req_range)))
+	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req->op_map.va.addr, req->op_map.va.range)))
 		return -EINVAL;
 
-	drm_gpuvm_for_each_va_range_safe(va, next, gpuvm, req_addr, req_end) {
+	drm_gpuvm_for_each_va_range_safe(va, next, gpuvm, req->op_map.va.addr, req_end) {
 		struct drm_gem_object *obj = va->gem.obj;
 		u64 offset = va->gem.offset;
 		u64 addr = va->va.addr;
@@ -2120,9 +2123,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 		u64 end = addr + range;
 		bool merge = !!va->gem.obj;
 
-		if (addr == req_addr) {
-			merge &= obj == req_obj &&
-				 offset == req_offset;
+		if (addr == req->op_map.va.addr) {
+			merge &= obj == req->op_map.gem.obj &&
+				 offset == req->op_map.gem.offset;
 
 			if (end == req_end) {
 				ret = op_unmap_cb(ops, priv, va, merge);
@@ -2141,9 +2144,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 			if (end > req_end) {
 				struct drm_gpuva_op_map n = {
 					.va.addr = req_end,
-					.va.range = range - req_range,
+					.va.range = range - req->op_map.va.range,
 					.gem.obj = obj,
-					.gem.offset = offset + req_range,
+					.gem.offset = offset + req->op_map.va.range,
 				};
 				struct drm_gpuva_op_unmap u = {
 					.va = va,
@@ -2155,8 +2158,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 					return ret;
 				break;
 			}
-		} else if (addr < req_addr) {
-			u64 ls_range = req_addr - addr;
+		} else if (addr < req->op_map.va.addr) {
+			u64 ls_range = req->op_map.va.addr - addr;
 			struct drm_gpuva_op_map p = {
 				.va.addr = addr,
 				.va.range = ls_range,
@@ -2165,8 +2168,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 			};
 			struct drm_gpuva_op_unmap u = { .va = va };
 
-			merge &= obj == req_obj &&
-				 offset + ls_range == req_offset;
+			merge &= obj == req->op_map.gem.obj &&
+				 offset + ls_range == req->op_map.gem.offset;
 			u.keep = merge;
 
 			if (end == req_end) {
@@ -2189,7 +2192,7 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 					.va.range = end - req_end,
 					.gem.obj = obj,
 					.gem.offset = offset + ls_range +
-						      req_range,
+						      req->op_map.va.range,
 				};
 
 				ret = op_remap_cb(ops, priv, &p, &n, &u);
@@ -2197,10 +2200,10 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 					return ret;
 				break;
 			}
-		} else if (addr > req_addr) {
-			merge &= obj == req_obj &&
-				 offset == req_offset +
-					   (addr - req_addr);
+		} else if (addr > req->op_map.va.addr) {
+			merge &= obj == req->op_map.gem.obj &&
+				 offset == req->op_map.gem.offset +
+					   (addr - req->op_map.va.addr);
 
 			if (end == req_end) {
 				ret = op_unmap_cb(ops, priv, va, merge);
@@ -2236,9 +2239,7 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 		}
 	}
 
-	return op_map_cb(ops, priv,
-			 req_addr, req_range,
-			 req_obj, req_offset);
+	return op_map_cb(ops, priv, req);
 }
 
 static int
@@ -2303,10 +2304,7 @@ __drm_gpuvm_sm_unmap(struct drm_gpuvm *gpuvm,
  * drm_gpuvm_sm_map() - calls the &drm_gpuva_op split/merge steps
  * @gpuvm: the &drm_gpuvm representing the GPU VA space
  * @priv: pointer to a driver private data structure
- * @req_addr: the start address of the new mapping
- * @req_range: the range of the new mapping
- * @req_obj: the &drm_gem_object to map
- * @req_offset: the offset within the &drm_gem_object
+ * @req: ptr to struct drm_gpuvm_map_req
  *
  * This function iterates the given range of the GPU VA space. It utilizes the
  * &drm_gpuvm_ops to call back into the driver providing the split and merge
@@ -2333,8 +2331,7 @@ __drm_gpuvm_sm_unmap(struct drm_gpuvm *gpuvm,
  */
 int
 drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv,
-		 u64 req_addr, u64 req_range,
-		 struct drm_gem_object *req_obj, u64 req_offset)
+		 const struct drm_gpuvm_map_req *req)
 {
 	const struct drm_gpuvm_ops *ops = gpuvm->ops;
 
@@ -2343,9 +2340,7 @@ drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv,
 		       ops->sm_step_unmap)))
 		return -EINVAL;
 
-	return __drm_gpuvm_sm_map(gpuvm, ops, priv,
-				  req_addr, req_range,
-				  req_obj, req_offset);
+	return __drm_gpuvm_sm_map(gpuvm, ops, priv, req);
 }
 EXPORT_SYMBOL_GPL(drm_gpuvm_sm_map);
 
@@ -2421,10 +2416,7 @@ static const struct drm_gpuvm_ops lock_ops = {
  * @gpuvm: the &drm_gpuvm representing the GPU VA space
  * @exec: the &drm_exec locking context
  * @num_fences: for newly mapped objects, the # of fences to reserve
- * @req_addr: the start address of the range to unmap
- * @req_range: the range of the mappings to unmap
- * @req_obj: the &drm_gem_object to map
- * @req_offset: the offset within the &drm_gem_object
+ * @req: ptr to drm_gpuvm_map_req struct
  *
  * This function locks (drm_exec_lock_obj()) objects that will be unmapped/
  * remapped, and locks+prepares (drm_exec_prepare_object()) objects that
@@ -2445,9 +2437,7 @@ static const struct drm_gpuvm_ops lock_ops = {
  *                    ret = drm_gpuvm_sm_unmap_exec_lock(gpuvm, &exec, op->addr, op->range);
  *                    break;
  *                case DRIVER_OP_MAP:
- *                    ret = drm_gpuvm_sm_map_exec_lock(gpuvm, &exec, num_fences,
- *                                                     op->addr, op->range,
- *                                                     obj, op->obj_offset);
+ *                    ret = drm_gpuvm_sm_map_exec_lock(gpuvm, &exec, num_fences, &req);
  *                    break;
  *                }
  *
@@ -2478,18 +2468,15 @@ static const struct drm_gpuvm_ops lock_ops = {
 int
 drm_gpuvm_sm_map_exec_lock(struct drm_gpuvm *gpuvm,
 			   struct drm_exec *exec, unsigned int num_fences,
-			   u64 req_addr, u64 req_range,
-			   struct drm_gem_object *req_obj, u64 req_offset)
+			   struct drm_gpuvm_map_req *req)
 {
-	if (req_obj) {
-		int ret = drm_exec_prepare_obj(exec, req_obj, num_fences);
+	if (req->op_map.gem.obj) {
+		int ret = drm_exec_prepare_obj(exec, req->op_map.gem.obj, num_fences);
 		if (ret)
 			return ret;
 	}
 
-	return __drm_gpuvm_sm_map(gpuvm, &lock_ops, exec,
-				  req_addr, req_range,
-				  req_obj, req_offset);
+	return __drm_gpuvm_sm_map(gpuvm, &lock_ops, exec, req);
 
 }
 EXPORT_SYMBOL_GPL(drm_gpuvm_sm_map_exec_lock);
@@ -2611,10 +2598,7 @@ static const struct drm_gpuvm_ops gpuvm_list_ops = {
 /**
  * drm_gpuvm_sm_map_ops_create() - creates the &drm_gpuva_ops to split and merge
  * @gpuvm: the &drm_gpuvm representing the GPU VA space
- * @req_addr: the start address of the new mapping
- * @req_range: the range of the new mapping
- * @req_obj: the &drm_gem_object to map
- * @req_offset: the offset within the &drm_gem_object
+ * @req: map request arguments
  *
  * This function creates a list of operations to perform splitting and merging
  * of existent mapping(s) with the newly requested one.
@@ -2642,8 +2626,7 @@ static const struct drm_gpuvm_ops gpuvm_list_ops = {
  */
 struct drm_gpuva_ops *
 drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
-			    u64 req_addr, u64 req_range,
-			    struct drm_gem_object *req_obj, u64 req_offset)
+			    const struct drm_gpuvm_map_req *req)
 {
 	struct drm_gpuva_ops *ops;
 	struct {
@@ -2661,9 +2644,7 @@ drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
 	args.vm = gpuvm;
 	args.ops = ops;
 
-	ret = __drm_gpuvm_sm_map(gpuvm, &gpuvm_list_ops, &args,
-				 req_addr, req_range,
-				 req_obj, req_offset);
+	ret = __drm_gpuvm_sm_map(gpuvm, &gpuvm_list_ops, &args, req);
 	if (ret)
 		goto err_free_ops;
 
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index 2896fa7501b1..0f6b4cdb5fd8 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -185,12 +185,17 @@ struct pvr_vm_bind_op {
 static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op *bind_op)
 {
 	switch (bind_op->type) {
-	case PVR_VM_BIND_TYPE_MAP:
+	case PVR_VM_BIND_TYPE_MAP: {
+		const struct drm_gpuvm_map_req map_req = {
+			.op_map.va.addr = bind_op->device_addr,
+			.op_map.va.range = bind_op->size,
+			.op_map.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
+			.op_map.gem.offset = bind_op->offset,
+		};
+
 		return drm_gpuvm_sm_map(&bind_op->vm_ctx->gpuvm_mgr,
-					bind_op, bind_op->device_addr,
-					bind_op->size,
-					gem_from_pvr_gem(bind_op->pvr_obj),
-					bind_op->offset);
+					bind_op, &map_req);
+	}
 
 	case PVR_VM_BIND_TYPE_UNMAP:
 		return drm_gpuvm_sm_unmap(&bind_op->vm_ctx->gpuvm_mgr,
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 3cd8562a5109..2ca408c40369 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -1172,10 +1172,17 @@ vm_bind_job_lock_objects(struct msm_vm_bind_job *job, struct drm_exec *exec)
 				break;
 			case MSM_VM_BIND_OP_MAP:
 			case MSM_VM_BIND_OP_MAP_NULL:
-				ret = drm_gpuvm_sm_map_exec_lock(job->vm, exec, 1,
-							    op->iova, op->range,
-							    op->obj, op->obj_offset);
+			{
+				struct drm_gpuvm_map_req map_req = {
+					.op_map.va.addr = op->iova,
+					.op_map.va.range = op->range,
+					.op_map.gem.obj = op->obj,
+					.op_map.gem.offset = op->obj_offset,
+				};
+
+				ret = drm_gpuvm_sm_map_exec_lock(job->vm, exec, 1, &map_req);
 				break;
+			}
 			default:
 				/*
 				 * lookup_op() should have already thrown an error for
@@ -1283,9 +1290,17 @@ vm_bind_job_prepare(struct msm_vm_bind_job *job)
 				arg.flags |= MSM_VMA_DUMP;
 			fallthrough;
 		case MSM_VM_BIND_OP_MAP_NULL:
-			ret = drm_gpuvm_sm_map(job->vm, &arg, op->iova,
-					       op->range, op->obj, op->obj_offset);
+		{
+			struct drm_gpuvm_map_req map_req = {
+				.op_map.va.addr = op->iova,
+				.op_map.va.range = op->range,
+				.op_map.gem.obj = op->obj,
+				.op_map.gem.offset = op->obj_offset,
+			};
+
+			ret = drm_gpuvm_sm_map(job->vm, &arg, &map_req);
 			break;
+		}
 		default:
 			/*
 			 * lookup_op() should have already thrown an error for
diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
index ddfc46bc1b3e..92f87520eeb8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
@@ -1276,6 +1276,12 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
 			break;
 		case OP_MAP: {
 			struct nouveau_uvma_region *reg;
+			struct drm_gpuvm_map_req map_req = {
+				.op_map.va.addr = op->va.addr,
+				.op_map.va.range = op->va.range,
+				.op_map.gem.obj = op->gem.obj,
+				.op_map.gem.offset = op->gem.offset,
+			};
 
 			reg = nouveau_uvma_region_find_first(uvmm,
 							     op->va.addr,
@@ -1301,10 +1307,7 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
 			}
 
 			op->ops = drm_gpuvm_sm_map_ops_create(&uvmm->base,
-							      op->va.addr,
-							      op->va.range,
-							      op->gem.obj,
-							      op->gem.offset);
+							      &map_req);
 			if (IS_ERR(op->ops)) {
 				ret = PTR_ERR(op->ops);
 				goto unwind_continue;
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 4140f697ba5a..5ed4573b3a6b 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -2169,15 +2169,22 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
 	mutex_lock(&vm->op_lock);
 	vm->op_ctx = op;
 	switch (op_type) {
-	case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP:
+	case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: {
+		const struct drm_gpuvm_map_req map_req = {
+			.op_map.va.addr = op->va.addr,
+			.op_map.va.range = op->va.range,
+			.op_map.gem.obj = op->map.vm_bo->obj,
+			.op_map.gem.offset = op->map.bo_offset,
+		};
+
 		if (vm->unusable) {
 			ret = -EINVAL;
 			break;
 		}
 
-		ret = drm_gpuvm_sm_map(&vm->base, vm, op->va.addr, op->va.range,
-				       op->map.vm_bo->obj, op->map.bo_offset);
+		ret = drm_gpuvm_sm_map(&vm->base, vm, &map_req);
 		break;
+	}
 
 	case DRM_PANTHOR_VM_BIND_OP_TYPE_UNMAP:
 		ret = drm_gpuvm_sm_unmap(&vm->base, vm, op->va.addr, op->va.range);
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 432ea325677d..9fcc52032a1d 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -2316,10 +2316,17 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
 
 	switch (operation) {
 	case DRM_XE_VM_BIND_OP_MAP:
-	case DRM_XE_VM_BIND_OP_MAP_USERPTR:
-		ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, addr, range,
-						  obj, bo_offset_or_userptr);
+	case DRM_XE_VM_BIND_OP_MAP_USERPTR: {
+		struct drm_gpuvm_map_req map_req = {
+			.op_map.va.addr = addr,
+			.op_map.va.range = range,
+			.op_map.gem.obj = obj,
+			.op_map.gem.offset = bo_offset_or_userptr,
+		};
+
+		ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, &map_req);
 		break;
+	}
 	case DRM_XE_VM_BIND_OP_UNMAP:
 		ops = drm_gpuvm_sm_unmap_ops_create(&vm->gpuvm, addr, range);
 		break;
diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
index 274532facfd6..3cf0a84b8b08 100644
--- a/include/drm/drm_gpuvm.h
+++ b/include/drm/drm_gpuvm.h
@@ -1058,10 +1058,20 @@ struct drm_gpuva_ops {
  */
 #define drm_gpuva_next_op(op) list_next_entry(op, entry)
 
+/**
+ * struct drm_gpuvm_map_req - arguments passed to drm_gpuvm_sm_map[_ops_create]()
+ */
+struct drm_gpuvm_map_req {
+	/**
+	 * @op_map: struct drm_gpuva_op_map
+	 */
+	struct drm_gpuva_op_map op_map;
+};
+
 struct drm_gpuva_ops *
 drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
-			    u64 addr, u64 range,
-			    struct drm_gem_object *obj, u64 offset);
+			    const struct drm_gpuvm_map_req *req);
+
 struct drm_gpuva_ops *
 drm_gpuvm_sm_unmap_ops_create(struct drm_gpuvm *gpuvm,
 			      u64 addr, u64 range);
@@ -1205,16 +1215,14 @@ struct drm_gpuvm_ops {
 };
 
 int drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv,
-		     u64 addr, u64 range,
-		     struct drm_gem_object *obj, u64 offset);
+		     const struct drm_gpuvm_map_req *req);
 
 int drm_gpuvm_sm_unmap(struct drm_gpuvm *gpuvm, void *priv,
 		       u64 addr, u64 range);
 
 int drm_gpuvm_sm_map_exec_lock(struct drm_gpuvm *gpuvm,
 			  struct drm_exec *exec, unsigned int num_fences,
-			  u64 req_addr, u64 req_range,
-			  struct drm_gem_object *obj, u64 offset);
+			  struct drm_gpuvm_map_req *req);
 
 int drm_gpuvm_sm_unmap_exec_lock(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
 				 u64 req_addr, u64 req_range);
-- 
2.34.1

[PATCH v6 03/26] drm/gpuvm: Support flags in drm_gpuvm_map_req

[Himal Prasad Ghimiray]

This change adds support for passing flags to drm_gpuvm_sm_map() and
sm_map_ops_create(), enabling future extensions that affect split/merge
logic in drm_gpuvm.

v2
- Move flag to drm_gpuvm_map_req

Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Boris Brezillon <bbrezillon@kernel.org>
Cc: Caterina Shablia <caterina.shablia@collabora.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: <dri-devel@lists.freedesktop.org>
Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
---
 include/drm/drm_gpuvm.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
index cbb9b6519462..116f77abd570 100644
--- a/include/drm/drm_gpuvm.h
+++ b/include/drm/drm_gpuvm.h
@@ -1049,6 +1049,13 @@ struct drm_gpuva_ops {
  */
 #define drm_gpuva_next_op(op) list_next_entry(op, entry)
 
+enum drm_gpuvm_sm_map_ops_flags {
+	 /**
+	  * %DRM_GPUVM_SM_MAP_OPS_FLAG_NONE: DEFAULT sm_map ops
+	  */
+	DRM_GPUVM_SM_MAP_OPS_FLAG_NONE = 0,
+};
+
 /**
  * struct drm_gpuvm_map_req - arguments passed to drm_gpuvm_sm_map[_ops_create]()
  */
@@ -1057,6 +1064,11 @@ struct drm_gpuvm_map_req {
 	 * @op_map: struct drm_gpuva_op_map
 	 */
 	struct drm_gpuva_op_map op_map;
+
+	/**
+	 * @flags: drm_gpuvm_sm_map_ops_flags for this mapping request
+	 */
+	enum drm_gpuvm_sm_map_ops_flags flags;
 };
 
 struct drm_gpuva_ops *
-- 
2.34.1

[PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Himal Prasad Ghimiray]

- DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE: This flag is used by
  drm_gpuvm_sm_map_ops_create to iterate over GPUVMA's in the
user-provided range and split the existing non-GEM object VMA if the
start or end of the input range lies within it. The operations can
create up to 2 REMAPS and 2 MAPs. The purpose of this operation is to be
used by the Xe driver to assign attributes to GPUVMA's within the
user-defined range. Unlike drm_gpuvm_sm_map_ops_flags in default mode,
the operation with this flag will never have UNMAPs and
merges, and can be without any final operations.

v2
- use drm_gpuvm_sm_map_ops_create with flags instead of defining new
  ops_create (Danilo)
- Add doc (Danilo)

v3
- Fix doc
- Fix unmapping check

v4
- Fix mapping for non madvise ops

v5
- Fix mapping (Matthew Brost)
- Rebase on top of struct changes

v6
- flag moved to map_req

Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Boris Brezillon <bbrezillon@kernel.org>
Cc: <dri-devel@lists.freedesktop.org>
Signed-off-by: Himal Prasad Ghimiray<himal.prasad.ghimiray@intel.com>
---
 drivers/gpu/drm/drm_gpuvm.c | 87 +++++++++++++++++++++++++++++++------
 include/drm/drm_gpuvm.h     | 11 +++++
 2 files changed, 84 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index b3a01c40001b..d8f5f594a415 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -2110,6 +2110,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 {
 	struct drm_gpuva *va, *next;
 	u64 req_end = req->op_map.va.addr + req->op_map.va.range;
+	bool is_madvise_ops = (req->flags & DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE);
+	bool needs_map = !is_madvise_ops;
 	int ret;
 
 	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req->op_map.va.addr, req->op_map.va.range)))
@@ -2122,26 +2124,35 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 		u64 range = va->va.range;
 		u64 end = addr + range;
 		bool merge = !!va->gem.obj;
+		bool skip_madvise_ops = is_madvise_ops && merge;
 
+		needs_map = !is_madvise_ops;
 		if (addr == req->op_map.va.addr) {
 			merge &= obj == req->op_map.gem.obj &&
 				 offset == req->op_map.gem.offset;
 
 			if (end == req_end) {
-				ret = op_unmap_cb(ops, priv, va, merge);
-				if (ret)
-					return ret;
+				if (!is_madvise_ops) {
+					ret = op_unmap_cb(ops, priv, va, merge);
+					if (ret)
+						return ret;
+				}
 				break;
 			}
 
 			if (end < req_end) {
-				ret = op_unmap_cb(ops, priv, va, merge);
-				if (ret)
-					return ret;
+				if (!is_madvise_ops) {
+					ret = op_unmap_cb(ops, priv, va, merge);
+					if (ret)
+						return ret;
+				}
 				continue;
 			}
 
 			if (end > req_end) {
+				if (skip_madvise_ops)
+					break;
+
 				struct drm_gpuva_op_map n = {
 					.va.addr = req_end,
 					.va.range = range - req->op_map.va.range,
@@ -2156,6 +2167,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 				ret = op_remap_cb(ops, priv, NULL, &n, &u);
 				if (ret)
 					return ret;
+
+				if (is_madvise_ops)
+					needs_map = true;
 				break;
 			}
 		} else if (addr < req->op_map.va.addr) {
@@ -2173,20 +2187,45 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 			u.keep = merge;
 
 			if (end == req_end) {
+				if (skip_madvise_ops)
+					break;
+
 				ret = op_remap_cb(ops, priv, &p, NULL, &u);
 				if (ret)
 					return ret;
+
+				if (is_madvise_ops)
+					needs_map = true;
+
 				break;
 			}
 
 			if (end < req_end) {
+				if (skip_madvise_ops)
+					continue;
+
 				ret = op_remap_cb(ops, priv, &p, NULL, &u);
 				if (ret)
 					return ret;
+
+				if (is_madvise_ops) {
+					struct drm_gpuvm_map_req map_req = {
+						.op_map.va.addr =  req->op_map.va.addr,
+						.op_map.va.range = end - req->op_map.va.addr,
+					};
+
+					ret = op_map_cb(ops, priv, &map_req);
+					if (ret)
+						return ret;
+				}
+
 				continue;
 			}
 
 			if (end > req_end) {
+				if (skip_madvise_ops)
+					break;
+
 				struct drm_gpuva_op_map n = {
 					.va.addr = req_end,
 					.va.range = end - req_end,
@@ -2198,6 +2237,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 				ret = op_remap_cb(ops, priv, &p, &n, &u);
 				if (ret)
 					return ret;
+
+				if (is_madvise_ops)
+					needs_map = true;
 				break;
 			}
 		} else if (addr > req->op_map.va.addr) {
@@ -2206,20 +2248,29 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 					   (addr - req->op_map.va.addr);
 
 			if (end == req_end) {
-				ret = op_unmap_cb(ops, priv, va, merge);
-				if (ret)
-					return ret;
+				if (!is_madvise_ops) {
+					ret = op_unmap_cb(ops, priv, va, merge);
+					if (ret)
+						return ret;
+				}
+
 				break;
 			}
 
 			if (end < req_end) {
-				ret = op_unmap_cb(ops, priv, va, merge);
-				if (ret)
-					return ret;
+				if (!is_madvise_ops) {
+					ret = op_unmap_cb(ops, priv, va, merge);
+					if (ret)
+						return ret;
+				}
+
 				continue;
 			}
 
 			if (end > req_end) {
+				if (skip_madvise_ops)
+					break;
+
 				struct drm_gpuva_op_map n = {
 					.va.addr = req_end,
 					.va.range = end - req_end,
@@ -2234,12 +2285,20 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 				ret = op_remap_cb(ops, priv, NULL, &n, &u);
 				if (ret)
 					return ret;
+
+				if (is_madvise_ops) {
+					struct drm_gpuvm_map_req map_req = {
+						.op_map.va.addr =  addr,
+						.op_map.va.range = req_end - addr,
+					};
+
+					return op_map_cb(ops, priv, &map_req);
+				}
 				break;
 			}
 		}
 	}
-
-	return op_map_cb(ops, priv, req);
+	return needs_map ? op_map_cb(ops, priv, req) : 0;
 }
 
 static int
diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
index 116f77abd570..fa2b74a54534 100644
--- a/include/drm/drm_gpuvm.h
+++ b/include/drm/drm_gpuvm.h
@@ -1054,6 +1054,17 @@ enum drm_gpuvm_sm_map_ops_flags {
 	  * %DRM_GPUVM_SM_MAP_OPS_FLAG_NONE: DEFAULT sm_map ops
 	  */
 	DRM_GPUVM_SM_MAP_OPS_FLAG_NONE = 0,
+
+	/**
+	 * @DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE: This flag is used by
+	 * drm_gpuvm_sm_map_ops_create to iterate over GPUVMA's in the
+	 * user-provided range and split the existing non-GEM object VMA if the
+	 * start or end of the input range lies within it. The operations can
+	 * create up to 2 REMAPS and 2 MAPs. Unlike drm_gpuvm_sm_map_ops_flags
+	 * in default mode, the operation with this flag will never have UNMAPs
+	 * and merges, and can be without any final operations.
+	 */
+	DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE = BIT(0),
 };
 
 /**
-- 
2.34.1

Re: [PATCH v6 01/26] drm/gpuvm: Pass map arguments through a struct

[Matthew Brost]

On Thu, Aug 07, 2025 at 10:13:13PM +0530, Himal Prasad Ghimiray wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
> 
> We are about to pass more arguments to drm_gpuvm_sm_map[_ops_create](),
> so, before we do that, let's pass arguments through a struct instead
> of changing each call site every time a new optional argument is added.
> 
> v5
>  - Use drm_gpuva_op_map—same as drm_gpuvm_map_req
>  - Rebase changes for drm_gpuvm_sm_map_exec_lock()
>  - Fix kernel-docs
> 
> v6
>  - Use drm_gpuvm_map_req (Danilo/Matt)
> 
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Brendan King <Brendan.King@imgtec.com>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: Caterina Shablia <caterina.shablia@collabora.com>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Matthew Brost <matthew.brost@intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Cc: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Caterina Shablia <caterina.shablia@collabora.com>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> ---
>  drivers/gpu/drm/drm_gpuvm.c            | 105 ++++++++++---------------
>  drivers/gpu/drm/imagination/pvr_vm.c   |  15 ++--
>  drivers/gpu/drm/msm/msm_gem_vma.c      |  25 ++++--
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c |  11 ++-
>  drivers/gpu/drm/panthor/panthor_mmu.c  |  13 ++-
>  drivers/gpu/drm/xe/xe_vm.c             |  13 ++-
>  include/drm/drm_gpuvm.h                |  20 +++--
>  7 files changed, 114 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index bbc7fecb6f4a..b3a01c40001b 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -486,13 +486,18 @@
>   *				  u64 addr, u64 range,
>   *				  struct drm_gem_object *obj, u64 offset)
>   *	{
> + *		struct drm_gpuvm_map_req map_req = {
> + *		        .op_map.va.addr = addr,
> + *	                .op_map.va.range = range,
> + *	                .op_map.gem.obj = obj,
> + *	                .op_map.gem.offset = offset,
> + *	           };
>   *		struct drm_gpuva_ops *ops;
>   *		struct drm_gpuva_op *op
>   *		struct drm_gpuvm_bo *vm_bo;
>   *
>   *		driver_lock_va_space();
> - *		ops = drm_gpuvm_sm_map_ops_create(gpuvm, addr, range,
> - *						  obj, offset);
> + *		ops = drm_gpuvm_sm_map_ops_create(gpuvm, &map_req);
>   *		if (IS_ERR(ops))
>   *			return PTR_ERR(ops);
>   *
> @@ -2054,16 +2059,15 @@ EXPORT_SYMBOL_GPL(drm_gpuva_unmap);
>  
>  static int
>  op_map_cb(const struct drm_gpuvm_ops *fn, void *priv,
> -	  u64 addr, u64 range,
> -	  struct drm_gem_object *obj, u64 offset)
> +	  const struct drm_gpuvm_map_req *req)
>  {
>  	struct drm_gpuva_op op = {};
>  
>  	op.op = DRM_GPUVA_OP_MAP;
> -	op.map.va.addr = addr;
> -	op.map.va.range = range;
> -	op.map.gem.obj = obj;
> -	op.map.gem.offset = offset;
> +	op.map.va.addr = req->op_map.va.addr;
> +	op.map.va.range = req->op_map.va.range;
> +	op.map.gem.obj = req->op_map.gem.obj;
> +	op.map.gem.offset = req->op_map.gem.offset;
>  
>  	return fn->sm_step_map(&op, priv);
>  }
> @@ -2102,17 +2106,16 @@ op_unmap_cb(const struct drm_gpuvm_ops *fn, void *priv,
>  static int
>  __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  		   const struct drm_gpuvm_ops *ops, void *priv,
> -		   u64 req_addr, u64 req_range,
> -		   struct drm_gem_object *req_obj, u64 req_offset)
> +		   const struct drm_gpuvm_map_req *req)
>  {
>  	struct drm_gpuva *va, *next;
> -	u64 req_end = req_addr + req_range;
> +	u64 req_end = req->op_map.va.addr + req->op_map.va.range;
>  	int ret;
>  
> -	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req_addr, req_range)))
> +	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req->op_map.va.addr, req->op_map.va.range)))
>  		return -EINVAL;
>  
> -	drm_gpuvm_for_each_va_range_safe(va, next, gpuvm, req_addr, req_end) {
> +	drm_gpuvm_for_each_va_range_safe(va, next, gpuvm, req->op_map.va.addr, req_end) {
>  		struct drm_gem_object *obj = va->gem.obj;
>  		u64 offset = va->gem.offset;
>  		u64 addr = va->va.addr;
> @@ -2120,9 +2123,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  		u64 end = addr + range;
>  		bool merge = !!va->gem.obj;
>  
> -		if (addr == req_addr) {
> -			merge &= obj == req_obj &&
> -				 offset == req_offset;
> +		if (addr == req->op_map.va.addr) {
> +			merge &= obj == req->op_map.gem.obj &&
> +				 offset == req->op_map.gem.offset;
>  
>  			if (end == req_end) {
>  				ret = op_unmap_cb(ops, priv, va, merge);
> @@ -2141,9 +2144,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  			if (end > req_end) {
>  				struct drm_gpuva_op_map n = {
>  					.va.addr = req_end,
> -					.va.range = range - req_range,
> +					.va.range = range - req->op_map.va.range,
>  					.gem.obj = obj,
> -					.gem.offset = offset + req_range,
> +					.gem.offset = offset + req->op_map.va.range,
>  				};
>  				struct drm_gpuva_op_unmap u = {
>  					.va = va,
> @@ -2155,8 +2158,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  					return ret;
>  				break;
>  			}
> -		} else if (addr < req_addr) {
> -			u64 ls_range = req_addr - addr;
> +		} else if (addr < req->op_map.va.addr) {
> +			u64 ls_range = req->op_map.va.addr - addr;
>  			struct drm_gpuva_op_map p = {
>  				.va.addr = addr,
>  				.va.range = ls_range,
> @@ -2165,8 +2168,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  			};
>  			struct drm_gpuva_op_unmap u = { .va = va };
>  
> -			merge &= obj == req_obj &&
> -				 offset + ls_range == req_offset;
> +			merge &= obj == req->op_map.gem.obj &&
> +				 offset + ls_range == req->op_map.gem.offset;
>  			u.keep = merge;
>  
>  			if (end == req_end) {
> @@ -2189,7 +2192,7 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  					.va.range = end - req_end,
>  					.gem.obj = obj,
>  					.gem.offset = offset + ls_range +
> -						      req_range,
> +						      req->op_map.va.range,
>  				};
>  
>  				ret = op_remap_cb(ops, priv, &p, &n, &u);
> @@ -2197,10 +2200,10 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  					return ret;
>  				break;
>  			}
> -		} else if (addr > req_addr) {
> -			merge &= obj == req_obj &&
> -				 offset == req_offset +
> -					   (addr - req_addr);
> +		} else if (addr > req->op_map.va.addr) {
> +			merge &= obj == req->op_map.gem.obj &&
> +				 offset == req->op_map.gem.offset +
> +					   (addr - req->op_map.va.addr);
>  
>  			if (end == req_end) {
>  				ret = op_unmap_cb(ops, priv, va, merge);
> @@ -2236,9 +2239,7 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  		}
>  	}
>  
> -	return op_map_cb(ops, priv,
> -			 req_addr, req_range,
> -			 req_obj, req_offset);
> +	return op_map_cb(ops, priv, req);
>  }
>  
>  static int
> @@ -2303,10 +2304,7 @@ __drm_gpuvm_sm_unmap(struct drm_gpuvm *gpuvm,
>   * drm_gpuvm_sm_map() - calls the &drm_gpuva_op split/merge steps
>   * @gpuvm: the &drm_gpuvm representing the GPU VA space
>   * @priv: pointer to a driver private data structure
> - * @req_addr: the start address of the new mapping
> - * @req_range: the range of the new mapping
> - * @req_obj: the &drm_gem_object to map
> - * @req_offset: the offset within the &drm_gem_object
> + * @req: ptr to struct drm_gpuvm_map_req
>   *
>   * This function iterates the given range of the GPU VA space. It utilizes the
>   * &drm_gpuvm_ops to call back into the driver providing the split and merge
> @@ -2333,8 +2331,7 @@ __drm_gpuvm_sm_unmap(struct drm_gpuvm *gpuvm,
>   */
>  int
>  drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv,
> -		 u64 req_addr, u64 req_range,
> -		 struct drm_gem_object *req_obj, u64 req_offset)
> +		 const struct drm_gpuvm_map_req *req)
>  {
>  	const struct drm_gpuvm_ops *ops = gpuvm->ops;
>  
> @@ -2343,9 +2340,7 @@ drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv,
>  		       ops->sm_step_unmap)))
>  		return -EINVAL;
>  
> -	return __drm_gpuvm_sm_map(gpuvm, ops, priv,
> -				  req_addr, req_range,
> -				  req_obj, req_offset);
> +	return __drm_gpuvm_sm_map(gpuvm, ops, priv, req);
>  }
>  EXPORT_SYMBOL_GPL(drm_gpuvm_sm_map);
>  
> @@ -2421,10 +2416,7 @@ static const struct drm_gpuvm_ops lock_ops = {
>   * @gpuvm: the &drm_gpuvm representing the GPU VA space
>   * @exec: the &drm_exec locking context
>   * @num_fences: for newly mapped objects, the # of fences to reserve
> - * @req_addr: the start address of the range to unmap
> - * @req_range: the range of the mappings to unmap
> - * @req_obj: the &drm_gem_object to map
> - * @req_offset: the offset within the &drm_gem_object
> + * @req: ptr to drm_gpuvm_map_req struct
>   *
>   * This function locks (drm_exec_lock_obj()) objects that will be unmapped/
>   * remapped, and locks+prepares (drm_exec_prepare_object()) objects that
> @@ -2445,9 +2437,7 @@ static const struct drm_gpuvm_ops lock_ops = {
>   *                    ret = drm_gpuvm_sm_unmap_exec_lock(gpuvm, &exec, op->addr, op->range);
>   *                    break;
>   *                case DRIVER_OP_MAP:
> - *                    ret = drm_gpuvm_sm_map_exec_lock(gpuvm, &exec, num_fences,
> - *                                                     op->addr, op->range,
> - *                                                     obj, op->obj_offset);
> + *                    ret = drm_gpuvm_sm_map_exec_lock(gpuvm, &exec, num_fences, &req);
>   *                    break;
>   *                }
>   *
> @@ -2478,18 +2468,15 @@ static const struct drm_gpuvm_ops lock_ops = {
>  int
>  drm_gpuvm_sm_map_exec_lock(struct drm_gpuvm *gpuvm,
>  			   struct drm_exec *exec, unsigned int num_fences,
> -			   u64 req_addr, u64 req_range,
> -			   struct drm_gem_object *req_obj, u64 req_offset)
> +			   struct drm_gpuvm_map_req *req)
>  {
> -	if (req_obj) {
> -		int ret = drm_exec_prepare_obj(exec, req_obj, num_fences);
> +	if (req->op_map.gem.obj) {
> +		int ret = drm_exec_prepare_obj(exec, req->op_map.gem.obj, num_fences);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	return __drm_gpuvm_sm_map(gpuvm, &lock_ops, exec,
> -				  req_addr, req_range,
> -				  req_obj, req_offset);
> +	return __drm_gpuvm_sm_map(gpuvm, &lock_ops, exec, req);
>  
>  }
>  EXPORT_SYMBOL_GPL(drm_gpuvm_sm_map_exec_lock);
> @@ -2611,10 +2598,7 @@ static const struct drm_gpuvm_ops gpuvm_list_ops = {
>  /**
>   * drm_gpuvm_sm_map_ops_create() - creates the &drm_gpuva_ops to split and merge
>   * @gpuvm: the &drm_gpuvm representing the GPU VA space
> - * @req_addr: the start address of the new mapping
> - * @req_range: the range of the new mapping
> - * @req_obj: the &drm_gem_object to map
> - * @req_offset: the offset within the &drm_gem_object
> + * @req: map request arguments
>   *
>   * This function creates a list of operations to perform splitting and merging
>   * of existent mapping(s) with the newly requested one.
> @@ -2642,8 +2626,7 @@ static const struct drm_gpuvm_ops gpuvm_list_ops = {
>   */
>  struct drm_gpuva_ops *
>  drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
> -			    u64 req_addr, u64 req_range,
> -			    struct drm_gem_object *req_obj, u64 req_offset)
> +			    const struct drm_gpuvm_map_req *req)
>  {
>  	struct drm_gpuva_ops *ops;
>  	struct {
> @@ -2661,9 +2644,7 @@ drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
>  	args.vm = gpuvm;
>  	args.ops = ops;
>  
> -	ret = __drm_gpuvm_sm_map(gpuvm, &gpuvm_list_ops, &args,
> -				 req_addr, req_range,
> -				 req_obj, req_offset);
> +	ret = __drm_gpuvm_sm_map(gpuvm, &gpuvm_list_ops, &args, req);
>  	if (ret)
>  		goto err_free_ops;
>  
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 2896fa7501b1..0f6b4cdb5fd8 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -185,12 +185,17 @@ struct pvr_vm_bind_op {
>  static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op *bind_op)
>  {
>  	switch (bind_op->type) {
> -	case PVR_VM_BIND_TYPE_MAP:
> +	case PVR_VM_BIND_TYPE_MAP: {
> +		const struct drm_gpuvm_map_req map_req = {
> +			.op_map.va.addr = bind_op->device_addr,
> +			.op_map.va.range = bind_op->size,
> +			.op_map.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> +			.op_map.gem.offset = bind_op->offset,
> +		};
> +
>  		return drm_gpuvm_sm_map(&bind_op->vm_ctx->gpuvm_mgr,
> -					bind_op, bind_op->device_addr,
> -					bind_op->size,
> -					gem_from_pvr_gem(bind_op->pvr_obj),
> -					bind_op->offset);
> +					bind_op, &map_req);
> +	}
>  
>  	case PVR_VM_BIND_TYPE_UNMAP:
>  		return drm_gpuvm_sm_unmap(&bind_op->vm_ctx->gpuvm_mgr,
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 3cd8562a5109..2ca408c40369 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -1172,10 +1172,17 @@ vm_bind_job_lock_objects(struct msm_vm_bind_job *job, struct drm_exec *exec)
>  				break;
>  			case MSM_VM_BIND_OP_MAP:
>  			case MSM_VM_BIND_OP_MAP_NULL:
> -				ret = drm_gpuvm_sm_map_exec_lock(job->vm, exec, 1,
> -							    op->iova, op->range,
> -							    op->obj, op->obj_offset);
> +			{
> +				struct drm_gpuvm_map_req map_req = {
> +					.op_map.va.addr = op->iova,
> +					.op_map.va.range = op->range,
> +					.op_map.gem.obj = op->obj,
> +					.op_map.gem.offset = op->obj_offset,
> +				};
> +
> +				ret = drm_gpuvm_sm_map_exec_lock(job->vm, exec, 1, &map_req);
>  				break;
> +			}
>  			default:
>  				/*
>  				 * lookup_op() should have already thrown an error for
> @@ -1283,9 +1290,17 @@ vm_bind_job_prepare(struct msm_vm_bind_job *job)
>  				arg.flags |= MSM_VMA_DUMP;
>  			fallthrough;
>  		case MSM_VM_BIND_OP_MAP_NULL:
> -			ret = drm_gpuvm_sm_map(job->vm, &arg, op->iova,
> -					       op->range, op->obj, op->obj_offset);
> +		{
> +			struct drm_gpuvm_map_req map_req = {
> +				.op_map.va.addr = op->iova,
> +				.op_map.va.range = op->range,
> +				.op_map.gem.obj = op->obj,
> +				.op_map.gem.offset = op->obj_offset,
> +			};
> +
> +			ret = drm_gpuvm_sm_map(job->vm, &arg, &map_req);
>  			break;
> +		}
>  		default:
>  			/*
>  			 * lookup_op() should have already thrown an error for
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index ddfc46bc1b3e..92f87520eeb8 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1276,6 +1276,12 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
>  			break;
>  		case OP_MAP: {
>  			struct nouveau_uvma_region *reg;
> +			struct drm_gpuvm_map_req map_req = {
> +				.op_map.va.addr = op->va.addr,
> +				.op_map.va.range = op->va.range,
> +				.op_map.gem.obj = op->gem.obj,
> +				.op_map.gem.offset = op->gem.offset,
> +			};
>  
>  			reg = nouveau_uvma_region_find_first(uvmm,
>  							     op->va.addr,
> @@ -1301,10 +1307,7 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
>  			}
>  
>  			op->ops = drm_gpuvm_sm_map_ops_create(&uvmm->base,
> -							      op->va.addr,
> -							      op->va.range,
> -							      op->gem.obj,
> -							      op->gem.offset);
> +							      &map_req);
>  			if (IS_ERR(op->ops)) {
>  				ret = PTR_ERR(op->ops);
>  				goto unwind_continue;
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 4140f697ba5a..5ed4573b3a6b 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -2169,15 +2169,22 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
>  	mutex_lock(&vm->op_lock);
>  	vm->op_ctx = op;
>  	switch (op_type) {
> -	case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP:
> +	case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: {
> +		const struct drm_gpuvm_map_req map_req = {
> +			.op_map.va.addr = op->va.addr,
> +			.op_map.va.range = op->va.range,
> +			.op_map.gem.obj = op->map.vm_bo->obj,
> +			.op_map.gem.offset = op->map.bo_offset,
> +		};
> +
>  		if (vm->unusable) {
>  			ret = -EINVAL;
>  			break;
>  		}
>  
> -		ret = drm_gpuvm_sm_map(&vm->base, vm, op->va.addr, op->va.range,
> -				       op->map.vm_bo->obj, op->map.bo_offset);
> +		ret = drm_gpuvm_sm_map(&vm->base, vm, &map_req);
>  		break;
> +	}
>  
>  	case DRM_PANTHOR_VM_BIND_OP_TYPE_UNMAP:
>  		ret = drm_gpuvm_sm_unmap(&vm->base, vm, op->va.addr, op->va.range);
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 432ea325677d..9fcc52032a1d 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2316,10 +2316,17 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
>  
>  	switch (operation) {
>  	case DRM_XE_VM_BIND_OP_MAP:
> -	case DRM_XE_VM_BIND_OP_MAP_USERPTR:
> -		ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, addr, range,
> -						  obj, bo_offset_or_userptr);
> +	case DRM_XE_VM_BIND_OP_MAP_USERPTR: {
> +		struct drm_gpuvm_map_req map_req = {
> +			.op_map.va.addr = addr,
> +			.op_map.va.range = range,
> +			.op_map.gem.obj = obj,
> +			.op_map.gem.offset = bo_offset_or_userptr,
> +		};
> +
> +		ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, &map_req);
>  		break;
> +	}
>  	case DRM_XE_VM_BIND_OP_UNMAP:
>  		ops = drm_gpuvm_sm_unmap_ops_create(&vm->gpuvm, addr, range);
>  		break;
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index 274532facfd6..3cf0a84b8b08 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -1058,10 +1058,20 @@ struct drm_gpuva_ops {
>   */
>  #define drm_gpuva_next_op(op) list_next_entry(op, entry)
>  
> +/**
> + * struct drm_gpuvm_map_req - arguments passed to drm_gpuvm_sm_map[_ops_create]()
> + */
> +struct drm_gpuvm_map_req {
> +	/**
> +	 * @op_map: struct drm_gpuva_op_map
> +	 */
> +	struct drm_gpuva_op_map op_map;
> +};
> +
>  struct drm_gpuva_ops *
>  drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
> -			    u64 addr, u64 range,
> -			    struct drm_gem_object *obj, u64 offset);
> +			    const struct drm_gpuvm_map_req *req);
> +
>  struct drm_gpuva_ops *
>  drm_gpuvm_sm_unmap_ops_create(struct drm_gpuvm *gpuvm,
>  			      u64 addr, u64 range);
> @@ -1205,16 +1215,14 @@ struct drm_gpuvm_ops {
>  };
>  
>  int drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv,
> -		     u64 addr, u64 range,
> -		     struct drm_gem_object *obj, u64 offset);
> +		     const struct drm_gpuvm_map_req *req);
>  
>  int drm_gpuvm_sm_unmap(struct drm_gpuvm *gpuvm, void *priv,
>  		       u64 addr, u64 range);
>  
>  int drm_gpuvm_sm_map_exec_lock(struct drm_gpuvm *gpuvm,
>  			  struct drm_exec *exec, unsigned int num_fences,
> -			  u64 req_addr, u64 req_range,
> -			  struct drm_gem_object *obj, u64 offset);
> +			  struct drm_gpuvm_map_req *req);
>  
>  int drm_gpuvm_sm_unmap_exec_lock(struct drm_gpuvm *gpuvm, struct drm_exec *exec,
>  				 u64 req_addr, u64 req_range);
> -- 
> 2.34.1
> 

Re: [PATCH v6 03/26] drm/gpuvm: Support flags in drm_gpuvm_map_req

[Matthew Brost]

On Thu, Aug 07, 2025 at 10:13:15PM +0530, Himal Prasad Ghimiray wrote:
> This change adds support for passing flags to drm_gpuvm_sm_map() and
> sm_map_ops_create(), enabling future extensions that affect split/merge
> logic in drm_gpuvm.
> 
> v2
> - Move flag to drm_gpuvm_map_req
> 
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: Caterina Shablia <caterina.shablia@collabora.com>
> Cc: Matthew Brost <matthew.brost@intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Cc: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> ---
>  include/drm/drm_gpuvm.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index cbb9b6519462..116f77abd570 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -1049,6 +1049,13 @@ struct drm_gpuva_ops {
>   */
>  #define drm_gpuva_next_op(op) list_next_entry(op, entry)
>  
> +enum drm_gpuvm_sm_map_ops_flags {
> +	 /**
> +	  * %DRM_GPUVM_SM_MAP_OPS_FLAG_NONE: DEFAULT sm_map ops
> +	  */
> +	DRM_GPUVM_SM_MAP_OPS_FLAG_NONE = 0,
> +};
> +
>  /**
>   * struct drm_gpuvm_map_req - arguments passed to drm_gpuvm_sm_map[_ops_create]()
>   */
> @@ -1057,6 +1064,11 @@ struct drm_gpuvm_map_req {
>  	 * @op_map: struct drm_gpuva_op_map
>  	 */
>  	struct drm_gpuva_op_map op_map;
> +
> +	/**
> +	 * @flags: drm_gpuvm_sm_map_ops_flags for this mapping request
> +	 */
> +	enum drm_gpuvm_sm_map_ops_flags flags;
>  };
>  
>  struct drm_gpuva_ops *
> -- 
> 2.34.1
> 

Re: [PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Matthew Brost]

On Thu, Aug 07, 2025 at 10:13:16PM +0530, Himal Prasad Ghimiray wrote:
> - DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE: This flag is used by
>   drm_gpuvm_sm_map_ops_create to iterate over GPUVMA's in the
> user-provided range and split the existing non-GEM object VMA if the
> start or end of the input range lies within it. The operations can
> create up to 2 REMAPS and 2 MAPs. The purpose of this operation is to be
> used by the Xe driver to assign attributes to GPUVMA's within the
> user-defined range. Unlike drm_gpuvm_sm_map_ops_flags in default mode,
> the operation with this flag will never have UNMAPs and
> merges, and can be without any final operations.
> 
> v2
> - use drm_gpuvm_sm_map_ops_create with flags instead of defining new
>   ops_create (Danilo)
> - Add doc (Danilo)
> 
> v3
> - Fix doc
> - Fix unmapping check
> 
> v4
> - Fix mapping for non madvise ops
> 
> v5
> - Fix mapping (Matthew Brost)
> - Rebase on top of struct changes
> 
> v6
> - flag moved to map_req
> 

I’ll give this an RB—it looks right to me, though it’s a bit hard to be
certain. Before merge, I’d like to see xe_exec_system_allocator add a
section(s) that calls madvise() on each newly allocated memory; that should
creatd enough random fragmentation—particularly with threaded
sections—in the VMA state to be confident this is correct.

With that:
Reviewed-by: Matthew Brost matthew.brost@intel.com

> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Himal Prasad Ghimiray<himal.prasad.ghimiray@intel.com>
> ---
>  drivers/gpu/drm/drm_gpuvm.c | 87 +++++++++++++++++++++++++++++++------
>  include/drm/drm_gpuvm.h     | 11 +++++
>  2 files changed, 84 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index b3a01c40001b..d8f5f594a415 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -2110,6 +2110,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  {
>  	struct drm_gpuva *va, *next;
>  	u64 req_end = req->op_map.va.addr + req->op_map.va.range;
> +	bool is_madvise_ops = (req->flags & DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE);
> +	bool needs_map = !is_madvise_ops;
>  	int ret;
>  
>  	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req->op_map.va.addr, req->op_map.va.range)))
> @@ -2122,26 +2124,35 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  		u64 range = va->va.range;
>  		u64 end = addr + range;
>  		bool merge = !!va->gem.obj;
> +		bool skip_madvise_ops = is_madvise_ops && merge;
>  
> +		needs_map = !is_madvise_ops;
>  		if (addr == req->op_map.va.addr) {
>  			merge &= obj == req->op_map.gem.obj &&
>  				 offset == req->op_map.gem.offset;
>  
>  			if (end == req_end) {
> -				ret = op_unmap_cb(ops, priv, va, merge);
> -				if (ret)
> -					return ret;
> +				if (!is_madvise_ops) {
> +					ret = op_unmap_cb(ops, priv, va, merge);
> +					if (ret)
> +						return ret;
> +				}
>  				break;
>  			}
>  
>  			if (end < req_end) {
> -				ret = op_unmap_cb(ops, priv, va, merge);
> -				if (ret)
> -					return ret;
> +				if (!is_madvise_ops) {
> +					ret = op_unmap_cb(ops, priv, va, merge);
> +					if (ret)
> +						return ret;
> +				}
>  				continue;
>  			}
>  
>  			if (end > req_end) {
> +				if (skip_madvise_ops)
> +					break;
> +
>  				struct drm_gpuva_op_map n = {
>  					.va.addr = req_end,
>  					.va.range = range - req->op_map.va.range,
> @@ -2156,6 +2167,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  				ret = op_remap_cb(ops, priv, NULL, &n, &u);
>  				if (ret)
>  					return ret;
> +
> +				if (is_madvise_ops)
> +					needs_map = true;
>  				break;
>  			}
>  		} else if (addr < req->op_map.va.addr) {
> @@ -2173,20 +2187,45 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  			u.keep = merge;
>  
>  			if (end == req_end) {
> +				if (skip_madvise_ops)
> +					break;
> +
>  				ret = op_remap_cb(ops, priv, &p, NULL, &u);
>  				if (ret)
>  					return ret;
> +
> +				if (is_madvise_ops)
> +					needs_map = true;
> +
>  				break;
>  			}
>  
>  			if (end < req_end) {
> +				if (skip_madvise_ops)
> +					continue;
> +
>  				ret = op_remap_cb(ops, priv, &p, NULL, &u);
>  				if (ret)
>  					return ret;
> +
> +				if (is_madvise_ops) {
> +					struct drm_gpuvm_map_req map_req = {
> +						.op_map.va.addr =  req->op_map.va.addr,
> +						.op_map.va.range = end - req->op_map.va.addr,
> +					};
> +
> +					ret = op_map_cb(ops, priv, &map_req);
> +					if (ret)
> +						return ret;
> +				}
> +
>  				continue;
>  			}
>  
>  			if (end > req_end) {
> +				if (skip_madvise_ops)
> +					break;
> +
>  				struct drm_gpuva_op_map n = {
>  					.va.addr = req_end,
>  					.va.range = end - req_end,
> @@ -2198,6 +2237,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  				ret = op_remap_cb(ops, priv, &p, &n, &u);
>  				if (ret)
>  					return ret;
> +
> +				if (is_madvise_ops)
> +					needs_map = true;
>  				break;
>  			}
>  		} else if (addr > req->op_map.va.addr) {
> @@ -2206,20 +2248,29 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  					   (addr - req->op_map.va.addr);
>  
>  			if (end == req_end) {
> -				ret = op_unmap_cb(ops, priv, va, merge);
> -				if (ret)
> -					return ret;
> +				if (!is_madvise_ops) {
> +					ret = op_unmap_cb(ops, priv, va, merge);
> +					if (ret)
> +						return ret;
> +				}
> +
>  				break;
>  			}
>  
>  			if (end < req_end) {
> -				ret = op_unmap_cb(ops, priv, va, merge);
> -				if (ret)
> -					return ret;
> +				if (!is_madvise_ops) {
> +					ret = op_unmap_cb(ops, priv, va, merge);
> +					if (ret)
> +						return ret;
> +				}
> +
>  				continue;
>  			}
>  
>  			if (end > req_end) {
> +				if (skip_madvise_ops)
> +					break;
> +
>  				struct drm_gpuva_op_map n = {
>  					.va.addr = req_end,
>  					.va.range = end - req_end,
> @@ -2234,12 +2285,20 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  				ret = op_remap_cb(ops, priv, NULL, &n, &u);
>  				if (ret)
>  					return ret;
> +
> +				if (is_madvise_ops) {
> +					struct drm_gpuvm_map_req map_req = {
> +						.op_map.va.addr =  addr,
> +						.op_map.va.range = req_end - addr,
> +					};
> +
> +					return op_map_cb(ops, priv, &map_req);
> +				}
>  				break;
>  			}
>  		}
>  	}
> -
> -	return op_map_cb(ops, priv, req);
> +	return needs_map ? op_map_cb(ops, priv, req) : 0;
>  }
>  
>  static int
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index 116f77abd570..fa2b74a54534 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -1054,6 +1054,17 @@ enum drm_gpuvm_sm_map_ops_flags {
>  	  * %DRM_GPUVM_SM_MAP_OPS_FLAG_NONE: DEFAULT sm_map ops
>  	  */
>  	DRM_GPUVM_SM_MAP_OPS_FLAG_NONE = 0,
> +
> +	/**
> +	 * @DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE: This flag is used by
> +	 * drm_gpuvm_sm_map_ops_create to iterate over GPUVMA's in the
> +	 * user-provided range and split the existing non-GEM object VMA if the
> +	 * start or end of the input range lies within it. The operations can
> +	 * create up to 2 REMAPS and 2 MAPs. Unlike drm_gpuvm_sm_map_ops_flags
> +	 * in default mode, the operation with this flag will never have UNMAPs
> +	 * and merges, and can be without any final operations.
> +	 */
> +	DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE = BIT(0),
>  };
>  
>  /**
> -- 
> 2.34.1
> 

Re: [PATCH v6 01/26] drm/gpuvm: Pass map arguments through a struct

[Danilo Krummrich]

On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
>
> We are about to pass more arguments to drm_gpuvm_sm_map[_ops_create](),
> so, before we do that, let's pass arguments through a struct instead
> of changing each call site every time a new optional argument is added.
>
> v5
>  - Use drm_gpuva_op_map—same as drm_gpuvm_map_req
>  - Rebase changes for drm_gpuvm_sm_map_exec_lock()
>  - Fix kernel-docs
>
> v6
>  - Use drm_gpuvm_map_req (Danilo/Matt)
>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Brendan King <Brendan.King@imgtec.com>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: Caterina Shablia <caterina.shablia@collabora.com>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Caterina Shablia <caterina.shablia@collabora.com>

Caterina does not seem to be involved in handling this patch. Either you should
remove this SoB or adda Co-developed-by: tag for her if that's the case.

> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

You may also want to add a Co-developed-by: tag for yourself given that you made
significant changes to the patch. But that's between Boris and you of course.

> +/**
> + * struct drm_gpuvm_map_req - arguments passed to drm_gpuvm_sm_map[_ops_create]()
> + */
> +struct drm_gpuvm_map_req {
> +	/**
> +	 * @op_map: struct drm_gpuva_op_map
> +	 */
> +	struct drm_gpuva_op_map op_map;

I think this should just be 'op', the outer structure says 'map' already.

> +};

Re: [PATCH v6 03/26] drm/gpuvm: Support flags in drm_gpuvm_map_req

[Danilo Krummrich]

On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
> This change adds support for passing flags to drm_gpuvm_sm_map() and
> sm_map_ops_create(), enabling future extensions that affect split/merge
> logic in drm_gpuvm.
>
> v2
> - Move flag to drm_gpuvm_map_req
>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: Caterina Shablia <caterina.shablia@collabora.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> ---
>  include/drm/drm_gpuvm.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index cbb9b6519462..116f77abd570 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -1049,6 +1049,13 @@ struct drm_gpuva_ops {
>   */
>  #define drm_gpuva_next_op(op) list_next_entry(op, entry)
>  
> +enum drm_gpuvm_sm_map_ops_flags {

Please also add a doc-comment for the enum type itself, explaing where those
flags are used, etc.

> +	 /**
> +	  * %DRM_GPUVM_SM_MAP_OPS_FLAG_NONE: DEFAULT sm_map ops

Shouldn't this be '@DRM_GPUVM_SM_MAP_OPS_FLAG_NONE:'?

> +	  */
> +	DRM_GPUVM_SM_MAP_OPS_FLAG_NONE = 0,
> +};

Re: [PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Danilo Krummrich]

On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
> - DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE: This flag is used by
>   drm_gpuvm_sm_map_ops_create to iterate over GPUVMA's in the
> user-provided range and split the existing non-GEM object VMA if the

What do you mean with non-GEM object VMA? I assume you just mean sparse
mappings?

> start or end of the input range lies within it. The operations can
> create up to 2 REMAPS and 2 MAPs.

Wait -- that doesn't make sense with what I thought how this works.

In the RFC you gave the following example:

	Example:
	Input Range: 0x00007f0a54000000 to 0x00007f0a54400000
	GPU VMA: 0x0000000000000000 to 0x0000800000000000
	Operations Result:
	- REMAP:UNMAP: addr=0x0000000000000000, range=0x0000800000000000
	- REMAP:PREV: addr=0x0000000000000000, range=0x00007f0a54000000
	- REMAP:NEXT: addr=0x00007f0a54400000, range=0x000000f5abc00000
	- MAP: addr=0x00007f0a54000000, range=0x0000000000400000

That's exactly the same what the existing logic does. So in which case do you
have *two* MAP operations?

For completeness, the other example you gave was:

	Example:
	Input Range: 0x00007fc898800000 to 0x00007fc899000000
	GPU VMAs:
	- 0x0000000000000000 to 0x00007fc898800000
	- 0x00007fc898800000 to 0x00007fc898a00000
	- 0x00007fc898a00000 to 0x00007fc898c00000
	- 0x00007fc898c00000 to 0x00007fc899000000
	- 0x00007fc899000000 to 0x00007fc899200000
	Operations Result: None

This just means, if things are split already, at the defined edges, just don't
do anything, which is also conform with the existing logic except for the "no
merge" part, which is obviously fine given that it's explicitly for splitting
things.

Can you please provide some additional *simple* examples, like the documentation
of GPUVM does today for the normal split/merge stuff? I.e. please don't use
complex real addresses, that makes it hard to parse.

Also, can you please provide some information on what this whole thing does
*semantically*? I thought I understood it, but now I'm not so sure anymore.

> The purpose of this operation is to be
> used by the Xe driver to assign attributes to GPUVMA's within the
> user-defined range.

Well, hopefully it's useful to other drivers as well. :)

> Unlike drm_gpuvm_sm_map_ops_flags in default mode,
> the operation with this flag will never have UNMAPs and
> merges, and can be without any final operations.

I really think this is significant enough of a feature to add some proper
documentation about it.

Please add a separate section about madvise operations to the documentation at
the beginning of the drivers/gpu/drm/drm_gpuvm.c file.

>
> v2
> - use drm_gpuvm_sm_map_ops_create with flags instead of defining new
>   ops_create (Danilo)

If this turns out not to be what I thought semantically and we still agree it's
the correct approach, I think I have to take this back and it should indeed be
an entirely separate code path. But let's wait for your answers above.

Again, I really think this needs some proper documentation like in the
"DOC: Split and Merge" documentation section.

Re: [PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Ghimiray, Himal Prasad]

On 09-08-2025 18:53, Danilo Krummrich wrote:
> On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
>> - DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE: This flag is used by
>>    drm_gpuvm_sm_map_ops_create to iterate over GPUVMA's in the
>> user-provided range and split the existing non-GEM object VMA if the
> 
> What do you mean with non-GEM object VMA? I assume you just mean sparse
> mappings?

True>
>> start or end of the input range lies within it. The operations can
>> create up to 2 REMAPS and 2 MAPs.
> 
> Wait -- that doesn't make sense with what I thought how this works.
> 
> In the RFC you gave the following example:
> 
> 	Example:
> 	Input Range: 0x00007f0a54000000 to 0x00007f0a54400000
> 	GPU VMA: 0x0000000000000000 to 0x0000800000000000
> 	Operations Result:
> 	- REMAP:UNMAP: addr=0x0000000000000000, range=0x0000800000000000
> 	- REMAP:PREV: addr=0x0000000000000000, range=0x00007f0a54000000
> 	- REMAP:NEXT: addr=0x00007f0a54400000, range=0x000000f5abc00000
> 	- MAP: addr=0x00007f0a54000000, range=0x0000000000400000
> 
> That's exactly the same what the existing logic does. So in which case do you
> have *two* MAP operations?

Possible scenarios for ops functionality based on input start and end 
address from user:

a) User-provided range is a subset of an existing drm_gpuva
Expected Result: Same behavior as the default sm_map logic.
Reference: Case 1 from [1].

b) Either start or end (but not both) is not aligned with a drm_gpuva 
boundary
Expected Result: One REMAP and one MAP operation.
Reference: Case 3 from [1].

Existing GPUVMAs:

          drm_gpuva1                        drm_gpuva2
	[a----------------------------b-1][b-------------------c-1]

User Input to ops:
   start = inside drm_gpuva1
   end   = exactly at c-1 (end of drm_gpuva2)

Resulting Mapping:
	drm_gpuva1:pre       drm_gpuva:New map     drm_gpuva2
	[a---------start-1][start------- b-1] [b------------c-1]

Ops Created:
   REMAP:UNMAP drm_gpuva1 a to b
   REMAP:PREV a to start - 1
   MAP: start to b-1

Note: No unmap of drm_gpuvma2 and no merging of New map and drm_gpuva2.

c) Both start and end are not aligned with drm_gpuva boundaries, and 
they fall within different drm_gpuva regions
Expected Result: Two REMAP operations and two MAP operations.
Reference: Case 2 from [1].


d) User-provided range does not overlap with any existing drm_gpuva
Expected Result: No operations.
start and end exactly match the boundaries of one or more existing 
drm_gpuva regions

e) This includes cases where start is at the beginning of drm_gpuva1 and 
end is at the end of drm_gpuva2 (drm_gpuva1 and drm_gpuva2 can be same 
or different).
Expected Result: No operations

[1] 
https://lore.kernel.org/intel-xe/4203f450-4b49-401d-81a8-cdcca02035f9@intel.com/ 


> 
> For completeness, the other example you gave was:
> 
> 	Example:
> 	Input Range: 0x00007fc898800000 to 0x00007fc899000000
> 	GPU VMAs:
> 	- 0x0000000000000000 to 0x00007fc898800000
> 	- 0x00007fc898800000 to 0x00007fc898a00000
> 	- 0x00007fc898a00000 to 0x00007fc898c00000
> 	- 0x00007fc898c00000 to 0x00007fc899000000
> 	- 0x00007fc899000000 to 0x00007fc899200000
> 	Operations Result: None
> 
> This just means, if things are split already, at the defined edges, just don't
> do anything, which is also conform with the existing logic except for the "no
> merge" part, which is obviously fine given that it's explicitly for splitting
> things.
> 
> Can you please provide some additional *simple* examples, like the documentation
> of GPUVM does today for the normal split/merge stuff? I.e. please don't use
> complex real addresses, that makes it hard to parse.
> 
> Also, can you please provide some information on what this whole thing does
> *semantically*? I thought I understood it, but now I'm not so sure anymore.
> 

I’ve tried to explain the behavior/usecase with madvise and expected 
outcomes of the ops logic in detail in [1]. Could you please take a 
moment to review that and let me know if the explanation is sufficient 
or if any part needs further clarification?

>> The purpose of this operation is to be
>> used by the Xe driver to assign attributes to GPUVMA's within the
>> user-defined range.
> 
> Well, hopefully it's useful to other drivers as well. :)

It should be. :)

> 
>> Unlike drm_gpuvm_sm_map_ops_flags in default mode,
>> the operation with this flag will never have UNMAPs and
>> merges, and can be without any final operations.
> 
> I really think this is significant enough of a feature to add some proper
> documentation about it.
> 
> Please add a separate section about madvise operations to the documentation at
> the beginning of the drivers/gpu/drm/drm_gpuvm.c file.

Sure will do that.

> 
>>
>> v2
>> - use drm_gpuvm_sm_map_ops_create with flags instead of defining new
>>    ops_create (Danilo)
> 
> If this turns out not to be what I thought semantically and we still agree it's
> the correct approach, I think I have to take this back and it should indeed be
> an entirely separate code path. But let's wait for your answers above.
> 
> Again, I really think this needs some proper documentation like in the
> "DOC: Split and Merge" documentation section.

Sure

Re: [PATCH v6 01/26] drm/gpuvm: Pass map arguments through a struct

[Ghimiray, Himal Prasad]

On 09-08-2025 18:13, Danilo Krummrich wrote:
> On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
>> From: Boris Brezillon <boris.brezillon@collabora.com>
>>
>> We are about to pass more arguments to drm_gpuvm_sm_map[_ops_create](),
>> so, before we do that, let's pass arguments through a struct instead
>> of changing each call site every time a new optional argument is added.
>>
>> v5
>>   - Use drm_gpuva_op_map—same as drm_gpuvm_map_req
>>   - Rebase changes for drm_gpuvm_sm_map_exec_lock()
>>   - Fix kernel-docs
>>
>> v6
>>   - Use drm_gpuvm_map_req (Danilo/Matt)
>>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: Brendan King <Brendan.King@imgtec.com>
>> Cc: Boris Brezillon <bbrezillon@kernel.org>
>> Cc: Caterina Shablia <caterina.shablia@collabora.com>
>> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: <dri-devel@lists.freedesktop.org>
>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>> Signed-off-by: Caterina Shablia <caterina.shablia@collabora.com>
> 
> Caterina does not seem to be involved in handling this patch. Either you should
> remove this SoB or adda Co-developed-by: tag for her if that's the case.

Caterina will it be ok to remove this SoB ?

> 
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> 
> You may also want to add a Co-developed-by: tag for yourself given that you made
> significant changes to the patch. But that's between Boris and you of course.

If Boris doesn't have any objection would go ahead and add 
Co-developed-by: tag for myself.

> 
>> +/**
>> + * struct drm_gpuvm_map_req - arguments passed to drm_gpuvm_sm_map[_ops_create]()
>> + */
>> +struct drm_gpuvm_map_req {
>> +	/**
>> +	 * @op_map: struct drm_gpuva_op_map
>> +	 */
>> +	struct drm_gpuva_op_map op_map;
> 
> I think this should just be 'op', the outer structure says 'map' already.

Makes sense. Will update in next patch.

> 
>> +};

Re: [PATCH v6 03/26] drm/gpuvm: Support flags in drm_gpuvm_map_req

[Ghimiray, Himal Prasad]

On 09-08-2025 18:16, Danilo Krummrich wrote:
> On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
>> This change adds support for passing flags to drm_gpuvm_sm_map() and
>> sm_map_ops_create(), enabling future extensions that affect split/merge
>> logic in drm_gpuvm.
>>
>> v2
>> - Move flag to drm_gpuvm_map_req
>>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: Boris Brezillon <bbrezillon@kernel.org>
>> Cc: Caterina Shablia <caterina.shablia@collabora.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: <dri-devel@lists.freedesktop.org>
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>>   include/drm/drm_gpuvm.h | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
>> index cbb9b6519462..116f77abd570 100644
>> --- a/include/drm/drm_gpuvm.h
>> +++ b/include/drm/drm_gpuvm.h
>> @@ -1049,6 +1049,13 @@ struct drm_gpuva_ops {
>>    */
>>   #define drm_gpuva_next_op(op) list_next_entry(op, entry)
>>   
>> +enum drm_gpuvm_sm_map_ops_flags {
> 
> Please also add a doc-comment for the enum type itself, explaing where those
> flags are used, etc.

sure will do.

> 
>> +	 /**
>> +	  * %DRM_GPUVM_SM_MAP_OPS_FLAG_NONE: DEFAULT sm_map ops
> 
> Shouldn't this be '@DRM_GPUVM_SM_MAP_OPS_FLAG_NONE:'?

Yup. will change in next version.

> 
>> +	  */
>> +	DRM_GPUVM_SM_MAP_OPS_FLAG_NONE = 0,
>> +};

Re: [PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Danilo Krummrich]

On Mon Aug 11, 2025 at 8:52 AM CEST, Himal Prasad Ghimiray wrote:
> On 09-08-2025 18:53, Danilo Krummrich wrote:
> Possible scenarios for ops functionality based on input start and end 
> address from user:
>
> a) User-provided range is a subset of an existing drm_gpuva
> Expected Result: Same behavior as the default sm_map logic.
> Reference: Case 1 from [1].
>
> b) Either start or end (but not both) is not aligned with a drm_gpuva 
> boundary
> Expected Result: One REMAP and one MAP operation.
> Reference: Case 3 from [1].
>
> Existing GPUVMAs:
>
>           drm_gpuva1                        drm_gpuva2
> 	[a----------------------------b-1][b-------------------c-1]
>
> User Input to ops:
>    start = inside drm_gpuva1
>    end   = exactly at c-1 (end of drm_gpuva2)
>
> Resulting Mapping:
> 	drm_gpuva1:pre       drm_gpuva:New map     drm_gpuva2
> 	[a---------start-1][start------- b-1] [b------------c-1]
>
> Ops Created:
>    REMAP:UNMAP drm_gpuva1 a to b
>    REMAP:PREV a to start - 1
>    MAP: start to b-1
>
> Note: No unmap of drm_gpuvma2 and no merging of New map and drm_gpuva2.
>
> c) Both start and end are not aligned with drm_gpuva boundaries, and 
> they fall within different drm_gpuva regions
> Expected Result: Two REMAP operations and two MAP operations.
> Reference: Case 2 from [1].
>
>
> d) User-provided range does not overlap with any existing drm_gpuva
> Expected Result: No operations.
> start and end exactly match the boundaries of one or more existing 
> drm_gpuva regions
>
> e) This includes cases where start is at the beginning of drm_gpuva1 and 
> end is at the end of drm_gpuva2 (drm_gpuva1 and drm_gpuva2 can be same 
> or different).
> Expected Result: No operations
>
> [1] 
> https://lore.kernel.org/intel-xe/4203f450-4b49-401d-81a8-cdcca02035f9@intel.com/ 

<snip>

> I’ve tried to explain the behavior/usecase with madvise and expected 
> outcomes of the ops logic in detail in [1]. Could you please take a 
> moment to review that and let me know if the explanation is sufficient 
> or if any part needs further clarification?

Thanks a lot for writing this up!

I think this clarifies everything, the examples from [1] are good (sorry that
your reply from the RFC got lost somehow on my end).

>> Please add a separate section about madvise operations to the documentation at
>> the beginning of the drivers/gpu/drm/drm_gpuvm.c file.
>
> Sure will do that.

Great, this will help users (as well as reviewers) a lot. Please also add your
examples from [1] to this entry, similar to the existing examples for sm_map.

>>> v2
>>> - use drm_gpuvm_sm_map_ops_create with flags instead of defining new
>>>    ops_create (Danilo)
>> 
>> If this turns out not to be what I thought semantically and we still agree it's
>> the correct approach, I think I have to take this back and it should indeed be
>> an entirely separate code path. But let's wait for your answers above.

Having the correct understanding of how this is supposed to work (and seeing how
the code turns out) I think it's still OK to integrate it into sm_map().

However, it probably makes sense to factor out the code into a common function
and then build the madvise() and sm_map() functions on top of it.

Please also find some more comments on the patch itself.

>> Again, I really think this needs some proper documentation like in the
>> "DOC: Split and Merge" documentation section.
>
> Sure

Thanks!

Re: [PATCH v6 01/26] drm/gpuvm: Pass map arguments through a struct

[Danilo Krummrich]

On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
> @@ -2102,17 +2106,16 @@ op_unmap_cb(const struct drm_gpuvm_ops *fn, void *priv,
>  static int
>  __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  		   const struct drm_gpuvm_ops *ops, void *priv,
> -		   u64 req_addr, u64 req_range,
> -		   struct drm_gem_object *req_obj, u64 req_offset)
> +		   const struct drm_gpuvm_map_req *req)
>  {
>  	struct drm_gpuva *va, *next;
> -	u64 req_end = req_addr + req_range;
> +	u64 req_end = req->op_map.va.addr + req->op_map.va.range;

Forgot to add, please extract all previous values from req, such that the below
diff is minimal and the code remiains easier to read..

Re: [PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Danilo Krummrich]

On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
> @@ -2110,6 +2110,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  {
>  	struct drm_gpuva *va, *next;
>  	u64 req_end = req->op_map.va.addr + req->op_map.va.range;
> +	bool is_madvise_ops = (req->flags & DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE);

Let's just call this 'madvise'.

> +	bool needs_map = !is_madvise_ops;
>  	int ret;
>  
>  	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req->op_map.va.addr, req->op_map.va.range)))
> @@ -2122,26 +2124,35 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  		u64 range = va->va.range;
>  		u64 end = addr + range;
>  		bool merge = !!va->gem.obj;
> +		bool skip_madvise_ops = is_madvise_ops && merge;

IIUC, you're either going for continue or break in this case. I think continue
would always be correct and break is an optimization if end <= req_end?

If that's correct, please just do either

	if (madvise && va->gem.obj)
		continue;

or

	if (madvise && va->gem.obj) {
		if (end > req_end)
			break;
		else
			continue;
	}

instead of sprinkling the skip_madvise_ops checks everywhere.

>  
> +		needs_map = !is_madvise_ops;
>  		if (addr == req->op_map.va.addr) {
>  			merge &= obj == req->op_map.gem.obj &&
>  				 offset == req->op_map.gem.offset;
>  
>  			if (end == req_end) {
> -				ret = op_unmap_cb(ops, priv, va, merge);
> -				if (ret)
> -					return ret;
> +				if (!is_madvise_ops) {
> +					ret = op_unmap_cb(ops, priv, va, merge);
> +					if (ret)
> +						return ret;
> +				}
>  				break;
>  			}
>  
>  			if (end < req_end) {
> -				ret = op_unmap_cb(ops, priv, va, merge);
> -				if (ret)
> -					return ret;
> +				if (!is_madvise_ops) {
> +					ret = op_unmap_cb(ops, priv, va, merge);

I think we should pass madvise as argument to op_unmap_cb() and make it a noop
internally rather than having all the conditionals.

> +					if (ret)
> +						return ret;
> +				}
>  				continue;
>  			}
>  
>  			if (end > req_end) {
> +				if (skip_madvise_ops)
> +					break;
> +
>  				struct drm_gpuva_op_map n = {
>  					.va.addr = req_end,
>  					.va.range = range - req->op_map.va.range,
> @@ -2156,6 +2167,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  				ret = op_remap_cb(ops, priv, NULL, &n, &u);
>  				if (ret)
>  					return ret;
> +
> +				if (is_madvise_ops)
> +					needs_map = true;

I don't like this needs_map state...

Maybe we could have

	struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;

at the beginning of the function and then change this line to

	if (madvise)
		op_map = req;

and op_map_cb() can just handle a NULL pointer.

Yeah, I feel like that's better.

Re: [PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Ghimiray, Himal Prasad]

On 12-08-2025 21:36, Danilo Krummrich wrote:
> On Mon Aug 11, 2025 at 8:52 AM CEST, Himal Prasad Ghimiray wrote:
>> On 09-08-2025 18:53, Danilo Krummrich wrote:
>> Possible scenarios for ops functionality based on input start and end
>> address from user:
>>
>> a) User-provided range is a subset of an existing drm_gpuva
>> Expected Result: Same behavior as the default sm_map logic.
>> Reference: Case 1 from [1].
>>
>> b) Either start or end (but not both) is not aligned with a drm_gpuva
>> boundary
>> Expected Result: One REMAP and one MAP operation.
>> Reference: Case 3 from [1].
>>
>> Existing GPUVMAs:
>>
>>            drm_gpuva1                        drm_gpuva2
>> 	[a----------------------------b-1][b-------------------c-1]
>>
>> User Input to ops:
>>     start = inside drm_gpuva1
>>     end   = exactly at c-1 (end of drm_gpuva2)
>>
>> Resulting Mapping:
>> 	drm_gpuva1:pre       drm_gpuva:New map     drm_gpuva2
>> 	[a---------start-1][start------- b-1] [b------------c-1]
>>
>> Ops Created:
>>     REMAP:UNMAP drm_gpuva1 a to b
>>     REMAP:PREV a to start - 1
>>     MAP: start to b-1
>>
>> Note: No unmap of drm_gpuvma2 and no merging of New map and drm_gpuva2.
>>
>> c) Both start and end are not aligned with drm_gpuva boundaries, and
>> they fall within different drm_gpuva regions
>> Expected Result: Two REMAP operations and two MAP operations.
>> Reference: Case 2 from [1].
>>
>>
>> d) User-provided range does not overlap with any existing drm_gpuva
>> Expected Result: No operations.
>> start and end exactly match the boundaries of one or more existing
>> drm_gpuva regions
>>
>> e) This includes cases where start is at the beginning of drm_gpuva1 and
>> end is at the end of drm_gpuva2 (drm_gpuva1 and drm_gpuva2 can be same
>> or different).
>> Expected Result: No operations
>>
>> [1]
>> https://lore.kernel.org/intel-xe/4203f450-4b49-401d-81a8-cdcca02035f9@intel.com/
> 
> <snip>
> 
>> I’ve tried to explain the behavior/usecase with madvise and expected
>> outcomes of the ops logic in detail in [1]. Could you please take a
>> moment to review that and let me know if the explanation is sufficient
>> or if any part needs further clarification?
> 
> Thanks a lot for writing this up!
> 
> I think this clarifies everything, the examples from [1] are good (sorry that
> your reply from the RFC got lost somehow on my end).
> 
>>> Please add a separate section about madvise operations to the documentation at
>>> the beginning of the drivers/gpu/drm/drm_gpuvm.c file.
>>
>> Sure will do that.
> 
> Great, this will help users (as well as reviewers) a lot. Please also add your
> examples from [1] to this entry, similar to the existing examples for sm_map.
> 
>>>> v2
>>>> - use drm_gpuvm_sm_map_ops_create with flags instead of defining new
>>>>     ops_create (Danilo)
>>>
>>> If this turns out not to be what I thought semantically and we still agree it's
>>> the correct approach, I think I have to take this back and it should indeed be
>>> an entirely separate code path. But let's wait for your answers above.
> 
> Having the correct understanding of how this is supposed to work (and seeing how
> the code turns out) I think it's still OK to integrate it into sm_map().
> 
> However, it probably makes sense to factor out the code into a common function
> and then build the madvise() and sm_map() functions on top of it.

__drm_gpuvm_sm_map is that common function, and does 
drm_gpuvm_madvise_ops_create sound OK? With separate functions for 
sm_map and madvise, I see there's no need to add a flag to 
drm_gpuvm_map_req at this moment. I will drop [1] in the next version.

[1] https://patchwork.freedesktop.org/patch/667561/?series=149550&rev=6

Thanks
> 
> Please also find some more comments on the patch itself.
> 
>>> Again, I really think this needs some proper documentation like in the
>>> "DOC: Split and Merge" documentation section.
>>
>> Sure
> 
> Thanks!

Re: [PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Ghimiray, Himal Prasad]

On 12-08-2025 22:28, Danilo Krummrich wrote:
> On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
>> @@ -2110,6 +2110,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   {
>>   	struct drm_gpuva *va, *next;
>>   	u64 req_end = req->op_map.va.addr + req->op_map.va.range;
>> +	bool is_madvise_ops = (req->flags & DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE);
> 
> Let's just call this 'madvise'.

Sure.

> 
>> +	bool needs_map = !is_madvise_ops;
>>   	int ret;
>>   
>>   	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req->op_map.va.addr, req->op_map.va.range)))
>> @@ -2122,26 +2124,35 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   		u64 range = va->va.range;
>>   		u64 end = addr + range;
>>   		bool merge = !!va->gem.obj;
>> +		bool skip_madvise_ops = is_madvise_ops && merge;
> 
> IIUC, you're either going for continue or break in this case. I think continue
> would always be correct and break is an optimization if end <= req_end?
> 
> If that's correct, please just do either
> 
> 	if (madvise && va->gem.obj)
> 		continue;

Will use this.>
> or
> 
> 	if (madvise && va->gem.obj) {
> 		if (end > req_end)
> 			break;
> 		else
> 			continue;
> 	}
> 
> instead of sprinkling the skip_madvise_ops checks everywhere.

True, recommended checks make it cleaner.

> 
>>   
>> +		needs_map = !is_madvise_ops;
>>   		if (addr == req->op_map.va.addr) {
>>   			merge &= obj == req->op_map.gem.obj &&
>>   				 offset == req->op_map.gem.offset;
>>   
>>   			if (end == req_end) {
>> -				ret = op_unmap_cb(ops, priv, va, merge);
>> -				if (ret)
>> -					return ret;
>> +				if (!is_madvise_ops) {
>> +					ret = op_unmap_cb(ops, priv, va, merge);
>> +					if (ret)
>> +						return ret;
>> +				}
>>   				break;
>>   			}
>>   
>>   			if (end < req_end) {
>> -				ret = op_unmap_cb(ops, priv, va, merge);
>> -				if (ret)
>> -					return ret;
>> +				if (!is_madvise_ops) {
>> +					ret = op_unmap_cb(ops, priv, va, merge);
> 
> I think we should pass madvise as argument to op_unmap_cb() and make it a noop
> internally rather than having all the conditionals.

Makes sense. Will modify in next version.

> 
>> +					if (ret)
>> +						return ret;
>> +				}
>>   				continue;
>>   			}
>>   
>>   			if (end > req_end) {
>> +				if (skip_madvise_ops)
>> +					break;
>> +
>>   				struct drm_gpuva_op_map n = {
>>   					.va.addr = req_end,
>>   					.va.range = range - req->op_map.va.range,
>> @@ -2156,6 +2167,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   				ret = op_remap_cb(ops, priv, NULL, &n, &u);
>>   				if (ret)
>>   					return ret;
>> +
>> +				if (is_madvise_ops)
>> +					needs_map = true;
> 
> I don't like this needs_map state...
> 
> Maybe we could have
> 
> 	struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;
> 
> at the beginning of the function and then change this line to
> 
> 	if (madvise)
> 		op_map = req;
> 
> and op_map_cb() can just handle a NULL pointer.
> 
> Yeah, I feel like that's better.

Agreed.

Thanks for the review.

Re: [PATCH v6 01/26] drm/gpuvm: Pass map arguments through a struct

[Ghimiray, Himal Prasad]

On 12-08-2025 22:21, Danilo Krummrich wrote:
> On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:
>> @@ -2102,17 +2106,16 @@ op_unmap_cb(const struct drm_gpuvm_ops *fn, void *priv,
>>   static int
>>   __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   		   const struct drm_gpuvm_ops *ops, void *priv,
>> -		   u64 req_addr, u64 req_range,
>> -		   struct drm_gem_object *req_obj, u64 req_offset)
>> +		   const struct drm_gpuvm_map_req *req)
>>   {
>>   	struct drm_gpuva *va, *next;
>> -	u64 req_end = req_addr + req_range;
>> +	u64 req_end = req->op_map.va.addr + req->op_map.va.range;
> 
> Forgot to add, please extract all previous values from req, such that the below
> diff is minimal and the code remiains easier to read..

Noted.