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;
> }
>
> /**
