[PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Himal Prasad Ghimiray]

- DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
  range.

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

Cc: Danilo Krummrich <dakr@redhat.com>
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            | 93 ++++++++++++++++++++------
 drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
 drivers/gpu/drm/xe/xe_vm.c             |  1 +
 include/drm/drm_gpuvm.h                | 25 ++++++-
 4 files changed, 98 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index e89b932e987c..c7779588ea38 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -2103,10 +2103,13 @@ static int
 __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 		   const struct drm_gpuvm_ops *ops, void *priv,
 		   u64 req_addr, u64 req_range,
+		   enum drm_gpuvm_sm_map_ops_flags flags,
 		   struct drm_gem_object *req_obj, u64 req_offset)
 {
 	struct drm_gpuva *va, *next;
 	u64 req_end = req_addr + req_range;
+	bool is_madvise_ops = (flags == DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE);
+	bool needs_map = !is_madvise_ops;
 	int ret;
 
 	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req_addr, req_range)))
@@ -2119,26 +2122,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_addr) {
 			merge &= obj == req_obj &&
 				 offset == req_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_range,
@@ -2153,6 +2165,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_addr) {
@@ -2170,20 +2185,42 @@ __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) {
+					ret = op_map_cb(ops, priv, req_addr,
+							min(end - req_addr, req_end - end),
+							NULL, req_offset);
+					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,
@@ -2195,6 +2232,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_addr) {
@@ -2203,20 +2243,29 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 					   (addr - req_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,
@@ -2231,14 +2280,16 @@ __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)
+					return op_map_cb(ops, priv, addr,
+							(req_end - addr), NULL, req_offset);
 				break;
 			}
 		}
 	}
-
-	return op_map_cb(ops, priv,
-			 req_addr, req_range,
-			 req_obj, req_offset);
+	return needs_map ? op_map_cb(ops, priv, req_addr,
+			   req_range, req_obj, req_offset) : 0;
 }
 
 static int
