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

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

[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@amd.com>
---
 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);
+		}
+	}
+
 	/* 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));
-- 
2.34.1

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

[Christian König]

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@amd.com>
> ---
>   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@amd.com>

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