[PATCH] drm/amdgpu: right shift 2 bits for SDMA_GFX_RB_WPTR_POLL_ADDR_LO v2

[Ding, Pixel]

Hi Alex,

We found that this statement introduces issue.

In sdma_v3_0_ring_set_wptr():
WRITE_ONCE(*wb, (lower_32_bits(ring->wptr) << 2));

When changing it to normal assignment operation, issue is gone, also if adding some dumps here to make it slower, issue is gone…

Any idea? Is there some reason to use this macro?

---------------
Sincerely Yours,
Pixel







On 25/09/2017, 3:12 PM, "Ding, Pixel" <Pixel.Ding at amd.com> wrote:

>Yes, it seems not related to the seen issue. The previous change simplifies the shift operations while the logic is same. Please ignore.
>— 
>Sincerely Yours,
>Pixel
>
>
>
>
>
>
>
>
>On 25/09/2017, 3:08 PM, "Christian König" <ckoenig.leichtzumerken at gmail.com> wrote:
>
>>NAK, that doesn't looks correct to me.
>>
>>> Both Tonga and Vega register SPECs indicate that this registers only
>>> use 31:2 bits in DW.
>>This means that the value must be DW aligned and NOT that it needs to be 
>>shifted by 2!
>>
>>Regards,
>>Christian.
>>
>>Am 25.09.2017 um 08:38 schrieb Pixel Ding:
>>> Both Tonga and Vega register SPECs indicate that this registers only
>>> use 31:2 bits in DW. SRIOV test case immediately fails withtout this
>>> shift.
>>>
>>> v2: write to ADDR field
>>>
>>> Signed-off-by: Pixel Ding <Pixel.Ding at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c | 9 +++++----
>>>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 8 +++++---
>>>   2 files changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
>>> index 72f31cc..8b83b96 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
>>> @@ -643,7 +643,7 @@ static void sdma_v3_0_enable(struct amdgpu_device *adev, bool enable)
>>>   static int sdma_v3_0_gfx_resume(struct amdgpu_device *adev)
>>>   {
>>>   	struct amdgpu_ring *ring;
>>> -	u32 rb_cntl, ib_cntl, wptr_poll_cntl;
>>> +	u32 rb_cntl, ib_cntl, wptr_poll_cntl, wptr_poll_addr_lo;
>>>   	u32 rb_bufsz;
>>>   	u32 wb_offset;
>>>   	u32 doorbell;
>>> @@ -712,9 +712,10 @@ static int sdma_v3_0_gfx_resume(struct amdgpu_device *adev)
>>>   
>>>   		/* setup the wptr shadow polling */
>>>   		wptr_gpu_addr = adev->wb.gpu_addr + (ring->wptr_offs * 4);
>>> -
>>> -		WREG32(mmSDMA0_GFX_RB_WPTR_POLL_ADDR_LO + sdma_offsets[i],
>>> -		       lower_32_bits(wptr_gpu_addr));
>>> +		wptr_poll_addr_lo = RREG32(mmSDMA0_GFX_RB_WPTR_POLL_ADDR_LO + sdma_offsets[i]);
>>> +		wptr_poll_addr_lo = REG_SET_FIELD(wptr_poll_addr_lo, SDMA0_GFX_RB_WPTR_POLL_ADDR_LO,
>>> +						  ADDR, lower_32_bits(wptr_gpu_addr) >> 2);
>>> +		WREG32(mmSDMA0_GFX_RB_WPTR_POLL_ADDR_LO + sdma_offsets[i], wptr_poll_addr_lo;
>>>   		WREG32(mmSDMA0_GFX_RB_WPTR_POLL_ADDR_HI + sdma_offsets[i],
>>>   		       upper_32_bits(wptr_gpu_addr));
>>>   		wptr_poll_cntl = RREG32(mmSDMA0_GFX_RB_WPTR_POLL_CNTL + sdma_offsets[i]);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>>> index c26d205..8b8338d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>>> @@ -574,7 +574,7 @@ static void sdma_v4_0_enable(struct amdgpu_device *adev, bool enable)
>>>   static int sdma_v4_0_gfx_resume(struct amdgpu_device *adev)
>>>   {
>>>   	struct amdgpu_ring *ring;
>>> -	u32 rb_cntl, ib_cntl, wptr_poll_cntl;
>>> +	u32 rb_cntl, ib_cntl, wptr_poll_cntl, wptr_poll_addr_lo
>>>   	u32 rb_bufsz;
>>>   	u32 wb_offset;
>>>   	u32 doorbell;
>>> @@ -664,8 +664,10 @@ static int sdma_v4_0_gfx_resume(struct amdgpu_device *adev)
>>>   
>>>   		/* setup the wptr shadow polling */
>>>   		wptr_gpu_addr = adev->wb.gpu_addr + (ring->wptr_offs * 4);
>>> -		WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_GFX_RB_WPTR_POLL_ADDR_LO),
>>> -		       lower_32_bits(wptr_gpu_addr));
>>> +		wptr_poll_addr_lo = RREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_GFX_RB_WPTR_POLL_ADDR_LO));
>>> +		wptr_poll_addr_lo = REG_SET_FIELD(wptr_poll_addr_lo, SDMA0_GFX_RB_WPTR_POLL_ADDR_LO,
>>> +						  ADDR, lower_32_bits(wptr_gpu_addr) >> 2);
>>> +		WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_GFX_RB_WPTR_POLL_ADDR_LO), wptr_poll_addr_lo);
>>>   		WREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_GFX_RB_WPTR_POLL_ADDR_HI),
>>>   		       upper_32_bits(wptr_gpu_addr));
>>>   		wptr_poll_cntl = RREG32(sdma_v4_0_get_reg_offset(i, mmSDMA0_GFX_RB_WPTR_POLL_CNTL));
>>
>>