Re: [PATCH] drm/xe: Move VM dma-resv lock from xe_exec_queue_create to __xe_exec_queue_init

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Den 2024-07-24 kl. 17:28, skrev Matthew Brost:
> The critical section which requires the VM dma-resv is the call
> xe_lrc_create in __xe_exec_queue_init. Move this lock to
> __xe_exec_queue_init holding it just around xe_lrc_create. Not only is
> good practice, this also fixes a locking double of the VM dma-resv in
> the error paths of __xe_exec_queue_init as xe_lrc_put tries to acquire
> this too resulting in a deadlock.
> 
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matthw Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_exec_queue.c | 23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 69867a7b7c77..0d72846af9bf 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -105,22 +105,35 @@ static struct xe_exec_queue *__xe_exec_queue_alloc(struct xe_device *xe,
>   
>   static int __xe_exec_queue_init(struct xe_exec_queue *q)
>   {
> +	struct xe_vm *vm = q->vm;
>   	int i, err;
>   
> +	if (vm) {
> +		err = xe_vm_lock(vm, true);
> +		if (err)
> +			return err;
> +	}
> +
>   	for (i = 0; i < q->width; ++i) {
>   		q->lrc[i] = xe_lrc_create(q->hwe, q->vm, SZ_16K);
>   		if (IS_ERR(q->lrc[i])) {
>   			err = PTR_ERR(q->lrc[i]);
> -			goto err_lrc;
> +			goto err_unlock;
>   		}
>   	}
>   
> +	if (vm)
> +		xe_vm_unlock(vm);
> +
>   	err = q->ops->init(q);
>   	if (err)
>   		goto err_lrc;
>   
>   	return 0;
>   
> +err_unlock:
> +	if (vm)
> +		xe_vm_unlock(vm);
>   err_lrc:
>   	for (i = i - 1; i >= 0; --i)
>   		xe_lrc_put(q->lrc[i]);
> @@ -140,15 +153,7 @@ struct xe_exec_queue *xe_exec_queue_create(struct xe_device *xe, struct xe_vm *v
>   	if (IS_ERR(q))
>   		return q;
>   
> -	if (vm) {
> -		err = xe_vm_lock(vm, true);
> -		if (err)
> -			goto err_post_alloc;
> -	}
> -
>   	err = __xe_exec_queue_init(q);
> -	if (vm)
> -		xe_vm_unlock(vm);
>   	if (err)
>   		goto err_post_alloc;
>