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

["Huang, Honglei" <honghuan@amd.com>]

On 9/2/2026 3:57 AM, Matthew Brost wrote:
> 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.

Agreed, will change to `svm_pages[0].flags.unmapped` .

>   
>>   	}
>>   
>>   	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.
> 

Actually, I hesitated when modifying the code whether to perform a 
rollback here. And I decided to not rollback here, driver can reuse the
previous success mapping when return -EAGAIN, and for other error code,
driver will call drm_gpusvm_unmap_pages then range_free safely.

And actullay I put some comments below, after if (err),
and int drm_gpusvm.c:1663, I put same documents for this situlation.

And I will also resent this series to trigger Sashiko review, and use AI 
to scan this part to ensure it is safe.

Regards,
Honglei

> 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
>>