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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 21-08-2025 18:31, Boris Brezillon wrote:
> On Thu, 21 Aug 2025 14:55:06 +0200
> "Danilo Krummrich" <dakr@kernel.org> wrote:
> 
>> 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?
> 
> I need the flags field in the op_map too (so I can propagate it to the
> drm_gpuva object), so I'd rather go with an op_map object directly and
> kill drm_gpuvm_map_req now.

Thanks, Boris, for your comments, and Danilo for joining the discussion.

The patch I built upon is this version, where I dropped 
drm_gpuvm_map_req and opted to use drm_gpuva_op_map directly.[1]

As I understand it, the initial recommendation was to introduce 
drm_gpuvm_map_req with a flag to control the split/merge logic in 
gpuvm_layer. However, with the introduction of madvise, we eventually 
decided to proceed with a separate API, so the flag wasn’t added.

If needed, drm_gpuva_op_map can still introduce a flag for 
driver-specific use cases.

If we’re confident that the flags in drm_gpuvm_map_req and 
drm_gpuva_op_map will always align, I’m okay with dropping map_req.

Until we reach a final decision, I’ll hold off on submitting this patch.

[1] https://patchwork.freedesktop.org/patch/666205/?series=149550&rev=5