[PATCH RESEND 0/2] drm/gpuvm+msm: Handle in-place remaps

[PATCH RESEND 0/2] drm/gpuvm+msm: Handle in-place remaps

[Rob Clark]

turnip+msm uses a DUMP flag on the gpuva to indicate VA ranges to dump
(ie. for devcoredump).  In most cases (internal BOs like shader
instructions) this is known at the time the BO is MAPd, and the DUMP
flag can be set at the same time as the BO is initially bound into the
VM.  But for descriptor buffers, this isn't known until VkBuffer is
bound to the already mapped VkDeviceMemory, requiring an atomic remap
to set the flag.

The problem is that drmvm turns this into discreet unmap and remap
steps.  So there is a window where the VA is not mapped, which can
race with cmdstream exec (SUBMIT).

This series attempts to avoid that by turning an exact-remap into a
remap op instead, where the driver can handle the special case since
it can see both the unmap and map steps at the same time.

Rob Clark (2):
  drm/gpuvm: Send in-place re-maps to the driver as remap
  drm/msm: Handle in-place remaps

 drivers/gpu/drm/drm_gpuvm.c            | 21 +++++++++++++++++++++
 drivers/gpu/drm/msm/msm_gem_vma.c      | 17 +++++++++++++++--
 drivers/gpu/drm/nouveau/nouveau_uvmm.c |  3 ++-
 3 files changed, 38 insertions(+), 3 deletions(-)

-- 
2.50.1

[PATCH RESEND 1/2] drm/gpuvm: Send in-place re-maps to the driver as remap

[Rob Clark]

The 'keep' hint on the unmap is only half useful, without being able to
link it to a map cb.  Instead combine the two ops into a remap op to
give the driver a chance to figure things out.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/drm_gpuvm.c            | 21 +++++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_uvmm.c |  3 ++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index bbc7fecb6f4a..e21782a97fbe 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -2125,6 +2125,27 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
 				 offset == req_offset;
 
 			if (end == req_end) {
+				if (merge) {
+					/*
+					 * This is an exact remap of the existing
+					 * VA (potentially flags change)?  Pass
+					 * this to the driver as a remap so it can
+					 * do an in-place update:
+					 */
+					struct drm_gpuva_op_map n = {
+						.va.addr = va->va.addr,
+						.va.range = va->va.range,
+						.gem.obj = va->gem.obj,
+						.gem.offset = va->gem.offset,
+					};
+					struct drm_gpuva_op_unmap u = {
+						.va = va,
+						.keep = true,
+					};
+
+					return op_remap_cb(ops, priv, NULL, &n, &u);
+				}
+
 				ret = op_unmap_cb(ops, priv, va, merge);
 				if (ret)
 					return ret;
diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
index 48f105239f42..c3e3a15eb3c8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
@@ -820,7 +820,8 @@ op_remap(struct drm_gpuva_op_remap *r,
 	if (r->next)
 		end = r->next->va.addr;
 
-	op_unmap_range(u, addr, end - addr);
+	if (!u->keep)
+		op_unmap_range(u, addr, end - addr);
 }
 
 static int
-- 
2.50.1

[PATCH RESEND 2/2] drm/msm: Handle in-place remaps

[Rob Clark]

Handle the special case of a MAP op simply updating the va flags by
detecting the special case, and skip pgtable updates.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_gem_vma.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index dc54c693b28d..d4b1cfb3aa03 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -519,9 +519,10 @@ msm_gem_vm_sm_step_map(struct drm_gpuva_op *op, void *arg)
 }
 
 static int
