Re: [RFC 2/5] drm/gpusvm: embed struct drm_device into drm_gpusvm_pages

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Jun 03, 2026 at 02:56:17PM +0800, Honglei Huang wrote:
> From: Honglei Huang <honghuan@amd.com>
> 
> 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 <matthew.brost@intel.com>
> Signed-off-by: Honglei Huang <honghuan@amd.com>
> ---
>  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
>