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

[Ghimiray, Himal Prasad]

On 22-08-2025 13:05, Boris Brezillon wrote:
> On Thu, 21 Aug 2025 22:25:06 +0530
> "Ghimiray, Himal Prasad" <himal.prasad.ghimiray at intel.com> wrote:
> 
>> On 21-08-2025 19:05, Danilo Krummrich wrote:
>>> On Thu Aug 21, 2025 at 3:01 PM CEST, Boris Brezillon wrote:
>>>> On Thu, 21 Aug 2025 14:55:06 +0200
>>>> "Danilo Krummrich" <dakr at 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 at 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.
>>>
>>> In this case I agree, let's use struct drm_gpuva_op_map directly.
>>
>> According to the kernel documentation for the drm_gpuva_op_map
>> structure, it is intended to represent a single map operation generated
>> as the output of ops_create or the GPU VA manager. Using it as a direct
>> input to ops_create contradicts this definition.
>>
>> For drm_gpuvm_sm_map_ops_create, the values align with those in
>> drm_gpuvm_map_req. However, this is not the case for
>> drm_gpuvm_madvise_ops_create.
>>
>> If we plan to proceed with deprecating drm_gpuvm_map_req, we need to
>> clarify the fundamental definition of drm_gpuva_op_map:
>> Should it represent a user-requested map, or an operation generated by
>> the GPU VA manager?
> 
> I would say, update the doc to reflect it can be used to pass a user
> map request too, but I'll let Danilo make the final call. BTW,
> embedding an op in _map_req is equivalent to saying the _op_map object
> can describe a user map request to me :P.
True :P