[AMD Official Use Only - AMD Internal Distribution Only]
>-----Original Message-----
>From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx>
>Sent: Monday, June 24, 2024 11:31 AM
>To: Slivka, Danijel <Danijel.Slivka@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
>Prica, Nikola <Nikola.Prica@xxxxxxx>
>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@xxxxxxx>
>> ---
>> 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
