Re: [PATCH v1 9/9] drm/panthor: Add a GEM shrinker

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

On Wed, 21 Jan 2026 11:49:34 +0000
Akash Goel <akash.goel@arm.com> wrote:

> Hi Boris,
> 
> On 1/9/26 13:08, Boris Brezillon wrote:
> > From: Akash Goel <akash.goel@arm.com>
> > 
> > This implementation is losely based on the MSM shrinker, and it's
> > relying on the drm_gpuvm eviction/validation infrastructure.
> > 
> > Right now we only support swapout/eviction, but we could add an extra
> > flag to specify when buffer content doesn't need to be preserved to
> > avoid the swapout/swapin dance.
> > 
> > Locking is a bit of a nightmare, but using _trylock() all the way in
> > the reclaim path seems to make lockdep happy. And yes, we might be
> > missing opportunities to reclaim when the system is under heavy GPU
> > load/heavy memory pressure/heavy GPU VM activity, but that's better
> > than no reclaim at all.
> > 
> > Signed-off-by: Akash Goel <akash.goel@arm.com>
> > Co-developed-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >   drivers/gpu/drm/panthor/panthor_device.c |  11 +-
> >   drivers/gpu/drm/panthor/panthor_device.h |  73 ++++
> >   drivers/gpu/drm/panthor/panthor_gem.c    | 427 ++++++++++++++++++++++-
> >   drivers/gpu/drm/panthor/panthor_gem.h    |  67 ++++
> >   drivers/gpu/drm/panthor/panthor_mmu.c    | 338 +++++++++++++++++-
> >   drivers/gpu/drm/panthor/panthor_mmu.h    |   8 +
> >   6 files changed, 901 insertions(+), 23 deletions(-)
> >   
> 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > index f35e52b9546a..bc348aa9634e 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.h
> > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> > @@ -14,6 +14,7 @@
> >   #include <linux/spinlock.h>
> >   
> >   #include <drm/drm_device.h>
> > +#include <drm/drm_gem.h>
> >   #include <drm/drm_mm.h>
> >   #include <drm/gpu_scheduler.h>
> >   #include <drm/panthor_drm.h>
> > @@ -157,6 +158,78 @@ struct panthor_device {
> >   	/** @devfreq: Device frequency scaling management data. */
> >   	struct panthor_devfreq *devfreq;
> >   
> >   
> > +static bool is_gpu_mapped(struct panthor_gem_object *bo,
> > +			  enum panthor_gem_reclaim_state *state)
> > +{
> > +	struct drm_gpuvm *vm = NULL;
> > +	struct drm_gpuvm_bo *vm_bo;
> > +
> > +	drm_gem_for_each_gpuvm_bo(vm_bo, &bo->base) {
> > +		if (!vm) {
> > +			*state = PANTHOR_GEM_GPU_MAPPED_PRIVATE;
> > +			vm = vm_bo->vm;
> > +		} else if (vm != vm_bo->vm) {
> > +			*state = PANTHOR_GEM_GPU_MAPPED_SHARED;
> > +			break;
> > +		}
> > +	}
> > +
> > +	return !!vm;
> > +}
> > +
> > +static enum panthor_gem_reclaim_state
> > +panthor_gem_evaluate_reclaim_state_locked(struct panthor_gem_object *bo)
> > +{
> > +	enum panthor_gem_reclaim_state gpu_mapped_state;
> > +
> > +	dma_resv_assert_held(bo->base.resv);
> > +	lockdep_assert_held(&bo->base.gpuva.lock);
> > +
> > +	/* If pages have not been allocated, there's nothing to reclaim. */
> > +	if (!bo->backing.pages)
> > +		return PANTHOR_GEM_UNRECLAIMABLE;
> > +
> > +	/* If memory is pinned, we prevent reclaim. */
> > +	if (refcount_read(&bo->backing.pin_count))
> > +		return PANTHOR_GEM_UNRECLAIMABLE;
> > +
> > +	if (is_gpu_mapped(bo, &gpu_mapped_state))
> > +		return gpu_mapped_state;
> > +
> > +	if (refcount_read(&bo->cmap.mmap_count) && bo->backing.pages)
> > +		return PANTHOR_GEM_MMAPPED;
> > +
> > +	return PANTHOR_GEM_UNUSED;
> > +}
> > +
> > +void panthor_gem_update_reclaim_state_locked(struct panthor_gem_object *bo,
> > +					     enum panthor_gem_reclaim_state *old_statep)
> > +{
> > +	struct panthor_device *ptdev = container_of(bo->base.dev, struct panthor_device, base);
> > +	enum panthor_gem_reclaim_state old_state = bo->reclaim_state;
> > +	enum panthor_gem_reclaim_state new_state;
> > +	bool was_gpu_mapped, is_gpu_mapped;
> > +
> > +	if (old_statep)
> > +		*old_statep = old_state;
> > +
> > +	new_state = panthor_gem_evaluate_reclaim_state_locked(bo);
> > +	if (new_state == old_state)
> > +		return;
> > +
> > +	was_gpu_mapped = old_state == PANTHOR_GEM_GPU_MAPPED_SHARED ||
> > +			 old_state == PANTHOR_GEM_GPU_MAPPED_PRIVATE;
> > +	is_gpu_mapped = new_state == PANTHOR_GEM_GPU_MAPPED_SHARED ||
> > +			new_state == PANTHOR_GEM_GPU_MAPPED_PRIVATE;
> > +
> > +	if (is_gpu_mapped && !was_gpu_mapped)
> > +		ptdev->reclaim.gpu_mapped_count += bo->base.size >> PAGE_SHIFT;
> > +	else if (!is_gpu_mapped && was_gpu_mapped)
> > +		ptdev->reclaim.gpu_mapped_count -= bo->base.size >> PAGE_SHIFT;
> > +
> > +	switch (new_state) {
> > +	case PANTHOR_GEM_UNUSED:
> > +		drm_gem_lru_move_tail(&ptdev->reclaim.unused, &bo->base);
> > +		break;
> > +	case PANTHOR_GEM_MMAPPED:
> > +		drm_gem_lru_move_tail(&ptdev->reclaim.mmapped, &bo->base);
> > +		break;
> > +	case PANTHOR_GEM_GPU_MAPPED_PRIVATE:
> > +		panthor_vm_update_bo_reclaim_lru_locked(bo);
> > +		break;
> > +	case PANTHOR_GEM_GPU_MAPPED_SHARED:
> > +		drm_gem_lru_move_tail(&ptdev->reclaim.gpu_mapped_shared, &bo->base);
> > +		break;
> > +	case PANTHOR_GEM_UNRECLAIMABLE:
> > +		drm_gem_lru_remove(&bo->base);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	bo->reclaim_state = new_state;
> > +}
> > +
> >   static void
> >   panthor_gem_backing_cleanup(struct panthor_gem_object *bo)
> >   {
> > @@ -153,8 +249,12 @@ static int panthor_gem_backing_pin_locked(struct panthor_gem_object *bo)
> >   		return 0;
> >   
> >   	ret = panthor_gem_backing_get_pages_locked(bo);
> > -	if (!ret)
> > +	if (!ret) {
> >   		refcount_set(&bo->backing.pin_count, 1);
> > +		mutex_lock(&bo->base.gpuva.lock);
> > +		panthor_gem_update_reclaim_state_locked(bo, NULL);
> > +		mutex_unlock(&bo->base.gpuva.lock);
> > +	}
> >   
> >   	return ret;
> >   }
> > @@ -167,8 +267,12 @@ static void panthor_gem_backing_unpin_locked(struct panthor_gem_object *bo)
> >   	/* 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))
> > +	if (!refcount_dec_not_one(&bo->backing.pin_count)) {
> >   		refcount_set(&bo->backing.pin_count, 0);
> > +		mutex_lock(&bo->base.gpuva.lock);
> > +		panthor_gem_update_reclaim_state_locked(bo, NULL);
> > +		mutex_unlock(&bo->base.gpuva.lock);
> > +	}
> >   }
> >   
> >   static void
> > @@ -531,6 +635,49 @@ void panthor_gem_unpin(struct panthor_gem_object *bo)
> >   	dma_resv_unlock(bo->base.resv);
> >   }
> >   
> > +
> > +static void panthor_gem_evict_locked(struct panthor_gem_object *bo)
> > +{
> > +	dma_resv_assert_held(bo->base.resv);
> > +	lockdep_assert_held(&bo->base.gpuva.lock);
> > +
> > +	if (drm_WARN_ON_ONCE(bo->base.dev, drm_gem_is_imported(&bo->base)))
> > +		return;
> > +
> > +	if (drm_WARN_ON_ONCE(bo->base.dev, refcount_read(&bo->backing.pin_count)))
> > +		return;
> > +
> > +	if (drm_WARN_ON_ONCE(bo->base.dev, !bo->backing.pages))
> > +		return;
> > +
> > +	panthor_gem_dev_map_cleanup(bo);
> > +	panthor_gem_backing_cleanup(bo);
> > +	panthor_gem_update_reclaim_state_locked(bo, NULL);
> > +}
> > +  
> 
> > +
> > +static bool should_wait(enum panthor_gem_reclaim_state reclaim_state)
> > +{
> > +	return reclaim_state == PANTHOR_GEM_GPU_MAPPED_PRIVATE ||
> > +	       reclaim_state == PANTHOR_GEM_GPU_MAPPED_SHARED;
> > +}
> > +
> > +bool panthor_gem_try_evict(struct drm_gem_object *obj,
> > +			   struct ww_acquire_ctx *ticket)
> > +{
> > +	/*
> > +	 * Track last locked entry for unwinding locks in error and
> > +	 * success paths
> > +	 */
> > +	struct panthor_gem_object *bo = to_panthor_bo(obj);
> > +	struct drm_gpuvm_bo *vm_bo, *last_locked = NULL;
> > +	enum panthor_gem_reclaim_state old_state;
> > +	int ret = 0;
> > +
> > +	/* To avoid potential lock ordering issue between bo_gpuva and
> > +	 * mapping->i_mmap_rwsem, unmap the pages from CPU side before
> > +	 * acquring the bo_gpuva lock. As the bo_resv lock is held, CPU
> > +	 * page fault handler won't be able to map in the pages whilst
> > +	 * eviction is in progress.
> > +	 */
> > +	drm_vma_node_unmap(&bo->base.vma_node, bo->base.dev->anon_inode->i_mapping);
> > +
> > +	/* We take this lock when walking the list to prevent
> > +	 * insertion/deletion.
> > +	 */
> > +	/* We can only trylock in that path, because
> > +	 * - allocation might happen while some of these locks are held
> > +	 * - lock ordering is different in other paths
> > +	 *     vm_resv -> bo_resv -> bo_gpuva
> > +	 *     vs
> > +	 *     bo_resv -> bo_gpuva -> vm_resv
> > +	 *
> > +	 * If we fail to lock that's fine, we back off and will get
> > +	 * back to it later.
> > +	 */
> > +	if (!mutex_trylock(&bo->base.gpuva.lock))
> > +		return false;
> > +
> > +	drm_gem_for_each_gpuvm_bo(vm_bo, obj) {
> > +		struct dma_resv *resv = drm_gpuvm_resv(vm_bo->vm);
> > +
> > +		if (resv == obj->resv)
> > +			continue;
> > +
> > +		if (!dma_resv_trylock(resv)) {
> > +			ret = -EDEADLK;
> > +			goto out_unlock;
> > +		}
> > +
> > +		last_locked = vm_bo;
> > +	}
> > +
> > +	/* Update the state before trying to evict the buffer, if the state was
> > +	 * updated to something that's harder to reclaim (higher value in the
> > +	 * enum), skip it (will be processed when the relevant LRU is).
> > +	 */
> > +	panthor_gem_update_reclaim_state_locked(bo, &old_state);
> > +	if (old_state < bo->reclaim_state) {
> > +		ret = -EAGAIN;
> > +		goto out_unlock;
> > +	}
> > +
> > +	/* Wait was too long, skip. */
> > +	if (should_wait(bo->reclaim_state) &&
> > +	    dma_resv_wait_timeout(bo->base.resv, DMA_RESV_USAGE_BOOKKEEP, false, 10) <= 0) {
> > +		ret = -ETIMEDOUT;
> > +		goto out_unlock;
> > +	}
> > +
> > +	/* Couldn't teardown the GPU mappings? Skip. */
> > +	ret = panthor_vm_evict_bo_mappings_locked(bo);
> > +	if (ret)
> > +		goto out_unlock;
> > +
> > +	/* If everything went fine, evict the object. */
> > +	panthor_gem_evict_locked(bo);
> > +
> > +out_unlock:
> > +	if (last_locked) {
> > +		drm_gem_for_each_gpuvm_bo(vm_bo, obj) {
> > +			struct dma_resv *resv = drm_gpuvm_resv(vm_bo->vm);
> > +
> > +			if (resv == obj->resv)
> > +				continue;
> > +
> > +			dma_resv_unlock(resv);
> > +
> > +			if (last_locked == vm_bo)
> > +				break;
> > +		}
> > +	}
> > +	mutex_unlock(&bo->base.gpuva.lock);
> > +
> > +	return ret == 0;
> > +}  
> 
> 
> > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > index 3290e0b5facb..ffd821b3be46 100644
> > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > @@ -1,6 +1,7 @@
> >   // SPDX-License-Identifier: GPL-2.0 or MIT
> >   /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
> >   /* Copyright 2023 Collabora ltd. */
> > +/* Copyright 2025 ARM Limited. All rights reserved. */
> >   
> >   #include <drm/drm_debugfs.h>
> >   #include <drm/drm_drv.h>
> > @@ -131,6 +132,9 @@ struct panthor_vma {
> >   	 * Only map related flags are accepted.
> >   	 */
> >   	u32 flags;
> > +
> > +	/** @evicted: True if the VMA has been evicted. */
> > +	bool evicted;
> >   };
> >   
> >   /**
> >   
> > +void panthor_vm_update_bo_reclaim_lru_locked(struct panthor_gem_object *bo)
> > +{
> > +	struct panthor_device *ptdev = container_of(bo->base.dev, struct panthor_device, base);
> > +	struct panthor_vm *vm = NULL;
> > +	struct drm_gpuvm_bo *vm_bo;
> > +
> > +	dma_resv_assert_held(bo->base.resv);
> > +	lockdep_assert_held(&bo->base.gpuva.lock);
> > +
> > +	drm_gem_for_each_gpuvm_bo(vm_bo, &bo->base) {
> > +		/* We're only supposed to have one vm_bo in the list if we get there. */
> > +		drm_WARN_ON(&ptdev->base, vm);
> > +		vm = container_of(vm_bo->vm, struct panthor_vm, base);
> > +
> > +		mutex_lock(&ptdev->reclaim.lock);
> > +		drm_gem_lru_move_tail_locked(&vm->reclaim.lru, &bo->base);
> > +		if (list_empty(&vm->reclaim.lru_node))
> > +			list_move(&vm->reclaim.lru_node, &ptdev->reclaim.vms);
> > +		mutex_unlock(&ptdev->reclaim.lock);
> > +	}
> > +}
> > +
> > +int panthor_vm_evict_bo_mappings_locked(struct panthor_gem_object *bo)
> > +{
> > +	struct drm_gpuvm_bo *vm_bo;
> > +
> > +	drm_gem_for_each_gpuvm_bo(vm_bo, &bo->base) {
> > +		struct panthor_vm *vm = container_of(vm_bo->vm, struct panthor_vm, base);
> > +		struct drm_gpuva *va;
> > +
> > +		if (!mutex_trylock(&vm->op_lock))
> > +			return -EDEADLK;
> > +
> > +		drm_gpuvm_bo_evict(vm_bo, true);
> > +		drm_gpuvm_bo_for_each_va(va, vm_bo) {
> > +			struct panthor_vma *vma = container_of(va, struct panthor_vma, base);
> > +
> > +			panthor_vm_lock_region(vm, va->va.addr, va->va.range);
> > +			panthor_vm_unmap_pages(vm, va->va.addr, va->va.range);  
> 
> On further testing, I ran into a kernel warning issue.
> 
> https://elixir.bootlin.com/linux/v6.18-rc5/source/drivers/iommu/io-pgtable-arm.c#L641
> 
> https://elixir.bootlin.com/linux/v6.18-rc5/source/drivers/gpu/drm/panthor/panthor_mmu.c#L930
> 
> Call trace:
>   __arm_lpae_unmap+0x570/0x5c8 (P)
>   __arm_lpae_unmap+0x144/0x5c8
>   __arm_lpae_unmap+0x144/0x5c8
>   arm_lpae_unmap_pages+0xe8/0x120
>   panthor_vm_unmap_pages+0x1f0/0x3f8 [panthor]
>   panthor_vm_evict_bo_mappings_locked+0xf4/0x1d8 [panthor]
>   panthor_gem_try_evict+0x190/0x7c8 [panthor]
>   drm_gem_lru_scan+0x388/0x698
> 
> Following sequence leads to the kernel warnings.
> 
> - BO is mapped to GPU and is in GPU_MAPPED_PRIVATE state.
> 
> - Shrinker callback gets invoked and that BO is evicted. As a result BO 
> is moved to UNRECLAIMABLE state.
> 
> - Userspace accesses the evicted BO and panthor_gem_fault() gets the 
> backing pages again. BO is moved back to GPU_MAPPED_PRIVATE state (even 
> though technically the BO is still in evicted state, both vm_bo->evicted 
> and vma->evicted are TRUE.
> 
> - Shrinker callback is invoked again and tries to evict the same BO.
> 
> - panthor_vm_evict_bo_mappings_locked() calls panthor_vm_unmap_pages() 
> and the kernel warnings are emiited as PTEs are already invalid.

Yep, it looks like the other side of the problem pointed out by Steve:
CPU mappings can make the buffer reclaimable again, but those are still
evicted from the VM PoV.

> 
> 
> Made the following change locally to avoid the warning.
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c 
> b/drivers/gpu/drm/panthor/panthor_mmu.c
> index ffd821b3be46..e0a1dda1675f 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -2344,6 +2344,8 @@ int panthor_vm_evict_bo_mappings_locked(struct 
> panthor_gem_object *bo)
>                  drm_gpuvm_bo_for_each_va(va, vm_bo) {
>                          struct panthor_vma *vma = container_of(va, 
> struct panthor_vma, base);
> 
> +                       if (vma->evicted)
> +                               continue;
>                          panthor_vm_lock_region(vm, va->va.addr, 
> va->va.range);
>                          panthor_vm_unmap_pages(vm, va->va.addr, 
> va->va.range);
>                          panthor_vm_unlock_region(vm);
> 
> 
> 
> Do you think we can also update is_gpu_mapped() so that an evicted BO 
> moves to MMAPPED state, instead of GPU_MAPPED_PRIVATE state, on CPU 
> access ?.
> 
> Not sure if the following change makes sense.
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c 
> b/drivers/gpu/drm/panthor/panthor_gem.c
> index 6e91c5022d0d..8a8411fed195 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -125,6 +125,8 @@ static bool is_gpu_mapped(struct panthor_gem_object *bo,
>          struct drm_gpuvm_bo *vm_bo;
> 
>          drm_gem_for_each_gpuvm_bo(vm_bo, &bo->base) {
> +               if (vm_bo->evicted)
> +                       continue;
>                  if (!vm) {
>                          *state = PANTHOR_GEM_GPU_MAPPED_PRIVATE;
>                          vm = vm_bo->vm;
> 
> 
> Please let me know.

Yep, this looks correct. I'll add that to my list of fixups.

Thanks,

Boris