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