> -----Original Message----- > From: Semwal, Sumit > Sent: Tuesday, September 27, 2011 11:12 AM > To: Hiremath, Vaibhav > Cc: Taneja, Archit; Valkeinen, Tomi; linux-omap@xxxxxxxxxxxxxxx; linux- > media@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in > omap_vout_isr > > 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? Sumit, Please look at the patch carefully, it does exactly same thing. I understand the use-case what Archit explained in the last email but in this patch context, the use-case change anything here in this patch. Can you review it carefully again? Thanks, Vaibhav > >>> > >>> 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-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
