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

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

Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")
Cc: stable@vger.kernel.org
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 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 2121345a61af..9a1f272e41c7 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

[PATCH v3 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
changes in v3:
- clarify comment to speak of runtime-pm not power-domain

Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")
Cc: stable@vger.kernel.org
Signed-off-by: Sandy Huang <hjc@rock-chips.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 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 9a1f272e41c7..ae8a69793aed 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 runtime-pm state of the
+	 * vop-device is disabled 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

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

[Marc Zyngier]

Hi Heiko,

On 12/06/18 13:15, 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
> changes in v3:
> - clarify comment to speak of runtime-pm not power-domain
> 
> Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sandy Huang <hjc@rock-chips.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  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 9a1f272e41c7..ae8a69793aed 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 runtime-pm state of the
> +	 * vop-device is disabled 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;

As I mentioned before, a WARN_ON() in an interrupt handler is a good way
to make a bad problem even worse, and will give information (full
register and stack dump) that is mostly useless to the context at hand.
Turning it to a dev_warn_ratelimited() (or DRM_ERROR_RATELIMITED if you
want to be DRM compliant) would be a better approach, IMHO.

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

A couple of nits: ret is already set to IRQ_NONE at this stage, and you
could simply rewrite it as:

	if (!active_irq)
		goto out_disable;

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

with the "out_disable" label placed here.

> +	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 v3 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[JeffyChen]

Hi Heiko,

On 06/12/2018 08:15 PM, 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.

hmmm, i think this patch actually fixes another stall case by removing 
the unpaired disable_irq() in vop_bind().

if we do disable_irq() in vop_bind() without enable it again in 
vop_unbind(), the irq_shutdown() called after vop_unbind()(when 
releasing devres) will confuse the irq depth, so the irq will stay 
disabled and could never be enabled again.

>
> 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
> changes in v3:
> - clarify comment to speak of runtime-pm not power-domain
>
> Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sandy Huang <hjc@rock-chips.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>   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 9a1f272e41c7..ae8a69793aed 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 runtime-pm state of the
> +	 * vop-device is disabled 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:
>

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

[Heiko Stuebner]

Am Dienstag, 12. Juni 2018, 14:39:03 CEST schrieb Marc Zyngier:
> Hi Heiko,
> 
> On 12/06/18 13:15, 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
> > changes in v3:
> > - clarify comment to speak of runtime-pm not power-domain
> > 
> > Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sandy Huang <hjc@rock-chips.com>
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > Tested-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  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 9a1f272e41c7..ae8a69793aed 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 runtime-pm state of the
> > +	 * vop-device is disabled 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;
> 
> As I mentioned before, a WARN_ON() in an interrupt handler is a good way
> to make a bad problem even worse, and will give information (full
> register and stack dump) that is mostly useless to the context at hand.
> Turning it to a dev_warn_ratelimited() (or DRM_ERROR_RATELIMITED if you
> want to be DRM compliant) would be a better approach, IMHO.

Gah, sorry that I forgot to address your comment from v2 and thanks
for the reminder.


> > +
> >  	/*
> >  	 * 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;
> > +	}
> 
> A couple of nits: ret is already set to IRQ_NONE at this stage, and you
> could simply rewrite it as:
>
> 	if (!active_irq)
> 		goto out_disable;

That's only one nit :-P ... but will change the patch accordingly.


Heiko

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

[Tomasz Figa]

Hi Heiko,

On Tue, Jun 12, 2018 at 9:15 PM Heiko Stuebner <heiko@sntech.de> 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
> changes in v3:
> - clarify comment to speak of runtime-pm not power-domain
[snip]
> @@ -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);

nit: If we're adding "out:", couldn't we also add "out_clks:" and move
the call to vop_core_clks_disable() there?

> +               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;
>  }

Other than that:

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

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

[Heiko Stuebner]

Am Montag, 18. Juni 2018, 10:44:58 CEST schrieb Tomasz Figa:
> Hi Heiko,
> 
> On Tue, Jun 12, 2018 at 9:15 PM Heiko Stuebner <heiko@sntech.de> 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
> > changes in v3:
> > - clarify comment to speak of runtime-pm not power-domain
> [snip]
> > @@ -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);
> 
> nit: If we're adding "out:", couldn't we also add "out_clks:" and move
> the call to vop_core_clks_disable() there?
> 
> > +               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;
> >  }
> 
> Other than that:
> 
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>

That's similar to what Marc suggested and thus already part of v4
posted last tuesday, so I'll just carry over your Reviewed-by.

Could you possibly also give patch1 a nod of approval? So I can honor
the strong suggestion in the drm-misc documentation? ;-)

Thanks
Heiko