[{"id":1085,"web_url":"https://patchwork.libcamera.org/comment/1085/","msgid":"<20190321093325.GI4615@pendragon.ideasonboard.com>","date":"2019-03-21T09:33:25","subject":"Re: [libcamera-devel] [PATCH v4 06/31] libcamera: ipu3: Initialize\n\tand configure ImgUs","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 Wed, Mar 20, 2019 at 05:30:30PM +0100, Jacopo Mondi wrote:\n> Create video devices and subdevices associated with an ImgU unit at\n> camera registration time.\n> \n> Statically assign imgu0 to the first camera and imgu1 to the second one\n> and limit support to two camera. This will have to be revised in future.\n\ns/camera/cameras/\ns/in future/in the future/\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 223 +++++++++++++++++++++++++--\n> 1 file changed, 209 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 2e351d04a784..26fc56a76eb1 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -27,6 +27,38 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPU3)\n> \n> +class ImgUDevice\n> +{\n> +public:\n> +\tImgUDevice()\n> +\t\t: imgu(nullptr), input(nullptr), output(nullptr),\n> +\t\t viewfinder(nullptr), stat(nullptr)\n> +\t{\n> +\t}\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> +\tvoid init(MediaDevice *media, unsigned int index);\n> +\n> +\tunsigned int index_;\n> +\tstd::string imguName_;\n\nAs this is the ImgUDevice class, maybe just \"name_\" ?\n\n> +\tMediaDevice *mediaDevice_;\n> +\n> +\tV4L2Subdevice *imgu;\n> +\tV4L2Device *input;\n> +\tV4L2Device *output;\n> +\tV4L2Device *viewfinder;\n> +\tV4L2Device *stat;\n\nMissing _ at the end of each variable.\n\n> +\t/* \\todo Add param video device for 3A tuning */\n> +};\n> +\n> struct CIO2Device {\n> \tCIO2Device()\n> \t\t: output(nullptr), csi2(nullptr), sensor(nullptr)\n> @@ -68,6 +100,7 @@ public:\n> \tbool match(DeviceEnumerator *enumerator);\n> \n> private:\n> +\tstatic constexpr unsigned int IPU3_IMGU_COUNT = 2;\n> \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> \n> \tclass IPU3CameraData : public CameraData\n> @@ -81,6 +114,7 @@ private:\n> \t\tvoid bufferReady(Buffer *buffer);\n> \n> \t\tCIO2Device cio2;\n> +\t\tImgUDevice *imgu;\n\nHere too.\n\n> \n> \t\tStream stream_;\n> \t};\n> @@ -92,10 +126,18 @@ private:\n> \t}\n> \n> \tint mediaBusToCIO2Format(unsigned int code);\n> +\tV4L2Device *openDevice(MediaDevice *media, const std::string &name);\n> +\tV4L2Subdevice *openSubdevice(MediaDevice *media,\n> +\t\t\t\t const std::string &name);\n> \n> \tint initCIO2(unsigned int index, CIO2Device *cio2);\n> +\tvoid deleteCIO2(CIO2Device *cio2);\n> +\n> +\tint initImgU(ImgUDevice *imgu);\n> +\n> \tvoid registerCameras();\n> \n> +\tImgUDevice imgus_[IPU3_IMGU_COUNT];\n> \tstd::shared_ptr cio2MediaDev_;\n> \tstd::shared_ptr imguMediaDev_;\n> };\n> @@ -355,11 +397,45 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> \t\treturn false;\n> \t}\n> \n> +\tif (imguMediaDev_->open()) {\n> +\t\tcio2MediaDev_->close();\n> +\t\treturn false;\n> +\t}\n> +\n> +\tif (imguMediaDev_->disableLinks())\n> +\t\tgoto error_close_mdev;\n> +\n> +\tfor (unsigned int i = 0; i < IPU3_IMGU_COUNT; ++i)\n> +\t\timgus_[i].init(imguMediaDev_.get(), i);\n> +\n> \tregisterCameras();\n> \n> \tcio2MediaDev_->close();\n> +\timguMediaDev_->close();\n> \n> \treturn true;\n> +\n> +error_close_mdev:\n\nThere's a single error label, just call it error.\n\n> +\tcio2MediaDev_->close();\n> +\timguMediaDev_->close();\n> +\n> +\treturn false;\n> +}\n> +\n> +/* ----------------------------------------------------------------------------\n> + * Helpers\n> + */\n> +\n> +/**\n> + * \\brief Initialize fields of the ImgU instance\n> + * \\param mediaDevice The ImgU instance media device\n> + * \\param index The ImgU instance index\n> + */\n> +void ImgUDevice::init(MediaDevice *mediaDevice, unsigned int index)\n> +{\n> +\tindex_ = index;\n> +\timguName_ = \"ipu3-imgu \" + std::to_string(index_);\n> +\tmediaDevice_ = mediaDevice;\n> }\n> \n> int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)\n> @@ -378,6 +454,114 @@ int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)\n> \t}\n> }\n> \n> +/**\n> + * \\brief Create and open the video device with \\a name in media device \\a media\n> + *\n> + * \\todo Make a generic helper out of this method.\n\nPlease do :-) You can create a static function in the V4L2Device class\nthat takes a media device and entity name and return a newly created\nV4L2Device. I would however keep the open() call outside of the helper\nas we'll need to refactor open/close later.\n\n> + *\n> + * \\return Pointer to the video device on success, nullptr otherwise\n> + */\n> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media,\n> +\t\t\t\t\t const std::string &name)\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> +\tV4L2Device *dev = new V4L2Device(entity);\n> +\tif (dev->open()) {\n> +\t\tdelete dev;\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\treturn dev;\n> +}\n> +\n> +/**\n> + * \\brief Create and open the subdevice with \\a name in media device \\a media\n> + *\n> + * \\todo Make a generic helper out of this method.\n\nI'd say the same here, but you use this helper in a single location, so\nI would just inline it.\n\n> + *\n> + * \\return Pointer to the subdevice on success, nullptr otherwise\n> + */\n> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media,\n> +\t\t\t\t\t\t const std::string &name)\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> +\tV4L2Subdevice *dev = new V4L2Subdevice(entity);\n> +\tif (dev->open()) {\n> +\t\tdelete dev;\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\treturn dev;\n> +}\n> +\n> +/* ----------------------------------------------------------------------------\n> + * IPU3 pipeline configuration\n> + */\n> +\n> +/**\n> + * \\brief Initialize and configure components of the ImgU instance\n> + *\n> + * Create and open the devices and subdevices in the ImgU instance.\n> + * This methods configures the ImgU instance for capture operations, and\n> + * should be called at stream configuration time.\n> + *\n> + * \\todo Expand the ImgU configuration with controls setting\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENODEV Failed to open one of the video devices or subdevices\n> + */\n> +int PipelineHandlerIPU3::initImgU(ImgUDevice *imgu)\n> +{\n> +\timgu->imgu = openSubdevice(imgu->mediaDevice_, imgu->imguName_);\n> +\tif (!imgu->imgu)\n> +\t\treturn -ENODEV;\n> +\n> +\timgu->input = openDevice(imgu->mediaDevice_,\n> +\t\t\t\t imgu->imguName_ + \" input\");\n> +\tif (!imgu->input)\n> +\t\tgoto error_delete_imgu;\n> +\n> +\timgu->output = openDevice(imgu->mediaDevice_,\n> +\t\t\t\t imgu->imguName_ + \" output\");\n> +\tif (!imgu->output)\n> +\t\tgoto error_delete_input;\n> +\n> +\timgu->viewfinder = openDevice(imgu->mediaDevice_,\n> +\t\t\t\t imgu->imguName_ + \" viewfinder\");\n> +\tif (!imgu->viewfinder)\n> +\t\tgoto error_delete_output;\n> +\n> +\timgu->stat = openDevice(imgu->mediaDevice_,\n> +\t\t\t\timgu->imguName_ + \" 3a stat\");\n> +\tif (!imgu->stat)\n> +\t\tgoto error_delete_vf;\n> +\n> +\treturn 0;\n> +\n> +error_delete_vf:\n> +\tdelete imgu->viewfinder;\n> +error_delete_output:\n> +\tdelete imgu->output;\n> +error_delete_input:\n> +\tdelete imgu->input;\n> +error_delete_imgu:\n> +\tdelete imgu->imgu;\n\nLet's simplify this by removing the need for this code. The caller can\njust propagate the error up, the match function will fail, and the imgu\ninstances will be destroyed.\n\n> +\n> +\treturn -ENODEV;\n> +}\n> +\n> /**\n> * \\brief Initialize components of the CIO2 device \\a index used by a camera\n> * \\param index The CIO2 device index\n> @@ -458,23 +642,12 @@ int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2)\n> \tif (ret)\n> \t\tgoto error_delete_csi2;\n> \n> -\tentity = cio2MediaDev_->getEntityByName(cio2Name);\n> -\tif (!entity) {\n> -\t\tLOG(IPU3, Error)\n> -\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> -\t\tret = -EINVAL;\n> +\tcio2->output = openDevice(cio2MediaDev_.get(), cio2Name);\n> +\tif (!cio2->output)\n> \t\tgoto error_delete_csi2;\n> -\t}\n> -\n> -\tcio2->output = new V4L2Device(entity);\n> -\tret = cio2->output->open();\n> -\tif (ret)\n> -\t\tgoto error_delete_output;\n> \n> \treturn 0;\n> \n> -error_delete_output:\n> -\tdelete cio2->output;\n> error_delete_csi2:\n> \tdelete cio2->csi2;\n> error_delete_sensor:\n> @@ -483,6 +656,16 @@ error_delete_sensor:\n> \treturn ret;\n> }\n> \n> +/**\n> + * \\brief Delete all devices associated with a CIO2 unit\n> + */\n> +void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2)\n> +{\n> +\tdelete cio2->output;\n> +\tdelete cio2->csi2;\n> +\tdelete cio2->sensor;\n> +}\n\nI don't think this is needed for the reason explained above.\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> @@ -495,7 +678,7 @@ void PipelineHandlerIPU3::registerCameras()\n> \t * image sensor is connected to it.\n> \t */\n> \tunsigned int numCameras = 0;\n> -\tfor (unsigned int id = 0; id < 4; ++id) {\n> +\tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> \t\tstd::unique_ptr data =\n> \t\t\tutils::make_unique(this);\n> \t\tstd::set streams{ &data->stream_ };\n> @@ -506,6 +689,18 @@ void PipelineHandlerIPU3::registerCameras()\n> \t\tif (ret)\n> \t\t\tcontinue;\n> \n> +\t\t/**\n> +\t\t * \\todo Dynamically assign ImgU devices; as of now, limit\n> +\t\t * support to two cameras only, and assign imgu0 to the first\n> +\t\t * one and imgu1 to the second.\n> +\t\t */\n> +\t\tdata->imgu = &imgus_[numCameras];\n> +\t\tret = initImgU(data->imgu);\n\nThe imgus are separate from the cameras, please initialize them before\nthis loop, and return an error if initialization fails.\n\n> +\t\tif (ret) {\n> +\t\t\tdeleteCIO2(cio2);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> \t\tstd::string cameraName = cio2->sensor->deviceName() + \" \"\n> \t\t\t\t + std::to_string(id);\n> \t\tstd::shared_ptr camera = Camera::create(this,","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 E6C7B600F9\n\tfor ;\n\tThu, 21 Mar 2019 10:33:37 +0100 (CET)","from pendragon.ideasonboard.com (30.net042126252.t-com.ne.jp\n\t[42.126.252.30])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8070B23A;\n\tThu, 21 Mar 2019 10:33:36 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1553160817;\n\tbh=Wj3slVTPTVhn3q/3mXQH1kB9AucQQ3wOD2kogJOFDUM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VAfT4TqRJ40CE3TqsLrjJnhuPo4aFZO/mgtR5efl/wGIjYBsvsqDlYAXavMzeE+np\n\tJ8xWx0wptEpP6y28okjVqXTPuWpB18cd8zAtV65h2X+sVCoX+AurH4INlZs0A99Ngf\n\tdYbjhOJoEY4fJe/vqAx7SrhIs1BCpuePSYYF6P40=","Date":"Thu, 21 Mar 2019 11:33:25 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190321093325.GI4615@pendragon.ideasonboard.com>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-7-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190320163055.22056-7-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 06/31] libcamera: ipu3: Initialize\n\tand configure ImgUs","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, 21 Mar 2019 09:33:38 -0000"}},{"id":1120,"web_url":"https://patchwork.libcamera.org/comment/1120/","msgid":"<20190325094306.amyivpnd55q2i23f@uno.localdomain>","date":"2019-03-25T09:43:06","subject":"Re: [libcamera-devel] [PATCH v4 06/31] libcamera: ipu3: Initialize\n\tand configure ImgUs","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Mar 21, 2019 at 11:33:25AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Mar 20, 2019 at 05:30:30PM +0100, Jacopo Mondi wrote:\n> > Create video devices and subdevices associated with an ImgU unit at\n> > camera registration time.\n> >\n> > Statically assign imgu0 to the first camera and imgu1 to the second one\n> > and limit support to two camera. This will have to be revised in future.\n>\n> s/camera/cameras/\n> s/in future/in the future/\n>\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 223 +++++++++++++++++++++++++--\n> > 1 file changed, 209 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 2e351d04a784..26fc56a76eb1 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -27,6 +27,38 @@ namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(IPU3)\n> >\n> > +class ImgUDevice\n> > +{\n> > +public:\n> > +\tImgUDevice()\n> > +\t\t: imgu(nullptr), input(nullptr), output(nullptr),\n> > +\t\t viewfinder(nullptr), stat(nullptr)\n> > +\t{\n> > +\t}\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> > +\tvoid init(MediaDevice *media, unsigned int index);\n> > +\n> > +\tunsigned int index_;\n> > +\tstd::string imguName_;\n>\n> As this is the ImgUDevice class, maybe just \"name_\" ?\n>\n> > +\tMediaDevice *mediaDevice_;\n> > +\n> > +\tV4L2Subdevice *imgu;\n> > +\tV4L2Device *input;\n> > +\tV4L2Device *output;\n> > +\tV4L2Device *viewfinder;\n> > +\tV4L2Device *stat;\n>\n> Missing _ at the end of each variable.\n>\n> > +\t/* \\todo Add param video device for 3A tuning */\n> > +};\n> > +\n> > struct CIO2Device {\n> > \tCIO2Device()\n> > \t\t: output(nullptr), csi2(nullptr), sensor(nullptr)\n> > @@ -68,6 +100,7 @@ public:\n> > \tbool match(DeviceEnumerator *enumerator);\n> >\n> > private:\n> > +\tstatic constexpr unsigned int IPU3_IMGU_COUNT = 2;\n> > \tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> >\n> > \tclass IPU3CameraData : public CameraData\n> > @@ -81,6 +114,7 @@ private:\n> > \t\tvoid bufferReady(Buffer *buffer);\n> >\n> > \t\tCIO2Device cio2;\n> > +\t\tImgUDevice *imgu;\n>\n> Here too.\n>\n> >\n> > \t\tStream stream_;\n> > \t};\n> > @@ -92,10 +126,18 @@ private:\n> > \t}\n> >\n> > \tint mediaBusToCIO2Format(unsigned int code);\n> > +\tV4L2Device *openDevice(MediaDevice *media, const std::string &name);\n> > +\tV4L2Subdevice *openSubdevice(MediaDevice *media,\n> > +\t\t\t\t const std::string &name);\n> >\n> > \tint initCIO2(unsigned int index, CIO2Device *cio2);\n> > +\tvoid deleteCIO2(CIO2Device *cio2);\n> > +\n> > +\tint initImgU(ImgUDevice *imgu);\n> > +\n> > \tvoid registerCameras();\n> >\n> > +\tImgUDevice imgus_[IPU3_IMGU_COUNT];\n> > \tstd::shared_ptr cio2MediaDev_;\n> > \tstd::shared_ptr imguMediaDev_;\n> > };\n> > @@ -355,11 +397,45 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > \t\treturn false;\n> > \t}\n> >\n> > +\tif (imguMediaDev_->open()) {\n> > +\t\tcio2MediaDev_->close();\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tif (imguMediaDev_->disableLinks())\n> > +\t\tgoto error_close_mdev;\n> > +\n> > +\tfor (unsigned int i = 0; i < IPU3_IMGU_COUNT; ++i)\n> > +\t\timgus_[i].init(imguMediaDev_.get(), i);\n> > +\n> > \tregisterCameras();\n> >\n> > \tcio2MediaDev_->close();\n> > +\timguMediaDev_->close();\n> >\n> > \treturn true;\n> > +\n> > +error_close_mdev:\n>\n> There's a single error label, just call it error.\n>\n> > +\tcio2MediaDev_->close();\n> > +\timguMediaDev_->close();\n> > +\n> > +\treturn false;\n> > +}\n> > +\n> > +/* ----------------------------------------------------------------------------\n> > + * Helpers\n> > + */\n> > +\n> > +/**\n> > + * \\brief Initialize fields of the ImgU instance\n> > + * \\param mediaDevice The ImgU instance media device\n> > + * \\param index The ImgU instance index\n> > + */\n> > +void ImgUDevice::init(MediaDevice *mediaDevice, unsigned int index)\n> > +{\n> > +\tindex_ = index;\n> > +\timguName_ = \"ipu3-imgu \" + std::to_string(index_);\n> > +\tmediaDevice_ = mediaDevice;\n> > }\n> >\n> > int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)\n> > @@ -378,6 +454,114 @@ int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)\n> > \t}\n> > }\n> >\n> > +/**\n> > + * \\brief Create and open the video device with \\a name in media device \\a media\n> > + *\n> > + * \\todo Make a generic helper out of this method.\n>\n> Please do :-) You can create a static function in the V4L2Device class\n> that takes a media device and entity name and return a newly created\n> V4L2Device. I would however keep the open() call outside of the helper\n> as we'll need to refactor open/close later.\n>\n> > + *\n> > + * \\return Pointer to the video device on success, nullptr otherwise\n> > + */\n> > +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media,\n> > +\t\t\t\t\t const std::string &name)\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> > +\tV4L2Device *dev = new V4L2Device(entity);\n> > +\tif (dev->open()) {\n> > +\t\tdelete dev;\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\treturn dev;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Create and open the subdevice with \\a name in media device \\a media\n> > + *\n> > + * \\todo Make a generic helper out of this method.\n>\n> I'd say the same here, but you use this helper in a single location, so\n> I would just inline it.\n>\n> > + *\n> > + * \\return Pointer to the subdevice on success, nullptr otherwise\n> > + */\n> > +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media,\n> > +\t\t\t\t\t\t const std::string &name)\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> > +\tV4L2Subdevice *dev = new V4L2Subdevice(entity);\n> > +\tif (dev->open()) {\n> > +\t\tdelete dev;\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\treturn dev;\n> > +}\n> > +\n> > +/* ----------------------------------------------------------------------------\n> > + * IPU3 pipeline configuration\n> > + */\n> > +\n> > +/**\n> > + * \\brief Initialize and configure components of the ImgU instance\n> > + *\n> > + * Create and open the devices and subdevices in the ImgU instance.\n> > + * This methods configures the ImgU instance for capture operations, and\n> > + * should be called at stream configuration time.\n> > + *\n> > + * \\todo Expand the ImgU configuration with controls setting\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENODEV Failed to open one of the video devices or subdevices\n> > + */\n> > +int PipelineHandlerIPU3::initImgU(ImgUDevice *imgu)\n> > +{\n> > +\timgu->imgu = openSubdevice(imgu->mediaDevice_, imgu->imguName_);\n> > +\tif (!imgu->imgu)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\timgu->input = openDevice(imgu->mediaDevice_,\n> > +\t\t\t\t imgu->imguName_ + \" input\");\n> > +\tif (!imgu->input)\n> > +\t\tgoto error_delete_imgu;\n> > +\n> > +\timgu->output = openDevice(imgu->mediaDevice_,\n> > +\t\t\t\t imgu->imguName_ + \" output\");\n> > +\tif (!imgu->output)\n> > +\t\tgoto error_delete_input;\n> > +\n> > +\timgu->viewfinder = openDevice(imgu->mediaDevice_,\n> > +\t\t\t\t imgu->imguName_ + \" viewfinder\");\n> > +\tif (!imgu->viewfinder)\n> > +\t\tgoto error_delete_output;\n> > +\n> > +\timgu->stat = openDevice(imgu->mediaDevice_,\n> > +\t\t\t\timgu->imguName_ + \" 3a stat\");\n> > +\tif (!imgu->stat)\n> > +\t\tgoto error_delete_vf;\n> > +\n> > +\treturn 0;\n> > +\n> > +error_delete_vf:\n> > +\tdelete imgu->viewfinder;\n> > +error_delete_output:\n> > +\tdelete imgu->output;\n> > +error_delete_input:\n> > +\tdelete imgu->input;\n> > +error_delete_imgu:\n> > +\tdelete imgu->imgu;\n>\n> Let's simplify this by removing the need for this code. The caller can\n> just propagate the error up, the match function will fail, and the imgu\n> instances will be destroyed.\n>\n\nFrom your comment I get that we can fail when intializing the ImgUs,\npropagate the error up and make PipelineHandler::match() fail, which\ndeletes the pipeline handler instance and consequentially the ImgU\ninstances.\n\nJust to make sure we're on the same page:\n- the pipeline manager instances are destroyed at\n CameraManager::stop() time only\n- if PipelineHandler::match() fails, the pipeline handler is not\n destroyed.\n- It is not enough to return an error from the\n PipelineHandler::match() function to have the ImgU initialized instances\n (if any) deleted\n\nIt is very likely we can wait for CameraManager::stop() and have the\nImgU instances destroyed there, but I wonder if it would not be worth\ncleaning them up as soon as we fail. The easies way would be\ninstanciate them at initialization time, and delete the newly created\ninstances if one of the two initializations fail, or associate them\nwith camera data as we do for CIO2Device instances, but then your\ncomment to initialize ImgUs separately from camera creation won't\napply anymore.\n\nMy proposal would be something like:\n\nclass ImgUDevice\n{\n ...\n\n ImgUDevice *imgu0_;\n ImgUDevice *imgu1_;\n}\n\nPipelineHandler::match()\n{\n\n\n ret = registerCameras();\n if (ret) {\n closeMediaDevices\n return false;\n }\n}\n\nPipelineHandler::registerCameras()\n{\n\n imgu0_ = new ImgUDevice();\n ret = imgu0_->init();\n if (ret) {\n delete imgu0_;\n return -ENODEV;\n }\n\n imgu1_ = new ImgUDevice();\n ret = imgu1_->init();\n if (ret) {\n delete imgu0_;\n delete imgu1_;\n return -ENODEV;\n }\n\n for (i = 0 ; i < 4; ++i) {\n ret = cio2::init();\n if (ret)\n continue;\n\n registerCamera();\n ++numCameras;\n }\n\n if (!numCameras) {\n delete imgu0_;\n delete imgu1_;\n\n return -ENODEV;\n }\n\n return 0;\n}\n\nOpinions?\n\nThanks\n j\n\n> > +\n> > +\treturn -ENODEV;\n> > +}\n> > +\n> > /**\n> > * \\brief Initialize components of the CIO2 device \\a index used by a camera\n> > * \\param index The CIO2 device index\n> > @@ -458,23 +642,12 @@ int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2)\n> > \tif (ret)\n> > \t\tgoto error_delete_csi2;\n> >\n> > -\tentity = cio2MediaDev_->getEntityByName(cio2Name);\n> > -\tif (!entity) {\n> > -\t\tLOG(IPU3, Error)\n> > -\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> > -\t\tret = -EINVAL;\n> > +\tcio2->output = openDevice(cio2MediaDev_.get(), cio2Name);\n> > +\tif (!cio2->output)\n> > \t\tgoto error_delete_csi2;\n> > -\t}\n> > -\n> > -\tcio2->output = new V4L2Device(entity);\n> > -\tret = cio2->output->open();\n> > -\tif (ret)\n> > -\t\tgoto error_delete_output;\n> >\n> > \treturn 0;\n> >\n> > -error_delete_output:\n> > -\tdelete cio2->output;\n> > error_delete_csi2:\n> > \tdelete cio2->csi2;\n> > error_delete_sensor:\n> > @@ -483,6 +656,16 @@ error_delete_sensor:\n> > \treturn ret;\n> > }\n> >\n> > +/**\n> > + * \\brief Delete all devices associated with a CIO2 unit\n> > + */\n> > +void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2)\n> > +{\n> > +\tdelete cio2->output;\n> > +\tdelete cio2->csi2;\n> > +\tdelete cio2->sensor;\n> > +}\n>\n> I don't think this is needed for the reason explained above.\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> > @@ -495,7 +678,7 @@ void PipelineHandlerIPU3::registerCameras()\n> > \t * image sensor is connected to it.\n> > \t */\n> > \tunsigned int numCameras = 0;\n> > -\tfor (unsigned int id = 0; id < 4; ++id) {\n> > +\tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> > \t\tstd::unique_ptr data =\n> > \t\t\tutils::make_unique(this);\n> > \t\tstd::set streams{ &data->stream_ };\n> > @@ -506,6 +689,18 @@ void PipelineHandlerIPU3::registerCameras()\n> > \t\tif (ret)\n> > \t\t\tcontinue;\n> >\n> > +\t\t/**\n> > +\t\t * \\todo Dynamically assign ImgU devices; as of now, limit\n> > +\t\t * support to two cameras only, and assign imgu0 to the first\n> > +\t\t * one and imgu1 to the second.\n> > +\t\t */\n> > +\t\tdata->imgu = &imgus_[numCameras];\n> > +\t\tret = initImgU(data->imgu);\n>\n> The imgus are separate from the cameras, please initialize them before\n> this loop, and return an error if initialization fails.\n>\n> > +\t\tif (ret) {\n> > +\t\t\tdeleteCIO2(cio2);\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > \t\tstd::string cameraName = cio2->sensor->deviceName() + \" \"\n> > \t\t\t\t + std::to_string(id);\n> > \t\tstd::shared_ptr camera = Camera::create(this,\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 76F57600F9\n\tfor ;\n\tMon, 25 Mar 2019 10:42:26 +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 C3D526000A;\n\tMon, 25 Mar 2019 09:42:25 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 25 Mar 2019 10:43:06 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190325094306.amyivpnd55q2i23f@uno.localdomain>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-7-jacopo@jmondi.org>\n\t<20190321093325.GI4615@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"wckts7ijj2wwiu3w\"","Content-Disposition":"inline","In-Reply-To":"<20190321093325.GI4615@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 06/31] libcamera: ipu3: Initialize\n\tand configure ImgUs","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, 25 Mar 2019 09:42:26 -0000"}},{"id":1121,"web_url":"https://patchwork.libcamera.org/comment/1121/","msgid":"<20190325095233.fzl7vgvuz3qakxip@uno.localdomain>","date":"2019-03-25T09:52:33","subject":"Re: [libcamera-devel] [PATCH v4 06/31] libcamera: ipu3: Initialize\n\tand configure ImgUs","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Ups, self correction...\n\nOn Mon, Mar 25, 2019 at 10:43:06AM +0100, Jacopo Mondi wrote:\n> Hi Laurent,\n>\n\n[snip]\n\n> > > +\treturn 0;\n> > > +\n> > > +error_delete_vf:\n> > > +\tdelete imgu->viewfinder;\n> > > +error_delete_output:\n> > > +\tdelete imgu->output;\n> > > +error_delete_input:\n> > > +\tdelete imgu->input;\n> > > +error_delete_imgu:\n> > > +\tdelete imgu->imgu;\n> >\n> > Let's simplify this by removing the need for this code. The caller can\n> > just propagate the error up, the match function will fail, and the imgu\n> > instances will be destroyed.\n> >\n>\n> From your comment I get that we can fail when intializing the ImgUs,\n> propagate the error up and make PipelineHandler::match() fail, which\n> deletes the pipeline handler instance and consequentially the ImgU\n> instances.\n>\n> Just to make sure we're on the same page:\n> - the pipeline manager instances are destroyed at\n> CameraManager::stop() time only\n> - if PipelineHandler::match() fails, the pipeline handler is not\n> destroyed.\n\nIt is, as it is created as shared pointer with a single reference by\nPipelineHandlerFactory::create().\n\nSorry for the noise, I had missed that the one created at match() time\nis the single reference to the newly created pipeline handler.\n\nThanks for pointing me to that.\n\n\n> - It is not enough to return an error from the\n> PipelineHandler::match() function to have the ImgU initialized instances\n> (if any) deleted\n>\n> It is very likely we can wait for CameraManager::stop() and have the\n> ImgU instances destroyed there, but I wonder if it would not be worth\n> cleaning them up as soon as we fail. The easies way would be\n> instanciate them at initialization time, and delete the newly created\n> instances if one of the two initializations fail, or associate them\n> with camera data as we do for CIO2Device instances, but then your\n> comment to initialize ImgUs separately from camera creation won't\n> apply anymore.\n>\n> My proposal would be something like:\n>\n> class ImgUDevice\n> {\n> ...\n>\n> ImgUDevice *imgu0_;\n> ImgUDevice *imgu1_;\n> }\n>\n> PipelineHandler::match()\n> {\n>\n>\n> ret = registerCameras();\n> if (ret) {\n> closeMediaDevices\n> return false;\n> }\n> }\n>\n> PipelineHandler::registerCameras()\n> {\n>\n> imgu0_ = new ImgUDevice();\n> ret = imgu0_->init();\n> if (ret) {\n> delete imgu0_;\n> return -ENODEV;\n> }\n>\n> imgu1_ = new ImgUDevice();\n> ret = imgu1_->init();\n> if (ret) {\n> delete imgu0_;\n> delete imgu1_;\n> return -ENODEV;\n> }\n>\n> for (i = 0 ; i < 4; ++i) {\n> ret = cio2::init();\n> if (ret)\n> continue;\n>\n> registerCamera();\n> ++numCameras;\n> }\n>\n> if (!numCameras) {\n> delete imgu0_;\n> delete imgu1_;\n>\n> return -ENODEV;\n> }\n>\n> return 0;\n> }\n>\n> Opinions?\n>\n> Thanks\n> j\n>\n> > > +\n> > > +\treturn -ENODEV;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Initialize components of the CIO2 device \\a index used by a camera\n> > > * \\param index The CIO2 device index\n> > > @@ -458,23 +642,12 @@ int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2)\n> > > \tif (ret)\n> > > \t\tgoto error_delete_csi2;\n> > >\n> > > -\tentity = cio2MediaDev_->getEntityByName(cio2Name);\n> > > -\tif (!entity) {\n> > > -\t\tLOG(IPU3, Error)\n> > > -\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> > > -\t\tret = -EINVAL;\n> > > +\tcio2->output = openDevice(cio2MediaDev_.get(), cio2Name);\n> > > +\tif (!cio2->output)\n> > > \t\tgoto error_delete_csi2;\n> > > -\t}\n> > > -\n> > > -\tcio2->output = new V4L2Device(entity);\n> > > -\tret = cio2->output->open();\n> > > -\tif (ret)\n> > > -\t\tgoto error_delete_output;\n> > >\n> > > \treturn 0;\n> > >\n> > > -error_delete_output:\n> > > -\tdelete cio2->output;\n> > > error_delete_csi2:\n> > > \tdelete cio2->csi2;\n> > > error_delete_sensor:\n> > > @@ -483,6 +656,16 @@ error_delete_sensor:\n> > > \treturn ret;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Delete all devices associated with a CIO2 unit\n> > > + */\n> > > +void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2)\n> > > +{\n> > > +\tdelete cio2->output;\n> > > +\tdelete cio2->csi2;\n> > > +\tdelete cio2->sensor;\n> > > +}\n> >\n> > I don't think this is needed for the reason explained above.\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> > > @@ -495,7 +678,7 @@ void PipelineHandlerIPU3::registerCameras()\n> > > \t * image sensor is connected to it.\n> > > \t */\n> > > \tunsigned int numCameras = 0;\n> > > -\tfor (unsigned int id = 0; id < 4; ++id) {\n> > > +\tfor (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {\n> > > \t\tstd::unique_ptr data =\n> > > \t\t\tutils::make_unique(this);\n> > > \t\tstd::set streams{ &data->stream_ };\n> > > @@ -506,6 +689,18 @@ void PipelineHandlerIPU3::registerCameras()\n> > > \t\tif (ret)\n> > > \t\t\tcontinue;\n> > >\n> > > +\t\t/**\n> > > +\t\t * \\todo Dynamically assign ImgU devices; as of now, limit\n> > > +\t\t * support to two cameras only, and assign imgu0 to the first\n> > > +\t\t * one and imgu1 to the second.\n> > > +\t\t */\n> > > +\t\tdata->imgu = &imgus_[numCameras];\n> > > +\t\tret = initImgU(data->imgu);\n> >\n> > The imgus are separate from the cameras, please initialize them before\n> > this loop, and return an error if initialization fails.\n> >\n> > > +\t\tif (ret) {\n> > > +\t\t\tdeleteCIO2(cio2);\n> > > +\t\t\tcontinue;\n> > > +\t\t}\n> > > +\n> > > \t\tstd::string cameraName = cio2->sensor->deviceName() + \" \"\n> > > \t\t\t\t + std::to_string(id);\n> > > \t\tstd::shared_ptr camera = Camera::create(this,\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n\n\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 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 E59E4600F9\n\tfor ;\n\tMon, 25 Mar 2019 10:51:52 +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 5A7C424000F;\n\tMon, 25 Mar 2019 09:51:52 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 25 Mar 2019 10:52:33 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190325095233.fzl7vgvuz3qakxip@uno.localdomain>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-7-jacopo@jmondi.org>\n\t<20190321093325.GI4615@pendragon.ideasonboard.com>\n\t<20190325094306.amyivpnd55q2i23f@uno.localdomain>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"dkfc7w2n7rb2zypa\"","Content-Disposition":"inline","In-Reply-To":"<20190325094306.amyivpnd55q2i23f@uno.localdomain>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 06/31] libcamera: ipu3: Initialize\n\tand configure ImgUs","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, 25 Mar 2019 09:51:53 -0000"}}]