[{"id":1006,"web_url":"https://patchwork.libcamera.org/comment/1006/","msgid":"<20190302214129.GH4682@pendragon.ideasonboard.com>","date":"2019-03-02T21:41:29","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and\n\tlink ImgU devices","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, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote:\n> Create video devices and subdevices associated with an ImgU unit, and\n> link the entities in the media graph to prepare the device for capture\n> operations at stream configuration time.\n> \n> As we support a single stream at the moment, always select imgu0.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++--\n> 1 file changed, 207 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 4f1ab72debf8..9fa59c1bc97e 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -24,6 +24,28 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPU3)\n> \n> +struct ImguDevice {\n\ns/ImguDevice/ImgUDevice/ ?\n\n> +\tImguDevice()\n> +\t\t: imgu(nullptr), input(nullptr), output(nullptr),\n> +\t\t viewfinder(nullptr), stat(nullptr) {}\n\n\t{\n\t}\n\n> +\n> +\t~ImguDevice()\n> +\t{\n> +\t\tdelete imgu;\n> +\t\tdelete input;\n> +\t\tdelete output;\n> +\t\tdelete viewfinder;\n> +\t\tdelete stat;\n> +\t}\n\nUnlike for the CIO2Device, this could be made a class, as I expect it\nwill have more code associated with it, and the ImgU instances will be\nshared between the multiple camera instances. The linkImgu function\nwould be a good candidate.\n\n> +\n> +\tV4L2Subdevice *imgu;\n> +\tV4L2Device *input;\n> +\tV4L2Device *output;\n> +\tV4L2Device *viewfinder;\n> +\tV4L2Device *stat;\n> +\t/* TODO: add param video device for 3A tuning */\n\n\\todo for consistency, even if this isn't a doxygen comment ?\n\n> +};\n> +\n> struct Cio2Device {\n> \tCio2Device()\n> \t\t: output(nullptr), csi2(nullptr), sensor(nullptr) {}\n> @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData\n> {\n> public:\n> \tCio2Device cio2;\n> +\tImguDevice *imgu;\n> \n> \tStream stream_;\n> };\n> @@ -71,6 +94,10 @@ public:\n> \tbool match(DeviceEnumerator *enumerator);\n> \n> private:\n> +\tstatic constexpr unsigned int IMGU_PAD_INPUT = 0;\n> +\tstatic constexpr unsigned int IMGU_PAD_OUTPUT = 2;\n> +\tstatic constexpr unsigned int IMGU_PAD_VF = 3;\n> +\tstatic constexpr unsigned int IMGU_PAD_STAT = 4;\n> \tstatic constexpr unsigned int IPU3_BUF_NUM = 4;\n\nI'd move that to the ImgUDevice struct/class. You can then remove the\nIMGU_ prefix.\n\n> \n> \tIPU3CameraData *cameraData(const Camera *camera)\n> @@ -79,9 +106,17 @@ private:\n> \t\t\tPipelineHandler::cameraData(camera));\n> \t}\n> \n> +\tint linkImgu(ImguDevice *imgu);\n> +\n> +\tV4L2Device *openDevice(MediaDevice *media, std::string &name);\n> +\tV4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name);\n> +\tint initImgu(ImguDevice *imgu);\n> \tint initCio2(unsigned int index, Cio2Device *cio2);\n> \tvoid registerCameras();\n> \n> +\tImguDevice imgu0_;\n> +\tImguDevice imgu1_;\n\nMaybe an array with two entries ?\n\n> +\n> \tstd::shared_ptr cio2MediaDev_;\n> \tstd::shared_ptr imguMediaDev_;\n> };\n> @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> \tV4L2DeviceFormat devFormat = {};\n> \tint ret;\n> \n> +\t/*\n> +\t * TODO: dynamically assign ImgU devices; as of now, with a single\n> +\t * stream supported, always use 'imgu0'.\n\n\t/**\n\t * \\todo\n\n?\n\n> +\t */\n> +\tdata->imgu = &imgu0_;\n\nHow about moving this to camera creation time, and assigned imgu0 to the\nfirst sensor and imgu1 to the second one ?\n\n> +\tret = linkImgu(data->imgu);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \t/*\n> \t * FIXME: as of now, the format gets applied to the sensor and is\n> \t * propagated along the pipeline. It should instead be applied on the\n> @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> \tif (cio2MediaDev_->open())\n> \t\tgoto error_release_mdev;\n> \n> +\tif (imguMediaDev_->open())\n> +\t\tgoto error_close_mdev;\n> +\n> \tif (cio2MediaDev_->disableLinks())\n> -\t\tgoto error_close_cio2;\n> +\t\tgoto error_close_mdev;\n> +\n> +\tif (initImgu(&imgu0_))\n> +\t\tgoto error_close_mdev;\n> +\n> +\tif (initImgu(&imgu1_))\n> +\t\tgoto error_close_mdev;\n> +\n> \n> \tregisterCameras();\n> \n> \tcio2MediaDev_->close();\n> +\timguMediaDev_->close();\n> \n> \treturn true;\n> \n> -error_close_cio2:\n> +error_close_mdev:\n> \tcio2MediaDev_->close();\n> +\timguMediaDev_->close();\n\nError handling will be simplified when you'll rebase. I'd go as far as\ncalling close() in the PipelineHandlerIPU3 destructor and remove the\nerror path here.\n\n> \n> error_release_mdev:\n> \tcio2MediaDev_->release();\n> @@ -353,6 +409,153 @@ error_release_mdev:\n> \treturn false;\n> }\n> \n> +/* Link entities in the ImgU unit to prepare for capture operations. */\n> +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice)\n> +{\n> +\tMediaLink *link;\n> +\tint ret;\n> +\n> +\tunsigned int index = imguDevice == &imgu0_ ? 0 : 1;\n> +\tstd::string imguName = \"ipu3-imgu \" + std::to_string(index);\n> +\tstd::string inputName = imguName + \" input\";\n> +\tstd::string outputName = imguName + \" output\";\n> +\tstd::string viewfinderName = imguName + \" viewfinder\";\n> +\tstd::string statName = imguName + \" 3a stat\";\n> +\n> +\tret = imguMediaDev_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = imguMediaDev_->disableLinks();\n\nThis isn't a good idea, as you will interfere with the other ImgU. You\nshould enable/disable links selectively based on your needs.\n\n> +\tif (ret) {\n> +\t\timguMediaDev_->close();\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Link entities to configure the IMGU unit for capture. */\n> +\tlink = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT);\n> +\tif (!link) {\n> +\t\tLOG(IPU3, Error)\n> +\t\t\t<< \"Failed to get link '\" << inputName << \"':0 -> '\"\n> +\t\t\t<< imguName << \"':0\";\n\nIdeally you should use the IMGU_PAD_* constants instead of hardcoding\nthem in error messages.\n\n> +\t\tret = -ENODEV;\n> +\t\tgoto error_close_mediadev;\n> +\t}\n> +\tlink->setEnabled(true);\n> +\n> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0);\n> +\tif (!link) {\n> +\t\tLOG(IPU3, Error)\n> +\t\t\t<< \"Failed to get link '\" << imguName << \"':2 -> '\"\n> +\t\t\t<< outputName << \"':0\";\n> +\t\tret = -ENODEV;\n> +\t\tgoto error_close_mediadev;\n> +\t}\n> +\tlink->setEnabled(true);\n> +\n> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0);\n> +\tif (!link) {\n> +\t\tLOG(IPU3, Error)\n> +\t\t\t<< \"Failed to get link '\" << imguName << \"':3 -> '\"\n> +\t\t\t<< viewfinderName << \"':0\";\n> +\t\tret = -ENODEV;\n> +\t\tgoto error_close_mediadev;\n> +\t}\n> +\tlink->setEnabled(true);\n\nWe have a single stream, why do you enable both output and viewfinder ?\n\n> +\n> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0);\n> +\tif (!link) {\n> +\t\tLOG(IPU3, Error)\n> +\t\t\t<< \"Failed to get link '\" << imguName << \"':4 -> '\"\n> +\t\t\t<< statName << \"':0\";\n> +\t\tret = -ENODEV;\n> +\t\tgoto error_close_mediadev;\n> +\t}\n> +\tlink->setEnabled(true);\n\nSame here, we don't make use of stats yes, there's no need to enable\nthis link.\n\n> +\n> +\timguMediaDev_->close();\n> +\n> +\treturn 0;\n> +\n> +error_close_mediadev:\n> +\timguMediaDev_->close();\n\nWe really really need to think about how to handle open/close and write\ndown a set of rules. Please make sure this is captured in a \\todo.\n\n> +\n> +\treturn ret;\n> +\n> +}\n> +\n> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media,\n> +\t\t\t\t\t std::string &name)\n\nconst std::string &\n\nSame below.\n\n> +{\n> +\tV4L2Device *dev;\n\nYou can move this line down to declare and initialize the variable at\nthe same time.\n\n> +\n> +\tMediaEntity *entity = media->getEntityByName(name);\n> +\tif (!entity) {\n> +\t\tLOG(IPU3, Error)\n> +\t\t\t<< \"Failed to get entity '\" << name << \"'\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tdev = new V4L2Device(entity);\n> +\tif (dev->open())\n> +\t\treturn nullptr;\n\nYou'll leak dev, a delete is needed.\n\n> +\n> +\treturn dev;\n> +}\n> +\n> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media,\n> +\t\t\t\t\t\t std::string &name)\n> +{\n> +\tV4L2Subdevice *dev;\n> +\n> +\tMediaEntity *entity = media->getEntityByName(name);\n> +\tif (!entity) {\n> +\t\tLOG(IPU3, Error)\n> +\t\t\t<< \"Failed to get entity '\" << name << \"'\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tdev = new V4L2Subdevice(entity);\n> +\tif (dev->open())\n\nLeak.\n\n> +\t\treturn nullptr;\n> +\n> +\treturn dev;\n> +}\n\nThese two functions may be candidates for future generic helpers. Could\nyou document them and add a \\todo to mention this ?\n\n> +\n> +/* Create video devices and subdevices for the ImgU instance. */\n> +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu)\n> +{\n> +\tunsigned int index = imgu == &imgu0_ ? 0 : 1;\n\nA bit ugly, how about adding an index field to the ImguDevice structure\n?\n\n> +\tstd::string imguName = \"ipu3-imgu \" + std::to_string(index);\n> +\tstd::string devName;\n> +\n> +\timgu->imgu = openSubdevice(imguMediaDev_.get(), imguName);\n> +\tif (!imgu->imgu)\n> +\t\treturn -ENODEV;\n> +\n> +\tdevName = imguName + \" input\";\n> +\timgu->input = openDevice(imguMediaDev_.get(), devName);\n> +\tif (!imgu->input)\n> +\t\treturn -ENODEV;\n> +\n> +\tdevName = imguName + \" output\";\n> +\timgu->output = openDevice(imguMediaDev_.get(), devName);\n> +\tif (!imgu->output)\n> +\t\treturn -ENODEV;\n> +\n> +\tdevName = imguName + \" viewfinder\";\n> +\timgu->viewfinder = openDevice(imguMediaDev_.get(), devName);\n> +\tif (!imgu->viewfinder)\n> +\t\treturn -ENODEV;\n> +\n> +\tdevName = imguName + \" 3a stat\";\n> +\timgu->stat = openDevice(imguMediaDev_.get(), devName);\n\nYou could drop devName and call\n\n\timgu->state = openDevice(imguMediaDev_.get(), imguName + \" 3a stat\");\n\nUp to you.\n\n> +\tif (!imgu->stat)\n> +\t\treturn -ENODEV;\n> +\n> +\treturn 0;\n> +}\n> +\n> int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)\n> {\n> \tint ret;\n> @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)\n> \t\treturn ret;\n> \n> \tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(index);\n> -\tentity = cio2MediaDev_->getEntityByName(cio2Name);\n> -\tif (!entity) {\n> -\t\tLOG(IPU3, Error)\n> -\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\tcio2->output = new V4L2Device(entity);\n> -\tret = cio2->output->open();\n> -\tif (ret)\n> +\tcio2->output = openDevice(cio2MediaDev_.get(), cio2Name);\n> +\tif (!cio2->output)\n> \t\treturn ret;\n> \n> \treturn 0;","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 D86B6610BF\n\tfor ;\n\tSat, 2 Mar 2019 22:41:35 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3D8F254E;\n\tSat, 2 Mar 2019 22:41:35 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551562895;\n\tbh=aoq1VQZU4lSm36s29HPPXFLlhYf0qwhYRZjJ+iWiQMQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z6FXC7ZET+fsGJHUxdUdPGe9a+12C6Anz/O6XM997Lpt1PgGCmhFgwyvLkGFqOsPP\n\t3D6yA+KpZpl6KyO7YVv9ryfOgThGWK9jaGxSUrVpgNi1N7nFtyD9MitaVSLTxvGqAa\n\tVnfIQO23xB0pD9d5GY4lcdIyzXVep10X1PEF9leo=","Date":"Sat, 2 Mar 2019 23:41:29 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190302214129.GH4682@pendragon.ideasonboard.com>","References":"<20190228200410.3022-1-jacopo@jmondi.org>\n\t<20190228200410.3022-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190228200410.3022-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and\n\tlink ImgU devices","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":"Sat, 02 Mar 2019 21:41:36 -0000"}},{"id":1021,"web_url":"https://patchwork.libcamera.org/comment/1021/","msgid":"<20190309205018.se6y2fjd7sgba54d@uno.localdomain>","date":"2019-03-09T20:50:18","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and\n\tlink ImgU devices","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Mar 02, 2019 at 11:41:29PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote:\n> > Create video devices and subdevices associated with an ImgU unit, and\n> > link the entities in the media graph to prepare the device for capture\n> > operations at stream configuration time.\n> >\n> > As we support a single stream at the moment, always select imgu0.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++--\n> > 1 file changed, 207 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 4f1ab72debf8..9fa59c1bc97e 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -24,6 +24,28 @@ namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(IPU3)\n> >\n> > +struct ImguDevice {\n>\n> s/ImguDevice/ImgUDevice/ ?\n\nI would like ImguDevice best, but I'll use ImgUDevice for consistency\nwith the kernel documentation.\n\n>\n> > +\tImguDevice()\n> > +\t\t: imgu(nullptr), input(nullptr), output(nullptr),\n> > +\t\t viewfinder(nullptr), stat(nullptr) {}\n>\n> \t{\n> \t}\n>\n> > +\n> > +\t~ImguDevice()\n> > +\t{\n> > +\t\tdelete imgu;\n> > +\t\tdelete input;\n> > +\t\tdelete output;\n> > +\t\tdelete viewfinder;\n> > +\t\tdelete stat;\n> > +\t}\n>\n> Unlike for the CIO2Device, this could be made a class, as I expect it\n> will have more code associated with it, and the ImgU instances will be\n> shared between the multiple camera instances. The linkImgu function\n> would be a good candidate.\n>\n\nI'll move that to this class. I will has well add a \"disableLinks()\"\nmethods as you suggested below.\n\n> > +\n> > +\tV4L2Subdevice *imgu;\n> > +\tV4L2Device *input;\n> > +\tV4L2Device *output;\n> > +\tV4L2Device *viewfinder;\n> > +\tV4L2Device *stat;\n> > +\t/* TODO: add param video device for 3A tuning */\n>\n> \\todo for consistency, even if this isn't a doxygen comment ?\n>\n\nWill change all of these.\n\n> > +};\n> > +\n> > struct Cio2Device {\n> > \tCio2Device()\n> > \t\t: output(nullptr), csi2(nullptr), sensor(nullptr) {}\n> > @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData\n> > {\n> > public:\n> > \tCio2Device cio2;\n> > +\tImguDevice *imgu;\n> >\n> > \tStream stream_;\n> > };\n> > @@ -71,6 +94,10 @@ public:\n> > \tbool match(DeviceEnumerator *enumerator);\n> >\n> > private:\n> > +\tstatic constexpr unsigned int IMGU_PAD_INPUT = 0;\n> > +\tstatic constexpr unsigned int IMGU_PAD_OUTPUT = 2;\n> > +\tstatic constexpr unsigned int IMGU_PAD_VF = 3;\n> > +\tstatic constexpr unsigned int IMGU_PAD_STAT = 4;\n> > \tstatic constexpr unsigned int IPU3_BUF_NUM = 4;\n>\n> I'd move that to the ImgUDevice struct/class. You can then remove the\n> IMGU_ prefix.\n>\n\nAgreed.\n\n> >\n> > \tIPU3CameraData *cameraData(const Camera *camera)\n> > @@ -79,9 +106,17 @@ private:\n> > \t\t\tPipelineHandler::cameraData(camera));\n> > \t}\n> >\n> > +\tint linkImgu(ImguDevice *imgu);\n> > +\n> > +\tV4L2Device *openDevice(MediaDevice *media, std::string &name);\n> > +\tV4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name);\n> > +\tint initImgu(ImguDevice *imgu);\n> > \tint initCio2(unsigned int index, Cio2Device *cio2);\n> > \tvoid registerCameras();\n> >\n> > +\tImguDevice imgu0_;\n> > +\tImguDevice imgu1_;\n>\n> Maybe an array with two entries ?\n>\n\nThey're two, and will stay two, but yes, I'll see how it looks.\n\n> > +\n> > \tstd::shared_ptr cio2MediaDev_;\n> > \tstd::shared_ptr imguMediaDev_;\n> > };\n> > @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > \tV4L2DeviceFormat devFormat = {};\n> > \tint ret;\n> >\n> > +\t/*\n> > +\t * TODO: dynamically assign ImgU devices; as of now, with a single\n> > +\t * stream supported, always use 'imgu0'.\n>\n> \t/**\n> \t * \\todo\n>\n> ?\n>\n> > +\t */\n> > +\tdata->imgu = &imgu0_;\n>\n> How about moving this to camera creation time, and assigned imgu0 to the\n> first sensor and imgu1 to the second one ?\n>\n\nI've put it here, as I assumed ImgU association with Cameras will\ndepend on the number of required streams. If you confirm my\nunderstanding I would keep it here instead of moving it back and\nforth.\n\n> > +\tret = linkImgu(data->imgu);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > \t/*\n> > \t * FIXME: as of now, the format gets applied to the sensor and is\n> > \t * propagated along the pipeline. It should instead be applied on the\n> > @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > \tif (cio2MediaDev_->open())\n> > \t\tgoto error_release_mdev;\n> >\n> > +\tif (imguMediaDev_->open())\n> > +\t\tgoto error_close_mdev;\n> > +\n> > \tif (cio2MediaDev_->disableLinks())\n> > -\t\tgoto error_close_cio2;\n> > +\t\tgoto error_close_mdev;\n> > +\n> > +\tif (initImgu(&imgu0_))\n> > +\t\tgoto error_close_mdev;\n> > +\n> > +\tif (initImgu(&imgu1_))\n> > +\t\tgoto error_close_mdev;\n> > +\n> >\n> > \tregisterCameras();\n> >\n> > \tcio2MediaDev_->close();\n> > +\timguMediaDev_->close();\n> >\n> > \treturn true;\n> >\n> > -error_close_cio2:\n> > +error_close_mdev:\n> > \tcio2MediaDev_->close();\n> > +\timguMediaDev_->close();\n>\n> Error handling will be simplified when you'll rebase. I'd go as far as\n> calling close() in the PipelineHandlerIPU3 destructor and remove the\n> error path here.\n>\n\nIt has been simplified by rebase, yes. I'll see how moving close() in\ndestructor looks like (I think it's good)\n\n> >\n> > error_release_mdev:\n> > \tcio2MediaDev_->release();\n> > @@ -353,6 +409,153 @@ error_release_mdev:\n> > \treturn false;\n> > }\n> >\n> > +/* Link entities in the ImgU unit to prepare for capture operations. */\n> > +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice)\n> > +{\n> > +\tMediaLink *link;\n> > +\tint ret;\n> > +\n> > +\tunsigned int index = imguDevice == &imgu0_ ? 0 : 1;\n> > +\tstd::string imguName = \"ipu3-imgu \" + std::to_string(index);\n> > +\tstd::string inputName = imguName + \" input\";\n> > +\tstd::string outputName = imguName + \" output\";\n> > +\tstd::string viewfinderName = imguName + \" viewfinder\";\n> > +\tstd::string statName = imguName + \" 3a stat\";\n> > +\n> > +\tret = imguMediaDev_->open();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = imguMediaDev_->disableLinks();\n>\n> This isn't a good idea, as you will interfere with the other ImgU. You\n> should enable/disable links selectively based on your needs.\n>\n\nAs I've said, I'll make a per-imgu instance link disabling method. I\nhope it is enough to disable all links in an imgu instance and we\ndon't have to go link by link...\n\n> > +\tif (ret) {\n> > +\t\timguMediaDev_->close();\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/* Link entities to configure the IMGU unit for capture. */\n> > +\tlink = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT);\n> > +\tif (!link) {\n> > +\t\tLOG(IPU3, Error)\n> > +\t\t\t<< \"Failed to get link '\" << inputName << \"':0 -> '\"\n> > +\t\t\t<< imguName << \"':0\";\n>\n> Ideally you should use the IMGU_PAD_* constants instead of hardcoding\n> them in error messages.\n>\n\nAh well, it's just more lines for a single printout, but ok.\n\n> > +\t\tret = -ENODEV;\n> > +\t\tgoto error_close_mediadev;\n> > +\t}\n> > +\tlink->setEnabled(true);\n> > +\n> > +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0);\n> > +\tif (!link) {\n> > +\t\tLOG(IPU3, Error)\n> > +\t\t\t<< \"Failed to get link '\" << imguName << \"':2 -> '\"\n> > +\t\t\t<< outputName << \"':0\";\n> > +\t\tret = -ENODEV;\n> > +\t\tgoto error_close_mediadev;\n> > +\t}\n> > +\tlink->setEnabled(true);\n> > +\n> > +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0);\n> > +\tif (!link) {\n> > +\t\tLOG(IPU3, Error)\n> > +\t\t\t<< \"Failed to get link '\" << imguName << \"':3 -> '\"\n> > +\t\t\t<< viewfinderName << \"':0\";\n> > +\t\tret = -ENODEV;\n> > +\t\tgoto error_close_mediadev;\n> > +\t}\n> > +\tlink->setEnabled(true);\n>\n> We have a single stream, why do you enable both output and viewfinder ?\n>\n\nFrom my testings, if I keep them disabled (== not linked) the system\nhangs even if I'm only capturing from the ouput video device. I will\ndo some more testing, as setting up those nodes, queuing and dequeuing\nbuffers there requires some not-nice code to keep track of the buffer\nindexes, as you've noticed.\n\n> > +\n> > +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0);\n> > +\tif (!link) {\n> > +\t\tLOG(IPU3, Error)\n> > +\t\t\t<< \"Failed to get link '\" << imguName << \"':4 -> '\"\n> > +\t\t\t<< statName << \"':0\";\n> > +\t\tret = -ENODEV;\n> > +\t\tgoto error_close_mediadev;\n> > +\t}\n> > +\tlink->setEnabled(true);\n>\n> Same here, we don't make use of stats yes, there's no need to enable\n> this link.\n>\n> > +\n> > +\timguMediaDev_->close();\n> > +\n> > +\treturn 0;\n> > +\n> > +error_close_mediadev:\n> > +\timguMediaDev_->close();\n>\n> We really really need to think about how to handle open/close and write\n> down a set of rules. Please make sure this is captured in a \\todo.\n>\n> > +\n> > +\treturn ret;\n> > +\n> > +}\n> > +\n> > +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media,\n> > +\t\t\t\t\t std::string &name)\n>\n> const std::string &\n>\n> Same below.\n>\n> > +{\n> > +\tV4L2Device *dev;\n>\n> You can move this line down to declare and initialize the variable at\n> the same time.\n>\n> > +\n> > +\tMediaEntity *entity = media->getEntityByName(name);\n> > +\tif (!entity) {\n> > +\t\tLOG(IPU3, Error)\n> > +\t\t\t<< \"Failed to get entity '\" << name << \"'\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tdev = new V4L2Device(entity);\n> > +\tif (dev->open())\n> > +\t\treturn nullptr;\n>\n> You'll leak dev, a delete is needed.\n>\n\nAh, right! will fix.\n\n> > +\n> > +\treturn dev;\n> > +}\n> > +\n> > +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media,\n> > +\t\t\t\t\t\t std::string &name)\n> > +{\n> > +\tV4L2Subdevice *dev;\n> > +\n> > +\tMediaEntity *entity = media->getEntityByName(name);\n> > +\tif (!entity) {\n> > +\t\tLOG(IPU3, Error)\n> > +\t\t\t<< \"Failed to get entity '\" << name << \"'\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tdev = new V4L2Subdevice(entity);\n> > +\tif (dev->open())\n>\n> Leak.\n>\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn dev;\n> > +}\n>\n> These two functions may be candidates for future generic helpers. Could\n> you document them and add a \\todo to mention this ?\n>\n\nAgreed!\n\n> > +\n> > +/* Create video devices and subdevices for the ImgU instance. */\n> > +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu)\n> > +{\n> > +\tunsigned int index = imgu == &imgu0_ ? 0 : 1;\n>\n> A bit ugly, how about adding an index field to the ImguDevice structure\n> ?\n\nYes. But when would you intialize such a field? at match time? In this\nfunction? I'll see how it'll look\n\n>\n> > +\tstd::string imguName = \"ipu3-imgu \" + std::to_string(index);\n> > +\tstd::string devName;\n> > +\n> > +\timgu->imgu = openSubdevice(imguMediaDev_.get(), imguName);\n> > +\tif (!imgu->imgu)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tdevName = imguName + \" input\";\n> > +\timgu->input = openDevice(imguMediaDev_.get(), devName);\n> > +\tif (!imgu->input)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tdevName = imguName + \" output\";\n> > +\timgu->output = openDevice(imguMediaDev_.get(), devName);\n> > +\tif (!imgu->output)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tdevName = imguName + \" viewfinder\";\n> > +\timgu->viewfinder = openDevice(imguMediaDev_.get(), devName);\n> > +\tif (!imgu->viewfinder)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tdevName = imguName + \" 3a stat\";\n> > +\timgu->stat = openDevice(imguMediaDev_.get(), devName);\n>\n> You could drop devName and call\n>\n> \timgu->state = openDevice(imguMediaDev_.get(), imguName + \" 3a stat\");\n>\n> Up to you.\n\nLess lines, so it's better\n\nThanks\n j\n\n>\n> > +\tif (!imgu->stat)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)\n> > {\n> > \tint ret;\n> > @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)\n> > \t\treturn ret;\n> >\n> > \tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(index);\n> > -\tentity = cio2MediaDev_->getEntityByName(cio2Name);\n> > -\tif (!entity) {\n> > -\t\tLOG(IPU3, Error)\n> > -\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > -\n> > -\tcio2->output = new V4L2Device(entity);\n> > -\tret = cio2->output->open();\n> > -\tif (ret)\n> > +\tcio2->output = openDevice(cio2MediaDev_.get(), cio2Name);\n> > +\tif (!cio2->output)\n> > \t\treturn ret;\n> >\n> > \treturn 0;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C86F600F9\n\tfor ;\n\tSat, 9 Mar 2019 21:49:44 +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 relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 0F8F61BF208;\n\tSat, 9 Mar 2019 20:49:43 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sat, 9 Mar 2019 21:50:18 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190309205018.se6y2fjd7sgba54d@uno.localdomain>","References":"<20190228200410.3022-1-jacopo@jmondi.org>\n\t<20190228200410.3022-4-jacopo@jmondi.org>\n\t<20190302214129.GH4682@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"6nrug27sht53rxyw\"","Content-Disposition":"inline","In-Reply-To":"<20190302214129.GH4682@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and\n\tlink ImgU devices","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":"Sat, 09 Mar 2019 20:49:44 -0000"}},{"id":1027,"web_url":"https://patchwork.libcamera.org/comment/1027/","msgid":"<20190310130833.GC4814@pendragon.ideasonboard.com>","date":"2019-03-10T13:08:33","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and\n\tlink ImgU devices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sat, Mar 09, 2019 at 09:50:18PM +0100, Jacopo Mondi wrote:\n> On Sat, Mar 02, 2019 at 11:41:29PM +0200, Laurent Pinchart wrote:\n> > On Thu, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote:\n> >> Create video devices and subdevices associated with an ImgU unit, and\n> >> link the entities in the media graph to prepare the device for capture\n> >> operations at stream configuration time.\n> >>\n> >> As we support a single stream at the moment, always select imgu0.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++--\n> >> 1 file changed, 207 insertions(+), 12 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 4f1ab72debf8..9fa59c1bc97e 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -24,6 +24,28 @@ namespace libcamera {\n> >>\n> >> LOG_DEFINE_CATEGORY(IPU3)\n> >>\n> >> +struct ImguDevice {\n> >\n> > s/ImguDevice/ImgUDevice/ ?\n> \n> I would like ImguDevice best, but I'll use ImgUDevice for consistency\n> with the kernel documentation.\n> \n> >> +\tImguDevice()\n> >> +\t\t: imgu(nullptr), input(nullptr), output(nullptr),\n> >> +\t\t viewfinder(nullptr), stat(nullptr) {}\n> >\n> > \t{\n> > \t}\n> >\n> >> +\n> >> +\t~ImguDevice()\n> >> +\t{\n> >> +\t\tdelete imgu;\n> >> +\t\tdelete input;\n> >> +\t\tdelete output;\n> >> +\t\tdelete viewfinder;\n> >> +\t\tdelete stat;\n> >> +\t}\n> >\n> > Unlike for the CIO2Device, this could be made a class, as I expect it\n> > will have more code associated with it, and the ImgU instances will be\n> > shared between the multiple camera instances. The linkImgu function\n> > would be a good candidate.\n> \n> I'll move that to this class. I will has well add a \"disableLinks()\"\n> methods as you suggested below.\n> \n> >> +\n> >> +\tV4L2Subdevice *imgu;\n> >> +\tV4L2Device *input;\n> >> +\tV4L2Device *output;\n> >> +\tV4L2Device *viewfinder;\n> >> +\tV4L2Device *stat;\n> >> +\t/* TODO: add param video device for 3A tuning */\n> >\n> > \\todo for consistency, even if this isn't a doxygen comment ?\n> \n> Will change all of these.\n> \n> >> +};\n> >> +\n> >> struct Cio2Device {\n> >> \tCio2Device()\n> >> \t\t: output(nullptr), csi2(nullptr), sensor(nullptr) {}\n> >> @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData\n> >> {\n> >> public:\n> >> \tCio2Device cio2;\n> >> +\tImguDevice *imgu;\n> >>\n> >> \tStream stream_;\n> >> };\n> >> @@ -71,6 +94,10 @@ public:\n> >> \tbool match(DeviceEnumerator *enumerator);\n> >>\n> >> private:\n> >> +\tstatic constexpr unsigned int IMGU_PAD_INPUT = 0;\n> >> +\tstatic constexpr unsigned int IMGU_PAD_OUTPUT = 2;\n> >> +\tstatic constexpr unsigned int IMGU_PAD_VF = 3;\n> >> +\tstatic constexpr unsigned int IMGU_PAD_STAT = 4;\n> >> \tstatic constexpr unsigned int IPU3_BUF_NUM = 4;\n> >\n> > I'd move that to the ImgUDevice struct/class. You can then remove the\n> > IMGU_ prefix.\n> \n> Agreed.\n> \n> >>\n> >> \tIPU3CameraData *cameraData(const Camera *camera)\n> >> @@ -79,9 +106,17 @@ private:\n> >> \t\t\tPipelineHandler::cameraData(camera));\n> >> \t}\n> >>\n> >> +\tint linkImgu(ImguDevice *imgu);\n> >> +\n> >> +\tV4L2Device *openDevice(MediaDevice *media, std::string &name);\n> >> +\tV4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name);\n> >> +\tint initImgu(ImguDevice *imgu);\n> >> \tint initCio2(unsigned int index, Cio2Device *cio2);\n> >> \tvoid registerCameras();\n> >>\n> >> +\tImguDevice imgu0_;\n> >> +\tImguDevice imgu1_;\n> >\n> > Maybe an array with two entries ?\n> \n> They're two, and will stay two, but yes, I'll see how it looks.\n> \n> >> +\n> >> \tstd::shared_ptr cio2MediaDev_;\n> >> \tstd::shared_ptr imguMediaDev_;\n> >> };\n> >> @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> >> \tV4L2DeviceFormat devFormat = {};\n> >> \tint ret;\n> >>\n> >> +\t/*\n> >> +\t * TODO: dynamically assign ImgU devices; as of now, with a single\n> >> +\t * stream supported, always use 'imgu0'.\n> >\n> > \t/**\n> > \t * \\todo\n> >\n> > ?\n> >\n> >> +\t */\n> >> +\tdata->imgu = &imgu0_;\n> >\n> > How about moving this to camera creation time, and assigned imgu0 to the\n> > first sensor and imgu1 to the second one ?\n> >\n> \n> I've put it here, as I assumed ImgU association with Cameras will\n> depend on the number of required streams. If you confirm my\n> understanding I would keep it here instead of moving it back and\n> forth.\n\nYour understanding is correct, but with this implementation imgu0 is\nused for both cameras, so we can only use one camera at a time. If we\nassign imgu0 to the first camera and imgu1 to the second one we can\nstart using both cameras, with up to two streams each, and later expand\nthat to more streams.\n\n> >> +\tret = linkImgu(data->imgu);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> \t/*\n> >> \t * FIXME: as of now, the format gets applied to the sensor and is\n> >> \t * propagated along the pipeline. It should instead be applied on the\n> >> @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> >> \tif (cio2MediaDev_->open())\n> >> \t\tgoto error_release_mdev;\n> >>\n> >> +\tif (imguMediaDev_->open())\n> >> +\t\tgoto error_close_mdev;\n> >> +\n> >> \tif (cio2MediaDev_->disableLinks())\n> >> -\t\tgoto error_close_cio2;\n> >> +\t\tgoto error_close_mdev;\n> >> +\n> >> +\tif (initImgu(&imgu0_))\n> >> +\t\tgoto error_close_mdev;\n> >> +\n> >> +\tif (initImgu(&imgu1_))\n> >> +\t\tgoto error_close_mdev;\n> >> +\n> >>\n> >> \tregisterCameras();\n> >>\n> >> \tcio2MediaDev_->close();\n> >> +\timguMediaDev_->close();\n> >>\n> >> \treturn true;\n> >>\n> >> -error_close_cio2:\n> >> +error_close_mdev:\n> >> \tcio2MediaDev_->close();\n> >> +\timguMediaDev_->close();\n> >\n> > Error handling will be simplified when you'll rebase. I'd go as far as\n> > calling close() in the PipelineHandlerIPU3 destructor and remove the\n> > error path here.\n> \n> It has been simplified by rebase, yes. I'll see how moving close() in\n> destructor looks like (I think it's good)\n> \n> >>\n> >> error_release_mdev:\n> >> \tcio2MediaDev_->release();\n> >> @@ -353,6 +409,153 @@ error_release_mdev:\n> >> \treturn false;\n> >> }\n> >>\n> >> +/* Link entities in the ImgU unit to prepare for capture operations. */\n> >> +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice)\n> >> +{\n> >> +\tMediaLink *link;\n> >> +\tint ret;\n> >> +\n> >> +\tunsigned int index = imguDevice == &imgu0_ ? 0 : 1;\n> >> +\tstd::string imguName = \"ipu3-imgu \" + std::to_string(index);\n> >> +\tstd::string inputName = imguName + \" input\";\n> >> +\tstd::string outputName = imguName + \" output\";\n> >> +\tstd::string viewfinderName = imguName + \" viewfinder\";\n> >> +\tstd::string statName = imguName + \" 3a stat\";\n> >> +\n> >> +\tret = imguMediaDev_->open();\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tret = imguMediaDev_->disableLinks();\n> >\n> > This isn't a good idea, as you will interfere with the other ImgU. You\n> > should enable/disable links selectively based on your needs.\n> \n> As I've said, I'll make a per-imgu instance link disabling method. I\n> hope it is enough to disable all links in an imgu instance and we\n> don't have to go link by link...\n\nI haven't checked recently, are the two ImgUs exposed as separate media\ndevices ? If so that's fine, otherwise you'll have to go link by link.\n\n> >> +\tif (ret) {\n> >> +\t\timguMediaDev_->close();\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\t/* Link entities to configure the IMGU unit for capture. */\n> >> +\tlink = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT);\n> >> +\tif (!link) {\n> >> +\t\tLOG(IPU3, Error)\n> >> +\t\t\t<< \"Failed to get link '\" << inputName << \"':0 -> '\"\n> >> +\t\t\t<< imguName << \"':0\";\n> >\n> > Ideally you should use the IMGU_PAD_* constants instead of hardcoding\n> > them in error messages.\n> \n> Ah well, it's just more lines for a single printout, but ok.\n> \n> >> +\t\tret = -ENODEV;\n> >> +\t\tgoto error_close_mediadev;\n> >> +\t}\n> >> +\tlink->setEnabled(true);\n> >> +\n> >> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0);\n> >> +\tif (!link) {\n> >> +\t\tLOG(IPU3, Error)\n> >> +\t\t\t<< \"Failed to get link '\" << imguName << \"':2 -> '\"\n> >> +\t\t\t<< outputName << \"':0\";\n> >> +\t\tret = -ENODEV;\n> >> +\t\tgoto error_close_mediadev;\n> >> +\t}\n> >> +\tlink->setEnabled(true);\n> >> +\n> >> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0);\n> >> +\tif (!link) {\n> >> +\t\tLOG(IPU3, Error)\n> >> +\t\t\t<< \"Failed to get link '\" << imguName << \"':3 -> '\"\n> >> +\t\t\t<< viewfinderName << \"':0\";\n> >> +\t\tret = -ENODEV;\n> >> +\t\tgoto error_close_mediadev;\n> >> +\t}\n> >> +\tlink->setEnabled(true);\n> >\n> > We have a single stream, why do you enable both output and viewfinder ?\n> \n> From my testings, if I keep them disabled (== not linked) the system\n> hangs even if I'm only capturing from the ouput video device. I will\n> do some more testing, as setting up those nodes, queuing and dequeuing\n> buffers there requires some not-nice code to keep track of the buffer\n> indexes, as you've noticed.\n\nAnother kernel bug fix candidate :-)\n\n> >> +\n> >> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0);\n> >> +\tif (!link) {\n> >> +\t\tLOG(IPU3, Error)\n> >> +\t\t\t<< \"Failed to get link '\" << imguName << \"':4 -> '\"\n> >> +\t\t\t<< statName << \"':0\";\n> >> +\t\tret = -ENODEV;\n> >> +\t\tgoto error_close_mediadev;\n> >> +\t}\n> >> +\tlink->setEnabled(true);\n> >\n> > Same here, we don't make use of stats yes, there's no need to enable\n> > this link.\n> >\n> >> +\n> >> +\timguMediaDev_->close();\n> >> +\n> >> +\treturn 0;\n> >> +\n> >> +error_close_mediadev:\n> >> +\timguMediaDev_->close();\n> >\n> > We really really need to think about how to handle open/close and write\n> > down a set of rules. Please make sure this is captured in a \\todo.\n> >\n> >> +\n> >> +\treturn ret;\n> >> +\n> >> +}\n> >> +\n> >> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media,\n> >> +\t\t\t\t\t std::string &name)\n> >\n> > const std::string &\n> >\n> > Same below.\n> >\n> >> +{\n> >> +\tV4L2Device *dev;\n> >\n> > You can move this line down to declare and initialize the variable at\n> > the same time.\n> >\n> >> +\n> >> +\tMediaEntity *entity = media->getEntityByName(name);\n> >> +\tif (!entity) {\n> >> +\t\tLOG(IPU3, Error)\n> >> +\t\t\t<< \"Failed to get entity '\" << name << \"'\";\n> >> +\t\treturn nullptr;\n> >> +\t}\n> >> +\n> >> +\tdev = new V4L2Device(entity);\n> >> +\tif (dev->open())\n> >> +\t\treturn nullptr;\n> >\n> > You'll leak dev, a delete is needed.\n> \n> Ah, right! will fix.\n> \n> >> +\n> >> +\treturn dev;\n> >> +}\n> >> +\n> >> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media,\n> >> +\t\t\t\t\t\t std::string &name)\n> >> +{\n> >> +\tV4L2Subdevice *dev;\n> >> +\n> >> +\tMediaEntity *entity = media->getEntityByName(name);\n> >> +\tif (!entity) {\n> >> +\t\tLOG(IPU3, Error)\n> >> +\t\t\t<< \"Failed to get entity '\" << name << \"'\";\n> >> +\t\treturn nullptr;\n> >> +\t}\n> >> +\n> >> +\tdev = new V4L2Subdevice(entity);\n> >> +\tif (dev->open())\n> >\n> > Leak.\n> >\n> >> +\t\treturn nullptr;\n> >> +\n> >> +\treturn dev;\n> >> +}\n> >\n> > These two functions may be candidates for future generic helpers. Could\n> > you document them and add a \\todo to mention this ?\n> \n> Agreed!\n> \n> >> +\n> >> +/* Create video devices and subdevices for the ImgU instance. */\n> >> +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu)\n> >> +{\n> >> +\tunsigned int index = imgu == &imgu0_ ? 0 : 1;\n> >\n> > A bit ugly, how about adding an index field to the ImguDevice structure\n> > ?\n> \n> Yes. But when would you intialize such a field? at match time? In this\n> function? I'll see how it'll look\n\nI think I'd do it at match time.\n\n> >> +\tstd::string imguName = \"ipu3-imgu \" + std::to_string(index);\n> >> +\tstd::string devName;\n> >> +\n> >> +\timgu->imgu = openSubdevice(imguMediaDev_.get(), imguName);\n> >> +\tif (!imgu->imgu)\n> >> +\t\treturn -ENODEV;\n> >> +\n> >> +\tdevName = imguName + \" input\";\n> >> +\timgu->input = openDevice(imguMediaDev_.get(), devName);\n> >> +\tif (!imgu->input)\n> >> +\t\treturn -ENODEV;\n> >> +\n> >> +\tdevName = imguName + \" output\";\n> >> +\timgu->output = openDevice(imguMediaDev_.get(), devName);\n> >> +\tif (!imgu->output)\n> >> +\t\treturn -ENODEV;\n> >> +\n> >> +\tdevName = imguName + \" viewfinder\";\n> >> +\timgu->viewfinder = openDevice(imguMediaDev_.get(), devName);\n> >> +\tif (!imgu->viewfinder)\n> >> +\t\treturn -ENODEV;\n> >> +\n> >> +\tdevName = imguName + \" 3a stat\";\n> >> +\timgu->stat = openDevice(imguMediaDev_.get(), devName);\n> >\n> > You could drop devName and call\n> >\n> > \timgu->state = openDevice(imguMediaDev_.get(), imguName + \" 3a stat\");\n> >\n> > Up to you.\n> \n> Less lines, so it's better\n> \n> >> +\tif (!imgu->stat)\n> >> +\t\treturn -ENODEV;\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)\n> >> {\n> >> \tint ret;\n> >> @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)\n> >> \t\treturn ret;\n> >>\n> >> \tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(index);\n> >> -\tentity = cio2MediaDev_->getEntityByName(cio2Name);\n> >> -\tif (!entity) {\n> >> -\t\tLOG(IPU3, Error)\n> >> -\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> >> -\t\treturn -EINVAL;\n> >> -\t}\n> >> -\n> >> -\tcio2->output = new V4L2Device(entity);\n> >> -\tret = cio2->output->open();\n> >> -\tif (ret)\n> >> +\tcio2->output = openDevice(cio2MediaDev_.get(), cio2Name);\n> >> +\tif (!cio2->output)\n> >> \t\treturn ret;\n> >>\n> >> \treturn 0;","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 F0C91600CB\n\tfor ;\n\tSun, 10 Mar 2019 14:08:39 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 667FC57D;\n\tSun, 10 Mar 2019 14:08:39 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1552223319;\n\tbh=kzKtizZYmXXdOXGXyGwFAuo72C3BqxH9buJVoU0Y6wg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gs8YUzIAE/pmkvvheW0lp8LmBeJZd+SNvQBjUwvzMJIk12zPiDInafXq+1laOEb/1\n\tvos1k9wiba/OGUz03NnXTlgvk5iPHQS7Zdj3H9/HLbRSd0vZr8tjla4guevUV9pSUF\n\tWpD/j2q6aUcI8Uvwu0kD5qwceS8d4fTHMl8g+noY=","Date":"Sun, 10 Mar 2019 15:08:33 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190310130833.GC4814@pendragon.ideasonboard.com>","References":"<20190228200410.3022-1-jacopo@jmondi.org>\n\t<20190228200410.3022-4-jacopo@jmondi.org>\n\t<20190302214129.GH4682@pendragon.ideasonboard.com>\n\t<20190309205018.se6y2fjd7sgba54d@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190309205018.se6y2fjd7sgba54d@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and\n\tlink ImgU devices","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":"Sun, 10 Mar 2019 13:08:40 -0000"}},{"id":1039,"web_url":"https://patchwork.libcamera.org/comment/1039/","msgid":"<20190311081041.e3qt325qf3afb765@uno.localdomain>","date":"2019-03-11T08:10:41","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and\n\tlink ImgU devices","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Mar 10, 2019 at 03:08:33PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Sat, Mar 09, 2019 at 09:50:18PM +0100, Jacopo Mondi wrote:\n> > On Sat, Mar 02, 2019 at 11:41:29PM +0200, Laurent Pinchart wrote:\n> > > On Thu, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote:\n> > >> Create video devices and subdevices associated with an ImgU unit, and\n> > >> link the entities in the media graph to prepare the device for capture\n> > >> operations at stream configuration time.\n> > >>\n> > >> As we support a single stream at the moment, always select imgu0.\n> > >>\n> > >> Signed-off-by: Jacopo Mondi \n> > >> ---\n> > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++--\n> > >> 1 file changed, 207 insertions(+), 12 deletions(-)\n> > >>\n> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> index 4f1ab72debf8..9fa59c1bc97e 100644\n> > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> @@ -24,6 +24,28 @@ namespace libcamera {\n> > >>\n> > >> LOG_DEFINE_CATEGORY(IPU3)\n> > >>\n> > >> +struct ImguDevice {\n> > >\n> > > s/ImguDevice/ImgUDevice/ ?\n> >\n> > I would like ImguDevice best, but I'll use ImgUDevice for consistency\n> > with the kernel documentation.\n> >\n> > >> +\tImguDevice()\n> > >> +\t\t: imgu(nullptr), input(nullptr), output(nullptr),\n> > >> +\t\t viewfinder(nullptr), stat(nullptr) {}\n> > >\n> > > \t{\n> > > \t}\n> > >\n> > >> +\n> > >> +\t~ImguDevice()\n> > >> +\t{\n> > >> +\t\tdelete imgu;\n> > >> +\t\tdelete input;\n> > >> +\t\tdelete output;\n> > >> +\t\tdelete viewfinder;\n> > >> +\t\tdelete stat;\n> > >> +\t}\n> > >\n> > > Unlike for the CIO2Device, this could be made a class, as I expect it\n> > > will have more code associated with it, and the ImgU instances will be\n> > > shared between the multiple camera instances. The linkImgu function\n> > > would be a good candidate.\n> >\n> > I'll move that to this class. I will has well add a \"disableLinks()\"\n> > methods as you suggested below.\n> >\n> > >> +\n> > >> +\tV4L2Subdevice *imgu;\n> > >> +\tV4L2Device *input;\n> > >> +\tV4L2Device *output;\n> > >> +\tV4L2Device *viewfinder;\n> > >> +\tV4L2Device *stat;\n> > >> +\t/* TODO: add param video device for 3A tuning */\n> > >\n> > > \\todo for consistency, even if this isn't a doxygen comment ?\n> >\n> > Will change all of these.\n> >\n> > >> +};\n> > >> +\n> > >> struct Cio2Device {\n> > >> \tCio2Device()\n> > >> \t\t: output(nullptr), csi2(nullptr), sensor(nullptr) {}\n> > >> @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData\n> > >> {\n> > >> public:\n> > >> \tCio2Device cio2;\n> > >> +\tImguDevice *imgu;\n> > >>\n> > >> \tStream stream_;\n> > >> };\n> > >> @@ -71,6 +94,10 @@ public:\n> > >> \tbool match(DeviceEnumerator *enumerator);\n> > >>\n> > >> private:\n> > >> +\tstatic constexpr unsigned int IMGU_PAD_INPUT = 0;\n> > >> +\tstatic constexpr unsigned int IMGU_PAD_OUTPUT = 2;\n> > >> +\tstatic constexpr unsigned int IMGU_PAD_VF = 3;\n> > >> +\tstatic constexpr unsigned int IMGU_PAD_STAT = 4;\n> > >> \tstatic constexpr unsigned int IPU3_BUF_NUM = 4;\n> > >\n> > > I'd move that to the ImgUDevice struct/class. You can then remove the\n> > > IMGU_ prefix.\n> >\n> > Agreed.\n> >\n> > >>\n> > >> \tIPU3CameraData *cameraData(const Camera *camera)\n> > >> @@ -79,9 +106,17 @@ private:\n> > >> \t\t\tPipelineHandler::cameraData(camera));\n> > >> \t}\n> > >>\n> > >> +\tint linkImgu(ImguDevice *imgu);\n> > >> +\n> > >> +\tV4L2Device *openDevice(MediaDevice *media, std::string &name);\n> > >> +\tV4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name);\n> > >> +\tint initImgu(ImguDevice *imgu);\n> > >> \tint initCio2(unsigned int index, Cio2Device *cio2);\n> > >> \tvoid registerCameras();\n> > >>\n> > >> +\tImguDevice imgu0_;\n> > >> +\tImguDevice imgu1_;\n> > >\n> > > Maybe an array with two entries ?\n> >\n> > They're two, and will stay two, but yes, I'll see how it looks.\n> >\n> > >> +\n> > >> \tstd::shared_ptr cio2MediaDev_;\n> > >> \tstd::shared_ptr imguMediaDev_;\n> > >> };\n> > >> @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > >> \tV4L2DeviceFormat devFormat = {};\n> > >> \tint ret;\n> > >>\n> > >> +\t/*\n> > >> +\t * TODO: dynamically assign ImgU devices; as of now, with a single\n> > >> +\t * stream supported, always use 'imgu0'.\n> > >\n> > > \t/**\n> > > \t * \\todo\n> > >\n> > > ?\n> > >\n> > >> +\t */\n> > >> +\tdata->imgu = &imgu0_;\n> > >\n> > > How about moving this to camera creation time, and assigned imgu0 to the\n> > > first sensor and imgu1 to the second one ?\n> > >\n> >\n> > I've put it here, as I assumed ImgU association with Cameras will\n> > depend on the number of required streams. If you confirm my\n> > understanding I would keep it here instead of moving it back and\n> > forth.\n>\n> Your understanding is correct, but with this implementation imgu0 is\n> used for both cameras, so we can only use one camera at a time. If we\n> assign imgu0 to the first camera and imgu1 to the second one we can\n> start using both cameras, with up to two streams each, and later expand\n> that to more streams.\n>\n\nI see. I'll do the assignement there (and limit the number of\nsupported cameras to two for now)\n\n> > >> +\tret = linkImgu(data->imgu);\n> > >> +\tif (ret)\n> > >> +\t\treturn ret;\n> > >> +\n> > >> \t/*\n> > >> \t * FIXME: as of now, the format gets applied to the sensor and is\n> > >> \t * propagated along the pipeline. It should instead be applied on the\n> > >> @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > >> \tif (cio2MediaDev_->open())\n> > >> \t\tgoto error_release_mdev;\n> > >>\n> > >> +\tif (imguMediaDev_->open())\n> > >> +\t\tgoto error_close_mdev;\n> > >> +\n> > >> \tif (cio2MediaDev_->disableLinks())\n> > >> -\t\tgoto error_close_cio2;\n> > >> +\t\tgoto error_close_mdev;\n> > >> +\n> > >> +\tif (initImgu(&imgu0_))\n> > >> +\t\tgoto error_close_mdev;\n> > >> +\n> > >> +\tif (initImgu(&imgu1_))\n> > >> +\t\tgoto error_close_mdev;\n> > >> +\n> > >>\n> > >> \tregisterCameras();\n> > >>\n> > >> \tcio2MediaDev_->close();\n> > >> +\timguMediaDev_->close();\n> > >>\n> > >> \treturn true;\n> > >>\n> > >> -error_close_cio2:\n> > >> +error_close_mdev:\n> > >> \tcio2MediaDev_->close();\n> > >> +\timguMediaDev_->close();\n> > >\n> > > Error handling will be simplified when you'll rebase. I'd go as far as\n> > > calling close() in the PipelineHandlerIPU3 destructor and remove the\n> > > error path here.\n> >\n> > It has been simplified by rebase, yes. I'll see how moving close() in\n> > destructor looks like (I think it's good)\n> >\n\nActually, the pipeline handler will be destroyed at library tear-down\ntime, isn't it? So I don't think we can rely on its destructor to close the\nmedia devices...\n\n> > >>\n> > >> error_release_mdev:\n> > >> \tcio2MediaDev_->release();\n> > >> @@ -353,6 +409,153 @@ error_release_mdev:\n> > >> \treturn false;\n> > >> }\n> > >>\n> > >> +/* Link entities in the ImgU unit to prepare for capture operations. */\n> > >> +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice)\n> > >> +{\n> > >> +\tMediaLink *link;\n> > >> +\tint ret;\n> > >> +\n> > >> +\tunsigned int index = imguDevice == &imgu0_ ? 0 : 1;\n> > >> +\tstd::string imguName = \"ipu3-imgu \" + std::to_string(index);\n> > >> +\tstd::string inputName = imguName + \" input\";\n> > >> +\tstd::string outputName = imguName + \" output\";\n> > >> +\tstd::string viewfinderName = imguName + \" viewfinder\";\n> > >> +\tstd::string statName = imguName + \" 3a stat\";\n> > >> +\n> > >> +\tret = imguMediaDev_->open();\n> > >> +\tif (ret)\n> > >> +\t\treturn ret;\n> > >> +\n> > >> +\tret = imguMediaDev_->disableLinks();\n> > >\n> > > This isn't a good idea, as you will interfere with the other ImgU. You\n> > > should enable/disable links selectively based on your needs.\n> >\n> > As I've said, I'll make a per-imgu instance link disabling method. I\n> > hope it is enough to disable all links in an imgu instance and we\n> > don't have to go link by link...\n>\n> I haven't checked recently, are the two ImgUs exposed as separate media\n> devices ? If so that's fine, otherwise you'll have to go link by link.\n>\n\nNo, the imgu units are part of the same media device. But it's fine\nanyway, I will create a method that goes link by link :(\n\n> > >> +\tif (ret) {\n> > >> +\t\timguMediaDev_->close();\n> > >> +\t\treturn ret;\n> > >> +\t}\n> > >> +\n> > >> +\t/* Link entities to configure the IMGU unit for capture. */\n> > >> +\tlink = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT);\n> > >> +\tif (!link) {\n> > >> +\t\tLOG(IPU3, Error)\n> > >> +\t\t\t<< \"Failed to get link '\" << inputName << \"':0 -> '\"\n> > >> +\t\t\t<< imguName << \"':0\";\n> > >\n> > > Ideally you should use the IMGU_PAD_* constants instead of hardcoding\n> > > them in error messages.\n> >\n> > Ah well, it's just more lines for a single printout, but ok.\n> >\n> > >> +\t\tret = -ENODEV;\n> > >> +\t\tgoto error_close_mediadev;\n> > >> +\t}\n> > >> +\tlink->setEnabled(true);\n> > >> +\n> > >> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0);\n> > >> +\tif (!link) {\n> > >> +\t\tLOG(IPU3, Error)\n> > >> +\t\t\t<< \"Failed to get link '\" << imguName << \"':2 -> '\"\n> > >> +\t\t\t<< outputName << \"':0\";\n> > >> +\t\tret = -ENODEV;\n> > >> +\t\tgoto error_close_mediadev;\n> > >> +\t}\n> > >> +\tlink->setEnabled(true);\n> > >> +\n> > >> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0);\n> > >> +\tif (!link) {\n> > >> +\t\tLOG(IPU3, Error)\n> > >> +\t\t\t<< \"Failed to get link '\" << imguName << \"':3 -> '\"\n> > >> +\t\t\t<< viewfinderName << \"':0\";\n> > >> +\t\tret = -ENODEV;\n> > >> +\t\tgoto error_close_mediadev;\n> > >> +\t}\n> > >> +\tlink->setEnabled(true);\n> > >\n> > > We have a single stream, why do you enable both output and viewfinder ?\n> >\n> > From my testings, if I keep them disabled (== not linked) the system\n> > hangs even if I'm only capturing from the ouput video device. I will\n> > do some more testing, as setting up those nodes, queuing and dequeuing\n> > buffers there requires some not-nice code to keep track of the buffer\n> > indexes, as you've noticed.\n>\n> Another kernel bug fix candidate :-)\n>\n\nI plan to do more testing once I have included all these comments in\nv2. Will report how they go..\n\nThanks\n j\n\n> > >> +\n> > >> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0);\n> > >> +\tif (!link) {\n> > >> +\t\tLOG(IPU3, Error)\n> > >> +\t\t\t<< \"Failed to get link '\" << imguName << \"':4 -> '\"\n> > >> +\t\t\t<< statName << \"':0\";\n> > >> +\t\tret = -ENODEV;\n> > >> +\t\tgoto error_close_mediadev;\n> > >> +\t}\n> > >> +\tlink->setEnabled(true);\n> > >\n> > > Same here, we don't make use of stats yes, there's no need to enable\n> > > this link.\n> > >\n> > >> +\n> > >> +\timguMediaDev_->close();\n> > >> +\n> > >> +\treturn 0;\n> > >> +\n> > >> +error_close_mediadev:\n> > >> +\timguMediaDev_->close();\n> > >\n> > > We really really need to think about how to handle open/close and write\n> > > down a set of rules. Please make sure this is captured in a \\todo.\n> > >\n> > >> +\n> > >> +\treturn ret;\n> > >> +\n> > >> +}\n> > >> +\n> > >> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media,\n> > >> +\t\t\t\t\t std::string &name)\n> > >\n> > > const std::string &\n> > >\n> > > Same below.\n> > >\n> > >> +{\n> > >> +\tV4L2Device *dev;\n> > >\n> > > You can move this line down to declare and initialize the variable at\n> > > the same time.\n> > >\n> > >> +\n> > >> +\tMediaEntity *entity = media->getEntityByName(name);\n> > >> +\tif (!entity) {\n> > >> +\t\tLOG(IPU3, Error)\n> > >> +\t\t\t<< \"Failed to get entity '\" << name << \"'\";\n> > >> +\t\treturn nullptr;\n> > >> +\t}\n> > >> +\n> > >> +\tdev = new V4L2Device(entity);\n> > >> +\tif (dev->open())\n> > >> +\t\treturn nullptr;\n> > >\n> > > You'll leak dev, a delete is needed.\n> >\n> > Ah, right! will fix.\n> >\n> > >> +\n> > >> +\treturn dev;\n> > >> +}\n> > >> +\n> > >> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media,\n> > >> +\t\t\t\t\t\t std::string &name)\n> > >> +{\n> > >> +\tV4L2Subdevice *dev;\n> > >> +\n> > >> +\tMediaEntity *entity = media->getEntityByName(name);\n> > >> +\tif (!entity) {\n> > >> +\t\tLOG(IPU3, Error)\n> > >> +\t\t\t<< \"Failed to get entity '\" << name << \"'\";\n> > >> +\t\treturn nullptr;\n> > >> +\t}\n> > >> +\n> > >> +\tdev = new V4L2Subdevice(entity);\n> > >> +\tif (dev->open())\n> > >\n> > > Leak.\n> > >\n> > >> +\t\treturn nullptr;\n> > >> +\n> > >> +\treturn dev;\n> > >> +}\n> > >\n> > > These two functions may be candidates for future generic helpers. Could\n> > > you document them and add a \\todo to mention this ?\n> >\n> > Agreed!\n> >\n> > >> +\n> > >> +/* Create video devices and subdevices for the ImgU instance. */\n> > >> +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu)\n> > >> +{\n> > >> +\tunsigned int index = imgu == &imgu0_ ? 0 : 1;\n> > >\n> > > A bit ugly, how about adding an index field to the ImguDevice structure\n> > > ?\n> >\n> > Yes. But when would you intialize such a field? at match time? In this\n> > function? I'll see how it'll look\n>\n> I think I'd do it at match time.\n>\n> > >> +\tstd::string imguName = \"ipu3-imgu \" + std::to_string(index);\n> > >> +\tstd::string devName;\n> > >> +\n> > >> +\timgu->imgu = openSubdevice(imguMediaDev_.get(), imguName);\n> > >> +\tif (!imgu->imgu)\n> > >> +\t\treturn -ENODEV;\n> > >> +\n> > >> +\tdevName = imguName + \" input\";\n> > >> +\timgu->input = openDevice(imguMediaDev_.get(), devName);\n> > >> +\tif (!imgu->input)\n> > >> +\t\treturn -ENODEV;\n> > >> +\n> > >> +\tdevName = imguName + \" output\";\n> > >> +\timgu->output = openDevice(imguMediaDev_.get(), devName);\n> > >> +\tif (!imgu->output)\n> > >> +\t\treturn -ENODEV;\n> > >> +\n> > >> +\tdevName = imguName + \" viewfinder\";\n> > >> +\timgu->viewfinder = openDevice(imguMediaDev_.get(), devName);\n> > >> +\tif (!imgu->viewfinder)\n> > >> +\t\treturn -ENODEV;\n> > >> +\n> > >> +\tdevName = imguName + \" 3a stat\";\n> > >> +\timgu->stat = openDevice(imguMediaDev_.get(), devName);\n> > >\n> > > You could drop devName and call\n> > >\n> > > \timgu->state = openDevice(imguMediaDev_.get(), imguName + \" 3a stat\");\n> > >\n> > > Up to you.\n> >\n> > Less lines, so it's better\n> >\n> > >> +\tif (!imgu->stat)\n> > >> +\t\treturn -ENODEV;\n> > >> +\n> > >> +\treturn 0;\n> > >> +}\n> > >> +\n> > >> int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)\n> > >> {\n> > >> \tint ret;\n> > >> @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)\n> > >> \t\treturn ret;\n> > >>\n> > >> \tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(index);\n> > >> -\tentity = cio2MediaDev_->getEntityByName(cio2Name);\n> > >> -\tif (!entity) {\n> > >> -\t\tLOG(IPU3, Error)\n> > >> -\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> > >> -\t\treturn -EINVAL;\n> > >> -\t}\n> > >> -\n> > >> -\tcio2->output = new V4L2Device(entity);\n> > >> -\tret = cio2->output->open();\n> > >> -\tif (ret)\n> > >> +\tcio2->output = openDevice(cio2MediaDev_.get(), cio2Name);\n> > >> +\tif (!cio2->output)\n> > >> \t\treturn ret;\n> > >>\n> > >> \treturn 0;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8DF81600FC\n\tfor ;\n\tMon, 11 Mar 2019 09:10:07 +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 relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 075CF60003;\n\tMon, 11 Mar 2019 08:10:06 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 11 Mar 2019 09:10:41 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190311081041.e3qt325qf3afb765@uno.localdomain>","References":"<20190228200410.3022-1-jacopo@jmondi.org>\n\t<20190228200410.3022-4-jacopo@jmondi.org>\n\t<20190302214129.GH4682@pendragon.ideasonboard.com>\n\t<20190309205018.se6y2fjd7sgba54d@uno.localdomain>\n\t<20190310130833.GC4814@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"tiqapx6unau3khby\"","Content-Disposition":"inline","In-Reply-To":"<20190310130833.GC4814@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and\n\tlink ImgU devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 11 Mar 2019 08:10:07 -0000"}},{"id":1041,"web_url":"https://patchwork.libcamera.org/comment/1041/","msgid":"<20190311083320.GC4775@pendragon.ideasonboard.com>","date":"2019-03-11T08:33:20","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and\n\tlink ImgU devices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Mar 11, 2019 at 09:10:41AM +0100, Jacopo Mondi wrote:\n> On Sun, Mar 10, 2019 at 03:08:33PM +0200, Laurent Pinchart wrote:\n> > On Sat, Mar 09, 2019 at 09:50:18PM +0100, Jacopo Mondi wrote:\n> >> On Sat, Mar 02, 2019 at 11:41:29PM +0200, Laurent Pinchart wrote:\n> >>> On Thu, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote:\n> >>>> Create video devices and subdevices associated with an ImgU unit, and\n> >>>> link the entities in the media graph to prepare the device for capture\n> >>>> operations at stream configuration time.\n> >>>>\n> >>>> As we support a single stream at the moment, always select imgu0.\n> >>>>\n> >>>> Signed-off-by: Jacopo Mondi \n> >>>> ---\n> >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++--\n> >>>> 1 file changed, 207 insertions(+), 12 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> index 4f1ab72debf8..9fa59c1bc97e 100644\n> >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> @@ -24,6 +24,28 @@ namespace libcamera {\n> >>>>\n> >>>> LOG_DEFINE_CATEGORY(IPU3)\n> >>>>\n> >>>> +struct ImguDevice {\n> >>>\n> >>> s/ImguDevice/ImgUDevice/ ?\n> >>\n> >> I would like ImguDevice best, but I'll use ImgUDevice for consistency\n> >> with the kernel documentation.\n> >>\n> >>>> +\tImguDevice()\n> >>>> +\t\t: imgu(nullptr), input(nullptr), output(nullptr),\n> >>>> +\t\t viewfinder(nullptr), stat(nullptr) {}\n> >>>\n> >>> \t{\n> >>> \t}\n> >>>\n> >>>> +\n> >>>> +\t~ImguDevice()\n> >>>> +\t{\n> >>>> +\t\tdelete imgu;\n> >>>> +\t\tdelete input;\n> >>>> +\t\tdelete output;\n> >>>> +\t\tdelete viewfinder;\n> >>>> +\t\tdelete stat;\n> >>>> +\t}\n> >>>\n> >>> Unlike for the CIO2Device, this could be made a class, as I expect it\n> >>> will have more code associated with it, and the ImgU instances will be\n> >>> shared between the multiple camera instances. The linkImgu function\n> >>> would be a good candidate.\n> >>\n> >> I'll move that to this class. I will has well add a \"disableLinks()\"\n> >> methods as you suggested below.\n> >>\n> >>>> +\n> >>>> +\tV4L2Subdevice *imgu;\n> >>>> +\tV4L2Device *input;\n> >>>> +\tV4L2Device *output;\n> >>>> +\tV4L2Device *viewfinder;\n> >>>> +\tV4L2Device *stat;\n> >>>> +\t/* TODO: add param video device for 3A tuning */\n> >>>\n> >>> \\todo for consistency, even if this isn't a doxygen comment ?\n> >>\n> >> Will change all of these.\n> >>\n> >>>> +};\n> >>>> +\n> >>>> struct Cio2Device {\n> >>>> \tCio2Device()\n> >>>> \t\t: output(nullptr), csi2(nullptr), sensor(nullptr) {}\n> >>>> @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData\n> >>>> {\n> >>>> public:\n> >>>> \tCio2Device cio2;\n> >>>> +\tImguDevice *imgu;\n> >>>>\n> >>>> \tStream stream_;\n> >>>> };\n> >>>> @@ -71,6 +94,10 @@ public:\n> >>>> \tbool match(DeviceEnumerator *enumerator);\n> >>>>\n> >>>> private:\n> >>>> +\tstatic constexpr unsigned int IMGU_PAD_INPUT = 0;\n> >>>> +\tstatic constexpr unsigned int IMGU_PAD_OUTPUT = 2;\n> >>>> +\tstatic constexpr unsigned int IMGU_PAD_VF = 3;\n> >>>> +\tstatic constexpr unsigned int IMGU_PAD_STAT = 4;\n> >>>> \tstatic constexpr unsigned int IPU3_BUF_NUM = 4;\n> >>>\n> >>> I'd move that to the ImgUDevice struct/class. You can then remove the\n> >>> IMGU_ prefix.\n> >>\n> >> Agreed.\n> >>\n> >>>>\n> >>>> \tIPU3CameraData *cameraData(const Camera *camera)\n> >>>> @@ -79,9 +106,17 @@ private:\n> >>>> \t\t\tPipelineHandler::cameraData(camera));\n> >>>> \t}\n> >>>>\n> >>>> +\tint linkImgu(ImguDevice *imgu);\n> >>>> +\n> >>>> +\tV4L2Device *openDevice(MediaDevice *media, std::string &name);\n> >>>> +\tV4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name);\n> >>>> +\tint initImgu(ImguDevice *imgu);\n> >>>> \tint initCio2(unsigned int index, Cio2Device *cio2);\n> >>>> \tvoid registerCameras();\n> >>>>\n> >>>> +\tImguDevice imgu0_;\n> >>>> +\tImguDevice imgu1_;\n> >>>\n> >>> Maybe an array with two entries ?\n> >>\n> >> They're two, and will stay two, but yes, I'll see how it looks.\n> >>\n> >>>> +\n> >>>> \tstd::shared_ptr cio2MediaDev_;\n> >>>> \tstd::shared_ptr imguMediaDev_;\n> >>>> };\n> >>>> @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> >>>> \tV4L2DeviceFormat devFormat = {};\n> >>>> \tint ret;\n> >>>>\n> >>>> +\t/*\n> >>>> +\t * TODO: dynamically assign ImgU devices; as of now, with a single\n> >>>> +\t * stream supported, always use 'imgu0'.\n> >>>\n> >>> \t/**\n> >>> \t * \\todo\n> >>>\n> >>> ?\n> >>>\n> >>>> +\t */\n> >>>> +\tdata->imgu = &imgu0_;\n> >>>\n> >>> How about moving this to camera creation time, and assigned imgu0 to the\n> >>> first sensor and imgu1 to the second one ?\n> >>>\n> >>\n> >> I've put it here, as I assumed ImgU association with Cameras will\n> >> depend on the number of required streams. If you confirm my\n> >> understanding I would keep it here instead of moving it back and\n> >> forth.\n> >\n> > Your understanding is correct, but with this implementation imgu0 is\n> > used for both cameras, so we can only use one camera at a time. If we\n> > assign imgu0 to the first camera and imgu1 to the second one we can\n> > start using both cameras, with up to two streams each, and later expand\n> > that to more streams.\n> >\n> \n> I see. I'll do the assignement there (and limit the number of\n> supported cameras to two for now)\n> \n> >>>> +\tret = linkImgu(data->imgu);\n> >>>> +\tif (ret)\n> >>>> +\t\treturn ret;\n> >>>> +\n> >>>> \t/*\n> >>>> \t * FIXME: as of now, the format gets applied to the sensor and is\n> >>>> \t * propagated along the pipeline. It should instead be applied on the\n> >>>> @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> >>>> \tif (cio2MediaDev_->open())\n> >>>> \t\tgoto error_release_mdev;\n> >>>>\n> >>>> +\tif (imguMediaDev_->open())\n> >>>> +\t\tgoto error_close_mdev;\n> >>>> +\n> >>>> \tif (cio2MediaDev_->disableLinks())\n> >>>> -\t\tgoto error_close_cio2;\n> >>>> +\t\tgoto error_close_mdev;\n> >>>> +\n> >>>> +\tif (initImgu(&imgu0_))\n> >>>> +\t\tgoto error_close_mdev;\n> >>>> +\n> >>>> +\tif (initImgu(&imgu1_))\n> >>>> +\t\tgoto error_close_mdev;\n> >>>> +\n> >>>>\n> >>>> \tregisterCameras();\n> >>>>\n> >>>> \tcio2MediaDev_->close();\n> >>>> +\timguMediaDev_->close();\n> >>>>\n> >>>> \treturn true;\n> >>>>\n> >>>> -error_close_cio2:\n> >>>> +error_close_mdev:\n> >>>> \tcio2MediaDev_->close();\n> >>>> +\timguMediaDev_->close();\n> >>>\n> >>> Error handling will be simplified when you'll rebase. I'd go as far as\n> >>> calling close() in the PipelineHandlerIPU3 destructor and remove the\n> >>> error path here.\n> >>\n> >> It has been simplified by rebase, yes. I'll see how moving close() in\n> >> destructor looks like (I think it's good)\n> >>\n> \n> Actually, the pipeline handler will be destroyed at library tear-down\n> time, isn't it? So I don't think we can rely on its destructor to close the\n> media devices...\n\nIt will be destroyed at the earlier of CameraManager::stop() time, when\nthe device is unplugged, or immediately after the match() function\nreturns an error. I think we'll need to forward the camera open() and\nclose() calls to the pipeline manager for normal operation. The error\npath could still rely on the destructor, but that would be assymetrical,\nso maybe not the best idea. Feel free to keep the calls here.\n\n> >>>>\n> >>>> error_release_mdev:\n> >>>> \tcio2MediaDev_->release();\n> >>>> @@ -353,6 +409,153 @@ error_release_mdev:\n> >>>> \treturn false;\n> >>>> }\n> >>>>\n> >>>> +/* Link entities in the ImgU unit to prepare for capture operations. */\n> >>>> +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice)\n> >>>> +{\n> >>>> +\tMediaLink *link;\n> >>>> +\tint ret;\n> >>>> +\n> >>>> +\tunsigned int index = imguDevice == &imgu0_ ? 0 : 1;\n> >>>> +\tstd::string imguName = \"ipu3-imgu \" + std::to_string(index);\n> >>>> +\tstd::string inputName = imguName + \" input\";\n> >>>> +\tstd::string outputName = imguName + \" output\";\n> >>>> +\tstd::string viewfinderName = imguName + \" viewfinder\";\n> >>>> +\tstd::string statName = imguName + \" 3a stat\";\n> >>>> +\n> >>>> +\tret = imguMediaDev_->open();\n> >>>> +\tif (ret)\n> >>>> +\t\treturn ret;\n> >>>> +\n> >>>> +\tret = imguMediaDev_->disableLinks();\n> >>>\n> >>> This isn't a good idea, as you will interfere with the other ImgU. You\n> >>> should enable/disable links selectively based on your needs.\n> >>\n> >> As I've said, I'll make a per-imgu instance link disabling method. I\n> >> hope it is enough to disable all links in an imgu instance and we\n> >> don't have to go link by link...\n> >\n> > I haven't checked recently, are the two ImgUs exposed as separate media\n> > devices ? If so that's fine, otherwise you'll have to go link by link.\n> \n> No, the imgu units are part of the same media device. But it's fine\n> anyway, I will create a method that goes link by link :(\n\nJust thinking out loud, would a helper in MediaDevice that would take a\nvector of links be useful for other pipeline handlers ?\n\n> >>>> +\tif (ret) {\n> >>>> +\t\timguMediaDev_->close();\n> >>>> +\t\treturn ret;\n> >>>> +\t}\n> >>>> +\n> >>>> +\t/* Link entities to configure the IMGU unit for capture. */\n> >>>> +\tlink = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT);\n> >>>> +\tif (!link) {\n> >>>> +\t\tLOG(IPU3, Error)\n> >>>> +\t\t\t<< \"Failed to get link '\" << inputName << \"':0 -> '\"\n> >>>> +\t\t\t<< imguName << \"':0\";\n> >>>\n> >>> Ideally you should use the IMGU_PAD_* constants instead of hardcoding\n> >>> them in error messages.\n> >>\n> >> Ah well, it's just more lines for a single printout, but ok.\n> >>\n> >>>> +\t\tret = -ENODEV;\n> >>>> +\t\tgoto error_close_mediadev;\n> >>>> +\t}\n> >>>> +\tlink->setEnabled(true);\n> >>>> +\n> >>>> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0);\n> >>>> +\tif (!link) {\n> >>>> +\t\tLOG(IPU3, Error)\n> >>>> +\t\t\t<< \"Failed to get link '\" << imguName << \"':2 -> '\"\n> >>>> +\t\t\t<< outputName << \"':0\";\n> >>>> +\t\tret = -ENODEV;\n> >>>> +\t\tgoto error_close_mediadev;\n> >>>> +\t}\n> >>>> +\tlink->setEnabled(true);\n> >>>> +\n> >>>> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0);\n> >>>> +\tif (!link) {\n> >>>> +\t\tLOG(IPU3, Error)\n> >>>> +\t\t\t<< \"Failed to get link '\" << imguName << \"':3 -> '\"\n> >>>> +\t\t\t<< viewfinderName << \"':0\";\n> >>>> +\t\tret = -ENODEV;\n> >>>> +\t\tgoto error_close_mediadev;\n> >>>> +\t}\n> >>>> +\tlink->setEnabled(true);\n> >>>\n> >>> We have a single stream, why do you enable both output and viewfinder ?\n> >>\n> >> From my testings, if I keep them disabled (== not linked) the system\n> >> hangs even if I'm only capturing from the ouput video device. I will\n> >> do some more testing, as setting up those nodes, queuing and dequeuing\n> >> buffers there requires some not-nice code to keep track of the buffer\n> >> indexes, as you've noticed.\n> >\n> > Another kernel bug fix candidate :-)\n> \n> I plan to do more testing once I have included all these comments in\n> v2. Will report how they go..\n> \n> >>>> +\n> >>>> +\tlink = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0);\n> >>>> +\tif (!link) {\n> >>>> +\t\tLOG(IPU3, Error)\n> >>>> +\t\t\t<< \"Failed to get link '\" << imguName << \"':4 -> '\"\n> >>>> +\t\t\t<< statName << \"':0\";\n> >>>> +\t\tret = -ENODEV;\n> >>>> +\t\tgoto error_close_mediadev;\n> >>>> +\t}\n> >>>> +\tlink->setEnabled(true);\n> >>>\n> >>> Same here, we don't make use of stats yes, there's no need to enable\n> >>> this link.\n> >>>\n> >>>> +\n> >>>> +\timguMediaDev_->close();\n> >>>> +\n> >>>> +\treturn 0;\n> >>>> +\n> >>>> +error_close_mediadev:\n> >>>> +\timguMediaDev_->close();\n> >>>\n> >>> We really really need to think about how to handle open/close and write\n> >>> down a set of rules. Please make sure this is captured in a \\todo.\n> >>>\n> >>>> +\n> >>>> +\treturn ret;\n> >>>> +\n> >>>> +}\n> >>>> +\n> >>>> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media,\n> >>>> +\t\t\t\t\t std::string &name)\n> >>>\n> >>> const std::string &\n> >>>\n> >>> Same below.\n> >>>\n> >>>> +{\n> >>>> +\tV4L2Device *dev;\n> >>>\n> >>> You can move this line down to declare and initialize the variable at\n> >>> the same time.\n> >>>\n> >>>> +\n> >>>> +\tMediaEntity *entity = media->getEntityByName(name);\n> >>>> +\tif (!entity) {\n> >>>> +\t\tLOG(IPU3, Error)\n> >>>> +\t\t\t<< \"Failed to get entity '\" << name << \"'\";\n> >>>> +\t\treturn nullptr;\n> >>>> +\t}\n> >>>> +\n> >>>> +\tdev = new V4L2Device(entity);\n> >>>> +\tif (dev->open())\n> >>>> +\t\treturn nullptr;\n> >>>\n> >>> You'll leak dev, a delete is needed.\n> >>\n> >> Ah, right! will fix.\n> >>\n> >>>> +\n> >>>> +\treturn dev;\n> >>>> +}\n> >>>> +\n> >>>> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media,\n> >>>> +\t\t\t\t\t\t std::string &name)\n> >>>> +{\n> >>>> +\tV4L2Subdevice *dev;\n> >>>> +\n> >>>> +\tMediaEntity *entity = media->getEntityByName(name);\n> >>>> +\tif (!entity) {\n> >>>> +\t\tLOG(IPU3, Error)\n> >>>> +\t\t\t<< \"Failed to get entity '\" << name << \"'\";\n> >>>> +\t\treturn nullptr;\n> >>>> +\t}\n> >>>> +\n> >>>> +\tdev = new V4L2Subdevice(entity);\n> >>>> +\tif (dev->open())\n> >>>\n> >>> Leak.\n> >>>\n> >>>> +\t\treturn nullptr;\n> >>>> +\n> >>>> +\treturn dev;\n> >>>> +}\n> >>>\n> >>> These two functions may be candidates for future generic helpers. Could\n> >>> you document them and add a \\todo to mention this ?\n> >>\n> >> Agreed!\n> >>\n> >>>> +\n> >>>> +/* Create video devices and subdevices for the ImgU instance. */\n> >>>> +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu)\n> >>>> +{\n> >>>> +\tunsigned int index = imgu == &imgu0_ ? 0 : 1;\n> >>>\n> >>> A bit ugly, how about adding an index field to the ImguDevice structure\n> >>> ?\n> >>\n> >> Yes. But when would you intialize such a field? at match time? In this\n> >> function? I'll see how it'll look\n> >\n> > I think I'd do it at match time.\n> >\n> >>>> +\tstd::string imguName = \"ipu3-imgu \" + std::to_string(index);\n> >>>> +\tstd::string devName;\n> >>>> +\n> >>>> +\timgu->imgu = openSubdevice(imguMediaDev_.get(), imguName);\n> >>>> +\tif (!imgu->imgu)\n> >>>> +\t\treturn -ENODEV;\n> >>>> +\n> >>>> +\tdevName = imguName + \" input\";\n> >>>> +\timgu->input = openDevice(imguMediaDev_.get(), devName);\n> >>>> +\tif (!imgu->input)\n> >>>> +\t\treturn -ENODEV;\n> >>>> +\n> >>>> +\tdevName = imguName + \" output\";\n> >>>> +\timgu->output = openDevice(imguMediaDev_.get(), devName);\n> >>>> +\tif (!imgu->output)\n> >>>> +\t\treturn -ENODEV;\n> >>>> +\n> >>>> +\tdevName = imguName + \" viewfinder\";\n> >>>> +\timgu->viewfinder = openDevice(imguMediaDev_.get(), devName);\n> >>>> +\tif (!imgu->viewfinder)\n> >>>> +\t\treturn -ENODEV;\n> >>>> +\n> >>>> +\tdevName = imguName + \" 3a stat\";\n> >>>> +\timgu->stat = openDevice(imguMediaDev_.get(), devName);\n> >>>\n> >>> You could drop devName and call\n> >>>\n> >>> \timgu->state = openDevice(imguMediaDev_.get(), imguName + \" 3a stat\");\n> >>>\n> >>> Up to you.\n> >>\n> >> Less lines, so it's better\n> >>\n> >>>> +\tif (!imgu->stat)\n> >>>> +\t\treturn -ENODEV;\n> >>>> +\n> >>>> +\treturn 0;\n> >>>> +}\n> >>>> +\n> >>>> int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)\n> >>>> {\n> >>>> \tint ret;\n> >>>> @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)\n> >>>> \t\treturn ret;\n> >>>>\n> >>>> \tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(index);\n> >>>> -\tentity = cio2MediaDev_->getEntityByName(cio2Name);\n> >>>> -\tif (!entity) {\n> >>>> -\t\tLOG(IPU3, Error)\n> >>>> -\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> >>>> -\t\treturn -EINVAL;\n> >>>> -\t}\n> >>>> -\n> >>>> -\tcio2->output = new V4L2Device(entity);\n> >>>> -\tret = cio2->output->open();\n> >>>> -\tif (ret)\n> >>>> +\tcio2->output = openDevice(cio2MediaDev_.get(), cio2Name);\n> >>>> +\tif (!cio2->output)\n> >>>> \t\treturn ret;\n> >>>>\n> >>>> \treturn 0;","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 7A3A5600FC\n\tfor ;\n\tMon, 11 Mar 2019 09:33:27 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B96E6255;\n\tMon, 11 Mar 2019 09:33:26 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1552293206;\n\tbh=uoNsXuH0O0m6XTy4SaQ48qW8hqWIItLnOliOFBDVr4A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IuS3xA/40cFuLb70jwKtzd/PDsO8uURIGsVd4Y0H7trmxTtIHNfWX18HuRpTg0RMf\n\tHFsRPUlJqdi98OLcuejqXjVSnW6+Utuac13hkZHFSKq1kOxdMz4kgPefEw7EfMDpdD\n\tAWMH+ukA0YelN98EWfL04Vnw4A6gIAz6CsHA6lU8=","Date":"Mon, 11 Mar 2019 10:33:20 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190311083320.GC4775@pendragon.ideasonboard.com>","References":"<20190228200410.3022-1-jacopo@jmondi.org>\n\t<20190228200410.3022-4-jacopo@jmondi.org>\n\t<20190302214129.GH4682@pendragon.ideasonboard.com>\n\t<20190309205018.se6y2fjd7sgba54d@uno.localdomain>\n\t<20190310130833.GC4814@pendragon.ideasonboard.com>\n\t<20190311081041.e3qt325qf3afb765@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190311081041.e3qt325qf3afb765@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and\n\tlink ImgU devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 11 Mar 2019 08:33:27 -0000"}}]