Message-ID: <CACSVV01zZ=o55YY=GHTKaJY1HorRUu73KA+5Puz2MonqH7JwuQ@mail.gmail.com>
Date: Tue, 5 Aug 2025 07:59:27 -0700
From: Rob Clark <rob.clark@....qualcomm.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
        linux-arm-msm@...r.kernel.org, Danilo Krummrich <dakr@...hat.com>,
        Connor Abbott <cwabbott0@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        Lyude Paul <lyude@...hat.com>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH RESEND 1/2] drm/gpuvm: Send in-place re-maps to the driver
 as remap

On Tue, Aug 5, 2025 at 7:48 AM Danilo Krummrich <dakr@...nel.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@...nel.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;
>

Powered by blists - more mailing lists