drm/rockchip: try to fix vblank hang resulting from iommu irq change

From: Sandy Huang <[email protected]>

The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled, but the vop irq handler gets called.

But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case. For this we can simply check the power-domain state of the vop, similar to how the iommu driver does it.

So remove the enable/disable handling and add appropriate condition to the irq handler.

changes in v2:
- move to just check the power-domain state
- add clock handling

Signed-off-by Sandy Huang <[email protected]>

[add commit message, moved to pm_runtime_get_if_in_use]

Signed-off-by Heiko Stuebner <[email protected]>

---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++-------
 1 file changed, 19 insertions(+), 9 deletions(-)

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -4

index b55156b8ba3b..615a5b44bfe9 100644
@@ -573,8 +573,6 @@ static int vop_enable(struct drm_crtc *crtc)
 
 	spin_unlock(&vop->reg_lock);
 
-	enable_irq(vop->irq);
-
 	drm_crtc_vblank_on(crtc);
 
 	return 0;

@@ -618,8 +616,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
 
 	vop_dsp_hold_valid_irq_disable(vop);
 
-	disable_irq(vop->irq);
-
 	vop->is_enabled = false;
 
 	/*

@@ -1195,6 +1191,16 @@ static irqreturn_t vop_isr(int irq, void *data)
 	uint32_t active_irqs;

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +11 -2

+		goto out;
+
 	/*
 	 * interrupt register has interrupt status, enable and clear bits, we
 	 * must hold irq_lock to avoid a race with enable/disable_vblank().

@@ -1209,8 +1215,11 @@ static irqreturn_t vop_isr(int irq, void *data)
 	spin_unlock(&vop->irq_lock);
 
 	/* This is expected for vop iommu irqs, since the irq is shared */
-	if (!active_irqs)
-		return IRQ_NONE;
+	if (!active_irqs) {
+		ret = IRQ_NONE;
+		vop_core_clks_disable(vop);
+		goto out;
+	}
 
 	if (active_irqs & DSP_HOLD_VALID_INTR) {
 		complete(&vop->dsp_hold_completion);

@@ -1236,6 +1245,10 @@ static irqreturn_t vop_isr(int irq, void *data)
 		DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
 			      active_irqs);
 
+	vop_core_clks_disable(vop);
+
+out:
+	pm_runtime_put(vop->dev);
 	return ret;
 }
 

From: Sandy Huang <[email protected]>

The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled, but the vop irq handler gets called.

But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case. For this we can simply check the power-domain state of the vop, similar to how the iommu driver does it.

So remove the enable/disable handling and add appropriate condition to the irq handler.

changes in v2:
- move to just check the power-domain state
- add clock handling

Signed-off-by Sandy Huang <[email protected]>

[add commit message, moved to pm_runtime_get_if_in_use]

Signed-off-by Heiko Stuebner <[email protected]>

---
  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++-------
  1 file changed, 19 insertions(+), 9 deletions(-)

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -4

index b55156b8ba3b..615a5b44bfe9 100644
@@ -573,8 +573,6 @@ static int vop_enable(struct drm_crtc *crtc)
 
  	spin_unlock(&vop->reg_lock);
 
-	enable_irq(vop->irq);
-
  	drm_crtc_vblank_on(crtc);
 
  	return 0;

@@ -618,8 +616,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
 
  	vop_dsp_hold_valid_irq_disable(vop);
 
-	disable_irq(vop->irq);
-
  	vop->is_enabled = false;
 
  	/*

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +16 -2

+	 */
+	if (!pm_runtime_get_if_in_use(vop->dev))
+		return IRQ_NONE;
+
+	if (WARN_ON(vop_core_clks_enable(vop)))
+		goto out;
+
  	/*
  	 * interrupt register has interrupt status, enable and clear bits, we
  	 * must hold irq_lock to avoid a race with enable/disable_vblank().

@@ -1209,8 +1215,11 @@ static irqreturn_t vop_isr(int irq, void *data)
  	spin_unlock(&vop->irq_lock);
 
  	/* This is expected for vop iommu irqs, since the irq is shared */
-	if (!active_irqs)
-		return IRQ_NONE;
+	if (!active_irqs) {
+		ret = IRQ_NONE;
+		vop_core_clks_disable(vop);
+		goto out;
+	}
 
  	if (active_irqs & DSP_HOLD_VALID_INTR) {
  		complete(&vop->dsp_hold_completion);

@@ -1236,6 +1245,10 @@ static irqreturn_t vop_isr(int irq, void *data)
  		DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
  			      active_irqs);
 
