Message ID | 1426015494-16799-3-git-send-email-tim.nordell@logicpd.com (mailing list archive) |
---|---|
State | New |
Delegated to: | Laurent Pinchart |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1YVPyM-0004uK-OB; Tue, 10 Mar 2015 20:37:46 +0100 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.72/mailfrontend-7) with esmtp id 1YVPyK-00005W-0v; Tue, 10 Mar 2015 20:37:46 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752772AbbCJThk (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 10 Mar 2015 15:37:40 -0400 Received: from smtp.logicpd.com ([174.46.170.145]:51779 "HELO smtp.logicpd.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751737AbbCJThf (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 10 Mar 2015 15:37:35 -0400 X-ASG-Debug-ID: 1426015509-083a4f3ca92a8fd0001-rVgMWv Received: from EDPR-MAIL01.logicpd.com (edpr-mail01.logicpd.com [10.1.18.127]) by smtp.logicpd.com with ESMTP id denw3nYVRNBJ5cR3 (version=TLSv1 cipher=ECDHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 10 Mar 2015 14:25:09 -0500 (CDT) X-Barracuda-Envelope-From: tim.nordell@logicpd.com Received: from mplswks572_linux.logicpd.com (10.0.5.78) by EDPR-MAIL01.logicpd.com (10.1.18.127) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Tue, 10 Mar 2015 14:25:09 -0500 From: Tim Nordell <tim.nordell@logicpd.com> To: <linux-media@vger.kernel.org> CC: <laurent.pinchart@ideasonboard.com>, <sakari.ailus@iki.fi>, Tim Nordell <tim.nordell@logicpd.com> Subject: [PATCH 2/3] omap3isp: Disable CCDC's VD0 and VD1 interrupts when stream is not enabled Date: Tue, 10 Mar 2015 14:24:53 -0500 X-ASG-Orig-Subj: [PATCH 2/3] omap3isp: Disable CCDC's VD0 and VD1 interrupts when stream is not enabled Message-ID: <1426015494-16799-3-git-send-email-tim.nordell@logicpd.com> X-Mailer: git-send-email 2.0.4 In-Reply-To: <1426015494-16799-1-git-send-email-tim.nordell@logicpd.com> References: <1426015494-16799-1-git-send-email-tim.nordell@logicpd.com> MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.0.5.78] X-ClientProxiedBy: EDPR-MAIL01.logicpd.com (10.1.18.127) To EDPR-MAIL01.logicpd.com (10.1.18.127) X-Barracuda-Connect: edpr-mail01.logicpd.com[10.1.18.127] X-Barracuda-Start-Time: 1426015509 X-Barracuda-Encrypted: ECDHE-RSA-AES256-SHA X-Barracuda-URL: https://edpr-cuda.logicpd.com:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at logicpd.com X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.00 X-Barracuda-Spam-Status: No, SCORE=0.00 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=9.0 tests=BSF_SC0_MISMATCH_TO X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.16481 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.00 BSF_SC0_MISMATCH_TO Envelope rcpt doesn't match header 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-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2015.3.10.192720 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_2000_2999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, DATE_TZ_NA 0, REFERENCES 0, URI_ENDS_IN_HTML 0, WEBMAIL_SOURCE 0, WEBMAIL_XOIP 0, WEBMAIL_X_IP_HDR 0, __ANY_URI 0, __CP_MEDIA_BODY 0, __CP_URI_IN_BODY 0, __CT 0, __CT_TEXT_PLAIN 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_XOIP 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __LINES_OF_YELLING 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __REFERENCES 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
Tim Nordell
March 10, 2015, 7:24 p.m. UTC
During testing there appeared to be a race condition where the IRQs
for VD0 and VD1 could be triggered while enabling the CCDC module
before the pipeline status was updated. Simply modify the trigger
conditions for VD0 and VD1 so they won't occur when the CCDC module
is not enabled.
(When this occurred during testing, the VD0 interrupt was occurring
over and over again starving the rest of the system.)
Signed-off-by: Tim Nordell <tim.nordell@logicpd.com>
---
drivers/media/platform/omap3isp/ispccdc.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
Comments
Hi Tim, Thank you for the patch. On Tuesday 10 March 2015 14:24:53 Tim Nordell wrote: > During testing there appeared to be a race condition where the IRQs > for VD0 and VD1 could be triggered while enabling the CCDC module > before the pipeline status was updated. Simply modify the trigger > conditions for VD0 and VD1 so they won't occur when the CCDC module > is not enabled. > > (When this occurred during testing, the VD0 interrupt was occurring > over and over again starving the rest of the system.) I'm curious, might this be caused by the input (adv7180 in your case) being enabled before the ISP ? The CCDC is very sensitive to any glitch in its input signals, you need to make sure that the source is disabled before its subdev s_stream operation is called. Given that the adv7180 driver doesn't implement s_stream, I expect it to be free-running, which is definitely a problem. > Signed-off-by: Tim Nordell <tim.nordell@logicpd.com> > --- > drivers/media/platform/omap3isp/ispccdc.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/ispccdc.c > b/drivers/media/platform/omap3isp/ispccdc.c index 587489a..d5de843 100644 > --- a/drivers/media/platform/omap3isp/ispccdc.c > +++ b/drivers/media/platform/omap3isp/ispccdc.c > @@ -1218,13 +1218,6 @@ static void ccdc_configure(struct isp_ccdc_device > *ccdc) } > ccdc_config_imgattr(ccdc, ccdc_pattern); > > - /* Generate VD0 on the last line of the image and VD1 on the > - * 2/3 height line. > - */ > - isp_reg_writel(isp, ((format->height - 2) << ISPCCDC_VDINT_0_SHIFT) | > - ((format->height * 2 / 3) << ISPCCDC_VDINT_1_SHIFT), > - OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT); > - > /* CCDC_PAD_SOURCE_OF */ > format = &ccdc->formats[CCDC_PAD_SOURCE_OF]; > crop = &ccdc->crop; > @@ -1316,11 +1309,29 @@ unlock: > > static void __ccdc_enable(struct isp_ccdc_device *ccdc, int enable) > { > + struct v4l2_mbus_framefmt *format = &ccdc->formats[CCDC_PAD_SINK]; > struct isp_device *isp = to_isp_device(ccdc); > + int vd0, vd1; > > isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_PCR, > ISPCCDC_PCR_EN, enable ? ISPCCDC_PCR_EN : 0); > > + /* Generate VD0 on the last line of the image and VD1 on the > + * 2/3 height line when enabled. Otherwise, set VD0 and VD1 > + * interrupts high enough that they won't be generated. > + */ > + if (enable) { > + vd0 = format->height - 2; > + vd1 = format->height * 2 / 3; > + } else { > + vd0 = 0xffff; > + vd1 = 0xffff; > + } > + > + isp_reg_writel(isp, (vd0 << ISPCCDC_VDINT_0_SHIFT) | > + (vd1 << ISPCCDC_VDINT_1_SHIFT), > + OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT); > + > ccdc->running = enable; > }
Laurent - On 03/18/15 10:19, Laurent Pinchart wrote: > Hi Tim, > > Thank you for the patch. > > On Tuesday 10 March 2015 14:24:53 Tim Nordell wrote: >> During testing there appeared to be a race condition where the IRQs >> for VD0 and VD1 could be triggered while enabling the CCDC module >> before the pipeline status was updated. Simply modify the trigger >> conditions for VD0 and VD1 so they won't occur when the CCDC module >> is not enabled. >> >> (When this occurred during testing, the VD0 interrupt was occurring >> over and over again starving the rest of the system.) > I'm curious, might this be caused by the input (adv7180 in your case) being > enabled before the ISP ? The CCDC is very sensitive to any glitch in its input > signals, you need to make sure that the source is disabled before its subdev > s_stream operation is called. Given that the adv7180 driver doesn't implement > s_stream, I expect it to be free-running, which is definitely a problem. > I'll give that a shot and try add code into the adv7180 driver to turn on and off its output signals. However, it seems like if the driver can avoid a problem presented by external hardware (or other drivers), that it should. Something like either turning off the VD0 and VD1 interrupts when not in use, or by simply moving the trigger points for those interrupts (as I did here) to avoid problems by presented by signals to the system is probably a good thing for robustness. - Tim -- 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 Tim, On Wednesday 18 March 2015 10:25:34 Tim Nordell wrote: > On 03/18/15 10:19, Laurent Pinchart wrote: > > On Tuesday 10 March 2015 14:24:53 Tim Nordell wrote: > >> During testing there appeared to be a race condition where the IRQs > >> for VD0 and VD1 could be triggered while enabling the CCDC module > >> before the pipeline status was updated. Simply modify the trigger > >> conditions for VD0 and VD1 so they won't occur when the CCDC module > >> is not enabled. > >> > >> (When this occurred during testing, the VD0 interrupt was occurring > >> over and over again starving the rest of the system.) > > > > I'm curious, might this be caused by the input (adv7180 in your case) > > being enabled before the ISP ? The CCDC is very sensitive to any glitch in > > its input signals, you need to make sure that the source is disabled > > before its subdev s_stream operation is called. Given that the adv7180 > > driver doesn't implement s_stream, I expect it to be free-running, which > > is definitely a problem. > > I'll give that a shot and try add code into the adv7180 driver to turn on > and off its output signals. However, it seems like if the driver can avoid > a problem presented by external hardware (or other drivers), that it should. > Something like either turning off the VD0 and VD1 interrupts when not in > use, or by simply moving the trigger points for those interrupts (as I did > here) to avoid problems by presented by signals to the system is probably a > good thing for robustness. I don't disagree with that. I'll have to review the patch in details, as the CCDC code is quite sensitive. In order to do so, I'd like to know whether the problem in your case was caused by the adv7180 always being enabled. Any luck with adding a s_stream implementation in the adv7180 driver ? :-)
Laurent - On 04/21/15 12:58, Laurent Pinchart wrote: > Hi Tim, > > On Wednesday 18 March 2015 10:25:34 Tim Nordell wrote: >> I'll give that a shot and try add code into the adv7180 driver to turn on >> and off its output signals. However, it seems like if the driver can avoid >> a problem presented by external hardware (or other drivers), that it should. >> Something like either turning off the VD0 and VD1 interrupts when not in >> use, or by simply moving the trigger points for those interrupts (as I did >> here) to avoid problems by presented by signals to the system is probably a >> good thing for robustness. > I don't disagree with that. I'll have to review the patch in details, as the > CCDC code is quite sensitive. In order to do so, I'd like to know whether the > problem in your case was caused by the adv7180 always being enabled. Any luck > with adding a s_stream implementation in the adv7180 driver ? :-) > I did add the stream on/off code, but it still seemed to have some difficulties. The codebase has effectively been handed off to our client, however, at this point. I still happen to have hardware (we're wrapping things up with the client), but likely I won't have the hardware in a week or so. I still think that the driver should avoid having the interrupts enabled if it knows it shouldn't be receiving any at a given point. I personally like the approach of modifying the VD0/VD1 trigger points as it effectively silences those interrupts without touching the central interrupt register (less potential locking issues between the various components in the OMAP3 ISP), but it could be reworked of course to touch the central interrupt register too. - Tim -- 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/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c index 587489a..d5de843 100644 --- a/drivers/media/platform/omap3isp/ispccdc.c +++ b/drivers/media/platform/omap3isp/ispccdc.c @@ -1218,13 +1218,6 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc) } ccdc_config_imgattr(ccdc, ccdc_pattern); - /* Generate VD0 on the last line of the image and VD1 on the - * 2/3 height line. - */ - isp_reg_writel(isp, ((format->height - 2) << ISPCCDC_VDINT_0_SHIFT) | - ((format->height * 2 / 3) << ISPCCDC_VDINT_1_SHIFT), - OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT); - /* CCDC_PAD_SOURCE_OF */ format = &ccdc->formats[CCDC_PAD_SOURCE_OF]; crop = &ccdc->crop; @@ -1316,11 +1309,29 @@ unlock: static void __ccdc_enable(struct isp_ccdc_device *ccdc, int enable) { + struct v4l2_mbus_framefmt *format = &ccdc->formats[CCDC_PAD_SINK]; struct isp_device *isp = to_isp_device(ccdc); + int vd0, vd1; isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_PCR, ISPCCDC_PCR_EN, enable ? ISPCCDC_PCR_EN : 0); + /* Generate VD0 on the last line of the image and VD1 on the + * 2/3 height line when enabled. Otherwise, set VD0 and VD1 + * interrupts high enough that they won't be generated. + */ + if (enable) { + vd0 = format->height - 2; + vd1 = format->height * 2 / 3; + } else { + vd0 = 0xffff; + vd1 = 0xffff; + } + + isp_reg_writel(isp, (vd0 << ISPCCDC_VDINT_0_SHIFT) | + (vd1 << ISPCCDC_VDINT_1_SHIFT), + OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT); + ccdc->running = enable; }