[{"id":1576,"web_url":"https://patchwork.libcamera.org/comment/1576/","msgid":"<20190511034123.GI12768@pendragon.ideasonboard.com>","date":"2019-05-11T03:41:23","subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: media_device:\n\tHandle media 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 Wed, May 08, 2019 at 05:18:24PM +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> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> ---\n>  src/libcamera/include/media_device.h         |  2 +-\n>  src/libcamera/media_device.cpp               | 29 +++++++-------\n>  src/libcamera/pipeline/ipu3/ipu3.cpp         | 39 +++---------------\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 42 +++++---------------\n>  test/media_device/media_device_link_test.cpp |  7 ----\n>  5 files changed, 33 insertions(+), 86 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 416a0d0c207ea72d..ca260c9ea991dd53 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -40,10 +40,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> @@ -51,12 +50,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> @@ -66,8 +59,8 @@ LOG_DEFINE_CATEGORY(MediaDevice)\n>   * \\brief Construct a MediaDevice\n>   * \\param[in] 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> @@ -92,7 +85,8 @@ MediaDevice::~MediaDevice()\n>   * busy() function.\n>   *\n>   * Once claimed the device shall be released by its user when not needed anymore\n> - * by calling the release() function.\n> + * by calling the release() function. Acquiring the media device opens a file\n> + * descriptor to the device which is kept open until release() is called.\n>   *\n>   * Exclusive access is only guaranteed if all users of the media device abide by\n>   * the device claiming mechanism, as it isn't enforced by the media device\n> @@ -107,15 +101,22 @@ bool MediaDevice::acquire()\n>  \tif (acquired_)\n>  \t\treturn false;\n>  \n> +\tif (open())\n> +\t\treturn false;\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> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 75e878afad4e67a9..8a6a0e2721955d15 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -631,23 +631,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> @@ -674,14 +661,10 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>  \t */\n>  \tret = imguMediaDev_->disableLinks();\n>  \tif (ret)\n> -\t\tgoto error;\n> +\t\treturn ret;\n>  \n>  \tret = registerCameras();\n>  \n> -error:\n> -\tcio2MediaDev_->close();\n> -\timguMediaDev_->close();\n> -\n>  \treturn ret == 0;\n>  }\n>  \n> @@ -1139,29 +1122,19 @@ 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> +\t\treturn ret;\n>  \n>  \tret = linkSetup(name_, PAD_OUTPUT, outputName, 0, enable);\n>  \tif (ret)\n> -\t\tgoto done;\n> +\t\treturn ret;\n>  \n>  \tret = linkSetup(name_, PAD_VF, viewfinderName, 0, enable);\n>  \tif (ret)\n> -\t\tgoto done;\n> +\t\treturn ret;\n>  \n> -\tret = linkSetup(name_, PAD_STAT, statName, 0, enable);\n> -\n> -done:\n> -\tmedia_->close();\n> -\n> -\treturn ret;\n> +\treturn linkSetup(name_, PAD_STAT, statName, 0, enable);\n>  }\n>  \n>  /*------------------------------------------------------------------------------\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 134d3df4e1eb2b9b..b94d742dd6ec33a4 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -148,10 +148,6 @@ int PipelineHandlerRkISP1::configureStreams(Camera *camera,\n>  \t */\n>  \tconst MediaPad *pad = dphy_->entity()->getPadByIndex(0);\n>  \n> -\tret = media_->open();\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n>  \tfor (MediaLink *link : pad->links()) {\n>  \t\tbool enable = link->source()->entity() == sensor->entity();\n>  \n> @@ -169,8 +165,6 @@ int PipelineHandlerRkISP1::configureStreams(Camera *camera,\n>  \t\t\tbreak;\n>  \t}\n>  \n> -\tmedia_->close();\n> -\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n> @@ -352,7 +346,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>  bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  {\n>  \tconst MediaPad *pad;\n> -\tint ret;\n>  \n>  \tDeviceMatch dm(\"rkisp1\");\n>  \tdm.add(\"rkisp1-isp-subdev\");\n> @@ -368,35 +361,27 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  \n>  \tmedia_->acquire();\n>  \n> -\tret = media_->open();\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n>  \t/* Create the V4L2 subdevices we will need. */\n>  \tdphy_ = V4L2Subdevice::fromEntityName(media_.get(),\n>  \t\t\t\t\t      \"rockchip-sy-mipi-dphy\");\n> -\tret = dphy_->open();\n> -\tif (ret < 0)\n> -\t\tgoto done;\n> +\tif (dphy_->open() < 0)\n> +\t\treturn false;\n>  \n>  \tisp_ = V4L2Subdevice::fromEntityName(media_.get(), \"rkisp1-isp-subdev\");\n> -\tret = isp_->open();\n> -\tif (ret < 0)\n> -\t\tgoto done;\n> +\tif (isp_->open() < 0)\n> +\t\treturn false;\n>  \n>  \t/* Locate and open the capture video node. */\n>  \tvideo_ = V4L2Device::fromEntityName(media_.get(), \"rkisp1_mainpath\");\n> -\tret = video_->open();\n> -\tif (ret < 0)\n> -\t\tgoto done;\n> +\tif (video_->open() < 0)\n> +\t\treturn false;\n>  \n>  \tvideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n>  \n>  \t/* Configure default links. */\n> -\tret = initLinks();\n> -\tif (ret < 0) {\n> +\tif (initLinks() < 0) {\n>  \t\tLOG(RkISP1, Error) << \"Failed to setup links\";\n> -\t\tgoto done;\n> +\t\treturn false;\n>  \t}\n>  \n>  \t/*\n> @@ -404,18 +389,13 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  \t * camera instance for each of them.\n>  \t */\n>  \tpad = dphy_->entity()->getPadByIndex(0);\n> -\tif (!pad) {\n> -\t\tret = -EINVAL;\n> -\t\tgoto done;\n> -\t}\n> +\tif (!pad)\n> +\t\treturn false;\n>  \n>  \tfor (MediaLink *link : pad->links())\n>  \t\tcreateCamera(link->source()->entity());\n>  \n> -done:\n> -\tmedia_->close();\n> -\n> -\treturn ret == 0;\n> +\treturn true;\n>  }\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 484d3691310f78a0..334bb44a6fc14371 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>","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8DD8160E4D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 11 May 2019 05:41:40 +0200 (CEST)","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 F2F632DF;\n\tSat, 11 May 2019 05:41:39 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557546100;\n\tbh=SRh/SQfRZNA316YfffoO9kSSeu3iO+kFeAlkhpBp+D0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HipB21GzCMMdk/g4QBvf/2nRAu6ZbELlj6uwNlJDuKssjeQgda9FVp3IXqs6Tq8zb\n\tqKbsk9ufQbs0nD+Qo1EZfXdhTlJUDLilK1IXXEwV5SX37BTXBZNjAByt1P8ItGEMxx\n\tRgO2FZozHxWfDKdwNVG9EbpdybTnZiXcUX8nmk6U=","Date":"Sat, 11 May 2019 06:41:23 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190511034123.GI12768@pendragon.ideasonboard.com>","References":"<20190508151831.12274-1-niklas.soderlund@ragnatech.se>\n\t<20190508151831.12274-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":"<20190508151831.12274-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 04/11] libcamera: media_device:\n\tHandle media device fd in acquire() and release()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 11 May 2019 03:41:40 -0000"}}]