+	vop_core_clks_disable(vop);
+
+out:
+	pm_runtime_put(vop->dev);
  	return ret;
  }
 

Quoted 55 lines

From: Sandy Huang <[email protected]>

The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled, but the vop irq handler gets called.

But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case. For this we can simply check the power-domain state of the vop, similar to how the iommu driver does it.

So remove the enable/disable handling and add appropriate condition to the irq handler.

changes in v2:
- move to just check the power-domain state
- add clock handling

Signed-off-by Sandy Huang <[email protected]>

[add commit message, moved to pm_runtime_get_if_in_use]

Signed-off-by Heiko Stuebner <[email protected]>

---

  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++-------
  1 file changed, 19 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index b55156b8ba3b..615a5b44bfe9 100644

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -2

@@ -573,8 +573,6 @@ static int vop_enable(struct drm_crtc *crtc)
 
  	spin_unlock(&vop->reg_lock);
 
-	enable_irq(vop->irq);
-
 
  	drm_crtc_vblank_on(crtc);
 

return 0;

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -2

@@ -618,8 +616,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc
 *crtc,> 
  	vop_dsp_hold_valid_irq_disable(vop);
 
-	disable_irq(vop->irq);
-
 
  	vop->is_enabled = false;
 

/*

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static irqreturn_t vop_isr(int irq, void *data) · line 1197

Quoted 7 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +3 -0

@@ -1195,6 +1191,16 @@ static irqreturn_t vop_isr(int irq, void *data)
 
  	uint32_t active_irqs;
  	int ret = IRQ_NONE;
 
+	/*
+	 * The irq is shared with the iommu. If the power-domain is off
+	 * the irq has to be targetted at the iommu.

Hmm, aren't the IOMMUs in the same power domain as their respective master, though? I would naively assume so, and it does look that way from the DTs in the BSP kernel.

AFAICS the IOMMU usage count should never be greater than the VOP usage count (except before the VOP driver has probed, but I don't think that matters), so although this looks like a sensible change in general I can't help be a little bit puzzled at how and why the flow works.

Quoted 52 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +7 -0

+	 */
+	if (!pm_runtime_get_if_in_use(vop->dev))
+		return IRQ_NONE;
+
+	if (WARN_ON(vop_core_clks_enable(vop)))
+		goto out;
+
 
  	/*

* interrupt register has interrupt status, enable and clear bits, we
* must hold irq_lock to avoid a race with enable/disable_vblank().

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +5 -2

@@ -1209,8 +1215,11 @@ static irqreturn_t vop_isr(int irq, void *data)
 
  	spin_unlock(&vop->irq_lock);
 
  	/* This is expected for vop iommu irqs, since the irq is shared */
 
-	if (!active_irqs)
-		return IRQ_NONE;
+	if (!active_irqs) {
+		ret = IRQ_NONE;
+		vop_core_clks_disable(vop);
+		goto out;
+	}
 

  	if (active_irqs & DSP_HOLD_VALID_INTR) {
  	
  		complete(&vop->dsp_hold_completion);

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +4 -0

@@ -1236,6 +1245,10 @@ static irqreturn_t vop_isr(int irq, void *data)
 
  		DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
 
  			      active_irqs);
 
+	vop_core_clks_disable(vop);
+
+out:
+	pm_runtime_put(vop->dev);
 

return ret;

  }

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -0

@@ -1614,9 +1627,6 @@ static int vop_bind(struct device *dev, struct

device *master, void *data)>
if (ret)

goto err_disable_pm_runtime;

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_bind(struct device *dev, struct device *master, void *data) · line 1632

Quoted 7 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -3

-	/* IRQ is initially disabled; it gets enabled in power_on */
-	disable_irq(vop->irq);
-
 
  	return 0;
 
  err_disable_pm_runtime:

Am Dienstag, 29. Mai 2018, 13:59:42 CEST schrieb Robin Murphy:

Quoted 68 lines · Heiko Stuebner

Quoted 55 lines

From: Sandy Huang <[email protected]>

The vop irq is shared between vop and iommu and irq probing in the iommu driver moved to the probe function recently. This can in some cases lead to a stall if the irq is triggered while the vop driver still has it disabled, but the vop irq handler gets called.

But there is no real need to disable the irq, as the vop can simply also track its enabled state and ignore irqs in that case. For this we can simply check the power-domain state of the vop, similar to how the iommu driver does it.

