[{"id":464,"web_url":"https://patchwork.libcamera.org/comment/464/","msgid":"<20190121205352.GH4439@pendragon.ideasonboard.com>","date":"2019-01-21T20:53:52","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: ipu3: Create CIO2 V4L2\n\tdevices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Jan 21, 2019 at 06:27:04PM +0100, Jacopo Mondi wrote:\n> Create V4L2 devices for the CIO2 capture video nodes.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 36 +++++++++++++++++++++++++---\n> 1 file changed, 33 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index daf681c..0689ce8 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -15,6 +15,8 @@\n> #include \"log.h\"\n> #include \"media_device.h\"\n> #include \"pipeline_handler.h\"\n> +#include \"utils.h\"\n> +#include \"v4l2_device.h\"\n> \n> namespace libcamera {\n> \n> @@ -30,6 +32,9 @@ private:\n> \tMediaDevice *cio2_;\n> \tMediaDevice *imgu_;\n> \n> +\tstd::vector> videoDevices_;\n> +\n\nI think this is where you start needed per-camera data in the pipeline.\nI would already model it as such.\n\n> +\tvoid createVideoDevices();\n> \tvoid registerCameras(CameraManager *manager);\n> };\n> \n> @@ -91,6 +96,12 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer\n> \tcio2_->acquire();\n> \timgu_->acquire();\n> \n> +\t/* Create video device nodes for CIO2 outputs */\n> +\tif (cio2_->open())\n> +\t\tgoto error_release_mdev;\n> +\n\nDo you need to open the cio2 media device to create the V4L2Device\ninstances ?\n\n> +\tcreateVideoDevices();\n> +\n> \t/*\n> \t * Disable all links that are enabled by default on CIO2, as camera\n> \t * creation enables all valid links it finds.\n> @@ -98,9 +109,6 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer\n> \t * Close the CIO2 media device after, as links are enabled and should\n> \t * not need to be changed after.\n> \t */\n> -\tif (cio2_->open())\n> -\t\tgoto error_release_mdev;\n> -\n> \tif (cio2_->disableLinks())\n> \t\tgoto error_close_cio2;\n> \n> @@ -120,6 +128,28 @@ error_release_mdev:\n> \treturn false;\n> }\n> \n> +/*\n> + * Create video devices for the CIO2 unit capture nodes and cache them\n> + * for later reuse.\n> + */\n> +void PipelineHandlerIPU3::createVideoDevices()\n> +{\n> +\tfor (unsigned int id = 0; id < 3; ++id) {\n\nI assume you meant < 4 ?\n\n> +\t\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> +\t\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> +\t\tif (!cio2)\n> +\t\t\tcontinue;\n> +\n> +\t\tstd::unique_ptr dev =\n> +\t\t\tutils::make_unique(*cio2);\n\nDo we really need to use std::unique_ptr<> for this ? Ownership of the\nV4L2Device will never be transferred, so I assume ths reason is to get\nthe pointer deleted automatically. If you create a per-camera IPU3\npipeline object, you could embed V4L2Device in that object instead of\nallocating it manually, which would achieve the same without using\nstd::unique_ptr<>.\n\n> +\t\tif (dev->open())\n> +\t\t\tcontinue;\n> +\t\tdev->close();\n> +\n> +\t\tvideoDevices_.push_back(std::move(dev));\n> +\t}\n\nYou should only create V4L2 devices for the CIO2 channels associated\nwith a camera, the other ones are not needed. I would advice splitting\nthe creation of cameras from registerCameras() to a registerCamera()\nfunction, and moving creation of the V4L2 device there.\n\n> +}\n> +\n> /*\n> * Cameras are created associating an image sensor (represented by a\n> * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four","headers":{"Return-Path":"","Received":["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 B030F60B1D\n\tfor ;\n\tMon, 21 Jan 2019 21:53:53 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3441853E;\n\tMon, 21 Jan 2019 21:53:53 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548104033;\n\tbh=Xt0UCfwsLWa6ZUdY6HdnNwmzKEKc74YwKaE84L7uMtU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n0CfySODNpfPn2WZv9wz0k8i/NNK2/CsHscP6cZOPjEfE9FVyG1sr6TN2WxotprPf\n\tzVDxRLv8a2EjLeLsC4zOyHIkqYC+9E2PolYbWjq0wmMHvrFjc4Vp9Qa5iXlZ2x4zTR\n\t+pWamt4CaJpiib27qs/eZw3m15Lm7tsxIa7R7X1k=","Date":"Mon, 21 Jan 2019 22:53:52 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190121205352.GH4439@pendragon.ideasonboard.com>","References":"<20190121172705.19985-1-jacopo@jmondi.org>\n\t<20190121172705.19985-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190121172705.19985-6-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: ipu3: Create CIO2 V4L2\n\tdevices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 21 Jan 2019 20:53:53 -0000"}},{"id":502,"web_url":"https://patchwork.libcamera.org/comment/502/","msgid":"<20190122145505.tzj2i5sj65nnghmv@uno.localdomain>","date":"2019-01-22T14:55:05","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: ipu3: Create CIO2 V4L2\n\tdevices","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n this patch was sketeched out mainly to test creation of V4L2\ndevices in the pipeline handler. When I wrote this I didn't think that\neach CIO2 device had to be associated with a Camera, but that's actually the\ncase with the IPU3, so I welcome your suggestion to use this as an\nopportunity to start sketching out per camera specific data.\n\nRemember though, that currently Cameras do not have a reference to\ntheir pipeline handlers, but that will be soon added I assume.\n\nOn Mon, Jan 21, 2019 at 10:53:52PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Jan 21, 2019 at 06:27:04PM +0100, Jacopo Mondi wrote:\n> > Create V4L2 devices for the CIO2 capture video nodes.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 36 +++++++++++++++++++++++++---\n> > 1 file changed, 33 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index daf681c..0689ce8 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -15,6 +15,8 @@\n> > #include \"log.h\"\n> > #include \"media_device.h\"\n> > #include \"pipeline_handler.h\"\n> > +#include \"utils.h\"\n> > +#include \"v4l2_device.h\"\n> >\n> > namespace libcamera {\n> >\n> > @@ -30,6 +32,9 @@ private:\n> > \tMediaDevice *cio2_;\n> > \tMediaDevice *imgu_;\n> >\n> > +\tstd::vector> videoDevices_;\n> > +\n>\n> I think this is where you start needed per-camera data in the pipeline.\n> I would already model it as such.\n>\n> > +\tvoid createVideoDevices();\n> > \tvoid registerCameras(CameraManager *manager);\n> > };\n> >\n> > @@ -91,6 +96,12 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer\n> > \tcio2_->acquire();\n> > \timgu_->acquire();\n> >\n> > +\t/* Create video device nodes for CIO2 outputs */\n> > +\tif (cio2_->open())\n> > +\t\tgoto error_release_mdev;\n> > +\n>\n> Do you need to open the cio2 media device to create the V4L2Device\n> instances ?\n\nPossibly no, it has been enumerated already...\n\n>\n> > +\tcreateVideoDevices();\n> > +\n> > \t/*\n> > \t * Disable all links that are enabled by default on CIO2, as camera\n> > \t * creation enables all valid links it finds.\n> > @@ -98,9 +109,6 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer\n> > \t * Close the CIO2 media device after, as links are enabled and should\n> > \t * not need to be changed after.\n> > \t */\n> > -\tif (cio2_->open())\n> > -\t\tgoto error_release_mdev;\n> > -\n> > \tif (cio2_->disableLinks())\n> > \t\tgoto error_close_cio2;\n> >\n> > @@ -120,6 +128,28 @@ error_release_mdev:\n> > \treturn false;\n> > }\n> >\n> > +/*\n> > + * Create video devices for the CIO2 unit capture nodes and cache them\n> > + * for later reuse.\n> > + */\n> > +void PipelineHandlerIPU3::createVideoDevices()\n> > +{\n> > +\tfor (unsigned int id = 0; id < 3; ++id) {\n>\n> I assume you meant < 4 ?\n\nYes, I got confused by the 2 and the end of \"cio2\" :)\n\n>\n> > +\t\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > +\t\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > +\t\tif (!cio2)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tstd::unique_ptr dev =\n> > +\t\t\tutils::make_unique(*cio2);\n>\n> Do we really need to use std::unique_ptr<> for this ? Ownership of the\n> V4L2Device will never be transferred, so I assume ths reason is to get\n> the pointer deleted automatically. If you create a per-camera IPU3\n> pipeline object, you could embed V4L2Device in that object instead of\n> allocating it manually, which would achieve the same without using\n> std::unique_ptr<>.\n\nYes, automatic deletion was the reason.\n\nI'll see how I should design this, but if I will need a sub-class of a\ngeneric base CameraData class, I should instantiate it with:\n\n camera.data = new IPU3CameraData()\n\nand the problem of having to keep track of that instance (which might\ncontain the V4L2 device) will represent itself, won't it?\n\n>\n> > +\t\tif (dev->open())\n> > +\t\t\tcontinue;\n> > +\t\tdev->close();\n> > +\n> > +\t\tvideoDevices_.push_back(std::move(dev));\n> > +\t}\n>\n> You should only create V4L2 devices for the CIO2 channels associated\n> with a camera, the other ones are not needed. I would advice splitting\n> the creation of cameras from registerCameras() to a registerCamera()\n> function, and moving creation of the V4L2 device there.\n>\n\nAgreed, let's use this to model Camera specific data.\n\nThanks\n j\n\n> > +}\n> > +\n> > /*\n> > * Cameras are created associating an image sensor (represented by a\n> > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 034C260B1B\n\tfor ;\n\tTue, 22 Jan 2019 15:54:53 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 74C3710000E;\n\tTue, 22 Jan 2019 14:54:53 +0000 (UTC)"],"Date":"Tue, 22 Jan 2019 15:55:05 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190122145505.tzj2i5sj65nnghmv@uno.localdomain>","References":"<20190121172705.19985-1-jacopo@jmondi.org>\n\t<20190121172705.19985-6-jacopo@jmondi.org>\n\t<20190121205352.GH4439@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"lplpqdtr7edwwd7q\"","Content-Disposition":"inline","In-Reply-To":"<20190121205352.GH4439@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: ipu3: Create CIO2 V4L2\n\tdevices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 22 Jan 2019 14:54:54 -0000"}},{"id":504,"web_url":"https://patchwork.libcamera.org/comment/504/","msgid":"<20190122151543.GA11461@pendragon.ideasonboard.com>","date":"2019-01-22T15:15:43","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: ipu3: Create CIO2 V4L2\n\tdevices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Jan 22, 2019 at 03:55:05PM +0100, Jacopo Mondi wrote:\n> Hi Laurent,\n> this patch was sketeched out mainly to test creation of V4L2\n> devices in the pipeline handler. When I wrote this I didn't think that\n> each CIO2 device had to be associated with a Camera, but that's actually the\n> case with the IPU3, so I welcome your suggestion to use this as an\n> opportunity to start sketching out per camera specific data.\n> \n> Remember though, that currently Cameras do not have a reference to\n> their pipeline handlers, but that will be soon added I assume.\n\nYes, it is needed. Would you like to give it a try, solving all the\nlifetime management issues it creates as you go ? :-)\n\n> On Mon, Jan 21, 2019 at 10:53:52PM +0200, Laurent Pinchart wrote:\n> > On Mon, Jan 21, 2019 at 06:27:04PM +0100, Jacopo Mondi wrote:\n> >> Create V4L2 devices for the CIO2 capture video nodes.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 36 +++++++++++++++++++++++++---\n> >> 1 file changed, 33 insertions(+), 3 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index daf681c..0689ce8 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -15,6 +15,8 @@\n> >> #include \"log.h\"\n> >> #include \"media_device.h\"\n> >> #include \"pipeline_handler.h\"\n> >> +#include \"utils.h\"\n> >> +#include \"v4l2_device.h\"\n> >>\n> >> namespace libcamera {\n> >>\n> >> @@ -30,6 +32,9 @@ private:\n> >> \tMediaDevice *cio2_;\n> >> \tMediaDevice *imgu_;\n> >>\n> >> +\tstd::vector> videoDevices_;\n> >> +\n> >\n> > I think this is where you start needed per-camera data in the pipeline.\n> > I would already model it as such.\n> >\n> >> +\tvoid createVideoDevices();\n> >> \tvoid registerCameras(CameraManager *manager);\n> >> };\n> >>\n> >> @@ -91,6 +96,12 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer\n> >> \tcio2_->acquire();\n> >> \timgu_->acquire();\n> >>\n> >> +\t/* Create video device nodes for CIO2 outputs */\n> >> +\tif (cio2_->open())\n> >> +\t\tgoto error_release_mdev;\n> >> +\n> >\n> > Do you need to open the cio2 media device to create the V4L2Device\n> > instances ?\n> \n> Possibly no, it has been enumerated already...\n> \n> >> +\tcreateVideoDevices();\n> >> +\n> >> \t/*\n> >> \t * Disable all links that are enabled by default on CIO2, as camera\n> >> \t * creation enables all valid links it finds.\n> >> @@ -98,9 +109,6 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer\n> >> \t * Close the CIO2 media device after, as links are enabled and should\n> >> \t * not need to be changed after.\n> >> \t */\n> >> -\tif (cio2_->open())\n> >> -\t\tgoto error_release_mdev;\n> >> -\n> >> \tif (cio2_->disableLinks())\n> >> \t\tgoto error_close_cio2;\n> >>\n> >> @@ -120,6 +128,28 @@ error_release_mdev:\n> >> \treturn false;\n> >> }\n> >>\n> >> +/*\n> >> + * Create video devices for the CIO2 unit capture nodes and cache them\n> >> + * for later reuse.\n> >> + */\n> >> +void PipelineHandlerIPU3::createVideoDevices()\n> >> +{\n> >> +\tfor (unsigned int id = 0; id < 3; ++id) {\n> >\n> > I assume you meant < 4 ?\n> \n> Yes, I got confused by the 2 and the end of \"cio2\" :)\n> \n> >> +\t\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> >> +\t\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> >> +\t\tif (!cio2)\n> >> +\t\t\tcontinue;\n> >> +\n> >> +\t\tstd::unique_ptr dev =\n> >> +\t\t\tutils::make_unique(*cio2);\n> >\n> > Do we really need to use std::unique_ptr<> for this ? Ownership of the\n> > V4L2Device will never be transferred, so I assume ths reason is to get\n> > the pointer deleted automatically. If you create a per-camera IPU3\n> > pipeline object, you could embed V4L2Device in that object instead of\n> > allocating it manually, which would achieve the same without using\n> > std::unique_ptr<>.\n> \n> Yes, automatic deletion was the reason.\n> \n> I'll see how I should design this, but if I will need a sub-class of a\n> generic base CameraData class, I should instantiate it with:\n> \n> camera.data = new IPU3CameraData()\n\nOr possibly camera.setData(new IPU3CameraData()), with a camera.data()\naccessor. This is especially important if you want to use\nstd::unique_ptr<> to store the camera data, as we'll need to borrow\nreferences all the time, and having a data() accessor for that would be\nsimpler.\n\n> and the problem of having to keep track of that instance (which might\n> contain the V4L2 device) will represent itself, won't it?\n\nYou can either delete it in the destructor of the Camera class, or use a\nstd::unique_ptr<> to store it.\n\n> >> +\t\tif (dev->open())\n> >> +\t\t\tcontinue;\n> >> +\t\tdev->close();\n> >> +\n> >> +\t\tvideoDevices_.push_back(std::move(dev));\n> >> +\t}\n> >\n> > You should only create V4L2 devices for the CIO2 channels associated\n> > with a camera, the other ones are not needed. I would advice splitting\n> > the creation of cameras from registerCameras() to a registerCamera()\n> > function, and moving creation of the V4L2 device there.\n> \n> Agreed, let's use this to model Camera specific data.\n> \n> >> +}\n> >> +\n> >> /*\n> >> * Cameras are created associating an image sensor (represented by a\n> >> * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four","headers":{"Return-Path":"","Received":["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 D447060B1B\n\tfor ;\n\tTue, 22 Jan 2019 16:15:44 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 43FF653E;\n\tTue, 22 Jan 2019 16:15:44 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548170144;\n\tbh=FFE6nfZwNP5Rl+0PphueFyIPkNDY+W4cGzWotPbL3fg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wNqgNuM0PhE82eah0jYM/8j/PVCBAj8+IUyjsIXvFvbUTruyuu4DQoMd4fTHW9vSj\n\tsVYpX1vn59YSvxnP8r0flIQqjzxtWoFn7ufN7c3qsdJkWjEIFYlq1Yapb4WIAAeP1b\n\t1fF0jK9rmLtgNmijqnb1aASbWx+uOHXp7CfOeuMY=","Date":"Tue, 22 Jan 2019 17:15:43 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190122151543.GA11461@pendragon.ideasonboard.com>","References":"<20190121172705.19985-1-jacopo@jmondi.org>\n\t<20190121172705.19985-6-jacopo@jmondi.org>\n\t<20190121205352.GH4439@pendragon.ideasonboard.com>\n\t<20190122145505.tzj2i5sj65nnghmv@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190122145505.tzj2i5sj65nnghmv@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: ipu3: Create CIO2 V4L2\n\tdevices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 22 Jan 2019 15:15:45 -0000"}},{"id":505,"web_url":"https://patchwork.libcamera.org/comment/505/","msgid":"<20190122191233.GE4127@bigcity.dyn.berto.se>","date":"2019-01-22T19:12:33","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: ipu3: Create CIO2 V4L2\n\tdevices","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"On 2019-01-22 17:15:43 +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> On Tue, Jan 22, 2019 at 03:55:05PM +0100, Jacopo Mondi wrote:\n> > Hi Laurent,\n> > this patch was sketeched out mainly to test creation of V4L2\n> > devices in the pipeline handler. When I wrote this I didn't think that\n> > each CIO2 device had to be associated with a Camera, but that's actually the\n> > case with the IPU3, so I welcome your suggestion to use this as an\n> > opportunity to start sketching out per camera specific data.\n> > \n> > Remember though, that currently Cameras do not have a reference to\n> > their pipeline handlers, but that will be soon added I assume.\n> \n> Yes, it is needed. Would you like to give it a try, solving all the\n> lifetime management issues it creates as you go ? :-)\n\nI have had had a go at solving this, before you spend time of it have a \nlook at my pipe branch. It might not solve the issue, but I attempt to \ndo so :-)\n\nI feel there might be an improvement in there where we possibly could \nlift the responsibility hand managing the life-time issue from the \nspecific PipelineHandler to the base class of all PipelineHandlers. But \nI will not attempt to solve this now as there are a flora of other \nissues to sort out first.\n\n> \n> > On Mon, Jan 21, 2019 at 10:53:52PM +0200, Laurent Pinchart wrote:\n> > > On Mon, Jan 21, 2019 at 06:27:04PM +0100, Jacopo Mondi wrote:\n> > >> Create V4L2 devices for the CIO2 capture video nodes.\n> > >>\n> > >> Signed-off-by: Jacopo Mondi \n> > >> ---\n> > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 36 +++++++++++++++++++++++++---\n> > >> 1 file changed, 33 insertions(+), 3 deletions(-)\n> > >>\n> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> index daf681c..0689ce8 100644\n> > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> @@ -15,6 +15,8 @@\n> > >> #include \"log.h\"\n> > >> #include \"media_device.h\"\n> > >> #include \"pipeline_handler.h\"\n> > >> +#include \"utils.h\"\n> > >> +#include \"v4l2_device.h\"\n> > >>\n> > >> namespace libcamera {\n> > >>\n> > >> @@ -30,6 +32,9 @@ private:\n> > >> \tMediaDevice *cio2_;\n> > >> \tMediaDevice *imgu_;\n> > >>\n> > >> +\tstd::vector> videoDevices_;\n> > >> +\n> > >\n> > > I think this is where you start needed per-camera data in the pipeline.\n> > > I would already model it as such.\n> > >\n> > >> +\tvoid createVideoDevices();\n> > >> \tvoid registerCameras(CameraManager *manager);\n> > >> };\n> > >>\n> > >> @@ -91,6 +96,12 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer\n> > >> \tcio2_->acquire();\n> > >> \timgu_->acquire();\n> > >>\n> > >> +\t/* Create video device nodes for CIO2 outputs */\n> > >> +\tif (cio2_->open())\n> > >> +\t\tgoto error_release_mdev;\n> > >> +\n> > >\n> > > Do you need to open the cio2 media device to create the V4L2Device\n> > > instances ?\n> > \n> > Possibly no, it has been enumerated already...\n> > \n> > >> +\tcreateVideoDevices();\n> > >> +\n> > >> \t/*\n> > >> \t * Disable all links that are enabled by default on CIO2, as camera\n> > >> \t * creation enables all valid links it finds.\n> > >> @@ -98,9 +109,6 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer\n> > >> \t * Close the CIO2 media device after, as links are enabled and should\n> > >> \t * not need to be changed after.\n> > >> \t */\n> > >> -\tif (cio2_->open())\n> > >> -\t\tgoto error_release_mdev;\n> > >> -\n> > >> \tif (cio2_->disableLinks())\n> > >> \t\tgoto error_close_cio2;\n> > >>\n> > >> @@ -120,6 +128,28 @@ error_release_mdev:\n> > >> \treturn false;\n> > >> }\n> > >>\n> > >> +/*\n> > >> + * Create video devices for the CIO2 unit capture nodes and cache them\n> > >> + * for later reuse.\n> > >> + */\n> > >> +void PipelineHandlerIPU3::createVideoDevices()\n> > >> +{\n> > >> +\tfor (unsigned int id = 0; id < 3; ++id) {\n> > >\n> > > I assume you meant < 4 ?\n> > \n> > Yes, I got confused by the 2 and the end of \"cio2\" :)\n> > \n> > >> +\t\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > >> +\t\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > >> +\t\tif (!cio2)\n> > >> +\t\t\tcontinue;\n> > >> +\n> > >> +\t\tstd::unique_ptr dev =\n> > >> +\t\t\tutils::make_unique(*cio2);\n> > >\n> > > Do we really need to use std::unique_ptr<> for this ? Ownership of the\n> > > V4L2Device will never be transferred, so I assume ths reason is to get\n> > > the pointer deleted automatically. If you create a per-camera IPU3\n> > > pipeline object, you could embed V4L2Device in that object instead of\n> > > allocating it manually, which would achieve the same without using\n> > > std::unique_ptr<>.\n> > \n> > Yes, automatic deletion was the reason.\n> > \n> > I'll see how I should design this, but if I will need a sub-class of a\n> > generic base CameraData class, I should instantiate it with:\n> > \n> > camera.data = new IPU3CameraData()\n> \n> Or possibly camera.setData(new IPU3CameraData()), with a camera.data()\n> accessor. This is especially important if you want to use\n> std::unique_ptr<> to store the camera data, as we'll need to borrow\n> references all the time, and having a data() accessor for that would be\n> simpler.\n> \n> > and the problem of having to keep track of that instance (which might\n> > contain the V4L2 device) will represent itself, won't it?\n> \n> You can either delete it in the destructor of the Camera class, or use a\n> std::unique_ptr<> to store it.\n> \n> > >> +\t\tif (dev->open())\n> > >> +\t\t\tcontinue;\n> > >> +\t\tdev->close();\n> > >> +\n> > >> +\t\tvideoDevices_.push_back(std::move(dev));\n> > >> +\t}\n> > >\n> > > You should only create V4L2 devices for the CIO2 channels associated\n> > > with a camera, the other ones are not needed. I would advice splitting\n> > > the creation of cameras from registerCameras() to a registerCamera()\n> > > function, and moving creation of the V4L2 device there.\n> > \n> > Agreed, let's use this to model Camera specific data.\n> > \n> > >> +}\n> > >> +\n> > >> /*\n> > >> * Cameras are created associating an image sensor (represented by a\n> > >> * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E298260C7D\n\tfor ;\n\tTue, 22 Jan 2019 20:12:36 +0100 (CET)","by mail-lf1-x144.google.com with SMTP id v5so18930606lfe.7\n\tfor ;\n\tTue, 22 Jan 2019 11:12:36 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tm12sm132810lfc.30.2019.01.22.11.12.33\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 22 Jan 2019 11:12:34 -0800 (PST)"],"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\t:user-agent; bh=8Fccy1JH85Qk+3C/eP3eJTowdIMdwKiQ+qOzjPfezSw=;\n\tb=nr3R+bLbOI3kI4mZ6XwmidKE4uGs76gsrilYCsIpG333vpIWLoS4E1fY3fuGD+4UTZ\n\tF9JTQlcATRspavtsiACZqTgxAyPmWfBZ9+wFMIapK4sWs9tm6QRaJCDsHj6rXVwL74Ac\n\tooYL/Me8BzYR58rDEE8EiFo9g+U0SaOnYImHUUKH/FMuU6iIWTQCmzI3W5dVZ8sAAY85\n\tUal+F8JF3k+Nj0aBMaYgpyBQbX2zhjwk/KiXw5OI5VZ/+Q6qaMCucYps3g2IXyMyydMJ\n\tWXHffJPfvK5ZomKljDMthBKbD5ZHI6sfJoVBn+Hn6xgKLggqNkN9z4oP1xkUXYNA+jQI\n\tkagQ==","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:user-agent;\n\tbh=8Fccy1JH85Qk+3C/eP3eJTowdIMdwKiQ+qOzjPfezSw=;\n\tb=kjsMwRTLoF9BL/Jz+ek60DO7uPvJg6fLsVuUt3jtUCQrty3M/QrkayRVX/NOj8mPAF\n\t6LiCjoial5TKs79+2E0ru/pM/L6bpUE5bRQoE+F31a7aTt3/l4puUiPqZA+4yheHuhkS\n\tG3y84fbwzTUMBOwH3roKBdr8ZqCRaVvDH642bbHhsb3qrjne36v1GwG0nCQdFmIUxDsZ\n\ti5GLrS3o0Oj6Uu7yKl9oNxqD98Z4rOhHnNt7avMguTTKol0VKgegPvAiFW3rdTd8xlvL\n\tNecYwCH8VFVWeHBJEdayvgieIRxcKgq3qcQ6uMVaemwdt57d26e0sIVYE8YcveUmn5oD\n\t95ug==","X-Gm-Message-State":"AJcUukeGVxwEgYHD/eDfLo3F5X6j5soytz4Q++XdsNKREg6D5YrHstWU\n\tqCgrQ97JUKnWqHYe9yo7xbitOQ==","X-Google-Smtp-Source":"ALg8bN4W79+GXFJUzkZcaoCI2afWxcCdTs0QWj20tCnb2A6hXh0Cgsg7K2X1QmMoIUwRexe8qhwt9Q==","X-Received":"by 2002:a19:26ce:: with SMTP id\n\tm197mr21002480lfm.23.1548184355877; \n\tTue, 22 Jan 2019 11:12:35 -0800 (PST)","Date":"Tue, 22 Jan 2019 20:12:33 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20190122191233.GE4127@bigcity.dyn.berto.se>","References":"<20190121172705.19985-1-jacopo@jmondi.org>\n\t<20190121172705.19985-6-jacopo@jmondi.org>\n\t<20190121205352.GH4439@pendragon.ideasonboard.com>\n\t<20190122145505.tzj2i5sj65nnghmv@uno.localdomain>\n\t<20190122151543.GA11461@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190122151543.GA11461@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: ipu3: Create CIO2 V4L2\n\tdevices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 22 Jan 2019 19:12:37 -0000"}}]