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

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 24.06.24 um 12:06 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 | 34 ++++++++++++++++++++++++++++
>   1 file changed, 34 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..cbc70016f479 100644
> --- a/drivers/gpu/drm/amd/amdgpu/ih_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/ih_v6_0.c
> @@ -135,6 +135,40 @@ 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) {
> +		/* Unset the CLEAR_OVERFLOW bit to make sure the next step
> +		 * is switching the bit from 0 to 1
> +		 */
> +		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);
> +		}
> +
> +		/* Clear RB_OVERFLOW bit */
> +		tmp = REG_SET_FIELD(tmp, IH_RB_CNTL, WPTR_OVERFLOW_CLEAR, 1);
> +		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);
> +		}

The last write can be merged with the one below to eventually enable the 
interrupt. So this chunk here can be dropped.

With that fixed feel free to add Reviewed-by: Christian König 
<christian.koenig@xxxxxxx>

Regards,
Christian.

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