@@ -2337,15 +2388,15 @@ drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv,
 		 struct drm_gem_object *req_obj, u64 req_offset)
 {
 	const struct drm_gpuvm_ops *ops = gpuvm->ops;
+	enum drm_gpuvm_sm_map_ops_flags flags = DRM_GPUVM_SM_MAP_NOT_MADVISE;
 
 	if (unlikely(!(ops && ops->sm_step_map &&
 		       ops->sm_step_remap &&
 		       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_addr, req_range,
+				  flags, req_obj, req_offset);
 }
 EXPORT_SYMBOL_GPL(drm_gpuvm_sm_map);
 
@@ -2487,6 +2538,7 @@ static const struct drm_gpuvm_ops gpuvm_list_ops = {
  * @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
+ * @drm_gpuvm_sm_map_ops_flag: ops flag determining madvise or not
  * @req_obj: the &drm_gem_object to map
  * @req_offset: the offset within the &drm_gem_object
  *
@@ -2517,6 +2569,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,
+			    enum drm_gpuvm_sm_map_ops_flags flags,
 			    struct drm_gem_object *req_obj, u64 req_offset)
 {
 	struct drm_gpuva_ops *ops;
@@ -2536,7 +2589,7 @@ drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
 	args.ops = ops;
 
 	ret = __drm_gpuvm_sm_map(gpuvm, &gpuvm_list_ops, &args,
-				 req_addr, req_range,
+				 req_addr, req_range, flags,
 				 req_obj, req_offset);
 	if (ret)
 		goto err_free_ops;
diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
index 48f105239f42..26e13fcdbdb8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
@@ -1303,6 +1303,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,
+							      DRM_GPUVM_SM_MAP_NOT_MADVISE,
 							      op->gem.obj,
 							      op->gem.offset);
 			if (IS_ERR(op->ops)) {
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 2035604121e6..b2ed99551b6e 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -2318,6 +2318,7 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
 	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,
+						  DRM_GPUVM_SM_MAP_NOT_MADVISE,
 						  obj, bo_offset_or_userptr);
 		break;
 	case DRM_XE_VM_BIND_OP_UNMAP:
diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
index 2a9629377633..c589b886a4fd 100644
--- a/include/drm/drm_gpuvm.h
+++ b/include/drm/drm_gpuvm.h
@@ -211,6 +211,27 @@ enum drm_gpuvm_flags {
 	DRM_GPUVM_USERBITS = BIT(1),
 };
 
+/**
+ * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge ops
+ */
+enum drm_gpuvm_sm_map_ops_flags {
+	/**
+	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
+	 */
+	DRM_GPUVM_SM_MAP_NOT_MADVISE = 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_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),
+};
+
 /**
  * struct drm_gpuvm - DRM GPU VA Manager
  *
@@ -1059,8 +1080,8 @@ struct drm_gpuva_ops {
 #define drm_gpuva_next_op(op) list_next_entry(op, entry)
 
 struct drm_gpuva_ops *
-drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
-			    u64 addr, u64 range,
+drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm, u64 addr, u64 range,
+			    enum drm_gpuvm_sm_map_ops_flags flags,
 			    struct drm_gem_object *obj, u64 offset);
 struct drm_gpuva_ops *
 drm_gpuvm_sm_unmap_ops_create(struct drm_gpuvm *gpuvm,
-- 
2.34.1

Re: [PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Danilo Krummrich]

(Cc: Caterina)

On Tue Jul 22, 2025 at 3:35 PM CEST, Himal Prasad Ghimiray wrote:
> - DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
>   range.
>
> - 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. 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
>
> Cc: Danilo Krummrich <dakr@redhat.com>
> 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            | 93 ++++++++++++++++++++------
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
>  drivers/gpu/drm/xe/xe_vm.c             |  1 +

What about the other drivers using GPUVM, aren't they affected by the changes?

>  include/drm/drm_gpuvm.h                | 25 ++++++-
>  4 files changed, 98 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index e89b932e987c..c7779588ea38 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -2103,10 +2103,13 @@ static int
>  __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  		   const struct drm_gpuvm_ops *ops, void *priv,
>  		   u64 req_addr, u64 req_range,
> +		   enum drm_gpuvm_sm_map_ops_flags flags,

Please coordinate with Boris and Caterina here. They're adding a new request
structure, struct drm_gpuvm_map_req.

I think we can define it as

	struct drm_gpuvm_map_req {
		struct drm_gpuva_op_map map;
		struct drm_gpuvm_sm_map_ops_flags flags;
	}

eventually.

Please also coordinate on the changes in __drm_gpuvm_sm_map() below regarding
Caterina's series [1], it looks like they're conflicting.

[1] https://lore.kernel.org/all/20250707170442.1437009-1-caterina.shablia@collabora.com/

> +/**
> + * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge ops
> + */
> +enum drm_gpuvm_sm_map_ops_flags {
> +	/**
> +	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
> +	 */
> +	DRM_GPUVM_SM_MAP_NOT_MADVISE = 0,

Why would we name this "NOT_MADVISE"? What if we add more flags for other
purposes?

> +	/**
> +	 * @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_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),
> +};

Re: [PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Matthew Brost]

On Tue, Jul 22, 2025 at 03:38:14PM +0200, Danilo Krummrich wrote:
> (Cc: Caterina)
> 
> On Tue Jul 22, 2025 at 3:35 PM CEST, Himal Prasad Ghimiray wrote:
> > - DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
> >   range.
> >
> > - 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. 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
> >
> > Cc: Danilo Krummrich <dakr@redhat.com>
> > 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            | 93 ++++++++++++++++++++------
> >  drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
> >  drivers/gpu/drm/xe/xe_vm.c             |  1 +
> 
> What about the other drivers using GPUVM, aren't they affected by the changes?
> 

Yes, this seemly would break the build or other users. If the baseline
includes the patch below that I suggest to pull in this is a moot point
though.

> >  include/drm/drm_gpuvm.h                | 25 ++++++-
> >  4 files changed, 98 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> > index e89b932e987c..c7779588ea38 100644
> > --- a/drivers/gpu/drm/drm_gpuvm.c
> > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > @@ -2103,10 +2103,13 @@ static int
> >  __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> >  		   const struct drm_gpuvm_ops *ops, void *priv,
> >  		   u64 req_addr, u64 req_range,
> > +		   enum drm_gpuvm_sm_map_ops_flags flags,
> 
> Please coordinate with Boris and Caterina here. They're adding a new request
> structure, struct drm_gpuvm_map_req.
> 
> I think we can define it as
> 
> 	struct drm_gpuvm_map_req {
> 		struct drm_gpuva_op_map map;
> 		struct drm_gpuvm_sm_map_ops_flags flags;
> 	}

+1, I see the patch [2] and the suggested change to drm_gpuva_op_map
[3]. Both patch and your suggestion look good to me.

Perhaps we try to accelerate [2] landing ahead of either series as
overall just looks like a good cleanup which can be merged asap.

Himal - I'd rebase on top [2], with Danilo suggestion in [3] if this
hasn't landed by your next rev.

[2] https://lore.kernel.org/all/20250707170442.1437009-4-caterina.shablia@collabora.com/
[3] https://lore.kernel.org/all/DB61N61AKIJ3.FG7GUJBG386P@kernel.org/

> 
> eventually.
> 
> Please also coordinate on the changes in __drm_gpuvm_sm_map() below regarding
> Caterina's series [1], it looks like they're conflicting.
> 

It looks pretty minor actually. I'm sure if really matter who this is
race but yes, always good to coordinate.

> [1] https://lore.kernel.org/all/20250707170442.1437009-1-caterina.shablia@collabora.com/
> 
> > +/**
> > + * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge ops
> > + */
> > +enum drm_gpuvm_sm_map_ops_flags {
> > +	/**
> > +	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
> > +	 */
> > +	DRM_GPUVM_SM_MAP_NOT_MADVISE = 0,
> 
> Why would we name this "NOT_MADVISE"? What if we add more flags for other
> purposes?
> 

How about...

s/DRM_GPUVM_SM_MAP_NOT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_NONE/

> > +	/**
> > +	 * @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_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),

Then normalize this one...

s/DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE/

Matt

> > +};

Re: [PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Ghimiray, Himal Prasad]

On 22-07-2025 19:08, Danilo Krummrich wrote:
> (Cc: Caterina)
> 
> On Tue Jul 22, 2025 at 3:35 PM CEST, Himal Prasad Ghimiray wrote:
>> - DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
>>    range.
>>
>> - 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. 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
>>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> 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            | 93 ++++++++++++++++++++------
>>   drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
>>   drivers/gpu/drm/xe/xe_vm.c             |  1 +
> 
> What about the other drivers using GPUVM, aren't they affected by the changes?

Apart from xe, nouveau_uvmm.c is the only user of 
drm_gpuvm_sm_map_ops_create api and patch takes care for nouveau_uvmm.c


> 
>>   include/drm/drm_gpuvm.h                | 25 ++++++-
>>   4 files changed, 98 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
>> index e89b932e987c..c7779588ea38 100644
>> --- a/drivers/gpu/drm/drm_gpuvm.c
>> +++ b/drivers/gpu/drm/drm_gpuvm.c
>> @@ -2103,10 +2103,13 @@ static int
>>   __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   		   const struct drm_gpuvm_ops *ops, void *priv,
>>   		   u64 req_addr, u64 req_range,
>> +		   enum drm_gpuvm_sm_map_ops_flags flags,
> 
> Please coordinate with Boris and Caterina here. They're adding a new request
> structure, struct drm_gpuvm_map_req.
> 
> I think we can define it as
> 
> 	struct drm_gpuvm_map_req {
> 		struct drm_gpuva_op_map map;
> 		struct drm_gpuvm_sm_map_ops_flags flags;
> 	}
> 
> eventually.

Sure will check this.

> 
> Please also coordinate on the changes in __drm_gpuvm_sm_map() below regarding
> Caterina's series [1], it looks like they're conflicting.

Will give it a look

> 
> [1] https://lore.kernel.org/all/20250707170442.1437009-1-caterina.shablia@collabora.com/
> 
>> +/**
>> + * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge ops
>> + */
>> +enum drm_gpuvm_sm_map_ops_flags {
>> +	/**
>> +	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
>> +	 */
>> +	DRM_GPUVM_SM_MAP_NOT_MADVISE = 0,
> 
> Why would we name this "NOT_MADVISE"? What if we add more flags for other
> purposes?

How about something like DRM_GPUVM_SM_MAP_DEFAULT ?

> 
>> +	/**
>> +	 * @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_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),
>> +};

Re: [PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Ghimiray, Himal Prasad]

On 24-07-2025 06:13, Matthew Brost wrote:
> On Tue, Jul 22, 2025 at 03:38:14PM +0200, Danilo Krummrich wrote:
>> (Cc: Caterina)
>>
>> On Tue Jul 22, 2025 at 3:35 PM CEST, Himal Prasad Ghimiray wrote:
>>> - DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
>>>    range.
>>>
>>> - 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. 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
>>>
>>> Cc: Danilo Krummrich <dakr@redhat.com>
>>> 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            | 93 ++++++++++++++++++++------
>>>   drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
>>>   drivers/gpu/drm/xe/xe_vm.c             |  1 +
>>
>> What about the other drivers using GPUVM, aren't they affected by the changes?
>>
> 
> Yes, this seemly would break the build or other users. If the baseline
> includes the patch below that I suggest to pull in this is a moot point
> though.
> 
>>>   include/drm/drm_gpuvm.h                | 25 ++++++-
>>>   4 files changed, 98 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
>>> index e89b932e987c..c7779588ea38 100644
>>> --- a/drivers/gpu/drm/drm_gpuvm.c
>>> +++ b/drivers/gpu/drm/drm_gpuvm.c
>>> @@ -2103,10 +2103,13 @@ static int
>>>   __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>>   		   const struct drm_gpuvm_ops *ops, void *priv,
>>>   		   u64 req_addr, u64 req_range,
>>> +		   enum drm_gpuvm_sm_map_ops_flags flags,
>>
>> Please coordinate with Boris and Caterina here. They're adding a new request
>> structure, struct drm_gpuvm_map_req.
>>
>> I think we can define it as
>>
>> 	struct drm_gpuvm_map_req {
>> 		struct drm_gpuva_op_map map;
>> 		struct drm_gpuvm_sm_map_ops_flags flags;
>> 	}
> 
> +1, I see the patch [2] and the suggested change to drm_gpuva_op_map
> [3]. Both patch and your suggestion look good to me.
> 
> Perhaps we try to accelerate [2] landing ahead of either series as
> overall just looks like a good cleanup which can be merged asap.
> 
> Himal - I'd rebase on top [2], with Danilo suggestion in [3] if this
> hasn't landed by your next rev.
> 
> [2] https://lore.kernel.org/all/20250707170442.1437009-4-caterina.shablia@collabora.com/
> [3] https://lore.kernel.org/all/DB61N61AKIJ3.FG7GUJBG386P@kernel.org/
> 

Sure will take care of this.
  >>
>> eventually.
>>
>> Please also coordinate on the changes in __drm_gpuvm_sm_map() below regarding
>> Caterina's series [1], it looks like they're conflicting.
>>
> 
> It looks pretty minor actually. I'm sure if really matter who this is
> race but yes, always good to coordinate.
> 
>> [1] https://lore.kernel.org/all/20250707170442.1437009-1-caterina.shablia@collabora.com/
>>
>>> +/**
>>> + * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge ops
>>> + */
>>> +enum drm_gpuvm_sm_map_ops_flags {
>>> +	/**
>>> +	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
>>> +	 */
>>> +	DRM_GPUVM_SM_MAP_NOT_MADVISE = 0,
>>
>> Why would we name this "NOT_MADVISE"? What if we add more flags for other
>> purposes?
>>
> 
> How about...
> 
> s/DRM_GPUVM_SM_MAP_NOT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_NONE/

I was thinking DRM_GPUVM_SM_MAP_DEFAULT, but 
DRM_GPUVM_SM_MAP_OPS_FLAG_NONE looks better. will update it in next rev.

> 
>>> +	/**
>>> +	 * @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_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),
> 
> Then normalize this one...
> 
> s/DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE/

Sure

> 
> Matt
> 
>>> +};

Re: [PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Caterina Shablia]

El jueves, 24 de julio de 2025 2:43:56 (hora de verano de Europa central), 
Matthew Brost escribió:
> On Tue, Jul 22, 2025 at 03:38:14PM +0200, Danilo Krummrich wrote:
> > (Cc: Caterina)
> > 
> > On Tue Jul 22, 2025 at 3:35 PM CEST, Himal Prasad Ghimiray wrote:
> > > - DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
> > > 
> > >   range.
> > > 
> > > - 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. 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
> > > 
> > > Cc: Danilo Krummrich <dakr@redhat.com>
> > > 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            | 93 ++++++++++++++++++++------
> > >  drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
> > >  drivers/gpu/drm/xe/xe_vm.c             |  1 +
> > 
> > What about the other drivers using GPUVM, aren't they affected by the
> > changes?
> Yes, this seemly would break the build or other users. If the baseline
> includes the patch below that I suggest to pull in this is a moot point
> though.
> 
> > >  include/drm/drm_gpuvm.h                | 25 ++++++-
> > >  4 files changed, 98 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> > > index e89b932e987c..c7779588ea38 100644
> > > --- a/drivers/gpu/drm/drm_gpuvm.c
> > > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > > @@ -2103,10 +2103,13 @@ static int
> > > 
> > >  __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> > >  
> > >  		   const struct drm_gpuvm_ops *ops, void *priv,
> > >  		   u64 req_addr, u64 req_range,
> > > 
> > > +		   enum drm_gpuvm_sm_map_ops_flags flags,
> > 
> > Please coordinate with Boris and Caterina here. They're adding a new
> > request structure, struct drm_gpuvm_map_req.
> > 
> > I think we can define it as
> > 
> > 	struct drm_gpuvm_map_req {
> > 	
> > 		struct drm_gpuva_op_map map;
> > 		struct drm_gpuvm_sm_map_ops_flags flags;
> > 	
> > 	}
> 
> +1, I see the patch [2] and the suggested change to drm_gpuva_op_map
> [3]. Both patch and your suggestion look good to me.
> 
> Perhaps we try to accelerate [2] landing ahead of either series as
> overall just looks like a good cleanup which can be merged asap.
I'm not sure my patchset would be in a mergeable state any time soon -- I've 
discovered some issues with split/merge of repeated mappings while writing the 
doc, so it will be a while before I'll be submitting that again. [2] itself is 
in a good shape, absolutely feel free to submit that as part of your series.
> 
> Himal - I'd rebase on top [2], with Danilo suggestion in [3] if this
> hasn't landed by your next rev.
> 
> [2]
> https://lore.kernel.org/all/20250707170442.1437009-4-caterina.shablia@colla
> bora.com/ [3]
> https://lore.kernel.org/all/DB61N61AKIJ3.FG7GUJBG386P@kernel.org/
> > eventually.
> > 
> > Please also coordinate on the changes in __drm_gpuvm_sm_map() below
> > regarding Caterina's series [1], it looks like they're conflicting.
> 
> It looks pretty minor actually. I'm sure if really matter who this is
> race but yes, always good to coordinate.
> 
> > [1]
> > https://lore.kernel.org/all/20250707170442.1437009-1-caterina.shablia@col
> > labora.com/> 
> > > +/**
> > > + * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge
> > > ops
> > > + */
> > > +enum drm_gpuvm_sm_map_ops_flags {
> > > +	/**
> > > +	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
> > > +	 */
> > > +	DRM_GPUVM_SM_MAP_NOT_MADVISE = 0,
> > 
> > Why would we name this "NOT_MADVISE"? What if we add more flags for other
> > purposes?
> 
> How about...
> 
> s/DRM_GPUVM_SM_MAP_NOT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_NONE/
> 
> > > +	/**
> > > +	 * @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_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),
> 
> Then normalize this one...
> 
> s/DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MA
> DVISE/
> 
> Matt
> 
> > > +};

Re: [PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Matthew Brost]

On Tue, Jul 22, 2025 at 07:05:04PM +0530, Himal Prasad Ghimiray wrote:
> - DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
>   range.
> 
> - 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. 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
> 
> Cc: Danilo Krummrich <dakr@redhat.com>
> 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            | 93 ++++++++++++++++++++------
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
>  drivers/gpu/drm/xe/xe_vm.c             |  1 +
>  include/drm/drm_gpuvm.h                | 25 ++++++-
>  4 files changed, 98 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index e89b932e987c..c7779588ea38 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -2103,10 +2103,13 @@ static int
>  __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  		   const struct drm_gpuvm_ops *ops, void *priv,
>  		   u64 req_addr, u64 req_range,
> +		   enum drm_gpuvm_sm_map_ops_flags flags,
>  		   struct drm_gem_object *req_obj, u64 req_offset)
>  {
>  	struct drm_gpuva *va, *next;
>  	u64 req_end = req_addr + req_range;
> +	bool is_madvise_ops = (flags == DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE);
> +	bool needs_map = !is_madvise_ops;
>  	int ret;
>  
>  	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req_addr, req_range)))
> @@ -2119,26 +2122,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_addr) {
>  			merge &= obj == req_obj &&
>  				 offset == req_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_range,
> @@ -2153,6 +2165,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_addr) {
> @@ -2170,20 +2185,42 @@ __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) {
> +					ret = op_map_cb(ops, priv, req_addr,
> +							min(end - req_addr, req_end - end),

This doesn't look right.

This creating a new MAP operation to replace what the REMAP operation
unmapped but didn't remap. In Xe debug speak, this is where we are:

REMAP:UNMAP
REMAP:PREV
MAP <-- This is the calculation we are doing.

We want to 'MAP' to size here to be:

'REMAP:UNMAP.end - REMAP:PREV.end'

Which is 'end - req_addr'. So delete the min statement here and replace
with 'end - req_addr'.

Matt

> +							NULL, req_offset);
> +					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,
> @@ -2195,6 +2232,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_addr) {
> @@ -2203,20 +2243,29 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  					   (addr - req_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,
> @@ -2231,14 +2280,16 @@ __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)
> +					return op_map_cb(ops, priv, addr,
> +							(req_end - addr), NULL, req_offset);
>  				break;
>  			}
>  		}
>  	}
> -
> -	return op_map_cb(ops, priv,
> -			 req_addr, req_range,
> -			 req_obj, req_offset);
> +	return needs_map ? op_map_cb(ops, priv, req_addr,
> +			   req_range, req_obj, req_offset) : 0;
>  }
>  
>  static int
> @@ -2337,15 +2388,15 @@ drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv,
>  		 struct drm_gem_object *req_obj, u64 req_offset)
>  {
>  	const struct drm_gpuvm_ops *ops = gpuvm->ops;
> +	enum drm_gpuvm_sm_map_ops_flags flags = DRM_GPUVM_SM_MAP_NOT_MADVISE;
>  
>  	if (unlikely(!(ops && ops->sm_step_map &&
>  		       ops->sm_step_remap &&
>  		       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_addr, req_range,
> +				  flags, req_obj, req_offset);
>  }
>  EXPORT_SYMBOL_GPL(drm_gpuvm_sm_map);
>  
> @@ -2487,6 +2538,7 @@ static const struct drm_gpuvm_ops gpuvm_list_ops = {
>   * @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
> + * @drm_gpuvm_sm_map_ops_flag: ops flag determining madvise or not
>   * @req_obj: the &drm_gem_object to map
>   * @req_offset: the offset within the &drm_gem_object
>   *
> @@ -2517,6 +2569,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,
> +			    enum drm_gpuvm_sm_map_ops_flags flags,
>  			    struct drm_gem_object *req_obj, u64 req_offset)
>  {
>  	struct drm_gpuva_ops *ops;
> @@ -2536,7 +2589,7 @@ drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
>  	args.ops = ops;
>  
>  	ret = __drm_gpuvm_sm_map(gpuvm, &gpuvm_list_ops, &args,
> -				 req_addr, req_range,
> +				 req_addr, req_range, flags,
>  				 req_obj, req_offset);
>  	if (ret)
>  		goto err_free_ops;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index 48f105239f42..26e13fcdbdb8 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1303,6 +1303,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,
> +							      DRM_GPUVM_SM_MAP_NOT_MADVISE,
>  							      op->gem.obj,
>  							      op->gem.offset);
>  			if (IS_ERR(op->ops)) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 2035604121e6..b2ed99551b6e 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2318,6 +2318,7 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
>  	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,
> +						  DRM_GPUVM_SM_MAP_NOT_MADVISE,
>  						  obj, bo_offset_or_userptr);
>  		break;
>  	case DRM_XE_VM_BIND_OP_UNMAP:
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index 2a9629377633..c589b886a4fd 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -211,6 +211,27 @@ enum drm_gpuvm_flags {
>  	DRM_GPUVM_USERBITS = BIT(1),
>  };
>  
> +/**
> + * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge ops
> + */
> +enum drm_gpuvm_sm_map_ops_flags {
> +	/**
> +	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
> +	 */
> +	DRM_GPUVM_SM_MAP_NOT_MADVISE = 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_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),
> +};
> +
>  /**
>   * struct drm_gpuvm - DRM GPU VA Manager
>   *
> @@ -1059,8 +1080,8 @@ struct drm_gpuva_ops {
>  #define drm_gpuva_next_op(op) list_next_entry(op, entry)
>  
>  struct drm_gpuva_ops *
> -drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
> -			    u64 addr, u64 range,
> +drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm, u64 addr, u64 range,
> +			    enum drm_gpuvm_sm_map_ops_flags flags,
>  			    struct drm_gem_object *obj, u64 offset);
>  struct drm_gpuva_ops *
>  drm_gpuvm_sm_unmap_ops_create(struct drm_gpuvm *gpuvm,
> -- 
> 2.34.1
> 

Re: [PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Ghimiray, Himal Prasad]

On 28-07-2025 02:48, Matthew Brost wrote:
> On Tue, Jul 22, 2025 at 07:05:04PM +0530, Himal Prasad Ghimiray wrote:
>> - DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
>>    range.
>>
>> - 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. 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
>>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> 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            | 93 ++++++++++++++++++++------
>>   drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
>>   drivers/gpu/drm/xe/xe_vm.c             |  1 +
>>   include/drm/drm_gpuvm.h                | 25 ++++++-
>>   4 files changed, 98 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
>> index e89b932e987c..c7779588ea38 100644
>> --- a/drivers/gpu/drm/drm_gpuvm.c
>> +++ b/drivers/gpu/drm/drm_gpuvm.c
>> @@ -2103,10 +2103,13 @@ static int
>>   __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   		   const struct drm_gpuvm_ops *ops, void *priv,
>>   		   u64 req_addr, u64 req_range,
>> +		   enum drm_gpuvm_sm_map_ops_flags flags,
>>   		   struct drm_gem_object *req_obj, u64 req_offset)
>>   {
>>   	struct drm_gpuva *va, *next;
>>   	u64 req_end = req_addr + req_range;
>> +	bool is_madvise_ops = (flags == DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE);
>> +	bool needs_map = !is_madvise_ops;
>>   	int ret;
>>   
>>   	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req_addr, req_range)))
>> @@ -2119,26 +2122,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_addr) {
>>   			merge &= obj == req_obj &&
>>   				 offset == req_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_range,
>> @@ -2153,6 +2165,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_addr) {
>> @@ -2170,20 +2185,42 @@ __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) {
>> +					ret = op_map_cb(ops, priv, req_addr,
>> +							min(end - req_addr, req_end - end),
> 
> This doesn't look right.
> 
> This creating a new MAP operation to replace what the REMAP operation
> unmapped but didn't remap. In Xe debug speak, this is where we are:
> 
> REMAP:UNMAP
> REMAP:PREV
> MAP <-- This is the calculation we are doing.
> 
> We want to 'MAP' to size here to be:
> 
> 'REMAP:UNMAP.end - REMAP:PREV.end'
> 
> Which is 'end - req_addr'. So delete the min statement here and replace
> with 'end - req_addr'.
> 
> Matt

True, will fix this.

> 
>> +							NULL, req_offset);
>> +					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,
>> @@ -2195,6 +2232,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_addr) {
>> @@ -2203,20 +2243,29 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   					   (addr - req_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,
>> @@ -2231,14 +2280,16 @@ __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)
>> +					return op_map_cb(ops, priv, addr,
>> +							(req_end - addr), NULL, req_offset);
>>   				break;
>>   			}
>>   		}
>>   	}
>> -
>> -	return op_map_cb(ops, priv,
>> -			 req_addr, req_range,
>> -			 req_obj, req_offset);
>> +	return needs_map ? op_map_cb(ops, priv, req_addr,
>> +			   req_range, req_obj, req_offset) : 0;
>>   }
>>   
>>   static int
>> @@ -2337,15 +2388,15 @@ drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv,
>>   		 struct drm_gem_object *req_obj, u64 req_offset)
>>   {
>>   	const struct drm_gpuvm_ops *ops = gpuvm->ops;
>> +	enum drm_gpuvm_sm_map_ops_flags flags = DRM_GPUVM_SM_MAP_NOT_MADVISE;
>>   
>>   	if (unlikely(!(ops && ops->sm_step_map &&
>>   		       ops->sm_step_remap &&
>>   		       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_addr, req_range,
>> +				  flags, req_obj, req_offset);
>>   }
>>   EXPORT_SYMBOL_GPL(drm_gpuvm_sm_map);
>>   
>> @@ -2487,6 +2538,7 @@ static const struct drm_gpuvm_ops gpuvm_list_ops = {
>>    * @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
>> + * @drm_gpuvm_sm_map_ops_flag: ops flag determining madvise or not
>>    * @req_obj: the &drm_gem_object to map
>>    * @req_offset: the offset within the &drm_gem_object
>>    *
>> @@ -2517,6 +2569,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,
>> +			    enum drm_gpuvm_sm_map_ops_flags flags,
>>   			    struct drm_gem_object *req_obj, u64 req_offset)
>>   {
>>   	struct drm_gpuva_ops *ops;
>> @@ -2536,7 +2589,7 @@ drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
>>   	args.ops = ops;
>>   
>>   	ret = __drm_gpuvm_sm_map(gpuvm, &gpuvm_list_ops, &args,
>> -				 req_addr, req_range,
>> +				 req_addr, req_range, flags,
>>   				 req_obj, req_offset);
>>   	if (ret)
>>   		goto err_free_ops;
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
>> index 48f105239f42..26e13fcdbdb8 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
>> @@ -1303,6 +1303,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,
>> +							      DRM_GPUVM_SM_MAP_NOT_MADVISE,
>>   							      op->gem.obj,
>>   							      op->gem.offset);
>>   			if (IS_ERR(op->ops)) {
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index 2035604121e6..b2ed99551b6e 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -2318,6 +2318,7 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
>>   	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,
>> +						  DRM_GPUVM_SM_MAP_NOT_MADVISE,
>>   						  obj, bo_offset_or_userptr);
>>   		break;
>>   	case DRM_XE_VM_BIND_OP_UNMAP:
>> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
>> index 2a9629377633..c589b886a4fd 100644
>> --- a/include/drm/drm_gpuvm.h
>> +++ b/include/drm/drm_gpuvm.h
>> @@ -211,6 +211,27 @@ enum drm_gpuvm_flags {
>>   	DRM_GPUVM_USERBITS = BIT(1),
>>   };
>>   
>> +/**
>> + * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge ops
>> + */
>> +enum drm_gpuvm_sm_map_ops_flags {
>> +	/**
>> +	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
>> +	 */
>> +	DRM_GPUVM_SM_MAP_NOT_MADVISE = 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_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),
>> +};
>> +
>>   /**
>>    * struct drm_gpuvm - DRM GPU VA Manager
>>    *
>> @@ -1059,8 +1080,8 @@ struct drm_gpuva_ops {
>>   #define drm_gpuva_next_op(op) list_next_entry(op, entry)
>>   
>>   struct drm_gpuva_ops *
>> -drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm,
>> -			    u64 addr, u64 range,
>> +drm_gpuvm_sm_map_ops_create(struct drm_gpuvm *gpuvm, u64 addr, u64 range,
>> +			    enum drm_gpuvm_sm_map_ops_flags flags,
>>   			    struct drm_gem_object *obj, u64 offset);
>>   struct drm_gpuva_ops *
>>   drm_gpuvm_sm_unmap_ops_create(struct drm_gpuvm *gpuvm,
>> -- 
>> 2.34.1
>>

Re: [PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Ghimiray, Himal Prasad]

On 24-07-2025 16:02, Caterina Shablia wrote:
> El jueves, 24 de julio de 2025 2:43:56 (hora de verano de Europa central),
> Matthew Brost escribió:
>> On Tue, Jul 22, 2025 at 03:38:14PM +0200, Danilo Krummrich wrote:
>>> (Cc: Caterina)
>>>
>>> On Tue Jul 22, 2025 at 3:35 PM CEST, Himal Prasad Ghimiray wrote:
>>>> - DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
>>>>
>>>>    range.
>>>>
>>>> - 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. 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
>>>>
>>>> Cc: Danilo Krummrich <dakr@redhat.com>
>>>> 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            | 93 ++++++++++++++++++++------
>>>>   drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
>>>>   drivers/gpu/drm/xe/xe_vm.c             |  1 +
>>>
>>> What about the other drivers using GPUVM, aren't they affected by the
>>> changes?
>> Yes, this seemly would break the build or other users. If the baseline
>> includes the patch below that I suggest to pull in this is a moot point
>> though.
>>
>>>>   include/drm/drm_gpuvm.h                | 25 ++++++-
>>>>   4 files changed, 98 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
>>>> index e89b932e987c..c7779588ea38 100644
>>>> --- a/drivers/gpu/drm/drm_gpuvm.c
>>>> +++ b/drivers/gpu/drm/drm_gpuvm.c
>>>> @@ -2103,10 +2103,13 @@ static int
>>>>
>>>>   __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>>>   
>>>>   		   const struct drm_gpuvm_ops *ops, void *priv,
>>>>   		   u64 req_addr, u64 req_range,
>>>>
>>>> +		   enum drm_gpuvm_sm_map_ops_flags flags,
>>>
>>> Please coordinate with Boris and Caterina here. They're adding a new
>>> request structure, struct drm_gpuvm_map_req.
>>>
>>> I think we can define it as
>>>
>>> 	struct drm_gpuvm_map_req {
>>> 	
>>> 		struct drm_gpuva_op_map map;
>>> 		struct drm_gpuvm_sm_map_ops_flags flags;
>>> 	
>>> 	}
>>
>> +1, I see the patch [2] and the suggested change to drm_gpuva_op_map
>> [3]. Both patch and your suggestion look good to me.
>>
>> Perhaps we try to accelerate [2] landing ahead of either series as
>> overall just looks like a good cleanup which can be merged asap.
> I'm not sure my patchset would be in a mergeable state any time soon -- I've
> discovered some issues with split/merge of repeated mappings while writing the
> doc, so it will be a while before I'll be submitting that again. [2] itself is
> in a good shape, absolutely feel free to submit that as part of your series.

Thanks for the confirmation. Will update next rev accordingly.

>>
>> Himal - I'd rebase on top [2], with Danilo suggestion in [3] if this
>> hasn't landed by your next rev.
>>
>> [2]
>> https://lore.kernel.org/all/20250707170442.1437009-4-caterina.shablia@colla
>> bora.com/ [3]
>> https://lore.kernel.org/all/DB61N61AKIJ3.FG7GUJBG386P@kernel.org/
>>> eventually.
>>>
>>> Please also coordinate on the changes in __drm_gpuvm_sm_map() below
>>> regarding Caterina's series [1], it looks like they're conflicting.
>>
>> It looks pretty minor actually. I'm sure if really matter who this is
>> race but yes, always good to coordinate.
>>
>>> [1]
>>> https://lore.kernel.org/all/20250707170442.1437009-1-caterina.shablia@col
>>> labora.com/>
>>>> +/**
>>>> + * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge
>>>> ops
>>>> + */
>>>> +enum drm_gpuvm_sm_map_ops_flags {
>>>> +	/**
>>>> +	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
>>>> +	 */
>>>> +	DRM_GPUVM_SM_MAP_NOT_MADVISE = 0,
>>>
>>> Why would we name this "NOT_MADVISE"? What if we add more flags for other
>>> purposes?
>>
>> How about...
>>
>> s/DRM_GPUVM_SM_MAP_NOT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_NONE/
>>
>>>> +	/**
>>>> +	 * @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_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),
>>
>> Then normalize this one...
>>
>> s/DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MA
>> DVISE/
>>
>> Matt
>>
>>>> +};
> 
> 
> 
>