Re: [CI 4/7] drm/xe: Move vma rebinding to the drm_exec locking loop

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi, Matt,

apparently I forget to remove the old series of patches before hitting
git-send-email so the whole series got mixed. :(, but please see below.


On Thu, 2024-03-21 at 19:21 +0000, Matthew Brost wrote:
> On Tue, Mar 12, 2024 at 03:04:44PM +0100, Thomas Hellström wrote:
> > Rebinding might allocate page-table bos, causing evictions.
> > To support blocking locking during these evictions,
> > perform the rebinding in the drm_exec locking loop.
> > 
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_exec.c | 13 ++++++++++---
> >  drivers/gpu/drm/xe/xe_vm.c   | 16 +++++++++-------
> >  2 files changed, 19 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_exec.c
> > b/drivers/gpu/drm/xe/xe_exec.c
> > index 13d1fba2c9d7..f36fb22a7861 100644
> > --- a/drivers/gpu/drm/xe/xe_exec.c
> > +++ b/drivers/gpu/drm/xe/xe_exec.c
> > @@ -96,13 +96,20 @@
> >  
> >  static int xe_exec_fn(struct drm_gpuvm_exec *vm_exec)
> >  {
> > +	struct xe_vm *vm = container_of(vm_exec->vm, struct xe_vm,
> > gpuvm);
> >  	struct drm_gem_object *obj;
> >  	unsigned long index;
> >  	int ret;
> >  
> > -	ret = drm_gpuvm_validate(vm_exec->vm, &vm_exec->exec);
> > -	if (ret)
> > -		return ret;
> > +	do {
> > +		ret = drm_gpuvm_validate(vm_exec->vm, &vm_exec-
> > >exec);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = xe_vm_rebind(vm, false);
> > +		if (ret)
> > +			return ret;
> > +	} while (!list_empty(&vm->gpuvm.evict.list));
> >  
> 
> You need to delete xe_vm_rebind from xe_exec_ioctl too.

Good catch. Thanks, I'll do that.


> 
> >  	drm_exec_for_each_locked_object(&vm_exec->exec, index,
> > obj) {
> >  		ret = dma_resv_reserve_fences(obj->resv, 1);
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > b/drivers/gpu/drm/xe/xe_vm.c
> > index faad7f52b3ca..fad5b1ae7f2d 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -513,9 +513,15 @@ static int xe_preempt_work_begin(struct
> > drm_exec *exec, struct xe_vm *vm,
> >  	if (err)
> >  		return err;
> >  
> > -	err = drm_gpuvm_validate(&vm->gpuvm, exec);
> > -	if (err)
> > -		return err;
> > +	do {
> > +		err = drm_gpuvm_validate(&vm->gpuvm, exec);
> > +		if (err)
> > +			return err;
> > +
> > +		err = xe_vm_rebind(vm, true);
> > +		if (err)
> > +			return err;
> > +	} while (!list_empty(&vm->gpuvm.evict.list));
> 
> Same code pattern twice here? Opportunity for a helper?

I'll take a look.

> 
> Matt
> 
> >  
> >  	drm_exec_for_each_locked_object(exec, index, obj) {
> >  		err = dma_resv_reserve_fences(obj->resv, vm-
> > >preempt.num_exec_queues);
> > @@ -575,10 +581,6 @@ static void preempt_rebind_work_func(struct
> > work_struct *w)
> >  	if (err)
> >  		goto out_unlock;
> >  
> > -	err = xe_vm_rebind(vm, true);
> > -	if (err)
> > -		goto out_unlock;
> > -
> >  	if (vm->rebind_fence) {
> >  		dma_fence_wait(vm->rebind_fence, false);
> >  		dma_fence_put(vm->rebind_fence);
> > -- 
> > 2.44.0
> >