[PATCH v2 0/2] drm/rockchip: try to fix vblank hang resulting from iommu irq change

[PATCH v2 0/2] drm/rockchip: try to fix vblank hang resulting from iommu irq change

[Heiko Stuebner]

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.


changes in v2:
- adapt approach ... don't try to count usage ourself, because of
  possible concurrency issues with vop enable/disable

Heiko Stuebner (1):
  drm/rockchip: vop: split out core clock enablement into separate
    functions

Sandy Huang (1):
  drm/rockchip: vop: fix irq disabled after vop driver probed

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 72 ++++++++++++++-------
 1 file changed, 50 insertions(+), 22 deletions(-)

-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] drm/rockchip: vop: split out core clock enablement into separate functions

[Heiko Stuebner]

Judging from the iommu code, both the hclk and aclk are necessary for
register access. Split them off into separate functions from the regular
vop enablement, so that we can use them elsewhere as well.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 44 +++++++++++++++------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 45847d4a2e14..b55156b8ba3b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -486,6 +486,31 @@ static void vop_line_flag_irq_disable(struct vop *vop)
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
 }
 
+static int vop_core_clks_enable(struct vop *vop)
+{
+	int ret;
+
+	ret = clk_enable(vop->hclk);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_enable(vop->aclk);
+	if (ret < 0)
+		goto err_disable_hclk;
+
+	return 0;
+
+err_disable_hclk:
+	clk_disable(vop->hclk);
+	return ret;
+}
+
+static void vop_core_clks_disable(struct vop *vop)
+{
+	clk_disable(vop->aclk);
+	clk_disable(vop->hclk);
+}
+
 static int vop_enable(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
@@ -497,17 +522,13 @@ static int vop_enable(struct drm_crtc *crtc)
 		return ret;
 	}
 
-	ret = clk_enable(vop->hclk);
+	ret = vop_core_clks_enable(vop);
 	if (WARN_ON(ret < 0))
 		goto err_put_pm_runtime;
 
 	ret = clk_enable(vop->dclk);
 	if (WARN_ON(ret < 0))
-		goto err_disable_hclk;
-
-	ret = clk_enable(vop->aclk);
-	if (WARN_ON(ret < 0))
-		goto err_disable_dclk;
+		goto err_disable_core;
 
 	/*
 	 * Slave iommu shares power, irq and clock with vop.  It was associated
@@ -519,7 +540,7 @@ static int vop_enable(struct drm_crtc *crtc)
 	if (ret) {
 		DRM_DEV_ERROR(vop->dev,
 			      "failed to attach dma mapping, %d\n", ret);
-		goto err_disable_aclk;
+		goto err_disable_dclk;
 	}
 
 	spin_lock(&vop->reg_lock);
@@ -558,12 +579,10 @@ static int vop_enable(struct drm_crtc *crtc)
 
 	return 0;
 
-err_disable_aclk:
-	clk_disable(vop->aclk);
 err_disable_dclk:
 	clk_disable(vop->dclk);
-err_disable_hclk:
-	clk_disable(vop->hclk);
+err_disable_core:
+	vop_core_clks_disable(vop);
 err_put_pm_runtime:
 	pm_runtime_put_sync(vop->dev);
 	return ret;
@@ -609,8 +628,7 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
 	rockchip_drm_dma_detach_device(vop->drm_dev, vop->dev);
 
 	clk_disable(vop->dclk);
-	clk_disable(vop->aclk);
-	clk_disable(vop->hclk);
+	vop_core_clks_disable(vop);
 	pm_runtime_put(vop->dev);
 	mutex_unlock(&vop->vop_lock);
 
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[Heiko Stuebner]

From: Sandy Huang <hjc@rock-chips.com>

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 <hjc@rock-chips.com>
[add commit message, moved to pm_runtime_get_if_in_use]
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++-------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index b55156b8ba3b..615a5b44bfe9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -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;
 	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.
+	 */
+	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;
 }
 
@@ -1614,9 +1627,6 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		goto err_disable_pm_runtime;
 
-	/* IRQ is initially disabled; it gets enabled in power_on */
-	disable_irq(vop->irq);
-
 	return 0;
 
 err_disable_pm_runtime:
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[Marc Zyngier]

Hi Heiko,

On 28/05/18 14:20, Heiko Stuebner wrote:
> From: Sandy Huang <hjc-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> 
> 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 <hjc-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> [add commit message, moved to pm_runtime_get_if_in_use]
> Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++-------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index b55156b8ba3b..615a5b44bfe9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -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;
>  	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.
> +	 */
> +	if (!pm_runtime_get_if_in_use(vop->dev))
> +		return IRQ_NONE;
> +
> +	if (WARN_ON(vop_core_clks_enable(vop)))

