[{"id":563,"web_url":"https://patchwork.libcamera.org/comment/563/","msgid":"<20190124190630.GE4127@bigcity.dyn.berto.se>","date":"2019-01-24T19:06:30","subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-01-24 12:30:20 +0100, Jacopo Mondi wrote:\n> Create V4L2 devices for the CIO2 capture video nodes and associate them\n> with the camera they are part of.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> 1 file changed, 50 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 8cbc10a..9f5a40f 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> @@ -29,9 +31,19 @@ public:\n> \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> \n> private:\n> +\tclass IPU3CameraData : public CameraData\n> +\t{\n> +\tpublic:\n> +\t\tIPU3CameraData()\n> +\t\t\t: dev_(nullptr) { }\n> +\t\t~IPU3CameraData() { delete dev_; }\n> +\t\tV4L2Device *dev_;\n> +\t};\n> +\n> \tMediaDevice *cio2_;\n> \tMediaDevice *imgu_;\n> \n> +\tV4L2Device *createVideoDevice(unsigned int id);\n> \tvoid registerCameras(CameraManager *manager);\n> };\n> \n> @@ -122,6 +134,24 @@ error_release_mdev:\n> \treturn false;\n> }\n> \n> +/* Create video devices for the CIO2 unit associated with a camera. */\n> +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> +{\n> +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> +\tif (!cio2)\n> +\t\treturn nullptr;\n> +\n> +\tV4L2Device *dev = new V4L2Device(*cio2);\n> +\tif (dev->open()) {\n> +\t\tdelete dev;\n> +\t\treturn nullptr;\n> +\t}\n> +\tdev->close();\n> +\n> +\treturn dev;\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> @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> \n> \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> +\n> +\t\tsetCameraData(camera.get(),\n> +\t\t\t std::move(utils::make_unique()));\n> +\t\tIPU3CameraData *data =\n> +\t\t\treinterpret_cast(cameraData(camera.get()));\n\nI'm not saying this is not needed, only that it looks a bit complex to \nmy feeble mind. Could you educate me why the following would not work?\n\n IPU3CameraData *data = new IPU3CameraData();\n data->dev_ = videoDev;\n\n setCameraData(camera.get(), data);\n\nApart from this I think this commit looks good.\n\n> +\n> +\t\t/*\n> +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> +\t\t * registered. The 'camera' shared pointer goes out of scope\n> +\t\t * and deletes the Camera it manages.\n> +\t\t */\n> +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> +\t\tif (!videoDev) {\n> +\t\t\tLOG(IPU3, Error)\n> +\t\t\t\t<< \"Failed to register camera[\"\n> +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tdata->dev_ = videoDev;\n> \t\tmanager->addCamera(std::move(camera));\n> \n> \t\tLOG(IPU3, Info)\n> -- \n> 2.20.1\n> \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-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C814060C82\n\tfor ;\n\tThu, 24 Jan 2019 20:06:32 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id s5-v6so6201938ljd.12\n\tfor ;\n\tThu, 24 Jan 2019 11:06:32 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tb17sm1074135lfc.21.2019.01.24.11.06.31\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 24 Jan 2019 11:06:31 -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=xGWAlJ00gNFNZN+ZRuAxb8ARRCtuy6GFFEkOVQje5+M=;\n\tb=dH35YXFuYsucZki0fm4W6F5inU5gxqG9McfUXZq7f+7qG4nO4LPDD7QFTRTCdr9gOR\n\tY4SfvZEiAv52uBGdxgM/cHVohv6tVwY2w/204x97Qt9YkPNPpjeTdnlCHyllMdY73c3I\n\tyfxrqF5ZPtRN1R5xZZM0/Rv0cWqOF4By4rVxKH+vJo84fDTVn/33J/ZnzeZ8U8hq4b0o\n\tLUT2ygakyEJRYriRRT2CaHjKnjpm4FCv13WNUH5OrRVJ6KlAtf5O6Vx0E377u/DIFAIC\n\tQN9Jstl0kxUtHexE7382O/WXd0Wjs0fuiBbXxN9P1dHqdUzHWc88Oooqqz8E1e/FbMj5\n\tFubw==","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=xGWAlJ00gNFNZN+ZRuAxb8ARRCtuy6GFFEkOVQje5+M=;\n\tb=lDsKwmEXQW9Nzzm7NHUE2kpsZl9I2EZFQiz4KfnC/HV29/GdPNWLgKyp+5i9nq+jUn\n\tEY7WO0a9rIqyaaluDwvZ4iJihnpBqiqcqeIjwjA6GpzWrCxDvntAJWdM6jTChdXmj7VB\n\tlgGBapkGH7yfUXw1KF9xucS6FzhB9SUjWbe64d0b3Rh2yXNrJifc0Jhl5vGPq7oCKHPf\n\tRWcQF1AMSdoY7UmISxJSjqvzrG/01l3an3AteXrxsHZGmOzTfRwfRqJCsILKgQ8YG0gB\n\tYIL3qwq7+86qABt3gnAtFMXQ6Jpnzso6DPhmEgX8AO/0WnvtHk0eNtVLC2a45j+QfFp4\n\t/C/Q==","X-Gm-Message-State":"AJcUukd95uF33NEfSI+AnbuK2xVCFpwHl0CDaOfAk9maa7pMXaOSucrf\n\tmDF6l2YxHdBymreysvoaQDiwnA==","X-Google-Smtp-Source":"ALg8bN6YZUkI1ZdPBUOQxtFFw/O5m7+llPZdYm8kimn9YRhiweB/pc4QYL2/TSg4vAOc2Zw8CoPrRg==","X-Received":"by 2002:a2e:5d12:: with SMTP id\n\tr18-v6mr6422420ljb.89.1548356791969; \n\tThu, 24 Jan 2019 11:06:31 -0800 (PST)","Date":"Thu, 24 Jan 2019 20:06:30 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190124190630.GE4127@bigcity.dyn.berto.se>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190124113020.7203-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Thu, 24 Jan 2019 19:06:33 -0000"}},{"id":564,"web_url":"https://patchwork.libcamera.org/comment/564/","msgid":"<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>","date":"2019-01-24T19:33:00","subject":"Re: [libcamera-devel] [PATCH 2/2] 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 Niklas\n thanks for review\n\nOn Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2019-01-24 12:30:20 +0100, Jacopo Mondi wrote:\n> > Create V4L2 devices for the CIO2 capture video nodes and associate them\n> > with the camera they are part of.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> > 1 file changed, 50 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 8cbc10a..9f5a40f 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> > @@ -29,9 +31,19 @@ public:\n> > \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> >\n> > private:\n> > +\tclass IPU3CameraData : public CameraData\n> > +\t{\n> > +\tpublic:\n> > +\t\tIPU3CameraData()\n> > +\t\t\t: dev_(nullptr) { }\n> > +\t\t~IPU3CameraData() { delete dev_; }\n> > +\t\tV4L2Device *dev_;\n> > +\t};\n> > +\n> > \tMediaDevice *cio2_;\n> > \tMediaDevice *imgu_;\n> >\n> > +\tV4L2Device *createVideoDevice(unsigned int id);\n> > \tvoid registerCameras(CameraManager *manager);\n> > };\n> >\n> > @@ -122,6 +134,24 @@ error_release_mdev:\n> > \treturn false;\n> > }\n> >\n> > +/* Create video devices for the CIO2 unit associated with a camera. */\n> > +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> > +{\n> > +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > +\tif (!cio2)\n> > +\t\treturn nullptr;\n> > +\n> > +\tV4L2Device *dev = new V4L2Device(*cio2);\n> > +\tif (dev->open()) {\n> > +\t\tdelete dev;\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\tdev->close();\n> > +\n> > +\treturn dev;\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> > @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> >\n> > \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> > \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> > +\n> > +\t\tsetCameraData(camera.get(),\n> > +\t\t\t std::move(utils::make_unique()));\n> > +\t\tIPU3CameraData *data =\n> > +\t\t\treinterpret_cast(cameraData(camera.get()));\n>\n> I'm not saying this is not needed, only that it looks a bit complex to\n> my feeble mind. Could you educate me why the following would not work?\n>\n> IPU3CameraData *data = new IPU3CameraData();\n> data->dev_ = videoDev;\n>\n> setCameraData(camera.get(), data);\n\nsetCameraData wants a unique_ptr. On the reason why we're passing it\nby value (hence the std::move() ) instead than by reference and move()\ninside the function see the discussion on v1. Basically, it makes\nclear that after setCameraData() the local reference it's not\nvalid anymore.\n\nThat said, I could indeed have created a unique_ptr<> from an already\nexisting reference, instead of using std::make_unique.\n\nWhat I have now looks like the following:\n\n\t\tV4L2Device *videoDev = createVideoDevice(id);\n\t\tif (!videoDev) {\n\t\t\tLOG(IPU3, Error)\n\t\t\t\t<< \"Failed to register camera[\"\n\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n\t\t\tcontinue;\n\t\t}\n\n\t\tIPU3CameraData *data = new IPU3CameraData(*videoDev);\n\t\tsetCameraData(camera.get(),\n\t\t\t std::move(std::unique_ptr(data)));\n\t\tmanager->addCamera(std::move(camera));\n\nWhich is in my opinion nicer and equally safe. Thanks a lot for pointing\nthis out. Is it any better?\n\nThanks\n j\n\n>\n> Apart from this I think this commit looks good.\n>\n> > +\n> > +\t\t/*\n> > +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> > +\t\t * registered. The 'camera' shared pointer goes out of scope\n> > +\t\t * and deletes the Camera it manages.\n> > +\t\t */\n> > +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> > +\t\tif (!videoDev) {\n> > +\t\t\tLOG(IPU3, Error)\n> > +\t\t\t\t<< \"Failed to register camera[\"\n> > +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tdata->dev_ = videoDev;\n> > \t\tmanager->addCamera(std::move(camera));\n> >\n> > \t\tLOG(IPU3, Info)\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50B8960C82\n\tfor ;\n\tThu, 24 Jan 2019 20:32:48 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id BD3951C0006;\n\tThu, 24 Jan 2019 19:32:47 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 24 Jan 2019 20:33:00 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190124190630.GE4127@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"a66u7pkfaclx4mtk\"","Content-Disposition":"inline","In-Reply-To":"<20190124190630.GE4127@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Thu, 24 Jan 2019 19:32:48 -0000"}},{"id":566,"web_url":"https://patchwork.libcamera.org/comment/566/","msgid":"<20190124200354.GF4127@bigcity.dyn.berto.se>","date":"2019-01-24T20:03:54","subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Hi Jacopo,\n\nOn 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote:\n> Hi Niklas\n> thanks for review\n> \n> On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2019-01-24 12:30:20 +0100, Jacopo Mondi wrote:\n> > > Create V4L2 devices for the CIO2 capture video nodes and associate them\n> > > with the camera they are part of.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> > > 1 file changed, 50 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 8cbc10a..9f5a40f 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> > > @@ -29,9 +31,19 @@ public:\n> > > \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> > >\n> > > private:\n> > > +\tclass IPU3CameraData : public CameraData\n> > > +\t{\n> > > +\tpublic:\n> > > +\t\tIPU3CameraData()\n> > > +\t\t\t: dev_(nullptr) { }\n> > > +\t\t~IPU3CameraData() { delete dev_; }\n> > > +\t\tV4L2Device *dev_;\n> > > +\t};\n> > > +\n> > > \tMediaDevice *cio2_;\n> > > \tMediaDevice *imgu_;\n> > >\n> > > +\tV4L2Device *createVideoDevice(unsigned int id);\n> > > \tvoid registerCameras(CameraManager *manager);\n> > > };\n> > >\n> > > @@ -122,6 +134,24 @@ error_release_mdev:\n> > > \treturn false;\n> > > }\n> > >\n> > > +/* Create video devices for the CIO2 unit associated with a camera. */\n> > > +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> > > +{\n> > > +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > > +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > > +\tif (!cio2)\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\tV4L2Device *dev = new V4L2Device(*cio2);\n> > > +\tif (dev->open()) {\n> > > +\t\tdelete dev;\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\tdev->close();\n> > > +\n> > > +\treturn dev;\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> > > @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> > >\n> > > \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> > > \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> > > +\n> > > +\t\tsetCameraData(camera.get(),\n> > > +\t\t\t std::move(utils::make_unique()));\n> > > +\t\tIPU3CameraData *data =\n> > > +\t\t\treinterpret_cast(cameraData(camera.get()));\n> >\n> > I'm not saying this is not needed, only that it looks a bit complex to\n> > my feeble mind. Could you educate me why the following would not work?\n> >\n> > IPU3CameraData *data = new IPU3CameraData();\n> > data->dev_ = videoDev;\n> >\n> > setCameraData(camera.get(), data);\n> \n> setCameraData wants a unique_ptr. On the reason why we're passing it\n> by value (hence the std::move() ) instead than by reference and move()\n> inside the function see the discussion on v1. Basically, it makes\n> clear that after setCameraData() the local reference it's not\n> valid anymore.\n> \n> That said, I could indeed have created a unique_ptr<> from an already\n> existing reference, instead of using std::make_unique.\n> \n> What I have now looks like the following:\n> \n> \t\tV4L2Device *videoDev = createVideoDevice(id);\n> \t\tif (!videoDev) {\n> \t\t\tLOG(IPU3, Error)\n> \t\t\t\t<< \"Failed to register camera[\"\n> \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> \t\t\tcontinue;\n> \t\t}\n> \n> \t\tIPU3CameraData *data = new IPU3CameraData(*videoDev);\n> \t\tsetCameraData(camera.get(),\n> \t\t\t std::move(std::unique_ptr(data)));\n> \t\tmanager->addCamera(std::move(camera));\n> \n> Which is in my opinion nicer and equally safe. Thanks a lot for pointing\n> this out. Is it any better?\n\nThanks for the explanation! This looks good to me, expect you are \nmissing 'data->dev_ = videoDev' but I assume that is not critical to \nyour demonstration ;-) With this fix,\n\nReviewed-by: Niklas Söderlund \n\n> \n> Thanks\n> j\n> \n> >\n> > Apart from this I think this commit looks good.\n> >\n> > > +\n> > > +\t\t/*\n> > > +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> > > +\t\t * registered. The 'camera' shared pointer goes out of scope\n> > > +\t\t * and deletes the Camera it manages.\n> > > +\t\t */\n> > > +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> > > +\t\tif (!videoDev) {\n> > > +\t\t\tLOG(IPU3, Error)\n> > > +\t\t\t\t<< \"Failed to register camera[\"\n> > > +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > > +\t\t\tcontinue;\n> > > +\t\t}\n> > > +\n> > > +\t\tdata->dev_ = videoDev;\n> > > \t\tmanager->addCamera(std::move(camera));\n> > >\n> > > \t\tLOG(IPU3, Info)\n> > > --\n> > > 2.20.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["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 D692B60C82\n\tfor ;\n\tThu, 24 Jan 2019 21:03:56 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id s5-v6so6349165ljd.12\n\tfor ;\n\tThu, 24 Jan 2019 12:03:56 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tp21sm1120712lfj.10.2019.01.24.12.03.55\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 24 Jan 2019 12:03:55 -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=4OOtDOZ4FsOJwNk869x0/xq2VX4GIcef3ubEbfGlEmk=;\n\tb=coWnjYdHx2t+FlGouO21KcmzL1Xz779x7S9pi03YKxjT5WZiAUGZStvU5JgJIIDuOb\n\tft7QS3t2Mpb2uFniKglczU7Jhr/bdsc22hCb/L9NSESkH7G9y0LSvScoU+06Pf3MseYs\n\tl/npfPn/NE+iTJLg+I0DtsPmrcF+zIp7RaA1yqQzafMcrUVhQMpoluvd6m81M9bmB0co\n\tHAcIDKnAcyPfacN+yTF1c3op/S10n18S5J5ZD9QC3GlJ+zwtyrkl/G9HPPx83j7BG5R/\n\tJd6Zi8AbrNLa39PsWqU4LuDZxtj7gMibj9RiUi/9xQVpVwje2atQUWpqjse1+MoFMhLv\n\tOI2w==","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=4OOtDOZ4FsOJwNk869x0/xq2VX4GIcef3ubEbfGlEmk=;\n\tb=gWNZRsFx6bz6dSzXBgd7MeUsbwD/ZuAdt5YGpZZoJka/h6p682FQvrgde0gKsXFHy3\n\tqwYmhu/wBC5IACp+ukRLP8sKTHXnKjyco095FsQArCz+GJlt9sRG8gCM4z51hJ9g+nYa\n\tEZ+cD0LFKpIml3XBn4fotMfOi/T9qcCgkUiJKf1VAVIENPWuBKHQZl0H/6LCY0V7A84O\n\t7enV/QwcHlnCxc6YGfliRD+HG2pVUPtXDPuYv88xgTfw2KtL+Q1rY2S+6UWKSSlXmo1o\n\tUmo8wLFtKBTnSLrA01WciLYUYeIf9EE8NLB5UK+Oo7UqmZgN+Ln5ssaIHW5/BAg4t7p9\n\tw6FA==","X-Gm-Message-State":"AJcUukfGDyfv8kUcqGCcgqafbspbLHrjmeK3CE+3vuS/5n2SCneGpnV/\n\t4KbTk2sz4NwZL95T5ZVKgEAJfw==","X-Google-Smtp-Source":"ALg8bN4oGk2/rWm+Ey7hKKRB84G65PGFUTG88qkHYbDZKGks44UiY5Q80ux0GTJJxrFMZFcA8Q/EEA==","X-Received":"by 2002:a2e:89d7:: with SMTP id\n\tc23-v6mr6854923ljk.0.1548360235907; \n\tThu, 24 Jan 2019 12:03:55 -0800 (PST)","Date":"Thu, 24 Jan 2019 21:03:54 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190124200354.GF4127@bigcity.dyn.berto.se>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190124190630.GE4127@bigcity.dyn.berto.se>\n\t<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Thu, 24 Jan 2019 20:03:57 -0000"}},{"id":567,"web_url":"https://patchwork.libcamera.org/comment/567/","msgid":"<20190124201249.awolj4ogmondsnbv@uno.localdomain>","date":"2019-01-24T20:12:49","subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"On Thu, Jan 24, 2019 at 09:03:54PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote:\n> > Hi Niklas\n> > thanks for review\n> >\n> > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2019-01-24 12:30:20 +0100, Jacopo Mondi wrote:\n> > > > Create V4L2 devices for the CIO2 capture video nodes and associate them\n> > > > with the camera they are part of.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> > > > 1 file changed, 50 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 8cbc10a..9f5a40f 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> > > > @@ -29,9 +31,19 @@ public:\n> > > > \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> > > >\n> > > > private:\n> > > > +\tclass IPU3CameraData : public CameraData\n> > > > +\t{\n> > > > +\tpublic:\n> > > > +\t\tIPU3CameraData()\n> > > > +\t\t\t: dev_(nullptr) { }\n> > > > +\t\t~IPU3CameraData() { delete dev_; }\n> > > > +\t\tV4L2Device *dev_;\n> > > > +\t};\n> > > > +\n> > > > \tMediaDevice *cio2_;\n> > > > \tMediaDevice *imgu_;\n> > > >\n> > > > +\tV4L2Device *createVideoDevice(unsigned int id);\n> > > > \tvoid registerCameras(CameraManager *manager);\n> > > > };\n> > > >\n> > > > @@ -122,6 +134,24 @@ error_release_mdev:\n> > > > \treturn false;\n> > > > }\n> > > >\n> > > > +/* Create video devices for the CIO2 unit associated with a camera. */\n> > > > +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> > > > +{\n> > > > +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > > > +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > > > +\tif (!cio2)\n> > > > +\t\treturn nullptr;\n> > > > +\n> > > > +\tV4L2Device *dev = new V4L2Device(*cio2);\n> > > > +\tif (dev->open()) {\n> > > > +\t\tdelete dev;\n> > > > +\t\treturn nullptr;\n> > > > +\t}\n> > > > +\tdev->close();\n> > > > +\n> > > > +\treturn dev;\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> > > > @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> > > >\n> > > > \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> > > > \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> > > > +\n> > > > +\t\tsetCameraData(camera.get(),\n> > > > +\t\t\t std::move(utils::make_unique()));\n> > > > +\t\tIPU3CameraData *data =\n> > > > +\t\t\treinterpret_cast(cameraData(camera.get()));\n> > >\n> > > I'm not saying this is not needed, only that it looks a bit complex to\n> > > my feeble mind. Could you educate me why the following would not work?\n> > >\n> > > IPU3CameraData *data = new IPU3CameraData();\n> > > data->dev_ = videoDev;\n> > >\n> > > setCameraData(camera.get(), data);\n> >\n> > setCameraData wants a unique_ptr. On the reason why we're passing it\n> > by value (hence the std::move() ) instead than by reference and move()\n> > inside the function see the discussion on v1. Basically, it makes\n> > clear that after setCameraData() the local reference it's not\n> > valid anymore.\n> >\n> > That said, I could indeed have created a unique_ptr<> from an already\n> > existing reference, instead of using std::make_unique.\n> >\n> > What I have now looks like the following:\n> >\n> > \t\tV4L2Device *videoDev = createVideoDevice(id);\n> > \t\tif (!videoDev) {\n> > \t\t\tLOG(IPU3, Error)\n> > \t\t\t\t<< \"Failed to register camera[\"\n> > \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > \t\t\tcontinue;\n> > \t\t}\n> >\n> > \t\tIPU3CameraData *data = new IPU3CameraData(*videoDev);\n> > \t\tsetCameraData(camera.get(),\n> > \t\t\t std::move(std::unique_ptr(data)));\n> > \t\tmanager->addCamera(std::move(camera));\n> >\n> > Which is in my opinion nicer and equally safe. Thanks a lot for pointing\n> > this out. Is it any better?\n>\n> Thanks for the explanation! This looks good to me, expect you are\n> missing 'data->dev_ = videoDev' but I assume that is not critical to\n> your demonstration ;-) With this fix,\n\nI added it to the constructor, sorry, it's not shown here.\nWant me to remove that?\n\n>\n> Reviewed-by: Niklas Söderlund \n>\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > > Apart from this I think this commit looks good.\n> > >\n> > > > +\n> > > > +\t\t/*\n> > > > +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> > > > +\t\t * registered. The 'camera' shared pointer goes out of scope\n> > > > +\t\t * and deletes the Camera it manages.\n> > > > +\t\t */\n> > > > +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> > > > +\t\tif (!videoDev) {\n> > > > +\t\t\tLOG(IPU3, Error)\n> > > > +\t\t\t\t<< \"Failed to register camera[\"\n> > > > +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > > > +\t\t\tcontinue;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tdata->dev_ = videoDev;\n> > > > \t\tmanager->addCamera(std::move(camera));\n> > > >\n> > > > \t\tLOG(IPU3, Info)\n> > > > --\n> > > > 2.20.1\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n>\n>\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9955D60C82\n\tfor ;\n\tThu, 24 Jan 2019 21:12:36 +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 relay12.mail.gandi.net (Postfix) with ESMTPSA id 25E32200006;\n\tThu, 24 Jan 2019 20:12:35 +0000 (UTC)"],"Date":"Thu, 24 Jan 2019 21:12:49 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190124201249.awolj4ogmondsnbv@uno.localdomain>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190124190630.GE4127@bigcity.dyn.berto.se>\n\t<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>\n\t<20190124200354.GF4127@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"e5a6newwz4qykvb3\"","Content-Disposition":"inline","In-Reply-To":"<20190124200354.GF4127@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Thu, 24 Jan 2019 20:12:36 -0000"}},{"id":568,"web_url":"https://patchwork.libcamera.org/comment/568/","msgid":"<20190124201753.GG4127@bigcity.dyn.berto.se>","date":"2019-01-24T20:17:53","subject":"Re: [libcamera-devel] [PATCH 2/2] 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-24 21:12:49 +0100, Jacopo Mondi wrote:\n> On Thu, Jan 24, 2019 at 09:03:54PM +0100, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote:\n> > > Hi Niklas\n> > > thanks for review\n> > >\n> > > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thanks for your work.\n> > > >\n> > > > On 2019-01-24 12:30:20 +0100, Jacopo Mondi wrote:\n> > > > > Create V4L2 devices for the CIO2 capture video nodes and associate them\n> > > > > with the camera they are part of.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> > > > > 1 file changed, 50 insertions(+)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > index 8cbc10a..9f5a40f 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> > > > > @@ -29,9 +31,19 @@ public:\n> > > > > \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> > > > >\n> > > > > private:\n> > > > > +\tclass IPU3CameraData : public CameraData\n> > > > > +\t{\n> > > > > +\tpublic:\n> > > > > +\t\tIPU3CameraData()\n> > > > > +\t\t\t: dev_(nullptr) { }\n> > > > > +\t\t~IPU3CameraData() { delete dev_; }\n> > > > > +\t\tV4L2Device *dev_;\n> > > > > +\t};\n> > > > > +\n> > > > > \tMediaDevice *cio2_;\n> > > > > \tMediaDevice *imgu_;\n> > > > >\n> > > > > +\tV4L2Device *createVideoDevice(unsigned int id);\n> > > > > \tvoid registerCameras(CameraManager *manager);\n> > > > > };\n> > > > >\n> > > > > @@ -122,6 +134,24 @@ error_release_mdev:\n> > > > > \treturn false;\n> > > > > }\n> > > > >\n> > > > > +/* Create video devices for the CIO2 unit associated with a camera. */\n> > > > > +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> > > > > +{\n> > > > > +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > > > > +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > > > > +\tif (!cio2)\n> > > > > +\t\treturn nullptr;\n> > > > > +\n> > > > > +\tV4L2Device *dev = new V4L2Device(*cio2);\n> > > > > +\tif (dev->open()) {\n> > > > > +\t\tdelete dev;\n> > > > > +\t\treturn nullptr;\n> > > > > +\t}\n> > > > > +\tdev->close();\n> > > > > +\n> > > > > +\treturn dev;\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> > > > > @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> > > > >\n> > > > > \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> > > > > \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> > > > > +\n> > > > > +\t\tsetCameraData(camera.get(),\n> > > > > +\t\t\t std::move(utils::make_unique()));\n> > > > > +\t\tIPU3CameraData *data =\n> > > > > +\t\t\treinterpret_cast(cameraData(camera.get()));\n> > > >\n> > > > I'm not saying this is not needed, only that it looks a bit complex to\n> > > > my feeble mind. Could you educate me why the following would not work?\n> > > >\n> > > > IPU3CameraData *data = new IPU3CameraData();\n> > > > data->dev_ = videoDev;\n> > > >\n> > > > setCameraData(camera.get(), data);\n> > >\n> > > setCameraData wants a unique_ptr. On the reason why we're passing it\n> > > by value (hence the std::move() ) instead than by reference and move()\n> > > inside the function see the discussion on v1. Basically, it makes\n> > > clear that after setCameraData() the local reference it's not\n> > > valid anymore.\n> > >\n> > > That said, I could indeed have created a unique_ptr<> from an already\n> > > existing reference, instead of using std::make_unique.\n> > >\n> > > What I have now looks like the following:\n> > >\n> > > \t\tV4L2Device *videoDev = createVideoDevice(id);\n> > > \t\tif (!videoDev) {\n> > > \t\t\tLOG(IPU3, Error)\n> > > \t\t\t\t<< \"Failed to register camera[\"\n> > > \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > > \t\t\tcontinue;\n> > > \t\t}\n> > >\n> > > \t\tIPU3CameraData *data = new IPU3CameraData(*videoDev);\n> > > \t\tsetCameraData(camera.get(),\n> > > \t\t\t std::move(std::unique_ptr(data)));\n> > > \t\tmanager->addCamera(std::move(camera));\n> > >\n> > > Which is in my opinion nicer and equally safe. Thanks a lot for pointing\n> > > this out. Is it any better?\n> >\n> > Thanks for the explanation! This looks good to me, expect you are\n> > missing 'data->dev_ = videoDev' but I assume that is not critical to\n> > your demonstration ;-) With this fix,\n> \n> I added it to the constructor, sorry, it's not shown here.\n> Want me to remove that?\n\nI leave that up to you as the future IPU3 pipeline master :-)\n\n> \n> >\n> > Reviewed-by: Niklas Söderlund \n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > >\n> > > > Apart from this I think this commit looks good.\n> > > >\n> > > > > +\n> > > > > +\t\t/*\n> > > > > +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> > > > > +\t\t * registered. The 'camera' shared pointer goes out of scope\n> > > > > +\t\t * and deletes the Camera it manages.\n> > > > > +\t\t */\n> > > > > +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> > > > > +\t\tif (!videoDev) {\n> > > > > +\t\t\tLOG(IPU3, Error)\n> > > > > +\t\t\t\t<< \"Failed to register camera[\"\n> > > > > +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > > > > +\t\t\tcontinue;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tdata->dev_ = videoDev;\n> > > > > \t\tmanager->addCamera(std::move(camera));\n> > > > >\n> > > > > \t\tLOG(IPU3, Info)\n> > > > > --\n> > > > > 2.20.1\n> > > > >\n> > > > > _______________________________________________\n> > > > > libcamera-devel mailing list\n> > > > > libcamera-devel@lists.libcamera.org\n> > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > >\n> > > > --\n> > > > Regards,\n> > > > Niklas Söderlund\n> >\n> >\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["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 C3E5D60C82\n\tfor ;\n\tThu, 24 Jan 2019 21:17:55 +0100 (CET)","by mail-lj1-x243.google.com with SMTP id t9-v6so6409022ljh.6\n\tfor ;\n\tThu, 24 Jan 2019 12:17:55 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tr29-v6sm1134124ljd.44.2019.01.24.12.17.54\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 24 Jan 2019 12:17:54 -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=KMkES4dy+GjVlDs8N3GMiPTscThn5rc9cGkcCG9ykqs=;\n\tb=XjvUaSNDDIZew7F09LKqHUXdpaLPUD1Y+kGKy1VL0ds/Kvpu3JFwmuMJMeTryO3KWa\n\tniwyvgZ8WcJwkRQDAfBl0yYOXuht0IraDcDIN67oFifYSQyJgQMrQyKd+jtCv9gFEQBl\n\tTmVZh/M26YrSdE/n7Y8FjqDgAQbX+iquWmjwjXU0Nje80OgfBtdHrk2LgU2cZoCsgC53\n\t++fb8dkNREi3jAirCwlRwq9WxAFWWEILJRNApLc7qYR3HnzDIHqsqArO6h3ZJ3DSPFgV\n\tr1m72VMd1lyClgUf1jCyOGqvrZQDMH/LZ/dT6O2/3qZgeS6A/7Ui5PK6VN6be4Bv2se1\n\tRzRw==","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=KMkES4dy+GjVlDs8N3GMiPTscThn5rc9cGkcCG9ykqs=;\n\tb=o6M3+Je4m3S3coVeaZaczmfl2kVidvCAW1SO2qvcsg5592MjiGKEklCG0KhWwa2Iq7\n\tbgsG7F4AAUj+Cqt1LwAlFP4ihFWfw66edvbVrehaF1o4x0TlZmB78lXVOT42e5y02IQe\n\tISxhjVGZ08oRbW27XIlEEux0idS19vcyP8aiWhL24rRveR5LYvXHlqNZZMAzRQxevbWv\n\te2gWbXMSE/cHZFuqA9/mddtyWJ26y9ewkFENNsax/MNhqzNiM9aX6JYVnwukrIyyHI1b\n\tEwTaeo4NVlUi1HuX2Yam8l4MFm7lLD9j1z11Bhkjd4uPYbMaz8zO1LrrdwPIxMRCrCbY\n\tp4yg==","X-Gm-Message-State":"AJcUukenFEbktcTaUCayaUGBn8E/V7VdyUm55h2M3QTqghdqhyQCjs1J\n\tNaLsNfR5DZ4u0qH+bgECyMuM+g==","X-Google-Smtp-Source":"ALg8bN57RqRPvKbp/wkyGn5C5Z27CfiJj9SZyZlqdTEUO8wpDJGOU0KuUYt2gYHUKNY14otU/odJ2g==","X-Received":"by 2002:a2e:630a:: with SMTP id\n\tx10-v6mr6674592ljb.11.1548361074966; \n\tThu, 24 Jan 2019 12:17:54 -0800 (PST)","Date":"Thu, 24 Jan 2019 21:17:53 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190124201753.GG4127@bigcity.dyn.berto.se>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190124190630.GE4127@bigcity.dyn.berto.se>\n\t<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>\n\t<20190124200354.GF4127@bigcity.dyn.berto.se>\n\t<20190124201249.awolj4ogmondsnbv@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190124201249.awolj4ogmondsnbv@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Thu, 24 Jan 2019 20:17:56 -0000"}},{"id":588,"web_url":"https://patchwork.libcamera.org/comment/588/","msgid":"<20190125133036.the7h5o66r42doyl@uno.localdomain>","date":"2019-01-25T13:30:36","subject":"Re: [libcamera-devel] [PATCH 2/2] 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 Niklas,\n\nOn Thu, Jan 24, 2019 at 09:17:53PM +0100, Niklas Söderlund wrote:\n> On 2019-01-24 21:12:49 +0100, Jacopo Mondi wrote:\n> > On Thu, Jan 24, 2019 at 09:03:54PM +0100, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote:\n> > > > Hi Niklas\n> > > > thanks for review\n> > > >\n> > > > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > Thanks for your work.\n> > > > >\n> > > > > On 2019-01-24 12:30:20 +0100, Jacopo Mondi wrote:\n> > > > > > Create V4L2 devices for the CIO2 capture video nodes and associate them\n> > > > > > with the camera they are part of.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> > > > > > 1 file changed, 50 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > index 8cbc10a..9f5a40f 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> > > > > > @@ -29,9 +31,19 @@ public:\n> > > > > > \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> > > > > >\n> > > > > > private:\n> > > > > > +\tclass IPU3CameraData : public CameraData\n> > > > > > +\t{\n> > > > > > +\tpublic:\n> > > > > > +\t\tIPU3CameraData()\n> > > > > > +\t\t\t: dev_(nullptr) { }\n> > > > > > +\t\t~IPU3CameraData() { delete dev_; }\n> > > > > > +\t\tV4L2Device *dev_;\n> > > > > > +\t};\n> > > > > > +\n> > > > > > \tMediaDevice *cio2_;\n> > > > > > \tMediaDevice *imgu_;\n> > > > > >\n> > > > > > +\tV4L2Device *createVideoDevice(unsigned int id);\n> > > > > > \tvoid registerCameras(CameraManager *manager);\n> > > > > > };\n> > > > > >\n> > > > > > @@ -122,6 +134,24 @@ error_release_mdev:\n> > > > > > \treturn false;\n> > > > > > }\n> > > > > >\n> > > > > > +/* Create video devices for the CIO2 unit associated with a camera. */\n> > > > > > +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> > > > > > +{\n> > > > > > +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > > > > > +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > > > > > +\tif (!cio2)\n> > > > > > +\t\treturn nullptr;\n> > > > > > +\n> > > > > > +\tV4L2Device *dev = new V4L2Device(*cio2);\n> > > > > > +\tif (dev->open()) {\n> > > > > > +\t\tdelete dev;\n> > > > > > +\t\treturn nullptr;\n> > > > > > +\t}\n> > > > > > +\tdev->close();\n> > > > > > +\n> > > > > > +\treturn dev;\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> > > > > > @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> > > > > >\n> > > > > > \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> > > > > > \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> > > > > > +\n> > > > > > +\t\tsetCameraData(camera.get(),\n> > > > > > +\t\t\t std::move(utils::make_unique()));\n> > > > > > +\t\tIPU3CameraData *data =\n> > > > > > +\t\t\treinterpret_cast(cameraData(camera.get()));\n> > > > >\n> > > > > I'm not saying this is not needed, only that it looks a bit complex to\n> > > > > my feeble mind. Could you educate me why the following would not work?\n> > > > >\n> > > > > IPU3CameraData *data = new IPU3CameraData();\n> > > > > data->dev_ = videoDev;\n> > > > >\n> > > > > setCameraData(camera.get(), data);\n> > > >\n> > > > setCameraData wants a unique_ptr. On the reason why we're passing it\n> > > > by value (hence the std::move() ) instead than by reference and move()\n> > > > inside the function see the discussion on v1. Basically, it makes\n> > > > clear that after setCameraData() the local reference it's not\n> > > > valid anymore.\n> > > >\n> > > > That said, I could indeed have created a unique_ptr<> from an already\n> > > > existing reference, instead of using std::make_unique.\n> > > >\n> > > > What I have now looks like the following:\n> > > >\n> > > > \t\tV4L2Device *videoDev = createVideoDevice(id);\n> > > > \t\tif (!videoDev) {\n> > > > \t\t\tLOG(IPU3, Error)\n> > > > \t\t\t\t<< \"Failed to register camera[\"\n> > > > \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > > > \t\t\tcontinue;\n> > > > \t\t}\n> > > >\n> > > > \t\tIPU3CameraData *data = new IPU3CameraData(*videoDev);\n> > > > \t\tsetCameraData(camera.get(),\n> > > > \t\t\t std::move(std::unique_ptr(data)));\n> > > > \t\tmanager->addCamera(std::move(camera));\n> > > >\n> > > > Which is in my opinion nicer and equally safe. Thanks a lot for pointing\n> > > > this out. Is it any better?\n> > >\n> > > Thanks for the explanation! This looks good to me, expect you are\n> > > missing 'data->dev_ = videoDev' but I assume that is not critical to\n> > > your demonstration ;-) With this fix,\n> >\n> > I added it to the constructor, sorry, it's not shown here.\n> > Want me to remove that?\n>\n> I leave that up to you as the future IPU3 pipeline master :-)\n>\n\nI happly abdicate from this, but in the meantime I have pushed these\npatches.\n\nI dropped the v4l2 device as constructor parameter, as it will\nunlikely be the only per-camera object we'll have to store.\n\nThanks\n j\n\n> >\n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > >\n> > > > > Apart from this I think this commit looks good.\n> > > > >\n> > > > > > +\n> > > > > > +\t\t/*\n> > > > > > +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> > > > > > +\t\t * registered. The 'camera' shared pointer goes out of scope\n> > > > > > +\t\t * and deletes the Camera it manages.\n> > > > > > +\t\t */\n> > > > > > +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> > > > > > +\t\tif (!videoDev) {\n> > > > > > +\t\t\tLOG(IPU3, Error)\n> > > > > > +\t\t\t\t<< \"Failed to register camera[\"\n> > > > > > +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > > > > > +\t\t\tcontinue;\n> > > > > > +\t\t}\n> > > > > > +\n> > > > > > +\t\tdata->dev_ = videoDev;\n> > > > > > \t\tmanager->addCamera(std::move(camera));\n> > > > > >\n> > > > > > \t\tLOG(IPU3, Info)\n> > > > > > --\n> > > > > > 2.20.1\n> > > > > >\n> > > > > > _______________________________________________\n> > > > > > libcamera-devel mailing list\n> > > > > > libcamera-devel@lists.libcamera.org\n> > > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > >\n> > > > > --\n> > > > > Regards,\n> > > > > Niklas Söderlund\n> > >\n> > >\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n>\n>\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7AD760C7F\n\tfor ;\n\tFri, 25 Jan 2019 14:30:22 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 350D41C0009;\n\tFri, 25 Jan 2019 13:30:21 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 25 Jan 2019 14:30:36 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190125133036.the7h5o66r42doyl@uno.localdomain>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190124190630.GE4127@bigcity.dyn.berto.se>\n\t<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>\n\t<20190124200354.GF4127@bigcity.dyn.berto.se>\n\t<20190124201249.awolj4ogmondsnbv@uno.localdomain>\n\t<20190124201753.GG4127@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"a4anxbvwj2xwb3qb\"","Content-Disposition":"inline","In-Reply-To":"<20190124201753.GG4127@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Fri, 25 Jan 2019 13:30:22 -0000"}},{"id":598,"web_url":"https://patchwork.libcamera.org/comment/598/","msgid":"<20190125153109.GF4320@pendragon.ideasonboard.com>","date":"2019-01-25T15:31:09","subject":"Re: [libcamera-devel] [PATCH 2/2] 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 Thu, Jan 24, 2019 at 12:30:20PM +0100, Jacopo Mondi wrote:\n> Create V4L2 devices for the CIO2 capture video nodes and associate them\n> with the camera they are part of.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> 1 file changed, 50 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 8cbc10a..9f5a40f 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> @@ -29,9 +31,19 @@ public:\n> \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> \n> private:\n> +\tclass IPU3CameraData : public CameraData\n> +\t{\n> +\tpublic:\n> +\t\tIPU3CameraData()\n> +\t\t\t: dev_(nullptr) { }\n> +\t\t~IPU3CameraData() { delete dev_; }\n> +\t\tV4L2Device *dev_;\n\nYou will soon need to add data to this, so I wouldn't inline the\nconstructor and destructor. As the class may also get used by other\ncompilation units later on I'd define it outside of the\nPipelineHandlerIPU3 class, to make it easier to later move it to a\nheader if necessary.\n\n> +\t};\n> +\n> \tMediaDevice *cio2_;\n> \tMediaDevice *imgu_;\n> \n> +\tV4L2Device *createVideoDevice(unsigned int id);\n> \tvoid registerCameras(CameraManager *manager);\n> };\n> \n> @@ -122,6 +134,24 @@ error_release_mdev:\n> \treturn false;\n> }\n> \n> +/* Create video devices for the CIO2 unit associated with a camera. */\n> +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> +{\n> +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> +\tif (!cio2)\n> +\t\treturn nullptr;\n> +\n> +\tV4L2Device *dev = new V4L2Device(*cio2);\n> +\tif (dev->open()) {\n> +\t\tdelete dev;\n> +\t\treturn nullptr;\n> +\t}\n> +\tdev->close();\n> +\n> +\treturn dev;\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> @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> \n> \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> +\n> +\t\tsetCameraData(camera.get(),\n> +\t\t\t std::move(utils::make_unique()));\n> +\t\tIPU3CameraData *data =\n> +\t\t\treinterpret_cast(cameraData(camera.get()));\n\nAs I expect you'll need to call this quite often, how about creating a\n\n\tIPU3CameraData *cameraData(const Camera *);\n\nfunction in the PipelineHandlerIPU3 class ?\n\n> +\n> +\t\t/*\n> +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> +\t\t * registered. The 'camera' shared pointer goes out of scope\n> +\t\t * and deletes the Camera it manages.\n> +\t\t */\n> +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> +\t\tif (!videoDev) {\n> +\t\t\tLOG(IPU3, Error)\n> +\t\t\t\t<< \"Failed to register camera[\"\n> +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> +\t\t\tcontinue;\n\nIf this fails the camera will be deleted (as the sole reference will the\nthe shared pointer local to the loop), but the shared data will stay.\nIt's no big deal, it will be unused and be deleted when the pipeline\nhandler is deleted, but that still bothers me a bit. I wonder whether we\nshouldn't call setCameraData() right before manager->addCamera()\ninstead.\n\n> +\t\t}\n> +\n> +\t\tdata->dev_ = videoDev;\n> \t\tmanager->addCamera(std::move(camera));\n> \n> \t\tLOG(IPU3, Info)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D9CAE60B1B\n\tfor ;\n\tFri, 25 Jan 2019 16:31:14 +0100 (CET)","from pendragon.ideasonboard.com (unknown [109.132.30.169])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 45BF8325;\n\tFri, 25 Jan 2019 16:31:10 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548430270;\n\tbh=oiGCgg4McCH2LF5EJdeut1+RmbbF+SZpiVKgSkLr4L0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JtWHCvniLuSirdN2xgT4pCLcjM9mGDHFbn4HvneoDOCn8BWRoMrUZt8cp3r+ErFZ2\n\tdIGKXcJTIQSJ79Pqqzn/GET+0HYuMy6j4t2QOs5lXHFPNqZD216ZoJH3SIrlU3/Yfv\n\tjo7rnEBkbRk5sKQ0rsT+Hu9/EIlZwxrp8Ke1wE0w=","Date":"Fri, 25 Jan 2019 17:31:09 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190125153109.GF4320@pendragon.ideasonboard.com>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190124113020.7203-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Fri, 25 Jan 2019 15:31:15 -0000"}},{"id":599,"web_url":"https://patchwork.libcamera.org/comment/599/","msgid":"<20190125153612.GG4320@pendragon.ideasonboard.com>","date":"2019-01-25T15:36:12","subject":"Re: [libcamera-devel] [PATCH 2/2] 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 Thu, Jan 24, 2019 at 08:33:00PM +0100, Jacopo Mondi wrote:\n> On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:\n> > On 2019-01-24 12:30:20 +0100, Jacopo Mondi wrote:\n> >> Create V4L2 devices for the CIO2 capture video nodes and associate them\n> >> with the camera they are part of.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> >> 1 file changed, 50 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 8cbc10a..9f5a40f 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> >> @@ -29,9 +31,19 @@ public:\n> >> \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> >>\n> >> private:\n> >> +\tclass IPU3CameraData : public CameraData\n> >> +\t{\n> >> +\tpublic:\n> >> +\t\tIPU3CameraData()\n> >> +\t\t\t: dev_(nullptr) { }\n> >> +\t\t~IPU3CameraData() { delete dev_; }\n> >> +\t\tV4L2Device *dev_;\n> >> +\t};\n> >> +\n> >> \tMediaDevice *cio2_;\n> >> \tMediaDevice *imgu_;\n> >>\n> >> +\tV4L2Device *createVideoDevice(unsigned int id);\n> >> \tvoid registerCameras(CameraManager *manager);\n> >> };\n> >>\n> >> @@ -122,6 +134,24 @@ error_release_mdev:\n> >> \treturn false;\n> >> }\n> >>\n> >> +/* Create video devices for the CIO2 unit associated with a camera. */\n> >> +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> >> +{\n> >> +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> >> +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> >> +\tif (!cio2)\n> >> +\t\treturn nullptr;\n> >> +\n> >> +\tV4L2Device *dev = new V4L2Device(*cio2);\n> >> +\tif (dev->open()) {\n> >> +\t\tdelete dev;\n> >> +\t\treturn nullptr;\n> >> +\t}\n> >> +\tdev->close();\n> >> +\n> >> +\treturn dev;\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> >> @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> >>\n> >> \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> >> \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> >> +\n> >> +\t\tsetCameraData(camera.get(),\n> >> +\t\t\t std::move(utils::make_unique()));\n> >> +\t\tIPU3CameraData *data =\n> >> +\t\t\treinterpret_cast(cameraData(camera.get()));\n> >\n> > I'm not saying this is not needed, only that it looks a bit complex to\n> > my feeble mind. Could you educate me why the following would not work?\n> >\n> > IPU3CameraData *data = new IPU3CameraData();\n> > data->dev_ = videoDev;\n> >\n> > setCameraData(camera.get(), data);\n> \n> setCameraData wants a unique_ptr. On the reason why we're passing it\n> by value (hence the std::move() ) instead than by reference and move()\n> inside the function see the discussion on v1. Basically, it makes\n> clear that after setCameraData() the local reference it's not\n> valid anymore.\n> \n> That said, I could indeed have created a unique_ptr<> from an already\n> existing reference, instead of using std::make_unique.\n> \n> What I have now looks like the following:\n> \n> \t\tV4L2Device *videoDev = createVideoDevice(id);\n> \t\tif (!videoDev) {\n> \t\t\tLOG(IPU3, Error)\n> \t\t\t\t<< \"Failed to register camera[\"\n> \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> \t\t\tcontinue;\n> \t\t}\n> \n> \t\tIPU3CameraData *data = new IPU3CameraData(*videoDev);\n> \t\tsetCameraData(camera.get(),\n> \t\t\t std::move(std::unique_ptr(data)));\n> \t\tmanager->addCamera(std::move(camera));\n> \n> Which is in my opinion nicer and equally safe. Thanks a lot for pointing\n> this out. Is it any better?\n\nHow about\n\n\t\tstd::unique_ptr data = utils::make_unique();\n \t\tdata->dev_ = createVideoDevice(id);\n \t\tif (!data->dev_) {\n \t\t\tLOG(IPU3, Error)\n \t\t\t\t<< \"Failed to register camera[\"\n \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n \t\t\tcontinue;\n \t\t}\n \n \t\tsetCameraData(camera.get(), std::move(data));\n \t\tmanager->addCamera(std::move(camera));\n\n> > Apart from this I think this commit looks good.\n> >\n> >> +\n> >> +\t\t/*\n> >> +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> >> +\t\t * registered. The 'camera' shared pointer goes out of scope\n> >> +\t\t * and deletes the Camera it manages.\n> >> +\t\t */\n> >> +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> >> +\t\tif (!videoDev) {\n> >> +\t\t\tLOG(IPU3, Error)\n> >> +\t\t\t\t<< \"Failed to register camera[\"\n> >> +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> >> +\t\t\tcontinue;\n> >> +\t\t}\n> >> +\n> >> +\t\tdata->dev_ = videoDev;\n> >> \t\tmanager->addCamera(std::move(camera));\n> >>\n> >> \t\tLOG(IPU3, Info)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AC3560B1B\n\tfor ;\n\tFri, 25 Jan 2019 16:36:13 +0100 (CET)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:4499:2700:1060:1d4c:d6a:8e80])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A0526325;\n\tFri, 25 Jan 2019 16:36:12 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548430572;\n\tbh=SHWzlacdg8xsiVrplIs52KCxoYpRboDc4NJMr3rV1EA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tTKH8GbG9iPggOVRVNLF7fTf505LQCNZfMgByWo81UXZuXSGwEjY5S7FmnbyaJCO7\n\t3McG56bmZwpiAEj1Asjep3/QjuapZJP0nEM6iH0aRIlfhMn6LrdRI7fsPuYEu2xDtT\n\tvY/wOK/pVACfmJRclv1qyMVK+PSRH0BL7Jy26n90=","Date":"Fri, 25 Jan 2019 17:36:12 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190125153612.GG4320@pendragon.ideasonboard.com>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190124190630.GE4127@bigcity.dyn.berto.se>\n\t<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Fri, 25 Jan 2019 15:36:13 -0000"}},{"id":601,"web_url":"https://patchwork.libcamera.org/comment/601/","msgid":"<20190125155147.cyt3f53qbym5vtid@uno.localdomain>","date":"2019-01-25T15:51:47","subject":"Re: [libcamera-devel] [PATCH 2/2] 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\nOn Fri, Jan 25, 2019 at 05:31:09PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Jan 24, 2019 at 12:30:20PM +0100, Jacopo Mondi wrote:\n> > Create V4L2 devices for the CIO2 capture video nodes and associate them\n> > with the camera they are part of.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> > 1 file changed, 50 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 8cbc10a..9f5a40f 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> > @@ -29,9 +31,19 @@ public:\n> > \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> >\n> > private:\n> > +\tclass IPU3CameraData : public CameraData\n> > +\t{\n> > +\tpublic:\n> > +\t\tIPU3CameraData()\n> > +\t\t\t: dev_(nullptr) { }\n> > +\t\t~IPU3CameraData() { delete dev_; }\n> > +\t\tV4L2Device *dev_;\n>\n> You will soon need to add data to this, so I wouldn't inline the\n\nAs this is for a follow-up patch, let's do that once it is required\n\n> constructor and destructor. As the class may also get used by other\n> compilation units later on I'd define it outside of the\n> PipelineHandlerIPU3 class, to make it easier to later move it to a\n> header if necessary.\n\nAs this is for a follow-up patch, let's do that once it is required\nonce (if) we split the IPU3 handler in multiple source files.\n\n>\n> > +\t};\n> > +\n> > \tMediaDevice *cio2_;\n> > \tMediaDevice *imgu_;\n> >\n> > +\tV4L2Device *createVideoDevice(unsigned int id);\n> > \tvoid registerCameras(CameraManager *manager);\n> > };\n> >\n> > @@ -122,6 +134,24 @@ error_release_mdev:\n> > \treturn false;\n> > }\n> >\n> > +/* Create video devices for the CIO2 unit associated with a camera. */\n> > +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> > +{\n> > +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > +\tif (!cio2)\n> > +\t\treturn nullptr;\n> > +\n> > +\tV4L2Device *dev = new V4L2Device(*cio2);\n> > +\tif (dev->open()) {\n> > +\t\tdelete dev;\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\tdev->close();\n> > +\n> > +\treturn dev;\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> > @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> >\n> > \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> > \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> > +\n> > +\t\tsetCameraData(camera.get(),\n> > +\t\t\t std::move(utils::make_unique()));\n> > +\t\tIPU3CameraData *data =\n> > +\t\t\treinterpret_cast(cameraData(camera.get()));\n>\n> As I expect you'll need to call this quite often, how about creating a\n>\n> \tIPU3CameraData *cameraData(const Camera *);\n>\n> function in the PipelineHandlerIPU3 class ?\n>\n\nAs this is for a follow-up patch, let's do that once it is required\n\n> > +\n> > +\t\t/*\n> > +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> > +\t\t * registered. The 'camera' shared pointer goes out of scope\n> > +\t\t * and deletes the Camera it manages.\n> > +\t\t */\n> > +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> > +\t\tif (!videoDev) {\n> > +\t\t\tLOG(IPU3, Error)\n> > +\t\t\t\t<< \"Failed to register camera[\"\n> > +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > +\t\t\tcontinue;\n>\n> If this fails the camera will be deleted (as the sole reference will the\n> the shared pointer local to the loop), but the shared data will stay.\n> It's no big deal, it will be unused and be deleted when the pipeline\n> handler is deleted, but that still bothers me a bit. I wonder whether we\n> shouldn't call setCameraData() right before manager->addCamera()\n> instead.\n>\n\nI'll reply to this in the other comment, as this is not what has been\nmerged.\n\nThanks\n j\n\n> > +\t\t}\n> > +\n> > +\t\tdata->dev_ = videoDev;\n> > \t\tmanager->addCamera(std::move(camera));\n> >\n> > \t\tLOG(IPU3, Info)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["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 4535660B1D\n\tfor ;\n\tFri, 25 Jan 2019 16:51:34 +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 relay6-d.mail.gandi.net (Postfix) with ESMTPSA id BD085C0013;\n\tFri, 25 Jan 2019 15:51:33 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 25 Jan 2019 16:51:47 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190125155147.cyt3f53qbym5vtid@uno.localdomain>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190125153109.GF4320@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"r7a3guwtn2k4fyq4\"","Content-Disposition":"inline","In-Reply-To":"<20190125153109.GF4320@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Fri, 25 Jan 2019 15:51:34 -0000"}},{"id":602,"web_url":"https://patchwork.libcamera.org/comment/602/","msgid":"<20190125155458.gl6akrdz2bg6d4gw@uno.localdomain>","date":"2019-01-25T15:54:58","subject":"Re: [libcamera-devel] [PATCH 2/2] 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\nOn Fri, Jan 25, 2019 at 05:36:12PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Thu, Jan 24, 2019 at 08:33:00PM +0100, Jacopo Mondi wrote:\n> > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:\n> > > On 2019-01-24 12:30:20 +0100, Jacopo Mondi wrote:\n> > >> Create V4L2 devices for the CIO2 capture video nodes and associate them\n> > >> with the camera they are part of.\n> > >>\n> > >> Signed-off-by: Jacopo Mondi \n> > >> ---\n> > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> > >> 1 file changed, 50 insertions(+)\n> > >>\n> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> index 8cbc10a..9f5a40f 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> > >> @@ -29,9 +31,19 @@ public:\n> > >> \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> > >>\n> > >> private:\n> > >> +\tclass IPU3CameraData : public CameraData\n> > >> +\t{\n> > >> +\tpublic:\n> > >> +\t\tIPU3CameraData()\n> > >> +\t\t\t: dev_(nullptr) { }\n> > >> +\t\t~IPU3CameraData() { delete dev_; }\n> > >> +\t\tV4L2Device *dev_;\n> > >> +\t};\n> > >> +\n> > >> \tMediaDevice *cio2_;\n> > >> \tMediaDevice *imgu_;\n> > >>\n> > >> +\tV4L2Device *createVideoDevice(unsigned int id);\n> > >> \tvoid registerCameras(CameraManager *manager);\n> > >> };\n> > >>\n> > >> @@ -122,6 +134,24 @@ error_release_mdev:\n> > >> \treturn false;\n> > >> }\n> > >>\n> > >> +/* Create video devices for the CIO2 unit associated with a camera. */\n> > >> +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> > >> +{\n> > >> +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > >> +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > >> +\tif (!cio2)\n> > >> +\t\treturn nullptr;\n> > >> +\n> > >> +\tV4L2Device *dev = new V4L2Device(*cio2);\n> > >> +\tif (dev->open()) {\n> > >> +\t\tdelete dev;\n> > >> +\t\treturn nullptr;\n> > >> +\t}\n> > >> +\tdev->close();\n> > >> +\n> > >> +\treturn dev;\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> > >> @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> > >>\n> > >> \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> > >> \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> > >> +\n> > >> +\t\tsetCameraData(camera.get(),\n> > >> +\t\t\t std::move(utils::make_unique()));\n> > >> +\t\tIPU3CameraData *data =\n> > >> +\t\t\treinterpret_cast(cameraData(camera.get()));\n> > >\n> > > I'm not saying this is not needed, only that it looks a bit complex to\n> > > my feeble mind. Could you educate me why the following would not work?\n> > >\n> > > IPU3CameraData *data = new IPU3CameraData();\n> > > data->dev_ = videoDev;\n> > >\n> > > setCameraData(camera.get(), data);\n> >\n> > setCameraData wants a unique_ptr. On the reason why we're passing it\n> > by value (hence the std::move() ) instead than by reference and move()\n> > inside the function see the discussion on v1. Basically, it makes\n> > clear that after setCameraData() the local reference it's not\n> > valid anymore.\n> >\n> > That said, I could indeed have created a unique_ptr<> from an already\n> > existing reference, instead of using std::make_unique.\n> >\n> > What I have now looks like the following:\n> >\n> > \t\tV4L2Device *videoDev = createVideoDevice(id);\n> > \t\tif (!videoDev) {\n> > \t\t\tLOG(IPU3, Error)\n> > \t\t\t\t<< \"Failed to register camera[\"\n> > \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > \t\t\tcontinue;\n> > \t\t}\n> >\n> > \t\tIPU3CameraData *data = new IPU3CameraData(*videoDev);\n> > \t\tsetCameraData(camera.get(),\n> > \t\t\t std::move(std::unique_ptr(data)));\n> > \t\tmanager->addCamera(std::move(camera));\n> >\n> > Which is in my opinion nicer and equally safe. Thanks a lot for pointing\n> > this out. Is it any better?\n>\n> How about\n>\n> \t\tstd::unique_ptr data = utils::make_unique();\n> \t\tdata->dev_ = createVideoDevice(id);\n> \t\tif (!data->dev_) {\n> \t\t\tLOG(IPU3, Error)\n> \t\t\t\t<< \"Failed to register camera[\"\n> \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> \t\t\tcontinue;\n> \t\t}\n>\n> \t\tsetCameraData(camera.get(), std::move(data));\n> \t\tmanager->addCamera(std::move(camera));\n>\n\nOk, but why is it better? I see an allocation which in case of failure\nin creating the video device could be skipped.\n\nThanks\n j\n\n\n> > > Apart from this I think this commit looks good.\n> > >\n> > >> +\n> > >> +\t\t/*\n> > >> +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> > >> +\t\t * registered. The 'camera' shared pointer goes out of scope\n> > >> +\t\t * and deletes the Camera it manages.\n> > >> +\t\t */\n> > >> +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> > >> +\t\tif (!videoDev) {\n> > >> +\t\t\tLOG(IPU3, Error)\n> > >> +\t\t\t\t<< \"Failed to register camera[\"\n> > >> +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > >> +\t\t\tcontinue;\n> > >> +\t\t}\n> > >> +\n> > >> +\t\tdata->dev_ = videoDev;\n> > >> \t\tmanager->addCamera(std::move(camera));\n> > >>\n> > >> \t\tLOG(IPU3, Info)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 25FDE60B1D\n\tfor ;\n\tFri, 25 Jan 2019 16:54:46 +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 relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 593ABE0010;\n\tFri, 25 Jan 2019 15:54:45 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 25 Jan 2019 16:54:58 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190125155458.gl6akrdz2bg6d4gw@uno.localdomain>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190124190630.GE4127@bigcity.dyn.berto.se>\n\t<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>\n\t<20190125153612.GG4320@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"vstmwdbepsfufyhz\"","Content-Disposition":"inline","In-Reply-To":"<20190125153612.GG4320@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Fri, 25 Jan 2019 15:54:46 -0000"}},{"id":604,"web_url":"https://patchwork.libcamera.org/comment/604/","msgid":"<20190125160303.GJ4320@pendragon.ideasonboard.com>","date":"2019-01-25T16:03:03","subject":"Re: [libcamera-devel] [PATCH 2/2] 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 Fri, Jan 25, 2019 at 04:51:47PM +0100, Jacopo Mondi wrote:\n> On Fri, Jan 25, 2019 at 05:31:09PM +0200, Laurent Pinchart wrote:\n> > On Thu, Jan 24, 2019 at 12:30:20PM +0100, Jacopo Mondi wrote:\n> >> Create V4L2 devices for the CIO2 capture video nodes and associate them\n> >> with the camera they are part of.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> >> 1 file changed, 50 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 8cbc10a..9f5a40f 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> >> @@ -29,9 +31,19 @@ public:\n> >> \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> >>\n> >> private:\n> >> +\tclass IPU3CameraData : public CameraData\n> >> +\t{\n> >> +\tpublic:\n> >> +\t\tIPU3CameraData()\n> >> +\t\t\t: dev_(nullptr) { }\n> >> +\t\t~IPU3CameraData() { delete dev_; }\n> >> +\t\tV4L2Device *dev_;\n> >\n> > You will soon need to add data to this, so I wouldn't inline the\n> \n> As this is for a follow-up patch, let's do that once it is required\n> \n> > constructor and destructor. As the class may also get used by other\n> > compilation units later on I'd define it outside of the\n> > PipelineHandlerIPU3 class, to make it easier to later move it to a\n> > header if necessary.\n> \n> As this is for a follow-up patch, let's do that once it is required\n> once (if) we split the IPU3 handler in multiple source files.\n\nI'm OK with that.\n\n> >> +\t};\n> >> +\n> >> \tMediaDevice *cio2_;\n> >> \tMediaDevice *imgu_;\n> >>\n> >> +\tV4L2Device *createVideoDevice(unsigned int id);\n> >> \tvoid registerCameras(CameraManager *manager);\n> >> };\n> >>\n> >> @@ -122,6 +134,24 @@ error_release_mdev:\n> >> \treturn false;\n> >> }\n> >>\n> >> +/* Create video devices for the CIO2 unit associated with a camera. */\n> >> +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> >> +{\n> >> +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> >> +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> >> +\tif (!cio2)\n> >> +\t\treturn nullptr;\n> >> +\n> >> +\tV4L2Device *dev = new V4L2Device(*cio2);\n> >> +\tif (dev->open()) {\n> >> +\t\tdelete dev;\n> >> +\t\treturn nullptr;\n> >> +\t}\n> >> +\tdev->close();\n> >> +\n> >> +\treturn dev;\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> >> @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> >>\n> >> \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> >> \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> >> +\n> >> +\t\tsetCameraData(camera.get(),\n> >> +\t\t\t std::move(utils::make_unique()));\n> >> +\t\tIPU3CameraData *data =\n> >> +\t\t\treinterpret_cast(cameraData(camera.get()));\n> >\n> > As I expect you'll need to call this quite often, how about creating a\n> >\n> > \tIPU3CameraData *cameraData(const Camera *);\n> >\n> > function in the PipelineHandlerIPU3 class ?\n> >\n> \n> As this is for a follow-up patch, let's do that once it is required\n\nThis however I would fix already, to make sure the IPU3 pipeline handler\nfollows all the best practices and can be used as an example.\n\n> >> +\n> >> +\t\t/*\n> >> +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> >> +\t\t * registered. The 'camera' shared pointer goes out of scope\n> >> +\t\t * and deletes the Camera it manages.\n> >> +\t\t */\n> >> +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> >> +\t\tif (!videoDev) {\n> >> +\t\t\tLOG(IPU3, Error)\n> >> +\t\t\t\t<< \"Failed to register camera[\"\n> >> +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> >> +\t\t\tcontinue;\n> >\n> > If this fails the camera will be deleted (as the sole reference will the\n> > the shared pointer local to the loop), but the shared data will stay.\n> > It's no big deal, it will be unused and be deleted when the pipeline\n> > handler is deleted, but that still bothers me a bit. I wonder whether we\n> > shouldn't call setCameraData() right before manager->addCamera()\n> > instead.\n> >\n> \n> I'll reply to this in the other comment, as this is not what has been\n> merged.\n> \n> >> +\t\t}\n> >> +\n> >> +\t\tdata->dev_ = videoDev;\n> >> \t\tmanager->addCamera(std::move(camera));\n> >>\n> >> \t\tLOG(IPU3, Info)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4619F60B1D\n\tfor ;\n\tFri, 25 Jan 2019 17:03:08 +0100 (CET)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:4499:2700:1060:1d4c:d6a:8e80])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CA722325;\n\tFri, 25 Jan 2019 17:03:03 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548432183;\n\tbh=jb7SH8Pabuduuinv6mS+5HxjLfQPO9qpUXUOnF9FQsA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uyf+YD9DaCdDRTvKOqwD/CFttRPEeFTME5qacKpy3rRl7c8J+nRBJzoyE5b022n6g\n\tXBKTktA2h/3psqO/J1Pdd3V+XMNEwzc5zPdgJ0HUSbgD3eEKA80khC/12NhECBrWQ6\n\tPaovOgyGhDQMnUJVrbaTxeJF5tbGj0vIFC2C7HoU=","Date":"Fri, 25 Jan 2019 18:03:03 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190125160303.GJ4320@pendragon.ideasonboard.com>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190125153109.GF4320@pendragon.ideasonboard.com>\n\t<20190125155147.cyt3f53qbym5vtid@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190125155147.cyt3f53qbym5vtid@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Fri, 25 Jan 2019 16:03:08 -0000"}},{"id":605,"web_url":"https://patchwork.libcamera.org/comment/605/","msgid":"<20190125160805.GK4320@pendragon.ideasonboard.com>","date":"2019-01-25T16:08:05","subject":"Re: [libcamera-devel] [PATCH 2/2] 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 Fri, Jan 25, 2019 at 04:54:58PM +0100, Jacopo Mondi wrote:\n> On Fri, Jan 25, 2019 at 05:36:12PM +0200, Laurent Pinchart wrote:\n> > On Thu, Jan 24, 2019 at 08:33:00PM +0100, Jacopo Mondi wrote:\n> >> On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:\n> >>> On 2019-01-24 12:30:20 +0100, Jacopo Mondi wrote:\n> >>>> Create V4L2 devices for the CIO2 capture video nodes and associate them\n> >>>> with the camera they are part of.\n> >>>>\n> >>>> Signed-off-by: Jacopo Mondi \n> >>>> ---\n> >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> >>>> 1 file changed, 50 insertions(+)\n> >>>>\n> >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> index 8cbc10a..9f5a40f 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> >>>> @@ -29,9 +31,19 @@ public:\n> >>>> \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> >>>>\n> >>>> private:\n> >>>> +\tclass IPU3CameraData : public CameraData\n> >>>> +\t{\n> >>>> +\tpublic:\n> >>>> +\t\tIPU3CameraData()\n> >>>> +\t\t\t: dev_(nullptr) { }\n> >>>> +\t\t~IPU3CameraData() { delete dev_; }\n> >>>> +\t\tV4L2Device *dev_;\n> >>>> +\t};\n> >>>> +\n> >>>> \tMediaDevice *cio2_;\n> >>>> \tMediaDevice *imgu_;\n> >>>>\n> >>>> +\tV4L2Device *createVideoDevice(unsigned int id);\n> >>>> \tvoid registerCameras(CameraManager *manager);\n> >>>> };\n> >>>>\n> >>>> @@ -122,6 +134,24 @@ error_release_mdev:\n> >>>> \treturn false;\n> >>>> }\n> >>>>\n> >>>> +/* Create video devices for the CIO2 unit associated with a camera. */\n> >>>> +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> >>>> +{\n> >>>> +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> >>>> +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> >>>> +\tif (!cio2)\n> >>>> +\t\treturn nullptr;\n> >>>> +\n> >>>> +\tV4L2Device *dev = new V4L2Device(*cio2);\n> >>>> +\tif (dev->open()) {\n> >>>> +\t\tdelete dev;\n> >>>> +\t\treturn nullptr;\n> >>>> +\t}\n> >>>> +\tdev->close();\n> >>>> +\n> >>>> +\treturn dev;\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> >>>> @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> >>>>\n> >>>> \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> >>>> \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> >>>> +\n> >>>> +\t\tsetCameraData(camera.get(),\n> >>>> +\t\t\t std::move(utils::make_unique()));\n> >>>> +\t\tIPU3CameraData *data =\n> >>>> +\t\t\treinterpret_cast(cameraData(camera.get()));\n> >>>\n> >>> I'm not saying this is not needed, only that it looks a bit complex to\n> >>> my feeble mind. Could you educate me why the following would not work?\n> >>>\n> >>> IPU3CameraData *data = new IPU3CameraData();\n> >>> data->dev_ = videoDev;\n> >>>\n> >>> setCameraData(camera.get(), data);\n> >>\n> >> setCameraData wants a unique_ptr. On the reason why we're passing it\n> >> by value (hence the std::move() ) instead than by reference and move()\n> >> inside the function see the discussion on v1. Basically, it makes\n> >> clear that after setCameraData() the local reference it's not\n> >> valid anymore.\n> >>\n> >> That said, I could indeed have created a unique_ptr<> from an already\n> >> existing reference, instead of using std::make_unique.\n> >>\n> >> What I have now looks like the following:\n> >>\n> >> \t\tV4L2Device *videoDev = createVideoDevice(id);\n> >> \t\tif (!videoDev) {\n> >> \t\t\tLOG(IPU3, Error)\n> >> \t\t\t\t<< \"Failed to register camera[\"\n> >> \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> >> \t\t\tcontinue;\n> >> \t\t}\n> >>\n> >> \t\tIPU3CameraData *data = new IPU3CameraData(*videoDev);\n> >> \t\tsetCameraData(camera.get(),\n> >> \t\t\t std::move(std::unique_ptr(data)));\n> >> \t\tmanager->addCamera(std::move(camera));\n> >>\n> >> Which is in my opinion nicer and equally safe. Thanks a lot for pointing\n> >> this out. Is it any better?\n> >\n> > How about\n> >\n> > \t\tstd::unique_ptr data = utils::make_unique();\n> > \t\tdata->dev_ = createVideoDevice(id);\n> > \t\tif (!data->dev_) {\n> > \t\t\tLOG(IPU3, Error)\n> > \t\t\t\t<< \"Failed to register camera[\"\n> > \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > \t\t\tcontinue;\n> > \t\t}\n> >\n> > \t\tsetCameraData(camera.get(), std::move(data));\n> > \t\tmanager->addCamera(std::move(camera));\n> >\n> \n> Ok, but why is it better? I see an allocation which in case of failure\n> in creating the video device could be skipped.\n\nFirst we allocate data as a unique pointer, so we know it will be either\ngiven to setCameraData() or deleted. No risk of leak. Then we create the\nvideo device and store it directly in the camera data structure, so we\nknow it will be deleted with the camera. No risk of leak. If additional\ninitialization was needed, it would follow at this point. Then we call\nsetCameraData() at the end, when we don't need the data anymore, which\navoids calling cameraData() in here to get a borrowed reference to\nsomething we just gave away (as in your original version). Finally we\nregister the camera. The setCameraData() and addCamera() calls could\neven be moved to a PipelineHandler::addCamera(std::unique_ptr\ncamera, std::unique_ptr data) function that would do both,\nsimplifying the API for pipeline handlers.\n\n> >>> Apart from this I think this commit looks good.\n> >>>\n> >>>> +\n> >>>> +\t\t/*\n> >>>> +\t\t * If V4L2 device creation fails, the Camera instance won't be\n> >>>> +\t\t * registered. The 'camera' shared pointer goes out of scope\n> >>>> +\t\t * and deletes the Camera it manages.\n> >>>> +\t\t */\n> >>>> +\t\tV4L2Device *videoDev = createVideoDevice(id);\n> >>>> +\t\tif (!videoDev) {\n> >>>> +\t\t\tLOG(IPU3, Error)\n> >>>> +\t\t\t\t<< \"Failed to register camera[\"\n> >>>> +\t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> >>>> +\t\t\tcontinue;\n> >>>> +\t\t}\n> >>>> +\n> >>>> +\t\tdata->dev_ = videoDev;\n> >>>> \t\tmanager->addCamera(std::move(camera));\n> >>>>\n> >>>> \t\tLOG(IPU3, Info)","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 80B3D60C65\n\tfor ;\n\tFri, 25 Jan 2019 17:08:08 +0100 (CET)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:4499:2700:1060:1d4c:d6a:8e80])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DBE6F325;\n\tFri, 25 Jan 2019 17:08:05 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548432486;\n\tbh=Kjhmxl1E6KHw36fX/J2HR3DbHCz3lVri4LRxxU3dg1o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=abSSM/8CGex6XnpOzCX8JC4qQpVa6sbHOgPYAWF1RnuZGZx/z4No2neVqaJ0BGBKT\n\t3S2WrhmQT/2Tzn8xYAinJX9PpKo6ikYmJADnuRgQ1T0Y2aTVhxLLgDdBAFp+yimhEP\n\tvZeQE95PjuBndrx0Jl3d16oeVc4gp3JCCMVVzNqg=","Date":"Fri, 25 Jan 2019 18:08:05 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190125160805.GK4320@pendragon.ideasonboard.com>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190124190630.GE4127@bigcity.dyn.berto.se>\n\t<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>\n\t<20190125153612.GG4320@pendragon.ideasonboard.com>\n\t<20190125155458.gl6akrdz2bg6d4gw@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190125155458.gl6akrdz2bg6d4gw@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Fri, 25 Jan 2019 16:08:08 -0000"}},{"id":607,"web_url":"https://patchwork.libcamera.org/comment/607/","msgid":"<20190125164953.5usv3dbmhpouttrc@uno.localdomain>","date":"2019-01-25T16:49:53","subject":"Re: [libcamera-devel] [PATCH 2/2] 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\nOn Fri, Jan 25, 2019 at 06:08:05PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Fri, Jan 25, 2019 at 04:54:58PM +0100, Jacopo Mondi wrote:\n> > On Fri, Jan 25, 2019 at 05:36:12PM +0200, Laurent Pinchart wrote:\n> > > On Thu, Jan 24, 2019 at 08:33:00PM +0100, Jacopo Mondi wrote:\n> > >> On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:\n> > >>> On 2019-01-24 12:30:20 +0100, Jacopo Mondi wrote:\n> > >>>> Create V4L2 devices for the CIO2 capture video nodes and associate them\n> > >>>> with the camera they are part of.\n> > >>>>\n> > >>>> Signed-off-by: Jacopo Mondi \n> > >>>> ---\n> > >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++\n> > >>>> 1 file changed, 50 insertions(+)\n> > >>>>\n> > >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>>> index 8cbc10a..9f5a40f 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> > >>>> @@ -29,9 +31,19 @@ public:\n> > >>>> \tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> > >>>>\n> > >>>> private:\n> > >>>> +\tclass IPU3CameraData : public CameraData\n> > >>>> +\t{\n> > >>>> +\tpublic:\n> > >>>> +\t\tIPU3CameraData()\n> > >>>> +\t\t\t: dev_(nullptr) { }\n> > >>>> +\t\t~IPU3CameraData() { delete dev_; }\n> > >>>> +\t\tV4L2Device *dev_;\n> > >>>> +\t};\n> > >>>> +\n> > >>>> \tMediaDevice *cio2_;\n> > >>>> \tMediaDevice *imgu_;\n> > >>>>\n> > >>>> +\tV4L2Device *createVideoDevice(unsigned int id);\n> > >>>> \tvoid registerCameras(CameraManager *manager);\n> > >>>> };\n> > >>>>\n> > >>>> @@ -122,6 +134,24 @@ error_release_mdev:\n> > >>>> \treturn false;\n> > >>>> }\n> > >>>>\n> > >>>> +/* Create video devices for the CIO2 unit associated with a camera. */\n> > >>>> +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)\n> > >>>> +{\n> > >>>> +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > >>>> +\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > >>>> +\tif (!cio2)\n> > >>>> +\t\treturn nullptr;\n> > >>>> +\n> > >>>> +\tV4L2Device *dev = new V4L2Device(*cio2);\n> > >>>> +\tif (dev->open()) {\n> > >>>> +\t\tdelete dev;\n> > >>>> +\t\treturn nullptr;\n> > >>>> +\t}\n> > >>>> +\tdev->close();\n> > >>>> +\n> > >>>> +\treturn dev;\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> > >>>> @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> > >>>>\n> > >>>> \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> > >>>> \t\tstd::shared_ptr camera = Camera::create(cameraName);\n> > >>>> +\n> > >>>> +\t\tsetCameraData(camera.get(),\n> > >>>> +\t\t\t std::move(utils::make_unique()));\n> > >>>> +\t\tIPU3CameraData *data =\n> > >>>> +\t\t\treinterpret_cast(cameraData(camera.get()));\n> > >>>\n> > >>> I'm not saying this is not needed, only that it looks a bit complex to\n> > >>> my feeble mind. Could you educate me why the following would not work?\n> > >>>\n> > >>> IPU3CameraData *data = new IPU3CameraData();\n> > >>> data->dev_ = videoDev;\n> > >>>\n> > >>> setCameraData(camera.get(), data);\n> > >>\n> > >> setCameraData wants a unique_ptr. On the reason why we're passing it\n> > >> by value (hence the std::move() ) instead than by reference and move()\n> > >> inside the function see the discussion on v1. Basically, it makes\n> > >> clear that after setCameraData() the local reference it's not\n> > >> valid anymore.\n> > >>\n> > >> That said, I could indeed have created a unique_ptr<> from an already\n> > >> existing reference, instead of using std::make_unique.\n> > >>\n> > >> What I have now looks like the following:\n> > >>\n> > >> \t\tV4L2Device *videoDev = createVideoDevice(id);\n> > >> \t\tif (!videoDev) {\n> > >> \t\t\tLOG(IPU3, Error)\n> > >> \t\t\t\t<< \"Failed to register camera[\"\n> > >> \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > >> \t\t\tcontinue;\n> > >> \t\t}\n> > >>\n> > >> \t\tIPU3CameraData *data = new IPU3CameraData(*videoDev);\n> > >> \t\tsetCameraData(camera.get(),\n> > >> \t\t\t std::move(std::unique_ptr(data)));\n> > >> \t\tmanager->addCamera(std::move(camera));\n> > >>\n> > >> Which is in my opinion nicer and equally safe. Thanks a lot for pointing\n> > >> this out. Is it any better?\n> > >\n> > > How about\n> > >\n> > > \t\tstd::unique_ptr data = utils::make_unique();\n> > > \t\tdata->dev_ = createVideoDevice(id);\n> > > \t\tif (!data->dev_) {\n> > > \t\t\tLOG(IPU3, Error)\n> > > \t\t\t\t<< \"Failed to register camera[\"\n> > > \t\t\t\t<< numCameras << \"] \\\"\" << cameraName << \"\\\"\";\n> > > \t\t\tcontinue;\n> > > \t\t}\n> > >\n> > > \t\tsetCameraData(camera.get(), std::move(data));\n> > > \t\tmanager->addCamera(std::move(camera));\n> > >\n> >\n> > Ok, but why is it better? I see an allocation which in case of failure\n> > in creating the video device could be skipped.\n>\n> First we allocate data as a unique pointer, so we know it will be either\n> given to setCameraData() or deleted. No risk of leak. Then we create the\n> video device and store it directly in the camera data structure, so we\n> know it will be deleted with the camera. No risk of leak. If additional\n> initialization was needed, it would follow at this point. Then we call\n> setCameraData() at the end, when we don't need the data anymore, which\n> avoids calling cameraData() in here to get a borrowed reference to\n> something we just gave away (as in your original version). Finally we\n> register the camera. The setCameraData() and addCamera() calls could\n> even be moved to a PipelineHandler::addCamera(std::unique_ptr\n> camera, std::unique_ptr data) function that would do both,\n> simplifying the API for pipeline handlers.\n>\n\nok\n\nThanks\n j","headers":{"Return-Path":"","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7EF9760C65\n\tfor ;\n\tFri, 25 Jan 2019 17:49:40 +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 relay1-d.mail.gandi.net (Postfix) with ESMTPSA id BCE58240008;\n\tFri, 25 Jan 2019 16:49:39 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 25 Jan 2019 17:49:53 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190125164953.5usv3dbmhpouttrc@uno.localdomain>","References":"<20190124113020.7203-1-jacopo@jmondi.org>\n\t<20190124113020.7203-3-jacopo@jmondi.org>\n\t<20190124190630.GE4127@bigcity.dyn.berto.se>\n\t<20190124193300.mjtj4hbo2jtbblgd@uno.localdomain>\n\t<20190125153612.GG4320@pendragon.ideasonboard.com>\n\t<20190125155458.gl6akrdz2bg6d4gw@uno.localdomain>\n\t<20190125160805.GK4320@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"r5lt5hrh7o4riv7o\"","Content-Disposition":"inline","In-Reply-To":"<20190125160805.GK4320@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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":"Fri, 25 Jan 2019 16:49:40 -0000"}}]