Re: [PATCH 1/2] drm/amdgpu: Unmap BO memory before calling amdgpu_bo_unref()

["Christian König" <christian.koenig@amd.com>]

Am 20.06.24 um 16:44 schrieb Thomas Zimmermann:
> Prepares for using ttm_bo_vmap() and ttm_bo_vunmap() in amdgpu. Both
> require the caller to hold the GEM reservation lock, which is not the
> case while releasing a buffer object. Hence, push a possible call to
> unmap out from the buffer-object release code. Warn if a buffer object
> with mapped pages is supposed to be released.

Yeah, I've looked into this a while ago as well and that unfortunately 
won't work like this.

Amdgpu also uses ttm_bo_kmap() on user allocations, so the 
amdgpu_bo_kunmap() in amdgpu_bo_destroy() is a must have.

On the other hand I'm pretty sure that calling ttm_bo_vunmap() without 
holding the reservation lock is ok in this situation.

After all it's guaranteed that nobody else is having a reference to the 
BO any more.

Regards,
Christian.

>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index a1b7438c43dc8..d58b11ea0ead5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -58,7 +58,12 @@ static void amdgpu_bo_destroy(struct ttm_buffer_object *tbo)
>   {
>   	struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo);
>   
> -	amdgpu_bo_kunmap(bo);
> +	/*
> +	 * BO memory pages should be unmapped at this point. Call
> +	 * amdgpu_bo_kunmap() before releasing the BO.
> +	 */
> +	if (drm_WARN_ON_ONCE(bo->tbo.base.dev, bo->kmap.bo))
> +		amdgpu_bo_kunmap(bo);
>   
>   	if (bo->tbo.base.import_attach)
>   		drm_prime_gem_destroy(&bo->tbo.base, bo->tbo.sg);
> @@ -450,9 +455,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
>   	WARN_ON(amdgpu_ttm_adev((*bo)->tbo.bdev)->in_suspend);
>   
>   	if (likely(amdgpu_bo_reserve(*bo, true) == 0)) {
> -		if (cpu_addr)
> -			amdgpu_bo_kunmap(*bo);
> -
> +		amdgpu_bo_kunmap(*bo);
>   		amdgpu_bo_unpin(*bo);
>   		amdgpu_bo_unreserve(*bo);
>   	}