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

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

On Wed, 11 Feb 2026 15:58:27 +0000
Steven Price <steven.price@arm.com> wrote:

> On 11/02/2026 08:03, 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
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> Looks good, but one issue I missed previously below.
> 
> [...]
> 
> > +
> > +static void *
> > +panthor_gem_vmap_get_locked(struct panthor_gem_object *bo)
> > +{
> > +	pgprot_t prot = PAGE_KERNEL;
> > +	void *vaddr;
> > +	int ret;
> > +
> > +	dma_resv_assert_held(bo->base.resv);
> > +
> > +	if (drm_WARN_ON_ONCE(bo->base.dev, drm_gem_is_imported(&bo->base)))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (refcount_inc_not_zero(&bo->cmap.vaddr_use_count)) {
> > +		drm_WARN_ON_ONCE(bo->base.dev, !bo->cmap.vaddr);
> > +		return bo->cmap.vaddr;
> > +	}
> > +
> > +	ret = panthor_gem_backing_pin_locked(bo);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	ret = panthor_gem_prep_for_cpu_map_locked(bo);
> > +	if (ret)
> > +		return ERR_PTR(ret);  
> 
> This should be "goto err_unpin" to drop the pin.

Oops, will fix in v4.