Re: [PATCH v2 21/32] drm/gpusvm: Make drm_gpusvm_for_each_* macros public

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Apr 07, 2025 at 03:47:08PM +0530, Himal Prasad Ghimiray wrote:
> The drm_gpusvm_for_each_notifier, drm_gpusvm_for_each_notifier_safe and
> drm_gpusvm_for_each_range_safe macros are useful for locating notifiers
> and ranges within a user-specified range. By making these macros public,
> we enable broader access and utility for developers who need to leverage
> them in their implementations.
> 
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

Assuming you have users of these, makes sense to make these public.

A couple of nits below.

> ---
>  drivers/gpu/drm/drm_gpusvm.c | 89 +--------------------------------
>  include/drm/drm_gpusvm.h     | 96 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 96 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 149ac56eff70..09708eef1c86 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -391,97 +391,10 @@ struct drm_gpusvm_range *
>  drm_gpusvm_range_find(struct drm_gpusvm_notifier *notifier, unsigned long start,
>  		      unsigned long end)
>  {
> -	struct interval_tree_node *itree;
> -
> -	itree = interval_tree_iter_first(&notifier->root, start, end - 1);
> -
> -	if (itree)
> -		return container_of(itree, struct drm_gpusvm_range, itree);
> -	else
> -		return NULL;
> +	return __drm_gpusvm_range_find(notifier, start, end);

My do you __drm_gpusvm_range_find as inline? Can't you just call
drm_gpusvm_range_find in public iterators?

Also I think rather than inlines for functions which touch the interval
tree, I'd leave those functions in drm_gpusvm.c - that is what
drm_gpuvm.c does.

>  }
>  EXPORT_SYMBOL_GPL(drm_gpusvm_range_find);
>  
> -/**
> - * drm_gpusvm_for_each_range_safe() - Safely iterate over GPU SVM ranges in a notifier
> - * @range__: Iterator variable for the ranges
> - * @next__: Iterator variable for the ranges temporay storage
> - * @notifier__: Pointer to the GPU SVM notifier
> - * @start__: Start address of the range
> - * @end__: End address of the range
> - *
> - * This macro is used to iterate over GPU SVM ranges in a notifier while
> - * removing ranges from it.
> - */
> -#define drm_gpusvm_for_each_range_safe(range__, next__, notifier__, start__, end__)	\
> -	for ((range__) = drm_gpusvm_range_find((notifier__), (start__), (end__)),	\
> -	     (next__) = __drm_gpusvm_range_next(range__);				\
> -	     (range__) && (drm_gpusvm_range_start(range__) < (end__));			\
> -	     (range__) = (next__), (next__) = __drm_gpusvm_range_next(range__))
> -
> -/**
> - * __drm_gpusvm_notifier_next() - get the next drm_gpusvm_notifier in the list
> - * @notifier: a pointer to the current drm_gpusvm_notifier
> - *
> - * Return: A pointer to the next drm_gpusvm_notifier if available, or NULL if
> - *         the current notifier is the last one or if the input notifier is
> - *         NULL.
> - */
> -static struct drm_gpusvm_notifier *
> -__drm_gpusvm_notifier_next(struct drm_gpusvm_notifier *notifier)
> -{
> -	if (notifier && !list_is_last(&notifier->entry,
> -				      &notifier->gpusvm->notifier_list))
> -		return list_next_entry(notifier, entry);
> -
> -	return NULL;
> -}
> -
> -static struct drm_gpusvm_notifier *
> -notifier_iter_first(struct rb_root_cached *root, unsigned long start,
> -		    unsigned long last)
> -{

So make this one an exported function I think.

s/notifier_iter_first/drm_gpusvm_notifier_find

And adjust the arguments.

> -	struct interval_tree_node *itree;
> -
> -	itree = interval_tree_iter_first(root, start, last);
> -
> -	if (itree)
> -		return container_of(itree, struct drm_gpusvm_notifier, itree);
> -	else
> -		return NULL;
> -}
> -
> -/**
> - * drm_gpusvm_for_each_notifier() - Iterate over GPU SVM notifiers in a gpusvm
> - * @notifier__: Iterator variable for the notifiers
> - * @notifier__: Pointer to the GPU SVM notifier
> - * @start__: Start address of the notifier
> - * @end__: End address of the notifier
> - *
> - * This macro is used to iterate over GPU SVM notifiers in a gpusvm.
> - */
> -#define drm_gpusvm_for_each_notifier(notifier__, gpusvm__, start__, end__)		\
> -	for ((notifier__) = notifier_iter_first(&(gpusvm__)->root, (start__), (end__) - 1);	\
> -	     (notifier__) && (drm_gpusvm_notifier_start(notifier__) < (end__));		\
> -	     (notifier__) = __drm_gpusvm_notifier_next(notifier__))
> -
> -/**
> - * drm_gpusvm_for_each_notifier_safe() - Safely iterate over GPU SVM notifiers in a gpusvm
> - * @notifier__: Iterator variable for the notifiers
> - * @next__: Iterator variable for the notifiers temporay storage
> - * @notifier__: Pointer to the GPU SVM notifier
> - * @start__: Start address of the notifier
> - * @end__: End address of the notifier
> - *
> - * This macro is used to iterate over GPU SVM notifiers in a gpusvm while
> - * removing notifiers from it.
> - */
> -#define drm_gpusvm_for_each_notifier_safe(notifier__, next__, gpusvm__, start__, end__)	\
> -	for ((notifier__) = notifier_iter_first(&(gpusvm__)->root, (start__), (end__) - 1),	\
> -	     (next__) = __drm_gpusvm_notifier_next(notifier__);				\
> -	     (notifier__) && (drm_gpusvm_notifier_start(notifier__) < (end__));		\
> -	     (notifier__) = (next__), (next__) = __drm_gpusvm_notifier_next(notifier__))
> -
>  /**
>   * drm_gpusvm_notifier_invalidate() - Invalidate a GPU SVM notifier.
>   * @mni: Pointer to the mmu_interval_notifier structure.
> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> index 8093cc6ab1f4..8b70361db351 100644
> --- a/include/drm/drm_gpusvm.h
> +++ b/include/drm/drm_gpusvm.h
> @@ -491,6 +491,20 @@ __drm_gpusvm_range_next(struct drm_gpusvm_range *range)
>  	return NULL;
>  }
>  
> +static inline struct drm_gpusvm_range *
> +__drm_gpusvm_range_find(struct drm_gpusvm_notifier *notifier,
> +			unsigned long start, unsigned long end)
> +{
> +	struct interval_tree_node *itree;
> +
> +	itree = interval_tree_iter_first(&notifier->root, start, end - 1);
> +
> +	if (itree)
> +		return container_of(itree, struct drm_gpusvm_range, itree);
> +	else
> +		return NULL;
> +}
> +
>  /**
>   * drm_gpusvm_for_each_range() - Iterate over GPU SVM ranges in a notifier
>   * @range__: Iterator variable for the ranges. If set, it indicates the start of
> @@ -504,8 +518,88 @@ __drm_gpusvm_range_next(struct drm_gpusvm_range *range)
>   */
>  #define drm_gpusvm_for_each_range(range__, notifier__, start__, end__)	\
>  	for ((range__) = (range__) ?:					\
> -	     drm_gpusvm_range_find((notifier__), (start__), (end__));	\
> +	     __drm_gpusvm_range_find((notifier__), (start__), (end__));	\
>  	     (range__) && (drm_gpusvm_range_start(range__) < (end__));	\
>  	     (range__) = __drm_gpusvm_range_next(range__))
>  
> +/**
> + * drm_gpusvm_for_each_range_safe() - Safely iterate over GPU SVM ranges in a notifier
> + * @range__: Iterator variable for the ranges
> + * @next__: Iterator variable for the ranges temporay storage
> + * @notifier__: Pointer to the GPU SVM notifier
> + * @start__: Start address of the range
> + * @end__: End address of the range
> + *
> + * This macro is used to iterate over GPU SVM ranges in a notifier while
> + * removing ranges from it.
> + */
> +#define drm_gpusvm_for_each_range_safe(range__, next__, notifier__, start__, end__)	\
> +	for ((range__) = __drm_gpusvm_range_find((notifier__), (start__), (end__)),	\
> +	     (next__) = __drm_gpusvm_range_next(range__);				\
> +	     (range__) && (drm_gpusvm_range_start(range__) < (end__));			\
> +	     (range__) = (next__), (next__) = __drm_gpusvm_range_next(range__))
> +
> +/**
> + * __drm_gpusvm_notifier_next() - get the next drm_gpusvm_notifier in the list
> + * @notifier: a pointer to the current drm_gpusvm_notifier
> + *
> + * Return: A pointer to the next drm_gpusvm_notifier if available, or NULL if
> + *         the current notifier is the last one or if the input notifier is
> + *         NULL.
> + */
> +static inline struct drm_gpusvm_notifier *
> +__drm_gpusvm_notifier_next(struct drm_gpusvm_notifier *notifier)
> +{

This one seems reasonable as an inline.

Matt

> +	if (notifier && !list_is_last(&notifier->entry,
> +				      &notifier->gpusvm->notifier_list))
> +		return list_next_entry(notifier, entry);
> +
> +	return NULL;
> +}
> +
> +static inline struct drm_gpusvm_notifier *
> +notifier_iter_first(struct rb_root_cached *root, unsigned long start,
> +		    unsigned long last)
> +{
> +	struct interval_tree_node *itree;
> +
> +	itree = interval_tree_iter_first(root, start, last);
> +
> +	if (itree)
> +		return container_of(itree, struct drm_gpusvm_notifier, itree);
> +	else
> +		return NULL;
> +}
> +
> +/**
> + * drm_gpusvm_for_each_notifier() - Iterate over GPU SVM notifiers in a gpusvm
> + * @notifier__: Iterator variable for the notifiers
> + * @notifier__: Pointer to the GPU SVM notifier
> + * @start__: Start address of the notifier
> + * @end__: End address of the notifier
> + *
> + * This macro is used to iterate over GPU SVM notifiers in a gpusvm.
> + */
> +#define drm_gpusvm_for_each_notifier(notifier__, gpusvm__, start__, end__)		\
> +	for ((notifier__) = notifier_iter_first(&(gpusvm__)->root, (start__), (end__) - 1);	\
> +	     (notifier__) && (drm_gpusvm_notifier_start(notifier__) < (end__));		\
> +	     (notifier__) = __drm_gpusvm_notifier_next(notifier__))
> +
> +/**
> + * drm_gpusvm_for_each_notifier_safe() - Safely iterate over GPU SVM notifiers in a gpusvm
> + * @notifier__: Iterator variable for the notifiers
> + * @next__: Iterator variable for the notifiers temporay storage
> + * @notifier__: Pointer to the GPU SVM notifier
> + * @start__: Start address of the notifier
> + * @end__: End address of the notifier
> + *
> + * This macro is used to iterate over GPU SVM notifiers in a gpusvm while
> + * removing notifiers from it.
> + */
> +#define drm_gpusvm_for_each_notifier_safe(notifier__, next__, gpusvm__, start__, end__)	\
> +	for ((notifier__) = notifier_iter_first(&(gpusvm__)->root, (start__), (end__) - 1),	\
> +	     (next__) = __drm_gpusvm_notifier_next(notifier__);				\
> +	     (notifier__) && (drm_gpusvm_notifier_start(notifier__) < (end__));		\
> +	     (notifier__) = (next__), (next__) = __drm_gpusvm_notifier_next(notifier__))
> +
>  #endif /* __DRM_GPUSVM_H__ */
> -- 
> 2.34.1
>