On Tue, 24 Mar 2026 23:52:32 +0000
Adrián Larumbe <[email protected]> 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);
> > + }
> > +}
