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

["Huang, Honglei" <honghuan@amd.com>]

On 9/2/2026 3:39 AM, Matthew Brost wrote:
> 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.

Got it, you can give me RB after you are really satisfied. I can keep 
modifying until you feel there are no problems.

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

Makes sense, working on it to reduce memory waste.

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

Agreed, the important part is driver maybe miss this change and the 
compiles still fine, maybe worth changing xe_res_first_dma() to take a 
drm_gpusvm_pages instead of a bare pointer.

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

Will modify the code according to your suggestion.

Regards,
Honglei


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