Re: [PATCH] drm/amdgpu: clear RB_OVERFLOW bit if detected when enabling interrupts

[Christian König]

Am 24.06.24 um 08:58 schrieb Danijel Slivka:
Why:
Setting IH_RB_WPTR register to 0 will not clear the RB_OVERFLOW bit
if RB_ENABLE is not set.
How to fix:
Set WPTR_OVERFLOW_CLEAR bit after RB_ENABLE bit is set.
The RB_ENABLE bit is required to be set, together with
WPTR_OVERFLOW_ENABLE bit so that setting WPTR_OVERFLOW_CLEAR bit
would clear the RB_OVERFLOW.

Signed-off-by: Danijel Slivka <danijel.sli...@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/ih_v6_0.c | 23 +++++++++++++++++++++++
  1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/ih_v6_0.c 
b/drivers/gpu/drm/amd/amdgpu/ih_v6_0.c
index 3cb64c8f7175..44872a8ce6a2 100644
--- a/drivers/gpu/drm/amd/amdgpu/ih_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/ih_v6_0.c
@@ -135,6 +135,29 @@ static int ih_v6_0_toggle_ring_interrupts(struct 
amdgpu_device *adev,
  
  	tmp = RREG32(ih_regs->ih_rb_cntl);
        tmp = REG_SET_FIELD(tmp, IH_RB_CNTL, RB_ENABLE, (enable ? 1 : 0));
+
+       if (enable && REG_GET_FIELD(RREG32_NO_KIQ(ih_regs->ih_rb_wptr), 
IH_RB_WPTR, RB_OVERFLOW)) {

Please completely drop that extra read of the WPTR and just always try 
to clear the overflow bit.

+               /* Clear RB_OVERFLOW bit if detected */
+               tmp = REG_SET_FIELD(tmp, IH_RB_CNTL, WPTR_OVERFLOW_CLEAR, 1);

I think we should have a sequence which writes 0, then 1 and then 0 again.

Apart from that looks good to me.

Regards,
Christian.

+               if (amdgpu_sriov_vf(adev) && 
amdgpu_sriov_reg_indirect_ih(adev)) {
+                       if (psp_reg_program(&adev->psp, ih_regs->psp_reg_id, 
tmp))
+                               return -ETIMEDOUT;
+               } else {
+                       WREG32_NO_KIQ(ih_regs->ih_rb_cntl, tmp);
+               }
+
+               /* Unset the CLEAR_OVERFLOW bit immediately so new overflows
+                * can be detected.
+                */
+               tmp = REG_SET_FIELD(tmp, IH_RB_CNTL, WPTR_OVERFLOW_CLEAR, 0);
+               if (amdgpu_sriov_vf(adev) && 
amdgpu_sriov_reg_indirect_ih(adev)) {
+                       if (psp_reg_program(&adev->psp, ih_regs->psp_reg_id, 
tmp))
+                               return -ETIMEDOUT;
+               } else {
+                       WREG32_NO_KIQ(ih_regs->ih_rb_cntl, tmp);
+               }
+       }
+
        /* enable_intr field is only valid in ring0 */
        if (ih == &adev->irq.ih)
                tmp = REG_SET_FIELD(tmp, IH_RB_CNTL, ENABLE_INTR, (enable ? 1 : 
0));