[PATCH 4/4] drm/xe: destroy userptr vma on UNMAP event

[Zeng, Oak]

Hi Thomas,

> -----Original Message-----
> From: Hellstrom, Thomas <thomas.hellstrom at intel.com>
> Sent: Thursday, March 21, 2024 10:30 AM
> To: intel-xe at lists.freedesktop.org; Zeng, Oak <oak.zeng at intel.com>
> Cc: Brost, Matthew <matthew.brost at intel.com>; Welty, Brian
> <brian.welty at intel.com>; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray at intel.com>; De Marchi, Lucas
> <lucas.demarchi at intel.com>
> Subject: Re: [PATCH 4/4] drm/xe: destroy userptr vma on UNMAP event
> 
> On Wed, 2024-03-20 at 22:29 -0400, Oak Zeng wrote:
> > When there is MMU_NOTIFY_UNMAP event happens, the userptr
> > is munmapped from CPU. There is no need to keep the xe_vma
> > for this userptr from GPU side. So we destroy it.
> >
> > But we can't destroy vma directly from the mmu notifier
> > callback function, because we need to remove mmu
> > notifier during vma destroy. If we remove mmu notifier
> > directly from mmu notifier callback, it is a deadlock.
> > xe_vma_destroy is modified to destroy vma in a worker
> > thread.
> >
> > Another reason of this change is, for the future
> > hmmptr codes, we destroy vma when hmmptr is unmapped
> > from CPU. We want to unify the hmmptr and userptr
> > code.
> 
> I understand the plan that for the 1:1 approach of
> vma <-> pt_state, this would mark the pt_state invalid, but the xe_vma
> remaining, possibly merge it with neightboring invalid xe_vmas?

Yes, we need to destroy the userptr/populated vma, and recreate another system_alloator/unpopulated/invalid vma - during this process, it will merge with the neighbor invalid system allocator vma...

> 
> For 1:N this would only invalidate certain pt_state chunks.
> 
> >
> > I believe this is also the correct behavior for userptr.
> > This patch is experimental for CI and open to discuss
> 
> With the recent "invalid userptr" discussion we had with compute UMD,
> they requested that if a new cpu_mm vma was added, the userptr would be
> functional again. 

Do you mean the userptr would be functional again *without* another vm_bind? If yes,  This seems strange behavior to me... I wonder how does UMD know when to perform a vm_bind and when not? It seems to me this behavior complicate both umd and kmd. It is against some basic design principle as I said in a previous reply to Matt's email.

I wrote an email to confirm with UMD. Let's see what they have to say.

Oak

Nobody actually tested that that's the case with the
> current code, but if so, this would break that behaviour. IMO the
> current behavior should be kept. xe_vma remains but is marked invalid.
> 
> 
> >
> > Signed-off-by: Oak Zeng <oak.zeng at intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_vm.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 11a4bb9d5415..90d1163c1090 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -40,6 +40,8 @@
> >  #include "xe_wa.h"
> >  #include "xe_hmm.h"
> >
> > +static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence
> > *fence);
> > +
> >  static struct drm_gem_object *xe_vm_obj(struct xe_vm *vm)
> >  {
> >  	return vm->gpuvm.r_obj;
> > @@ -604,6 +606,9 @@ static bool vma_userptr_invalidate(struct
> > mmu_interval_notifier *mni,
> >
> >  	trace_xe_vma_userptr_invalidate_complete(vma);
> >
> > +	if (range->event == MMU_NOTIFY_UNMAP)
> > +		xe_vma_destroy(vma, NULL);
> > +
> >  	return true;
> >  }
> >
> > @@ -901,7 +906,8 @@ static void xe_vma_destroy(struct xe_vma *vma,
> > struct dma_fence *fence)
> >  			xe_vma_destroy_late(vma);
> >  		}
> >  	} else {
> > -		xe_vma_destroy_late(vma);
> > +		INIT_WORK(&vma->destroy_work,
> > vma_destroy_work_func);
> > +		queue_work(system_unbound_wq, &vma->destroy_work);
> >  	}
> >  }
> >