[libcamera-devel] [RFC 04/11] libcamera: media_device: Handle media device fd in acquire() and release()

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sun, Apr 14, 2019 at 03:34:59AM +0200, Niklas Söderlund wrote:
> 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.

I think this is fine. It might interfere with power handling on the
kernel side, but I don't think the kernel should perform power
management on open/close of media devices, so it's an issue that would
need to be handled there.

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/include/media_device.h         |  2 +-
>  src/libcamera/media_device.cpp               | 33 ++++++++++----------
>  src/libcamera/pipeline/ipu3/ipu3.cpp         | 25 +--------------
>  test/media_device/media_device_link_test.cpp |  7 -----
>  test/v4l2_subdevice/v4l2_subdevice_test.cpp  | 10 +-----
>  5 files changed, 19 insertions(+), 58 deletions(-)
> 
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index 9f038093b2b22fc7..883985055eb419dd 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 54706bb73a7591d5..644c6143fb15e1fa 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -39,10 +39,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
> @@ -50,12 +49,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.
> @@ -65,8 +58,8 @@ LOG_DEFINE_CATEGORY(MediaDevice)
>   * \brief Construct a MediaDevice
>   * \param 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)
> @@ -106,15 +99,22 @@ bool MediaDevice::acquire()
>  	if (acquired_)
>  		return false;
>  
> +	if (open())
> +		return false;
> +

I think you should document that an acquired media device will keep an
fd open until the device is released.

>  	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()
> @@ -127,10 +127,6 @@ bool MediaDevice::acquire()
>  /**
>   * \brief Open a media device and retrieve device information

I think you should remove " and retrieve device information" in the
previous patch that moves information retrieval to populate().

>   *
> - * Before populating the media graph or performing any operation that interact
> - * with the device node associated with the media device, the device node must
> - * be opened.
> - *

And maybe removing "populating the media graph or " in a previous patch
too ?

>   * If the device is already open the function returns -EBUSY.
>   *
>   * \return 0 on success or a negative error code otherwise
> @@ -201,6 +197,9 @@ int MediaDevice::populate()
>  	__u64 version = -1;
>  	int ret;
>  
> +	if (fd_ != -1)
> +		return -EBUSY;
> +

Can this happen ? If you think the check is useful, I would log an error
message, and update the method's documentation.

>  	clear();
>  
>  	ret = open();
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index d8de6b3c36314fc0..a87eff8dfec6d143 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -468,23 +468,10 @@ 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())
> +	if (cio2MediaDev_->disableLinks())
>  		return false;
>  
> -	if (cio2MediaDev_->disableLinks()) {
> -		cio2MediaDev_->close();
> -		return false;
> -	}
> -
> -	if (imguMediaDev_->open()) {
> -		cio2MediaDev_->close();
> -		return false;
> -	}
> -
>  	/*
>  	 * FIXME: enabled links in one ImgU instance interfere with capture
>  	 * operations on the other one. This can be easily triggered by
> @@ -516,9 +503,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>  	ret = registerCameras();
>  
>  error:

You can remove the error label and return directly instead of using
goto.

> -	cio2MediaDev_->close();
> -	imguMediaDev_->close();
> -
>  	return ret == 0;
>  }
>  
> @@ -961,11 +945,6 @@ 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;
> @@ -981,8 +960,6 @@ int ImgUDevice::enableLinks(bool enable)
>  	ret = linkSetup(name_, PAD_STAT, statName, 0, enable);
>  
>  done:

Same here.

> -	media_->close();
> -
>  	return ret;
>  }
>  
> diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp
> index 4b8de3bee722e912..3989da43ca9ec30f 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();
>  	}
>  
> diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> index 21335efbad4d5668..31f3465a7ba3a5b1 100644
> --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> @@ -51,14 +51,6 @@ int V4L2SubdeviceTest::init()
>  		return TestSkip;
>  	}
>  
> -	int ret = media_->open();
> -	if (ret) {
> -		cerr << "Unable to open media device: " << media_->deviceNode()
> -		     << ": " << strerror(ret) << endl;
> -		media_->release();
> -		return TestSkip;
> -	}
> -
>  	MediaEntity *videoEntity = media_->getEntityByName("Scaler");
>  	if (!videoEntity) {
>  		cerr << "Unable to find media entity 'Scaler'" << endl;
> @@ -67,7 +59,7 @@ int V4L2SubdeviceTest::init()
>  	}
>  
>  	scaler_ = new V4L2Subdevice(videoEntity);
> -	ret = scaler_->open();
> +	int ret = scaler_->open();
>  	if (ret) {
>  		cerr << "Unable to open video subdevice "
>  		     << scaler_->deviceNode() << endl;

-- 
Regards,

Laurent Pinchart