Re: [PATCH] drm/amdgpu: cleanup ttm debug sdma vram access function

[Christian König <christian.koenig@xxxxxxx>]

Am 12.01.22 um 16:59 schrieb Jonathan Kim:
> Some suggested cleanups to declutter ttm when doing debug VRAM access over
> SDMA.
>
> Signed-off-by: Jonathan Kim <jonathan.kim@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  9 +++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 23 +++++++----------------
>   2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index a675dde81ce0..4d77842f2183 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1448,6 +1448,15 @@ int amdgpu_device_set_cg_state(struct amdgpu_device *adev,
>   int amdgpu_device_set_pg_state(struct amdgpu_device *adev,
>   			       enum amd_powergating_state state);
>   
> +static inline bool amdgpu_allow_post_mortem_debug(struct amdgpu_device *adev)

Give that a better name, something like 
amdgpu_device_are_timeouts_enabled().

Apart from that looks good to me,
Christian.

> +{
> +	return amdgpu_gpu_recovery == 0 ||
> +		adev->gfx_timeout == MAX_SCHEDULE_TIMEOUT ||
> +		adev->compute_timeout == MAX_SCHEDULE_TIMEOUT ||
> +		adev->sdma_timeout == MAX_SCHEDULE_TIMEOUT ||
> +		adev->video_timeout == MAX_SCHEDULE_TIMEOUT;
> +}
> +
>   #include "amdgpu_object.h"
>   
>   static inline bool amdgpu_is_tmz(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 33781509838c..02515f1ea5fa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1460,10 +1460,11 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>   	if (r)
>   		goto out;
>   
> -	src_addr = write ? amdgpu_bo_gpu_offset(adev->mman.sdma_access_bo) :
> -			amdgpu_bo_gpu_offset(abo);
> -	dst_addr = write ? amdgpu_bo_gpu_offset(abo) :
> -			amdgpu_bo_gpu_offset(adev->mman.sdma_access_bo);
> +	src_addr = amdgpu_bo_gpu_offset(abo);
> +	dst_addr = amdgpu_bo_gpu_offset(adev->mman.sdma_access_bo);
> +	if (write)
> +		swap(src_addr, dst_addr);
> +
>   	amdgpu_emit_copy_buffer(adev, &job->ibs[0], src_addr, dst_addr, PAGE_SIZE, false);
>   
>   	amdgpu_ring_pad_ib(adev->mman.buffer_funcs_ring, &job->ibs[0]);
> @@ -1486,15 +1487,6 @@ static int amdgpu_ttm_access_memory_sdma(struct ttm_buffer_object *bo,
>   	return r;
>   }
>   
> -static inline bool amdgpu_ttm_allow_post_mortem_debug(struct amdgpu_device *adev)
> -{
> -	return amdgpu_gpu_recovery == 0 ||
> -		adev->gfx_timeout == MAX_SCHEDULE_TIMEOUT ||
> -		adev->compute_timeout == MAX_SCHEDULE_TIMEOUT ||
> -		adev->sdma_timeout == MAX_SCHEDULE_TIMEOUT ||
> -		adev->video_timeout == MAX_SCHEDULE_TIMEOUT;
> -}
> -
>   /**
>    * amdgpu_ttm_access_memory - Read or Write memory that backs a buffer object.
>    *
> @@ -1519,7 +1511,7 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo,
>   	if (bo->resource->mem_type != TTM_PL_VRAM)
>   		return -EIO;
>   
> -	if (!amdgpu_ttm_allow_post_mortem_debug(adev) &&
> +	if (!amdgpu_allow_post_mortem_debug(adev) &&
>   			!amdgpu_ttm_access_memory_sdma(bo, offset, buf, len, write))
>   		return len;
>   
> @@ -1909,8 +1901,7 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev)
>   	ttm_range_man_fini(&adev->mman.bdev, AMDGPU_PL_OA);
>   	ttm_device_fini(&adev->mman.bdev);
>   	adev->mman.initialized = false;
> -	if (adev->mman.sdma_access_ptr)
> -		amdgpu_bo_free_kernel(&adev->mman.sdma_access_bo, NULL,
> +	amdgpu_bo_free_kernel(&adev->mman.sdma_access_bo, NULL,
>   					&adev->mman.sdma_access_ptr);
>   	DRM_INFO("amdgpu: ttm finalized\n");
>   }