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

["Danilo Krummrich" <dakr@kernel.org>]

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;