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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2241 bytes --]

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@ideasonboard.com>
>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@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 ?

Yep, I think it's fine for the time being.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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