| Message ID | 1316167233-1437-4-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@canuck.infradead.org Delivery-date: Fri, 16 Sep 2011 09:59:25 +0000 Received: from canuck.infradead.org [134.117.69.58] by localhost.localdomain with IMAP (fetchmail-6.3.20) for <mchehab@localhost> (single-drop); Fri, 16 Sep 2011 07:56:09 -0300 (BRT) Received: from casper.infradead.org ([2001:770:15f::2]) by canuck.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1R4VCe-0005WZ-QI for mchehab@canuck.infradead.org; Fri, 16 Sep 2011 09:59:24 +0000 Received: from vger.kernel.org ([209.132.180.67]) by casper.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1R4VCc-0006tG-9s; Fri, 16 Sep 2011 09:59:23 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752531Ab1IPJ7Q (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Fri, 16 Sep 2011 05:59:16 -0400 Received: from bear.ext.ti.com ([192.94.94.41]:34248 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752348Ab1IPJ7O (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 16 Sep 2011 05:59:14 -0400 Received: from dlep33.itg.ti.com ([157.170.170.112]) by bear.ext.ti.com (8.13.7/8.13.7) with ESMTP id p8G9xENp021270 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 16 Sep 2011 04:59:14 -0500 Received: from dlep26.itg.ti.com (smtp-le.itg.ti.com [157.170.170.27]) by dlep33.itg.ti.com (8.13.7/8.13.8) with ESMTP id p8G9xEMA025386; Fri, 16 Sep 2011 04:59:14 -0500 (CDT) Received: from DLEE74.ent.ti.com (localhost [127.0.0.1]) by dlep26.itg.ti.com (8.13.8/8.13.8) with ESMTP id p8G9xEV1022545; Fri, 16 Sep 2011 04:59:14 -0500 (CDT) Received: from dlelxv23.itg.ti.com (172.17.1.198) by DLEE74.ent.ti.com (157.170.170.8) with Microsoft SMTP Server id 14.1.323.3; Fri, 16 Sep 2011 04:59:14 -0500 Received: from legion.dal.design.ti.com (legion.dal.design.ti.com [128.247.22.53]) by dlelxv23.itg.ti.com (8.13.8/8.13.8) with ESMTP id p8G9xD8E022072; Fri, 16 Sep 2011 04:59:13 -0500 Received: from localhost (a0393947pc.apr.dhcp.ti.com [172.24.137.144]) by legion.dal.design.ti.com (8.11.7p1+Sun/8.11.7) with ESMTP id p8G9xB010549; Fri, 16 Sep 2011 04:59:11 -0500 (CDT) From: Archit Taneja <archit@ti.com> To: <hvaibhav@ti.com> CC: <tomi.valkeinen@ti.com>, <linux-omap@vger.kernel.org>, <sumit.semwal@ti.com>, <linux-media@vger.kernel.org>, Archit Taneja <archit@ti.com> Subject: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr Date: Fri, 16 Sep 2011 15:30:31 +0530 Message-ID: <1316167233-1437-4-git-send-email-archit@ti.com> X-Mailer: git-send-email 1.7.1 In-Reply-To: <1316167233-1437-1-git-send-email-archit@ti.com> References: <1316167233-1437-1-git-send-email-archit@ti.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20110916_105922_736930_BE19313F X-CRM114-Status: GOOD ( 18.06 ) X-Spam-Score: -2.2 (--) X-Spam-Report: SpamAssassin version 3.3.2 on casper.infradead.org summary: Content analysis details: (-2.2 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [209.132.180.67 listed in list.dnswl.org] 2.8 KB_DATE_CONTAINS_TAB KB_DATE_CONTAINS_TAB 2.5 TAB_IN_FROM From starts with a tab -0.5 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] |
Commit Message
Archit Taneja
Sept. 16, 2011, 10 a.m. UTC
Currently, in omap_vout_isr(), if the panel type is DPI, and if we
get either VSYNC or VSYNC2 interrupts, we proceed ahead to set the
current buffers state to VIDEOBUF_DONE and prepare to display the
next frame in the queue.
On OMAP4, because we have 2 LCD managers, the panel type itself is not
sufficient to tell if we have received the correct irq, i.e, we shouldn't
proceed ahead if we get a VSYNC interrupt for LCD2 manager, or a VSYNC2
interrupt for LCD manager.
Fix this by correlating LCD manager to VSYNC interrupt and LCD2 manager
to VSYNC2 interrupt.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/video/omap/omap_vout.c | 14 +++++++++++---
1 files changed, 11 insertions(+), 3 deletions(-)
Comments
> -----Original Message----- > From: Taneja, Archit > Sent: Friday, September 16, 2011 3:31 PM > To: Hiremath, Vaibhav > Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux- > media@vger.kernel.org; Taneja, Archit > Subject: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in > omap_vout_isr > > Currently, in omap_vout_isr(), if the panel type is DPI, and if we > get either VSYNC or VSYNC2 interrupts, we proceed ahead to set the > current buffers state to VIDEOBUF_DONE and prepare to display the > next frame in the queue. > > On OMAP4, because we have 2 LCD managers, the panel type itself is not > sufficient to tell if we have received the correct irq, i.e, we shouldn't > proceed ahead if we get a VSYNC interrupt for LCD2 manager, or a VSYNC2 > interrupt for LCD manager. > > Fix this by correlating LCD manager to VSYNC interrupt and LCD2 manager > to VSYNC2 interrupt. > > Signed-off-by: Archit Taneja <archit@ti.com> > --- > drivers/media/video/omap/omap_vout.c | 14 +++++++++++--- > 1 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/video/omap/omap_vout.c > b/drivers/media/video/omap/omap_vout.c > index c5f2ea0..20638c3 100644 > --- a/drivers/media/video/omap/omap_vout.c > +++ b/drivers/media/video/omap/omap_vout.c > @@ -566,8 +566,8 @@ err: > > static void omap_vout_isr(void *arg, unsigned int irqstatus) > { > - int ret, fid; > - u32 addr; > + int ret, fid, mgr_id; > + u32 addr, irq; > struct omap_overlay *ovl; > struct timeval timevalue; > struct omapvideo_info *ovid; > @@ -583,6 +583,7 @@ static void omap_vout_isr(void *arg, unsigned int > irqstatus) > if (!ovl->manager || !ovl->manager->device) > return; > > + mgr_id = ovl->manager->id; > cur_display = ovl->manager->device; > > spin_lock(&vout->vbq_lock); > @@ -590,7 +591,14 @@ static void omap_vout_isr(void *arg, unsigned int > irqstatus) > > switch (cur_display->type) { > case OMAP_DISPLAY_TYPE_DPI: > - 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? Thanks, Vaibhav > case OMAP_DISPLAY_TYPE_VENC: > -- > 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wednesday 21 September 2011 07:04 PM, Hiremath, Vaibhav wrote: >> -----Original Message----- >> From: Taneja, Archit >> Sent: Friday, September 16, 2011 3:31 PM >> To: Hiremath, Vaibhav >> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux- >> media@vger.kernel.org; Taneja, Archit >> Subject: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in >> omap_vout_isr >> >> Currently, in omap_vout_isr(), if the panel type is DPI, and if we >> get either VSYNC or VSYNC2 interrupts, we proceed ahead to set the >> current buffers state to VIDEOBUF_DONE and prepare to display the >> next frame in the queue. >> >> On OMAP4, because we have 2 LCD managers, the panel type itself is not >> sufficient to tell if we have received the correct irq, i.e, we shouldn't >> proceed ahead if we get a VSYNC interrupt for LCD2 manager, or a VSYNC2 >> interrupt for LCD manager. >> >> Fix this by correlating LCD manager to VSYNC interrupt and LCD2 manager >> to VSYNC2 interrupt. >> >> Signed-off-by: Archit Taneja<archit@ti.com> >> --- >> drivers/media/video/omap/omap_vout.c | 14 +++++++++++--- >> 1 files changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/video/omap/omap_vout.c >> b/drivers/media/video/omap/omap_vout.c >> index c5f2ea0..20638c3 100644 >> --- a/drivers/media/video/omap/omap_vout.c >> +++ b/drivers/media/video/omap/omap_vout.c >> @@ -566,8 +566,8 @@ err: >> >> static void omap_vout_isr(void *arg, unsigned int irqstatus) >> { >> - int ret, fid; >> - u32 addr; >> + int ret, fid, mgr_id; >> + u32 addr, irq; >> struct omap_overlay *ovl; >> struct timeval timevalue; >> struct omapvideo_info *ovid; >> @@ -583,6 +583,7 @@ static void omap_vout_isr(void *arg, unsigned int >> irqstatus) >> if (!ovl->manager || !ovl->manager->device) >> return; >> >> + mgr_id = ovl->manager->id; >> cur_display = ovl->manager->device; >> >> spin_lock(&vout->vbq_lock); >> @@ -590,7 +591,14 @@ static void omap_vout_isr(void *arg, unsigned int >> irqstatus) >> >> switch (cur_display->type) { >> case OMAP_DISPLAY_TYPE_DPI: >> - 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. Regards, Archit > > Thanks, > Vaibhav > > >> case OMAP_DISPLAY_TYPE_VENC: >> -- >> 1.7.1 > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Taneja, Archit > Sent: Thursday, September 22, 2011 11:46 AM > To: Hiremath, Vaibhav > Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux- > media@vger.kernel.org > Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in > omap_vout_isr > > Hi, > > On Wednesday 21 September 2011 07:04 PM, Hiremath, Vaibhav wrote: > >> -----Original Message----- > >> From: Taneja, Archit > >> Sent: Friday, September 16, 2011 3:31 PM > >> To: Hiremath, Vaibhav > >> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux- > >> media@vger.kernel.org; Taneja, Archit > >> Subject: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in > >> omap_vout_isr > >> > >> Currently, in omap_vout_isr(), if the panel type is DPI, and if we > >> get either VSYNC or VSYNC2 interrupts, we proceed ahead to set the > >> current buffers state to VIDEOBUF_DONE and prepare to display the > >> next frame in the queue. > >> > >> On OMAP4, because we have 2 LCD managers, the panel type itself is not > >> sufficient to tell if we have received the correct irq, i.e, we > shouldn't > >> proceed ahead if we get a VSYNC interrupt for LCD2 manager, or a VSYNC2 > >> interrupt for LCD manager. > >> > >> Fix this by correlating LCD manager to VSYNC interrupt and LCD2 manager > >> to VSYNC2 interrupt. > >> > >> Signed-off-by: Archit Taneja<archit@ti.com> > >> --- > >> drivers/media/video/omap/omap_vout.c | 14 +++++++++++--- > >> 1 files changed, 11 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/media/video/omap/omap_vout.c > >> b/drivers/media/video/omap/omap_vout.c > >> index c5f2ea0..20638c3 100644 > >> --- a/drivers/media/video/omap/omap_vout.c > >> +++ b/drivers/media/video/omap/omap_vout.c > >> @@ -566,8 +566,8 @@ err: > >> > >> static void omap_vout_isr(void *arg, unsigned int irqstatus) > >> { > >> - int ret, fid; > >> - u32 addr; > >> + int ret, fid, mgr_id; > >> + u32 addr, irq; > >> struct omap_overlay *ovl; > >> struct timeval timevalue; > >> struct omapvideo_info *ovid; > >> @@ -583,6 +583,7 @@ static void omap_vout_isr(void *arg, unsigned int > >> irqstatus) > >> if (!ovl->manager || !ovl->manager->device) > >> return; > >> > >> + mgr_id = ovl->manager->id; > >> cur_display = ovl->manager->device; > >> > >> spin_lock(&vout->vbq_lock); > >> @@ -590,7 +591,14 @@ static void omap_vout_isr(void *arg, unsigned int > >> irqstatus) > >> > >> switch (cur_display->type) { > >> case OMAP_DISPLAY_TYPE_DPI: > >> - 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". For me both are doing same thing, the original code is optimized way of implementation. Can you please review it again? Thanks, Vaibhav > Regards, > Archit > > > > > Thanks, > > Vaibhav > > > > > >> case OMAP_DISPLAY_TYPE_VENC: > >> -- > >> 1.7.1 > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vaibhav, >> >> On Mon, Sep 26, 2011 at 3:49 PM, Hiremath, Vaibhav <hvaibhav@ti.com> wrote: >>> >>> > -----Original Message----- >>> > From: Taneja, Archit >>> > Sent: Thursday, September 22, 2011 11:46 AM >>> > To: Hiremath, Vaibhav >>> > Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux- >>> > media@vger.kernel.org >>> > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Semwal, Sumit > Sent: Tuesday, September 27, 2011 11:12 AM > To: Hiremath, Vaibhav > Cc: Taneja, Archit; Valkeinen, Tomi; linux-omap@vger.kernel.org; linux- > media@vger.kernel.org > 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@ti.com> > wrote: > >>> > >>> > -----Original Message----- > >>> > From: Taneja, Archit > >>> > Sent: Thursday, September 22, 2011 11:46 AM > >>> > To: Hiremath, Vaibhav > >>> > Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; > linux- > >>> > media@vger.kernel.org > >>> > 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-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-09-27 at 12:09 +0530, Hiremath, Vaibhav wrote: > 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. With the current code, the ISR code will be ran for a panel connected to LCD1 output when VSYNC for LCD2 happens. After Archit's patch, this no longer happens. I don't know what the ISR code does, so it may not cause any problems, but it sure doesn't sound right running the code when a wrong interrupt happens. Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vaibhav, On Tue, Sep 27, 2011 at 12:09 PM, Hiremath, Vaibhav <hvaibhav@ti.com> wrote: >> -----Original Message----- >> From: Semwal, Sumit >> Sent: Tuesday, September 27, 2011 11:12 AM >> To: Hiremath, Vaibhav >> Cc: Taneja, Archit; Valkeinen, Tomi; linux-omap@vger.kernel.org; linux- >> media@vger.kernel.org >> Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in >> omap_vout_isr >> >> Hi Vaibhav, <snip> >> >>> 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 - I did review it carefully (the first time, and again), and maybe it is something that you're able to see which I can't. Could you please explain why you think (1) if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2))) goto vout_isr_err; is *exactly* the same as (2) if (!((mgr==LCD) && (irqstatus & DISPC_IRQ_VSYNC)) || (mgr==LCD2) && (irqstatus & DISPC_IRQ_VSYNC2)) ) goto vout_isr_err; [which I understand is what Archit's patch does] I am not able to see any correlation in (1) between mgr and irq, whereas it is quite clear in (2). Let me know if I missed something? Best regards, ~Sumit. > > Thanks, > Vaibhav <snip> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Valkeinen, Tomi > Sent: Tuesday, September 27, 2011 12:19 PM > To: Hiremath, Vaibhav > Cc: Semwal, Sumit; Taneja, Archit; linux-omap@vger.kernel.org; linux- > media@vger.kernel.org > Subject: RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in > omap_vout_isr > > On Tue, 2011-09-27 at 12:09 +0530, Hiremath, Vaibhav wrote: > > 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. > > With the current code, the ISR code will be ran for a panel connected to > LCD1 output when VSYNC for LCD2 happens. > > After Archit's patch, this no longer happens. > > I don't know what the ISR code does, so it may not cause any problems, > but it sure doesn't sound right running the code when a wrong interrupt > happens. > If you look at the patch, the patch barely checks for the condition and makes sure that the interrupt is either of VSYNC or VSYNC2, else return. Rest everything is same. The right fix is in streamon api, where you mask the interrupt before registering it. Thanks, Vaibhav > Tomi > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 27 September 2011 12:24 PM, Hiremath, Vaibhav wrote: >> -----Original Message----- >> From: Valkeinen, Tomi >> Sent: Tuesday, September 27, 2011 12:19 PM >> To: Hiremath, Vaibhav >> Cc: Semwal, Sumit; Taneja, Archit; linux-omap@vger.kernel.org; linux- >> media@vger.kernel.org >> Subject: RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in >> omap_vout_isr >> >> On Tue, 2011-09-27 at 12:09 +0530, Hiremath, Vaibhav wrote: >>> 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. >> >> With the current code, the ISR code will be ran for a panel connected to >> LCD1 output when VSYNC for LCD2 happens. >> >> After Archit's patch, this no longer happens. >> >> I don't know what the ISR code does, so it may not cause any problems, >> but it sure doesn't sound right running the code when a wrong interrupt >> happens. >> > > If you look at the patch, the patch barely checks for the condition and > makes sure that the interrupt is either of VSYNC or VSYNC2, else return. Rest everything is same. It doesn't only make sure that the interrupt it one of them, it uses it later too in the check: if (!(irqstatus & irq)) goto vout_isr_err; ... ... > > The right fix is in streamon api, where you mask the interrupt before > registering it. If this is the right fix, we should have a purely selective method of selecting the interrupts. Even for OMAP3, we register interrupts for LCD and TV, and then check the interrupt in the handler using panel type. Now, since have 2 different interrupts for the same panel type, we have to further distinguish using the manager id. Archit > > Thanks, > Vaibhav > >> Tomi >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-09-27 at 12:24 +0530, Hiremath, Vaibhav wrote: > If you look at the patch, the patch barely checks for the condition > and > makes sure that the interrupt is either of VSYNC or VSYNC2, else > return. Rest everything is same. This is what the current code does, in clearer form and slightly pseudo code: if (irq == VSYNC || irq == VSYNC2) { do isr stuff } This is what it does after Archit's patch: if ((lcd == LCD1 && irq == VSYNC) || (lcd == LCD2 && irq == VSYNC2)) { do isr stuff; } I see a clear difference there. Or am I missing something? > The right fix is in streamon api, where you mask the interrupt before > registering it. I'm not familiar with v4l so I don't know what that means, but yes, it would be better if it's possible to only register for the needed interrupts. But the ISR code is still needed. If you are using both LCDs, you will get both interrupts and you need to check if the interrupt is for the right output. Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Taneja, Archit > Sent: Tuesday, September 27, 2011 12:32 PM > To: Hiremath, Vaibhav > Cc: Valkeinen, Tomi; Semwal, Sumit; linux-omap@vger.kernel.org; linux- > media@vger.kernel.org > Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in > omap_vout_isr > > On Tuesday 27 September 2011 12:24 PM, Hiremath, Vaibhav wrote: > >> -----Original Message----- > >> From: Valkeinen, Tomi > >> Sent: Tuesday, September 27, 2011 12:19 PM > >> To: Hiremath, Vaibhav > >> Cc: Semwal, Sumit; Taneja, Archit; linux-omap@vger.kernel.org; linux- > >> media@vger.kernel.org > >> Subject: RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in > >> omap_vout_isr > >> > >> On Tue, 2011-09-27 at 12:09 +0530, Hiremath, Vaibhav wrote: > >>> 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. > >> > >> With the current code, the ISR code will be ran for a panel connected > to > >> LCD1 output when VSYNC for LCD2 happens. > >> > >> After Archit's patch, this no longer happens. > >> > >> I don't know what the ISR code does, so it may not cause any problems, > >> but it sure doesn't sound right running the code when a wrong interrupt > >> happens. > >> > > > > If you look at the patch, the patch barely checks for the condition and > > makes sure that the interrupt is either of VSYNC or VSYNC2, else return. > Rest everything is same. > > It doesn't only make sure that the interrupt it one of them, it uses it > later too in the check: > > if (!(irqstatus & irq)) > goto vout_isr_err; > ... > ... > > > > > The right fix is in streamon api, where you mask the interrupt before > > registering it. > > If this is the right fix, we should have a purely selective method of > selecting the interrupts. Even for OMAP3, we register interrupts for LCD > and TV, and then check the interrupt in the handler using panel type. > Now, since have 2 different interrupts for the same panel type, we have > to further distinguish using the manager id. > I have to agree here with you that we do not have this available now. Also I had reviewed the patch again, and I think I now understand what usecase you are referring here. My bad, I concluded early on this.... Thanks, Vaibhav > Archit > > > > > Thanks, > > Vaibhav > > > >> Tomi > >> > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c index c5f2ea0..20638c3 100644 --- a/drivers/media/video/omap/omap_vout.c +++ b/drivers/media/video/omap/omap_vout.c @@ -566,8 +566,8 @@ err: static void omap_vout_isr(void *arg, unsigned int irqstatus) { - int ret, fid; - u32 addr; + int ret, fid, mgr_id; + u32 addr, irq; struct omap_overlay *ovl; struct timeval timevalue; struct omapvideo_info *ovid; @@ -583,6 +583,7 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus) if (!ovl->manager || !ovl->manager->device) return; + mgr_id = ovl->manager->id; cur_display = ovl->manager->device; spin_lock(&vout->vbq_lock); @@ -590,7 +591,14 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus) switch (cur_display->type) { case OMAP_DISPLAY_TYPE_DPI: - 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; case OMAP_DISPLAY_TYPE_VENC: