[{"id":12064,"web_url":"https://patchwork.libcamera.org/comment/12064/","msgid":"<20200820094652.n5itgknr3p7stg4a@uno.localdomain>","date":"2020-08-20T09:46:52","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1:\n\tConfigure self path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Aug 13, 2020 at 02:52:43AM +0200, Niklas Söderlund wrote:\n> Allow for both the main and self path streams to be configured. This\n> change adds the self path as an internal stream to the pipeline handler.\n> It is not exposed as a Camera stream so it can not yet be used.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++-------\n> 1 file changed, 139 insertions(+), 58 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 132878c13b10b555..671098e11a2e2750 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -133,7 +133,8 @@ public:\n> \t\t\t V4L2VideoDevice *selfPathVideo)\n> \t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> \t\t frameInfo_(pipe), mainPathVideo_(mainPathVideo),\n> -\t\t selfPathVideo_(selfPathVideo)\n> +\t\t selfPathVideo_(selfPathVideo), mainPathActive_(false),\n> +\t\t selfPathActive_(false)\n> \t{\n> \t}\n>\n> @@ -145,6 +146,7 @@ public:\n> \tint loadIPA();\n>\n> \tStream mainPathStream_;\n> +\tStream selfPathStream_;\n\nWould this, and all other duplicated fields be nicer is stored as\narrays and accessed with fixed indexes ? I don't mind the way it is\ntoday though\n\n> \tCameraSensor *sensor_;\n> \tunsigned int frame_;\n> \tstd::vector ipaBuffers_;\n> @@ -154,6 +156,9 @@ public:\n> \tV4L2VideoDevice *mainPathVideo_;\n> \tV4L2VideoDevice *selfPathVideo_;\n>\n> +\tbool mainPathActive_;\n> +\tbool selfPathActive_;\n> +\n> private:\n> \tvoid queueFrameAction(unsigned int frame,\n> \t\t\t const IPAOperationData &action);\n> @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tRkISP1CameraConfiguration *config =\n> \t\tstatic_cast(c);\n> \tRkISP1CameraData *data = cameraData(camera);\n> -\tStreamConfiguration &cfg = config->at(0);\n> \tCameraSensor *sensor = data->sensor_;\n> \tint ret;\n>\n> @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>\n> \tLOG(RkISP1, Debug) << \"ISP output pad configured with \" << format.toString();\n>\n> -\tret = mainPathResizer_->setFormat(0, &format);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n> -\tLOG(RkISP1, Debug) << \"Resizer input pad configured with \" << format.toString();\n> -\n> -\tformat.size = cfg.size;\n> -\n> -\tLOG(RkISP1, Debug) << \"Configuring resizer output pad with \" << format.toString();\n> -\n> -\tret = mainPathResizer_->setFormat(1, &format);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n> -\tLOG(RkISP1, Debug) << \"Resizer output pad configured with \" << format.toString();\n> -\n> -\tV4L2DeviceFormat outputFormat = {};\n> -\toutputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);\n> -\toutputFormat.size = cfg.size;\n> -\toutputFormat.planesCount = 2;\n> -\n> -\tret = mainPathVideo_->setFormat(&outputFormat);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tif (outputFormat.size != cfg.size ||\n> -\t outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) {\n> -\t\tLOG(RkISP1, Error)\n> -\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> -\t\treturn -EINVAL;\n> +\tdata->mainPathActive_ = false;\n> +\tdata->selfPathActive_ = false;\n> +\tfor (const StreamConfiguration &cfg : *config) {\n> +\t\tV4L2SubdeviceFormat ispFormat = format;\n> +\t\tV4L2Subdevice *resizer;\n> +\t\tV4L2VideoDevice *video;\n> +\n> +\t\tif (cfg.stream() == &data->mainPathStream_) {\n> +\t\t\tresizer = mainPathResizer_;\n> +\t\t\tvideo = mainPathVideo_;\n> +\t\t\tdata->mainPathActive_ = true;\n> +\t\t} else {\n> +\t\t\tresizer = selfPathResizer_;\n> +\t\t\tvideo = selfPathVideo_;\n> +\t\t\tdata->selfPathActive_ = true;\n> +\t\t}\n> +\n> +\t\tret = resizer->setFormat(0, &ispFormat);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\n> +\t\tLOG(RkISP1, Debug) << \"Resizer input pad configured with \" << ispFormat.toString();\n> +\n> +\t\tispFormat.size = cfg.size;\n> +\n> +\t\tLOG(RkISP1, Debug) << \"Configuring resizer output pad with \" << ispFormat.toString();\n> +\n> +\t\tret = resizer->setFormat(1, &ispFormat);\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\n> +\t\tLOG(RkISP1, Debug) << \"Resizer output pad configured with \" << ispFormat.toString();\n> +\n> +\t\tV4L2DeviceFormat outputFormat = {};\n> +\t\toutputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat);\n> +\t\toutputFormat.size = cfg.size;\n> +\t\toutputFormat.planesCount = 2;\n\nDoesn't the planes count depend on the format ?\n\n> +\n> +\t\tret = video->setFormat(&outputFormat);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tif (outputFormat.size != cfg.size ||\n> +\t\t outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> \t}\n>\n> \tV4L2DeviceFormat paramFormat = {};\n> @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tif (ret)\n> \t\treturn ret;\n>\n> -\tcfg.setStream(&data->mainPathStream_);\n> -\n> \treturn 0;\n> }\n>\n> int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream,\n> \t\t\t\t\t std::vector> *buffers)\n> {\n> +\tRkISP1CameraData *data = cameraData(camera);\n> \tunsigned int count = stream->configuration().bufferCount;\n> -\treturn mainPathVideo_->exportBuffers(count, buffers);\n> +\n> +\tif (stream == &data->mainPathStream_)\n> +\t\treturn mainPathVideo_->exportBuffers(count, buffers);\n> +\n> +\tif (stream == &data->selfPathStream_)\n> +\t\treturn selfPathVideo_->exportBuffers(count, buffers);\n> +\n> +\treturn -EINVAL;\n> }\n>\n> int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> {\n> \tRkISP1CameraData *data = cameraData(camera);\n> -\tunsigned int count = data->mainPathStream_.configuration().bufferCount;\n> \tunsigned int ipaBufferId = 1;\n> \tint ret;\n>\n> -\tret = mainPathVideo_->importBuffers(count);\n> -\tif (ret < 0)\n> -\t\tgoto error;\n> +\tunsigned int count = std::max({\n> +\t\tdata->mainPathStream_.configuration().bufferCount,\n> +\t\tdata->selfPathStream_.configuration().bufferCount,\n> +\t});\n> +\n> +\tif (data->mainPathActive_) {\n> +\t\tret = mainPathVideo_->importBuffers(count);\n> +\t\tif (ret < 0)\n> +\t\t\tgoto error;\n\nIs it safe to call releaseBuffers if importing fails ?\n\n> +\t}\n> +\n> +\tif (data->selfPathActive_) {\n> +\t\tret = selfPathVideo_->importBuffers(count);\n> +\t\tif (ret < 0)\n> +\t\t\tgoto error;\n> +\t}\n>\n> \tret = param_->allocateBuffers(count, ¶mBuffers_);\n> \tif (ret < 0)\n> @@ -754,6 +794,7 @@ error:\n> \tparamBuffers_.clear();\n> \tstatBuffers_.clear();\n> \tmainPathVideo_->releaseBuffers();\n> +\tselfPathVideo_->releaseBuffers();\n>\n> \treturn ret;\n> }\n> @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> \tif (mainPathVideo_->releaseBuffers())\n> \t\tLOG(RkISP1, Error) << \"Failed to release main path buffers\";\n>\n> +\tif (selfPathVideo_->releaseBuffers())\n> +\t\tLOG(RkISP1, Error) << \"Failed to release self path buffers\";\n> +\n> \treturn 0;\n> }\n>\n> @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> \t\treturn ret;\n> \t}\n>\n> -\tret = mainPathVideo_->streamOn();\n> -\tif (ret) {\n> -\t\tparam_->streamOff();\n> -\t\tstat_->streamOff();\n> -\t\tdata->ipa_->stop();\n> -\t\tfreeBuffers(camera);\n> -\n> -\t\tLOG(RkISP1, Error)\n> -\t\t\t<< \"Failed to start camera \" << camera->id();\n> +\tstd::map streamConfig;\n> +\n> +\tif (data->mainPathActive_) {\n> +\t\tret = mainPathVideo_->streamOn();\n> +\t\tif (ret) {\n> +\t\t\tparam_->streamOff();\n> +\t\t\tstat_->streamOff();\n> +\t\t\tdata->ipa_->stop();\n> +\t\t\tfreeBuffers(camera);\n> +\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Failed to start main path \" << camera->id();\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tstreamConfig[0] = {\n> +\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> +\t\t\t.size = data->mainPathStream_.configuration().size,\n> +\t\t};\n\nWhat's this for ? And why assume [0] is assigned to main path ?\n\n> +\t}\n> +\n> +\tif (data->selfPathActive_) {\n> +\t\tret = selfPathVideo_->streamOn();\n> +\t\tif (ret) {\n> +\t\t\tif (data->mainPathActive_)\n> +\t\t\t\tmainPathVideo_->streamOff();\n> +\n> +\t\t\tparam_->streamOff();\n> +\t\t\tstat_->streamOff();\n> +\t\t\tdata->ipa_->stop();\n> +\t\t\tfreeBuffers(camera);\n> +\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Failed to start self path \" << camera->id();\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tstreamConfig[1] = {\n> +\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n> +\t\t\t.size = data->selfPathStream_.configuration().size,\n> +\t\t};\n> \t}\n>\n> \tactiveCamera_ = camera;\n> @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> \t\tret = 0;\n> \t}\n>\n> -\tstd::map streamConfig;\n> -\tstreamConfig[0] = {\n> -\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> -\t\t.size = data->mainPathStream_.configuration().size,\n> -\t};\n> -\n> \tstd::map entityControls;\n> \tentityControls.emplace(0, data->sensor_->controls());\n>\n> @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> \tRkISP1CameraData *data = cameraData(camera);\n> \tint ret;\n>\n> -\tret = mainPathVideo_->streamOff();\n> -\tif (ret)\n> -\t\tLOG(RkISP1, Warning)\n> -\t\t\t<< \"Failed to stop camera \" << camera->id();\n> +\tif (data->selfPathActive_) {\n> +\t\tret = selfPathVideo_->streamOff();\n> +\t\tif (ret)\n> +\t\t\tLOG(RkISP1, Warning)\n> +\t\t\t\t<< \"Failed to stop self path \" << camera->id();\n> +\t}\n\nCan we call streamOff unconditionally ? It depends on the driver\nimplementation I guess\n\nOverall the patch looks good!\nThanks\n j\n\n> +\n> +\tif (data->mainPathActive_) {\n> +\t\tret = mainPathVideo_->streamOff();\n> +\t\tif (ret)\n> +\t\t\tLOG(RkISP1, Warning)\n> +\t\t\t\t<< \"Failed to stop main path \" << camera->id();\n> +\t}\n>\n> \tret = stat_->streamOff();\n> \tif (ret)\n> @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera,\n> \t\tstd::string resizer;\n> \t\tif (cfg.stream() == &data->mainPathStream_)\n> \t\t\tresizer = \"rkisp1_resizer_mainpath\";\n> +\t\telse if (cfg.stream() == &data->selfPathStream_)\n> +\t\t\tresizer = \"rkisp1_resizer_selfpath\";\n> \t\telse\n> \t\t\treturn -EINVAL;\n>\n> --\n> 2.28.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 3EB3CBE173\n\tfor ;\n\tThu, 20 Aug 2020 09:43:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 39D7E61FA6;\n\tThu, 20 Aug 2020 11:43:12 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AEA7861ED9\n\tfor ;\n\tThu, 20 Aug 2020 11:43:10 +0200 (CEST)","from uno.localdomain (host-82-52-18-94.retail.telecomitalia.it\n\t[82.52.18.94]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 8BD5BFF80E;\n\tThu, 20 Aug 2020 09:43:09 +0000 (UTC)"],"X-Originating-IP":"82.52.18.94","Date":"Thu, 20 Aug 2020 11:46:52 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200820094652.n5itgknr3p7stg4a@uno.localdomain>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-11-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200813005246.3265807-11-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1:\n\tConfigure self path","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":12493,"web_url":"https://patchwork.libcamera.org/comment/12493/","msgid":"<20200914110901.GI1127199@oden.dyn.berto.se>","date":"2020-09-14T11:09:01","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1:\n\tConfigure self path","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2020-08-20 11:46:52 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Aug 13, 2020 at 02:52:43AM +0200, Niklas Söderlund wrote:\n> > Allow for both the main and self path streams to be configured. This\n> > change adds the self path as an internal stream to the pipeline handler.\n> > It is not exposed as a Camera stream so it can not yet be used.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++-------\n> > 1 file changed, 139 insertions(+), 58 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 132878c13b10b555..671098e11a2e2750 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -133,7 +133,8 @@ public:\n> > \t\t\t V4L2VideoDevice *selfPathVideo)\n> > \t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> > \t\t frameInfo_(pipe), mainPathVideo_(mainPathVideo),\n> > -\t\t selfPathVideo_(selfPathVideo)\n> > +\t\t selfPathVideo_(selfPathVideo), mainPathActive_(false),\n> > +\t\t selfPathActive_(false)\n> > \t{\n> > \t}\n> >\n> > @@ -145,6 +146,7 @@ public:\n> > \tint loadIPA();\n> >\n> > \tStream mainPathStream_;\n> > +\tStream selfPathStream_;\n> \n> Would this, and all other duplicated fields be nicer is stored as\n> arrays and accessed with fixed indexes ? I don't mind the way it is\n> today though\n\nI guess this is another bikeshedding topic. I don't see any clear \nadvantage of one over the other.\n\n mainPathStream_\n\n vs\n\n streams_[RkISP1::MainPAth]\n\nIf we are to switch to an array approach I do think we should do so \nbefore or after this series. I picked this solution as it was the one \nalready used in the pipeline.\n\n> \n> > \tCameraSensor *sensor_;\n> > \tunsigned int frame_;\n> > \tstd::vector ipaBuffers_;\n> > @@ -154,6 +156,9 @@ public:\n> > \tV4L2VideoDevice *mainPathVideo_;\n> > \tV4L2VideoDevice *selfPathVideo_;\n> >\n> > +\tbool mainPathActive_;\n> > +\tbool selfPathActive_;\n> > +\n> > private:\n> > \tvoid queueFrameAction(unsigned int frame,\n> > \t\t\t const IPAOperationData &action);\n> > @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \tRkISP1CameraConfiguration *config =\n> > \t\tstatic_cast(c);\n> > \tRkISP1CameraData *data = cameraData(camera);\n> > -\tStreamConfiguration &cfg = config->at(0);\n> > \tCameraSensor *sensor = data->sensor_;\n> > \tint ret;\n> >\n> > @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >\n> > \tLOG(RkISP1, Debug) << \"ISP output pad configured with \" << format.toString();\n> >\n> > -\tret = mainPathResizer_->setFormat(0, &format);\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > -\n> > -\tLOG(RkISP1, Debug) << \"Resizer input pad configured with \" << format.toString();\n> > -\n> > -\tformat.size = cfg.size;\n> > -\n> > -\tLOG(RkISP1, Debug) << \"Configuring resizer output pad with \" << format.toString();\n> > -\n> > -\tret = mainPathResizer_->setFormat(1, &format);\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > -\n> > -\tLOG(RkISP1, Debug) << \"Resizer output pad configured with \" << format.toString();\n> > -\n> > -\tV4L2DeviceFormat outputFormat = {};\n> > -\toutputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);\n> > -\toutputFormat.size = cfg.size;\n> > -\toutputFormat.planesCount = 2;\n> > -\n> > -\tret = mainPathVideo_->setFormat(&outputFormat);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > -\tif (outputFormat.size != cfg.size ||\n> > -\t outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) {\n> > -\t\tLOG(RkISP1, Error)\n> > -\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> > -\t\treturn -EINVAL;\n> > +\tdata->mainPathActive_ = false;\n> > +\tdata->selfPathActive_ = false;\n> > +\tfor (const StreamConfiguration &cfg : *config) {\n> > +\t\tV4L2SubdeviceFormat ispFormat = format;\n> > +\t\tV4L2Subdevice *resizer;\n> > +\t\tV4L2VideoDevice *video;\n> > +\n> > +\t\tif (cfg.stream() == &data->mainPathStream_) {\n> > +\t\t\tresizer = mainPathResizer_;\n> > +\t\t\tvideo = mainPathVideo_;\n> > +\t\t\tdata->mainPathActive_ = true;\n> > +\t\t} else {\n> > +\t\t\tresizer = selfPathResizer_;\n> > +\t\t\tvideo = selfPathVideo_;\n> > +\t\t\tdata->selfPathActive_ = true;\n> > +\t\t}\n> > +\n> > +\t\tret = resizer->setFormat(0, &ispFormat);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\tLOG(RkISP1, Debug) << \"Resizer input pad configured with \" << ispFormat.toString();\n> > +\n> > +\t\tispFormat.size = cfg.size;\n> > +\n> > +\t\tLOG(RkISP1, Debug) << \"Configuring resizer output pad with \" << ispFormat.toString();\n> > +\n> > +\t\tret = resizer->setFormat(1, &ispFormat);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\tLOG(RkISP1, Debug) << \"Resizer output pad configured with \" << ispFormat.toString();\n> > +\n> > +\t\tV4L2DeviceFormat outputFormat = {};\n> > +\t\toutputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat);\n> > +\t\toutputFormat.size = cfg.size;\n> > +\t\toutputFormat.planesCount = 2;\n> \n> Doesn't the planes count depend on the format ?\n\nGood catch. This is a preexisting bug in the pipeline. I will add a \nseparate patch to this series to sort this out.\n\n> \n> > +\n> > +\t\tret = video->setFormat(&outputFormat);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\tif (outputFormat.size != cfg.size ||\n> > +\t\t outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) {\n> > +\t\t\tLOG(RkISP1, Error)\n> > +\t\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > \t}\n> >\n> > \tV4L2DeviceFormat paramFormat = {};\n> > @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \tif (ret)\n> > \t\treturn ret;\n> >\n> > -\tcfg.setStream(&data->mainPathStream_);\n> > -\n> > \treturn 0;\n> > }\n> >\n> > int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream,\n> > \t\t\t\t\t std::vector> *buffers)\n> > {\n> > +\tRkISP1CameraData *data = cameraData(camera);\n> > \tunsigned int count = stream->configuration().bufferCount;\n> > -\treturn mainPathVideo_->exportBuffers(count, buffers);\n> > +\n> > +\tif (stream == &data->mainPathStream_)\n> > +\t\treturn mainPathVideo_->exportBuffers(count, buffers);\n> > +\n> > +\tif (stream == &data->selfPathStream_)\n> > +\t\treturn selfPathVideo_->exportBuffers(count, buffers);\n> > +\n> > +\treturn -EINVAL;\n> > }\n> >\n> > int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> > {\n> > \tRkISP1CameraData *data = cameraData(camera);\n> > -\tunsigned int count = data->mainPathStream_.configuration().bufferCount;\n> > \tunsigned int ipaBufferId = 1;\n> > \tint ret;\n> >\n> > -\tret = mainPathVideo_->importBuffers(count);\n> > -\tif (ret < 0)\n> > -\t\tgoto error;\n> > +\tunsigned int count = std::max({\n> > +\t\tdata->mainPathStream_.configuration().bufferCount,\n> > +\t\tdata->selfPathStream_.configuration().bufferCount,\n> > +\t});\n> > +\n> > +\tif (data->mainPathActive_) {\n> > +\t\tret = mainPathVideo_->importBuffers(count);\n> > +\t\tif (ret < 0)\n> > +\t\t\tgoto error;\n> \n> Is it safe to call releaseBuffers if importing fails ?\n\nYes, is it not always safe to call releaseBuffers? The idea is that if \nallocateBuffers() fails it should undo all operations. Remember we \ndepend on buffer orphaning so buffers allocated by for example the \nFrameBufferAllocater are still OK.\n\n> \n> > +\t}\n> > +\n> > +\tif (data->selfPathActive_) {\n> > +\t\tret = selfPathVideo_->importBuffers(count);\n> > +\t\tif (ret < 0)\n> > +\t\t\tgoto error;\n> > +\t}\n> >\n> > \tret = param_->allocateBuffers(count, ¶mBuffers_);\n> > \tif (ret < 0)\n> > @@ -754,6 +794,7 @@ error:\n> > \tparamBuffers_.clear();\n> > \tstatBuffers_.clear();\n> > \tmainPathVideo_->releaseBuffers();\n> > +\tselfPathVideo_->releaseBuffers();\n> >\n> > \treturn ret;\n> > }\n> > @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> > \tif (mainPathVideo_->releaseBuffers())\n> > \t\tLOG(RkISP1, Error) << \"Failed to release main path buffers\";\n> >\n> > +\tif (selfPathVideo_->releaseBuffers())\n> > +\t\tLOG(RkISP1, Error) << \"Failed to release self path buffers\";\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > \t\treturn ret;\n> > \t}\n> >\n> > -\tret = mainPathVideo_->streamOn();\n> > -\tif (ret) {\n> > -\t\tparam_->streamOff();\n> > -\t\tstat_->streamOff();\n> > -\t\tdata->ipa_->stop();\n> > -\t\tfreeBuffers(camera);\n> > -\n> > -\t\tLOG(RkISP1, Error)\n> > -\t\t\t<< \"Failed to start camera \" << camera->id();\n> > +\tstd::map streamConfig;\n> > +\n> > +\tif (data->mainPathActive_) {\n> > +\t\tret = mainPathVideo_->streamOn();\n> > +\t\tif (ret) {\n> > +\t\t\tparam_->streamOff();\n> > +\t\t\tstat_->streamOff();\n> > +\t\t\tdata->ipa_->stop();\n> > +\t\t\tfreeBuffers(camera);\n> > +\n> > +\t\t\tLOG(RkISP1, Error)\n> > +\t\t\t\t<< \"Failed to start main path \" << camera->id();\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tstreamConfig[0] = {\n> > +\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> > +\t\t\t.size = data->mainPathStream_.configuration().size,\n> > +\t\t};\n> \n> What's this for ? And why assume [0] is assigned to main path ?\n\nThis is part of the yet undocumented IPA protocl for the RkISP1. The \nstream configuration map is mainpath = 0 and selfpath = 1. When we get \nthe new IPC framework in place all of this should of course be \ndocumented. In reality this is have currently no impact as the RkISP1 \nIPA does not examine the stream configuration at all.\n\n> \n> > +\t}\n> > +\n> > +\tif (data->selfPathActive_) {\n> > +\t\tret = selfPathVideo_->streamOn();\n> > +\t\tif (ret) {\n> > +\t\t\tif (data->mainPathActive_)\n> > +\t\t\t\tmainPathVideo_->streamOff();\n> > +\n> > +\t\t\tparam_->streamOff();\n> > +\t\t\tstat_->streamOff();\n> > +\t\t\tdata->ipa_->stop();\n> > +\t\t\tfreeBuffers(camera);\n> > +\n> > +\t\t\tLOG(RkISP1, Error)\n> > +\t\t\t\t<< \"Failed to start self path \" << camera->id();\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tstreamConfig[1] = {\n> > +\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n> > +\t\t\t.size = data->selfPathStream_.configuration().size,\n> > +\t\t};\n> > \t}\n> >\n> > \tactiveCamera_ = camera;\n> > @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > \t\tret = 0;\n> > \t}\n> >\n> > -\tstd::map streamConfig;\n> > -\tstreamConfig[0] = {\n> > -\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> > -\t\t.size = data->mainPathStream_.configuration().size,\n> > -\t};\n> > -\n> > \tstd::map entityControls;\n> > \tentityControls.emplace(0, data->sensor_->controls());\n> >\n> > @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> > \tRkISP1CameraData *data = cameraData(camera);\n> > \tint ret;\n> >\n> > -\tret = mainPathVideo_->streamOff();\n> > -\tif (ret)\n> > -\t\tLOG(RkISP1, Warning)\n> > -\t\t\t<< \"Failed to stop camera \" << camera->id();\n> > +\tif (data->selfPathActive_) {\n> > +\t\tret = selfPathVideo_->streamOff();\n> > +\t\tif (ret)\n> > +\t\t\tLOG(RkISP1, Warning)\n> > +\t\t\t\t<< \"Failed to stop self path \" << camera->id();\n> > +\t}\n> \n> Can we call streamOff unconditionally ? It depends on the driver\n> implementation I guess\n\nWe can call it unconditionally, I think even v4l2-compliance tests \nmultiple s_stream(1) and s_stream(0) calls does it not? But I agree \nthere might of course be driver specific quirks.\n\n> \n> Overall the patch looks good!\n> Thanks\n> j\n> \n> > +\n> > +\tif (data->mainPathActive_) {\n> > +\t\tret = mainPathVideo_->streamOff();\n> > +\t\tif (ret)\n> > +\t\t\tLOG(RkISP1, Warning)\n> > +\t\t\t\t<< \"Failed to stop main path \" << camera->id();\n> > +\t}\n> >\n> > \tret = stat_->streamOff();\n> > \tif (ret)\n> > @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera,\n> > \t\tstd::string resizer;\n> > \t\tif (cfg.stream() == &data->mainPathStream_)\n> > \t\t\tresizer = \"rkisp1_resizer_mainpath\";\n> > +\t\telse if (cfg.stream() == &data->selfPathStream_)\n> > +\t\t\tresizer = \"rkisp1_resizer_selfpath\";\n> > \t\telse\n> > \t\t\treturn -EINVAL;\n> >\n> > --\n> > 2.28.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 43801C3B5B\n\tfor ;\n\tMon, 14 Sep 2020 11:09:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A24C262D99;\n\tMon, 14 Sep 2020 13:09:04 +0200 (CEST)","from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8715162B90\n\tfor ;\n\tMon, 14 Sep 2020 13:09:03 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id z19so12961541lfr.4\n\tfor ;\n\tMon, 14 Sep 2020 04:09:03 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tl9sm3272980lfh.285.2020.09.14.04.09.01\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 14 Sep 2020 04:09:01 -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=\"Zcx0ASm8\"; 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=SBgdsBL0IVDTRWUlGDBK2XP7dJBnBESVLhnmyhDLxuY=;\n\tb=Zcx0ASm886UXa/55bLErRZoo08EWR2ScYxmyqEzjXjdf93mISPXaI2UHbcv0wqWMzm\n\tYNMUJKs5wAS5ECoijBQp8Qe6VyL/4F+qO/UaRtXDZ5/0p63tgZgbaqiGWV7ip/uzTwJo\n\tN3Vv9TERWOKBnApTkedr6OpEVm+H5Bg/E73akEACLmSwOmqWLVCHU4VXoQnw9t+bRCv7\n\tHXq8tWX38nsH1X0SWCxFv8LprisDX0Pgb9FWXq2s1TQFwWcFS4Psmn2R2XmpICtABOBt\n\tDQubZgAoYf3fNHSbAlmOQ3mkqohjMIbbzJ5t6FWT1u7ydCWKRwyh1Dw9iAWB6ZkilAn0\n\trTmA==","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=SBgdsBL0IVDTRWUlGDBK2XP7dJBnBESVLhnmyhDLxuY=;\n\tb=cDDOvrU+Xxek192+6owAYNjl3EOG7lofhvRhhrv3Z3bToFt+mO+LRCE31ri0RmMH6B\n\tbC/F1VIOepIA9JA9GpP4k+C+dmnerOxXLMIiI5F3V9w0IppAsOG5kch7/07/99yNOxfb\n\tK7E2ivUC9YPv5W3Z1iVouNkJdlMPOQKJ7Aw+AF6aEp8mfm+zySE/vjS4i0g1QwjTvNmV\n\t2oG+fYfWbM/Z533UBdtwkpNoV8WsxLeBsNxvlKmvqiFilRtCRTyt4BAj/CKkNjVOnHgV\n\tnpVHcg4uZEgBPYEfYjZ6WXDBArTgvbNI3cgMdJf68DLxK+GbI8dB9rCwLVLMJV/050yQ\n\tq8QA==","X-Gm-Message-State":"AOAM530U+oGEm/JoADTP2aYhb8QCfnRrUMbsY6+t6rrGgPaYoYykIgY6\n\t9W+LHV1vG9QgH6KPkfKh+TjlGg==","X-Google-Smtp-Source":"ABdhPJzuabeBTdSL2geflrmOOhZIPlJ74CD4+S5wPm6urXfqKoVM3rjJddjw+FAdvFEAv1y6WaDGWg==","X-Received":"by 2002:ac2:4d4c:: with SMTP id 12mr3982211lfp.332.1600081742784;\n\tMon, 14 Sep 2020 04:09:02 -0700 (PDT)","Date":"Mon, 14 Sep 2020 13:09:01 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200914110901.GI1127199@oden.dyn.berto.se>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-11-niklas.soderlund@ragnatech.se>\n\t<20200820094652.n5itgknr3p7stg4a@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200820094652.n5itgknr3p7stg4a@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1:\n\tConfigure self path","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":12495,"web_url":"https://patchwork.libcamera.org/comment/12495/","msgid":"","date":"2020-09-14T11:19:54","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1:\n\tConfigure self path","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 14/09/2020 12:09, Niklas Söderlund wrote:\n> Hi Jacopo,\n> \n> On 2020-08-20 11:46:52 +0200, Jacopo Mondi wrote:\n>> Hi Niklas,\n>>\n>> On Thu, Aug 13, 2020 at 02:52:43AM +0200, Niklas Söderlund wrote:\n>>> Allow for both the main and self path streams to be configured. This\n>>> change adds the self path as an internal stream to the pipeline handler.\n>>> It is not exposed as a Camera stream so it can not yet be used.\n>>>\n>>> Signed-off-by: Niklas Söderlund \n>>> ---\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++-------\n>>> 1 file changed, 139 insertions(+), 58 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> index 132878c13b10b555..671098e11a2e2750 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> @@ -133,7 +133,8 @@ public:\n>>> \t\t\t V4L2VideoDevice *selfPathVideo)\n>>> \t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n>>> \t\t frameInfo_(pipe), mainPathVideo_(mainPathVideo),\n>>> -\t\t selfPathVideo_(selfPathVideo)\n>>> +\t\t selfPathVideo_(selfPathVideo), mainPathActive_(false),\n>>> +\t\t selfPathActive_(false)\n>>> \t{\n>>> \t}\n>>>\n>>> @@ -145,6 +146,7 @@ public:\n>>> \tint loadIPA();\n>>>\n>>> \tStream mainPathStream_;\n>>> +\tStream selfPathStream_;\n>>\n>> Would this, and all other duplicated fields be nicer is stored as\n>> arrays and accessed with fixed indexes ? I don't mind the way it is\n>> today though\n> \n> I guess this is another bikeshedding topic. I don't see any clear \n> advantage of one over the other.\n> \n> mainPathStream_\n> \n> vs\n> \n> streams_[RkISP1::MainPAth]\n> \n> If we are to switch to an array approach I do think we should do so \n> before or after this series. I picked this solution as it was the one \n> already used in the pipeline.\n\n\nJumping into the bikeshed mostly because it really stood out in the\ntitle of this and the preceding patch.\n\nThe 'self' path sounds horrible.\nI assume it derives from 'selfie' ... but really it's just a secondary ...\n\n\nI would probably prefer the array route (can be refactored anytime,\ndoesn't have to be here). If one path is more feature full than the\nother then perhaps naming them makes sense, but otherwise I'd just have\nthem as numerical pipelines.\n\nAnd rather than have an array of streams_ and an array of\nV4L2VideoDevice and an array of Sensors etc... I think this can be\nmoddelled as a class, and then it's just a single array of that class.\n\n(Or if there's only two, It could actually be just two instances of that\nclass at that point)\n\nRkISPPipe main;\nRkISPPipe secondary;\n\n\nedit: ohh yuck. I see\n> \n> +\tselfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_selfpath\");\n\nso it's named the 'self path' at the ISP ...\n\n--\nKieran\n\n\n\n>>\n>>> \tCameraSensor *sensor_;\n>>> \tunsigned int frame_;\n>>> \tstd::vector ipaBuffers_;\n>>> @@ -154,6 +156,9 @@ public:\n>>> \tV4L2VideoDevice *mainPathVideo_;\n>>> \tV4L2VideoDevice *selfPathVideo_;\n>>>\n>>> +\tbool mainPathActive_;\n>>> +\tbool selfPathActive_;\n>>> +\n>>> private:\n>>> \tvoid queueFrameAction(unsigned int frame,\n>>> \t\t\t const IPAOperationData &action);\n>>> @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>>> \tRkISP1CameraConfiguration *config =\n>>> \t\tstatic_cast(c);\n>>> \tRkISP1CameraData *data = cameraData(camera);\n>>> -\tStreamConfiguration &cfg = config->at(0);\n>>> \tCameraSensor *sensor = data->sensor_;\n>>> \tint ret;\n>>>\n>>> @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>>>\n>>> \tLOG(RkISP1, Debug) << \"ISP output pad configured with \" << format.toString();\n>>>\n>>> -\tret = mainPathResizer_->setFormat(0, &format);\n>>> -\tif (ret < 0)\n>>> -\t\treturn ret;\n>>> -\n>>> -\tLOG(RkISP1, Debug) << \"Resizer input pad configured with \" << format.toString();\n>>> -\n>>> -\tformat.size = cfg.size;\n>>> -\n>>> -\tLOG(RkISP1, Debug) << \"Configuring resizer output pad with \" << format.toString();\n>>> -\n>>> -\tret = mainPathResizer_->setFormat(1, &format);\n>>> -\tif (ret < 0)\n>>> -\t\treturn ret;\n>>> -\n>>> -\tLOG(RkISP1, Debug) << \"Resizer output pad configured with \" << format.toString();\n>>> -\n>>> -\tV4L2DeviceFormat outputFormat = {};\n>>> -\toutputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);\n>>> -\toutputFormat.size = cfg.size;\n>>> -\toutputFormat.planesCount = 2;\n>>> -\n>>> -\tret = mainPathVideo_->setFormat(&outputFormat);\n>>> -\tif (ret)\n>>> -\t\treturn ret;\n>>> -\n>>> -\tif (outputFormat.size != cfg.size ||\n>>> -\t outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) {\n>>> -\t\tLOG(RkISP1, Error)\n>>> -\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n>>> -\t\treturn -EINVAL;\n>>> +\tdata->mainPathActive_ = false;\n>>> +\tdata->selfPathActive_ = false;\n>>> +\tfor (const StreamConfiguration &cfg : *config) {\n>>> +\t\tV4L2SubdeviceFormat ispFormat = format;\n>>> +\t\tV4L2Subdevice *resizer;\n>>> +\t\tV4L2VideoDevice *video;\n>>> +\n>>> +\t\tif (cfg.stream() == &data->mainPathStream_) {\n>>> +\t\t\tresizer = mainPathResizer_;\n>>> +\t\t\tvideo = mainPathVideo_;\n>>> +\t\t\tdata->mainPathActive_ = true;\n>>> +\t\t} else {\n>>> +\t\t\tresizer = selfPathResizer_;\n>>> +\t\t\tvideo = selfPathVideo_;\n>>> +\t\t\tdata->selfPathActive_ = true;\n>>> +\t\t}\n>>> +\n>>> +\t\tret = resizer->setFormat(0, &ispFormat);\n>>> +\t\tif (ret < 0)\n>>> +\t\t\treturn ret;\n>>> +\n>>> +\t\tLOG(RkISP1, Debug) << \"Resizer input pad configured with \" << ispFormat.toString();\n>>> +\n>>> +\t\tispFormat.size = cfg.size;\n>>> +\n>>> +\t\tLOG(RkISP1, Debug) << \"Configuring resizer output pad with \" << ispFormat.toString();\n>>> +\n>>> +\t\tret = resizer->setFormat(1, &ispFormat);\n>>> +\t\tif (ret < 0)\n>>> +\t\t\treturn ret;\n>>> +\n>>> +\t\tLOG(RkISP1, Debug) << \"Resizer output pad configured with \" << ispFormat.toString();\n>>> +\n>>> +\t\tV4L2DeviceFormat outputFormat = {};\n>>> +\t\toutputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat);\n>>> +\t\toutputFormat.size = cfg.size;\n>>> +\t\toutputFormat.planesCount = 2;\n>>\n>> Doesn't the planes count depend on the format ?\n> \n> Good catch. This is a preexisting bug in the pipeline. I will add a \n> separate patch to this series to sort this out.\n> \n>>\n>>> +\n>>> +\t\tret = video->setFormat(&outputFormat);\n>>> +\t\tif (ret)\n>>> +\t\t\treturn ret;\n>>> +\n>>> +\t\tif (outputFormat.size != cfg.size ||\n>>> +\t\t outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) {\n>>> +\t\t\tLOG(RkISP1, Error)\n>>> +\t\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n>>> +\t\t\treturn -EINVAL;\n>>> +\t\t}\n>>> \t}\n>>>\n>>> \tV4L2DeviceFormat paramFormat = {};\n>>> @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>>> \tif (ret)\n>>> \t\treturn ret;\n>>>\n>>> -\tcfg.setStream(&data->mainPathStream_);\n>>> -\n>>> \treturn 0;\n>>> }\n>>>\n>>> int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream,\n>>> \t\t\t\t\t std::vector> *buffers)\n>>> {\n>>> +\tRkISP1CameraData *data = cameraData(camera);\n>>> \tunsigned int count = stream->configuration().bufferCount;\n>>> -\treturn mainPathVideo_->exportBuffers(count, buffers);\n>>> +\n>>> +\tif (stream == &data->mainPathStream_)\n>>> +\t\treturn mainPathVideo_->exportBuffers(count, buffers);\n>>> +\n>>> +\tif (stream == &data->selfPathStream_)\n>>> +\t\treturn selfPathVideo_->exportBuffers(count, buffers);\n>>> +\n>>> +\treturn -EINVAL;\n>>> }\n>>>\n>>> int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>>> {\n>>> \tRkISP1CameraData *data = cameraData(camera);\n>>> -\tunsigned int count = data->mainPathStream_.configuration().bufferCount;\n>>> \tunsigned int ipaBufferId = 1;\n>>> \tint ret;\n>>>\n>>> -\tret = mainPathVideo_->importBuffers(count);\n>>> -\tif (ret < 0)\n>>> -\t\tgoto error;\n>>> +\tunsigned int count = std::max({\n>>> +\t\tdata->mainPathStream_.configuration().bufferCount,\n>>> +\t\tdata->selfPathStream_.configuration().bufferCount,\n>>> +\t});\n>>> +\n>>> +\tif (data->mainPathActive_) {\n>>> +\t\tret = mainPathVideo_->importBuffers(count);\n>>> +\t\tif (ret < 0)\n>>> +\t\t\tgoto error;\n>>\n>> Is it safe to call releaseBuffers if importing fails ?\n> \n> Yes, is it not always safe to call releaseBuffers? The idea is that if \n> allocateBuffers() fails it should undo all operations. Remember we \n> depend on buffer orphaning so buffers allocated by for example the \n> FrameBufferAllocater are still OK.\n> \n>>\n>>> +\t}\n>>> +\n>>> +\tif (data->selfPathActive_) {\n>>> +\t\tret = selfPathVideo_->importBuffers(count);\n>>> +\t\tif (ret < 0)\n>>> +\t\t\tgoto error;\n>>> +\t}\n>>>\n>>> \tret = param_->allocateBuffers(count, ¶mBuffers_);\n>>> \tif (ret < 0)\n>>> @@ -754,6 +794,7 @@ error:\n>>> \tparamBuffers_.clear();\n>>> \tstatBuffers_.clear();\n>>> \tmainPathVideo_->releaseBuffers();\n>>> +\tselfPathVideo_->releaseBuffers();\n>>>\n>>> \treturn ret;\n>>> }\n>>> @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n>>> \tif (mainPathVideo_->releaseBuffers())\n>>> \t\tLOG(RkISP1, Error) << \"Failed to release main path buffers\";\n>>>\n>>> +\tif (selfPathVideo_->releaseBuffers())\n>>> +\t\tLOG(RkISP1, Error) << \"Failed to release self path buffers\";\n>>> +\n>>> \treturn 0;\n>>> }\n>>>\n>>> @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>>> \t\treturn ret;\n>>> \t}\n>>>\n>>> -\tret = mainPathVideo_->streamOn();\n>>> -\tif (ret) {\n>>> -\t\tparam_->streamOff();\n>>> -\t\tstat_->streamOff();\n>>> -\t\tdata->ipa_->stop();\n>>> -\t\tfreeBuffers(camera);\n>>> -\n>>> -\t\tLOG(RkISP1, Error)\n>>> -\t\t\t<< \"Failed to start camera \" << camera->id();\n>>> +\tstd::map streamConfig;\n>>> +\n>>> +\tif (data->mainPathActive_) {\n>>> +\t\tret = mainPathVideo_->streamOn();\n>>> +\t\tif (ret) {\n>>> +\t\t\tparam_->streamOff();\n>>> +\t\t\tstat_->streamOff();\n>>> +\t\t\tdata->ipa_->stop();\n>>> +\t\t\tfreeBuffers(camera);\n>>> +\n>>> +\t\t\tLOG(RkISP1, Error)\n>>> +\t\t\t\t<< \"Failed to start main path \" << camera->id();\n>>> +\t\t\treturn ret;\n>>> +\t\t}\n>>> +\n>>> +\t\tstreamConfig[0] = {\n>>> +\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n>>> +\t\t\t.size = data->mainPathStream_.configuration().size,\n>>> +\t\t};\n>>\n>> What's this for ? And why assume [0] is assigned to main path ?\n> \n> This is part of the yet undocumented IPA protocl for the RkISP1. The \n> stream configuration map is mainpath = 0 and selfpath = 1. When we get \n> the new IPC framework in place all of this should of course be \n> documented. In reality this is have currently no impact as the RkISP1 \n> IPA does not examine the stream configuration at all.\n> \n>>\n>>> +\t}\n>>> +\n>>> +\tif (data->selfPathActive_) {\n>>> +\t\tret = selfPathVideo_->streamOn();\n>>> +\t\tif (ret) {\n>>> +\t\t\tif (data->mainPathActive_)\n>>> +\t\t\t\tmainPathVideo_->streamOff();\n>>> +\n>>> +\t\t\tparam_->streamOff();\n>>> +\t\t\tstat_->streamOff();\n>>> +\t\t\tdata->ipa_->stop();\n>>> +\t\t\tfreeBuffers(camera);\n>>> +\n>>> +\t\t\tLOG(RkISP1, Error)\n>>> +\t\t\t\t<< \"Failed to start self path \" << camera->id();\n>>> +\t\t\treturn ret;\n>>> +\t\t}\n>>> +\n>>> +\t\tstreamConfig[1] = {\n>>> +\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n>>> +\t\t\t.size = data->selfPathStream_.configuration().size,\n>>> +\t\t};\n>>> \t}\n>>>\n>>> \tactiveCamera_ = camera;\n>>> @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>>> \t\tret = 0;\n>>> \t}\n>>>\n>>> -\tstd::map streamConfig;\n>>> -\tstreamConfig[0] = {\n>>> -\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n>>> -\t\t.size = data->mainPathStream_.configuration().size,\n>>> -\t};\n>>> -\n>>> \tstd::map entityControls;\n>>> \tentityControls.emplace(0, data->sensor_->controls());\n>>>\n>>> @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n>>> \tRkISP1CameraData *data = cameraData(camera);\n>>> \tint ret;\n>>>\n>>> -\tret = mainPathVideo_->streamOff();\n>>> -\tif (ret)\n>>> -\t\tLOG(RkISP1, Warning)\n>>> -\t\t\t<< \"Failed to stop camera \" << camera->id();\n>>> +\tif (data->selfPathActive_) {\n>>> +\t\tret = selfPathVideo_->streamOff();\n>>> +\t\tif (ret)\n>>> +\t\t\tLOG(RkISP1, Warning)\n>>> +\t\t\t\t<< \"Failed to stop self path \" << camera->id();\n>>> +\t}\n>>\n>> Can we call streamOff unconditionally ? It depends on the driver\n>> implementation I guess\n> \n> We can call it unconditionally, I think even v4l2-compliance tests \n> multiple s_stream(1) and s_stream(0) calls does it not? But I agree \n> there might of course be driver specific quirks.\n> \n>>\n>> Overall the patch looks good!\n>> Thanks\n>> j\n>>\n>>> +\n>>> +\tif (data->mainPathActive_) {\n>>> +\t\tret = mainPathVideo_->streamOff();\n>>> +\t\tif (ret)\n>>> +\t\t\tLOG(RkISP1, Warning)\n>>> +\t\t\t\t<< \"Failed to stop main path \" << camera->id();\n>>> +\t}\n>>>\n>>> \tret = stat_->streamOff();\n>>> \tif (ret)\n>>> @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera,\n>>> \t\tstd::string resizer;\n>>> \t\tif (cfg.stream() == &data->mainPathStream_)\n>>> \t\t\tresizer = \"rkisp1_resizer_mainpath\";\n>>> +\t\telse if (cfg.stream() == &data->selfPathStream_)\n>>> +\t\t\tresizer = \"rkisp1_resizer_selfpath\";\n>>> \t\telse\n>>> \t\t\treturn -EINVAL;\n>>>\n>>> --\n>>> 2.28.0\n>>>\n>>> _______________________________________________\n>>> libcamera-devel mailing list\n>>> libcamera-devel@lists.libcamera.org\n>>> https://lists.libcamera.org/listinfo/libcamera-devel\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 ECF00BF01C\n\tfor ;\n\tMon, 14 Sep 2020 11:20:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7AFE062D99;\n\tMon, 14 Sep 2020 13:20:18 +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 AC57962B90\n\tfor ;\n\tMon, 14 Sep 2020 13:20:16 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 04FCC275;\n\tMon, 14 Sep 2020 13:19:57 +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=\"rw9UWyOR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600082407;\n\tbh=/hrl0o/qz4jbs+VUvBn8Vu7iiYovIjVhdIzIa/LKmB4=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=rw9UWyOR8M5wvKEtfOXG7LFnEAmFd1NgUAJmSd5hfXUEBxfd1uermJsFQWka2In7S\n\tFGuJvihllyvdGTeI2TXh+Vyol1VGx07P4XJNf5XdNNd+Jam6wJGNRM/JR5sUWQ/Ilm\n\t2HVk8G/Bn4N61BjDi3hPajHS/zxxHSGBfvSRPWc8=","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tJacopo Mondi ","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-11-niklas.soderlund@ragnatech.se>\n\t<20200820094652.n5itgknr3p7stg4a@uno.localdomain>\n\t<20200914110901.GI1127199@oden.dyn.berto.se>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 14 Sep 2020 12:19:54 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200914110901.GI1127199@oden.dyn.berto.se>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1:\n\tConfigure self path","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","Reply-To":"kieran.bingham@ideasonboard.com","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":12496,"web_url":"https://patchwork.libcamera.org/comment/12496/","msgid":"<92b81e60-465c-b16e-9359-381727a7ba6f@ideasonboard.com>","date":"2020-09-14T11:24:22","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1:\n\tConfigure self path","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 14/09/2020 12:19, Kieran Bingham wrote:\n> Hi Niklas,\n> \n> On 14/09/2020 12:09, Niklas Söderlund wrote:\n>> Hi Jacopo,\n>>\n>> On 2020-08-20 11:46:52 +0200, Jacopo Mondi wrote:\n>>> Hi Niklas,\n>>>\n>>> On Thu, Aug 13, 2020 at 02:52:43AM +0200, Niklas Söderlund wrote:\n>>>> Allow for both the main and self path streams to be configured. This\n>>>> change adds the self path as an internal stream to the pipeline handler.\n>>>> It is not exposed as a Camera stream so it can not yet be used.\n>>>>\n>>>> Signed-off-by: Niklas Söderlund \n>>>> ---\n>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++-------\n>>>> 1 file changed, 139 insertions(+), 58 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> index 132878c13b10b555..671098e11a2e2750 100644\n>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> @@ -133,7 +133,8 @@ public:\n>>>> \t\t\t V4L2VideoDevice *selfPathVideo)\n>>>> \t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n>>>> \t\t frameInfo_(pipe), mainPathVideo_(mainPathVideo),\n>>>> -\t\t selfPathVideo_(selfPathVideo)\n>>>> +\t\t selfPathVideo_(selfPathVideo), mainPathActive_(false),\n>>>> +\t\t selfPathActive_(false)\n>>>> \t{\n>>>> \t}\n>>>>\n>>>> @@ -145,6 +146,7 @@ public:\n>>>> \tint loadIPA();\n>>>>\n>>>> \tStream mainPathStream_;\n>>>> +\tStream selfPathStream_;\n>>>\n>>> Would this, and all other duplicated fields be nicer is stored as\n>>> arrays and accessed with fixed indexes ? I don't mind the way it is\n>>> today though\n>>\n>> I guess this is another bikeshedding topic. I don't see any clear \n>> advantage of one over the other.\n>>\n>> mainPathStream_\n>>\n>> vs\n>>\n>> streams_[RkISP1::MainPAth]\n>>\n>> If we are to switch to an array approach I do think we should do so \n>> before or after this series. I picked this solution as it was the one \n>> already used in the pipeline.\n> \n> \n> Jumping into the bikeshed mostly because it really stood out in the\n> title of this and the preceding patch.\n> \n> The 'self' path sounds horrible.\n> I assume it derives from 'selfie' ... but really it's just a secondary ...\n> \n> \n> I would probably prefer the array route (can be refactored anytime,\n> doesn't have to be here). If one path is more feature full than the\n> other then perhaps naming them makes sense, but otherwise I'd just have\n> them as numerical pipelines.\n> \n> And rather than have an array of streams_ and an array of\n> V4L2VideoDevice and an array of Sensors etc... I think this can be\n> moddelled as a class, and then it's just a single array of that class.\n> \n> (Or if there's only two, It could actually be just two instances of that\n> class at that point)\n> \n> RkISPPipe main;\n> RkISPPipe secondary;\n> \n> \n> edit: ohh yuck. I see\n>>\n>> +\tselfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_selfpath\");\n> \n> so it's named the 'self path' at the ISP ...\n> \n\n\nRe-edit: Aha, I think I mis-understood this slightly. I thought this was\ndealing with multiple camera paths ... but it might be dealing with\nmultiple stream paths from a single camera... so it's not separate pipes\nnecessarily ... :-S\n\nPerhaps it's an RkISPStream object instead though ...\n\n> --\n> Kieran\n> \n> \n> \n>>>\n>>>> \tCameraSensor *sensor_;\n>>>> \tunsigned int frame_;\n>>>> \tstd::vector ipaBuffers_;\n>>>> @@ -154,6 +156,9 @@ public:\n>>>> \tV4L2VideoDevice *mainPathVideo_;\n>>>> \tV4L2VideoDevice *selfPathVideo_;\n>>>>\n>>>> +\tbool mainPathActive_;\n>>>> +\tbool selfPathActive_;\n>>>> +\n>>>> private:\n>>>> \tvoid queueFrameAction(unsigned int frame,\n>>>> \t\t\t const IPAOperationData &action);\n>>>> @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>>>> \tRkISP1CameraConfiguration *config =\n>>>> \t\tstatic_cast(c);\n>>>> \tRkISP1CameraData *data = cameraData(camera);\n>>>> -\tStreamConfiguration &cfg = config->at(0);\n>>>> \tCameraSensor *sensor = data->sensor_;\n>>>> \tint ret;\n>>>>\n>>>> @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>>>>\n>>>> \tLOG(RkISP1, Debug) << \"ISP output pad configured with \" << format.toString();\n>>>>\n>>>> -\tret = mainPathResizer_->setFormat(0, &format);\n>>>> -\tif (ret < 0)\n>>>> -\t\treturn ret;\n>>>> -\n>>>> -\tLOG(RkISP1, Debug) << \"Resizer input pad configured with \" << format.toString();\n>>>> -\n>>>> -\tformat.size = cfg.size;\n>>>> -\n>>>> -\tLOG(RkISP1, Debug) << \"Configuring resizer output pad with \" << format.toString();\n>>>> -\n>>>> -\tret = mainPathResizer_->setFormat(1, &format);\n>>>> -\tif (ret < 0)\n>>>> -\t\treturn ret;\n>>>> -\n>>>> -\tLOG(RkISP1, Debug) << \"Resizer output pad configured with \" << format.toString();\n>>>> -\n>>>> -\tV4L2DeviceFormat outputFormat = {};\n>>>> -\toutputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);\n>>>> -\toutputFormat.size = cfg.size;\n>>>> -\toutputFormat.planesCount = 2;\n>>>> -\n>>>> -\tret = mainPathVideo_->setFormat(&outputFormat);\n>>>> -\tif (ret)\n>>>> -\t\treturn ret;\n>>>> -\n>>>> -\tif (outputFormat.size != cfg.size ||\n>>>> -\t outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) {\n>>>> -\t\tLOG(RkISP1, Error)\n>>>> -\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n>>>> -\t\treturn -EINVAL;\n>>>> +\tdata->mainPathActive_ = false;\n>>>> +\tdata->selfPathActive_ = false;\n>>>> +\tfor (const StreamConfiguration &cfg : *config) {\n>>>> +\t\tV4L2SubdeviceFormat ispFormat = format;\n>>>> +\t\tV4L2Subdevice *resizer;\n>>>> +\t\tV4L2VideoDevice *video;\n>>>> +\n>>>> +\t\tif (cfg.stream() == &data->mainPathStream_) {\n>>>> +\t\t\tresizer = mainPathResizer_;\n>>>> +\t\t\tvideo = mainPathVideo_;\n>>>> +\t\t\tdata->mainPathActive_ = true;\n>>>> +\t\t} else {\n>>>> +\t\t\tresizer = selfPathResizer_;\n>>>> +\t\t\tvideo = selfPathVideo_;\n>>>> +\t\t\tdata->selfPathActive_ = true;\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tret = resizer->setFormat(0, &ispFormat);\n>>>> +\t\tif (ret < 0)\n>>>> +\t\t\treturn ret;\n>>>> +\n>>>> +\t\tLOG(RkISP1, Debug) << \"Resizer input pad configured with \" << ispFormat.toString();\n>>>> +\n>>>> +\t\tispFormat.size = cfg.size;\n>>>> +\n>>>> +\t\tLOG(RkISP1, Debug) << \"Configuring resizer output pad with \" << ispFormat.toString();\n>>>> +\n>>>> +\t\tret = resizer->setFormat(1, &ispFormat);\n>>>> +\t\tif (ret < 0)\n>>>> +\t\t\treturn ret;\n>>>> +\n>>>> +\t\tLOG(RkISP1, Debug) << \"Resizer output pad configured with \" << ispFormat.toString();\n>>>> +\n>>>> +\t\tV4L2DeviceFormat outputFormat = {};\n>>>> +\t\toutputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat);\n>>>> +\t\toutputFormat.size = cfg.size;\n>>>> +\t\toutputFormat.planesCount = 2;\n>>>\n>>> Doesn't the planes count depend on the format ?\n>>\n>> Good catch. This is a preexisting bug in the pipeline. I will add a \n>> separate patch to this series to sort this out.\n>>\n>>>\n>>>> +\n>>>> +\t\tret = video->setFormat(&outputFormat);\n>>>> +\t\tif (ret)\n>>>> +\t\t\treturn ret;\n>>>> +\n>>>> +\t\tif (outputFormat.size != cfg.size ||\n>>>> +\t\t outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) {\n>>>> +\t\t\tLOG(RkISP1, Error)\n>>>> +\t\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n>>>> +\t\t\treturn -EINVAL;\n>>>> +\t\t}\n>>>> \t}\n>>>>\n>>>> \tV4L2DeviceFormat paramFormat = {};\n>>>> @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>>>> \tif (ret)\n>>>> \t\treturn ret;\n>>>>\n>>>> -\tcfg.setStream(&data->mainPathStream_);\n>>>> -\n>>>> \treturn 0;\n>>>> }\n>>>>\n>>>> int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream,\n>>>> \t\t\t\t\t std::vector> *buffers)\n>>>> {\n>>>> +\tRkISP1CameraData *data = cameraData(camera);\n>>>> \tunsigned int count = stream->configuration().bufferCount;\n>>>> -\treturn mainPathVideo_->exportBuffers(count, buffers);\n>>>> +\n>>>> +\tif (stream == &data->mainPathStream_)\n>>>> +\t\treturn mainPathVideo_->exportBuffers(count, buffers);\n>>>> +\n>>>> +\tif (stream == &data->selfPathStream_)\n>>>> +\t\treturn selfPathVideo_->exportBuffers(count, buffers);\n>>>> +\n>>>> +\treturn -EINVAL;\n>>>> }\n>>>>\n>>>> int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n>>>> {\n>>>> \tRkISP1CameraData *data = cameraData(camera);\n>>>> -\tunsigned int count = data->mainPathStream_.configuration().bufferCount;\n>>>> \tunsigned int ipaBufferId = 1;\n>>>> \tint ret;\n>>>>\n>>>> -\tret = mainPathVideo_->importBuffers(count);\n>>>> -\tif (ret < 0)\n>>>> -\t\tgoto error;\n>>>> +\tunsigned int count = std::max({\n>>>> +\t\tdata->mainPathStream_.configuration().bufferCount,\n>>>> +\t\tdata->selfPathStream_.configuration().bufferCount,\n>>>> +\t});\n>>>> +\n>>>> +\tif (data->mainPathActive_) {\n>>>> +\t\tret = mainPathVideo_->importBuffers(count);\n>>>> +\t\tif (ret < 0)\n>>>> +\t\t\tgoto error;\n>>>\n>>> Is it safe to call releaseBuffers if importing fails ?\n>>\n>> Yes, is it not always safe to call releaseBuffers? The idea is that if \n>> allocateBuffers() fails it should undo all operations. Remember we \n>> depend on buffer orphaning so buffers allocated by for example the \n>> FrameBufferAllocater are still OK.\n>>\n>>>\n>>>> +\t}\n>>>> +\n>>>> +\tif (data->selfPathActive_) {\n>>>> +\t\tret = selfPathVideo_->importBuffers(count);\n>>>> +\t\tif (ret < 0)\n>>>> +\t\t\tgoto error;\n>>>> +\t}\n>>>>\n>>>> \tret = param_->allocateBuffers(count, ¶mBuffers_);\n>>>> \tif (ret < 0)\n>>>> @@ -754,6 +794,7 @@ error:\n>>>> \tparamBuffers_.clear();\n>>>> \tstatBuffers_.clear();\n>>>> \tmainPathVideo_->releaseBuffers();\n>>>> +\tselfPathVideo_->releaseBuffers();\n>>>>\n>>>> \treturn ret;\n>>>> }\n>>>> @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n>>>> \tif (mainPathVideo_->releaseBuffers())\n>>>> \t\tLOG(RkISP1, Error) << \"Failed to release main path buffers\";\n>>>>\n>>>> +\tif (selfPathVideo_->releaseBuffers())\n>>>> +\t\tLOG(RkISP1, Error) << \"Failed to release self path buffers\";\n>>>> +\n>>>> \treturn 0;\n>>>> }\n>>>>\n>>>> @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>>>> \t\treturn ret;\n>>>> \t}\n>>>>\n>>>> -\tret = mainPathVideo_->streamOn();\n>>>> -\tif (ret) {\n>>>> -\t\tparam_->streamOff();\n>>>> -\t\tstat_->streamOff();\n>>>> -\t\tdata->ipa_->stop();\n>>>> -\t\tfreeBuffers(camera);\n>>>> -\n>>>> -\t\tLOG(RkISP1, Error)\n>>>> -\t\t\t<< \"Failed to start camera \" << camera->id();\n>>>> +\tstd::map streamConfig;\n>>>> +\n>>>> +\tif (data->mainPathActive_) {\n>>>> +\t\tret = mainPathVideo_->streamOn();\n>>>> +\t\tif (ret) {\n>>>> +\t\t\tparam_->streamOff();\n>>>> +\t\t\tstat_->streamOff();\n>>>> +\t\t\tdata->ipa_->stop();\n>>>> +\t\t\tfreeBuffers(camera);\n>>>> +\n>>>> +\t\t\tLOG(RkISP1, Error)\n>>>> +\t\t\t\t<< \"Failed to start main path \" << camera->id();\n>>>> +\t\t\treturn ret;\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tstreamConfig[0] = {\n>>>> +\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n>>>> +\t\t\t.size = data->mainPathStream_.configuration().size,\n>>>> +\t\t};\n>>>\n>>> What's this for ? And why assume [0] is assigned to main path ?\n>>\n>> This is part of the yet undocumented IPA protocl for the RkISP1. The \n>> stream configuration map is mainpath = 0 and selfpath = 1. When we get \n>> the new IPC framework in place all of this should of course be \n>> documented. In reality this is have currently no impact as the RkISP1 \n>> IPA does not examine the stream configuration at all.\n>>\n>>>\n>>>> +\t}\n>>>> +\n>>>> +\tif (data->selfPathActive_) {\n>>>> +\t\tret = selfPathVideo_->streamOn();\n>>>> +\t\tif (ret) {\n>>>> +\t\t\tif (data->mainPathActive_)\n>>>> +\t\t\t\tmainPathVideo_->streamOff();\n>>>> +\n>>>> +\t\t\tparam_->streamOff();\n>>>> +\t\t\tstat_->streamOff();\n>>>> +\t\t\tdata->ipa_->stop();\n>>>> +\t\t\tfreeBuffers(camera);\n>>>> +\n>>>> +\t\t\tLOG(RkISP1, Error)\n>>>> +\t\t\t\t<< \"Failed to start self path \" << camera->id();\n>>>> +\t\t\treturn ret;\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tstreamConfig[1] = {\n>>>> +\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n>>>> +\t\t\t.size = data->selfPathStream_.configuration().size,\n>>>> +\t\t};\n>>>> \t}\n>>>>\n>>>> \tactiveCamera_ = camera;\n>>>> @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>>>> \t\tret = 0;\n>>>> \t}\n>>>>\n>>>> -\tstd::map streamConfig;\n>>>> -\tstreamConfig[0] = {\n>>>> -\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n>>>> -\t\t.size = data->mainPathStream_.configuration().size,\n>>>> -\t};\n>>>> -\n>>>> \tstd::map entityControls;\n>>>> \tentityControls.emplace(0, data->sensor_->controls());\n>>>>\n>>>> @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n>>>> \tRkISP1CameraData *data = cameraData(camera);\n>>>> \tint ret;\n>>>>\n>>>> -\tret = mainPathVideo_->streamOff();\n>>>> -\tif (ret)\n>>>> -\t\tLOG(RkISP1, Warning)\n>>>> -\t\t\t<< \"Failed to stop camera \" << camera->id();\n>>>> +\tif (data->selfPathActive_) {\n>>>> +\t\tret = selfPathVideo_->streamOff();\n>>>> +\t\tif (ret)\n>>>> +\t\t\tLOG(RkISP1, Warning)\n>>>> +\t\t\t\t<< \"Failed to stop self path \" << camera->id();\n>>>> +\t}\n>>>\n>>> Can we call streamOff unconditionally ? It depends on the driver\n>>> implementation I guess\n>>\n>> We can call it unconditionally, I think even v4l2-compliance tests \n>> multiple s_stream(1) and s_stream(0) calls does it not? But I agree \n>> there might of course be driver specific quirks.\n>>\n>>>\n>>> Overall the patch looks good!\n>>> Thanks\n>>> j\n>>>\n>>>> +\n>>>> +\tif (data->mainPathActive_) {\n>>>> +\t\tret = mainPathVideo_->streamOff();\n>>>> +\t\tif (ret)\n>>>> +\t\t\tLOG(RkISP1, Warning)\n>>>> +\t\t\t\t<< \"Failed to stop main path \" << camera->id();\n>>>> +\t}\n>>>>\n>>>> \tret = stat_->streamOff();\n>>>> \tif (ret)\n>>>> @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera,\n>>>> \t\tstd::string resizer;\n>>>> \t\tif (cfg.stream() == &data->mainPathStream_)\n>>>> \t\t\tresizer = \"rkisp1_resizer_mainpath\";\n>>>> +\t\telse if (cfg.stream() == &data->selfPathStream_)\n>>>> +\t\t\tresizer = \"rkisp1_resizer_selfpath\";\n>>>> \t\telse\n>>>> \t\t\treturn -EINVAL;\n>>>>\n>>>> --\n>>>> 2.28.0\n>>>>\n>>>> _______________________________________________\n>>>> libcamera-devel mailing list\n>>>> libcamera-devel@lists.libcamera.org\n>>>> https://lists.libcamera.org/listinfo/libcamera-devel\n>>\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 B292FC3B5B\n\tfor ;\n\tMon, 14 Sep 2020 11:24:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4626C62D27;\n\tMon, 14 Sep 2020 13:24:34 +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 16CAC62B90\n\tfor ;\n\tMon, 14 Sep 2020 13:24:32 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DD523275;\n\tMon, 14 Sep 2020 13:24:24 +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=\"Sxe1yXQY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600082665;\n\tbh=3QG2/dMTy4lVrjY2z85qY1bHNntcOz4FXj/B512jpfc=;\n\th=Reply-To:Subject:From:To:Cc:References:Date:In-Reply-To:From;\n\tb=Sxe1yXQYFQvUWWIMfQLs8s2eDVB5liItsyFl2jHhcv6BBxAC37ncEiPUPuvVByb6t\n\t0fu8TuAT28NBY2eAThz+LR9RAdGPYPG+hD8xLZiIrVS7NAEPPiVQJS/7HhsiHwbQYA\n\tWvI3WAnG7pVJHQBg5EbzIPTGPV1fAQfkZ9zbG0vM=","From":"Kieran Bingham ","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tJacopo Mondi ","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-11-niklas.soderlund@ragnatech.se>\n\t<20200820094652.n5itgknr3p7stg4a@uno.localdomain>\n\t<20200914110901.GI1127199@oden.dyn.berto.se>\n\t","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<92b81e60-465c-b16e-9359-381727a7ba6f@ideasonboard.com>","Date":"Mon, 14 Sep 2020 12:24:22 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1:\n\tConfigure self path","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","Reply-To":"kieran.bingham@ideasonboard.com","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":12497,"web_url":"https://patchwork.libcamera.org/comment/12497/","msgid":"<20200914112733.GK1127199@oden.dyn.berto.se>","date":"2020-09-14T11:27:33","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1:\n\tConfigure self path","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kieran,\n\nOn 2020-09-14 12:19:54 +0100, Kieran Bingham wrote:\n> Hi Niklas,\n> \n> On 14/09/2020 12:09, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> > \n> > On 2020-08-20 11:46:52 +0200, Jacopo Mondi wrote:\n> >> Hi Niklas,\n> >>\n> >> On Thu, Aug 13, 2020 at 02:52:43AM +0200, Niklas Söderlund wrote:\n> >>> Allow for both the main and self path streams to be configured. This\n> >>> change adds the self path as an internal stream to the pipeline handler.\n> >>> It is not exposed as a Camera stream so it can not yet be used.\n> >>>\n> >>> Signed-off-by: Niklas Söderlund \n> >>> ---\n> >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 197 ++++++++++++++++-------\n> >>> 1 file changed, 139 insertions(+), 58 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> index 132878c13b10b555..671098e11a2e2750 100644\n> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> @@ -133,7 +133,8 @@ public:\n> >>> \t\t\t V4L2VideoDevice *selfPathVideo)\n> >>> \t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> >>> \t\t frameInfo_(pipe), mainPathVideo_(mainPathVideo),\n> >>> -\t\t selfPathVideo_(selfPathVideo)\n> >>> +\t\t selfPathVideo_(selfPathVideo), mainPathActive_(false),\n> >>> +\t\t selfPathActive_(false)\n> >>> \t{\n> >>> \t}\n> >>>\n> >>> @@ -145,6 +146,7 @@ public:\n> >>> \tint loadIPA();\n> >>>\n> >>> \tStream mainPathStream_;\n> >>> +\tStream selfPathStream_;\n> >>\n> >> Would this, and all other duplicated fields be nicer is stored as\n> >> arrays and accessed with fixed indexes ? I don't mind the way it is\n> >> today though\n> > \n> > I guess this is another bikeshedding topic. I don't see any clear \n> > advantage of one over the other.\n> > \n> > mainPathStream_\n> > \n> > vs\n> > \n> > streams_[RkISP1::MainPAth]\n> > \n> > If we are to switch to an array approach I do think we should do so \n> > before or after this series. I picked this solution as it was the one \n> > already used in the pipeline.\n> \n> \n> Jumping into the bikeshed mostly because it really stood out in the\n> title of this and the preceding patch.\n> \n> The 'self' path sounds horrible.\n> I assume it derives from 'selfie' ... but really it's just a secondary ...\n> \n> \n> I would probably prefer the array route (can be refactored anytime,\n> doesn't have to be here). If one path is more feature full than the\n> other then perhaps naming them makes sense, but otherwise I'd just have\n> them as numerical pipelines.\n> \n> And rather than have an array of streams_ and an array of\n> V4L2VideoDevice and an array of Sensors etc... I think this can be\n> moddelled as a class, and then it's just a single array of that class.\n> \n> (Or if there's only two, It could actually be just two instances of that\n> class at that point)\n> \n> RkISPPipe main;\n> RkISPPipe secondary;\n\nThis have been suggested to another patch in this series and I agree \nit's a good idea. I think that work should go on-top (or as a series \nbefore) tho as it will be quiet large in LoC and I don't wish to do that \nwork in this already large series.\n\n> \n> \n> edit: ohh yuck. I see\n> > \n> > +\tselfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, \"rkisp1_selfpath\");\n> \n> so it's named the 'self path' at the ISP ...\n\nI know :-( So I think we are stuck with the man and self path names.\n\n> \n> --\n> Kieran\n> \n> \n> \n> >>\n> >>> \tCameraSensor *sensor_;\n> >>> \tunsigned int frame_;\n> >>> \tstd::vector ipaBuffers_;\n> >>> @@ -154,6 +156,9 @@ public:\n> >>> \tV4L2VideoDevice *mainPathVideo_;\n> >>> \tV4L2VideoDevice *selfPathVideo_;\n> >>>\n> >>> +\tbool mainPathActive_;\n> >>> +\tbool selfPathActive_;\n> >>> +\n> >>> private:\n> >>> \tvoid queueFrameAction(unsigned int frame,\n> >>> \t\t\t const IPAOperationData &action);\n> >>> @@ -615,7 +620,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >>> \tRkISP1CameraConfiguration *config =\n> >>> \t\tstatic_cast(c);\n> >>> \tRkISP1CameraData *data = cameraData(camera);\n> >>> -\tStreamConfiguration &cfg = config->at(0);\n> >>> \tCameraSensor *sensor = data->sensor_;\n> >>> \tint ret;\n> >>>\n> >>> @@ -657,36 +661,54 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >>>\n> >>> \tLOG(RkISP1, Debug) << \"ISP output pad configured with \" << format.toString();\n> >>>\n> >>> -\tret = mainPathResizer_->setFormat(0, &format);\n> >>> -\tif (ret < 0)\n> >>> -\t\treturn ret;\n> >>> -\n> >>> -\tLOG(RkISP1, Debug) << \"Resizer input pad configured with \" << format.toString();\n> >>> -\n> >>> -\tformat.size = cfg.size;\n> >>> -\n> >>> -\tLOG(RkISP1, Debug) << \"Configuring resizer output pad with \" << format.toString();\n> >>> -\n> >>> -\tret = mainPathResizer_->setFormat(1, &format);\n> >>> -\tif (ret < 0)\n> >>> -\t\treturn ret;\n> >>> -\n> >>> -\tLOG(RkISP1, Debug) << \"Resizer output pad configured with \" << format.toString();\n> >>> -\n> >>> -\tV4L2DeviceFormat outputFormat = {};\n> >>> -\toutputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);\n> >>> -\toutputFormat.size = cfg.size;\n> >>> -\toutputFormat.planesCount = 2;\n> >>> -\n> >>> -\tret = mainPathVideo_->setFormat(&outputFormat);\n> >>> -\tif (ret)\n> >>> -\t\treturn ret;\n> >>> -\n> >>> -\tif (outputFormat.size != cfg.size ||\n> >>> -\t outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) {\n> >>> -\t\tLOG(RkISP1, Error)\n> >>> -\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> >>> -\t\treturn -EINVAL;\n> >>> +\tdata->mainPathActive_ = false;\n> >>> +\tdata->selfPathActive_ = false;\n> >>> +\tfor (const StreamConfiguration &cfg : *config) {\n> >>> +\t\tV4L2SubdeviceFormat ispFormat = format;\n> >>> +\t\tV4L2Subdevice *resizer;\n> >>> +\t\tV4L2VideoDevice *video;\n> >>> +\n> >>> +\t\tif (cfg.stream() == &data->mainPathStream_) {\n> >>> +\t\t\tresizer = mainPathResizer_;\n> >>> +\t\t\tvideo = mainPathVideo_;\n> >>> +\t\t\tdata->mainPathActive_ = true;\n> >>> +\t\t} else {\n> >>> +\t\t\tresizer = selfPathResizer_;\n> >>> +\t\t\tvideo = selfPathVideo_;\n> >>> +\t\t\tdata->selfPathActive_ = true;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tret = resizer->setFormat(0, &ispFormat);\n> >>> +\t\tif (ret < 0)\n> >>> +\t\t\treturn ret;\n> >>> +\n> >>> +\t\tLOG(RkISP1, Debug) << \"Resizer input pad configured with \" << ispFormat.toString();\n> >>> +\n> >>> +\t\tispFormat.size = cfg.size;\n> >>> +\n> >>> +\t\tLOG(RkISP1, Debug) << \"Configuring resizer output pad with \" << ispFormat.toString();\n> >>> +\n> >>> +\t\tret = resizer->setFormat(1, &ispFormat);\n> >>> +\t\tif (ret < 0)\n> >>> +\t\t\treturn ret;\n> >>> +\n> >>> +\t\tLOG(RkISP1, Debug) << \"Resizer output pad configured with \" << ispFormat.toString();\n> >>> +\n> >>> +\t\tV4L2DeviceFormat outputFormat = {};\n> >>> +\t\toutputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat);\n> >>> +\t\toutputFormat.size = cfg.size;\n> >>> +\t\toutputFormat.planesCount = 2;\n> >>\n> >> Doesn't the planes count depend on the format ?\n> > \n> > Good catch. This is a preexisting bug in the pipeline. I will add a \n> > separate patch to this series to sort this out.\n> > \n> >>\n> >>> +\n> >>> +\t\tret = video->setFormat(&outputFormat);\n> >>> +\t\tif (ret)\n> >>> +\t\t\treturn ret;\n> >>> +\n> >>> +\t\tif (outputFormat.size != cfg.size ||\n> >>> +\t\t outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) {\n> >>> +\t\t\tLOG(RkISP1, Error)\n> >>> +\t\t\t\t<< \"Unable to configure capture in \" << cfg.toString();\n> >>> +\t\t\treturn -EINVAL;\n> >>> +\t\t}\n> >>> \t}\n> >>>\n> >>> \tV4L2DeviceFormat paramFormat = {};\n> >>> @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> >>> \tif (ret)\n> >>> \t\treturn ret;\n> >>>\n> >>> -\tcfg.setStream(&data->mainPathStream_);\n> >>> -\n> >>> \treturn 0;\n> >>> }\n> >>>\n> >>> int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream,\n> >>> \t\t\t\t\t std::vector> *buffers)\n> >>> {\n> >>> +\tRkISP1CameraData *data = cameraData(camera);\n> >>> \tunsigned int count = stream->configuration().bufferCount;\n> >>> -\treturn mainPathVideo_->exportBuffers(count, buffers);\n> >>> +\n> >>> +\tif (stream == &data->mainPathStream_)\n> >>> +\t\treturn mainPathVideo_->exportBuffers(count, buffers);\n> >>> +\n> >>> +\tif (stream == &data->selfPathStream_)\n> >>> +\t\treturn selfPathVideo_->exportBuffers(count, buffers);\n> >>> +\n> >>> +\treturn -EINVAL;\n> >>> }\n> >>>\n> >>> int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> >>> {\n> >>> \tRkISP1CameraData *data = cameraData(camera);\n> >>> -\tunsigned int count = data->mainPathStream_.configuration().bufferCount;\n> >>> \tunsigned int ipaBufferId = 1;\n> >>> \tint ret;\n> >>>\n> >>> -\tret = mainPathVideo_->importBuffers(count);\n> >>> -\tif (ret < 0)\n> >>> -\t\tgoto error;\n> >>> +\tunsigned int count = std::max({\n> >>> +\t\tdata->mainPathStream_.configuration().bufferCount,\n> >>> +\t\tdata->selfPathStream_.configuration().bufferCount,\n> >>> +\t});\n> >>> +\n> >>> +\tif (data->mainPathActive_) {\n> >>> +\t\tret = mainPathVideo_->importBuffers(count);\n> >>> +\t\tif (ret < 0)\n> >>> +\t\t\tgoto error;\n> >>\n> >> Is it safe to call releaseBuffers if importing fails ?\n> > \n> > Yes, is it not always safe to call releaseBuffers? The idea is that if \n> > allocateBuffers() fails it should undo all operations. Remember we \n> > depend on buffer orphaning so buffers allocated by for example the \n> > FrameBufferAllocater are still OK.\n> > \n> >>\n> >>> +\t}\n> >>> +\n> >>> +\tif (data->selfPathActive_) {\n> >>> +\t\tret = selfPathVideo_->importBuffers(count);\n> >>> +\t\tif (ret < 0)\n> >>> +\t\t\tgoto error;\n> >>> +\t}\n> >>>\n> >>> \tret = param_->allocateBuffers(count, ¶mBuffers_);\n> >>> \tif (ret < 0)\n> >>> @@ -754,6 +794,7 @@ error:\n> >>> \tparamBuffers_.clear();\n> >>> \tstatBuffers_.clear();\n> >>> \tmainPathVideo_->releaseBuffers();\n> >>> +\tselfPathVideo_->releaseBuffers();\n> >>>\n> >>> \treturn ret;\n> >>> }\n> >>> @@ -787,6 +828,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)\n> >>> \tif (mainPathVideo_->releaseBuffers())\n> >>> \t\tLOG(RkISP1, Error) << \"Failed to release main path buffers\";\n> >>>\n> >>> +\tif (selfPathVideo_->releaseBuffers())\n> >>> +\t\tLOG(RkISP1, Error) << \"Failed to release self path buffers\";\n> >>> +\n> >>> \treturn 0;\n> >>> }\n> >>>\n> >>> @@ -829,15 +873,47 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> >>> \t\treturn ret;\n> >>> \t}\n> >>>\n> >>> -\tret = mainPathVideo_->streamOn();\n> >>> -\tif (ret) {\n> >>> -\t\tparam_->streamOff();\n> >>> -\t\tstat_->streamOff();\n> >>> -\t\tdata->ipa_->stop();\n> >>> -\t\tfreeBuffers(camera);\n> >>> -\n> >>> -\t\tLOG(RkISP1, Error)\n> >>> -\t\t\t<< \"Failed to start camera \" << camera->id();\n> >>> +\tstd::map streamConfig;\n> >>> +\n> >>> +\tif (data->mainPathActive_) {\n> >>> +\t\tret = mainPathVideo_->streamOn();\n> >>> +\t\tif (ret) {\n> >>> +\t\t\tparam_->streamOff();\n> >>> +\t\t\tstat_->streamOff();\n> >>> +\t\t\tdata->ipa_->stop();\n> >>> +\t\t\tfreeBuffers(camera);\n> >>> +\n> >>> +\t\t\tLOG(RkISP1, Error)\n> >>> +\t\t\t\t<< \"Failed to start main path \" << camera->id();\n> >>> +\t\t\treturn ret;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tstreamConfig[0] = {\n> >>> +\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> >>> +\t\t\t.size = data->mainPathStream_.configuration().size,\n> >>> +\t\t};\n> >>\n> >> What's this for ? And why assume [0] is assigned to main path ?\n> > \n> > This is part of the yet undocumented IPA protocl for the RkISP1. The \n> > stream configuration map is mainpath = 0 and selfpath = 1. When we get \n> > the new IPC framework in place all of this should of course be \n> > documented. In reality this is have currently no impact as the RkISP1 \n> > IPA does not examine the stream configuration at all.\n> > \n> >>\n> >>> +\t}\n> >>> +\n> >>> +\tif (data->selfPathActive_) {\n> >>> +\t\tret = selfPathVideo_->streamOn();\n> >>> +\t\tif (ret) {\n> >>> +\t\t\tif (data->mainPathActive_)\n> >>> +\t\t\t\tmainPathVideo_->streamOff();\n> >>> +\n> >>> +\t\t\tparam_->streamOff();\n> >>> +\t\t\tstat_->streamOff();\n> >>> +\t\t\tdata->ipa_->stop();\n> >>> +\t\t\tfreeBuffers(camera);\n> >>> +\n> >>> +\t\t\tLOG(RkISP1, Error)\n> >>> +\t\t\t\t<< \"Failed to start self path \" << camera->id();\n> >>> +\t\t\treturn ret;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tstreamConfig[1] = {\n> >>> +\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n> >>> +\t\t\t.size = data->selfPathStream_.configuration().size,\n> >>> +\t\t};\n> >>> \t}\n> >>>\n> >>> \tactiveCamera_ = camera;\n> >>> @@ -852,12 +928,6 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> >>> \t\tret = 0;\n> >>> \t}\n> >>>\n> >>> -\tstd::map streamConfig;\n> >>> -\tstreamConfig[0] = {\n> >>> -\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> >>> -\t\t.size = data->mainPathStream_.configuration().size,\n> >>> -\t};\n> >>> -\n> >>> \tstd::map entityControls;\n> >>> \tentityControls.emplace(0, data->sensor_->controls());\n> >>>\n> >>> @@ -873,10 +943,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> >>> \tRkISP1CameraData *data = cameraData(camera);\n> >>> \tint ret;\n> >>>\n> >>> -\tret = mainPathVideo_->streamOff();\n> >>> -\tif (ret)\n> >>> -\t\tLOG(RkISP1, Warning)\n> >>> -\t\t\t<< \"Failed to stop camera \" << camera->id();\n> >>> +\tif (data->selfPathActive_) {\n> >>> +\t\tret = selfPathVideo_->streamOff();\n> >>> +\t\tif (ret)\n> >>> +\t\t\tLOG(RkISP1, Warning)\n> >>> +\t\t\t\t<< \"Failed to stop self path \" << camera->id();\n> >>> +\t}\n> >>\n> >> Can we call streamOff unconditionally ? It depends on the driver\n> >> implementation I guess\n> > \n> > We can call it unconditionally, I think even v4l2-compliance tests \n> > multiple s_stream(1) and s_stream(0) calls does it not? But I agree \n> > there might of course be driver specific quirks.\n> > \n> >>\n> >> Overall the patch looks good!\n> >> Thanks\n> >> j\n> >>\n> >>> +\n> >>> +\tif (data->mainPathActive_) {\n> >>> +\t\tret = mainPathVideo_->streamOff();\n> >>> +\t\tif (ret)\n> >>> +\t\t\tLOG(RkISP1, Warning)\n> >>> +\t\t\t\t<< \"Failed to stop main path \" << camera->id();\n> >>> +\t}\n> >>>\n> >>> \tret = stat_->streamOff();\n> >>> \tif (ret)\n> >>> @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera,\n> >>> \t\tstd::string resizer;\n> >>> \t\tif (cfg.stream() == &data->mainPathStream_)\n> >>> \t\t\tresizer = \"rkisp1_resizer_mainpath\";\n> >>> +\t\telse if (cfg.stream() == &data->selfPathStream_)\n> >>> +\t\t\tresizer = \"rkisp1_resizer_selfpath\";\n> >>> \t\telse\n> >>> \t\t\treturn -EINVAL;\n> >>>\n> >>> --\n> >>> 2.28.0\n> >>>\n> >>> _______________________________________________\n> >>> libcamera-devel mailing list\n> >>> libcamera-devel@lists.libcamera.org\n> >>> https://lists.libcamera.org/listinfo/libcamera-devel\n> > \n> \n> -- \n> Regards\n> --\n> Kieran","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 49F4EC3B5B\n\tfor ;\n\tMon, 14 Sep 2020 11:27:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D224A62DBE;\n\tMon, 14 Sep 2020 13:27:35 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ABA8862B90\n\tfor ;\n\tMon, 14 Sep 2020 13:27:34 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id r24so18338583ljm.3\n\tfor ;\n\tMon, 14 Sep 2020 04:27:34 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ta17sm3273080lfd.148.2020.09.14.04.27.33\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 14 Sep 2020 04:27:33 -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=\"XUM8b0qj\"; 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=rGO9E08dTWEPOlQYEzatd5XO8/dScc7Vh/3Z4ahBX4o=;\n\tb=XUM8b0qjpcM6zXDLbwij/90E/Vd5MHFCU0psf8rSTW3ELr0MD7GTxH0lH3taUgi+08\n\tKz0YsTUEHYeQNk5a1xk0dIEA5/slzjs1qAkGbur0SWpCwWNPXzkITjQIziCzAKdEvZPV\n\t78V5c79ZfkYR9IcElzqnRYBPFfYN0I7Io5W7TLJst5sJwcm/m7sBiU5m51tgHsA/5pML\n\tjS5Q9VqwCyer6VR5jjvZToddHXhxTy+hlXRkHoDRWCbneBPubS9Kt/jWMoj931xF+5l/\n\tnrp88o+bY9BvK49wRuBIHrSfy539IXLZS9x11NHQOp6224l9tdFLYb8nmcWVn4gVSwcf\n\tdRCA==","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=rGO9E08dTWEPOlQYEzatd5XO8/dScc7Vh/3Z4ahBX4o=;\n\tb=J2ghUXqbs2FADUR6EUGZmDT3tsZ8Vct6v6LfDXeYs5VMpO3mp2jml1v3oN9vwN2IDf\n\to1rjo4NuBKszVWn2r66UsCNBddzqqyLIk6vOoGXhhpEVKaxNzimaPvOX9QQFKivqQkF7\n\tQWyAiewfdjGO4D4OMdiQZ3ErIq0QDZ2olqFBBQi7/9JiIlULhaG71Ebgk5ePV53j74IH\n\tquqp54DFGbZvjkQrILEXmetADkReLRBiGidaPijIZNlziS43zL/d1kUlmnUF4KlKDXWF\n\ttyBX98m5bsT0OTJRmcwMrlO8SLjH2Sh+tQjxy+ttri+hKKsW0Zz0oP2FNXCKgfXd+k/1\n\tRf5w==","X-Gm-Message-State":"AOAM532GWFm16IxBgDu/liP3Ce9WU1VpQULg8x8bgR8S/gwjLdBvjgRU\n\t0K114Mmb0SIFnNAbTgxnbtidkOORLICTuQ==","X-Google-Smtp-Source":"ABdhPJwZ5nVdNZEhDt9Yw2OfWOJXr0AOKJ7WPn3ZxvC6JUkFKsjUm1dzZGqgi6BiVrmrIi6/f7BSFw==","X-Received":"by 2002:a2e:8041:: with SMTP id p1mr5374568ljg.164.1600082853957;\n\tMon, 14 Sep 2020 04:27:33 -0700 (PDT)","Date":"Mon, 14 Sep 2020 13:27:33 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kieran Bingham ","Message-ID":"<20200914112733.GK1127199@oden.dyn.berto.se>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-11-niklas.soderlund@ragnatech.se>\n\t<20200820094652.n5itgknr3p7stg4a@uno.localdomain>\n\t<20200914110901.GI1127199@oden.dyn.berto.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1:\n\tConfigure self path","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\" "}}]