On 6/10/2026 12:07 PM, Matthew Brost wrote:
On Wed, Jun 03, 2026 at 02:56:17PM +0800, Honglei Huang wrote:From: Honglei Huang <[email protected]> drm_gpusvm_pages is the layer that actually represents physical pages/mappings it owns the dma_addr array, the dma_iova_state... With the previous patch, so drm_gpusvm_pages is now strictly about physical pages and their DMA view. Since now the drm_gpusvm_pages instance is inherently bound to one specific drm_device, make that ownership explicit by giving drm_gpusvm_pages its own drm_device handle, and drive all DMA through it instead of through the gpusvm: - Add drm to struct drm_gpusvm_pages and a matching drm parameter to drm_gpusvm_get_pages(); the dma device is bound on first use and immutable for the lifetime of the pages instance. - Route all DMA in drm_gpusvm_get_pages() / __drm_gpusvm_unmap_pages() through svm_pages->drm instead of gpusvm->drm. - Update existing callers (drm_gpusvm_range_get_pages, xe userptr) Suggested-by: Matthew Brost <[email protected]> Signed-off-by: Honglei Huang <[email protected]> --- drivers/gpu/drm/drm_gpusvm.c | 37 ++++++++++++++++++++++++--------- drivers/gpu/drm/xe/xe_userptr.c | 1 + include/drm/drm_gpusvm.h | 3 +++ 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c index 6000d587cf2..3f076178b2a 100644 --- a/drivers/gpu/drm/drm_gpusvm.c +++ b/drivers/gpu/drm/drm_gpusvm.c @@ -1135,11 +1135,16 @@ static void __drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm, unsigned long npages) { struct drm_pagemap *dpagemap = svm_pages->dpagemap; - struct device *dev = gpusvm->drm->dev; + struct device *dev; unsigned long i, j;lockdep_assert_held(&gpusvm->notifier_lock); + if (WARN_ON_ONCE(!svm_pages->drm))I think it is valid to reach this point without calling get_pages() and assigning ->drm, so I don’t believe a WARN_ON is required. One example would be creating a range, attempting to migrate it, and then failing because the user performs a munmap() on part of the range, resulting in the range being freed. It’s a weird race, but it’s possible, and I’m fairly certain Xe SVM tests exercise scenarios like this. So I would drop the WARN_ON, add a comment like “get_pages() never called,” and bail out silently. Alternatively, if drm is NULL and has_dma_mapping is set, then a WARN_ON might make sense, as that should not be possible.
Got it, will drop the WARN_ON.
+ return; + + dev = svm_pages->drm->dev; + if (svm_pages->flags.has_dma_mapping) { struct drm_gpusvm_pages_flags flags = { .__flags = svm_pages->flags.__flags, @@ -1379,6 +1384,7 @@ static bool drm_gpusvm_pages_valid_unlocked(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 + * @drm: The DRM device that will own the DMA mappings. Stored into @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 @@ -1392,6 +1398,7 @@ static bool drm_gpusvm_pages_valid_unlocked(struct drm_gpusvm *gpusvm, */ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, struct drm_gpusvm_pages *svm_pages, + struct drm_device *drm,You could also move drm into a function like drm_gpusvm_init_pages() (as mentioned in the cover letter). I don’t have a strong preference, but if we want a helper that calls hmm_range_fault() once and accepts an array of drm_gpusvm_pages to DMA-map, that might make sense.
Got it, will init the drm device in drm_gpusvm_init_pages.
struct mm_struct *mm, struct mmu_interval_notifier *notifier, unsigned long pages_start, unsigned long pages_end, @@ -1421,6 +1428,15 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, DMA_BIDIRECTIONAL; struct dma_iova_state *state = &svm_pages->state;+ if (!drm)+ return -EINVAL; + if (svm_pages->drm) { + if (svm_pages->drm != drm) + return -EINVAL; + } else { + svm_pages->drm = drm; + }Style nit: If we keep this I'd write this like: if (!drm || (svm_pages->drm && svm_pages->drm != drm)) return -EINVAL; svm_pages->drm = drm;
Got it will modify in next version. Regards, Honglei
Matt+ retry: if (time_after(jiffies, timeout)) return -EBUSY; @@ -1515,7 +1531,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,pagemap = page_pgmap(page);dpagemap = drm_pagemap_page_to_dpagemap(page); - if (drm_WARN_ON(gpusvm->drm, !dpagemap)) { + if (drm_WARN_ON(drm, !dpagemap)) { /* * Raced. This is not supposed to happen * since hmm_range_fault() should've migrated @@ -1527,10 +1543,10 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, } svm_pages->dma_addr[j] = dpagemap->ops->device_map(dpagemap, - gpusvm->drm->dev, + drm->dev, page, order, dma_dir); - if (dma_mapping_error(gpusvm->drm->dev, + if (dma_mapping_error(drm->dev, svm_pages->dma_addr[j].addr)) { err = -EFAULT; goto err_unmap; @@ -1550,11 +1566,11 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, }if (!i)- dma_iova_try_alloc(gpusvm->drm->dev, state, + dma_iova_try_alloc(drm->dev, state, 0, npages * PAGE_SIZE);if (dma_use_iova(state)) {- err = dma_iova_link(gpusvm->drm->dev, state, + err = dma_iova_link(drm->dev, state, hmm_pfn_to_phys(pfns[i]), svm_pages->state_offset, PAGE_SIZE << order, @@ -1565,11 +1581,11 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, addr = state->addr + svm_pages->state_offset; svm_pages->state_offset += PAGE_SIZE << order; } else { - addr = dma_map_page(gpusvm->drm->dev, + addr = dma_map_page(drm->dev, page, 0, PAGE_SIZE << order, dma_dir); - if (dma_mapping_error(gpusvm->drm->dev, addr)) { + if (dma_mapping_error(drm->dev, addr)) { err = -EFAULT; goto err_unmap; } @@ -1585,7 +1601,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm, }if (dma_use_iova(state)) {- err = dma_iova_sync(gpusvm->drm->dev, state, 0, + err = dma_iova_sync(drm->dev, state, 0, svm_pages->state_offset); if (err) goto err_unmap; @@ -1635,7 +1651,8 @@ int drm_gpusvm_range_get_pages(struct drm_gpusvm *gpusvm, struct drm_gpusvm_range *range, const struct drm_gpusvm_ctx *ctx) { - return drm_gpusvm_get_pages(gpusvm, &range->pages, gpusvm->mm, + return drm_gpusvm_get_pages(gpusvm, &range->pages, gpusvm->drm, + gpusvm->mm, &range->notifier->notifier, drm_gpusvm_range_start(range), drm_gpusvm_range_end(range), ctx); diff --git a/drivers/gpu/drm/xe/xe_userptr.c b/drivers/gpu/drm/xe/xe_userptr.c index 6761005c0b9..7e28f6868ff 100644 --- a/drivers/gpu/drm/xe/xe_userptr.c +++ b/drivers/gpu/drm/xe/xe_userptr.c @@ -75,6 +75,7 @@ int xe_vma_userptr_pin_pages(struct xe_userptr_vma *uvma) return 0;return drm_gpusvm_get_pages(&vm->svm.gpusvm, &uvma->userptr.pages,+ &xe->drm, 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 3dba4b9516f..ed228d9ff6b 100644 --- a/include/drm/drm_gpusvm.h +++ b/include/drm/drm_gpusvm.h @@ -127,6 +127,7 @@ struct drm_gpusvm_pages_flags { /** * struct drm_gpusvm_pages - Structure representing a GPU SVM mapped pages * + * @drm: The DRM device that owns the dma mappings * @dma_addr: Device address array * @dpagemap: The struct drm_pagemap of the device pages we're dma-mapping. * Note this is assuming only one drm_pagemap per range is allowed. @@ -136,6 +137,7 @@ struct drm_gpusvm_pages_flags { * @flags: Flags for the range; see &struct drm_gpusvm_pages_flags */ struct drm_gpusvm_pages { + struct drm_device *drm; struct drm_pagemap_addr *dma_addr; struct drm_pagemap *dpagemap; struct dma_iova_state state; @@ -328,6 +330,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, + struct drm_device *drm, struct mm_struct *mm, struct mmu_interval_notifier *notifier, unsigned long pages_start, unsigned long pages_end, -- 2.34.1