So remove the enable/disable handling and add appropriate condition to the irq handler.

changes in v2:
- move to just check the power-domain state
- add clock handling

Signed-off-by Sandy Huang <[email protected]>

[add commit message, moved to pm_runtime_get_if_in_use]

Signed-off-by Heiko Stuebner <[email protected]>

---

   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++-------
   1 file changed, 19 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index b55156b8ba3b..615a5b44bfe9 100644

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -2

@@ -573,8 +573,6 @@ static int vop_enable(struct drm_crtc *crtc)
 
   	spin_unlock(&vop->reg_lock);
 
-	enable_irq(vop->irq);
-
 
   	drm_crtc_vblank_on(crtc);
 

   	return 0;

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -2

@@ -618,8 +616,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc
 *crtc,>
   	vop_dsp_hold_valid_irq_disable(vop);
 
-	disable_irq(vop->irq);
-
 
   	vop->is_enabled = false;
 

   	/*

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static irqreturn_t vop_isr(int irq, void *data) · line 1197

Quoted 7 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +3 -0

@@ -1195,6 +1191,16 @@ static irqreturn_t vop_isr(int irq, void *data)
 
   	uint32_t active_irqs;
   	int ret = IRQ_NONE;
 
+	/*
+	 * The irq is shared with the iommu. If the power-domain is off
+	 * the irq has to be targetted at the iommu.

Hmm, aren't the IOMMUs in the same power domain as their respective master, though? I would naively assume so, and it does look that way from the DTs in the BSP kernel.

Quoted 4 lines

AFAICS the IOMMU usage count should never be greater than the VOP usage count (except before the VOP driver has probed, but I don't think that matters), so although this looks like a sensible change in general I can't help be a little bit puzzled at how and why the flow works.

Ok, the comment might be misleading. It actually means to use the runtime-pm state of the vop-_device_ as a check.

I.e. in vop_initials(), Marc added the patch clearing and masking all vop interrupts. In vop_enable() we have runtime_get_... + enablement of vop interrupts, which get disabled in vop_disable again.

That way, checking the runtime_pm state should be an indicator if the irq is for the vop and not the iommu.

Quoted 52 lines

Quoted 52 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +7 -0

+	 */
+	if (!pm_runtime_get_if_in_use(vop->dev))
+		return IRQ_NONE;
+
+	if (WARN_ON(vop_core_clks_enable(vop)))
+		goto out;
+
 
   	/*

* interrupt register has interrupt status, enable and clear bits, we
* must hold irq_lock to avoid a race with enable/disable_vblank().

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +5 -2

@@ -1209,8 +1215,11 @@ static irqreturn_t vop_isr(int irq, void *data)
 
   	spin_unlock(&vop->irq_lock);
 
   	/* This is expected for vop iommu irqs, since the irq is shared */
 
-	if (!active_irqs)
-		return IRQ_NONE;
+	if (!active_irqs) {
+		ret = IRQ_NONE;
+		vop_core_clks_disable(vop);
+		goto out;
+	}
 

   	if (active_irqs & DSP_HOLD_VALID_INTR) {
   	
   		complete(&vop->dsp_hold_completion);

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +4 -0

@@ -1236,6 +1245,10 @@ static irqreturn_t vop_isr(int irq, void *data)
 
   		DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
 
   			      active_irqs);
 
+	vop_core_clks_disable(vop);
+
+out:
+	pm_runtime_put(vop->dev);
 

   	return ret;
   
   }

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -0

@@ -1614,9 +1627,6 @@ static int vop_bind(struct device *dev, struct

device *master, void *data)>
if (ret)

   		goto err_disable_pm_runtime;

Quoted 7 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_bind(struct device *dev, struct device *master, void *data) · line 1632

Quoted 7 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -3

-	/* IRQ is initially disabled; it gets enabled in power_on */
-	disable_irq(vop->irq);
-
 
   	return 0;
 
   err_disable_pm_runtime:

This still tries to address the hang seen by Ezequiel Garcia on rk3288.

As Tomasz noted, trying to count enablement can run into concurrency issues, so instead we'll just check if the vop is runtime-enabled to see if it could be the source of the irq and then just do our own clk_enable in the isr to bridge the possible gap between pm_runtime_enable and clk_enable in the core vop_enable() function.

Display tested to still work on rk3328 and rk3399, but as I don't see the hang from Ezequiel I hope that this fixes it.