Re: [PATCH v2 2/4] drm/gpusvm: extract drm_gpusvm_dma_map_pages() helper

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

On Tue, Sep 01, 2026 at 05:00:58PM +0800, Honglei Huang wrote:
> Move the per-device DMA mapping loop of drm_gpusvm_get_pages() into a
> helper drm_gpusvm_dma_map_pages(). The mapping logic is only moved, not
> changed, so there is no functional change.
> 
> The helper maps the already-faulted pfns into one drm_gpusvm_pages
> instance under the notifier lock and unwinds its own partial mapping on
> error. The HMM fault and the notifier retry loop stay in get_pages()
> common code rather than being pushed down to drivers, so no driver has
> to reimplement the subtle fault and retry logic.
> 
> With the mapping isolated per instance, get_pages() can later fault once
> and DMA map an array of drm_gpusvm_pages plus a count, one per owning
> drm_device.
> 
> Suggested-by: Matthew Brost <matthew.brost@intel.com>

For some reason, Sashiko didn't run on this particular patch [1]. It
would be good to have it run through the tooling in case I'm missing
something.

That said, I can't spot anything incorrect, and this looks like a solid
cleanup.

With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

[1] https://sashiko.dev/#/patchset/20260901090100.2024933-1-honghuan%40amd.com

