Hi Vaibhav, >> >> On Mon, Sep 26, 2011 at 3:49 PM, Hiremath, Vaibhav <hvaibhav@xxxxxx> wrote: >>> >>> > -----Original Message----- >>> > From: Taneja, Archit >>> > Sent: Thursday, September 22, 2011 11:46 AM >>> > To: Hiremath, Vaibhav >>> > Cc: Valkeinen, Tomi; linux-omap@xxxxxxxxxxxxxxx; Semwal, Sumit; linux- >>> > media@xxxxxxxxxxxxxxx >>> > Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in >>> > omap_vout_isr >>> > >>> > Hi, > > <snip> >>> >>> > >> - if (!(irqstatus& (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2))) >>> > >> + if (mgr_id == OMAP_DSS_CHANNEL_LCD) >>> > >> + irq = DISPC_IRQ_VSYNC; >>> > >> + else if (mgr_id == OMAP_DSS_CHANNEL_LCD2) >>> > >> + irq = DISPC_IRQ_VSYNC2; >>> > >> + else >>> > >> + goto vout_isr_err; >>> > >> + >>> > >> + if (!(irqstatus& irq)) >>> > >> goto vout_isr_err; >>> > >> break; >>> > > I understand what this patch meant for, but I am more curious to know >>> > > What is value addition of this patch? >>> > > >>> > > if (!(irqstatus& (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2))) >>> > > >>> > > Vs >>> > > >>> > > if (mgr_id == OMAP_DSS_CHANNEL_LCD) >>> > > irq = DISPC_IRQ_VSYNC; >>> > > else if (mgr_id == OMAP_DSS_CHANNEL_LCD2) >>> > > irq = DISPC_IRQ_VSYNC2; >>> > > >>> > > Isn't this same, because we are not doing anything separate and special >>> > > for VSYNC and VSYNC2? >>> > >>> > Consider a use case where the primary LCD panel(connected to LCD >>> > manager) is configured at a 60 Hz refresh rate, and the secondary >>> > panel(connected to LCD2) refreshes at 30 Hz. Let the overlay >>> > configuration be something like this: >>> > >>> > /dev/video1----->overlay1----->LCD manager----->Primary panel(60 Hz) >>> > >>> > >>> > /dev/video2----->overlay2----->LCD2 manager----->Secondary Panel(30 Hz) >>> > >>> > >>> > Now if we are doing streaming on both video1 and video2, since we deque >>> > on either VSYNC or VSYNC2, video2 buffers will deque faster than the >>> > panels refresh, which is incorrect. So for video2, we should only deque >>> > when we get a VSYNC2 interrupt. Thats why there is a need to correlate >>> > the interrupt and the overlay manager. >>> > >>> >>> Archit, >>> >>> I think I am still not understood or convinced with your description above, >>> >>> The code snippet which we are referring here, does check whether the >>> interrupt is either VSYNC or VSYNC2, else fall back to "vout_isr_err". > > I am not quite sure I understand what is the confusing part here - below is my understanding; please correct me if you think otherwise. As I understand, this patch creates a (missing) correlation between a manager and the corresponding ISR. The earlier code would accept a VSYNC2 for LCD1 manager, which is not the correct thing to do. That is why the check of 'if ((mgr==LCD) && (IRQ==VSYNC))' kind of thing is needed; Which part of this do you think the above patch doesn't do? Or, do you think it is not needed / done correctly? >>> >>> For me both are doing same thing, the original code is optimized way of implementation. Can you please review it again? >>> >>> Thanks, >>> Vaibhav > > Thanks and best regards, ~Sumit. >>> >>> <snip> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
