Re: [PATCH v2 5/7] drm/xe: Move vma rebinding to the drm_exec locking loop

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Mar 22, 2024 at 10:02:11AM +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.
> 
> v2:
> - Remove a leftover call to xe_vm_rebind() (Matt Brost)
> - Add a helper function xe_vm_validate_rebind() (Matt Brost)
> 
> Fixes: 24f947d58fe5 ("drm/xe: Use DRM GPUVM helpers for external- and evicted objects")
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/xe_exec.c | 24 ++--------------
>  drivers/gpu/drm/xe/xe_vm.c   | 55 +++++++++++++++++++++++++++---------
>  drivers/gpu/drm/xe/xe_vm.h   |  3 ++
>  3 files changed, 47 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
> index 397a49b731f1..f442ef495235 100644
> --- a/drivers/gpu/drm/xe/xe_exec.c
> +++ b/drivers/gpu/drm/xe/xe_exec.c
> @@ -96,21 +96,9 @@
>  
>  static int xe_exec_fn(struct drm_gpuvm_exec *vm_exec)
>  {
> -	struct drm_gem_object *obj;
> -	unsigned long index;
> -	int ret;
> -
> -	ret = drm_gpuvm_validate(vm_exec->vm, &vm_exec->exec);
> -	if (ret)
> -		return ret;
> -
> -	drm_exec_for_each_locked_object(&vm_exec->exec, index, obj) {
> -		ret = dma_resv_reserve_fences(obj->resv, 1);
> -		if (ret)
> -			return ret;
> -	}
> +	struct xe_vm *vm = container_of(vm_exec->vm, struct xe_vm, gpuvm);
>  
> -	return 0;

Same comment as previous patch, maybe reorder these two / squash if you
think that is required + have a comment here.

> +	return xe_vm_validate_rebind(vm, &vm_exec->exec, 1);
>  }
>  
>  int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> @@ -264,14 +252,6 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  		goto err_exec;
>  	}
>  
> -	/*
> -	 * Rebind any invalidated userptr or evicted BOs in the VM, non-compute
> -	 * VM mode only.
> -	 */
> -	err = xe_vm_rebind(vm, false);
> -	if (err)
> -		goto err_put_job;
> -
>  	/* Wait behind rebinds */
>  	if (!xe_vm_in_lr_mode(vm)) {
>  		err = drm_sched_job_add_resv_dependencies(&job->drm,
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 6aefd6602310..b1d0df178a2d 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -482,11 +482,50 @@ static int xe_gpuvm_validate(struct drm_gpuvm_bo *vm_bo, struct drm_exec *exec)
>  	return 0;
>  }
>  
> -static int xe_preempt_work_begin(struct drm_exec *exec, struct xe_vm *vm,
> -				 bool *done)
> +/**
> + * xe_vm_validate_rebind() - Validate buffer objects and rebind vmas
> + * @vm: The vm for which we are rebinding.
> + * @exec: The struct drm_exec with the locked GEM objects.
> + * @num_fences: The number of fences to reserve for the operation, not
> + * including rebinds and validations.
> + *
> + * Validates all evicted gem objects and rebinds their vmas. Note that
> + * rebindings may cause evictions and hence the validation-rebind
> + * sequence is rerun until there are no more objects to validate.
> + *
> + * Return: 0 on success, negative error code on error. In particular,
> + * may return -EINTR or -ERESTARTSYS if interrupted, and -EDEADLK if
> + * the drm_exec transaction needs to be restarted.
> + */
> +int xe_vm_validate_rebind(struct xe_vm *vm, struct drm_exec *exec,
> +			  unsigned int num_fences)
>  {
>  	struct drm_gem_object *obj;
>  	unsigned long index;
> +	int ret;
> +
> +	do {
> +		ret = drm_gpuvm_validate(&vm->gpuvm, exec);
> +		if (ret)
> +			return ret;
> +
> +		ret = xe_vm_rebind(vm, false);
> +		if (ret)
> +			return ret;
> +	} while (!list_empty(&vm->gpuvm.evict.list));
> +
> +	drm_exec_for_each_locked_object(exec, index, obj) {
> +		ret = dma_resv_reserve_fences(obj->resv, num_fences);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int xe_preempt_work_begin(struct drm_exec *exec, struct xe_vm *vm,
> +				 bool *done)
> +{
>  	int err;
>  
>  	err = drm_gpuvm_prepare_vm(&vm->gpuvm, exec, 0);
> @@ -512,17 +551,7 @@ 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;
> -
> -	drm_exec_for_each_locked_object(exec, index, obj) {
> -		err = dma_resv_reserve_fences(obj->resv, vm->preempt.num_exec_queues);
> -		if (err)
> -			return err;
> -	}
> -
> -	return 0;

Same goes here wrt a comment, it wouldn't hurt to have one.

With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> +	return xe_vm_validate_rebind(vm, exec, vm->preempt.num_exec_queues);
>  }
>  
>  static void preempt_rebind_work_func(struct work_struct *w)
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 20009d8b4702..306cd0934a19 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -244,6 +244,9 @@ int xe_analyze_vm(struct drm_printer *p, struct xe_vm *vm, int gt_id);
>  
>  int xe_vm_lock_vma(struct drm_exec *exec, struct xe_vma *vma);
>  
> +int xe_vm_validate_rebind(struct xe_vm *vm, struct drm_exec *exec,
> +			  unsigned int num_fences);
> +
>  /**
>   * xe_vm_resv() - Return's the vm's reservation object
>   * @vm: The vm
> -- 
> 2.44.0
>