[{"id":1064,"web_url":"https://patchwork.libcamera.org/comment/1064/","msgid":"<20190319171156.zizto5tuzrzfeejm@uno.localdomain>","date":"2019-03-19T17:11:56","subject":"Re: [libcamera-devel] [PATCH v2 14/14] 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,\n one additional note, as I might have confused you.\n\nOn Tue, Mar 12, 2019 at 01:12:42PM +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 only enable the required links at\n> streamConfiguration time.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 107 +++++++++++++++++++++++++++\n> 1 file changed, 107 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index a113fd6ee74b..1c0d63f912d5 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -51,6 +51,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> @@ -270,6 +274,35 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> \t\treturn -EINVAL;\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, then trying to capture from the other\n> +\t * one right after without disabling media links in the media graph.\n> +\t *\n> +\t * The tricky part here is where to disable links on the ImgU instance\n> +\t * which is 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 here, before configuring formats,\n> +\t * the only place where to disable links would be 'stop()', but the\n> +\t * camera state machine allows start()<->stop() sequences without any\n> +\t * streamConfiguration() in between.\n> +\t *\n> +\t * As of now, disable all links in the media graph before enabling\n> +\t * the requested ones only.\n> +\t */\n> +\tdata->imgu->mediaDevice_->disableLinks();\n\nI have now noticed doing this here is wrong, as the media device is\nclose, and I get a failure in the logs which I didn't notice.\n\nThe reason my testing work it's because I have retainted the link disabling\nat match() time in [5/14] which Laurent suggested to remove not to\ninterfere with other cameras.\n\ntl;dr: The comment is correct and I think this might require some\nthinking, but this specific line is wrong and should be removed.\n\nSorry about this.\n\n\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> @@ -631,6 +664,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> 2.20.1\n>","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 84A4A610C5\n\tfor ;\n\tTue, 19 Mar 2019 18:11:18 +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 439DD1BF20E\n\tfor ;\n\tTue, 19 Mar 2019 17:11:18 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 19 Mar 2019 18:11:56 +0100","From":"Jacopo Mondi ","To":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190319171156.zizto5tuzrzfeejm@uno.localdomain>","References":"<20190312121242.2253-1-jacopo@jmondi.org>\n\t<20190312121242.2253-15-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"gpq3sihtlqrhdnwo\"","Content-Disposition":"inline","In-Reply-To":"<20190312121242.2253-15-jacopo@jmondi.org>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 14/14] 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":"Tue, 19 Mar 2019 17:11:18 -0000"}}]