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.
Thanks,
Steve
