Re: [PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Caterina Shablia <caterina.shablia@collabora.com>]

El jueves, 24 de julio de 2025 2:43:56 (hora de verano de Europa central), 
Matthew Brost escribió:
> On Tue, Jul 22, 2025 at 03:38:14PM +0200, Danilo Krummrich wrote:
> > (Cc: Caterina)
> > 
> > On Tue Jul 22, 2025 at 3:35 PM CEST, Himal Prasad Ghimiray wrote:
> > > - DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
> > > 
> > >   range.
> > > 
> > > - DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE: This flag is used by
> > > 
> > >   drm_gpuvm_sm_map_ops_create to iterate over GPUVMA's in the
> > > 
> > > user-provided range and split the existing non-GEM object VMA if the
> > > start or end of the input range lies within it. The operations can
> > > create up to 2 REMAPS and 2 MAPs. The purpose of this operation is to be
> > > used by the Xe driver to assign attributes to GPUVMA's within the
> > > user-defined range. Unlike drm_gpuvm_sm_map_ops_flags in default mode,
> > > the operation with this flag will never have UNMAPs and
> > > merges, and can be without any final operations.
> > > 
> > > v2
> > > - use drm_gpuvm_sm_map_ops_create with flags instead of defining new
> > > 
> > >   ops_create (Danilo)
> > > 
> > > - Add doc (Danilo)
> > > 
> > > v3
> > > - Fix doc
> > > - Fix unmapping check
> > > 
> > > v4
> > > - Fix mapping for non madvise ops
> > > 
> > > Cc: Danilo Krummrich <dakr@redhat.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Boris Brezillon <bbrezillon@kernel.org>
> > > Cc: <dri-devel@lists.freedesktop.org>
> > > Signed-off-by: Himal Prasad Ghimiray<himal.prasad.ghimiray@intel.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_gpuvm.c            | 93 ++++++++++++++++++++------
> > >  drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
> > >  drivers/gpu/drm/xe/xe_vm.c             |  1 +
> > 
> > What about the other drivers using GPUVM, aren't they affected by the
> > changes?
> Yes, this seemly would break the build or other users. If the baseline
> includes the patch below that I suggest to pull in this is a moot point
> though.
> 
> > >  include/drm/drm_gpuvm.h                | 25 ++++++-
> > >  4 files changed, 98 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> > > index e89b932e987c..c7779588ea38 100644
> > > --- a/drivers/gpu/drm/drm_gpuvm.c
> > > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > > @@ -2103,10 +2103,13 @@ static int
> > > 
> > >  __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> > >  
> > >  		   const struct drm_gpuvm_ops *ops, void *priv,
> > >  		   u64 req_addr, u64 req_range,
> > > 
> > > +		   enum drm_gpuvm_sm_map_ops_flags flags,
> > 
> > Please coordinate with Boris and Caterina here. They're adding a new
> > request structure, struct drm_gpuvm_map_req.
> > 
> > I think we can define it as
> > 
> > 	struct drm_gpuvm_map_req {
> > 	
> > 		struct drm_gpuva_op_map map;
> > 		struct drm_gpuvm_sm_map_ops_flags flags;
> > 	
> > 	}
> 
> +1, I see the patch [2] and the suggested change to drm_gpuva_op_map
> [3]. Both patch and your suggestion look good to me.
> 
> Perhaps we try to accelerate [2] landing ahead of either series as
> overall just looks like a good cleanup which can be merged asap.
I'm not sure my patchset would be in a mergeable state any time soon -- I've 
discovered some issues with split/merge of repeated mappings while writing the 
doc, so it will be a while before I'll be submitting that again. [2] itself is 
in a good shape, absolutely feel free to submit that as part of your series.
> 
> Himal - I'd rebase on top [2], with Danilo suggestion in [3] if this
> hasn't landed by your next rev.
> 
> [2]
> https://lore.kernel.org/all/20250707170442.1437009-4-caterina.shablia@colla
> bora.com/ [3]
> https://lore.kernel.org/all/DB61N61AKIJ3.FG7GUJBG386P@kernel.org/
> > eventually.
> > 
> > Please also coordinate on the changes in __drm_gpuvm_sm_map() below
> > regarding Caterina's series [1], it looks like they're conflicting.
> 
> It looks pretty minor actually. I'm sure if really matter who this is
> race but yes, always good to coordinate.
> 
> > [1]
> > https://lore.kernel.org/all/20250707170442.1437009-1-caterina.shablia@col
> > labora.com/> 
> > > +/**
> > > + * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge
> > > ops
> > > + */
> > > +enum drm_gpuvm_sm_map_ops_flags {
> > > +	/**
> > > +	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
> > > +	 */
> > > +	DRM_GPUVM_SM_MAP_NOT_MADVISE = 0,
> > 
> > Why would we name this "NOT_MADVISE"? What if we add more flags for other
> > purposes?
> 
> How about...
> 
> s/DRM_GPUVM_SM_MAP_NOT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_NONE/
> 
> > > +	/**
> > > +	 * @DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE: This flag is used by
> > > +	 * drm_gpuvm_sm_map_ops_create to iterate over GPUVMA's in the
> > > +	 * user-provided range and split the existing non-GEM object VMA 
if
> > > the
> > > +	 * start or end of the input range lies within it. The operations 
can
> > > +	 * create up to 2 REMAPS and 2 MAPs. Unlike 
drm_gpuvm_sm_map_ops_flags
> > > +	 * in default mode, the operation with this flag will never have
> > > UNMAPs and +	 * merges, and can be without any final operations.
> > > +	 */
> > > +	DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),
> 
> Then normalize this one...
> 
> s/DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MA
> DVISE/
> 
> Matt
> 
> > > +};