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

[Steven Price]

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@xxxxxxx>
> Reviewed-by: Akash Goel <akash.goel@xxxxxxx>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>

In itself I can't see anything wrong with this change, so:

Reviewed-by: Steven Price <steven.price@xxxxxxx>

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.

Thanks,
Steve

>  	}
>  }
>