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 <[email protected]>
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 <[email protected]> [1] https://sashiko.dev/#/patchset/20260901090100.2024933-1-honghuan%40amd.com > Signed-off-by: Honglei Huang <[email protected]> > --- > 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 >
