[PATCH 18/22] drm/amdgpu: Correct sdma_v4 get_wptr

[Monk.Liu@xxxxxxx (Liu, Monk)]

Yeah, agreed 

-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken@xxxxxxxxx] 
Sent: 2018å¹´2æ??26æ?¥ 18:22
To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Deng, Emily <Emily.Deng at amd.com>
Subject: Re: [PATCH 18/22] drm/amdgpu: Correct sdma_v4 get_wptr

Am 26.02.2018 um 06:18 schrieb Monk Liu:
> From: Emily Deng <Emily.Deng at amd.com>
>
> the original method will change the wptr value in wb.
>
> Change-Id: I984fabca35d9dcf1f5fa8ef7779b2afb7f7d7370
> Signed-off-by: Emily Deng <Emily.Deng at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c 
> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 3d5385d..8c6e408 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -240,13 +240,14 @@ static uint64_t sdma_v4_0_ring_get_wptr(struct amdgpu_ring *ring)
>   	struct amdgpu_device *adev = ring->adev;
>   	u64 *wptr = NULL;
>   	uint64_t local_wptr = 0;
> -
> +	u64 tmp = 0;

Please keep an empty line between deceleration and code.

>   	if (ring->use_doorbell) {
>   		/* XXX check if swapping is necessary on BE */
>   		wptr = ((u64 *)&adev->wb.wb[ring->wptr_offs]);
>   		DRM_DEBUG("wptr/doorbell before shift == 0x%016llx\n", *wptr);
> -		*wptr = (*wptr) >> 2;
> -		DRM_DEBUG("wptr/doorbell after shift == 0x%016llx\n", *wptr);
> +		tmp = (*wptr) >> 2;
> +		DRM_DEBUG("wptr/doorbell after shift == 0x%016llx\n", tmp);
> +		return tmp;

Well I completely agree that this code is absolutely nonsense and could result in complete random hardware behavior.

But I think we need further cleanup than what you already did. How about the following instead?

> static uint64_t sdma_v4_0_ring_get_wptr(struct amdgpu_ring *ring) {
> Â Â Â Â Â Â Â  struct amdgpu_device *adev = ring->adev;
> Â Â Â Â Â Â Â  uint64_t wptr;
>
> Â Â Â Â Â Â Â  if (ring->use_doorbell) {
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  /* XXX check if swapping is necessary on BE */
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  wptr = READ_ONCE(*((u64 
> *)&adev->wb.wb[ring->wptr_offs]));
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  DRM_DEBUG("wptr/doorbell == 0x%016llx\n", *wptr);
> Â Â Â Â Â Â Â  } else {
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  int me = (ring == &adev->sdma.instance[0].ring) ? 0 : 
> 1;
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  u32 lowbit, highbit;
>
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  lowbit = RREG32(sdma_v4_0_get_reg_offset(adev, me, 
> mmSDMA0_GFX_RB_WPTR));
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  highbit = RREG32(sdma_v4_0_get_reg_offset(adev, me, 
> mmSDMA0_GFX_RB_WPTR_HI));
>
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  DRM_DEBUG("wptr [%i]high== 0x%08x low==0x%08x\n",
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  me, highbit, lowbit);
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  wptr = highbit;
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  wptr = wptr << 32;
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  wptr |= lowbit;
> Â Â Â Â Â Â Â  }
>
> Â Â Â Â Â Â Â  return wptr >> 2;
> }

Good catch,
Christian.

>   	} else {
>   		u32 lowbit, highbit;
>   		int me = (ring == &adev->sdma.instance[0].ring) ? 0 : 1; @@ -260,9 
> +261,8 @@ static uint64_t sdma_v4_0_ring_get_wptr(struct amdgpu_ring *ring)
>   		*wptr = highbit;
>   		*wptr = (*wptr) << 32;
>   		*wptr |= lowbit;
> +		return *wptr;
>   	}
> -
> -	return *wptr;
>   }
>   
>   /**