Re: [PATCH v2 4/5] drm/panthor: Actually check huge-page mapping on sparse regions

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 5 Oct 2026 16:31:09 +0100
Steven Price <steven.price@arm.com> wrote:

> On 05/10/2026 12:53, Boris Brezillon wrote:
> > On Mon, 5 Oct 2026 12:03:26 +0100
> > Steven Price <steven.price@arm.com> wrote:
> >   
> >> On 24/09/2026 12:04, Boris Brezillon wrote:  
> >>> With the recent changes to iova_mapped_as_huge_page(), the check for
> >>> huge-page mapping of sparse BOs is actually simple:
> >>>
> >>> - for a sparse mapping, we know the BO offset any VA in this regions is
> >>>   va & (SZ_2M - 1)
> >>> - the VA we're searching the BO offset for is the 2M-aligned
> >>>   aligned_va value
> >>>
> >>> This guarantees that the BO offset to check is always zero in that case.
> >>>
> >>> This is simple enough to let the code check if page 0 is a huge page
> >>> and save the unmap+map dance when the dummy BO is not backed by a
> >>> a huge page. So let's do that and kill the comment that says it's too
> >>> complicated.
> >>>
> >>> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> >>> Reviewed-by: Akash Goel <akash.goel@arm.com>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>    
> >>
> >> In itself I can't see anything wrong with this change, so:
> >>
> >> Reviewed-by: Steven Price <steven.price@arm.com>
> >>
> >> However...
> >>  
> >>> ---
> >>>  drivers/gpu/drm/panthor/panthor_mmu.c | 15 ++++++++-------
> >>>  1 file changed, 8 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> >>> index d2897099763e..01564d250adf 100644
> >>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> >>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> >>> @@ -2337,18 +2337,18 @@ iova_mapped_as_huge_page(struct drm_gpuva *mapping, u64 va)
> >>>  
> >>>  		return false;
> >>>  	} else {
> >>> -		const struct page *pg = bo->backing.pages[bo_offset >> PAGE_SHIFT];
> >>>  		struct panthor_vma *vma = container_of(mapping, struct panthor_vma, base);
> >>>  		bool is_sparse = vma->flags & DRM_PANTHOR_VM_BIND_OP_MAP_SPARSE;
> >>> +		const struct page *pg;
> >>>  
> >>> -		/* If the unmapped VMA stands for a sparse mapping, always
> >>> -		 * assume the backing storage is a THP, since the overhead of
> >>> -		 * unmapping 2MiB worth of 4KiB pages and remapping some of
> >>> -		 * them is offset by the logic of working out whether it's
> >>> -		 * the opposite case right below.
> >>> +		/* BO offset on a sparse mapping is chosen so that 2M-aligned
> >>> +		 * VAs point to the start of the BO. Since aligned_va (the
> >>> +		 * address we check huge-page against) is 2M-aligned, the BO
> >>> +		 * offset is guaranteed to be zero.
> >>> +		 * Check panthor_fix_sparse_map_offset() for more details.
> >>>  		 */
> >>>  		if (is_sparse)
> >>> -			return true;
> >>> +			bo_offset = 0;
> >>>  
> >>>  		/* In case of shmem backing, we know we can only have a huge
> >>>  		 * mapping if the bo_offset is 2M aligned, meaning we can skip
> >>> @@ -2357,6 +2357,7 @@ iova_mapped_as_huge_page(struct drm_gpuva *mapping, u64 va)
> >>>  		if (!IS_ALIGNED(bo_offset, SZ_2M))
> >>>  			return false;
> >>>  
> >>> +		pg = bo->backing.pages[bo_offset >> PAGE_SHIFT];
> >>>  		return folio_size(page_folio(pg)) >= SZ_2M;    
> >>
> >> ... this seems like it could be problematic. On the mapping side we use
> >> the scatter list to decide whether the region is huge page mapped or
> >> not. The scatter list code can merge segments that are contiguous (see
> >> pages_are_mergeable()), so if we have a region which has small folios we
> >> fail this check even though the pages might have been mapped as huge pages.
> >>
> >> This is a problem on the non-sparse path as well (hence not really
> >> related to this patch). I'm not really sure how to test this though - I
> >> may well have overlooked something here.  
> > 
> > So, this is based on the assumption that shmem backing is allocated
> > with the buddy allocator, and because of how this allocator splits
> > bigger order blocks to service smaller allocations, it's my
> > understanding that two consecutive folios of the same size/order can't
> > be physically contiguous.  
> 
> Yes, you'd expect the buddy allocator to combine the folios back into a
> larger one if they were contiguous. I guess we should be safe, at least
> for now. I still feel it's unnecessarily fragile and complex trying to
> work out whether we've mapped as a huge page or not. But I don't
> actually have a better solution at the moment, and this series is at
> least improving things. I'll put it on my todo list to look at later.

One way to make that consistent would be to have the same logic on the
map_pages() side, where we'd use the pages array to check for physical
contiguity in addition to the dma_addr/size-based checks we already
have. This being said, this might be moot if we consider moving to
our own PT implementation with full sub-tree updates (see how [1]
doesn't have a get_pgsize() anymore). In the meantime, I can add a
comment explaining the assumptions at play here.

[1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commits/b4/panthor-part-ways-with-iopgtbl?ref_type=heads