Re: [PATCH v2 3/4] drm/gpusvm: let drm_gpusvm_get_pages() map an array of pages

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

On Tue, Sep 01, 2026 at 05:00:59PM +0800, Honglei Huang wrote:
> With the N:1 drm_gpusvm_pages layout, one CPU range mirrored on several
> drm_devices, the caller had to invoke get_pages() once per device and
> repeat the HMM fault every time.
> 
> Make get_pages() take a contiguous array of drm_gpusvm_pages plus a
> count: fault once, then DMA map each instance by
> drm_gpusvm_dma_map_pages() under a single read_retry gate. xe range and
> userptr callers are updated.
> 
> Document the N:1 array usage in the Overview, showing how get_pages()
> and drm_gpusvm_range_set_unmapped() take the whole array and its count
> while the unmap and free paths stay per-instance.
> 
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Honglei Huang <honghuan@amd.com>
> ---
>  drivers/gpu/drm/drm_gpusvm.c    | 141 ++++++++++++++++++++++++--------
>  drivers/gpu/drm/xe/xe_svm.c     |   2 +-
>  drivers/gpu/drm/xe/xe_userptr.c |   2 +-
>  include/drm/drm_gpusvm.h        |   1 +
>  4 files changed, 108 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 89c3061d8ef..810f801a9f7 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -80,6 +80,13 @@
>   *			};
>   *		};
>   *
> + *		static struct drm_gpusvm_pages *
> + *		driver_pages(struct driver_range *drange)
> + *		{
> + *			return drange->num_pages == 1 ? &drange->inline_pages :
> + *							drange->pages;
> + *		}
> + *
>   *	In the N:1 case the driver allocates the pages array with a zeroing
>   *	allocator (e.g. kcalloc(num_pages, ...)), initialises each entry with
>   *	drm_gpusvm_init_pages(), and frees each entry with
> @@ -89,6 +96,28 @@
>   *	Each drm_gpusvm_pages must be zero-initialised and initialised with
>   *	drm_gpusvm_init_pages(), called once per entry.
>   *
> + *	The 1:1 examples below pass @num_pages == 1 and &drange->pages. In the
> + *	N:1 case the driver instead passes the whole array and its count, so a
> + *	single call faults the CPU range once and DMA maps it for every owning
> + *	drm_device, e.g.:
> + *
> + *	.. code-block:: c
> + *
> + *		// GPU fault handler: one fault, one DMA mapping per device
> + *		err = drm_gpusvm_get_pages(gpusvm, driver_pages(drange),
> + *					   drange->num_pages, gpusvm->mm,
> + *					   &range->notifier->notifier,
> + *					   drm_gpusvm_range_start(range),
> + *					   drm_gpusvm_range_end(range), &ctx);
> + *
> + *		// Notifier callback: mark every instance unmapped in one call
> + *		drm_gpusvm_range_set_unmapped(range, driver_pages(drange),
> + *					      drange->num_pages, mmu_range);
> + *
> + *	The unmap and free paths stay per-instance: iterate @num_pages over
> + *	driver_pages(drange) and call drm_gpusvm_unmap_pages() /
> + *	drm_gpusvm_free_pages() for each entry.
> + *
>   * - Operations:
>   *	Define the interface for driver-specific GPU SVM operations such as
>   *	range allocation, notifier allocation, and invalidations.
> @@ -232,7 +261,7 @@
>   *				goto retry;
>   *		}
>   *
> - *		err = drm_gpusvm_get_pages(gpusvm, &drange->pages,
> + *		err = drm_gpusvm_get_pages(gpusvm, &drange->pages, 1,
>   *					   gpusvm->mm, &range->notifier->notifier,
>   *					   drm_gpusvm_range_start(range),
>   *					   drm_gpusvm_range_end(range), &ctx);
> @@ -1417,25 +1446,34 @@ EXPORT_SYMBOL_GPL(drm_gpusvm_pages_valid);
>  /**
>   * drm_gpusvm_pages_valid_unlocked() - GPU SVM pages valid unlocked
>   * @gpusvm: Pointer to the GPU SVM structure
> - * @svm_pages: Pointer to the GPU SVM pages structure
> + * @svm_pages: Array of GPU SVM pages structures
> + * @num_pages: Number of drm_gpusvm_pages instances in @svm_pages
>   *
> - * This function determines if a GPU SVM pages are valid. Expected be called
> + * This function determines if every GPU SVM pages instance is valid, dropping
> + * the stale dma_addr array of any instance which is not. Expected be called
>   * without holding gpusvm->notifier_lock.
>   *
> - * Return: True if GPU SVM pages are valid, False otherwise
> + * Return: True if all GPU SVM pages are valid, False otherwise
>   */
>  static bool drm_gpusvm_pages_valid_unlocked(struct drm_gpusvm *gpusvm,
> -					    struct drm_gpusvm_pages *svm_pages)
> +					    struct drm_gpusvm_pages *svm_pages,
> +					    unsigned int num_pages)
>  {
> -	bool pages_valid;
> +	bool pages_valid = true;
> +	unsigned int p;
>  
> -	if (!svm_pages->dma_addr)
> -		return false;
> +	for (p = 0; p < num_pages; ++p) {
> +		if (!svm_pages[p].dma_addr)
> +			return false;
> +	}
>  
>  	drm_gpusvm_notifier_lock(gpusvm);
> -	pages_valid = drm_gpusvm_pages_valid(gpusvm, svm_pages);
> -	if (!pages_valid)
> -		__drm_gpusvm_free_pages(gpusvm, svm_pages);
> +	for (p = 0; p < num_pages; ++p) {
> +		if (drm_gpusvm_pages_valid(gpusvm, &svm_pages[p]))
> +			continue;
> +		__drm_gpusvm_free_pages(gpusvm, &svm_pages[p]);
> +		pages_valid = false;
> +	}
>  	drm_gpusvm_notifier_unlock(gpusvm);
>  
>  	return pages_valid;
> @@ -1451,8 +1489,9 @@ static bool drm_gpusvm_pages_valid_unlocked(struct drm_gpusvm *gpusvm,
>   * @dma_dir: DMA data direction for the mappings
>   *
>   * 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.
> + * drm_device. Must be called under the notifier lock and only for an instance
> + * without a live mapping. On failure this unwinds the partial mapping of this
> + * instance before returning.
>   *
>   * Return: 0 on success, negative error code on failure.
>   */
> @@ -1475,6 +1514,9 @@ static int drm_gpusvm_dma_map_pages(struct drm_gpusvm *gpusvm,
>  
>  	lockdep_assert_held(&gpusvm->notifier_lock);
>  
> +	*state = (struct dma_iova_state){};
> +	svm_pages->state_offset = 0;
> +
>  	flags.__flags = svm_pages->flags.__flags;
>  
>  	for (i = 0, j = 0; i < npages; ++j) {
> @@ -1603,20 +1645,29 @@ static int drm_gpusvm_dma_map_pages(struct drm_gpusvm *gpusvm,
>  /**
>   * 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
> + * @svm_pages: Array of SVM pages instances to populate with dma addresses
> + * @num_pages: Number of drm_gpusvm_pages instances in @svm_pages
>   * @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.
> + * This function gets and maps pages for a CPU range and ensures they are
> + * mapped for DMA access. The HMM fault for the CPU range is performed once,
> + * the DMA mapping by drm_gpusvm_dma_map_pages() is then done per instance,
> + * one per owning drm_device. The retry against notifier races is kept here
> + * in common code so drivers never open code it.
> + * The common 1:1 case passes @num_pages == 1.
> + *
> + * On error the instances mapped before the failing one stay mapped, so the
> + * caller must unmap and free every instance regardless of the return value.
>   *
>   * Return: 0 on success, negative error code on failure.
>   */
>  int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  			 struct drm_gpusvm_pages *svm_pages,
> +			 unsigned int num_pages,
>  			 struct mm_struct *mm,
>  			 struct mmu_interval_notifier *notifier,
>  			 unsigned long pages_start, unsigned long pages_end,
> @@ -1638,9 +1689,11 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	int err = 0;
>  	enum dma_data_direction dma_dir = ctx->read_only ? DMA_TO_DEVICE :
>  							   DMA_BIDIRECTIONAL;
> +	unsigned int p;
>  
> -	if (!svm_pages->drm)
> -		return -EINVAL;
> +	for (p = 0; p < num_pages; ++p)
> +		if (!svm_pages[p].drm)
> +			return -EINVAL;
>  
>  retry:
>  	remaining = timeout - jiffies;
> @@ -1649,7 +1702,8 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  		return -EBUSY;
>  
>  	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
> -	if (drm_gpusvm_pages_valid_unlocked(gpusvm, svm_pages))
> +
> +	if (drm_gpusvm_pages_valid_unlocked(gpusvm, svm_pages, num_pages))
>  		goto set_seqno;
>  
>  	pfns = kvmalloc_array(npages, sizeof(*pfns), GFP_KERNEL);
> @@ -1667,18 +1721,17 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	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) {
> +	for (p = 0; p < num_pages; ++p) {
> +		if (svm_pages[p].dma_addr)
> +			continue;
> +		svm_pages[p].dma_addr =
> +			kvzalloc_objs(*svm_pages[p].dma_addr, npages);
> +		if (!svm_pages[p].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
> @@ -1686,10 +1739,12 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	 */
>  	drm_gpusvm_notifier_lock(gpusvm);
>  
> -	if (svm_pages->flags.unmapped) {
> -		drm_gpusvm_notifier_unlock(gpusvm);
> -		err = -EFAULT;
> -		goto err_free;
> +	for (p = 0; p < num_pages; ++p) {
> +		if (svm_pages[p].flags.unmapped) {
> +			drm_gpusvm_notifier_unlock(gpusvm);
> +			err = -EFAULT;
> +			goto err_free;
> +		}

I believe, given how the notifiers work, that checking
`svm_pages[0].flags.unmapped` is actually sufficient. It's a
micro-optimization, so I'm fine with it either way.
 
>  	}
>  
>  	if (mmu_interval_read_retry(notifier, hmm_range.notifier_seq)) {
> @@ -1698,15 +1753,29 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  		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;
> +	for (p = 0; p < num_pages; ++p) {
> +		if (drm_gpusvm_pages_valid(gpusvm, &svm_pages[p]))
> +			continue;
>  
> +		err = drm_gpusvm_dma_map_pages(gpusvm, &svm_pages[p], pfns,
> +					       npages, ctx, dma_dir);

One thing that is different here is that if `drm_gpusvm_dma_map_pages()`
fails, say at `p == 1`, then `p[0]` will already contain valid DMA
mappings. I think this is actually fine, though, because the existing
cleanup paths will eventually release those mappings one way or another.
 
That said, it's probably worth confirming this through a code-path audit
and adding a comment here explaining why this is safe.
 
Again, Sashiko didn't run on this patch, and it would be good to get a
run before merging this series.

Matt

> +		if (err) {
> +			/*
> +			 * The failing instance was unwound by the helper. Keep
> +			 * the ones mapped earlier: the -EAGAIN retry reuses
> +			 * them, and the driver unmaps every instance with the
> +			 * range on the other error paths.
> +			 */
> +			drm_gpusvm_notifier_unlock(gpusvm);
> +			goto err_free;
> +		}
> +	}
> +
> +	drm_gpusvm_notifier_unlock(gpusvm);
>  	kvfree(pfns);
>  set_seqno:
> -	svm_pages->notifier_seq = hmm_range.notifier_seq;
> +	for (p = 0; p < num_pages; ++p)
> +		svm_pages[p].notifier_seq = hmm_range.notifier_seq;
>  
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> index 627a741293d..1c7793d8caa 100644
> --- a/drivers/gpu/drm/xe/xe_svm.c
> +++ b/drivers/gpu/drm/xe/xe_svm.c
> @@ -1598,7 +1598,7 @@ int xe_svm_range_get_pages(struct xe_vm *vm, struct xe_svm_range *range,
>  
>  	lockdep_assert_held(&range->lock);
>  
> -	err = drm_gpusvm_get_pages(&vm->svm.gpusvm, &range->pages,
> +	err = drm_gpusvm_get_pages(&vm->svm.gpusvm, &range->pages, 1,
>  				   vm->svm.gpusvm.mm,
>  				   &range->base.notifier->notifier,
>  				   drm_gpusvm_range_start(&range->base),
> diff --git a/drivers/gpu/drm/xe/xe_userptr.c b/drivers/gpu/drm/xe/xe_userptr.c
> index 90ac141fc12..9c1dac0fce6 100644
> --- a/drivers/gpu/drm/xe/xe_userptr.c
> +++ b/drivers/gpu/drm/xe/xe_userptr.c
> @@ -91,7 +91,7 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma *uvma)
>  	if (vma->gpuva.flags & XE_VMA_DESTROYED)
>  		return 0;
>  
> -	return drm_gpusvm_get_pages(&vm->svm.gpusvm, &uvma->userptr.pages,
> +	return drm_gpusvm_get_pages(&vm->svm.gpusvm, &uvma->userptr.pages, 1,
>  				    uvma->userptr.notifier.mm,
>  				    &uvma->userptr.notifier,
>  				    xe_vma_userptr(vma),
> diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> index b7d987bf76a..d2b6f3d2b84 100644
> --- a/include/drm/drm_gpusvm.h
> +++ b/include/drm/drm_gpusvm.h
> @@ -324,6 +324,7 @@ void drm_gpusvm_range_set_unmapped(struct drm_gpusvm_range *range,
>  
>  int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  			 struct drm_gpusvm_pages *svm_pages,
> +			 unsigned int num_pages,
>  			 struct mm_struct *mm,
>  			 struct mmu_interval_notifier *notifier,
>  			 unsigned long pages_start, unsigned long pages_end,
> -- 
> 2.34.1
>