libcamera: media_device: Handle media device fd in acquire() and release()

To gain better control of when a file descriptor is open to the media
device and reduce the work needed in pipeline handler implementations,
handle the file descriptor in acquire() and release().

This changes the current behavior where a file descriptor is only open
when requested by the pipeline handler to that one is always open as
long a media device is acquired. This new behavior is desired to allow
implementing exclusive access to a pipeline handler between processes.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

5 files changed, 33 insertions, 86 deletions

diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
index 9f038093..88398505 100644
--- a/src/libcamera/include/media_device.h
+++ b/src/libcamera/include/media_device.h
@@ -27,7 +27,7 @@ public:
 	~MediaDevice();
 
 	bool acquire();
-	void release() { acquired_ = false; }
+	void release();
 	bool busy() const { return acquired_; }
 
 	int open();
diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
index eef348e3..b1cc37e8 100644
--- a/src/libcamera/media_device.cpp
+++ b/src/libcamera/media_device.cpp
@@ -40,10 +40,9 @@ LOG_DEFINE_CATEGORY(MediaDevice)
  * instance.
  *
  * The instance is created with an empty media graph. Before performing any
- * other operation, it must be opened with the open() function and the media
- * graph populated by calling populate(). Instances of MediaEntity, MediaPad and
- * MediaLink are created to model the media graph, and stored in a map indexed
- * by object id.
+ * other operation, it must be populate by calling populate(). Instances of
+ * MediaEntity, MediaPad and MediaLink are created to model the media graph,
+ * and stored in a map indexed by object id.
  *
  * The graph is valid once successfully populated, as reported by the valid()
  * function. It can be queried to list all entities(), or entities can be
@@ -51,12 +50,6 @@ LOG_DEFINE_CATEGORY(MediaDevice)
  * entity to entity through pads and links as exposed by the corresponding
  * classes.
  *
- * An open media device will keep an open file handle for the underlying media
- * controller device node. It can be closed at any time with a call to close().
- * This will not invalidate the media graph and all cached media objects remain
- * valid and can be accessed normally. The device can then be later reopened if
- * needed to perform other operations that interact with the device node.
- *
  * Media device can be claimed for exclusive use with acquire(), released with
  * release() and tested with busy(). This mechanism is aimed at pipeline
  * managers to claim media devices they support during enumeration.
@@ -66,8 +59,8 @@ LOG_DEFINE_CATEGORY(MediaDevice)
  * \brief Construct a MediaDevice
  * \param[in] deviceNode The media device node path
  *
- * Once constructed the media device is invalid, and must be opened and
- * populated with open() and populate() before the media graph can be queried.
+ * Once constructed the media device is invalid, and must be populated with
+ * populate() before the media graph can be queried.
  */
 MediaDevice::MediaDevice(const std::string &deviceNode)
 	: deviceNode_(deviceNode), fd_(-1), valid_(false), acquired_(false)
@@ -92,7 +85,8 @@ MediaDevice::~MediaDevice()
  * busy() function.
  *
  * Once claimed the device shall be released by its user when not needed anymore
- * by calling the release() function.
+ * by calling the release() function. Acquiring the media device opens a file
+ * descriptor to the device which is kept open until release() is called.
  *
  * Exclusive access is only guaranteed if all users of the media device abide by
  * the device claiming mechanism, as it isn't enforced by the media device
@@ -107,15 +101,22 @@ bool MediaDevice::acquire()
 	if (acquired_)
 		return false;
 
+	if (open())
+		return false;
+
 	acquired_ = true;
 	return true;
 }
 
 /**
- * \fn MediaDevice::release()
  * \brief Release a device previously claimed for exclusive use
  * \sa acquire(), busy()
  */
+void MediaDevice::release()
+{
+	close();
+	acquired_ = false;
+}
 
 /**
  * \fn MediaDevice::busy()
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 75e878af..8a6a0e27 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -631,22 +631,9 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
 	/*
 	 * Disable all links that are enabled by default on CIO2, as camera
 	 * creation enables all valid links it finds.
-	 *
-	 * Close the CIO2 media device after, as links are enabled and should
-	 * not need to be changed after.
 	 */
-	if (cio2MediaDev_->open())
-		return false;
-
-	if (cio2MediaDev_->disableLinks()) {
-		cio2MediaDev_->close();
-		return false;
-	}
-
-	if (imguMediaDev_->open()) {
-		cio2MediaDev_->close();
+	if (cio2MediaDev_->disableLinks())
 		return false;
-	}
 
 	/*
 	 * FIXME: enabled links in one ImgU instance interfere with capture
@@ -674,14 +661,10 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
 	 */
 	ret = imguMediaDev_->disableLinks();
 	if (ret)
