Am Dienstag, 29. Mai 2018, 13:59:42 CEST schrieb Robin Murphy:
> On 28/05/18 14:20, 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
> >
> > Signed-off-by: Sandy Huang <hjc@xxxxxxxxxxxxxx>
> > [add commit message, moved to pm_runtime_get_if_in_use]
> > Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
> > ---
> >
> > 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.
>
> Hmm, aren't the IOMMUs in the same power domain as their respective
> master, though? I would naively assume so, and it does look that way
> from the DTs in the BSP kernel.
>
> AFAICS the IOMMU usage count should never be greater than the VOP usage
> count (except before the VOP driver has probed, but I don't think that
> matters), so although this looks like a sensible change in general I
> can't help be a little bit puzzled at how and why the flow works.
Ok, the comment might be misleading. It actually means to use the runtime-pm
state of the vop-_device_ as a check.
I.e. in vop_initials(), Marc added the patch clearing and masking all vop
interrupts. In vop_enable() we have runtime_get_... + enablement of
vop interrupts, which get disabled in vop_disable again.
That way, checking the runtime_pm state should be an indicator if the
irq is for the vop and not 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:
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
