Yeah, agreed -----Original Message----- From: Christian König [mailto:[email protected]] Sent: 2018年2月26日 18:22 To: Liu, Monk <[email protected]>; [email protected] Cc: Deng, Emily <[email protected]> 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 <[email protected]> > > the original method will change the wptr value in wb. > > Change-Id: I984fabca35d9dcf1f5fa8ef7779b2afb7f7d7370 > Signed-off-by: Emily Deng <[email protected]> > --- > 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; > } > > /** _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