WARN_ON() in an interrupt handler can be quite dangerous. Could you tone
it down a bit, and at least make it only fire once? Something like a
pr_warn_once should be enough (the stack trace is not very relevant, and
seeing it once is enough to know that something is wrong).

> +		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;
>  }
>  
> @@ -1614,9 +1627,6 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		goto err_disable_pm_runtime;
>  
> -	/* IRQ is initially disabled; it gets enabled in power_on */
> -	disable_irq(vop->irq);
> -
>  	return 0;
>  
>  err_disable_pm_runtime:
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[Robin Murphy]

On 28/05/18 14:20, Heiko Stuebner wrote:
> From: Sandy Huang <hjc@rock-chips.com>
> 
> 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 <hjc@rock-chips.com>
> [add commit message, moved to pm_runtime_get_if_in_use]
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++-------
>   1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index b55156b8ba3b..615a5b44bfe9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -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;
>   	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.

Robin.

> +	 */
> +	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;
>   }
>   
> @@ -1614,9 +1627,6 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>   	if (ret)
>   		goto err_disable_pm_runtime;
>   
> -	/* IRQ is initially disabled; it gets enabled in power_on */
> -	disable_irq(vop->irq);
> -
>   	return 0;
>   
>   err_disable_pm_runtime:
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[Heiko Stübner]

Am Dienstag, 29. Mai 2018, 13:59:42 CEST schrieb Robin Murphy:
> On 28/05/18 14:20, Heiko Stuebner wrote:
> > From: Sandy Huang <hjc@rock-chips.com>
> > 
> > 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 <hjc@rock-chips.com>
> > [add commit message, moved to pm_runtime_get_if_in_use]
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > 
> >   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++-------
> >   1 file changed, 19 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index
> > b55156b8ba3b..615a5b44bfe9 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -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;
> >   	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.

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.


> > +	 */
> > +	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;
> >   
> >   }
> > 
> > @@ -1614,9 +1627,6 @@ static int vop_bind(struct device *dev, struct
> > device *master, void *data)> 
> >   	if (ret)
> >   	
> >   		goto err_disable_pm_runtime;
> > 
> > -	/* IRQ is initially disabled; it gets enabled in power_on */
> > -	disable_irq(vop->irq);
> > -
> > 
> >   	return 0;
> >   
> >   err_disable_pm_runtime:




_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[Robin Murphy]

On 29/05/18 13:17, Heiko Stübner wrote:
> Am Dienstag, 29. Mai 2018, 13:59:42 CEST schrieb Robin Murphy:
>> On 28/05/18 14:20, Heiko Stuebner wrote:
>>> From: Sandy Huang <hjc@rock-chips.com>
>>>
>>> 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 <hjc@rock-chips.com>
>>> [add commit message, moved to pm_runtime_get_if_in_use]
>>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>>> ---
>>>
>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++++++++++++-------
>>>    1 file changed, 19 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index
>>> b55156b8ba3b..615a5b44bfe9 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> @@ -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;
>>>    	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.
> 
> 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.

Right, but whenever the VOP is nominally-disabled, the IOMMU should also 
be nominally-disabled, in which case if it's even possible for the IRQ 
to be asserted, both drivers would ignore it for the same reason (plus 
the IOMMU driver would also spew a WARN(), which I'm not sure is always 
appropriate...). That's what I couldn't quite make sense of.

However, from serendipitously stumbling across [1] I see that the IOMMU 
is in fact going to get explicitly enabled by the driver core around 
probing the VOP, which does give a window during which the imbalance is 
present. I can imagine that the IOMMU reset via the VOP driver's 
dma_configure_call() might misbehave if e.g. the VOP was left running 
from a bootloader splash screen, but in that case I would expect to see 
various screaming from the IOMMU driver which wasn't apparent in 
Ezequiel's log. Oh well, as I said before the patch looks sane 
regardless of my ability to reason about it ;)

Robin.

[1] https://patchwork.kernel.org/patch/10408825/

>>> +	 */
>>> +	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;
>>>    
>>>    }
>>>
>>> @@ -1614,9 +1627,6 @@ static int vop_bind(struct device *dev, struct
>>> device *master, void *data)>
>>>    	if (ret)
>>>    	
>>>    		goto err_disable_pm_runtime;
>>>
>>> -	/* IRQ is initially disabled; it gets enabled in power_on */
>>> -	disable_irq(vop->irq);
>>> -
>>>
>>>    	return 0;
>>>    
>>>    err_disable_pm_runtime:
> 
> 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/2] drm/rockchip: try to fix vblank hang resulting from iommu irq change

[Ezequiel Garcia]

On Mon, 2018-05-28 at 15:20 +0200, Heiko Stuebner wrote:
> 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.
> 

Yup, this series fixes the issue I reported. Thanks for fixing so quickly.

Tested-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>

BTW, don't the patches need some stable tags as well?

Regards,
Eze