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.
Thanks,
Steve
> +
> + if (should_map_wc(bo))
> + prot = pgprot_writecombine(prot);
> +
> + vaddr = vmap(bo->backing.pages, bo->base.size >> PAGE_SHIFT, VM_MAP,
> prot);
> + if (!vaddr) {
> + ret = -ENOMEM;
> + goto err_unpin;
> + }
> +
> + bo->cmap.vaddr = vaddr;
> + refcount_set(&bo->cmap.vaddr_use_count, 1);
> + return vaddr;
> +
> +err_unpin:
> + panthor_gem_backing_unpin_locked(bo);
> + return ERR_PTR(ret);
> +}
> +
[...]
