[{"id":1107,"web_url":"https://patchwork.libcamera.org/comment/1107/","msgid":"<20190323130934.GD4587@pendragon.ideasonboard.com>","date":"2019-03-23T13:09:34","subject":"Re: [libcamera-devel] [PATCH v4 15/31] RFC: libcamera: ipu3: Enable\n\tImgU media links","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:39PM +0100, Jacopo Mondi wrote:\n> As the lenghty comment reports, link enable/disable is not trivial, as\n> links in one ImgU instance interfere with capture operations in the\n> other one. As I struggle to find where to disable links selectively,\n> as of now reset the media graph and enable the required links at\n> streamConfiguration time.\n\nDo we need this additional complexity ? Given that we currently have to\nlink all but the params video nodes, couldn't you just disable the\nparams link and enable all the other ones at match time, without\ntouching links when configuring the streams ?\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 110 +++++++++++++++++++++++++++\n> 1 file changed, 110 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index ed2409907cf0..43fa0e4a7f9d 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -52,6 +52,10 @@ public:\n> \t}\n> \n> \tvoid init(MediaDevice *media, unsigned int index);\n> +\tint linkSetup(const std::string &source, unsigned int sourcePad,\n> +\t\t const std::string &sink, unsigned int sinkPad,\n> +\t\t bool enable);\n> +\tint enableLinks(bool enable);\n> \n> \tunsigned int index_;\n> \tstd::string imguName_;\n> @@ -264,6 +268,14 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\t/*\n> +\t * \\todo: Enable links selectively based on the requested streams.\n> +\t * As of now, enable all links unconditionally.\n> +\t */\n> +\tret = data->imgu->enableLinks(true);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \t/*\n> \t * Pass the requested output image size to the sensor and get back the\n> \t * adjusted one to be propagated to the CIO2 device and to the ImgU\n> @@ -589,6 +601,30 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> \t\treturn false;\n> \t}\n> \n> +\t/*\n> +\t * FIXME: enabled links in one ImgU instance interfere with capture\n> +\t * operations on the other one. This can be easily triggered by\n> +\t * capturing from one camera and then trying to capture from the other\n> +\t * one right after, without disabling media links in the media graph\n> +\t * first.\n> +\t *\n> +\t * The tricky part here is where to disable links on the ImgU instance\n> +\t * which is currently not in use:\n> +\t * 1) Link enable/disable cannot be done at start/stop time as video\n> +\t * devices needs to be linked first before format can be configured on\n> +\t * them.\n> +\t * 2) As link enable has to be done at the least in configureStreams,\n> +\t * before configuring formats, the only place where to disable links\n> +\t * would be 'stop()', but the Camera class state machine allows\n> +\t * start()<->stop() sequences without any streamConfiguration() in\n> +\t * between.\n> +\t *\n> +\t * As of now, disable all links in the media graph at 'match()' time,\n> +\t * to allow testing different cameras in different test applications\n> +\t * runs. A test application that would use two distinct cameras without\n> +\t * going through a library teardown->match() sequence would fail\n> +\t * at the moment.\n> +\t */\n> \tif (imguMediaDev_->disableLinks())\n> \t\tgoto error_close_mdev;\n> \n> @@ -625,6 +661,80 @@ void ImgUDevice::init(MediaDevice *mediaDevice, unsigned int index)\n> \tmediaDevice_ = mediaDevice;\n> }\n> \n> +/**\n> + * \\brief Enable or disable a single link on the ImgU instance\n> + *\n> + * This method assumes the media device associated with the ImgU instance\n> + * is open.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int ImgUDevice::linkSetup(const std::string &source, unsigned int sourcePad,\n> +\t\t\t const std::string &sink, unsigned int sinkPad,\n> +\t\t\t bool enable)\n> +{\n> +\tMediaLink *link = mediaDevice_->link(source, sourcePad, sink, sinkPad);\n> +\tif (!link) {\n> +\t\tLOG(IPU3, Error)\n> +\t\t\t<< \"Failed to get link: '\" << source << \"':\"\n> +\t\t\t<< sourcePad << \" -> '\" << sink << \"':\" << sinkPad;\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\treturn link->setEnabled(enable);\n> +}\n> +\n> +/**\n> + * \\brief Enable or disable all media links in the ImgU instance to prepare\n> + * for capture operations\n> + *\n> + * \\todo This method will probably be removed or changed once links will be\n> + * enabled or disabled selectively.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int ImgUDevice::enableLinks(bool enable)\n> +{\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> +\tint ret;\n> +\n> +\t/* \\todo Establish rules to handle media devices open/close. */\n> +\tret = mediaDevice_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = linkSetup(inputName, 0, imguName_, PAD_INPUT, enable);\n> +\tif (ret) {\n> +\t\tmediaDevice_->close();\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = linkSetup(imguName_, PAD_OUTPUT, outputName, 0, enable);\n> +\tif (ret) {\n> +\t\tmediaDevice_->close();\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = linkSetup(imguName_, PAD_VF, viewfinderName, 0, enable);\n> +\tif (ret) {\n> +\t\tmediaDevice_->close();\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = linkSetup(imguName_, PAD_STAT, statName, 0, enable);\n> +\tif (ret) {\n> +\t\tmediaDevice_->close();\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tmediaDevice_->close();\n> +\n> +\treturn 0;\n> +}\n> +\n> int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)\n> {\n> \tswitch (code) {","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85E55610B6\n\tfor ;\n\tSat, 23 Mar 2019 14:09:47 +0100 (CET)","from pendragon.ideasonboard.com\n\t(p5269001-ipngn11702marunouchi.tokyo.ocn.ne.jp [114.158.195.1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DC42949;\n\tSat, 23 Mar 2019 14:09:45 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1553346587;\n\tbh=7CISO6d4Eqks4HTT4sP1OQZ5D8uv8Nvw7sptVjJBrfE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wZ/lSkgOWUZK6Q+DxzkfnPQQ11dRVakVC58Qp155mYCcdWV30wo8tmYksPxP+cIKP\n\tdffe8+2z60DzEJQULcEcz/XaQfcpQ+m0IK0I5LgGaA16ye3LL1mHS7q84aLqnBSNS/\n\tRT7+cUGLaqinGCfjxqWCFtk8MQKRJCYRFJQ/Zebw=","Date":"Sat, 23 Mar 2019 15:09:34 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190323130934.GD4587@pendragon.ideasonboard.com>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-16-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190320163055.22056-16-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 15/31] RFC: libcamera: ipu3: Enable\n\tImgU media links","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, 23 Mar 2019 13:09:47 -0000"}},{"id":1112,"web_url":"https://patchwork.libcamera.org/comment/1112/","msgid":"<20190323135142.rf2paagzxaoxnrsj@uno.localdomain>","date":"2019-03-23T13:51:42","subject":"Re: [libcamera-devel] [PATCH v4 15/31] RFC: libcamera: ipu3: Enable\n\tImgU media links","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 23, 2019 at 03:09:34PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Mar 20, 2019 at 05:30:39PM +0100, Jacopo Mondi wrote:\n> > As the lenghty comment reports, link enable/disable is not trivial, as\n> > links in one ImgU instance interfere with capture operations in the\n> > other one. As I struggle to find where to disable links selectively,\n> > as of now reset the media graph and enable the required links at\n> > streamConfiguration time.\n>\n> Do we need this additional complexity ? Given that we currently have to\n> link all but the params video nodes, couldn't you just disable the\n> params link and enable all the other ones at match time, without\n> touching links when configuring the streams ?\n\nEnable links on both the ImgU instances you mean?\n\nAs I tried to explain in the comment, this is not really possible,\nbecause if you enable links on the ImgU instance, the system expects\nframes to be queued to its input, otherwise the processing on the the\nother instance, which is in use, stall.\n\nThis can be easily triggered by enabling links on imgu1 in the\nipu3-process.sh script, which uses imgu0.\n\nThanks\n j\n\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 110 +++++++++++++++++++++++++++\n> > 1 file changed, 110 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index ed2409907cf0..43fa0e4a7f9d 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -52,6 +52,10 @@ public:\n> > \t}\n> >\n> > \tvoid init(MediaDevice *media, unsigned int index);\n> > +\tint linkSetup(const std::string &source, unsigned int sourcePad,\n> > +\t\t const std::string &sink, unsigned int sinkPad,\n> > +\t\t bool enable);\n> > +\tint enableLinks(bool enable);\n> >\n> > \tunsigned int index_;\n> > \tstd::string imguName_;\n> > @@ -264,6 +268,14 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > +\t/*\n> > +\t * \\todo: Enable links selectively based on the requested streams.\n> > +\t * As of now, enable all links unconditionally.\n> > +\t */\n> > +\tret = data->imgu->enableLinks(true);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > \t/*\n> > \t * Pass the requested output image size to the sensor and get back the\n> > \t * adjusted one to be propagated to the CIO2 device and to the ImgU\n> > @@ -589,6 +601,30 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > \t\treturn false;\n> > \t}\n> >\n> > +\t/*\n> > +\t * FIXME: enabled links in one ImgU instance interfere with capture\n> > +\t * operations on the other one. This can be easily triggered by\n> > +\t * capturing from one camera and then trying to capture from the other\n> > +\t * one right after, without disabling media links in the media graph\n> > +\t * first.\n> > +\t *\n> > +\t * The tricky part here is where to disable links on the ImgU instance\n> > +\t * which is currently not in use:\n> > +\t * 1) Link enable/disable cannot be done at start/stop time as video\n> > +\t * devices needs to be linked first before format can be configured on\n> > +\t * them.\n> > +\t * 2) As link enable has to be done at the least in configureStreams,\n> > +\t * before configuring formats, the only place where to disable links\n> > +\t * would be 'stop()', but the Camera class state machine allows\n> > +\t * start()<->stop() sequences without any streamConfiguration() in\n> > +\t * between.\n> > +\t *\n> > +\t * As of now, disable all links in the media graph at 'match()' time,\n> > +\t * to allow testing different cameras in different test applications\n> > +\t * runs. A test application that would use two distinct cameras without\n> > +\t * going through a library teardown->match() sequence would fail\n> > +\t * at the moment.\n> > +\t */\n> > \tif (imguMediaDev_->disableLinks())\n> > \t\tgoto error_close_mdev;\n> >\n> > @@ -625,6 +661,80 @@ void ImgUDevice::init(MediaDevice *mediaDevice, unsigned int index)\n> > \tmediaDevice_ = mediaDevice;\n> > }\n> >\n> > +/**\n> > + * \\brief Enable or disable a single link on the ImgU instance\n> > + *\n> > + * This method assumes the media device associated with the ImgU instance\n> > + * is open.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int ImgUDevice::linkSetup(const std::string &source, unsigned int sourcePad,\n> > +\t\t\t const std::string &sink, unsigned int sinkPad,\n> > +\t\t\t bool enable)\n> > +{\n> > +\tMediaLink *link = mediaDevice_->link(source, sourcePad, sink, sinkPad);\n> > +\tif (!link) {\n> > +\t\tLOG(IPU3, Error)\n> > +\t\t\t<< \"Failed to get link: '\" << source << \"':\"\n> > +\t\t\t<< sourcePad << \" -> '\" << sink << \"':\" << sinkPad;\n> > +\t\treturn -ENODEV;\n> > +\t}\n> > +\n> > +\treturn link->setEnabled(enable);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Enable or disable all media links in the ImgU instance to prepare\n> > + * for capture operations\n> > + *\n> > + * \\todo This method will probably be removed or changed once links will be\n> > + * enabled or disabled selectively.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int ImgUDevice::enableLinks(bool enable)\n> > +{\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> > +\tint ret;\n> > +\n> > +\t/* \\todo Establish rules to handle media devices open/close. */\n> > +\tret = mediaDevice_->open();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = linkSetup(inputName, 0, imguName_, PAD_INPUT, enable);\n> > +\tif (ret) {\n> > +\t\tmediaDevice_->close();\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tret = linkSetup(imguName_, PAD_OUTPUT, outputName, 0, enable);\n> > +\tif (ret) {\n> > +\t\tmediaDevice_->close();\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tret = linkSetup(imguName_, PAD_VF, viewfinderName, 0, enable);\n> > +\tif (ret) {\n> > +\t\tmediaDevice_->close();\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tret = linkSetup(imguName_, PAD_STAT, statName, 0, enable);\n> > +\tif (ret) {\n> > +\t\tmediaDevice_->close();\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tmediaDevice_->close();\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)\n> > {\n> > \tswitch (code) {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CAAE7610B6\n\tfor ;\n\tSat, 23 Mar 2019 14:51:01 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 4942AC0005;\n\tSat, 23 Mar 2019 13:51:01 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sat, 23 Mar 2019 14:51:42 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190323135142.rf2paagzxaoxnrsj@uno.localdomain>","References":"<20190320163055.22056-1-jacopo@jmondi.org>\n\t<20190320163055.22056-16-jacopo@jmondi.org>\n\t<20190323130934.GD4587@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"yysrlyzuodwe3jgy\"","Content-Disposition":"inline","In-Reply-To":"<20190323130934.GD4587@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 15/31] RFC: libcamera: ipu3: Enable\n\tImgU media links","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, 23 Mar 2019 13:51:02 -0000"}}]