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

[Heiko Stuebner <heiko@sntech.de>]

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@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
> > 
> > Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sandy Huang <hjc@rock-chips.com>
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > Tested-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  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



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel