Re: [PATCH v2 1/4] drm/gpusvm: move dma_addr allocation before the notifier lock

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

On Tue, Sep 01, 2026 at 05:00:57PM +0800, Honglei Huang wrote:
> The dma_addr allocation was in a lazy allocation flow, it needs unlock
> and goto map_pages. The allocation only needs npages, so just do it
> before taking the lock. Drop the map_pages label and the relock flow, so
> the sequence becomes fault, allocate, then lock, validate, map and
> unlock. No functional change intended.
> 
> Signed-off-by: Honglei Huang <honghuan@amd.com>

One follow up suggestion below - not blocker for merging this series.
Feel free to implement this or at some point (we) Intel will get around
to this.

This patch is:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  drivers/gpu/drm/drm_gpusvm.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index a93eee7ddb9..b507de539e6 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -1516,10 +1516,18 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	if (err)
>  		goto err_free;
>  
> +	if (!svm_pages->dma_addr) {
> +		svm_pages->dma_addr =
> +			kvzalloc_objs(*svm_pages->dma_addr, npages);

One thing that isn't great about the current code is that, regardless of
`npages`, the overwhelming majority of cases result in exactly one DMA
mapping. As a result, we end up wasting a significant amount of memory.
If an IOVA is allocated, we only ever need a single DMA mapping. Even
without IOVA support (e.g., `iommu=off` or `iommu=pt`), a 2 MiB backing
store implemented as a THP would still yield a single DMA mapping via
`dma_map_page()`.

Longer term, I'd like to do something like the structure changes below.
We would still preallocate unless `npages == 1`, but if
`drm_gpusvm_dma_map_pages()` finds exactly one DMA mapping, we could free
`dma_addr`, store the `drm_pagemap_addr` in `inline_addr`, and set an
`inline_dma_mapping` flag. This would save 8 KiB per page of
`drm_gpusvm_pages` for every 2 MiB THP-backed allocation.

Then the final piece is teach drivers to understand 'inline_dma_mapping'
in their iterators (xe_res_cursor.h in Xe) to correctly walk the
dma-mapping.

Matt 

diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
index b7d987bf76aa..73b7065610f0 100644
--- a/include/drm/drm_gpusvm.h
+++ b/include/drm/drm_gpusvm.h
@@ -121,6 +121,7 @@ struct drm_gpusvm_pages_flags {
                        u16 unmapped : 1;
                        u16 has_devmem_pages : 1;
                        u16 has_dma_mapping : 1;
+                       u16 inline_dma_mapping : 1;
                };
                u16 __flags;
        };
@@ -140,7 +141,10 @@ struct drm_gpusvm_pages_flags {
  */
 struct drm_gpusvm_pages {
        struct drm_device *drm;
-       struct drm_pagemap_addr *dma_addr;
+       union {
+               struct drm_pagemap_addr *dma_addr;
+               struct drm_pagemap_addr inline_addr;
+       };
        struct drm_pagemap *dpagemap;
        struct dma_iova_state state;
        unsigned long state_offset;


> +		if (!svm_pages->dma_addr) {
> +			err = -ENOMEM;
> +			goto err_free;
> +		}
> +	}
> +
>  	*state = (struct dma_iova_state){};
>  	svm_pages->state_offset = 0;
>  
> -map_pages:
>  	/*
>  	 * Perform all dma mappings under the notifier lock to not
>  	 * access freed pages. A notifier will either block on
> @@ -1540,18 +1548,6 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  		goto retry;
>  	}
>  
> -	if (!svm_pages->dma_addr) {
> -		/* Unlock and restart mapping to allocate memory. */
> -		drm_gpusvm_notifier_unlock(gpusvm);
> -		svm_pages->dma_addr =
> -			kvzalloc_objs(*svm_pages->dma_addr, npages);
> -		if (!svm_pages->dma_addr) {
> -			err = -ENOMEM;
> -			goto err_free;
> -		}
> -		goto map_pages;
> -	}
> -
>  	zdd = NULL;
>  	pagemap = NULL;
>  	num_dma_mapped = 0;
> -- 
> 2.34.1
>