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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Sandy Huang <hjc@xxxxxxxxxxxxxx>
> > Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
> > Tested-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > ---
> > 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
