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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx>
>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
>>> ---
>>>
>>> +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

Attachment: signature.asc
Description: OpenPGP digital signature

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

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux