[{"id":10899,"web_url":"https://patchwork.libcamera.org/comment/10899/","msgid":"<20200627101630.nnbcjdqywiazdc3o@uno.localdomain>","date":"2020-06-27T10:16:30","subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2\n\tand ImgU are stored in CameraData","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:42AM +0200, Niklas Söderlund wrote:\n> One is a pointer and the other is not. It is unintuitive to interact\n> with and with our current design of one ImgU for each camera there is no\n> advantage of having it as a pointer. Our current design is unlikely to\n> change as the system really only has one ImgU. Align the two to make the\n> pipeline more consistent.\n\nDo you recall why we wanted to assign ImgU devices dynamically ?\n\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 48 ++++++++++++----------------\n> 1 file changed, 20 insertions(+), 28 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 0ebd762982142471..6ae4728b470f9487 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -43,7 +43,7 @@ public:\n> \tvoid cio2BufferReady(FrameBuffer *buffer);\n>\n> \tCIO2Device cio2_;\n> -\tImgUDevice *imgu_;\n> +\tImgUDevice imgu_;\n>\n> \tStream outStream_;\n> \tStream vfStream_;\n> @@ -117,8 +117,6 @@ private:\n> \tint allocateBuffers(Camera *camera);\n> \tint freeBuffers(Camera *camera);\n>\n> -\tImgUDevice imgu0_;\n> -\tImgUDevice imgu1_;\n> \tMediaDevice *cio2MediaDev_;\n> \tMediaDevice *imguMediaDev_;\n> };\n> @@ -414,7 +412,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \tStream *outStream = &data->outStream_;\n> \tStream *vfStream = &data->vfStream_;\n> \tCIO2Device *cio2 = &data->cio2_;\n> -\tImgUDevice *imgu = data->imgu_;\n> +\tImgUDevice *imgu = &data->imgu_;\n> \tV4L2DeviceFormat outputFormat;\n> \tint ret;\n>\n> @@ -456,7 +454,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t * stream which is for raw capture, in which case no buffers will\n> \t * ever be queued to the ImgU.\n> \t */\n> -\tret = data->imgu_->enableLinks(true);\n> +\tret = data->imgu_.enableLinks(true);\n> \tif (ret)\n> \t\treturn ret;\n>\n> @@ -563,9 +561,9 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> \tunsigned int count = stream->configuration().bufferCount;\n>\n> \tif (stream == &data->outStream_)\n> -\t\treturn data->imgu_->output_.dev->exportBuffers(count, buffers);\n> +\t\treturn data->imgu_.output_.dev->exportBuffers(count, buffers);\n> \telse if (stream == &data->vfStream_)\n> -\t\treturn data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);\n> +\t\treturn data->imgu_.viewfinder_.dev->exportBuffers(count, buffers);\n> \telse if (stream == &data->rawStream_)\n> \t\treturn data->cio2_.exportBuffers(count, buffers);\n>\n> @@ -583,7 +581,7 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tImgUDevice *imgu = data->imgu_;\n> +\tImgUDevice *imgu = &data->imgu_;\n> \tunsigned int bufferCount;\n> \tint ret;\n>\n> @@ -604,7 +602,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n>\n> -\tdata->imgu_->freeBuffers();\n> +\tdata->imgu_.freeBuffers();\n>\n> \treturn 0;\n> }\n> @@ -613,7 +611,7 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> \tCIO2Device *cio2 = &data->cio2_;\n> -\tImgUDevice *imgu = data->imgu_;\n> +\tImgUDevice *imgu = &data->imgu_;\n> \tint ret;\n>\n> \t/* Allocate buffers for internal pipeline usage. */\n> @@ -650,7 +648,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tIPU3CameraData *data = cameraData(camera);\n> \tint ret = 0;\n>\n> -\tret |= data->imgu_->stop();\n> +\tret |= data->imgu_.stop();\n> \tret |= data->cio2_.stop();\n> \tif (ret)\n> \t\tLOG(IPU3, Warning) << \"Failed to stop camera \"\n> @@ -680,9 +678,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> \t\tint ret;\n>\n> \t\tif (stream == &data->outStream_)\n> -\t\t\tret = data->imgu_->output_.dev->queueBuffer(buffer);\n> +\t\t\tret = data->imgu_.output_.dev->queueBuffer(buffer);\n> \t\telse if (stream == &data->vfStream_)\n> -\t\t\tret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);\n> +\t\t\tret = data->imgu_.viewfinder_.dev->queueBuffer(buffer);\n> \t\telse\n> \t\t\tcontinue;\n>\n> @@ -757,16 +755,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> */\n> int PipelineHandlerIPU3::registerCameras()\n> {\n> -\tint ret;\n> -\n> -\tret = imgu0_.init(imguMediaDev_, 0);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tret = imgu1_.init(imguMediaDev_, 1);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> \t/*\n> \t * For each CSI-2 receiver on the IPU3, create a Camera if an\n> \t * image sensor is connected to it and the sensor can produce images\n> @@ -774,6 +762,8 @@ int PipelineHandlerIPU3::registerCameras()\n> \t */\n> \tunsigned int numCameras = 0;\n> \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> +\t\tint ret;\n> +\n> \t\tstd::unique_ptr data =\n> \t\t\tstd::make_unique(this);\n> \t\tstd::set streams = {\n> @@ -796,7 +786,9 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t * only, and assign imgu0 to the first one and imgu1 to the\n> \t\t * second.\n> \t\t */\n\n\t\t/**\n\t\t * \\todo Dynamically assign ImgU and output devices to each\n\t\t * stream and camera; as of now, limit support to two cameras\n\t\t * only, and assign imgu0 to the first one and imgu1 to the\n\t\t * second.\n\t\t */\n\nIs this a leftover ? Should you remove it as well?\n\n> -\t\tdata->imgu_ = numCameras ? &imgu1_ : &imgu0_;\n> +\t\tret = data->imgu_.init(imguMediaDev_, numCameras);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n>\n> \t\t/*\n> \t\t * Connect video devices' 'bufferReady' signals to their\n> @@ -808,11 +800,11 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t */\n> \t\tdata->cio2_.bufferReady.connect(data.get(),\n> \t\t\t\t\t&IPU3CameraData::cio2BufferReady);\n> -\t\tdata->imgu_->input_->bufferReady.connect(&data->cio2_,\n> +\t\tdata->imgu_.input_->bufferReady.connect(&data->cio2_,\n> \t\t\t\t\t&CIO2Device::tryReturnBuffer);\n> -\t\tdata->imgu_->output_.dev->bufferReady.connect(data.get(),\n> +\t\tdata->imgu_.output_.dev->bufferReady.connect(data.get(),\n> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> -\t\tdata->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),\n> +\t\tdata->imgu_.viewfinder_.dev->bufferReady.connect(data.get(),\n> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n>\n> \t\t/* Create and register the Camera instance. */\n> @@ -881,7 +873,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \t\treturn;\n> \t}\n>\n> -\timgu_->input_->queueBuffer(buffer);\n> +\timgu_.input_->queueBuffer(buffer);\n> }\n>\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\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 58832C2E69\n\tfor ;\n\tSat, 27 Jun 2020 10:13:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC7C9609D6;\n\tSat, 27 Jun 2020 12:13:00 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6D8B609C2\n\tfor ;\n\tSat, 27 Jun 2020 12:12:59 +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 relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 3E837C0003;\n\tSat, 27 Jun 2020 10:12:58 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sat, 27 Jun 2020 12:16:30 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200627101630.nnbcjdqywiazdc3o@uno.localdomain>","References":"<20200627030043.2088585-1-niklas.soderlund@ragnatech.se>\n\t<20200627030043.2088585-13-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200627030043.2088585-13-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2\n\tand ImgU are stored in CameraData","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":10904,"web_url":"https://patchwork.libcamera.org/comment/10904/","msgid":"<20200627104333.GE911447@oden.dyn.berto.se>","date":"2020-06-27T10:43:33","subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2\n\tand ImgU are stored in CameraData","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:16:30 +0200, Jacopo Mondi wrote:\n> On Sat, Jun 27, 2020 at 05:00:42AM +0200, Niklas Söderlund wrote:\n> > One is a pointer and the other is not. It is unintuitive to interact\n> > with and with our current design of one ImgU for each camera there is no\n> > advantage of having it as a pointer. Our current design is unlikely to\n> > change as the system really only has one ImgU. Align the two to make the\n> > pipeline more consistent.\n> \n> Do you recall why we wanted to assign ImgU devices dynamically ?\n\nWe had ideas of providing more then 2 streams per camera IIRC. This \nmight still happen but would likely be quiet an big change to the \npipeline so I see no reason to keep this small part in preparation for \nthis. Likely this would change in any case if we ever go down that \nroute.\n\n> \n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 48 ++++++++++++----------------\n> > 1 file changed, 20 insertions(+), 28 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 0ebd762982142471..6ae4728b470f9487 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -43,7 +43,7 @@ public:\n> > \tvoid cio2BufferReady(FrameBuffer *buffer);\n> >\n> > \tCIO2Device cio2_;\n> > -\tImgUDevice *imgu_;\n> > +\tImgUDevice imgu_;\n> >\n> > \tStream outStream_;\n> > \tStream vfStream_;\n> > @@ -117,8 +117,6 @@ private:\n> > \tint allocateBuffers(Camera *camera);\n> > \tint freeBuffers(Camera *camera);\n> >\n> > -\tImgUDevice imgu0_;\n> > -\tImgUDevice imgu1_;\n> > \tMediaDevice *cio2MediaDev_;\n> > \tMediaDevice *imguMediaDev_;\n> > };\n> > @@ -414,7 +412,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \tStream *outStream = &data->outStream_;\n> > \tStream *vfStream = &data->vfStream_;\n> > \tCIO2Device *cio2 = &data->cio2_;\n> > -\tImgUDevice *imgu = data->imgu_;\n> > +\tImgUDevice *imgu = &data->imgu_;\n> > \tV4L2DeviceFormat outputFormat;\n> > \tint ret;\n> >\n> > @@ -456,7 +454,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \t * stream which is for raw capture, in which case no buffers will\n> > \t * ever be queued to the ImgU.\n> > \t */\n> > -\tret = data->imgu_->enableLinks(true);\n> > +\tret = data->imgu_.enableLinks(true);\n> > \tif (ret)\n> > \t\treturn ret;\n> >\n> > @@ -563,9 +561,9 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > \tunsigned int count = stream->configuration().bufferCount;\n> >\n> > \tif (stream == &data->outStream_)\n> > -\t\treturn data->imgu_->output_.dev->exportBuffers(count, buffers);\n> > +\t\treturn data->imgu_.output_.dev->exportBuffers(count, buffers);\n> > \telse if (stream == &data->vfStream_)\n> > -\t\treturn data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);\n> > +\t\treturn data->imgu_.viewfinder_.dev->exportBuffers(count, buffers);\n> > \telse if (stream == &data->rawStream_)\n> > \t\treturn data->cio2_.exportBuffers(count, buffers);\n> >\n> > @@ -583,7 +581,7 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > -\tImgUDevice *imgu = data->imgu_;\n> > +\tImgUDevice *imgu = &data->imgu_;\n> > \tunsigned int bufferCount;\n> > \tint ret;\n> >\n> > @@ -604,7 +602,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> >\n> > -\tdata->imgu_->freeBuffers();\n> > +\tdata->imgu_.freeBuffers();\n> >\n> > \treturn 0;\n> > }\n> > @@ -613,7 +611,7 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \tCIO2Device *cio2 = &data->cio2_;\n> > -\tImgUDevice *imgu = data->imgu_;\n> > +\tImgUDevice *imgu = &data->imgu_;\n> > \tint ret;\n> >\n> > \t/* Allocate buffers for internal pipeline usage. */\n> > @@ -650,7 +648,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \tint ret = 0;\n> >\n> > -\tret |= data->imgu_->stop();\n> > +\tret |= data->imgu_.stop();\n> > \tret |= data->cio2_.stop();\n> > \tif (ret)\n> > \t\tLOG(IPU3, Warning) << \"Failed to stop camera \"\n> > @@ -680,9 +678,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > \t\tint ret;\n> >\n> > \t\tif (stream == &data->outStream_)\n> > -\t\t\tret = data->imgu_->output_.dev->queueBuffer(buffer);\n> > +\t\t\tret = data->imgu_.output_.dev->queueBuffer(buffer);\n> > \t\telse if (stream == &data->vfStream_)\n> > -\t\t\tret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);\n> > +\t\t\tret = data->imgu_.viewfinder_.dev->queueBuffer(buffer);\n> > \t\telse\n> > \t\t\tcontinue;\n> >\n> > @@ -757,16 +755,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > */\n> > int PipelineHandlerIPU3::registerCameras()\n> > {\n> > -\tint ret;\n> > -\n> > -\tret = imgu0_.init(imguMediaDev_, 0);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > -\tret = imgu1_.init(imguMediaDev_, 1);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > \t/*\n> > \t * For each CSI-2 receiver on the IPU3, create a Camera if an\n> > \t * image sensor is connected to it and the sensor can produce images\n> > @@ -774,6 +762,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t */\n> > \tunsigned int numCameras = 0;\n> > \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> > +\t\tint ret;\n> > +\n> > \t\tstd::unique_ptr data =\n> > \t\t\tstd::make_unique(this);\n> > \t\tstd::set streams = {\n> > @@ -796,7 +786,9 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\t * only, and assign imgu0 to the first one and imgu1 to the\n> > \t\t * second.\n> > \t\t */\n> \n> \t\t/**\n> \t\t * \\todo Dynamically assign ImgU and output devices to each\n> \t\t * stream and camera; as of now, limit support to two cameras\n> \t\t * only, and assign imgu0 to the first one and imgu1 to the\n> \t\t * second.\n> \t\t */\n> \n> Is this a leftover ? Should you remove it as well?\n\nGood point I will remove it, thanks.\n\n> \n> > -\t\tdata->imgu_ = numCameras ? &imgu1_ : &imgu0_;\n> > +\t\tret = data->imgu_.init(imguMediaDev_, numCameras);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> >\n> > \t\t/*\n> > \t\t * Connect video devices' 'bufferReady' signals to their\n> > @@ -808,11 +800,11 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\t */\n> > \t\tdata->cio2_.bufferReady.connect(data.get(),\n> > \t\t\t\t\t&IPU3CameraData::cio2BufferReady);\n> > -\t\tdata->imgu_->input_->bufferReady.connect(&data->cio2_,\n> > +\t\tdata->imgu_.input_->bufferReady.connect(&data->cio2_,\n> > \t\t\t\t\t&CIO2Device::tryReturnBuffer);\n> > -\t\tdata->imgu_->output_.dev->bufferReady.connect(data.get(),\n> > +\t\tdata->imgu_.output_.dev->bufferReady.connect(data.get(),\n> > \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> > -\t\tdata->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),\n> > +\t\tdata->imgu_.viewfinder_.dev->bufferReady.connect(data.get(),\n> > \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> >\n> > \t\t/* Create and register the Camera instance. */\n> > @@ -881,7 +873,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > \t\treturn;\n> > \t}\n> >\n> > -\timgu_->input_->queueBuffer(buffer);\n> > +\timgu_.input_->queueBuffer(buffer);\n> > }\n> >\n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\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 5FD41C2E69\n\tfor ;\n\tSat, 27 Jun 2020 10:43:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDE4B609DD;\n\tSat, 27 Jun 2020 12:43:36 +0200 (CEST)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 38547609C2\n\tfor ;\n\tSat, 27 Jun 2020 12:43:35 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id e4so12892129ljn.4\n\tfor ;\n\tSat, 27 Jun 2020 03:43:35 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\t23sm5845045lff.91.2020.06.27.03.43.33\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 27 Jun 2020 03:43: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=\"z2NFVBzY\"; 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=htKjDVcH+WOwSuI/6tUpaXkaAKGJwKjGmBVvKjGqlq8=;\n\tb=z2NFVBzYWiHEoJZ0cLYSwmnUfLlnAxse24ax6csUs13A/OAwJnUJx2PaMEAdDLgJpD\n\t3fs0L1iwBRWyDhMYvMGwwzI8zRJQS3hgW9PIUhYT5c6gP+b3Gjrvze3UzFBsMUaIW0gw\n\t3Sr/VFoFE+SsrwmN8ELjX8QiVAxv0Vwan0ZGZRjNDuv21y1Tkpnj1QIRiXIloejsQvrE\n\tymTtDvQaUP++4Zlu61fnvj43kXNsZXVaXATuQXjrqBzliVyc/e7CDXkVUkhNPt7Wh/Hx\n\tn9+picYEyhlJC0iUpnfmqeaqZpECS2UfLiFt6QB4ZUVl7Q+OPJs15O3qfr1SqQny9zbs\n\t5M+Q==","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=htKjDVcH+WOwSuI/6tUpaXkaAKGJwKjGmBVvKjGqlq8=;\n\tb=My2ohjbCNVWoePlunD7os6upiduQZL6xpAqxHV8kH1iIghw88iD8XBx6hX1JaxzaRK\n\tgmormcoz2ae30Z9CJpVhVZeI3vd3EXfA8Y4q0q8e4zS9qNqSx3j0VUW89J/pymgt3iK3\n\t0Rid9eoNL6gE12VH+bPBVvYivHHr2waSZi7mc9Zm2Nz4EqdlLX1REPr3LLm9MC18ekd/\n\tgEp8vc0//1Qx5zzqbC0Z1eEz8M9AqrtkeglAn4DnUpFp5aaqgUBvnI8lMjNwrSuWOE/p\n\t4PDRfbzGnYhmsX3qi326b4Q2/+s97fbz7/z60Vxt1sg3oy36wdzxINVCyHnsyh/aPprR\n\tVAQQ==","X-Gm-Message-State":"AOAM531aYLMu/WVudzOJoxi6v9z4v33rQg0JyzQC4OZjyXS33dBFfPaJ\n\t/7e0Fplo/GlrobUyEg95/kkWsJxXGPQ=","X-Google-Smtp-Source":"ABdhPJzK4yn04vi5S3vlQPRpUEhWTSgruIZwk49J/LM7ia89Js2hNiNlSp/qOSkfECBroVJFhp9qqg==","X-Received":"by 2002:a2e:2a42:: with SMTP id\n\tq63mr3794221ljq.265.1593254614467; \n\tSat, 27 Jun 2020 03:43:34 -0700 (PDT)","Date":"Sat, 27 Jun 2020 12:43:33 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200627104333.GE911447@oden.dyn.berto.se>","References":"<20200627030043.2088585-1-niklas.soderlund@ragnatech.se>\n\t<20200627030043.2088585-13-niklas.soderlund@ragnatech.se>\n\t<20200627101630.nnbcjdqywiazdc3o@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200627101630.nnbcjdqywiazdc3o@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2\n\tand ImgU are stored in CameraData","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":10914,"web_url":"https://patchwork.libcamera.org/comment/10914/","msgid":"<20200627164834.GJ5966@pendragon.ideasonboard.com>","date":"2020-06-27T16:48:34","subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2\n\tand ImgU are stored in CameraData","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 12:43:33PM +0200, Niklas Söderlund wrote:\n> On 2020-06-27 12:16:30 +0200, Jacopo Mondi wrote:\n> > On Sat, Jun 27, 2020 at 05:00:42AM +0200, Niklas Söderlund wrote:\n> > > One is a pointer and the other is not. It is unintuitive to interact\n> > > with and with our current design of one ImgU for each camera there is no\n> > > advantage of having it as a pointer. Our current design is unlikely to\n> > > change as the system really only has one ImgU. Align the two to make the\n> > > pipeline more consistent.\n> > \n> > Do you recall why we wanted to assign ImgU devices dynamically ?\n> \n> We had ideas of providing more then 2 streams per camera IIRC. This \n> might still happen but would likely be quiet an big change to the \n> pipeline so I see no reason to keep this small part in preparation for \n> this. Likely this would change in any case if we ever go down that \n> route.\n\nCorrect, we thought we could use two ImgU instances for a single camera\n(and we likely will need to do so in the future). I also agree that more\nchanges will be needed.\n\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 48 ++++++++++++----------------\n> > > 1 file changed, 20 insertions(+), 28 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 0ebd762982142471..6ae4728b470f9487 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -43,7 +43,7 @@ public:\n> > > \tvoid cio2BufferReady(FrameBuffer *buffer);\n> > >\n> > > \tCIO2Device cio2_;\n> > > -\tImgUDevice *imgu_;\n> > > +\tImgUDevice imgu_;\n> > >\n> > > \tStream outStream_;\n> > > \tStream vfStream_;\n> > > @@ -117,8 +117,6 @@ private:\n> > > \tint allocateBuffers(Camera *camera);\n> > > \tint freeBuffers(Camera *camera);\n> > >\n> > > -\tImgUDevice imgu0_;\n> > > -\tImgUDevice imgu1_;\n\nI'm not sure to like this much though. For all it's worth, I would have\ngone the other way around, make cio2_ a pointer too. There are 4 CIO2\ndevices, and we could thus support more than two cameras, provided\nthey're not all used at the same time. It would make sense to\ninstantiate 4 CIO2 devices in PipelineHandlerIPU3, and point to the\ncorresponding CIO2 in IPU3CameraData. That's most likely what we will\nend up doing, and if we embed both the CIO2 and IMGU in IPU3CameraData\nwe will then make more code changes that will rely on that design,\nmaking future cleanups more difficult. I also don't really see this\npatch performing any change that allows further simplifications in the\nrest of the series (as far as I can tell, 13/13 doesn't depend on it).\nI'd thus rather keep the current design, and if you want to unify the\ncio2_ and imgu_ field, replace the patch with one that would turn cio2_\ninto a pointer.\n\n> > > \tMediaDevice *cio2MediaDev_;\n> > > \tMediaDevice *imguMediaDev_;\n> > > };\n> > > @@ -414,7 +412,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > \tStream *outStream = &data->outStream_;\n> > > \tStream *vfStream = &data->vfStream_;\n> > > \tCIO2Device *cio2 = &data->cio2_;\n> > > -\tImgUDevice *imgu = data->imgu_;\n> > > +\tImgUDevice *imgu = &data->imgu_;\n> > > \tV4L2DeviceFormat outputFormat;\n> > > \tint ret;\n> > >\n> > > @@ -456,7 +454,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > \t * stream which is for raw capture, in which case no buffers will\n> > > \t * ever be queued to the ImgU.\n> > > \t */\n> > > -\tret = data->imgu_->enableLinks(true);\n> > > +\tret = data->imgu_.enableLinks(true);\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > >\n> > > @@ -563,9 +561,9 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > > \tunsigned int count = stream->configuration().bufferCount;\n> > >\n> > > \tif (stream == &data->outStream_)\n> > > -\t\treturn data->imgu_->output_.dev->exportBuffers(count, buffers);\n> > > +\t\treturn data->imgu_.output_.dev->exportBuffers(count, buffers);\n> > > \telse if (stream == &data->vfStream_)\n> > > -\t\treturn data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);\n> > > +\t\treturn data->imgu_.viewfinder_.dev->exportBuffers(count, buffers);\n> > > \telse if (stream == &data->rawStream_)\n> > > \t\treturn data->cio2_.exportBuffers(count, buffers);\n> > >\n> > > @@ -583,7 +581,7 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > > int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> > > {\n> > > \tIPU3CameraData *data = cameraData(camera);\n> > > -\tImgUDevice *imgu = data->imgu_;\n> > > +\tImgUDevice *imgu = &data->imgu_;\n> > > \tunsigned int bufferCount;\n> > > \tint ret;\n> > >\n> > > @@ -604,7 +602,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> > > {\n> > > \tIPU3CameraData *data = cameraData(camera);\n> > >\n> > > -\tdata->imgu_->freeBuffers();\n> > > +\tdata->imgu_.freeBuffers();\n> > >\n> > > \treturn 0;\n> > > }\n> > > @@ -613,7 +611,7 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> > > {\n> > > \tIPU3CameraData *data = cameraData(camera);\n> > > \tCIO2Device *cio2 = &data->cio2_;\n> > > -\tImgUDevice *imgu = data->imgu_;\n> > > +\tImgUDevice *imgu = &data->imgu_;\n> > > \tint ret;\n> > >\n> > > \t/* Allocate buffers for internal pipeline usage. */\n> > > @@ -650,7 +648,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > > \tIPU3CameraData *data = cameraData(camera);\n> > > \tint ret = 0;\n> > >\n> > > -\tret |= data->imgu_->stop();\n> > > +\tret |= data->imgu_.stop();\n> > > \tret |= data->cio2_.stop();\n> > > \tif (ret)\n> > > \t\tLOG(IPU3, Warning) << \"Failed to stop camera \"\n> > > @@ -680,9 +678,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > > \t\tint ret;\n> > >\n> > > \t\tif (stream == &data->outStream_)\n> > > -\t\t\tret = data->imgu_->output_.dev->queueBuffer(buffer);\n> > > +\t\t\tret = data->imgu_.output_.dev->queueBuffer(buffer);\n> > > \t\telse if (stream == &data->vfStream_)\n> > > -\t\t\tret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);\n> > > +\t\t\tret = data->imgu_.viewfinder_.dev->queueBuffer(buffer);\n> > > \t\telse\n> > > \t\t\tcontinue;\n> > >\n> > > @@ -757,16 +755,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > > */\n> > > int PipelineHandlerIPU3::registerCameras()\n> > > {\n> > > -\tint ret;\n> > > -\n> > > -\tret = imgu0_.init(imguMediaDev_, 0);\n> > > -\tif (ret)\n> > > -\t\treturn ret;\n> > > -\n> > > -\tret = imgu1_.init(imguMediaDev_, 1);\n> > > -\tif (ret)\n> > > -\t\treturn ret;\n> > > -\n> > > \t/*\n> > > \t * For each CSI-2 receiver on the IPU3, create a Camera if an\n> > > \t * image sensor is connected to it and the sensor can produce images\n> > > @@ -774,6 +762,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > > \t */\n> > > \tunsigned int numCameras = 0;\n> > > \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> > > +\t\tint ret;\n> > > +\n> > > \t\tstd::unique_ptr data =\n> > > \t\t\tstd::make_unique(this);\n> > > \t\tstd::set streams = {\n> > > @@ -796,7 +786,9 @@ int PipelineHandlerIPU3::registerCameras()\n> > > \t\t * only, and assign imgu0 to the first one and imgu1 to the\n> > > \t\t * second.\n> > > \t\t */\n> > \n> > \t\t/**\n> > \t\t * \\todo Dynamically assign ImgU and output devices to each\n> > \t\t * stream and camera; as of now, limit support to two cameras\n> > \t\t * only, and assign imgu0 to the first one and imgu1 to the\n> > \t\t * second.\n> > \t\t */\n> > \n> > Is this a leftover ? Should you remove it as well?\n> \n> Good point I will remove it, thanks.\n> \n> > > -\t\tdata->imgu_ = numCameras ? &imgu1_ : &imgu0_;\n> > > +\t\tret = data->imgu_.init(imguMediaDev_, numCameras);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > >\n> > > \t\t/*\n> > > \t\t * Connect video devices' 'bufferReady' signals to their\n> > > @@ -808,11 +800,11 @@ int PipelineHandlerIPU3::registerCameras()\n> > > \t\t */\n> > > \t\tdata->cio2_.bufferReady.connect(data.get(),\n> > > \t\t\t\t\t&IPU3CameraData::cio2BufferReady);\n> > > -\t\tdata->imgu_->input_->bufferReady.connect(&data->cio2_,\n> > > +\t\tdata->imgu_.input_->bufferReady.connect(&data->cio2_,\n> > > \t\t\t\t\t&CIO2Device::tryReturnBuffer);\n> > > -\t\tdata->imgu_->output_.dev->bufferReady.connect(data.get(),\n> > > +\t\tdata->imgu_.output_.dev->bufferReady.connect(data.get(),\n> > > \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> > > -\t\tdata->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),\n> > > +\t\tdata->imgu_.viewfinder_.dev->bufferReady.connect(data.get(),\n> > > \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> > >\n> > > \t\t/* Create and register the Camera instance. */\n> > > @@ -881,7 +873,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > \t\treturn;\n> > > \t}\n> > >\n> > > -\timgu_->input_->queueBuffer(buffer);\n> > > +\timgu_.input_->queueBuffer(buffer);\n> > > }\n> > >\n> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);","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 14EE0C2E66\n\tfor ;\n\tSat, 27 Jun 2020 16:48:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BC1F609C8;\n\tSat, 27 Jun 2020 18:48:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2162603B6\n\tfor ;\n\tSat, 27 Jun 2020 18:48:37 +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 27C7A576;\n\tSat, 27 Jun 2020 18:48:37 +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=\"SvHA1vmn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593276517;\n\tbh=q9LODwqmVj6WzjjDSG1TQ4s4Utf2QIbLQ4Gnj0YTNCQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SvHA1vmnXzAxBK1bH5hjB647bzHrFQ6lZdawvdI8N/H9kYDKbzDmOCF0E9+WrgBLw\n\tReLvWuUZ3DIe1n9356vKqQLgOruaLFXP+v2T/uJb/vk1LOp0BnfOLoTk104UinSkPU\n\t2Jy2h9mSQgTJ8W+o67YDNh7ZeLWkGQ0jiCHd5ZHk=","Date":"Sat, 27 Jun 2020 19:48:34 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200627164834.GJ5966@pendragon.ideasonboard.com>","References":"<20200627030043.2088585-1-niklas.soderlund@ragnatech.se>\n\t<20200627030043.2088585-13-niklas.soderlund@ragnatech.se>\n\t<20200627101630.nnbcjdqywiazdc3o@uno.localdomain>\n\t<20200627104333.GE911447@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200627104333.GE911447@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2\n\tand ImgU are stored in CameraData","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":10919,"web_url":"https://patchwork.libcamera.org/comment/10919/","msgid":"<20200627230511.GD1105424@oden.dyn.berto.se>","date":"2020-06-27T23:05:11","subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2\n\tand ImgU are stored in CameraData","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:48:34 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sat, Jun 27, 2020 at 12:43:33PM +0200, Niklas Söderlund wrote:\n> > On 2020-06-27 12:16:30 +0200, Jacopo Mondi wrote:\n> > > On Sat, Jun 27, 2020 at 05:00:42AM +0200, Niklas Söderlund wrote:\n> > > > One is a pointer and the other is not. It is unintuitive to interact\n> > > > with and with our current design of one ImgU for each camera there is no\n> > > > advantage of having it as a pointer. Our current design is unlikely to\n> > > > change as the system really only has one ImgU. Align the two to make the\n> > > > pipeline more consistent.\n> > > \n> > > Do you recall why we wanted to assign ImgU devices dynamically ?\n> > \n> > We had ideas of providing more then 2 streams per camera IIRC. This \n> > might still happen but would likely be quiet an big change to the \n> > pipeline so I see no reason to keep this small part in preparation for \n> > this. Likely this would change in any case if we ever go down that \n> > route.\n> \n> Correct, we thought we could use two ImgU instances for a single camera\n> (and we likely will need to do so in the future). I also agree that more\n> changes will be needed.\n> \n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 48 ++++++++++++----------------\n> > > > 1 file changed, 20 insertions(+), 28 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 0ebd762982142471..6ae4728b470f9487 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -43,7 +43,7 @@ public:\n> > > > \tvoid cio2BufferReady(FrameBuffer *buffer);\n> > > >\n> > > > \tCIO2Device cio2_;\n> > > > -\tImgUDevice *imgu_;\n> > > > +\tImgUDevice imgu_;\n> > > >\n> > > > \tStream outStream_;\n> > > > \tStream vfStream_;\n> > > > @@ -117,8 +117,6 @@ private:\n> > > > \tint allocateBuffers(Camera *camera);\n> > > > \tint freeBuffers(Camera *camera);\n> > > >\n> > > > -\tImgUDevice imgu0_;\n> > > > -\tImgUDevice imgu1_;\n> \n> I'm not sure to like this much though. For all it's worth, I would have\n> gone the other way around, make cio2_ a pointer too. There are 4 CIO2\n> devices, and we could thus support more than two cameras, provided\n> they're not all used at the same time. It would make sense to\n> instantiate 4 CIO2 devices in PipelineHandlerIPU3, and point to the\n> corresponding CIO2 in IPU3CameraData. That's most likely what we will\n> end up doing, and if we embed both the CIO2 and IMGU in IPU3CameraData\n> we will then make more code changes that will rely on that design,\n> making future cleanups more difficult. I also don't really see this\n> patch performing any change that allows further simplifications in the\n> rest of the series (as far as I can tell, 13/13 doesn't depend on it).\n> I'd thus rather keep the current design, and if you want to unify the\n> cio2_ and imgu_ field, replace the patch with one that would turn cio2_\n> into a pointer.\n\nYou are correct that 13/13 does not depend on this. This was something I \nspotted while working with the series which felt unnatural that the CIO2 \nand ImgU where handled differently.\n\nI find it unlikely that we will extend the IPU3 pipeline with support \nfor more then two cameras before we have at least added an basic IPA and \nand sorted out a lot of other things. Likewise I think it will be a \nwhile before we attempt to have a singe camera use more then one ImgU, \nif ever. I think code should reflect the current usage and take into \naccount future planed work which are schedule to happen and not \ntheoretical use-cases that are not on the horizon :-) I fear this is a \nbikesheeding issue and the patch itself does not add or remove much \nvalue from the real work in this series so I will drop it for v2.\n\n> \n> > > > \tMediaDevice *cio2MediaDev_;\n> > > > \tMediaDevice *imguMediaDev_;\n> > > > };\n> > > > @@ -414,7 +412,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > > \tStream *outStream = &data->outStream_;\n> > > > \tStream *vfStream = &data->vfStream_;\n> > > > \tCIO2Device *cio2 = &data->cio2_;\n> > > > -\tImgUDevice *imgu = data->imgu_;\n> > > > +\tImgUDevice *imgu = &data->imgu_;\n> > > > \tV4L2DeviceFormat outputFormat;\n> > > > \tint ret;\n> > > >\n> > > > @@ -456,7 +454,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > > \t * stream which is for raw capture, in which case no buffers will\n> > > > \t * ever be queued to the ImgU.\n> > > > \t */\n> > > > -\tret = data->imgu_->enableLinks(true);\n> > > > +\tret = data->imgu_.enableLinks(true);\n> > > > \tif (ret)\n> > > > \t\treturn ret;\n> > > >\n> > > > @@ -563,9 +561,9 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > > > \tunsigned int count = stream->configuration().bufferCount;\n> > > >\n> > > > \tif (stream == &data->outStream_)\n> > > > -\t\treturn data->imgu_->output_.dev->exportBuffers(count, buffers);\n> > > > +\t\treturn data->imgu_.output_.dev->exportBuffers(count, buffers);\n> > > > \telse if (stream == &data->vfStream_)\n> > > > -\t\treturn data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);\n> > > > +\t\treturn data->imgu_.viewfinder_.dev->exportBuffers(count, buffers);\n> > > > \telse if (stream == &data->rawStream_)\n> > > > \t\treturn data->cio2_.exportBuffers(count, buffers);\n> > > >\n> > > > @@ -583,7 +581,7 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > > > int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> > > > {\n> > > > \tIPU3CameraData *data = cameraData(camera);\n> > > > -\tImgUDevice *imgu = data->imgu_;\n> > > > +\tImgUDevice *imgu = &data->imgu_;\n> > > > \tunsigned int bufferCount;\n> > > > \tint ret;\n> > > >\n> > > > @@ -604,7 +602,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> > > > {\n> > > > \tIPU3CameraData *data = cameraData(camera);\n> > > >\n> > > > -\tdata->imgu_->freeBuffers();\n> > > > +\tdata->imgu_.freeBuffers();\n> > > >\n> > > > \treturn 0;\n> > > > }\n> > > > @@ -613,7 +611,7 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> > > > {\n> > > > \tIPU3CameraData *data = cameraData(camera);\n> > > > \tCIO2Device *cio2 = &data->cio2_;\n> > > > -\tImgUDevice *imgu = data->imgu_;\n> > > > +\tImgUDevice *imgu = &data->imgu_;\n> > > > \tint ret;\n> > > >\n> > > > \t/* Allocate buffers for internal pipeline usage. */\n> > > > @@ -650,7 +648,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > > > \tIPU3CameraData *data = cameraData(camera);\n> > > > \tint ret = 0;\n> > > >\n> > > > -\tret |= data->imgu_->stop();\n> > > > +\tret |= data->imgu_.stop();\n> > > > \tret |= data->cio2_.stop();\n> > > > \tif (ret)\n> > > > \t\tLOG(IPU3, Warning) << \"Failed to stop camera \"\n> > > > @@ -680,9 +678,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > > > \t\tint ret;\n> > > >\n> > > > \t\tif (stream == &data->outStream_)\n> > > > -\t\t\tret = data->imgu_->output_.dev->queueBuffer(buffer);\n> > > > +\t\t\tret = data->imgu_.output_.dev->queueBuffer(buffer);\n> > > > \t\telse if (stream == &data->vfStream_)\n> > > > -\t\t\tret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);\n> > > > +\t\t\tret = data->imgu_.viewfinder_.dev->queueBuffer(buffer);\n> > > > \t\telse\n> > > > \t\t\tcontinue;\n> > > >\n> > > > @@ -757,16 +755,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > > > */\n> > > > int PipelineHandlerIPU3::registerCameras()\n> > > > {\n> > > > -\tint ret;\n> > > > -\n> > > > -\tret = imgu0_.init(imguMediaDev_, 0);\n> > > > -\tif (ret)\n> > > > -\t\treturn ret;\n> > > > -\n> > > > -\tret = imgu1_.init(imguMediaDev_, 1);\n> > > > -\tif (ret)\n> > > > -\t\treturn ret;\n> > > > -\n> > > > \t/*\n> > > > \t * For each CSI-2 receiver on the IPU3, create a Camera if an\n> > > > \t * image sensor is connected to it and the sensor can produce images\n> > > > @@ -774,6 +762,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > > > \t */\n> > > > \tunsigned int numCameras = 0;\n> > > > \tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> > > > +\t\tint ret;\n> > > > +\n> > > > \t\tstd::unique_ptr data =\n> > > > \t\t\tstd::make_unique(this);\n> > > > \t\tstd::set streams = {\n> > > > @@ -796,7 +786,9 @@ int PipelineHandlerIPU3::registerCameras()\n> > > > \t\t * only, and assign imgu0 to the first one and imgu1 to the\n> > > > \t\t * second.\n> > > > \t\t */\n> > > \n> > > \t\t/**\n> > > \t\t * \\todo Dynamically assign ImgU and output devices to each\n> > > \t\t * stream and camera; as of now, limit support to two cameras\n> > > \t\t * only, and assign imgu0 to the first one and imgu1 to the\n> > > \t\t * second.\n> > > \t\t */\n> > > \n> > > Is this a leftover ? Should you remove it as well?\n> > \n> > Good point I will remove it, thanks.\n> > \n> > > > -\t\tdata->imgu_ = numCameras ? &imgu1_ : &imgu0_;\n> > > > +\t\tret = data->imgu_.init(imguMediaDev_, numCameras);\n> > > > +\t\tif (ret)\n> > > > +\t\t\treturn ret;\n> > > >\n> > > > \t\t/*\n> > > > \t\t * Connect video devices' 'bufferReady' signals to their\n> > > > @@ -808,11 +800,11 @@ int PipelineHandlerIPU3::registerCameras()\n> > > > \t\t */\n> > > > \t\tdata->cio2_.bufferReady.connect(data.get(),\n> > > > \t\t\t\t\t&IPU3CameraData::cio2BufferReady);\n> > > > -\t\tdata->imgu_->input_->bufferReady.connect(&data->cio2_,\n> > > > +\t\tdata->imgu_.input_->bufferReady.connect(&data->cio2_,\n> > > > \t\t\t\t\t&CIO2Device::tryReturnBuffer);\n> > > > -\t\tdata->imgu_->output_.dev->bufferReady.connect(data.get(),\n> > > > +\t\tdata->imgu_.output_.dev->bufferReady.connect(data.get(),\n> > > > \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> > > > -\t\tdata->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),\n> > > > +\t\tdata->imgu_.viewfinder_.dev->bufferReady.connect(data.get(),\n> > > > \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> > > >\n> > > > \t\t/* Create and register the Camera instance. */\n> > > > @@ -881,7 +873,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > > \t\treturn;\n> > > > \t}\n> > > >\n> > > > -\timgu_->input_->queueBuffer(buffer);\n> > > > +\timgu_.input_->queueBuffer(buffer);\n> > > > }\n> > > >\n> > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\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 BB50DC2E69\n\tfor ;\n\tSat, 27 Jun 2020 23:05:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 36F05609DB;\n\tSun, 28 Jun 2020 01:05:14 +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 205BD609A9\n\tfor ;\n\tSun, 28 Jun 2020 01:05:13 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id s1so14028866ljo.0\n\tfor ;\n\tSat, 27 Jun 2020 16:05:13 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tj19sm3549870ljg.28.2020.06.27.16.05.11\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 27 Jun 2020 16:05:11 -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=\"1O77TLAk\"; 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=y2AOzqR+feRCBy6wiYpbny0sYCDD3Y9UKhYH4suBRis=;\n\tb=1O77TLAkZxYs6PzYczSuzF8nqoyZ+TgDY/r50Gq7fBwlY6j8pzere7FWxez1xt0ekj\n\tTWzWnLdWotz+IhJU4JDtsrodGbKZbTOTsYV6MHKDQpTl+7TTE9GlrI7SB0PPS1Aov2Zp\n\tvirYdFneiJOsZTj/cLFj67QlFZOGD1G8TsrMQcdeeiGzKgOpxnA9H4X84jmQ6J9uYPB5\n\tmotIOdREZyVXwYrY5rhVgUBfdIgLFAAxMpD5qPGDMOXl/WINKQje4qTIIZ1lfXjx+uuZ\n\tBU8TQrkMHmheAzFQy+EDhMZOBQQ7ABd7yResR9naQ/rnc2xD4iNN1kCffTTvu4YFaYb/\n\tmrOQ==","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=y2AOzqR+feRCBy6wiYpbny0sYCDD3Y9UKhYH4suBRis=;\n\tb=optrvBWfOOsjYi0f0jI+zJQmFIRHKwSjfHIkesO5pv6T1gnnSOzEUnzskWiyJ26IM7\n\tdUzy+YYxmpviljHmXOKcYiDeL+NQ5UgWod8gBC8ki1/9KpvLjYtNZdkg7kA1BQYhfRDr\n\tOxaId8D6jUgznyKGQMChFhOCfgblJgd+5DL0M6hITsX/j3u70NqBshUkeikvFRSRj0jm\n\tdCtlSAIWh75QFIPP7O/nHm5iE6AcKTzUVk6+C+gyzkPtNWXlIclyb2wUBFae+B6ZX17W\n\t9j8vUs0by0TTVrVqhFTx3Uu1puCAnOBav3tTojOtbTd5fbeAkzw7MsVzak+mOBByAAv1\n\tbLiQ==","X-Gm-Message-State":"AOAM531NsMeAELma2HImDOPN7fOH8QlcKQxE56CAOAfW7krWMOCE3FEh\n\tSB/ox+zinvsoqtjosZZiVz034mDiZP8=","X-Google-Smtp-Source":"ABdhPJy2Akou1TiTbyH1r4Rf1A5qtAg8YTK/a4lwsWSUBZ1qsBiiF+z0gCOlIKO7fvjWL0VvmMcGBw==","X-Received":"by 2002:a2e:8296:: with SMTP id\n\ty22mr4828038ljg.238.1593299112306; \n\tSat, 27 Jun 2020 16:05:12 -0700 (PDT)","Date":"Sun, 28 Jun 2020 01:05:11 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200627230511.GD1105424@oden.dyn.berto.se>","References":"<20200627030043.2088585-1-niklas.soderlund@ragnatech.se>\n\t<20200627030043.2088585-13-niklas.soderlund@ragnatech.se>\n\t<20200627101630.nnbcjdqywiazdc3o@uno.localdomain>\n\t<20200627104333.GE911447@oden.dyn.berto.se>\n\t<20200627164834.GJ5966@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200627164834.GJ5966@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2\n\tand ImgU are stored in CameraData","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\" "}}]