-		goto error;
+		return ret;
 
 	ret = registerCameras();
 
-error:
-	cio2MediaDev_->close();
-	imguMediaDev_->close();
-
 	return ret == 0;
 }
 
@@ -1139,29 +1122,19 @@ int ImgUDevice::enableLinks(bool enable)
 	std::string inputName = name_ + " input";
 	int ret;
 
-	/* \todo Establish rules to handle media devices open/close. */
-	ret = media_->open();
-	if (ret)
-		return ret;
-
 	ret = linkSetup(inputName, 0, name_, PAD_INPUT, enable);
 	if (ret)
-		goto done;
+		return ret;
 
 	ret = linkSetup(name_, PAD_OUTPUT, outputName, 0, enable);
 	if (ret)
-		goto done;
+		return ret;
 
 	ret = linkSetup(name_, PAD_VF, viewfinderName, 0, enable);
 	if (ret)
-		goto done;
-
-	ret = linkSetup(name_, PAD_STAT, statName, 0, enable);
-
-done:
-	media_->close();
+		return ret;
 
-	return ret;
+	return linkSetup(name_, PAD_STAT, statName, 0, enable);
 }
 
 /*------------------------------------------------------------------------------
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 134d3df4..b94d742d 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -148,10 +148,6 @@ int PipelineHandlerRkISP1::configureStreams(Camera *camera,
 	 */
 	const MediaPad *pad = dphy_->entity()->getPadByIndex(0);
 
-	ret = media_->open();
-	if (ret < 0)
-		return ret;
-
 	for (MediaLink *link : pad->links()) {
 		bool enable = link->source()->entity() == sensor->entity();
 
@@ -169,8 +165,6 @@ int PipelineHandlerRkISP1::configureStreams(Camera *camera,
 			break;
 	}
 
-	media_->close();
-
 	if (ret < 0)
 		return ret;
 
@@ -352,7 +346,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
 {
 	const MediaPad *pad;
-	int ret;
 
 	DeviceMatch dm("rkisp1");
 	dm.add("rkisp1-isp-subdev");
@@ -368,35 +361,27 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
 
 	media_->acquire();
 
-	ret = media_->open();
-	if (ret < 0)
-		return ret;
-
 	/* Create the V4L2 subdevices we will need. */
 	dphy_ = V4L2Subdevice::fromEntityName(media_.get(),
 					      "rockchip-sy-mipi-dphy");
-	ret = dphy_->open();
-	if (ret < 0)
-		goto done;
+	if (dphy_->open() < 0)
+		return false;
 
 	isp_ = V4L2Subdevice::fromEntityName(media_.get(), "rkisp1-isp-subdev");
-	ret = isp_->open();
-	if (ret < 0)
-		goto done;
+	if (isp_->open() < 0)
+		return false;
 
 	/* Locate and open the capture video node. */
 	video_ = V4L2Device::fromEntityName(media_.get(), "rkisp1_mainpath");
-	ret = video_->open();
-	if (ret < 0)
-		goto done;
+	if (video_->open() < 0)
+		return false;
 
 	video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);
 
 	/* Configure default links. */
-	ret = initLinks();
-	if (ret < 0) {
+	if (initLinks() < 0) {
 		LOG(RkISP1, Error) << "Failed to setup links";
-		goto done;
+		return false;
 	}
 
 	/*
@@ -404,18 +389,13 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
 	 * camera instance for each of them.
 	 */
 	pad = dphy_->entity()->getPadByIndex(0);
-	if (!pad) {
-		ret = -EINVAL;
-		goto done;
-	}
+	if (!pad)
+		return false;
 
 	for (MediaLink *link : pad->links())
 		createCamera(link->source()->entity());
 
-done:
-	media_->close();
-
-	return ret == 0;
+	return true;
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp
index 484d3691..334bb44a 100644
--- a/test/media_device/media_device_link_test.cpp
+++ b/test/media_device/media_device_link_test.cpp
@@ -57,12 +57,6 @@ class MediaDeviceLinkTest : public Test
 			return TestSkip;
 		}
 
-		if (dev_->open()) {
-			cerr << "Failed to open media device at "
-			     << dev_->deviceNode() << endl;
-			return TestFail;
-		}
-
 		return 0;
 	}
 
@@ -238,7 +232,6 @@ class MediaDeviceLinkTest : public Test
 
 	void cleanup()
 	{
-		dev_->close();
 		dev_->release();
 	}