On Thu, Jan 15, 2026 at 06:27:00PM +0100, Boris Brezillon wrote:
> On Thu, 15 Jan 2026 16:51:31 +0000
> Liviu Dudau <[email protected]> wrote:
>
> > On Fri, Jan 09, 2026 at 02:07:57PM +0100, 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]/
> > >
> > > 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 | 696 ++++++++++++++++++++----
> > > drivers/gpu/drm/panthor/panthor_gem.h | 62 ++-
> > > drivers/gpu/drm/panthor/panthor_mmu.c | 49 +-
> > > drivers/gpu/drm/panthor/panthor_sched.c | 9 +-
> > > 7 files changed, 666 insertions(+), 174 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panthor/Kconfig
> > > b/drivers/gpu/drm/panthor/Kconfig
> > > index 55b40ad07f3b..911e7f4810c3 100644
> > > --- a/drivers/gpu/drm/panthor/Kconfig
> > > +++ b/drivers/gpu/drm/panthor/Kconfig
> > > @@ -8,7 +8,6 @@ config DRM_PANTHOR
> > > depends on MMU
> > > select DEVFREQ_GOV_SIMPLE_ONDEMAND
> > > select DRM_EXEC
> > > - select DRM_GEM_SHMEM_HELPER
> > > select DRM_GPUVM
> > > select DRM_SCHED
> > > select IOMMU_IO_PGTABLE_LPAE
> > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c
> > > b/drivers/gpu/drm/panthor/panthor_drv.c
> > > index 52c27a60c84a..90e9abc22d9e 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_drv.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> > > @@ -19,6 +19,7 @@
> > > #include <drm/drm_debugfs.h>
> > > #include <drm/drm_drv.h>
> > > #include <drm/drm_exec.h>
> > > +#include <drm/drm_file.h>
> > > #include <drm/drm_ioctl.h>
> > > #include <drm/drm_print.h>
> > > #include <drm/drm_syncobj.h>
> > > @@ -1457,7 +1458,7 @@ static int panthor_ioctl_bo_query_info(struct
> > > drm_device *ddev, void *data,
> > > args->create_flags = bo->flags;
> > >
> > > args->extra_flags = 0;
> > > - if (drm_gem_is_imported(&bo->base.base))
> > > + if (drm_gem_is_imported(&bo->base))
> > > args->extra_flags |= DRM_PANTHOR_BO_IS_IMPORTED;
> > >
> > > drm_gem_object_put(obj);
> > > @@ -1671,8 +1672,7 @@ static const struct drm_driver panthor_drm_driver =
> > > {
> > > .major = 1,
> > > .minor = 7,
> > >
> > > - .gem_create_object = panthor_gem_create_object,
> > > - .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> > > + .gem_prime_import_sg_table = panthor_gem_prime_import_sg_table,
> > > .gem_prime_import = panthor_gem_prime_import,
> > > #ifdef CONFIG_DEBUG_FS
> > > .debugfs_init = panthor_debugfs_init,
> > > @@ -1822,3 +1822,4 @@ module_exit(panthor_exit);
> > > MODULE_AUTHOR("Panthor Project Developers");
> > > MODULE_DESCRIPTION("Panthor DRM Driver");
> > > MODULE_LICENSE("Dual MIT/GPL");
> > > +MODULE_IMPORT_NS("DMA_BUF");
> > > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c
> > > b/drivers/gpu/drm/panthor/panthor_fw.c
> > > index a64ec8756bed..f135cf2130b8 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_fw.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> > > @@ -627,7 +627,6 @@ static int panthor_fw_load_section_entry(struct
> > > panthor_device *ptdev,
> > > u32 cache_mode = hdr.flags &
> > > CSF_FW_BINARY_IFACE_ENTRY_CACHE_MODE_MASK;
> > > struct panthor_gem_object *bo;
> > > u32 vm_map_flags = 0;
> > > - struct sg_table *sgt;
> > > u64 va = hdr.va.start;
> > >
> > > if (!(hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_WR))
> > > @@ -665,11 +664,12 @@ static int panthor_fw_load_section_entry(struct
> > > panthor_device *ptdev,
> > > panthor_fw_init_section_mem(ptdev, section);
> > >
> > > bo = to_panthor_bo(section->mem->obj);
> > > - sgt = drm_gem_shmem_get_pages_sgt(&bo->base);
> > > - if (IS_ERR(sgt))
> > > - return PTR_ERR(sgt);
> > >
> > > - dma_sync_sgtable_for_device(ptdev->base.dev, sgt,
> > > DMA_TO_DEVICE);
> > > + /* An sgt should have been requested when the kernel BO was
> > > GPU-mapped. */
> > > + if (drm_WARN_ON_ONCE(&ptdev->base, !bo->dmap.sgt))
> > > + return -EINVAL;
> > > +
> > > + dma_sync_sgtable_for_device(ptdev->base.dev, bo->dmap.sgt,
> > > DMA_TO_DEVICE);
> > > }
> > >
> > > if (hdr.va.start == CSF_MCU_SHARED_REGION_START)
> > > @@ -729,8 +729,10 @@ panthor_reload_fw_sections(struct panthor_device
> > > *ptdev, bool full_reload)
> > > continue;
> > >
> > > panthor_fw_init_section_mem(ptdev, section);
> > > - sgt =
> > > drm_gem_shmem_get_pages_sgt(&to_panthor_bo(section->mem->obj)->base);
> > > - if (!drm_WARN_ON(&ptdev->base, IS_ERR_OR_NULL(sgt)))
> > > +
> > > + /* An sgt should have been requested when the kernel BO was
> > > GPU-mapped. */
> >
> > I know these comments are helping us now when reviewing, but I feel that
> > they are not adding
> > actual information, specially as we do a WARN_ON anyway on it.
>
> Well, it helps the person hitting the WARN_ON() and checking the code
> understand what the expectations are, which I think the WARN_ON() alone
> doesn't really help with.
>
> >
> > > + sgt = to_panthor_bo(section->mem->obj)->dmap.sgt;
> > > + if (!drm_WARN_ON_ONCE(&ptdev->base, !sgt))
> > > dma_sync_sgtable_for_device(ptdev->base.dev, sgt,
> > > DMA_TO_DEVICE);
> > > }
> > > }
> > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c
> > > b/drivers/gpu/drm/panthor/panthor_gem.c
> > > index 4b3d82f001d8..0e52c7a07c87 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > > @@ -8,9 +8,11 @@
> > > #include <linux/dma-mapping.h>
> > > #include <linux/err.h>
> > > #include <linux/slab.h>
> > > +#include <linux/vmalloc.h>
> > >
> > > #include <drm/drm_debugfs.h>
> > > #include <drm/drm_file.h>
> > > +#include <drm/drm_prime.h>
> > > #include <drm/drm_print.h>
> > > #include <drm/panthor_drm.h>
> > >
> > > @@ -44,7 +46,7 @@ static void panthor_gem_debugfs_bo_init(struct
> > > panthor_gem_object *bo)
> > >
> > > static void panthor_gem_debugfs_bo_add(struct panthor_gem_object *bo)
> > > {
> > > - struct panthor_device *ptdev = container_of(bo->base.base.dev,
> > > + struct panthor_device *ptdev = container_of(bo->base.dev,
> > > struct panthor_device,
> > > base);
> > >
> > > bo->debugfs.creator.tgid = current->group_leader->pid;
> > > @@ -57,7 +59,7 @@ static void panthor_gem_debugfs_bo_add(struct
> > > panthor_gem_object *bo)
> > >
> > > static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo)
> > > {
> > > - struct panthor_device *ptdev = container_of(bo->base.base.dev,
> > > + struct panthor_device *ptdev = container_of(bo->base.dev,
> > > struct panthor_device,
> > > base);
> > >
> > > if (list_empty(&bo->debugfs.node))
> > > @@ -80,9 +82,9 @@ static void panthor_gem_debugfs_bo_init(struct
> > > panthor_gem_object *bo) {}
> > > #endif
> > >
> > > static bool
> > > -should_map_wc(struct panthor_gem_object *bo, struct panthor_vm
> > > *exclusive_vm)
> > > +should_map_wc(struct panthor_gem_object *bo)
> > > {
> > > - struct panthor_device *ptdev = container_of(bo->base.base.dev, struct
> > > panthor_device, base);
> > > + struct panthor_device *ptdev = container_of(bo->base.dev, struct
> > > panthor_device, base);
> > >
> > > /* We can't do uncached mappings if the device is coherent,
> > > * because the zeroing done by the shmem layer at page allocation
> > > @@ -112,6 +114,208 @@ should_map_wc(struct panthor_gem_object *bo, struct
> > > panthor_vm *exclusive_vm)
> > > return true;
> > > }
> > >
> > > +static void
> > > +panthor_gem_backing_cleanup(struct panthor_gem_object *bo)
> > > +{
> > > + if (!bo->backing.pages)
> > > + return;
> > > +
> > > + drm_gem_put_pages(&bo->base, bo->backing.pages, true, false);
> > > + bo->backing.pages = NULL;
> > > +}
> > > +
> > > +static int
> > > +panthor_gem_backing_get_pages_locked(struct panthor_gem_object *bo)
> > > +{
> > > + dma_resv_assert_held(bo->base.resv);
> > > +
> > > + if (bo->backing.pages)
> > > + return 0;
> > > +
> > > + bo->backing.pages = drm_gem_get_pages(&bo->base);
> > > + if (IS_ERR(bo->backing.pages)) {
> > > + drm_dbg_kms(bo->base.dev, "Failed to get pages (%pe)\n",
> > > + bo->backing.pages);
> > > + return PTR_ERR(bo->backing.pages);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int panthor_gem_backing_pin_locked(struct panthor_gem_object *bo)
> > > +{
> > > + int ret;
> > > +
> > > + dma_resv_assert_held(bo->base.resv);
> > > + drm_WARN_ON_ONCE(bo->base.dev, drm_gem_is_imported(&bo->base));
> > > +
> > > + if (refcount_inc_not_zero(&bo->backing.pin_count))
> > > + return 0;
> > > +
> > > + ret = panthor_gem_backing_get_pages_locked(bo);
> > > + if (!ret)
> > > + refcount_set(&bo->backing.pin_count, 1);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void panthor_gem_backing_unpin_locked(struct panthor_gem_object
> > > *bo)
> > > +{
> > > + dma_resv_assert_held(bo->base.resv);
> > > + drm_WARN_ON_ONCE(bo->base.dev, drm_gem_is_imported(&bo->base));
> > > +
> > > + /* We don't release anything when pin_count drops to zero.
> > > + * Pages stay there until an explicit cleanup is requested.
> > > + */
> > > + if (!refcount_dec_not_one(&bo->backing.pin_count))
> > > + refcount_set(&bo->backing.pin_count, 0);
> > > +}
> > > +
> > > +static void
> > > +panthor_gem_dev_map_cleanup(struct panthor_gem_object *bo)
> > > +{
> > > + if (!bo->dmap.sgt)
> > > + return;
> > > +
> > > + dma_unmap_sgtable(drm_dev_dma_dev(bo->base.dev), bo->dmap.sgt,
> > > DMA_BIDIRECTIONAL, 0);
> > > + sg_free_table(bo->dmap.sgt);
> > > + kfree(bo->dmap.sgt);
> > > + bo->dmap.sgt = NULL;
> > > +}
> > > +
> > > +static struct sg_table *
> > > +panthor_gem_dev_map_get_sgt_locked(struct panthor_gem_object *bo)
> > > +{
> > > + struct sg_table *sgt;
> > > + int ret;
> > > +
> > > + dma_resv_assert_held(bo->base.resv);
> > > +
> > > + if (bo->dmap.sgt)
> > > + return bo->dmap.sgt;
> > > +
> > > + if (drm_WARN_ON_ONCE(bo->base.dev, !bo->backing.pages))
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + /* Pages stay around after they've been allocated. At least that stands
> > > + * until we add a shrinker.
> > > + */
> > > + ret = panthor_gem_backing_get_pages_locked(bo);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> > > +
> > > + sgt = drm_prime_pages_to_sg(bo->base.dev, bo->backing.pages,
> > > + bo->base.size >> PAGE_SHIFT);
> > > + if (IS_ERR(sgt))
> > > + return sgt;
> > > +
> > > + /* Map the pages for use by the h/w. */
> > > + ret = dma_map_sgtable(drm_dev_dma_dev(bo->base.dev), sgt,
> > > DMA_BIDIRECTIONAL, 0);
> > > + if (ret)
> > > + goto err_free_sgt;
> > > +
> > > + bo->dmap.sgt = sgt;
> > > + return sgt;
> > > +
> > > +err_free_sgt:
> > > + sg_free_table(sgt);
> > > + kfree(sgt);
> > > + return ERR_PTR(ret);
> > > +}
> > > +
> > > +struct sg_table *
> > > +panthor_gem_get_dev_sgt(struct panthor_gem_object *bo)
> > > +{
> > > + struct sg_table *sgt;
> > > +
> > > + dma_resv_lock(bo->base.resv, NULL);
> > > + sgt = panthor_gem_dev_map_get_sgt_locked(bo);
> > > + dma_resv_unlock(bo->base.resv);
> > > +
> > > + return sgt;
> > > +}
> > > +
> > > +static void
> > > +panthor_gem_vmap_cleanup(struct panthor_gem_object *bo)
> >
> > As it was already discussed, either this function name needs _locked or
> > we need to acquire the reservation lock inside it.
>
> Yep, already done in the v2 I'm cooking.
>
> >
> > > +{
> > > + if (!bo->cmap.vaddr)
> > > + return;
> > > +
> > > + vunmap(bo->cmap.vaddr);
> > > + bo->cmap.vaddr = NULL;
> > > + panthor_gem_backing_unpin_locked(bo);
> > > +}
> > > +
>
> [...]
>
> > > +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);
> >
> > Is this drm_WARN_ON_ONCE() necessary? I can't see how we can ever trigger
> > it.
>
> I know it's not supposed to happen, but isn't WARN_ON() exactly about
> catching unexpected situations? :p
Agree, but I'm not convinced the WARN_ON() can be triggered at all as cmap.vaddr
should not be zero if cmap.vaddr_use_count is non-zero.
>
> >
> > > + return bo->cmap.vaddr;
> > > + }
>
> [...]
>
> > > +
> > > +static vm_fault_t panthor_gem_fault(struct vm_fault *vmf)
> > > +{
> > > + struct vm_area_struct *vma = vmf->vma;
> > > + struct drm_gem_object *obj = vma->vm_private_data;
> > > + struct panthor_gem_object *bo = to_panthor_bo(vma->vm_private_data);
> > > + loff_t num_pages = obj->size >> PAGE_SHIFT;
> > > + vm_fault_t ret;
> > > + pgoff_t page_offset;
> > > + unsigned long pfn;
> > > +
> > > + /* Offset to faulty address in the VMA. */
> > > + page_offset = vmf->pgoff - vma->vm_pgoff;
> >
> > You're missing a shift right by PAGE_SHIFT here for the rest of the code
> > to make sense.
>
> Pretty sure I picked that from drm_gem_shmem_helper.c, so if it's buggy
> here, it's buggy there too. I believe the pgoff values are in pages not
> bytes, so I'd say we're good.
I've managed to confuse myself by looking at the drm_gem_shmem_fault()
implementation
and failing to notice that the similar looking code is using pgoff and not
address
like the drm_gem_shmem_helper.c. Like I've said in the following email, line got
removed anyway so ignore me on this one.
Best regards,
Liviu
>
> >
> > With that fixed,
> >
> > Reviewed-by: Liviu Dudau <[email protected]>
