Re: [PATCH v2 08/08] drm/amdgpu: add vm root BO lock before accessing the vm

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 25.09.24 um 21:59 schrieb Arunpravin Paneer Selvam:
> Add a vm root BO lock before accessing the userqueue VM.
>
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index 43429661f62d..52722b738316 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -320,7 +320,6 @@ static const struct dma_fence_ops amdgpu_userq_fence_ops = {
>   /**
>    * amdgpu_userq_fence_read_wptr - Read the userq wptr value
>    *
> - * @filp: drm file private data structure
>    * @queue: user mode queue structure pointer
>    * @wptr: write pointer value
>    *
> @@ -330,23 +329,27 @@ static const struct dma_fence_ops amdgpu_userq_fence_ops = {
>    *
>    * Returns wptr value on success, error on failure.
>    */
> -static int amdgpu_userq_fence_read_wptr(struct drm_file *filp,
> -					struct amdgpu_usermode_queue *queue,
> +static int amdgpu_userq_fence_read_wptr(struct amdgpu_usermode_queue *queue,
>   					u64 *wptr)
>   {
> -	struct amdgpu_fpriv *fpriv = filp->driver_priv;
>   	struct amdgpu_bo_va_mapping *mapping;
> -	struct amdgpu_vm *vm = &fpriv->vm;
>   	struct amdgpu_bo *bo;
>   	u64 addr, *ptr;
>   	int r;
>   
> +	r = amdgpu_bo_reserve(queue->vm->root.bo, false);
> +	if (r)
> +		return r;
> +
>   	addr = queue->userq_prop->wptr_gpu_addr;
>   	addr &= AMDGPU_GMC_HOLE_MASK;
>   
> -	mapping = amdgpu_vm_bo_lookup_mapping(vm, addr >> PAGE_SHIFT);
> -	if (!mapping)
> +	mapping = amdgpu_vm_bo_lookup_mapping(queue->vm, addr >> PAGE_SHIFT);
> +	amdgpu_bo_unreserve(queue->vm->root.bo);

You need to keep the VM locked until you are done with the mapping. 
Otherwise the mapping could be released at any time.

Regards,
Christian.

> +	if (!mapping) {
> +		DRM_ERROR("Failed to lookup amdgpu_bo_va_mapping\n");
>   		return -EINVAL;
> +	}
>   
>   	bo = mapping->bo_va->base.bo;

If you only need the BO then grab a temporary BO reference here, drop 
the VM lock and acquire the BO.

When you are done with everything just drop the BO lock and then the 
temporary BO reference.

Regards,
Christian.

>   	r = amdgpu_bo_reserve(bo, true);
> @@ -448,7 +451,7 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>   		goto exec_fini;
>   	}
>   
> -	r = amdgpu_userq_fence_read_wptr(filp, queue, &wptr);
> +	r = amdgpu_userq_fence_read_wptr(queue, &wptr);
>   	if (r)
>   		goto exec_fini;
>