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.
> + 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.
> 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;
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
>
