Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req

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

On Thu Aug 21, 2025 at 1:25 PM CEST, Boris Brezillon wrote:
> On Thu, 21 Aug 2025 13:01:46 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>> On a second thought, I'm now wondering why we need drm_gpuvm_map_req in
>> the first place. It would kinda make sense if it was containing an
>> 
>> 	bool madvise;
>> 
>> field, so you don't have to pass it around, but even then, I'm
>> wondering if we wouldn't be better off adding this field to
>> drm_gpuva_op_map instead and passing an drm_gpuva_op_map object to
>> the various map helpers (like Danilo suggested in his review of the
>> REPEATED mode series Caterina sent).
>
> More on that: the very reason I introduced drm_gpuvm_map_req in the
> first place is so we have a clear differentiation between an overall
> map request and the sub-operations that are created to fulfill it.
> Looks like this was not a concern for Danilo and he was happy with us
> using _op_map for this.
>
> The other reason we might want to add drm_gpuvm_map_req is so that
> information we only need while splitting a req don't pollute
> drm_gpuva_op_map. Given I was going to pass the flags to the driver's
> callback anyway (meaning it's needed at the op_map level), and given
> you're passing madvise as a separate bool argument to various helpers
> (_map_req just contains the op, not the madvise bool), I don't think
> this aspect matters.

Good catch! Indeed, when Himal picked up your struct drm_gpuvm_map_req patch,
there were additional flags included in the structure. Now that it is
essentially a transparent wrapper, I prefer to use struct drm_gpuva_op_map
directly.

However, given that you still have patches in flight that will add a flags field
to struct drm_gpuvm_map_req I think it's probably fine to introduce it right
away. Or did you drop this plan of adding those flags?