[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.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.

> +		/* 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));