[{"id":1420,"web_url":"https://patchwork.libcamera.org/comment/1420/","msgid":"<20190416230738.GP4822@pendragon.ideasonboard.com>","date":"2019-04-16T23:07:38","subject":"Re: [libcamera-devel] [RFC 04/11] libcamera: media_device: Handle\n\tmedia device fd in acquire() and release()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sun, Apr 14, 2019 at 03:34:59AM +0200, Niklas Söderlund wrote:\n> To gain better control of when a file descriptor is open to the media\n> device and reduce the work needed in pipeline handler implementations,\n> handle the file descriptor in acquire() and release().\n> \n> This changes the current behavior where a file descriptor is only open\n> when requested by the pipeline handler to that one is always open as\n> long a media device is acquired. This new behavior is desired to allow\n> implementing exclusive access to a pipeline handler between processes.\n\nI think this is fine. It might interfere with power handling on the\nkernel side, but I don't think the kernel should perform power\nmanagement on open/close of media devices, so it's an issue that would\nneed to be handled there.\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/media_device.h | 2 +-\n> src/libcamera/media_device.cpp | 33 ++++++++++----------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 25 +--------------\n> test/media_device/media_device_link_test.cpp | 7 -----\n> test/v4l2_subdevice/v4l2_subdevice_test.cpp | 10 +-----\n> 5 files changed, 19 insertions(+), 58 deletions(-)\n> \n> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h\n> index 9f038093b2b22fc7..883985055eb419dd 100644\n> --- a/src/libcamera/include/media_device.h\n> +++ b/src/libcamera/include/media_device.h\n> @@ -27,7 +27,7 @@ public:\n> \t~MediaDevice();\n> \n> \tbool acquire();\n> -\tvoid release() { acquired_ = false; }\n> +\tvoid release();\n> \tbool busy() const { return acquired_; }\n> \n> \tint open();\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 54706bb73a7591d5..644c6143fb15e1fa 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -39,10 +39,9 @@ LOG_DEFINE_CATEGORY(MediaDevice)\n> * instance.\n> *\n> * The instance is created with an empty media graph. Before performing any\n> - * other operation, it must be opened with the open() function and the media\n> - * graph populated by calling populate(). Instances of MediaEntity, MediaPad and\n> - * MediaLink are created to model the media graph, and stored in a map indexed\n> - * by object id.\n> + * other operation, it must be populate by calling populate(). Instances of\n> + * MediaEntity, MediaPad and MediaLink are created to model the media graph,\n> + * and stored in a map indexed by object id.\n> *\n> * The graph is valid once successfully populated, as reported by the valid()\n> * function. It can be queried to list all entities(), or entities can be\n> @@ -50,12 +49,6 @@ LOG_DEFINE_CATEGORY(MediaDevice)\n> * entity to entity through pads and links as exposed by the corresponding\n> * classes.\n> *\n> - * An open media device will keep an open file handle for the underlying media\n> - * controller device node. It can be closed at any time with a call to close().\n> - * This will not invalidate the media graph and all cached media objects remain\n> - * valid and can be accessed normally. The device can then be later reopened if\n> - * needed to perform other operations that interact with the device node.\n> - *\n> * Media device can be claimed for exclusive use with acquire(), released with\n> * release() and tested with busy(). This mechanism is aimed at pipeline\n> * managers to claim media devices they support during enumeration.\n> @@ -65,8 +58,8 @@ LOG_DEFINE_CATEGORY(MediaDevice)\n> * \\brief Construct a MediaDevice\n> * \\param deviceNode The media device node path\n> *\n> - * Once constructed the media device is invalid, and must be opened and\n> - * populated with open() and populate() before the media graph can be queried.\n> + * Once constructed the media device is invalid, and must be populated with\n> + * populate() before the media graph can be queried.\n> */\n> MediaDevice::MediaDevice(const std::string &deviceNode)\n> \t: deviceNode_(deviceNode), fd_(-1), valid_(false), acquired_(false)\n> @@ -106,15 +99,22 @@ bool MediaDevice::acquire()\n> \tif (acquired_)\n> \t\treturn false;\n> \n> +\tif (open())\n> +\t\treturn false;\n> +\n\nI think you should document that an acquired media device will keep an\nfd open until the device is released.\n\n> \tacquired_ = true;\n> \treturn true;\n> }\n> \n> /**\n> - * \\fn MediaDevice::release()\n> * \\brief Release a device previously claimed for exclusive use\n> * \\sa acquire(), busy()\n> */\n> +void MediaDevice::release()\n> +{\n> +\tclose();\n> +\tacquired_ = false;\n> +}\n> \n> /**\n> * \\fn MediaDevice::busy()\n> @@ -127,10 +127,6 @@ bool MediaDevice::acquire()\n> /**\n> * \\brief Open a media device and retrieve device information\n\nI think you should remove \" and retrieve device information\" in the\nprevious patch that moves information retrieval to populate().\n\n> *\n> - * Before populating the media graph or performing any operation that interact\n> - * with the device node associated with the media device, the device node must\n> - * be opened.\n> - *\n\nAnd maybe removing \"populating the media graph or \" in a previous patch\ntoo ?\n\n> * If the device is already open the function returns -EBUSY.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> @@ -201,6 +197,9 @@ int MediaDevice::populate()\n> \t__u64 version = -1;\n> \tint ret;\n> \n> +\tif (fd_ != -1)\n> +\t\treturn -EBUSY;\n> +\n\nCan this happen ? If you think the check is useful, I would log an error\nmessage, and update the method's documentation.\n\n> \tclear();\n> \n> \tret = open();\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index d8de6b3c36314fc0..a87eff8dfec6d143 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -468,23 +468,10 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> \t/*\n> \t * Disable all links that are enabled by default on CIO2, as camera\n> \t * creation enables all valid links it finds.\n> -\t *\n> -\t * Close the CIO2 media device after, as links are enabled and should\n> -\t * not need to be changed after.\n> \t */\n> -\tif (cio2MediaDev_->open())\n> +\tif (cio2MediaDev_->disableLinks())\n> \t\treturn false;\n> \n> -\tif (cio2MediaDev_->disableLinks()) {\n> -\t\tcio2MediaDev_->close();\n> -\t\treturn false;\n> -\t}\n> -\n> -\tif (imguMediaDev_->open()) {\n> -\t\tcio2MediaDev_->close();\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> @@ -516,9 +503,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> \tret = registerCameras();\n> \n> error:\n\nYou can remove the error label and return directly instead of using\ngoto.\n\n> -\tcio2MediaDev_->close();\n> -\timguMediaDev_->close();\n> -\n> \treturn ret == 0;\n> }\n> \n> @@ -961,11 +945,6 @@ int ImgUDevice::enableLinks(bool enable)\n> \tstd::string inputName = name_ + \" input\";\n> \tint ret;\n> \n> -\t/* \\todo Establish rules to handle media devices open/close. */\n> -\tret = media_->open();\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> \tret = linkSetup(inputName, 0, name_, PAD_INPUT, enable);\n> \tif (ret)\n> \t\tgoto done;\n> @@ -981,8 +960,6 @@ int ImgUDevice::enableLinks(bool enable)\n> \tret = linkSetup(name_, PAD_STAT, statName, 0, enable);\n> \n> done:\n\nSame here.\n\n> -\tmedia_->close();\n> -\n> \treturn ret;\n> }\n> \n> diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp\n> index 4b8de3bee722e912..3989da43ca9ec30f 100644\n> --- a/test/media_device/media_device_link_test.cpp\n> +++ b/test/media_device/media_device_link_test.cpp\n> @@ -57,12 +57,6 @@ class MediaDeviceLinkTest : public Test\n> \t\t\treturn TestSkip;\n> \t\t}\n> \n> -\t\tif (dev_->open()) {\n> -\t\t\tcerr << \"Failed to open media device at \"\n> -\t\t\t << dev_->deviceNode() << endl;\n> -\t\t\treturn TestFail;\n> -\t\t}\n> -\n> \t\treturn 0;\n> \t}\n> \n> @@ -238,7 +232,6 @@ class MediaDeviceLinkTest : public Test\n> \n> \tvoid cleanup()\n> \t{\n> -\t\tdev_->close();\n> \t\tdev_->release();\n> \t}\n> \n> diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> index 21335efbad4d5668..31f3465a7ba3a5b1 100644\n> --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp\n> @@ -51,14 +51,6 @@ int V4L2SubdeviceTest::init()\n> \t\treturn TestSkip;\n> \t}\n> \n> -\tint ret = media_->open();\n> -\tif (ret) {\n> -\t\tcerr << \"Unable to open media device: \" << media_->deviceNode()\n> -\t\t << \": \" << strerror(ret) << endl;\n> -\t\tmedia_->release();\n> -\t\treturn TestSkip;\n> -\t}\n> -\n> \tMediaEntity *videoEntity = media_->getEntityByName(\"Scaler\");\n> \tif (!videoEntity) {\n> \t\tcerr << \"Unable to find media entity 'Scaler'\" << endl;\n> @@ -67,7 +59,7 @@ int V4L2SubdeviceTest::init()\n> \t}\n> \n> \tscaler_ = new V4L2Subdevice(videoEntity);\n> -\tret = scaler_->open();\n> +\tint ret = scaler_->open();\n> \tif (ret) {\n> \t\tcerr << \"Unable to open video subdevice \"\n> \t\t << scaler_->deviceNode() << endl;","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 D91FB60004\n\tfor ;\n\tWed, 17 Apr 2019 01:07:47 +0200 (CEST)","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 40766E2;\n\tWed, 17 Apr 2019 01:07:47 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555456067;\n\tbh=LxCZFCc0qg5UsemGtsQOhukg6N8nfUyN9Vhy/hGuwgk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LWV0AIQ8PBslW32DgEYrrQ0LWYZWSvKUNnH/SFkB/FaAZb1ytBSwhsHgrjdUSbWME\n\tQ443SnVdDoqVauisLGMy0RQeUYWVZtlaNmxR9sPbO8IJrmMSNVue2FGAZ664g97Clx\n\truMSKJ/gkRm1PpoW5jbLc3xUaJEXkQmS+bWKUlUs=","Date":"Wed, 17 Apr 2019 02:07:38 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190416230738.GP4822@pendragon.ideasonboard.com>","References":"<20190414013506.10515-1-niklas.soderlund@ragnatech.se>\n\t<20190414013506.10515-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190414013506.10515-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 04/11] libcamera: media_device: Handle\n\tmedia device fd in acquire() and release()","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, 16 Apr 2019 23:07:48 -0000"}}]