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 <[email protected]>
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 <[email protected]>
---
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
