[{"id":10921,"web_url":"https://patchwork.libcamera.org/comment/10921/","msgid":"<20200628065130.GA5961@pendragon.ideasonboard.com>","date":"2020-06-28T06:51:30","subject":"Re: [libcamera-devel] [PATCH v2 08/13] libcamera: ipu3: imgu: Use\n\tspecific functions to configure each sink","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sun, Jun 28, 2020 at 02:15:27AM +0200, Niklas Söderlund wrote:\n> When the IPU3 pipeline only provided streams to applications that came\n> from the ImgU it made sense to have a generic function to configure all\n> the different outputs. With the addition of the RAW stream this begins\n> to be cumbersome to read and make sense of in the PipelineHandlerIPU3\n> code. Replace the generic function that takes a specific argument for\n> which sink to configure with a specific function for each sink.\n> \n> This makes the code easier to follow as it's always clear which of the\n> ImgU sinks are being configured without knowing the content of a\n> generically named variable. It also paves way for future improvements.\n\ns/paves way/paves the way/\n\n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> ---\n> * Changes since v1\n> - s/sens/sense/\n> - Retain comment for configureVideoDevice()\n> - Inline the accessors for configureVideoDevice() in the .h file\n> - Update comment in PipelineHandlerIPU3::configure()\n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 20 +++++++--------\n> src/libcamera/pipeline/ipu3/imgu.h | 28 ++++++++++++++++++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------\n> 3 files changed, 57 insertions(+), 29 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 46d56a5d18dc3687..981239cba975f92e 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -139,18 +139,17 @@ int ImgUDevice::configureInput(const Size &size,\n> }\n> \n> /**\n> - * \\brief Configure the ImgU unit \\a id video output\n> - * \\param[in] output The ImgU output device to configure\n> + * \\brief Configure a video device on the ImgU\n> + * \\param[in] dev The video device to configure\n> + * \\param[in] pad The pad of the ImgU subdevice\n> * \\param[in] cfg The requested configuration\n> + * \\param[out] outputFormat The format set on the video device\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int ImgUDevice::configureOutput(ImgUOutput *output,\n> -\t\t\t\tconst StreamConfiguration &cfg,\n> -\t\t\t\tV4L2DeviceFormat *outputFormat)\n> +int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> +\t\t\t\t const StreamConfiguration &cfg,\n> +\t\t\t\t V4L2DeviceFormat *outputFormat)\n> {\n> -\tV4L2VideoDevice *dev = output->dev;\n> -\tunsigned int pad = output->pad;\n> -\n> \tV4L2SubdeviceFormat imguFormat = {};\n> \timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> \timguFormat.size = cfg.size;\n> @@ -160,7 +159,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> \t\treturn ret;\n> \n> \t/* No need to apply format to the stat node. */\n> -\tif (output == &stat_)\n> +\tif (dev == stat_.dev)\n> \t\treturn 0;\n> \n> \t*outputFormat = {};\n> @@ -172,7 +171,8 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\tLOG(IPU3, Debug) << \"ImgU \" << output->name << \" format = \"\n> +\tconst char *name = dev == output_.dev ? \"output\" : \"viewfinder\";\n> +\tLOG(IPU3, Debug) << \"ImgU \" << name << \" format = \"\n> \t\t\t << outputFormat->toString();\n> \n> \treturn 0;\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> index b6b08b4fef2d3a9d..5f1dbfd8f45f7924 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.h\n> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> @@ -49,9 +49,29 @@ public:\n> \t}\n> \n> \tint init(MediaDevice *media, unsigned int index);\n> +\n> \tint configureInput(const Size &size, V4L2DeviceFormat *inputFormat);\n> -\tint configureOutput(ImgUOutput *output, const StreamConfiguration &cfg,\n> -\t\t\t V4L2DeviceFormat *outputFormat);\n> +\n> +\tint configureOutput(const StreamConfiguration &cfg,\n> +\t\t\t V4L2DeviceFormat *outputFormat)\n> +\t{\n> +\t\treturn configureVideoDevice(output_.dev, PAD_OUTPUT, cfg,\n> +\t\t\t\t\t outputFormat);\n> +\t}\n> +\n> +\tint configureViewfinder(const StreamConfiguration &cfg,\n> +\t\t\t\tV4L2DeviceFormat *outputFormat)\n> +\t{\n> +\t\treturn configureVideoDevice(viewfinder_.dev, PAD_VF, cfg,\n> +\t\t\t\t\t outputFormat);\n> +\t}\n> +\n> +\tint configureStat(const StreamConfiguration &cfg,\n> +\t\t\t V4L2DeviceFormat *outputFormat)\n> +\t{\n> +\t\treturn configureVideoDevice(stat_.dev, PAD_STAT, cfg,\n> +\t\t\t\t\t outputFormat);\n> +\t}\n> \n> \tint allocateBuffers(unsigned int bufferCount);\n> \tvoid freeBuffers();\n> @@ -78,6 +98,10 @@ private:\n> \t\t const std::string &sink, unsigned int sinkPad,\n> \t\t bool enable);\n> \n> +\tint configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> +\t\t\t\t const StreamConfiguration &cfg,\n> +\t\t\t\t V4L2DeviceFormat *outputFormat);\n> +\n> \tstd::string name_;\n> \tMediaDevice *media_;\n> };\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index b41a789e8dc2a7b2..e817f842f1216a7f 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -504,19 +504,25 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t\tstream->active_ = true;\n> \t\tcfg.setStream(stream);\n> \n> -\t\t/*\n> -\t\t * The RAW still capture stream just copies buffers from the\n> -\t\t * internal queue and doesn't need any specific configuration.\n> -\t\t */\n> -\t\tif (stream->raw_) {\n> +\t\tif (stream == outStream) {\n> +\t\t\tret = imgu->configureOutput(cfg, &outputFormat);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\tcfg.stride = outputFormat.planes[0].bpl;\n> +\t\t} else if (stream == vfStream) {\n> +\t\t\tret = imgu->configureViewfinder(cfg, &outputFormat);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\tcfg.stride = outputFormat.planes[0].bpl;\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * The RAW still capture stream only uses en externel\n\ns/ en / an /\n\nand an external what ?\n\n> +\t\t\t * for the already configured CIO2 sink and doesn't need\n\nIsn't the CIO2 a source ?\n\nI know what you mean here as I know the code, but for someone who's not\nfamiliar with the IPU3 pipeline handler that would be a bit confusing.\n\n> +\t\t\t * any specific configuration of the ImgU.\n> +\t\t\t */\n> \t\t\tcfg.stride = cio2Format.planes[0].bpl;\n> -\t\t} else {\n> -\t\t\tret = imgu->configureOutput(stream->device_, cfg,\n> -\t\t\t\t\t\t &outputFormat);\n> -\t\t\tif (ret)\n> -\t\t\t\treturn ret;\n> -\n> -\t\t\tcfg.stride = outputFormat.planes[0].bpl;\n> \t\t}\n> \t}\n> \n> @@ -526,15 +532,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t * be at least one active stream in the configuration request).\n> \t */\n> \tif (!outStream->active_) {\n> -\t\tret = imgu->configureOutput(outStream->device_, config->at(0),\n> -\t\t\t\t\t &outputFormat);\n> +\t\tret = imgu->configureOutput(config->at(0), &outputFormat);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \t}\n> \n> \tif (!vfStream->active_) {\n> -\t\tret = imgu->configureOutput(vfStream->device_, config->at(0),\n> -\t\t\t\t\t &outputFormat);\n> +\t\tret = imgu->configureViewfinder(config->at(0), &outputFormat);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \t}\n> @@ -546,7 +550,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \tStreamConfiguration statCfg = {};\n> \tstatCfg.size = cio2Format.size;\n> \n> -\tret = imgu->configureOutput(&imgu->stat_, statCfg, &outputFormat);\n> +\tret = imgu->configureStat(statCfg, &outputFormat);\n> \tif (ret)\n> \t\treturn ret;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D67ABC2E66\n\tfor ;\n\tSun, 28 Jun 2020 06:51:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 326F7609C8;\n\tSun, 28 Jun 2020 08:51:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E094603B2\n\tfor ;\n\tSun, 28 Jun 2020 08:51:34 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 83A574FB;\n\tSun, 28 Jun 2020 08:51:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dlUGHm/J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593327093;\n\tbh=F8oTa/EXlK4mkd9Q05aqfmr1jU4MB09I8bHVTN8YLsk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dlUGHm/J6MquKbqmhPvNTffc4fPv0KGXnV9w2Y5uDEGlI+vNsKqCkx96shnv1uN7V\n\tadoh1nEPvh9dhQcJ4TUAWn1J/+pUN8gQSI9P2jlNrYXKmd9gm6VxdtvB5mVL6fMNwl\n\tObaXk2/+CxPWx5nmRgqXBywgklVT0FS2STUMj/3k=","Date":"Sun, 28 Jun 2020 09:51:30 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200628065130.GA5961@pendragon.ideasonboard.com>","References":"<20200628001532.2685967-1-niklas.soderlund@ragnatech.se>\n\t<20200628001532.2685967-9-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200628001532.2685967-9-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 08/13] libcamera: ipu3: imgu: Use\n\tspecific functions to configure each sink","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":10929,"web_url":"https://patchwork.libcamera.org/comment/10929/","msgid":"<20200628115645.GF1105424@oden.dyn.berto.se>","date":"2020-06-28T11:56:45","subject":"Re: [libcamera-devel] [PATCH v2 08/13] libcamera: ipu3: imgu: Use\n\tspecific functions to configure each sink","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-06-28 09:51:30 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sun, Jun 28, 2020 at 02:15:27AM +0200, Niklas Söderlund wrote:\n> > When the IPU3 pipeline only provided streams to applications that came\n> > from the ImgU it made sense to have a generic function to configure all\n> > the different outputs. With the addition of the RAW stream this begins\n> > to be cumbersome to read and make sense of in the PipelineHandlerIPU3\n> > code. Replace the generic function that takes a specific argument for\n> > which sink to configure with a specific function for each sink.\n> > \n> > This makes the code easier to follow as it's always clear which of the\n> > ImgU sinks are being configured without knowing the content of a\n> > generically named variable. It also paves way for future improvements.\n> \n> s/paves way/paves the way/\n> \n> > Signed-off-by: Niklas Söderlund \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > * Changes since v1\n> > - s/sens/sense/\n> > - Retain comment for configureVideoDevice()\n> > - Inline the accessors for configureVideoDevice() in the .h file\n> > - Update comment in PipelineHandlerIPU3::configure()\n> > ---\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 20 +++++++--------\n> > src/libcamera/pipeline/ipu3/imgu.h | 28 ++++++++++++++++++--\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------\n> > 3 files changed, 57 insertions(+), 29 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index 46d56a5d18dc3687..981239cba975f92e 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -139,18 +139,17 @@ int ImgUDevice::configureInput(const Size &size,\n> > }\n> > \n> > /**\n> > - * \\brief Configure the ImgU unit \\a id video output\n> > - * \\param[in] output The ImgU output device to configure\n> > + * \\brief Configure a video device on the ImgU\n> > + * \\param[in] dev The video device to configure\n> > + * \\param[in] pad The pad of the ImgU subdevice\n> > * \\param[in] cfg The requested configuration\n> > + * \\param[out] outputFormat The format set on the video device\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > -int ImgUDevice::configureOutput(ImgUOutput *output,\n> > -\t\t\t\tconst StreamConfiguration &cfg,\n> > -\t\t\t\tV4L2DeviceFormat *outputFormat)\n> > +int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> > +\t\t\t\t const StreamConfiguration &cfg,\n> > +\t\t\t\t V4L2DeviceFormat *outputFormat)\n> > {\n> > -\tV4L2VideoDevice *dev = output->dev;\n> > -\tunsigned int pad = output->pad;\n> > -\n> > \tV4L2SubdeviceFormat imguFormat = {};\n> > \timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> > \timguFormat.size = cfg.size;\n> > @@ -160,7 +159,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> > \t\treturn ret;\n> > \n> > \t/* No need to apply format to the stat node. */\n> > -\tif (output == &stat_)\n> > +\tif (dev == stat_.dev)\n> > \t\treturn 0;\n> > \n> > \t*outputFormat = {};\n> > @@ -172,7 +171,8 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> > -\tLOG(IPU3, Debug) << \"ImgU \" << output->name << \" format = \"\n> > +\tconst char *name = dev == output_.dev ? \"output\" : \"viewfinder\";\n> > +\tLOG(IPU3, Debug) << \"ImgU \" << name << \" format = \"\n> > \t\t\t << outputFormat->toString();\n> > \n> > \treturn 0;\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> > index b6b08b4fef2d3a9d..5f1dbfd8f45f7924 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.h\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> > @@ -49,9 +49,29 @@ public:\n> > \t}\n> > \n> > \tint init(MediaDevice *media, unsigned int index);\n> > +\n> > \tint configureInput(const Size &size, V4L2DeviceFormat *inputFormat);\n> > -\tint configureOutput(ImgUOutput *output, const StreamConfiguration &cfg,\n> > -\t\t\t V4L2DeviceFormat *outputFormat);\n> > +\n> > +\tint configureOutput(const StreamConfiguration &cfg,\n> > +\t\t\t V4L2DeviceFormat *outputFormat)\n> > +\t{\n> > +\t\treturn configureVideoDevice(output_.dev, PAD_OUTPUT, cfg,\n> > +\t\t\t\t\t outputFormat);\n> > +\t}\n> > +\n> > +\tint configureViewfinder(const StreamConfiguration &cfg,\n> > +\t\t\t\tV4L2DeviceFormat *outputFormat)\n> > +\t{\n> > +\t\treturn configureVideoDevice(viewfinder_.dev, PAD_VF, cfg,\n> > +\t\t\t\t\t outputFormat);\n> > +\t}\n> > +\n> > +\tint configureStat(const StreamConfiguration &cfg,\n> > +\t\t\t V4L2DeviceFormat *outputFormat)\n> > +\t{\n> > +\t\treturn configureVideoDevice(stat_.dev, PAD_STAT, cfg,\n> > +\t\t\t\t\t outputFormat);\n> > +\t}\n> > \n> > \tint allocateBuffers(unsigned int bufferCount);\n> > \tvoid freeBuffers();\n> > @@ -78,6 +98,10 @@ private:\n> > \t\t const std::string &sink, unsigned int sinkPad,\n> > \t\t bool enable);\n> > \n> > +\tint configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> > +\t\t\t\t const StreamConfiguration &cfg,\n> > +\t\t\t\t V4L2DeviceFormat *outputFormat);\n> > +\n> > \tstd::string name_;\n> > \tMediaDevice *media_;\n> > };\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index b41a789e8dc2a7b2..e817f842f1216a7f 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -504,19 +504,25 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \t\tstream->active_ = true;\n> > \t\tcfg.setStream(stream);\n> > \n> > -\t\t/*\n> > -\t\t * The RAW still capture stream just copies buffers from the\n> > -\t\t * internal queue and doesn't need any specific configuration.\n> > -\t\t */\n> > -\t\tif (stream->raw_) {\n> > +\t\tif (stream == outStream) {\n> > +\t\t\tret = imgu->configureOutput(cfg, &outputFormat);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\t\t\tcfg.stride = outputFormat.planes[0].bpl;\n> > +\t\t} else if (stream == vfStream) {\n> > +\t\t\tret = imgu->configureViewfinder(cfg, &outputFormat);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\t\t\tcfg.stride = outputFormat.planes[0].bpl;\n> > +\t\t} else {\n> > +\t\t\t/*\n> > +\t\t\t * The RAW still capture stream only uses en externel\n> \n> s/ en / an /\n> \n> and an external what ?\n> \n> > +\t\t\t * for the already configured CIO2 sink and doesn't need\n> \n> Isn't the CIO2 a source ?\n> \n> I know what you mean here as I know the code, but for someone who's not\n> familiar with the IPU3 pipeline handler that would be a bit confusing.\n\nHow about?\n\n\t/*\n\t * The RAW stream is configured as part of the CIO2 and\n\t * no configuration is needed for the ImgU.\n\t */\n\n> \n> > +\t\t\t * any specific configuration of the ImgU.\n> > +\t\t\t */\n> > \t\t\tcfg.stride = cio2Format.planes[0].bpl;\n> > -\t\t} else {\n> > -\t\t\tret = imgu->configureOutput(stream->device_, cfg,\n> > -\t\t\t\t\t\t &outputFormat);\n> > -\t\t\tif (ret)\n> > -\t\t\t\treturn ret;\n> > -\n> > -\t\t\tcfg.stride = outputFormat.planes[0].bpl;\n> > \t\t}\n> > \t}\n> > \n> > @@ -526,15 +532,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \t * be at least one active stream in the configuration request).\n> > \t */\n> > \tif (!outStream->active_) {\n> > -\t\tret = imgu->configureOutput(outStream->device_, config->at(0),\n> > -\t\t\t\t\t &outputFormat);\n> > +\t\tret = imgu->configureOutput(config->at(0), &outputFormat);\n> > \t\tif (ret)\n> > \t\t\treturn ret;\n> > \t}\n> > \n> > \tif (!vfStream->active_) {\n> > -\t\tret = imgu->configureOutput(vfStream->device_, config->at(0),\n> > -\t\t\t\t\t &outputFormat);\n> > +\t\tret = imgu->configureViewfinder(config->at(0), &outputFormat);\n> > \t\tif (ret)\n> > \t\t\treturn ret;\n> > \t}\n> > @@ -546,7 +550,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \tStreamConfiguration statCfg = {};\n> > \tstatCfg.size = cio2Format.size;\n> > \n> > -\tret = imgu->configureOutput(&imgu->stat_, statCfg, &outputFormat);\n> > +\tret = imgu->configureStat(statCfg, &outputFormat);\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1E451C2E69\n\tfor ;\n\tSun, 28 Jun 2020 11:56:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 81019609C9;\n\tSun, 28 Jun 2020 13:56:49 +0200 (CEST)","from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7C12603B2\n\tfor ;\n\tSun, 28 Jun 2020 13:56:47 +0200 (CEST)","by mail-lf1-x144.google.com with SMTP id s16so2062150lfp.12\n\tfor ;\n\tSun, 28 Jun 2020 04:56:47 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tv5sm225776ljd.10.2020.06.28.04.56.46\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 28 Jun 2020 04:56:46 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"UKovAyuB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=xTA/9Uf75A/7UWdav6s/lCCMBClFl8pA9x/8zTnjjyI=;\n\tb=UKovAyuBU0La2vmH6e2w4prQE/zlre1BHsQp4h4LuFPDTOItv6KdPkv3KehKA2jNXN\n\tMy937ExCD1CxDe9+JLBmI2SgQRGjb49aDQr+OpicBJXUOoK0LjmCaGKSYKu0b1nH2IBs\n\tvHoBvDTbuVvcIDIsTQa/taVthbqKq2OeOOfd27STYDS4MNDqsVa8UiB6y2Rg1Ktk6T4m\n\tW14X3vb/6SAV2ptQl/Lk2QB/uscD3YzTa5wzI7o9ckZGjr3jJkIzre9ogSaDaEI5/2Jj\n\taCiscgYNeOiu2GK8UkgICBI1FYG9cDybnuCNj04XXPU9Nll4S3e3oR2J2cNMO6Uzom4D\n\t8IyA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=xTA/9Uf75A/7UWdav6s/lCCMBClFl8pA9x/8zTnjjyI=;\n\tb=QHTh/pTrCDGMyYv2/buCFsmrzpdfj3MdU4jh1Vh1R0lydEl6OSjJ5qZM8BX9qQxLbD\n\tYH/qYJab+UxhDuD5XevWq6xFvQGeEAsR3OtmkDZljgfFwt/6GO99QxkBhkzuTUrz8/sc\n\thOsX5YevTOXzkjJIWcK+EtQs4AHpQrvlI8QL2W8tw/i1dGFMDzqAehrH6TTtoDUEJgV+\n\t6AGXCuPqWnJHpwtlszED2T40UuQhIFyYrB1Yk9kGQlcrTSeXoP5FDC9Q4ZJ6Tl9Aq7Zv\n\tEZdSEiY8epZ4F7QdnNJCQNF0jFosk+ICfxsdLtwaqGjO80lt06WaSBYi+kQ+McS5yePA\n\tA2Gg==","X-Gm-Message-State":"AOAM532v1yCSL48BVBw71DGy7GTjvRKATiIXodrDxmSgnlnN22f9o5o3\n\t/GI3rnsLhIJDRAVDL7m7X5qJLg==","X-Google-Smtp-Source":"ABdhPJyoHDXdmhYdFRru0nVnNF/wANZb6KZgbRp34tgs/HD0uZNH5N3l2EueYwTxFacEIUO+XGv1lw==","X-Received":"by 2002:a19:7e09:: with SMTP id z9mr6707637lfc.69.1593345406746; \n\tSun, 28 Jun 2020 04:56:46 -0700 (PDT)","Date":"Sun, 28 Jun 2020 13:56:45 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200628115645.GF1105424@oden.dyn.berto.se>","References":"<20200628001532.2685967-1-niklas.soderlund@ragnatech.se>\n\t<20200628001532.2685967-9-niklas.soderlund@ragnatech.se>\n\t<20200628065130.GA5961@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200628065130.GA5961@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 08/13] libcamera: ipu3: imgu: Use\n\tspecific functions to configure each sink","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":10930,"web_url":"https://patchwork.libcamera.org/comment/10930/","msgid":"<20200628115846.GQ6954@pendragon.ideasonboard.com>","date":"2020-06-28T11:58:46","subject":"Re: [libcamera-devel] [PATCH v2 08/13] libcamera: ipu3: imgu: Use\n\tspecific functions to configure each sink","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Sun, Jun 28, 2020 at 01:56:45PM +0200, Niklas Söderlund wrote:\n> On 2020-06-28 09:51:30 +0300, Laurent Pinchart wrote:\n> > On Sun, Jun 28, 2020 at 02:15:27AM +0200, Niklas Söderlund wrote:\n> > > When the IPU3 pipeline only provided streams to applications that came\n> > > from the ImgU it made sense to have a generic function to configure all\n> > > the different outputs. With the addition of the RAW stream this begins\n> > > to be cumbersome to read and make sense of in the PipelineHandlerIPU3\n> > > code. Replace the generic function that takes a specific argument for\n> > > which sink to configure with a specific function for each sink.\n> > > \n> > > This makes the code easier to follow as it's always clear which of the\n> > > ImgU sinks are being configured without knowing the content of a\n> > > generically named variable. It also paves way for future improvements.\n> > \n> > s/paves way/paves the way/\n> > \n> > > Signed-off-by: Niklas Söderlund \n> > > Reviewed-by: Jacopo Mondi \n> > > ---\n> > > * Changes since v1\n> > > - s/sens/sense/\n> > > - Retain comment for configureVideoDevice()\n> > > - Inline the accessors for configureVideoDevice() in the .h file\n> > > - Update comment in PipelineHandlerIPU3::configure()\n> > > ---\n> > > src/libcamera/pipeline/ipu3/imgu.cpp | 20 +++++++--------\n> > > src/libcamera/pipeline/ipu3/imgu.h | 28 ++++++++++++++++++--\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------\n> > > 3 files changed, 57 insertions(+), 29 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > index 46d56a5d18dc3687..981239cba975f92e 100644\n> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > @@ -139,18 +139,17 @@ int ImgUDevice::configureInput(const Size &size,\n> > > }\n> > > \n> > > /**\n> > > - * \\brief Configure the ImgU unit \\a id video output\n> > > - * \\param[in] output The ImgU output device to configure\n> > > + * \\brief Configure a video device on the ImgU\n> > > + * \\param[in] dev The video device to configure\n> > > + * \\param[in] pad The pad of the ImgU subdevice\n> > > * \\param[in] cfg The requested configuration\n> > > + * \\param[out] outputFormat The format set on the video device\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > > -int ImgUDevice::configureOutput(ImgUOutput *output,\n> > > -\t\t\t\tconst StreamConfiguration &cfg,\n> > > -\t\t\t\tV4L2DeviceFormat *outputFormat)\n> > > +int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> > > +\t\t\t\t const StreamConfiguration &cfg,\n> > > +\t\t\t\t V4L2DeviceFormat *outputFormat)\n> > > {\n> > > -\tV4L2VideoDevice *dev = output->dev;\n> > > -\tunsigned int pad = output->pad;\n> > > -\n> > > \tV4L2SubdeviceFormat imguFormat = {};\n> > > \timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> > > \timguFormat.size = cfg.size;\n> > > @@ -160,7 +159,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> > > \t\treturn ret;\n> > > \n> > > \t/* No need to apply format to the stat node. */\n> > > -\tif (output == &stat_)\n> > > +\tif (dev == stat_.dev)\n> > > \t\treturn 0;\n> > > \n> > > \t*outputFormat = {};\n> > > @@ -172,7 +171,8 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > \n> > > -\tLOG(IPU3, Debug) << \"ImgU \" << output->name << \" format = \"\n> > > +\tconst char *name = dev == output_.dev ? \"output\" : \"viewfinder\";\n> > > +\tLOG(IPU3, Debug) << \"ImgU \" << name << \" format = \"\n> > > \t\t\t << outputFormat->toString();\n> > > \n> > > \treturn 0;\n> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> > > index b6b08b4fef2d3a9d..5f1dbfd8f45f7924 100644\n> > > --- a/src/libcamera/pipeline/ipu3/imgu.h\n> > > +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> > > @@ -49,9 +49,29 @@ public:\n> > > \t}\n> > > \n> > > \tint init(MediaDevice *media, unsigned int index);\n> > > +\n> > > \tint configureInput(const Size &size, V4L2DeviceFormat *inputFormat);\n> > > -\tint configureOutput(ImgUOutput *output, const StreamConfiguration &cfg,\n> > > -\t\t\t V4L2DeviceFormat *outputFormat);\n> > > +\n> > > +\tint configureOutput(const StreamConfiguration &cfg,\n> > > +\t\t\t V4L2DeviceFormat *outputFormat)\n> > > +\t{\n> > > +\t\treturn configureVideoDevice(output_.dev, PAD_OUTPUT, cfg,\n> > > +\t\t\t\t\t outputFormat);\n> > > +\t}\n> > > +\n> > > +\tint configureViewfinder(const StreamConfiguration &cfg,\n> > > +\t\t\t\tV4L2DeviceFormat *outputFormat)\n> > > +\t{\n> > > +\t\treturn configureVideoDevice(viewfinder_.dev, PAD_VF, cfg,\n> > > +\t\t\t\t\t outputFormat);\n> > > +\t}\n> > > +\n> > > +\tint configureStat(const StreamConfiguration &cfg,\n> > > +\t\t\t V4L2DeviceFormat *outputFormat)\n> > > +\t{\n> > > +\t\treturn configureVideoDevice(stat_.dev, PAD_STAT, cfg,\n> > > +\t\t\t\t\t outputFormat);\n> > > +\t}\n> > > \n> > > \tint allocateBuffers(unsigned int bufferCount);\n> > > \tvoid freeBuffers();\n> > > @@ -78,6 +98,10 @@ private:\n> > > \t\t const std::string &sink, unsigned int sinkPad,\n> > > \t\t bool enable);\n> > > \n> > > +\tint configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> > > +\t\t\t\t const StreamConfiguration &cfg,\n> > > +\t\t\t\t V4L2DeviceFormat *outputFormat);\n> > > +\n> > > \tstd::string name_;\n> > > \tMediaDevice *media_;\n> > > };\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index b41a789e8dc2a7b2..e817f842f1216a7f 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -504,19 +504,25 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > \t\tstream->active_ = true;\n> > > \t\tcfg.setStream(stream);\n> > > \n> > > -\t\t/*\n> > > -\t\t * The RAW still capture stream just copies buffers from the\n> > > -\t\t * internal queue and doesn't need any specific configuration.\n> > > -\t\t */\n> > > -\t\tif (stream->raw_) {\n> > > +\t\tif (stream == outStream) {\n> > > +\t\t\tret = imgu->configureOutput(cfg, &outputFormat);\n> > > +\t\t\tif (ret)\n> > > +\t\t\t\treturn ret;\n> > > +\n> > > +\t\t\tcfg.stride = outputFormat.planes[0].bpl;\n> > > +\t\t} else if (stream == vfStream) {\n> > > +\t\t\tret = imgu->configureViewfinder(cfg, &outputFormat);\n> > > +\t\t\tif (ret)\n> > > +\t\t\t\treturn ret;\n> > > +\n> > > +\t\t\tcfg.stride = outputFormat.planes[0].bpl;\n> > > +\t\t} else {\n> > > +\t\t\t/*\n> > > +\t\t\t * The RAW still capture stream only uses en externel\n> > \n> > s/ en / an /\n> > \n> > and an external what ?\n> > \n> > > +\t\t\t * for the already configured CIO2 sink and doesn't need\n> > \n> > Isn't the CIO2 a source ?\n> > \n> > I know what you mean here as I know the code, but for someone who's not\n> > familiar with the IPU3 pipeline handler that would be a bit confusing.\n> \n> How about?\n> \n> \t/*\n> \t * The RAW stream is configured as part of the CIO2 and\n> \t * no configuration is needed for the ImgU.\n> \t */\n\nReviewed-by: Laurent Pinchart \n\n> > > +\t\t\t * any specific configuration of the ImgU.\n> > > +\t\t\t */\n> > > \t\t\tcfg.stride = cio2Format.planes[0].bpl;\n> > > -\t\t} else {\n> > > -\t\t\tret = imgu->configureOutput(stream->device_, cfg,\n> > > -\t\t\t\t\t\t &outputFormat);\n> > > -\t\t\tif (ret)\n> > > -\t\t\t\treturn ret;\n> > > -\n> > > -\t\t\tcfg.stride = outputFormat.planes[0].bpl;\n> > > \t\t}\n> > > \t}\n> > > \n> > > @@ -526,15 +532,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > \t * be at least one active stream in the configuration request).\n> > > \t */\n> > > \tif (!outStream->active_) {\n> > > -\t\tret = imgu->configureOutput(outStream->device_, config->at(0),\n> > > -\t\t\t\t\t &outputFormat);\n> > > +\t\tret = imgu->configureOutput(config->at(0), &outputFormat);\n> > > \t\tif (ret)\n> > > \t\t\treturn ret;\n> > > \t}\n> > > \n> > > \tif (!vfStream->active_) {\n> > > -\t\tret = imgu->configureOutput(vfStream->device_, config->at(0),\n> > > -\t\t\t\t\t &outputFormat);\n> > > +\t\tret = imgu->configureViewfinder(config->at(0), &outputFormat);\n> > > \t\tif (ret)\n> > > \t\t\treturn ret;\n> > > \t}\n> > > @@ -546,7 +550,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > \tStreamConfiguration statCfg = {};\n> > > \tstatCfg.size = cio2Format.size;\n> > > \n> > > -\tret = imgu->configureOutput(&imgu->stat_, statCfg, &outputFormat);\n> > > +\tret = imgu->configureStat(statCfg, &outputFormat);\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8EF84C2E66\n\tfor ;\n\tSun, 28 Jun 2020 11:58:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0EE2A609C9;\n\tSun, 28 Jun 2020 13:58:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01D61603B2\n\tfor ;\n\tSun, 28 Jun 2020 13:58:50 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 71D3B4FB;\n\tSun, 28 Jun 2020 13:58:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pWr0T03S\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593345530;\n\tbh=yWkEa//Tv2iE2T9jemJKRUqo4lHJwVw3vAZAHrLw0ME=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pWr0T03SUSa/QrAV5O4hTsuMrXwsU5oxXCtam1IvyFPFGImKK5ambdVGjQXRop++9\n\tQ34HM2ldCy38AzpCcxuOdx2fjWSrlKpdw2+FS9Qcrq885mHE4mQF5Mc6mA6WfFxKf3\n\tM88b6zQhdNFq4FxMNRxIt0J5u3yRH//RL8tEBHKw=","Date":"Sun, 28 Jun 2020 14:58:46 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200628115846.GQ6954@pendragon.ideasonboard.com>","References":"<20200628001532.2685967-1-niklas.soderlund@ragnatech.se>\n\t<20200628001532.2685967-9-niklas.soderlund@ragnatech.se>\n\t<20200628065130.GA5961@pendragon.ideasonboard.com>\n\t<20200628115645.GF1105424@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200628115645.GF1105424@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v2 08/13] libcamera: ipu3: imgu: Use\n\tspecific functions to configure each sink","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]