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

[PATCH v4 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

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

[Tomasz Figa]

Hi Heiko,

On Tue, Jun 12, 2018 at 10:20 PM Heiko Stuebner <heiko@sntech.de> wrote:
>
> 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(-)

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

Best regards,
Tomasz

[PATCH v4 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
changes in v4:
- address Marc's comments (clk-enable WARN_ON and style improvement)

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 | 25 ++++++++++++++-------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 9a1f272e41c7..d105e984cf09 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,18 @@ 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 (vop_core_clks_enable(vop)) {
+		DRM_DEV_ERROR_RATELIMITED(vop->dev, "couldn't enable clocks\n");
+		goto out;
+	}
+
 	/*
 	 * interrupt register has interrupt status, enable and clear bits, we
 	 * must hold irq_lock to avoid a race with enable/disable_vblank().
@@ -1210,7 +1218,7 @@ static irqreturn_t vop_isr(int irq, void *data)
 
 	/* This is expected for vop iommu irqs, since the irq is shared */
 	if (!active_irqs)
-		return IRQ_NONE;
+		goto out_disable;
 
 	if (active_irqs & DSP_HOLD_VALID_INTR) {
 		complete(&vop->dsp_hold_completion);
@@ -1236,6 +1244,10 @@ static irqreturn_t vop_isr(int irq, void *data)
 		DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
 			      active_irqs);
 
+out_disable:
+	vop_core_clks_disable(vop);
+out:
+	pm_runtime_put(vop->dev);
 	return ret;
 }
 
@@ -1614,9 +1626,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 v4 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[Marc Zyngier]

On 12/06/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
> changes in v3:
> - clarify comment to speak of runtime-pm not power-domain
> changes in v4:
> - address Marc's comments (clk-enable WARN_ON and style improvement)
> 
> 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>

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

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

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

[Heiko Stübner]

Hi Marc,

Am Mittwoch, 13. Juni 2018, 15:01:27 CEST schrieb Marc Zyngier:
> On 12/06/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
> > changes in v3:
> > - clarify comment to speak of runtime-pm not power-domain
> > changes in v4:
> > - address Marc's comments (clk-enable WARN_ON and style improvement)
> > 
> > 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>
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

could I ask you to also look at patch1 of this series, to give it an
Ack or Review? drm-misc documentation very strongly suggests [0]
to have at least another set of eyes on a patch and so far noone
came forward ;-)

This of course also applies to everybody else in the Cc list :-D .


Thanks
Heiko


[0] https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html#merge-criteria

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

[Marc Zyngier]

On 18/06/18 09:19, Heiko Stübner wrote:
> Hi Marc,
> 
> Am Mittwoch, 13. Juni 2018, 15:01:27 CEST schrieb Marc Zyngier:
>> On 12/06/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
>>> changes in v3:
>>> - clarify comment to speak of runtime-pm not power-domain
>>> changes in v4:
>>> - address Marc's comments (clk-enable WARN_ON and style improvement)
>>>
>>> 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>
>>
>> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> could I ask you to also look at patch1 of this series, to give it an
> Ack or Review? drm-misc documentation very strongly suggests [0]
> to have at least another set of eyes on a patch and so far noone
> came forward ;-)
> 
> This of course also applies to everybody else in the Cc list :-D

Please feel free to apply my

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

to that one.

Thanks,

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