On Mon, 5 Oct 2026 16:31:09 +0100 Steven Price <[email protected]> wrote:
> On 05/10/2026 12:53, Boris Brezillon wrote: > > On Mon, 5 Oct 2026 12:03:26 +0100 > > Steven Price <[email protected]> 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 <[email protected]> > >>> Reviewed-by: Akash Goel <[email protected]> > >>> Signed-off-by: Boris Brezillon <[email protected]> > >> > >> In itself I can't see anything wrong with this change, so: > >> > >> Reviewed-by: Steven Price <[email protected]> > >> > >> 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
