[PATCH v4 06/22] drm: omapdrm: Handle FIFO underflow IRQs internally

[Laurent Pinchart]

Hi Tomi,

On Wednesday 14 Dec 2016 12:22:29 Tomi Valkeinen wrote:
> On 14/12/16 02:27, Laurent Pinchart wrote:
> > As the FIFO underflow IRQ handler just prints an error message to the
> > kernel log, simplify the code by not registering one IRQ handler per
> > plane but print the messages directly from the main IRQ handler.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
> > ---
> > 
> > +static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
> > +				    u32 irqstatus)
> > +{
> > +	static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL,
> > +				      DEFAULT_RATELIMIT_BURST);
> > +	static const struct {
> > +		const char *name;
> > +		u32 mask;
> > +	} sources[] = {
> > +		{ "gfx", DISPC_IRQ_GFX_FIFO_UNDERFLOW },
> > +		{ "vid1", DISPC_IRQ_VID1_FIFO_UNDERFLOW },
> > +		{ "vid2", DISPC_IRQ_VID2_FIFO_UNDERFLOW },
> > +		{ "vid3", DISPC_IRQ_VID3_FIFO_UNDERFLOW },
> > +	};
> > +
> > +	const u32 mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW
> > +		       | DISPC_IRQ_VID1_FIFO_UNDERFLOW
> > +		       | DISPC_IRQ_VID2_FIFO_UNDERFLOW
> > +		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
> > +	unsigned int i;
> > +
> > +	spin_lock(&list_lock);
> > +	irqstatus &= priv->irq_mask & mask;
> > +	spin_unlock(&list_lock);
> > +
> > +	if (!irqstatus)
> > +		return;
> 
> This is called every time we get any DSS interrupt, so I think it would
> be good to have a fast-path here without the lock: irqstatus & mask.
>
> Or maybe store the enabled underflow irq bits separately from irq_mask,
> as the underflow bits are never changed after the initial setup, and
> then there's no need for locking.

I'd prefer going for the former, but I'm a bit concerned that an IRQ bit 
defined as FIFO overflow on one platform could be defined as something else on 
another platform and be mistaken.

Given that we already take the same lock in the IRQ handler to call the wait 
handlers, do you think this is really an issue ?

-- 
Regards,

Laurent Pinchart