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