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

From: Tomasz Figa <tfiga@chromium.org>
To: "Heiko Stübner" <heiko@sntech.de>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	Ezequiel Garcia <ezequiel@collabora.com>,
	Robin Murphy <robin.murphy@arm.com>,
	marc.zyngier@arm.com, Jeffy <jeffy.chen@rock-chips.com>,
	Sandy Huang <hjc@rock-chips.com>,
	enric.balletbo@collabora.co.uk,
	Tomeu Vizoso <tomeu.vizoso@collabora.co.uk>,
	stable@vger.kernel.org
Subject: Re: [PATCH v3 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed
Date: Mon, 18 Jun 2018 17:44:58 +0900	[thread overview]
Message-ID: <CAAFQd5D77W8qQm9YmOtr_bpD7WpA9VokjQ+jvTo05F4BFdyrTw@mail.gmail.com> (raw)
In-Reply-To: <20180612121537.31223-3-heiko@sntech.de>

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