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

["Heiko Stübner" <heiko@sntech.de>]

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@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 <hjc@rock-chips.com>
> > [add commit message, moved to pm_runtime_get_if_in_use]
> > Signed-off-by: Heiko Stuebner <heiko@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.
> 
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel