On 06/03/2026 12:15, Boris Brezillon wrote: > On Fri, 6 Mar 2026 11:58:09 +0000 > Steven Price <[email protected]> wrote: > >> On 05/03/2026 12:43, Boris Brezillon wrote: >>> While drm_gem_shmem_object does most of the job we need it to do, the >>> way sub-resources (pages, sgt, vmap) are handled and their lifetimes >>> gets in the way of BO reclaim. There has been attempts to address >>> that [1], but in the meantime, new gem_shmem users were introduced >>> (accel drivers), and some of them manually free some of these resources. >>> This makes things harder to control/sanitize/validate. >>> >>> Thomas Zimmerman is not a huge fan of enforcing lifetimes of sub-resources >>> and forcing gem_shmem users to go through new gem_shmem helpers when they >>> need manual control of some sort, and I believe this is a dead end if >>> we don't force users to follow some stricter rules through carefully >>> designed helpers, because there will always be one user doing crazy things >>> with gem_shmem_object internals, which ends up tripping out the common >>> helpers when they are called. >>> >>> The consensus we reached was that we would be better off forking >>> gem_shmem in panthor. So here we are, parting ways with gem_shmem. The >>> current transition tries to minimize the changes, but there are still >>> some aspects that are different, the main one being that we no longer >>> have a pages_use_count, and pages stays around until the GEM object is >>> destroyed (or when evicted once we've added a shrinker). The sgt also >>> no longer retains pages. This is losely based on how msm does things by >>> the way. >>> >>> If there's any interest in sharing code (probably with msm, since the >>> panthor shrinker is going to be losely based on the msm implementation), >>> we can always change gears and do that once we have everything >>> working/merged. >>> >>> [1]https://patchwork.kernel.org/project/dri-devel/patch/[email protected]/ >>> >>> v2: >>> - Fix refcounting >>> - Add a _locked suffix to a bunch of functions expecting the resv lock >>> to be held >>> - Take the lock before releasing resources in panthor_gem_free_object() >>> >>> v3: >>> - Use ERR_CAST() to fix an ERR-ptr deref >>> - Add missing resv_[un]lock() around a panthor_gem_backing_unpin_locked() >>> call >>> >>> v4: >>> - Fix an error path in panthor_gem_vmap_get_locked() >>> - Don't leave bo->base.pages with an ERR_PTR() >>> - Make panthor_gem_{pin,unpin}[_locked]() more consistent >>> - Don't fail in panthor_gem_dev_map_get_sgt_locked() if the pages are not >>> allocated >>> >>> Signed-off-by: Boris Brezillon <[email protected]> >>> --- >>> drivers/gpu/drm/panthor/Kconfig | 1 - >>> drivers/gpu/drm/panthor/panthor_drv.c | 7 +- >>> drivers/gpu/drm/panthor/panthor_fw.c | 16 +- >>> drivers/gpu/drm/panthor/panthor_gem.c | 700 ++++++++++++++++++++---- >>> drivers/gpu/drm/panthor/panthor_gem.h | 62 ++- >>> drivers/gpu/drm/panthor/panthor_mmu.c | 48 +- >>> drivers/gpu/drm/panthor/panthor_sched.c | 9 +- >>> 7 files changed, 669 insertions(+), 174 deletions(-) >>> >> >> One minor issue below (and of course the kernel test robot's report). >> >> [...] >> >>> @@ -646,7 +1108,7 @@ static void panthor_gem_debugfs_bo_print(struct >>> panthor_gem_object *bo, >>> struct seq_file *m, >>> struct gem_size_totals *totals) >>> { >>> - unsigned int refcount = kref_read(&bo->base.base.refcount); >>> + unsigned int refcount = kref_read(&bo->base.refcount); >>> char creator_info[32] = {}; >>> size_t resident_size; >>> u32 gem_usage_flags = bo->debugfs.flags; >>> @@ -656,21 +1118,21 @@ static void panthor_gem_debugfs_bo_print(struct >>> panthor_gem_object *bo, >>> if (!refcount) >>> return; >>> >>> - resident_size = bo->base.pages ? bo->base.base.size : 0; >>> + resident_size = bo->backing.pages ? bo->base.size : 0; >>> >>> snprintf(creator_info, sizeof(creator_info), >>> "%s/%d", bo->debugfs.creator.process_name, >>> bo->debugfs.creator.tgid); >>> seq_printf(m, "%-32s%-16d%-16d%-16zd%-16zd0x%-16lx", >>> creator_info, >>> - bo->base.base.name, >>> + bo->base.name, >>> refcount, >>> - bo->base.base.size, >>> + bo->base.size, >>> resident_size, >>> - drm_vma_node_start(&bo->base.base.vma_node)); >>> + drm_vma_node_start(&bo->base.vma_node)); >>> >>> - if (drm_gem_is_imported(&bo->base.base)) >>> + if (drm_gem_is_imported(&bo->base)) >>> gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED; >>> - if (bo->base.base.dma_buf) >>> + if (bo->base.dma_buf) >>> gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED; >>> >>> seq_printf(m, "0x%-8x 0x%-10x", gem_state_flags, gem_usage_flags); >>> @@ -679,10 +1141,8 @@ static void panthor_gem_debugfs_bo_print(struct >>> panthor_gem_object *bo, >>> seq_printf(m, "%s\n", bo->label.str ? : ""); >>> } >>> >>> - totals->size += bo->base.base.size; >>> + totals->size += bo->base.size; >>> totals->resident += resident_size; >>> - if (bo->base.madv > 0) >>> - totals->reclaimable += resident_size; >> >> You've dropped the code for calculating totals->reclaimable - but the >> code for printing this out is still there. So it'll print out "Total >> reclaimable: 0" always. > > I'm mean, it's always been zero anyway, because we never added MADVISE > support, so I'm not too sure it matters in this patch. We should > probably hook that up again in the commit adding the shrinker, based on > the object state.
Sure, I did double check that it wasn't just waiting for a later patch. My main concern was leaving the variable around looking like it was doing something useful. Thanks, Steve
