Re: [PATCH 5/7] drm/gpusvm: lower get/unmap pages

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

On Thu, Mar 20, 2025 at 05:30:02PM +0000, Matthew Auld wrote:

Maybe try rewording this. I find the lower in patch subject / usage
below a bit confusing.

> Lower get/unmap pages to facilitate operating on the lowest level
> pieces, without needing a full drm_gpusvm_range structure. In the next
> patch we want to extract get/unmap/free to operate on a different range
> type.
> 
> 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 | 90 ++++++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index f27731a51f34..2beca5a6dc0a 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -1323,38 +1323,28 @@ drm_gpusvm_range_pages_valid_unlocked(struct drm_gpusvm *gpusvm,
>  	return pages_valid;
>  }
>  
> -/**
> - * drm_gpusvm_range_get_pages() - Get pages for a GPU SVM range
> - * @gpusvm: Pointer to the GPU SVM structure
> - * @range: Pointer to the GPU SVM range structure
> - * @ctx: GPU SVM context
> - *
> - * This function gets pages for a GPU SVM range and ensures they are mapped for
> - * DMA access.
> - *
> - * Return: 0 on success, negative error code on failure.
> - */
> -int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
> -			       struct drm_gpusvm_range *range,
> -			       const struct drm_gpusvm_ctx *ctx)
> +static int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
> +				struct drm_gpusvm_pages *svm_pages,
> +				struct mm_struct *mm,
> +				struct mmu_interval_notifier *notifier,
> +				unsigned long *notifier_seq,
> +				unsigned long mm_start,
> +				unsigned long mm_end,

s/mm_start/pages_start ?
s/mm_end/pages_end ?

Matt

> +				const struct drm_gpusvm_ctx *ctx)
>  {
> -	struct drm_gpusvm_pages *svm_pages = &range->pages;
> -	struct mmu_interval_notifier *notifier = &range->notifier->notifier;
>  	struct hmm_range hmm_range = {
>  		.default_flags = HMM_PFN_REQ_FAULT | (ctx->read_only ? 0 :
>  			HMM_PFN_REQ_WRITE),
>  		.notifier = notifier,
> -		.start = drm_gpusvm_range_start(range),
> -		.end = drm_gpusvm_range_end(range),
> +		.start = mm_start,
> +		.end = mm_end,
>  		.dev_private_owner = gpusvm->device_private_page_owner,
>  	};
> -	struct mm_struct *mm = gpusvm->mm;
>  	struct drm_gpusvm_zdd *zdd;
>  	unsigned long timeout =
>  		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
>  	unsigned long i, j;
> -	unsigned long npages = npages_in_range(drm_gpusvm_range_start(range),
> -					       drm_gpusvm_range_end(range));
> +	unsigned long npages = npages_in_range(mm_start, mm_end);
>  	unsigned long num_dma_mapped;
>  	unsigned int order = 0;
>  	unsigned long *pfns;
> @@ -1518,7 +1508,7 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
>  	drm_gpusvm_notifier_unlock(gpusvm);
>  	kvfree(pfns);
>  set_seqno:
> -	range->notifier_seq = hmm_range.notifier_seq;
> +	*notifier_seq = hmm_range.notifier_seq;
>  
>  	return 0;
>  
> @@ -1531,8 +1521,48 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
>  		goto retry;
>  	return err;
>  }
> +
> +/**
> + * drm_gpusvm_range_get_pages() - Get pages for a GPU SVM range
> + * @gpusvm: Pointer to the GPU SVM structure
> + * @range: Pointer to the GPU SVM range structure
> + * @ctx: GPU SVM context
> + *
> + * This function gets pages for a GPU SVM range and ensures they are mapped for
> + * DMA access.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm,
> +			       struct drm_gpusvm_range *range,
> +			       const struct drm_gpusvm_ctx *ctx)
> +{
> +	return drm_gpusvm_get_pages(gpusvm, &range->pages, gpusvm->mm,
> +				    &range->notifier->notifier,
> +				    &range->notifier_seq,
> +				    drm_gpusvm_range_start(range),
> +				    drm_gpusvm_range_end(range), ctx);
> +}
>  EXPORT_SYMBOL_GPL(drm_gpusvm_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,
> +				   const struct drm_gpusvm_ctx *ctx)
> +{
> +	unsigned long npages = npages_in_range(mm_start, mm_end);
> +
> +	if (ctx->in_notifier)
> +		lockdep_assert_held_write(&gpusvm->notifier_lock);
> +	else
> +		drm_gpusvm_notifier_lock(gpusvm);
> +
> +	__drm_gpusvm_unmap_pages(gpusvm, svm_pages, npages);
> +
> +	if (!ctx->in_notifier)
> +		drm_gpusvm_notifier_unlock(gpusvm);
> +}
> +
>  /**
>   * drm_gpusvm_range_unmap_pages() - Unmap pages associated with a GPU SVM range
>   * @gpusvm: Pointer to the GPU SVM structure
> @@ -1549,19 +1579,9 @@ void drm_gpusvm_range_unmap_pages(struct drm_gpusvm *gpusvm,
>  				  struct drm_gpusvm_range *range,
>  				  const struct drm_gpusvm_ctx *ctx)
>  {
> -	struct drm_gpusvm_pages *svm_pages = &range->pages;
> -	unsigned long npages = npages_in_range(drm_gpusvm_range_start(range),
> -					       drm_gpusvm_range_end(range));
> -
> -	if (ctx->in_notifier)
> -		lockdep_assert_held_write(&gpusvm->notifier_lock);
> -	else
> -		drm_gpusvm_notifier_lock(gpusvm);
> -
> -	__drm_gpusvm_unmap_pages(gpusvm, svm_pages, npages);
> -
> -	if (!ctx->in_notifier)
> -		drm_gpusvm_notifier_unlock(gpusvm);
> +	return drm_gpusvm_unmap_pages(gpusvm, drm_gpusvm_range_start(range),
> +				      drm_gpusvm_range_end(range),
> +				      &range->pages, ctx);
>  }
>  EXPORT_SYMBOL_GPL(drm_gpusvm_range_unmap_pages);
>  
> -- 
> 2.48.1
>