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

[Slivka, Danijel]

[AMD Official Use Only - AMD Internal Distribution Only]

>-----Original Message-----
>From: Christian König <ckoenig.leichtzumerken at gmail.com>
>Sent: Monday, June 24, 2024 11:31 AM
>To: Slivka, Danijel <Danijel.Slivka at amd.com>; amd-gfx at lists.freedesktop.org;
>Prica, Nikola <Nikola.Prica at amd.com>
>Subject: Re: [PATCH] drm/amdgpu: clear RB_OVERFLOW bit if detected when
>enabling interrupts
>
>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.slivka at 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.

Done and changed the commit message accordingly. Sent out new patch.
>
>> +            /* 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.
>

Done.

>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));

BR,
Danijel Slivka