Re: [PATCH v4 5/9] drm/panthor: Part ways with drm_gem_shmem_object

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

On Fri, 6 Mar 2026 11:58:09 +0000
Steven Price <steven.price@arm.com> 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/20240105184624.508603-1-dmitry.osipenko@collabora.com/
> > 
> > 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 <boris.brezillon@collabora.com>
> > ---
> >  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.

> 
> Thanks,
> Steve
>