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

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

On Tue, 24 Mar 2026 23:52:32 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> > @@ -2178,8 +2222,10 @@ static int panthor_gpuva_sm_step_remap(struct drm_gpuva_op *op,
> >  	 * atomicity. panthor_vm_lock_region() bails out early if the new region
> >  	 * is already part of the locked region, so no need to do this check here.
> >  	 */
> > -	panthor_vm_lock_region(vm, unmap_start, unmap_range);
> > -	panthor_vm_unmap_pages(vm, unmap_start, unmap_range);
> > +	if (!unmap_vma->evicted) {
> > +		panthor_vm_lock_region(vm, unmap_start, unmap_range);
> > +		panthor_vm_unmap_pages(vm, unmap_start, unmap_range);
> > +	}
> >
> >  	if (op->remap.prev) {  
> 
> I think we'd want to make sure we don't map the pages for 'prev' when the original unmap
> vma is evicted, but we still want to create a new vma for it.

Absolutely. I'll fix that in v6.

> 
> >  		struct panthor_gem_object *bo = to_panthor_bo(op->remap.prev->gem.obj);
> > @@ -2193,6 +2239,7 @@ static int panthor_gpuva_sm_step_remap(struct drm_gpuva_op *op,
> >
> >  		prev_vma = panthor_vm_op_ctx_get_vma(op_ctx);
> >  		panthor_vma_init(prev_vma, unmap_vma->flags);
> > +		prev_vma->evicted = unmap_vma->evicted;
> >  	}
> >
> >  	if (op->remap.next) {  
> 
> Same as above.

Yep, will be fixed in v6.

> > +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) {
> > +		if (vm_bo->evicted)
> > +			continue;  
> 
> I found this a bit confusing, because this function is called for objects whose state is
> PANTHOR_GEM_GPU_MAPPED_PRIVATE, described as 'GEM is GPU mapped to only one VM'. So that
> made me think it was synonymous to panthor_gem_object::exclusive_vm_root_gem being set, but
> according to this code, a BO is shrinker-private if only one of its VM_BO's isn't evicted
> at any given time. I guess this definition is what matters wrt the shrinker, because it can
> go straight pick it up from the VM's reclaim.lru list. Maybe it'd be less confusing if
> the uAPI header file's description of PANTHOR_GEM_GPU_MAPPED_PRIVATE reflected this too?

I'll rename PANTHOR_GEM_GPU_MAPPED_PRIVATE into
PANTHOR_GEM_GPU_MAPPED_SINGLE_VM and PANTHOR_GEM_GPU_MAPPED_SHARED into
PANTHOR_GEM_GPU_MAPPED_MULTI_VM to clear the confusion.

> 
> > +		/* We're only supposed to have one non-evicted 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);
> > +	}
> > +}