-msm_gem_vm_sm_step_remap(struct drm_gpuva_op *op, void *arg)
+msm_gem_vm_sm_step_remap(struct drm_gpuva_op *op, void *_arg)
 {
-	struct msm_vm_bind_job *job = ((struct op_arg *)arg)->job;
+	struct op_arg *arg = _arg;
+	struct msm_vm_bind_job *job = arg->job;
 	struct drm_gpuvm *vm = job->vm;
 	struct drm_gpuva *orig_vma = op->remap.unmap->va;
 	struct drm_gpuva *prev_vma = NULL, *next_vma = NULL;
@@ -529,6 +530,18 @@ msm_gem_vm_sm_step_remap(struct drm_gpuva_op *op, void *arg)
 	bool mapped = to_msm_vma(orig_vma)->mapped;
 	unsigned flags;
 
+	/* Special case for in-place updates: */
+	if (op->remap.unmap->keep && arg->flags &&
+	    op->remap.next && !op->remap.prev &&
+	    (orig_vma->gem.obj == op->remap.next->gem.obj) &&
+	    (orig_vma->gem.offset == op->remap.next->gem.offset) &&
+	    (orig_vma->va.addr == op->remap.next->va.addr) &&
+	    (orig_vma->va.range == op->remap.next->va.range)) {
+		/* Only flags are changing, so update that in-place: */
+		unsigned orig_flags = orig_vma->flags & (DRM_GPUVA_USERBITS - 1);
+		orig_vma->flags |= orig_flags | arg->flags;
+	}
+
 	vm_dbg("orig_vma: %p:%p:%p: %016llx %016llx", vm, orig_vma,
 	       orig_vma->gem.obj, orig_vma->va.addr, orig_vma->va.range);
 
-- 
2.50.1

Re: [PATCH RESEND 1/2] drm/gpuvm: Send in-place re-maps to the driver as remap

[Danilo Krummrich]

(Cc: Thomas, Boris, Matt, Alice)

On Mon Aug 4, 2025 at 11:43 PM CEST, Rob Clark wrote:
> The 'keep' hint on the unmap is only half useful, without being able to
> link it to a map cb.  Instead combine the two ops into a remap op to
> give the driver a chance to figure things out.
>
> Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
> ---
>  drivers/gpu/drm/drm_gpuvm.c            | 21 +++++++++++++++++++++
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c |  3 ++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index bbc7fecb6f4a..e21782a97fbe 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -2125,6 +2125,27 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  				 offset == req_offset;
>  
>  			if (end == req_end) {
> +				if (merge) {
> +					/*
> +					 * This is an exact remap of the existing
> +					 * VA (potentially flags change)?  Pass
> +					 * this to the driver as a remap so it can
> +					 * do an in-place update:
> +					 */
> +					struct drm_gpuva_op_map n = {
> +						.va.addr = va->va.addr,
> +						.va.range = va->va.range,
> +						.gem.obj = va->gem.obj,
> +						.gem.offset = va->gem.offset,
> +					};
> +					struct drm_gpuva_op_unmap u = {
> +						.va = va,
> +						.keep = true,
> +					};
> +
> +					return op_remap_cb(ops, priv, NULL, &n, &u);
> +				}

I don't see why this is necessary, a struct drm_gpuva_op_unmap carries the
struct drm_gpuva to unmap. You can easily compare this to the original request
you gave to GPUVM, i.e. req_addr, req_range, req_obj, req_offset, etc.

Which is what you have to do for any other unmap operation that has keep == true
anyways, e.g. if D is the exact same as A, B and C.

	Cur
	---
	1                       N
	|---A---|---B---|---C---|
	
	Req
	---
	1                       N
	|-----------D-----------|

In this case you get three unmap ops with keep == true, which you can compare to
your request to figure out that you can keep the corresponding PTEs.

Besides that it changes the semantics that the documentation mentions and that
drivers are allowed to rely on, i.e. a struct drm_gpuva_op_remap represents
an actual change and any call to __drm_gpuvm_sm_map() results in an arbitrary
number of unmap ops, a maximum of two remap ops and exactly one map operation.

>  				ret = op_unmap_cb(ops, priv, va, merge);
>  				if (ret)
>  					return ret;

Re: [PATCH RESEND 1/2] drm/gpuvm: Send in-place re-maps to the driver as remap

[Rob Clark]

On Tue, Aug 5, 2025 at 2:33 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> (Cc: Thomas, Boris, Matt, Alice)
>
> On Mon Aug 4, 2025 at 11:43 PM CEST, Rob Clark wrote:
> > The 'keep' hint on the unmap is only half useful, without being able to
> > link it to a map cb.  Instead combine the two ops into a remap op to
> > give the driver a chance to figure things out.
> >
> > Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
> > ---
> >  drivers/gpu/drm/drm_gpuvm.c            | 21 +++++++++++++++++++++
> >  drivers/gpu/drm/nouveau/nouveau_uvmm.c |  3 ++-
> >  2 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> > index bbc7fecb6f4a..e21782a97fbe 100644
> > --- a/drivers/gpu/drm/drm_gpuvm.c
> > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > @@ -2125,6 +2125,27 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> >                                offset == req_offset;
> >
> >                       if (end == req_end) {
> > +                             if (merge) {
> > +                                     /*
> > +                                      * This is an exact remap of the existing
> > +                                      * VA (potentially flags change)?  Pass
> > +                                      * this to the driver as a remap so it can
> > +                                      * do an in-place update:
> > +                                      */
> > +                                     struct drm_gpuva_op_map n = {
> > +                                             .va.addr = va->va.addr,
> > +                                             .va.range = va->va.range,
> > +                                             .gem.obj = va->gem.obj,
> > +                                             .gem.offset = va->gem.offset,
> > +                                     };
> > +                                     struct drm_gpuva_op_unmap u = {
> > +                                             .va = va,
> > +                                             .keep = true,
> > +                                     };
> > +
> > +                                     return op_remap_cb(ops, priv, NULL, &n, &u);
> > +                             }
>
> I don't see why this is necessary, a struct drm_gpuva_op_unmap carries the
> struct drm_gpuva to unmap. You can easily compare this to the original request
> you gave to GPUVM, i.e. req_addr, req_range, req_obj, req_offset, etc.
>
> Which is what you have to do for any other unmap operation that has keep == true
> anyways, e.g. if D is the exact same as A, B and C.
>
>         Cur
>         ---
>         1                       N
>         |---A---|---B---|---C---|
>
>         Req
>         ---
>         1                       N
>         |-----------D-----------|

Ugg, this means carrying around more state between the unmap and map
callbacks, vs. just handing all the data to the driver in a single
callback.  For the keep==true case, nouveau just seems to skip the
unmap.. I guess in your case the map operation is tolerant of
overwriting existing mappings so this works out, which isn't the case
with io_pgtable.

I guess I could handle the specific case of an exact in-place remap in
the driver to handle this specific case.  But the example you give
with multiple mappings would be harder to cope with.

I still feel there is some room for improvement in gpuvm to make this
easier for drivers.  Maybe what I proposed isn't the best general
solution, but somehow giving the drivers info about both the unmaps
and maps in the same callback would make things easier (and the remap
callback is _almost_ that).

BR,
-R

>
> In this case you get three unmap ops with keep == true, which you can compare to
> your request to figure out that you can keep the corresponding PTEs.
>
> Besides that it changes the semantics that the documentation mentions and that
> drivers are allowed to rely on, i.e. a struct drm_gpuva_op_remap represents
> an actual change and any call to __drm_gpuvm_sm_map() results in an arbitrary
> number of unmap ops, a maximum of two remap ops and exactly one map operation.
>
> >                               ret = op_unmap_cb(ops, priv, va, merge);
> >                               if (ret)
> >                                       return ret;

Re: [PATCH RESEND 1/2] drm/gpuvm: Send in-place re-maps to the driver as remap

[Danilo Krummrich]

On Tue Aug 5, 2025 at 4:32 PM CEST, Rob Clark wrote:
> On Tue, Aug 5, 2025 at 2:33 AM Danilo Krummrich <dakr@kernel.org> wrote:
>> On Mon Aug 4, 2025 at 11:43 PM CEST, Rob Clark wrote:
>> > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
>> > index bbc7fecb6f4a..e21782a97fbe 100644
>> > --- a/drivers/gpu/drm/drm_gpuvm.c
>> > +++ b/drivers/gpu/drm/drm_gpuvm.c
>> > @@ -2125,6 +2125,27 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>> >                                offset == req_offset;
>> >
>> >                       if (end == req_end) {
>> > +                             if (merge) {
>> > +                                     /*
>> > +                                      * This is an exact remap of the existing
>> > +                                      * VA (potentially flags change)?  Pass
>> > +                                      * this to the driver as a remap so it can
>> > +                                      * do an in-place update:
>> > +                                      */
>> > +                                     struct drm_gpuva_op_map n = {
>> > +                                             .va.addr = va->va.addr,
>> > +                                             .va.range = va->va.range,
>> > +                                             .gem.obj = va->gem.obj,
>> > +                                             .gem.offset = va->gem.offset,
>> > +                                     };
>> > +                                     struct drm_gpuva_op_unmap u = {
>> > +                                             .va = va,
>> > +                                             .keep = true,
>> > +                                     };
>> > +
>> > +                                     return op_remap_cb(ops, priv, NULL, &n, &u);
>> > +                             }
>>
>> I don't see why this is necessary, a struct drm_gpuva_op_unmap carries the
>> struct drm_gpuva to unmap. You can easily compare this to the original request
>> you gave to GPUVM, i.e. req_addr, req_range, req_obj, req_offset, etc.
>>
>> Which is what you have to do for any other unmap operation that has keep == true
>> anyways, e.g. if D is the exact same as A, B and C.
>>
>>         Cur
>>         ---
>>         1                       N
>>         |---A---|---B---|---C---|
>>
>>         Req
>>         ---
>>         1                       N
>>         |-----------D-----------|
>
> Ugg, this means carrying around more state between the unmap and map
> callbacks, vs. just handing all the data to the driver in a single
> callback.  For the keep==true case, nouveau just seems to skip the
> unmap.. I guess in your case the map operation is tolerant of
> overwriting existing mappings so this works out, which isn't the case
> with io_pgtable.

There is no "your case" as far as I'm concerned. Please don't think that I don't
care about solving a problem, just because it's not relevant for any of the
drivers or subsystems I maintain. :)

> I guess I could handle the specific case of an exact in-place remap in
> the driver to handle this specific case.  But the example you give
> with multiple mappings would be harder to cope with.
>
> I still feel there is some room for improvement in gpuvm to make this
> easier for drivers.  Maybe what I proposed isn't the best general
> solution, but somehow giving the drivers info about both the unmaps
> and maps in the same callback would make things easier (and the remap
> callback is _almost_ that).

I generally agree with that, my concern is more about this specific patch.

There are patches on the list that replace all the req_* arguments of
__drm_gpuvm_sm_map() with a new struct drm_gpuvm_map_req.

Maybe the unmap callbacks could simply provide a pointer to this object?

> BR,
> -R
>
>>
>> In this case you get three unmap ops with keep == true, which you can compare to
>> your request to figure out that you can keep the corresponding PTEs.
>>
>> Besides that it changes the semantics that the documentation mentions and that
>> drivers are allowed to rely on, i.e. a struct drm_gpuva_op_remap represents
>> an actual change and any call to __drm_gpuvm_sm_map() results in an arbitrary
>> number of unmap ops, a maximum of two remap ops and exactly one map operation.
>>
>> >                               ret = op_unmap_cb(ops, priv, va, merge);
>> >                               if (ret)
>> >                                       return ret;

Re: [PATCH RESEND 1/2] drm/gpuvm: Send in-place re-maps to the driver as remap

[Rob Clark]

On Tue, Aug 5, 2025 at 7:48 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue Aug 5, 2025 at 4:32 PM CEST, Rob Clark wrote:
> > On Tue, Aug 5, 2025 at 2:33 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >> On Mon Aug 4, 2025 at 11:43 PM CEST, Rob Clark wrote:
> >> > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> >> > index bbc7fecb6f4a..e21782a97fbe 100644
> >> > --- a/drivers/gpu/drm/drm_gpuvm.c
> >> > +++ b/drivers/gpu/drm/drm_gpuvm.c
> >> > @@ -2125,6 +2125,27 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> >> >                                offset == req_offset;
> >> >
> >> >                       if (end == req_end) {
> >> > +                             if (merge) {
> >> > +                                     /*
> >> > +                                      * This is an exact remap of the existing
> >> > +                                      * VA (potentially flags change)?  Pass
> >> > +                                      * this to the driver as a remap so it can
> >> > +                                      * do an in-place update:
> >> > +                                      */
> >> > +                                     struct drm_gpuva_op_map n = {
> >> > +                                             .va.addr = va->va.addr,
> >> > +                                             .va.range = va->va.range,
> >> > +                                             .gem.obj = va->gem.obj,
> >> > +                                             .gem.offset = va->gem.offset,
> >> > +                                     };
> >> > +                                     struct drm_gpuva_op_unmap u = {
> >> > +                                             .va = va,
> >> > +                                             .keep = true,
> >> > +                                     };
> >> > +
> >> > +                                     return op_remap_cb(ops, priv, NULL, &n, &u);
> >> > +                             }
> >>
> >> I don't see why this is necessary, a struct drm_gpuva_op_unmap carries the
> >> struct drm_gpuva to unmap. You can easily compare this to the original request
> >> you gave to GPUVM, i.e. req_addr, req_range, req_obj, req_offset, etc.
> >>
> >> Which is what you have to do for any other unmap operation that has keep == true
> >> anyways, e.g. if D is the exact same as A, B and C.
> >>
> >>         Cur
> >>         ---
> >>         1                       N
> >>         |---A---|---B---|---C---|
> >>
> >>         Req
> >>         ---
> >>         1                       N
> >>         |-----------D-----------|
> >
> > Ugg, this means carrying around more state between the unmap and map
> > callbacks, vs. just handing all the data to the driver in a single
> > callback.  For the keep==true case, nouveau just seems to skip the
> > unmap.. I guess in your case the map operation is tolerant of
> > overwriting existing mappings so this works out, which isn't the case
> > with io_pgtable.
>
> There is no "your case" as far as I'm concerned. Please don't think that I don't
> care about solving a problem, just because it's not relevant for any of the
> drivers or subsystems I maintain. :)

Sorry, I didn't mean to imply that.. I was just trying to point out a
constraint that you might not be aware of :-)

> > I guess I could handle the specific case of an exact in-place remap in
> > the driver to handle this specific case.  But the example you give
> > with multiple mappings would be harder to cope with.
> >
> > I still feel there is some room for improvement in gpuvm to make this
> > easier for drivers.  Maybe what I proposed isn't the best general
> > solution, but somehow giving the drivers info about both the unmaps
> > and maps in the same callback would make things easier (and the remap
> > callback is _almost_ that).
>
> I generally agree with that, my concern is more about this specific patch.
>
> There are patches on the list that replace all the req_* arguments of
> __drm_gpuvm_sm_map() with a new struct drm_gpuvm_map_req.
>
> Maybe the unmap callbacks could simply provide a pointer to this object?

I think that would help.. I'd probably want some additional
information about overlapping 'keep' unmaps in the map callback as
well, or at least the range of the 'keep' unmaps so the map callback
knows that part of the new va is already mapped. But this seems
doable.

BR,
-R

> > BR,
> > -R
> >
> >>
> >> In this case you get three unmap ops with keep == true, which you can compare to
> >> your request to figure out that you can keep the corresponding PTEs.
> >>
> >> Besides that it changes the semantics that the documentation mentions and that
> >> drivers are allowed to rely on, i.e. a struct drm_gpuva_op_remap represents
> >> an actual change and any call to __drm_gpuvm_sm_map() results in an arbitrary
> >> number of unmap ops, a maximum of two remap ops and exactly one map operation.
> >>
> >> >                               ret = op_unmap_cb(ops, priv, va, merge);
> >> >                               if (ret)
> >> >                                       return ret;
>