Re: [PATCH 6/7] drm/gpusvm: support basic_range

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

On Thu, Mar 20, 2025 at 05:30:03PM +0000, Matthew Auld wrote:
> Idea is to use this for userptr, where we mostly just need
> get/unmap/free pages, plus some kind of common notifier lock at the svm
> level (provided by gpusvm). The range itself also maps to a single
> notifier, covering the entire range (provided by the user).
> 

So, same comment as in patch #7 [1]: could we drop the idea of a basic
GPUSVM and unify SVM and userptr GPUSVM to share the locking?

Following that, rather than having wrappers in GPU SVM for
drm_gpusvm_basic_range, can we expose the lower-level GPU SVM functions
that operate on pages and have wrappers call these in the Xe layer?

Again, adding +Himal and Thomas for their opinions.

Matt

[1] https://patchwork.freedesktop.org/patch/643976/?series=146553&rev=1#comment_1177820

> TODO: needs proper kernel-doc, assuming this change makes sense.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/drm_gpusvm.c | 138 +++++++++++++++++++++++++++++------
>  include/drm/drm_gpusvm.h     |  23 ++++++
>  2 files changed, 137 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 2beca5a6dc0a..ca571610214c 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -521,6 +521,41 @@ static const struct mmu_interval_notifier_ops drm_gpusvm_notifier_ops = {
>  	.invalidate = drm_gpusvm_notifier_invalidate,
>  };
>  
> +static void __drm_gpusvm_init(struct drm_gpusvm *gpusvm, const char *name,
> +			      struct drm_device *drm, struct mm_struct *mm,
> +			      void *device_private_page_owner,
> +			      unsigned long mm_start, unsigned long mm_range,
> +			      unsigned long notifier_size,
> +			      const struct drm_gpusvm_ops *ops,
> +			      const unsigned long *chunk_sizes, int num_chunks)
> +{
> +	gpusvm->name = name;
> +	gpusvm->drm = drm;
> +	gpusvm->mm = mm;
> +	gpusvm->device_private_page_owner = device_private_page_owner;
> +	gpusvm->mm_start = mm_start;
> +	gpusvm->mm_range = mm_range;
> +	gpusvm->notifier_size = notifier_size;
> +	gpusvm->ops = ops;
> +	gpusvm->chunk_sizes = chunk_sizes;
> +	gpusvm->num_chunks = num_chunks;
> +
> +	if (mm)
> +		mmgrab(mm);
> +	gpusvm->root = RB_ROOT_CACHED;
> +	INIT_LIST_HEAD(&gpusvm->notifier_list);
> +
> +	init_rwsem(&gpusvm->notifier_lock);
> +
> +	fs_reclaim_acquire(GFP_KERNEL);
> +	might_lock(&gpusvm->notifier_lock);
> +	fs_reclaim_release(GFP_KERNEL);
> +
> +#ifdef CONFIG_LOCKDEP
> +	gpusvm->lock_dep_map = NULL;
> +#endif
> +}
> +
>  /**
>   * drm_gpusvm_init() - Initialize the GPU SVM.
>   * @gpusvm: Pointer to the GPU SVM structure.
> @@ -552,35 +587,32 @@ int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
>  	if (!ops->invalidate || !num_chunks)
>  		return -EINVAL;
>  
> -	gpusvm->name = name;
> -	gpusvm->drm = drm;
> -	gpusvm->mm = mm;
> -	gpusvm->device_private_page_owner = device_private_page_owner;
> -	gpusvm->mm_start = mm_start;
> -	gpusvm->mm_range = mm_range;
> -	gpusvm->notifier_size = notifier_size;
> -	gpusvm->ops = ops;
> -	gpusvm->chunk_sizes = chunk_sizes;
> -	gpusvm->num_chunks = num_chunks;
> -
> -	mmgrab(mm);
> -	gpusvm->root = RB_ROOT_CACHED;
> -	INIT_LIST_HEAD(&gpusvm->notifier_list);
> -
> -	init_rwsem(&gpusvm->notifier_lock);
> -
> -	fs_reclaim_acquire(GFP_KERNEL);
> -	might_lock(&gpusvm->notifier_lock);
> -	fs_reclaim_release(GFP_KERNEL);
> -
> -#ifdef CONFIG_LOCKDEP
> -	gpusvm->lock_dep_map = NULL;
> -#endif
> +	__drm_gpusvm_init(gpusvm, name, drm, mm, device_private_page_owner,
> +			  mm_start, mm_range, notifier_size, ops, chunk_sizes,
> +			  num_chunks);
>  
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(drm_gpusvm_init);
>  
> +static bool drm_gpusvm_is_basic(struct drm_gpusvm *svm)
> +{
> +	return !svm->mm;
> +}
> +
> +void drm_gpusvm_basic_init(struct drm_gpusvm *gpusvm, const char *name,
> +			   struct drm_device *drm)
> +{
> +	__drm_gpusvm_init(gpusvm, name, drm, NULL, NULL, 0, 0, 0, NULL, NULL,
> +			  0);
> +}
> +EXPORT_SYMBOL_GPL(drm_gpusvm_basic_init);
> +
> +void drm_gpusvm_basic_fini(struct drm_gpusvm *gpusvm)
> +{
> +}
> +EXPORT_SYMBOL_GPL(drm_gpusvm_basic_fini);
> +
>  /**
>   * drm_gpusvm_notifier_find() - Find GPU SVM notifier
>   * @gpusvm: Pointer to the GPU SVM structure
> @@ -1001,6 +1033,27 @@ static void drm_gpusvm_driver_lock_held(struct drm_gpusvm *gpusvm)
>  }
>  #endif
>  
> +void drm_gpusvm_basic_range_init(struct drm_gpusvm *svm,
> +				 struct drm_gpusvm_basic_range *range,
> +				 struct mmu_interval_notifier *notifier,
> +				 unsigned long *notifier_seq)
> +{
> +	WARN_ON(!drm_gpusvm_is_basic(svm));
> +
> +	range->gpusvm = svm;
> +	range->notifier = notifier;
> +	range->notifier_seq = notifier_seq;
> +	*notifier_seq = LONG_MAX;
> +	memset(&range->pages, 0, sizeof(range->pages));
> +}
> +EXPORT_SYMBOL_GPL(drm_gpusvm_basic_range_init);
> +
> +void drm_gpusvm_basic_range_fini(struct drm_gpusvm_basic_range *range)
> +{
> +	WARN_ON(range->pages.flags.has_dma_mapping);
> +}
> +EXPORT_SYMBOL_GPL(drm_gpusvm_basic_range_fini);
> +
>  /**
>   * drm_gpusvm_range_find_or_insert() - Find or insert GPU SVM range
>   * @gpusvm: Pointer to the GPU SVM structure
> @@ -1176,6 +1229,19 @@ static void drm_gpusvm_free_pages(struct drm_gpusvm *gpusvm,
>  	}
>  }
>  
> +void drm_gpusvm_basic_range_free_pages(struct drm_gpusvm_basic_range *range)
> +{
> +	unsigned long npages =
> +		npages_in_range(range->notifier->interval_tree.start,
> +				range->notifier->interval_tree.last + 1);
> +
> +	drm_gpusvm_notifier_lock(range->gpusvm);
> +	__drm_gpusvm_unmap_pages(range->gpusvm, &range->pages, npages);
> +	drm_gpusvm_free_pages(range->gpusvm, &range->pages);
> +	drm_gpusvm_notifier_unlock(range->gpusvm);
> +}
> +EXPORT_SYMBOL_GPL(drm_gpusvm_basic_range_free_pages);
> +
>  /**
>   * drm_gpusvm_range_remove() - Remove GPU SVM range
>   * @gpusvm: Pointer to the GPU SVM structure
> @@ -1545,6 +1611,20 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
>  }
>  EXPORT_SYMBOL_GPL(drm_gpusvm_range_get_pages);
>  
> +int drm_gpusvm_basic_range_get_pages(struct drm_gpusvm_basic_range *range,
> +				     const struct drm_gpusvm_ctx *ctx)
> +{
> +	drm_gpusvm_driver_lock_held(range->gpusvm);
> +
> +	return drm_gpusvm_get_pages(range->gpusvm, &range->pages,
> +				    range->notifier->mm, range->notifier,
> +				    range->notifier_seq,
> +				    range->notifier->interval_tree.start,
> +				    range->notifier->interval_tree.last + 1,
> +				    ctx);
> +}
> +EXPORT_SYMBOL_GPL(drm_gpusvm_basic_range_get_pages);
> +
>  static void drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm,
>  				   unsigned long mm_start, unsigned long mm_end,
>  				   struct drm_gpusvm_pages *svm_pages,
> @@ -1585,6 +1665,16 @@ void drm_gpusvm_range_unmap_pages(struct drm_gpusvm *gpusvm,
>  }
>  EXPORT_SYMBOL_GPL(drm_gpusvm_range_unmap_pages);
>  
> +void drm_gpusvm_basic_range_unmap_pages(struct drm_gpusvm_basic_range *range,
> +					const struct drm_gpusvm_ctx *ctx)
> +{
> +	drm_gpusvm_unmap_pages(range->gpusvm,
> +			       range->notifier->interval_tree.start,
> +			       range->notifier->interval_tree.last + 1,
> +			       &range->pages, ctx);
> +}
> +EXPORT_SYMBOL_GPL(drm_gpusvm_basic_range_unmap_pages);
> +
>  /**
>   * drm_gpusvm_migration_unlock_put_page() - Put a migration page
>   * @page: Pointer to the page to put
> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> index c15c733ef0ad..82c4e58fa84c 100644
> --- a/include/drm/drm_gpusvm.h
> +++ b/include/drm/drm_gpusvm.h
> @@ -305,6 +305,29 @@ struct drm_gpusvm_ctx {
>  	unsigned int devmem_possible :1;
>  };
>  
> +struct drm_gpusvm_basic_range {
> +	struct drm_gpusvm *gpusvm;
> +	struct drm_gpusvm_pages pages;
> +	struct mmu_interval_notifier *notifier;
> +	unsigned long *notifier_seq;
> +};
> +
> +void drm_gpusvm_basic_init(struct drm_gpusvm *gpusvm, const char *name,
> +			   struct drm_device *drm);
> +void drm_gpusvm_basic_fini(struct drm_gpusvm *gpusvm);
> +
> +void drm_gpusvm_basic_range_init(struct drm_gpusvm *svm,
> +				 struct drm_gpusvm_basic_range *range,
> +				 struct mmu_interval_notifier *notifier,
> +				 unsigned long *notifier_seq);
> +void drm_gpusvm_basic_range_fini(struct drm_gpusvm_basic_range *range);
> +
> +int drm_gpusvm_basic_range_get_pages(struct drm_gpusvm_basic_range *range,
> +				     const struct drm_gpusvm_ctx *ctx);
> +void drm_gpusvm_basic_range_unmap_pages(struct drm_gpusvm_basic_range *range,
> +					const struct drm_gpusvm_ctx *ctx);
> +void drm_gpusvm_basic_range_free_pages(struct drm_gpusvm_basic_range *range);
> +
>  int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
>  		    const char *name, struct drm_device *drm,
>  		    struct mm_struct *mm, void *device_private_page_owner,
> -- 
> 2.48.1
>