Hi Heiko,
On 28/05/18 14:20, Heiko Stuebner wrote:
> From: Sandy Huang <h...@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 <h...@rock-chips.com>
> [add commit message, moved to pm_runtime_get_if_in_use]
> Signed-off-by: Heiko Stuebner <he...@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)))
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...
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
