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

[Tomi Valkeinen]

On 14/12/16 13:48, Laurent Pinchart wrote:
> 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.

I meant that you could first check irqstatus & mask, and exit if 0.
After that you could do the above spinlock, and the check against
priv->irq_mask & mask.

So yes, it's not perfect either, and in the worst case could mean still
doing the spinlock at every irq... But, if I'm not mistaken, the current
HW versions do not have overlapping irq bits.

> 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 ?

I don't think it's really an issue. But it's nice to optimize irq
handlers, especially as this is a hard-irq.

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161214/6dab0fac/attachment.sig>