From patchwork Mon Mar 13 09:30:07 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Russell King (Oracle)" X-Patchwork-Id: 39991 Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cnMKI-0000sy-A9; Mon, 13 Mar 2017 09:31:38 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.84_2/mailfrontend-7) with esmtp id 1cnMKG-0007w1-07; Mon, 13 Mar 2017 10:31:38 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751921AbdCMJbP (ORCPT + 1 other); Mon, 13 Mar 2017 05:31:15 -0400 Received: from pandora.armlinux.org.uk ([78.32.30.218]:55680 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751130AbdCMJbO (ORCPT ); Mon, 13 Mar 2017 05:31:14 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2014; h=Sender:In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=d16N51aUZahNfhJC+cWMVx3v7Sag7N6zpXU4OAmWWbE=; b=TA8o+HSmLH0xOQR/KfmcEdOzY6Q4sRzeyMpjL+lZc/XmxO7s3Bl1wjGsz/92e83oor9jVJmuCLKAXAqiYgYGH+3Gp28ydjFAEChy1m2c5ktPr7qCBw9W9A3/Bt8s2FJ2P+nuizWMkernmanHYALA+VzNueZCQ/rYyEk2rgiwH0I=; Received: from n2100.armlinux.org.uk ([2002:4e20:1eda:1:214:fdff:fe10:4f86]:45728) by pandora.armlinux.org.uk with esmtpsa (TLSv1:DHE-RSA-AES256-SHA:256) (Exim 4.82_1-5b7a7c0-XX) (envelope-from ) id 1cnMJ0-0002mc-HR; Mon, 13 Mar 2017 09:30:18 +0000 Received: from linux by n2100.armlinux.org.uk with local (Exim 4.76) (envelope-from ) id 1cnMIr-0001wm-H9; Mon, 13 Mar 2017 09:30:09 +0000 Date: Mon, 13 Mar 2017 09:30:07 +0000 From: Russell King - ARM Linux To: Steve Longerbeam Cc: mark.rutland@arm.com, andrew-ct.chen@mediatek.com, minghsiu.tsai@mediatek.com, sakari.ailus@linux.intel.com, nick@shmanahar.org, songjun.wu@microchip.com, hverkuil@xs4all.nl, Steve Longerbeam , pavel@ucw.cz, robert.jarzmik@free.fr, devel@driverdev.osuosl.org, markus.heiser@darmarIT.de, laurent.pinchart+renesas@ideasonboard.com, shuah@kernel.org, geert@linux-m68k.org, linux-media@vger.kernel.org, devicetree@vger.kernel.org, kernel@pengutronix.de, arnd@arndb.de, mchehab@kernel.org, bparrot@ti.com, robh+dt@kernel.org, horms+renesas@verge.net.au, tiffany.lin@mediatek.com, linux-arm-kernel@lists.infradead.org, niklas.soderlund+renesas@ragnatech.se, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, jean-christophe.trotin@st.com, p.zabel@pengutronix.de, fabio.estevam@nxp.com, shawnguo@kernel.org, sudipm.mukherjee@gmail.com Subject: Re: [PATCH v5 00/39] i.MX Media Driver Message-ID: <20170313093007.GD21222@n2100.armlinux.org.uk> References: <1489121599-23206-1-git-send-email-steve_longerbeam@mentor.com> <20170310201356.GA21222@n2100.armlinux.org.uk> <47542ef8-3e91-b4cd-cc65-95000105f172@gmail.com> <20170312195741.GS21222@n2100.armlinux.org.uk> <20170312202240.GT21222@n2100.armlinux.org.uk> <20170313081625.GX21222@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170313081625.GX21222@n2100.armlinux.org.uk> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2017.3.13.92716 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODY_SIZE_5000_5999 0, BODY_SIZE_7000_LESS 0, DKIM_SIGNATURE 0, FROM_NAME_PHRASE 0, IN_REP_TO 0, LEGITIMATE_SIGNS 0, MSG_THREAD 0, MULTIPLE_REAL_RCPTS 0, NO_URI_HTTPS 0, REFERENCES 0, SINGLE_URI_IN_BODY 0, __ANY_URI 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __CC_NAME 0, __CC_NAME_DIFF_FROM_ACC 0, __CC_REAL_NAMES 0, __CD 0, __CP_MEDIA_BODY 0, __CP_URI_IN_BODY 0, __CT 0, __CT_TEXT_PLAIN 0, __FORWARDED_MSG 0, __HAS_CC_HDR 0, __HAS_FROM 0, __HAS_LIST_ID 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MIME_TEXT_P 0, __MIME_TEXT_P1 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __NO_HTML_TAG_RAW 0, __REFERENCES 0, __SANE_MSGID 0, __SINGLE_URI_TEXT 0, __SUBJ_ALPHA_END 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 0, __TO_NAME 0, __TO_NAME_DIFF_FROM_ACC 0, __TO_REAL_NAMES 0, __URI_IN_BODY 0, __URI_NO_MAILTO 0, __URI_NS , __URI_WITH_PATH 0, __USER_AGENT 0' On Mon, Mar 13, 2017 at 08:16:25AM +0000, Russell King - ARM Linux wrote: > On Sun, Mar 12, 2017 at 09:26:41PM -0700, Steve Longerbeam wrote: > > On 03/12/2017 01:22 PM, Russell King - ARM Linux wrote: > > >What I had was this patch for your v3. I never got to testing your > > >v4 because of the LP-11 problem. > > > > > >In v5, you've changed to propagate the ipu_cpmem_set_image() error > > >code to avoid the resulting corruption, but that leaves the other bits > > >of this patch unaddressed, along my "media: imx: smfc: add support > > >for bayer formats" patch. > > > > > >Your driver basically has no support for bayer formats. > > > > You added the patches to this driver that adds the bayer support, > > I don't think there is anything more required of the driver at this > > point to support bayer, the remaining work needs to happen in the IPUv3 > > driver. > > There is more work, because the way you've merged my changes to > imx_smfc_setup_channel() into csi_idmac_setup_channel() is wrong with > respect to the burst size. > > You always set it to 8 or 16 depending on the width: > > burst_size = (image.pix.width & 0xf) ? 8 : 16; > > ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); > > and then you have my switch() statement which assigns burst_size. > My _tested_ code removed the above, added the switch, which had > a default case which reflected the above setting: > > default: > burst_size = (outfmt->width & 0xf) ? 8 : 16; > > and then went on to set the burst size _after_ the switch statement: > > ipu_cpmem_set_burstsize(priv->smfc_ch, burst_size); > > The effect is unchanged for non-bayer formats. For bayer formats, the > burst size is determined by the bayer data size. > > So, even if it's appropriate to fix ipu_cpmem_set_image(), fixing the > above is still required. > > I'm not convinced that fixing ipu_cpmem_set_image() is even the best > solution - it's not as trivial as it looks on the surface: > > ipu_cpmem_set_resolution(ch, image->rect.width, image->rect.height); > ipu_cpmem_set_stride(ch, pix->bytesperline); > > this is fine, it doesn't depend on the format. However, the next line: > > ipu_cpmem_set_fmt(ch, v4l2_pix_fmt_to_drm_fourcc(pix->pixelformat)); > > does - v4l2_pix_fmt_to_drm_fourcc() is a locally defined function (it > isn't v4l2 code) that converts a v4l2 pixel format to a DRM fourcc. > DRM knows nothing about bayer formats, there aren't fourcc codes in > DRM for it. The result is that v4l2_pix_fmt_to_drm_fourcc() returns > -EINVAL cast to a u32, which gets passed unchecked into ipu_cpmem_set_fmt(). > > ipu_cpmem_set_fmt() won't recognise that, and also returns -EINVAL - and > it's a bug that this is not checked and propagated. If it is checked and > propagated, then we need this to support bayer formats, and I don't see > DRM people wanting bayer format fourcc codes added without there being > a real DRM driver wanting to use them. > > Then there's the business of calculating the top-left offset of the image, > which for bayer always needs to be an even number of pixels - as this > function takes the top-left offset, it ought to respect it, but if it > doesn't meet this criteria, what should it do? csi_idmac_setup_channel() > always sets them to zero, but that's not really something that > ipu_cpmem_set_image() should assume. For the time being, I've restored the functionality along the same lines as I originally had. This seems to get me working capture, but might break non-bayer passthrough mode: diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c index fc0036aa84d0..df336971a009 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -314,14 +314,6 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) image.phys0 = phys[0]; image.phys1 = phys[1]; - ret = ipu_cpmem_set_image(priv->idmac_ch, &image); - if (ret) - return ret; - - burst_size = (image.pix.width & 0xf) ? 8 : 16; - - ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); - /* * Check for conditions that require the IPU to handle the * data internally as generic data, aka passthrough mode: @@ -346,15 +338,29 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) passthrough_bits = 16; break; default: + burst_size = (image.pix.width & 0xf) ? 8 : 16; passthrough = (sensor_ep->bus_type != V4L2_MBUS_CSI2 && sensor_ep->bus.parallel.bus_width >= 16); passthrough_bits = 16; break; } - if (passthrough) + if (passthrough) { + ipu_cpmem_set_resolution(priv->idmac_ch, image.rect.width, + image.rect.height); + ipu_cpmem_set_stride(priv->idmac_ch, image.pix.bytesperline); + ipu_cpmem_set_buffer(priv->idmac_ch, 0, image.phys0); + ipu_cpmem_set_buffer(priv->idmac_ch, 1, image.phys1); + ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); ipu_cpmem_set_format_passthrough(priv->idmac_ch, passthrough_bits); + } else { + ret = ipu_cpmem_set_image(priv->idmac_ch, &image); + if (ret) + return ret; + + ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size); + } /* * Set the channel for the direct CSI-->memory via SMFC