[PATCH 2/2] drm/amdgpu: fix debug sdma vram access checks

[Christian König]

Am 18.01.22 um 00:43 schrieb Jonathan Kim:
> drm_dev_enter returns true when accessiable so correct the error check.
>
> Source VRAM buffer is reserved by TTM but not pinned so the gpu offset
> fetch throws a warning.

Well it throws a warning because what you try to do here won't work.

>    Get the gpu offset without a check and then
> double check to make sure the source buffer hasn't fallen into a hole.
> Otherwise use MMIO access to deal with non-contiguous VRAM cases as
> usual.
>
> Signed-off-by: Jonathan Kim <jonathan.kim at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 6178ae7ba624..0acfd872bfef 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1440,6 +1440,7 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>   	struct dma_fence *fence;
>   	uint64_t src_addr, dst_addr;
>   	unsigned int num_dw;
> +	bool vram_is_contiguous;
>   	int r, idx;
>   
>   	if (len != PAGE_SIZE)
> @@ -1448,9 +1449,8 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>   	if (!adev->mman.sdma_access_ptr)
>   		return -EACCES;
>   
> -	r = drm_dev_enter(adev_to_drm(adev), &idx);
> -	if (r)
> -		return r;
> +	if (!drm_dev_enter(adev_to_drm(adev), &idx))
> +		return -ENODEV;
>   
>   	if (write)
>   		memcpy(adev->mman.sdma_access_ptr, buf, len);
> @@ -1460,7 +1460,18 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>   	if (r)
>   		goto out;
>   
> -	src_addr = amdgpu_bo_gpu_offset(abo);
> +	src_addr = amdgpu_bo_gpu_offset_no_check(abo);
> +	vram_is_contiguous = (adev->gmc.real_vram_size - 1 ==
> +				adev->gmc.vram_end - adev->gmc.vram_start) &&
> +				src_addr >= adev->gmc.vram_start &&
> +				src_addr + len <= adev->gmc.vram_end;

That code is utterly nonsense. What you need to do is to use the 
iterator to get the real VRAM address.

The return value of amdgpu_bo_gpu_offset() and 
amdgpu_bo_gpu_offset_no_check() is only valid for pinned contiguous buffers.

Regards,
Christian.

> +
> +	if (!vram_is_contiguous) {
> +		amdgpu_job_free(job);
> +		r = -EACCES;
> +		goto out;
> +	}
> +
>   	dst_addr = amdgpu_bo_gpu_offset(adev->mman.sdma_access_bo);
>   	if (write)
>   		swap(src_addr, dst_addr);