On Tue Aug 5, 2025 at 4:32 PM CEST, Rob Clark wrote:
> On Tue, Aug 5, 2025 at 2:33 AM Danilo Krummrich <d...@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;