> Signed-off-by: Honglei Huang <honghuan@amd.com>
> ---
>  drivers/gpu/drm/drm_gpusvm.c | 225 ++++++++++++++++++++---------------
>  1 file changed, 129 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index b507de539e6..89c3061d8ef 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -1442,115 +1442,41 @@ static bool drm_gpusvm_pages_valid_unlocked(struct drm_gpusvm *gpusvm,
>  }
>  
>  /**
> - * drm_gpusvm_get_pages() - Get pages and populate GPU SVM pages struct
> + * drm_gpusvm_dma_map_pages() - DMA map one drm_gpusvm_pages instance
>   * @gpusvm: Pointer to the GPU SVM structure
> - * @svm_pages: The SVM pages to populate. This will contain the dma-addresses
> - * @mm: The mm corresponding to the CPU range
> - * @notifier: The corresponding notifier for the given CPU range
> - * @pages_start: Start CPU address for the pages
> - * @pages_end: End CPU address for the pages (exclusive)
> + * @svm_pages: The SVM pages instance to populate with dma-addresses
> + * @pfns: The already-faulted pfn array (size @npages)
> + * @npages: Number of pages in the CPU range
>   * @ctx: GPU SVM context
> + * @dma_dir: DMA data direction for the mappings
>   *
> - * This function gets and maps pages for CPU range and ensures they are
> - * mapped for DMA access.
> + * Map the faulted @pfns into @svm_pages for DMA access through its owning
> + * drm_device. Must be called under the notifier lock. On failure this unwinds
> + * the partial mapping of this instance before returning.
>   *
>   * Return: 0 on success, negative error code on failure.
>   */
> -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 pages_start, unsigned long pages_end,
> -			 const struct drm_gpusvm_ctx *ctx)
> +static int drm_gpusvm_dma_map_pages(struct drm_gpusvm *gpusvm,
> +				    struct drm_gpusvm_pages *svm_pages,
> +				    unsigned long *pfns,
> +				    unsigned long npages,
> +				    const struct drm_gpusvm_ctx *ctx,
> +				    enum dma_data_direction dma_dir)
>  {
> -	struct hmm_range hmm_range = {
> -		.default_flags = HMM_PFN_REQ_FAULT | (ctx->read_only ? 0 :
> -			HMM_PFN_REQ_WRITE),
> -		.notifier = notifier,
> -		.start = pages_start,
> -		.end = pages_end,
> -		.dev_private_owner = ctx->device_private_page_owner,
> -	};
> -	void *zdd;
> -	unsigned long timeout =
> -		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> -	unsigned long remaining;
> +	void *zdd = NULL;
>  	unsigned long i, j;
> -	unsigned long npages = npages_in_range(pages_start, pages_end);
> -	unsigned long num_dma_mapped;
> +	unsigned long num_dma_mapped = 0;
>  	unsigned int order = 0;
> -	unsigned long *pfns;
>  	int err = 0;
> -	struct dev_pagemap *pagemap;
> +	struct dev_pagemap *pagemap = NULL;
>  	struct drm_pagemap *dpagemap;
>  	struct drm_gpusvm_pages_flags flags;
> -	enum dma_data_direction dma_dir = ctx->read_only ? DMA_TO_DEVICE :
> -							   DMA_BIDIRECTIONAL;
>  	struct dma_iova_state *state = &svm_pages->state;
>  
> -	if (!svm_pages->drm)
> -		return -EINVAL;
> -
> -retry:
> -	remaining = timeout - jiffies;
> -
> -	if (time_after_eq(jiffies, timeout))
> -		return -EBUSY;
> -
> -	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
> -	if (drm_gpusvm_pages_valid_unlocked(gpusvm, svm_pages))
> -		goto set_seqno;
> -
> -	pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
> -	if (!pfns)
> -		return -ENOMEM;
> -
> -	if (!mmget_not_zero(mm)) {
> -		err = -EFAULT;
> -		goto err_free;
> -	}
> -
> -	hmm_range.hmm_pfns = pfns;
> -	err = hmm_range_fault_unlocked_timeout(&hmm_range, remaining);
> -	mmput(mm);
> -	if (err)
> -		goto err_free;
> -
> -	if (!svm_pages->dma_addr) {
> -		svm_pages->dma_addr =
> -			kvzalloc_objs(*svm_pages->dma_addr, npages);
> -		if (!svm_pages->dma_addr) {
> -			err = -ENOMEM;
> -			goto err_free;
> -		}
> -	}
> -
> -	*state = (struct dma_iova_state){};
> -	svm_pages->state_offset = 0;
> -
> -	/*
> -	 * Perform all dma mappings under the notifier lock to not
> -	 * access freed pages. A notifier will either block on
> -	 * the notifier lock or unmap dma.
> -	 */
> -	drm_gpusvm_notifier_lock(gpusvm);
> +	lockdep_assert_held(&gpusvm->notifier_lock);
>  
>  	flags.__flags = svm_pages->flags.__flags;
> -	if (flags.unmapped) {
> -		drm_gpusvm_notifier_unlock(gpusvm);
> -		err = -EFAULT;
> -		goto err_free;
> -	}
> -
> -	if (mmu_interval_read_retry(notifier, hmm_range.notifier_seq)) {
> -		drm_gpusvm_notifier_unlock(gpusvm);
> -		kvfree(pfns);
> -		goto retry;
> -	}
>  
> -	zdd = NULL;
> -	pagemap = NULL;
> -	num_dma_mapped = 0;
>  	for (i = 0, j = 0; i < npages; ++j) {
>  		struct page *page = hmm_pfn_to_page(pfns[i]);
>  
> @@ -1666,17 +1592,124 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	/* WRITE_ONCE pairs with READ_ONCE for opportunistic checks */
>  	WRITE_ONCE(svm_pages->flags.__flags, flags.__flags);
>  
> +	return 0;
> +
> +err_unmap:
> +	svm_pages->flags.has_dma_mapping = true;
> +	__drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);
> +	return err;
> +}
> +
> +/**
> + * drm_gpusvm_get_pages() - Get pages and populate GPU SVM pages struct
> + * @gpusvm: Pointer to the GPU SVM structure
> + * @svm_pages: The SVM pages to populate. This will contain the dma-addresses
> + * @mm: The mm corresponding to the CPU range
> + * @notifier: The corresponding notifier for the given CPU range
> + * @pages_start: Start CPU address for the pages
> + * @pages_end: End CPU address for the pages (exclusive)
> + * @ctx: GPU SVM context
> + *
> + * This function gets and maps pages for CPU range and ensures they are
> + * mapped for DMA access.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +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 pages_start, unsigned long pages_end,
> +			 const struct drm_gpusvm_ctx *ctx)
> +{
> +	struct hmm_range hmm_range = {
> +		.default_flags = HMM_PFN_REQ_FAULT | (ctx->read_only ? 0 :
> +			HMM_PFN_REQ_WRITE),
> +		.notifier = notifier,
> +		.start = pages_start,
> +		.end = pages_end,
> +		.dev_private_owner = ctx->device_private_page_owner,
> +	};
> +	unsigned long timeout =
> +		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> +	unsigned long remaining;
> +	unsigned long npages = npages_in_range(pages_start, pages_end);
> +	unsigned long *pfns;
> +	int err = 0;
> +	enum dma_data_direction dma_dir = ctx->read_only ? DMA_TO_DEVICE :
> +							   DMA_BIDIRECTIONAL;
> +
> +	if (!svm_pages->drm)
> +		return -EINVAL;
> +
> +retry:
> +	remaining = timeout - jiffies;
> +
> +	if (time_after_eq(jiffies, timeout))
> +		return -EBUSY;
> +
> +	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
> +	if (drm_gpusvm_pages_valid_unlocked(gpusvm, svm_pages))
> +		goto set_seqno;
> +
> +	pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
> +	if (!pfns)
> +		return -ENOMEM;
> +
> +	if (!mmget_not_zero(mm)) {
> +		err = -EFAULT;
> +		goto err_free;
> +	}
> +
> +	hmm_range.hmm_pfns = pfns;
> +	err = hmm_range_fault_unlocked_timeout(&hmm_range, remaining);
> +	mmput(mm);
> +	if (err)
> +		goto err_free;
> +
> +	if (!svm_pages->dma_addr) {
> +		svm_pages->dma_addr =
> +			kvzalloc_objs(*svm_pages->dma_addr, npages);
> +		if (!svm_pages->dma_addr) {
> +			err = -ENOMEM;
> +			goto err_free;
> +		}
> +	}
> +
> +	svm_pages->state = (struct dma_iova_state){};
> +	svm_pages->state_offset = 0;
> +
> +	/*
> +	 * Perform all dma mappings under the notifier lock to not
> +	 * access freed pages. A notifier will either block on
> +	 * the notifier lock or unmap dma.
> +	 */
> +	drm_gpusvm_notifier_lock(gpusvm);
> +
> +	if (svm_pages->flags.unmapped) {
> +		drm_gpusvm_notifier_unlock(gpusvm);
> +		err = -EFAULT;
> +		goto err_free;
> +	}
> +
> +	if (mmu_interval_read_retry(notifier, hmm_range.notifier_seq)) {
> +		drm_gpusvm_notifier_unlock(gpusvm);
> +		kvfree(pfns);
> +		goto retry;
> +	}
> +
> +	err = drm_gpusvm_dma_map_pages(gpusvm, svm_pages, pfns, npages, ctx,
> +				       dma_dir);
>  	drm_gpusvm_notifier_unlock(gpusvm);
> +	if (err)
> +		goto err_free;
> +
>  	kvfree(pfns);
>  set_seqno:
>  	svm_pages->notifier_seq = hmm_range.notifier_seq;
>  
>  	return 0;
>  
> -err_unmap:
> -	svm_pages->flags.has_dma_mapping = true;
> -	__drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);
> -	drm_gpusvm_notifier_unlock(gpusvm);
>  err_free:
>  	kvfree(pfns);
>  	if (err == -EAGAIN)
> -- 
> 2.34.1
>