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 <d...@redhat.com>
> > Cc: Matthew Brost <matthew.br...@intel.com>
> > Cc: Boris Brezillon <bbrezil...@kernel.org>
> > Cc: <dri-devel@lists.freedesktop.org>
> > Signed-off-by: Himal Prasad Ghimiray<himal.prasad.ghimi...@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.
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.shab...@collabora.com/
[3] https://lore.kernel.org/all/db61n61akij3.fg7gujbg3...@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.shab...@collabora.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_MADVISE/
Matt
> > +};
