On Wed, 11 Feb 2026 15:58:27 +0000 Steven Price <[email protected]> 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/[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 > > > > Signed-off-by: Boris Brezillon <[email protected]> > > 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.
