On Thu, 21 Aug 2025 22:25:06 +0530 "Ghimiray, Himal Prasad" <[email protected]> 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" <[email protected]> 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 <[email protected]> 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.
