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.
