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

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

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

Will resent this series only to intel-xe@list and dri-devel@list to 
retigger the Sashiko review.

Regards,
Honglei

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