[{"id":10896,"web_url":"https://patchwork.libcamera.org/comment/10896/","msgid":"<20200627100248.coibptdmw4lirea5@uno.localdomain>","date":"2020-06-27T10:02:48","subject":"Re: [libcamera-devel] [PATCH 08/13] libcamera: ipu3: imgu: Use\n\tspecific functions to configure each sink","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Sat, Jun 27, 2020 at 05:00:38AM +0200, Niklas Söderlund wrote:\n> When the IPU3 pipeline only provided streams to applications that came\n> from the ImgU it made sens to have a generic function to configure all\n\nsense\n\n> the different outputs. With the addition of the RAW stream this begins\n> to be cumbersome to read and make sens of in the PipelineHandlerIPU3\n\nsense\n\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> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++---------\n> src/libcamera/pipeline/ipu3/imgu.h | 11 +++++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------\n> 3 files changed, 53 insertions(+), 30 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 5d11539605f26303..d54e844d9bfc5147 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -139,19 +139,28 @@ int ImgUDevice::configureInput(const Size &size,\n> \treturn 0;\n> }\n>\n> -/**\n> - * \\brief Configure the ImgU unit \\a id video output\n> - * \\param[in] output The ImgU output device to configure\n> - * \\param[in] cfg The requested configuration\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> +int ImgUDevice::configureOutput(const StreamConfiguration &cfg,\n> \t\t\t\tV4L2DeviceFormat *outputFormat)\n> {\n> -\tV4L2VideoDevice *dev = output->dev;\n> -\tunsigned int pad = output->pad;\n> +\treturn configureVideoDevice(output_.dev, PAD_OUTPUT, cfg, outputFormat);\n> +}\n>\n> +int ImgUDevice::configureViewfinder(const StreamConfiguration &cfg,\n> +\t\t\t\t V4L2DeviceFormat *outputFormat)\n\nNo documentation but no complains in the build. Is doxygen ignoreing\nthis file ?\n\n> +{\n> +\treturn configureVideoDevice(viewfinder_.dev, PAD_VF, cfg, outputFormat);\n> +}\n> +\n> +int ImgUDevice::configureStat(const StreamConfiguration &cfg,\n> +\t\t\t V4L2DeviceFormat *outputFormat)\n> +{\n> +\treturn configureVideoDevice(stat_.dev, PAD_STAT, cfg, outputFormat);\n> +}\n> +\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> \tV4L2SubdeviceFormat imguFormat = {};\n> \timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> \timguFormat.size = cfg.size;\n> @@ -161,7 +170,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> @@ -173,7 +182,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..f8fd54089753b40e 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.h\n> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> @@ -49,9 +49,14 @@ 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> +\tint configureOutput(const StreamConfiguration &cfg,\n> \t\t\t V4L2DeviceFormat *outputFormat);\n> +\tint configureViewfinder(const StreamConfiguration &cfg,\n> +\t\t\t\tV4L2DeviceFormat *outputFormat);\n> +\tint configureStat(const StreamConfiguration &cfg,\n> +\t\t\t V4L2DeviceFormat *outputFormat);\n>\n> \tint allocateBuffers(unsigned int bufferCount);\n> \tvoid freeBuffers();\n> @@ -78,6 +83,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..ae7a01b81dacf498 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 just copies buffers from\n> +\t\t\t * the internal queue and doesn't need any specific\n> +\t\t\t * configuration.\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> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 AE88AC2E69\n\tfor ;\n\tSat, 27 Jun 2020 09:59:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CC54609C8;\n\tSat, 27 Jun 2020 11:59:19 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81F1C603B5\n\tfor ;\n\tSat, 27 Jun 2020 11:59:17 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id D675D60003;\n\tSat, 27 Jun 2020 09:59:16 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sat, 27 Jun 2020 12:02:48 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200627100248.coibptdmw4lirea5@uno.localdomain>","References":"<20200627030043.2088585-1-niklas.soderlund@ragnatech.se>\n\t<20200627030043.2088585-9-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200627030043.2088585-9-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 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":10903,"web_url":"https://patchwork.libcamera.org/comment/10903/","msgid":"<20200627104016.GD911447@oden.dyn.berto.se>","date":"2020-06-27T10:40:16","subject":"Re: [libcamera-devel] [PATCH 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 Jacopo,\n\nThanks for your feedback.\n\nOn 2020-06-27 12:02:48 +0200, Jacopo Mondi wrote:\n> On Sat, Jun 27, 2020 at 05:00:38AM +0200, Niklas Söderlund wrote:\n> > When the IPU3 pipeline only provided streams to applications that came\n> > from the ImgU it made sens to have a generic function to configure all\n> \n> sense\n> \n> > the different outputs. With the addition of the RAW stream this begins\n> > to be cumbersome to read and make sens of in the PipelineHandlerIPU3\n> \n> sense\n> \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> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++---------\n> > src/libcamera/pipeline/ipu3/imgu.h | 11 +++++++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------\n> > 3 files changed, 53 insertions(+), 30 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index 5d11539605f26303..d54e844d9bfc5147 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -139,19 +139,28 @@ int ImgUDevice::configureInput(const Size &size,\n> > \treturn 0;\n> > }\n> >\n> > -/**\n> > - * \\brief Configure the ImgU unit \\a id video output\n> > - * \\param[in] output The ImgU output device to configure\n> > - * \\param[in] cfg The requested configuration\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> > +int ImgUDevice::configureOutput(const StreamConfiguration &cfg,\n> > \t\t\t\tV4L2DeviceFormat *outputFormat)\n> > {\n> > -\tV4L2VideoDevice *dev = output->dev;\n> > -\tunsigned int pad = output->pad;\n> > +\treturn configureVideoDevice(output_.dev, PAD_OUTPUT, cfg, outputFormat);\n> > +}\n> >\n> > +int ImgUDevice::configureViewfinder(const StreamConfiguration &cfg,\n> > +\t\t\t\t V4L2DeviceFormat *outputFormat)\n> \n> No documentation but no complains in the build. Is doxygen ignoreing\n> this file ?\n\nWe don't run Doxygen for pipeline handlers.\n\n> \n> > +{\n> > +\treturn configureVideoDevice(viewfinder_.dev, PAD_VF, cfg, outputFormat);\n> > +}\n> > +\n> > +int ImgUDevice::configureStat(const StreamConfiguration &cfg,\n> > +\t\t\t V4L2DeviceFormat *outputFormat)\n> > +{\n> > +\treturn configureVideoDevice(stat_.dev, PAD_STAT, cfg, outputFormat);\n> > +}\n> > +\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> > \tV4L2SubdeviceFormat imguFormat = {};\n> > \timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> > \timguFormat.size = cfg.size;\n> > @@ -161,7 +170,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> > @@ -173,7 +182,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..f8fd54089753b40e 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.h\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> > @@ -49,9 +49,14 @@ 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> > +\tint configureOutput(const StreamConfiguration &cfg,\n> > \t\t\t V4L2DeviceFormat *outputFormat);\n> > +\tint configureViewfinder(const StreamConfiguration &cfg,\n> > +\t\t\t\tV4L2DeviceFormat *outputFormat);\n> > +\tint configureStat(const StreamConfiguration &cfg,\n> > +\t\t\t V4L2DeviceFormat *outputFormat);\n> >\n> > \tint allocateBuffers(unsigned int bufferCount);\n> > \tvoid freeBuffers();\n> > @@ -78,6 +83,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..ae7a01b81dacf498 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 just copies buffers from\n> > +\t\t\t * the internal queue and doesn't need any specific\n> > +\t\t\t * configuration.\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> > 2.27.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 B00D9C2E66\n\tfor ;\n\tSat, 27 Jun 2020 10:40:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A742609D6;\n\tSat, 27 Jun 2020 12:40:20 +0200 (CEST)","from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D45E3609C2\n\tfor ;\n\tSat, 27 Jun 2020 12:40:18 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id 9so12876429ljv.5\n\tfor ;\n\tSat, 27 Jun 2020 03:40:18 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tu7sm8628132lfi.45.2020.06.27.03.40.17\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 27 Jun 2020 03:40:17 -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=\"NuMjq56g\"; 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=2CdYMdZvSgBL5Id7r/XOh3OkPqfgUroFn+mV9uds/Hs=;\n\tb=NuMjq56gjvkHQ+YL8EJ/5TpRxntDPr6H/0L784agmR+jEPQN6pcEIFqsE/k5acUNHW\n\tuGcfV+BxWVQz2VoCeGNODSk2G4PC59ZW5sqPXfomSBaOcWOBzaNswoWhaSgAXoWQ02KP\n\tvriH6N05td2M4Ppw31CyF1M0b2DNt4V18G11n+rdSX+G6jwCrFTlYB3czCH57V3FiKv3\n\talhLtxWmQz0Ha/ZeKEZAEmk9XsUVdDZm0ycaI7du/sG1KP8iXexJBrHSFboGDnhB5Q6G\n\tC26aqENr6b4oV0U8Ua/LTRj71BHu/iRipnifqHi2ThbwR+LQLYqqdgH6lRLlJvYKuxcc\n\t3JXg==","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=2CdYMdZvSgBL5Id7r/XOh3OkPqfgUroFn+mV9uds/Hs=;\n\tb=hQbL0IHvFsSmzSD9/yDyjYKwWQCxPFTZm+c8NF6Gu0UIIHvJX0fgFjXVq2u9k4DFho\n\tb10bF56578Hbrj6aFmEI9z9IDHd7ddqong2A9y444iO8MPhKrIFo1HHOUVrAn8AVSRkL\n\tZI/ITjsKSmtl7VdNUuP+Kz3//Dgh4oQQgXzHaNa4eb1szAXHD+HR3mnIEuvkCcDK2gQT\n\tqmDEZsDYZoSVqJVcXmvg7m8wRCTWZKee1KS/8gBs0kO7Lsn3QIorA+6utTXO6sqZsken\n\tcLVGxMyYzFOKT7T2QrA2Jcrs/+DFjQvygO8JGY350V1SNdpZxqaZfjBzbgZQYwlkSTwK\n\t6v5A==","X-Gm-Message-State":"AOAM5317WqEsPQKGqbWqT9ZV4LDqpxny6XJDkYZoosXs5x/0XTXXA0tO\n\tW/RN3++bfeRObr3tXEcim3BC+rTyItY=","X-Google-Smtp-Source":"ABdhPJzYg8KKW1NpOlZ1w6zBpNY6kfaYnL43mGMjsK1l002snYzGeY7eV01a+J7Rq2R4RIAu7jjdDw==","X-Received":"by 2002:a05:651c:213:: with SMTP id\n\ty19mr3312984ljn.232.1593254418098; \n\tSat, 27 Jun 2020 03:40:18 -0700 (PDT)","Date":"Sat, 27 Jun 2020 12:40:16 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200627104016.GD911447@oden.dyn.berto.se>","References":"<20200627030043.2088585-1-niklas.soderlund@ragnatech.se>\n\t<20200627030043.2088585-9-niklas.soderlund@ragnatech.se>\n\t<20200627100248.coibptdmw4lirea5@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200627100248.coibptdmw4lirea5@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 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":10906,"web_url":"https://patchwork.libcamera.org/comment/10906/","msgid":"<20200627161053.GC5966@pendragon.ideasonboard.com>","date":"2020-06-27T16:10:53","subject":"Re: [libcamera-devel] [PATCH 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 Sat, Jun 27, 2020 at 05:00:38AM +0200, Niklas Söderlund wrote:\n> When the IPU3 pipeline only provided streams to applications that came\n> from the ImgU it made sens 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 sens 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> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++---------\n> src/libcamera/pipeline/ipu3/imgu.h | 11 +++++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------\n> 3 files changed, 53 insertions(+), 30 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 5d11539605f26303..d54e844d9bfc5147 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -139,19 +139,28 @@ int ImgUDevice::configureInput(const Size &size,\n> \treturn 0;\n> }\n> \n> -/**\n> - * \\brief Configure the ImgU unit \\a id video output\n> - * \\param[in] output The ImgU output device to configure\n> - * \\param[in] cfg The requested configuration\n> - * \\return 0 on success or a negative error code otherwise\n> - */\n\nShould we retain the documentation ? It may not be directly clear from\nthe function prototype what outputFormat is used for.\n\n> -int ImgUDevice::configureOutput(ImgUOutput *output,\n> -\t\t\t\tconst StreamConfiguration &cfg,\n> +int ImgUDevice::configureOutput(const StreamConfiguration &cfg,\n> \t\t\t\tV4L2DeviceFormat *outputFormat)\n> {\n> -\tV4L2VideoDevice *dev = output->dev;\n> -\tunsigned int pad = output->pad;\n> +\treturn configureVideoDevice(output_.dev, PAD_OUTPUT, cfg, outputFormat);\n> +}\n> \n> +int ImgUDevice::configureViewfinder(const StreamConfiguration &cfg,\n> +\t\t\t\t V4L2DeviceFormat *outputFormat)\n> +{\n> +\treturn configureVideoDevice(viewfinder_.dev, PAD_VF, cfg, outputFormat);\n> +}\n> +\n> +int ImgUDevice::configureStat(const StreamConfiguration &cfg,\n> +\t\t\t V4L2DeviceFormat *outputFormat)\n> +{\n> +\treturn configureVideoDevice(stat_.dev, PAD_STAT, cfg, outputFormat);\n> +}\n\nShould these functions be inlined in the .h file for efficiency ?\n\n> +\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\nWhen I read the commit message I got worried you would duplicate quite a\nbit of code, I'm relieved now :-)\n\n> \tV4L2SubdeviceFormat imguFormat = {};\n> \timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> \timguFormat.size = cfg.size;\n> @@ -161,7 +170,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> @@ -173,7 +182,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..f8fd54089753b40e 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.h\n> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> @@ -49,9 +49,14 @@ 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> +\tint configureOutput(const StreamConfiguration &cfg,\n> \t\t\t V4L2DeviceFormat *outputFormat);\n> +\tint configureViewfinder(const StreamConfiguration &cfg,\n> +\t\t\t\tV4L2DeviceFormat *outputFormat);\n> +\tint configureStat(const StreamConfiguration &cfg,\n> +\t\t\t V4L2DeviceFormat *outputFormat);\n> \n> \tint allocateBuffers(unsigned int bufferCount);\n> \tvoid freeBuffers();\n> @@ -78,6 +83,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..ae7a01b81dacf498 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 just copies buffers from\n> +\t\t\t * the internal queue and doesn't need any specific\n> +\t\t\t * configuration.\n\ns/internal queue/internal CIO2 queue/ ?\ns/configuration/configuration of the ImgU/ ?\n\nAnd we don't copy buffers anymore, do we ?\n\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\nI'm not sure I find the result more readable, but I'm also not opposed\nto it.\n\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 E001FC2E66\n\tfor ;\n\tSat, 27 Jun 2020 16:10:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69B68609DD;\n\tSat, 27 Jun 2020 18:10:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0499609C2\n\tfor ;\n\tSat, 27 Jun 2020 18:10:56 +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 0EC6A576;\n\tSat, 27 Jun 2020 18:10:55 +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=\"VOzF8zrd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593274256;\n\tbh=JNZgWNsa1tsuaitRjLqHuMvBD1wd8KwTKVzwpVQAr/U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VOzF8zrd6vfW8PxufcYpXO/s6FBwHZTislEqtBulS7oLuQREi6LzWjX2L2A/pNPHd\n\tZ1kFt3F6OebjTETz4Ylbyu1fSj8pWxBJ9FOHEREK8bliOhWVVEBbTHN4s0ACwkjnjl\n\tvK2DIclDxpqlS3LN2ktaXfRrujw0z/43A1ditIgE=","Date":"Sat, 27 Jun 2020 19:10:53 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200627161053.GC5966@pendragon.ideasonboard.com>","References":"<20200627030043.2088585-1-niklas.soderlund@ragnatech.se>\n\t<20200627030043.2088585-9-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200627030043.2088585-9-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 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":10916,"web_url":"https://patchwork.libcamera.org/comment/10916/","msgid":"<20200627223924.GA1105424@oden.dyn.berto.se>","date":"2020-06-27T22:39:24","subject":"Re: [libcamera-devel] [PATCH 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-27 19:10:53 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sat, Jun 27, 2020 at 05:00:38AM +0200, Niklas Söderlund wrote:\n> > When the IPU3 pipeline only provided streams to applications that came\n> > from the ImgU it made sens 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 sens 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> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++++++++++++++---------\n> > src/libcamera/pipeline/ipu3/imgu.h | 11 +++++++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 +++++++++++++++-------------\n> > 3 files changed, 53 insertions(+), 30 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index 5d11539605f26303..d54e844d9bfc5147 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -139,19 +139,28 @@ int ImgUDevice::configureInput(const Size &size,\n> > \treturn 0;\n> > }\n> > \n> > -/**\n> > - * \\brief Configure the ImgU unit \\a id video output\n> > - * \\param[in] output The ImgU output device to configure\n> > - * \\param[in] cfg The requested configuration\n> > - * \\return 0 on success or a negative error code otherwise\n> > - */\n> \n> Should we retain the documentation ? It may not be directly clear from\n> the function prototype what outputFormat is used for.\n\nI find little value in this type of documentation in pipeline handlers. \nBut it's already there so I will update it and retain it for v2.\n\n> \n> > -int ImgUDevice::configureOutput(ImgUOutput *output,\n> > -\t\t\t\tconst StreamConfiguration &cfg,\n> > +int ImgUDevice::configureOutput(const StreamConfiguration &cfg,\n> > \t\t\t\tV4L2DeviceFormat *outputFormat)\n> > {\n> > -\tV4L2VideoDevice *dev = output->dev;\n> > -\tunsigned int pad = output->pad;\n> > +\treturn configureVideoDevice(output_.dev, PAD_OUTPUT, cfg, outputFormat);\n> > +}\n> > \n> > +int ImgUDevice::configureViewfinder(const StreamConfiguration &cfg,\n> > +\t\t\t\t V4L2DeviceFormat *outputFormat)\n> > +{\n> > +\treturn configureVideoDevice(viewfinder_.dev, PAD_VF, cfg, outputFormat);\n> > +}\n> > +\n> > +int ImgUDevice::configureStat(const StreamConfiguration &cfg,\n> > +\t\t\t V4L2DeviceFormat *outputFormat)\n> > +{\n> > +\treturn configureVideoDevice(stat_.dev, PAD_STAT, cfg, outputFormat);\n> > +}\n> \n> Should these functions be inlined in the .h file for efficiency ?\n\nGood idea.\n\n\n> \n> > +\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> \n> When I read the commit message I got worried you would duplicate quite a\n> bit of code, I'm relieved now :-)\n\n:-)\n\n> \n> > \tV4L2SubdeviceFormat imguFormat = {};\n> > \timguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;\n> > \timguFormat.size = cfg.size;\n> > @@ -161,7 +170,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> > @@ -173,7 +182,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..f8fd54089753b40e 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.h\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> > @@ -49,9 +49,14 @@ 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> > +\tint configureOutput(const StreamConfiguration &cfg,\n> > \t\t\t V4L2DeviceFormat *outputFormat);\n> > +\tint configureViewfinder(const StreamConfiguration &cfg,\n> > +\t\t\t\tV4L2DeviceFormat *outputFormat);\n> > +\tint configureStat(const StreamConfiguration &cfg,\n> > +\t\t\t V4L2DeviceFormat *outputFormat);\n> > \n> > \tint allocateBuffers(unsigned int bufferCount);\n> > \tvoid freeBuffers();\n> > @@ -78,6 +83,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..ae7a01b81dacf498 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 just copies buffers from\n> > +\t\t\t * the internal queue and doesn't need any specific\n> > +\t\t\t * configuration.\n> \n> s/internal queue/internal CIO2 queue/ ?\n> s/configuration/configuration of the ImgU/ ?\n> \n> And we don't copy buffers anymore, do we ?\n\nGood point, I will rewrite it for v2.\n\n> \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> \n> I'm not sure I find the result more readable, but I'm also not opposed\n> to it.\n> \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 4A64DC2E66\n\tfor ;\n\tSat, 27 Jun 2020 22:39:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C28C9609DB;\n\tSun, 28 Jun 2020 00:39:28 +0200 (CEST)","from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28FA7609A9\n\tfor ;\n\tSun, 28 Jun 2020 00:39:27 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id 9so13968244ljc.8\n\tfor ;\n\tSat, 27 Jun 2020 15:39:27 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tj25sm6324724lfh.95.2020.06.27.15.39.25\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 27 Jun 2020 15:39:25 -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=\"ZSI/Vt+d\"; 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=7fD7KiPesyZVNJCJ0QM3JvgBOyfyURpcpmuERruMAJg=;\n\tb=ZSI/Vt+dY3yI5iJB35OUEL9liPb76zMTp0Tdh6RlUNrdUwa8gqjIz97P8/pjrXbyys\n\tP9iywEUbWeBj34sJwtTDT1kR/u1SVP88SRkZJu3ABNcajlpdy5Q+dSPGun28oc83IdF6\n\tnXcTrEeQnVi8K1YX+ovTOB9ZszksbsyxIotr958/eDlH3mDO39Sv73OGjjskviKaE8Wm\n\thm/AaqJuIjj7CxNR+c/uz+L5KaFD7fC9o5jh5avSBZenXN1W97JYsySj/JdglYDBcXqf\n\tTUHddu4gmFrapVhNdj/8NoJWHIPuyUOUK2KblShMFg9E1hiwZdwuQ2jF+3l6agzRGl85\n\tUyIw==","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=7fD7KiPesyZVNJCJ0QM3JvgBOyfyURpcpmuERruMAJg=;\n\tb=rY5t/gRhXWX25KUnNVMqBkak8BAz8OfOdKeSVO4QsQbPpQ15gUA0MzqDcFzKf/wi4s\n\tK7NLvmYJzUUkG0EUJw33F4bLxau4aLIsPoxNcpkc06QbGe5otnL2O+IhZFljGsui/z5X\n\t55pq/tjMEe9cl7B0o6gGQfmO26WGMhdM05Z5lfuZSjiYDKjJcnkfY4bxOXN4SS3CWrht\n\tztVzqNiJXbaZZhXFxsIEXHlZSWMIJ1O+0zdedfk3v2Q1lorc17XJlf/Sz8xEp/bG/R2J\n\t97KyJ9QE4kRZxvZ0E/SbGZQ/LVL+J6UaKJuPM1D+8D7kbkBegInV2AfDWGD2Yzotrj/z\n\tRBaw==","X-Gm-Message-State":"AOAM531HRvf60tJyXY6QZHHSHAO0LunsppQXKglSmQ9040ItdDPfsC4Y\n\tgyzRG1/qB9cQab8ZCOLBKbd/XiYD0MA=","X-Google-Smtp-Source":"ABdhPJy0ZoNxLyWM0FWum34UaEaAqoRkyhd5nSSDnoDovBfgXegNPdTolwrn8vJ1I1DgCwlQ3dNYog==","X-Received":"by 2002:a2e:b541:: with SMTP id a1mr4323890ljn.4.1593297566265; \n\tSat, 27 Jun 2020 15:39:26 -0700 (PDT)","Date":"Sun, 28 Jun 2020 00:39:24 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200627223924.GA1105424@oden.dyn.berto.se>","References":"<20200627030043.2088585-1-niklas.soderlund@ragnatech.se>\n\t<20200627030043.2088585-9-niklas.soderlund@ragnatech.se>\n\t<20200627161053.GC5966@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200627161053.GC5966@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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\" "}}]