[libcamera-devel] [RFC 08/11] libcamera: media_device: Restrict acquire() and release()

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sun, Apr 14, 2019 at 03:35:03AM +0200, Niklas Söderlund wrote:
> The only valid call site for acquire() and release() are inside the
> PipelineHandler base class. Make them private to with a specific friend
> clause to block specific pipeline handler implementations from calling
> them.

This looks OK to me, but I have a bit of trouble understanding the
rationale behind this change. The locking introduced in the next patches
doesn't seem to interact with acquire() and release() of the media
device at all.

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/include/media_device.h        |  6 +-
>  src/libcamera/media_device.cpp              | 85 +++++++++++----------
>  test/v4l2_device/v4l2_device_test.cpp       |  5 --
>  test/v4l2_subdevice/v4l2_subdevice_test.cpp | 10 ---
>  4 files changed, 47 insertions(+), 59 deletions(-)
> 
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index e513a2fee1b91a1b..5d28a7bb8214ef4a 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -26,8 +26,6 @@ public:
>  	MediaDevice(const std::string &deviceNode);
>  	~MediaDevice();
>  
> -	bool acquire();
> -	void release();
>  	bool busy() const { return acquired_; }
>  
>  	int populate();
> @@ -62,6 +60,10 @@ private:
>  	int open();
>  	void close();
>  
> +	friend class PipelineHandler;
> +	bool acquire();
> +	void release();
> +
>  	std::map<unsigned int, MediaObject *> objects_;
>  	MediaObject *object(unsigned int id);
>  	bool addObject(MediaObject *object);
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 0138be526f886c90..46931f52688f6c82 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -73,48 +73,6 @@ MediaDevice::~MediaDevice()
>  	clear();
>  }
>  
> -/**
> - * \brief Claim a device for exclusive use
> - *
> - * The device claiming mechanism offers simple media device access arbitration
> - * between multiple users. When the media device is created, it is available to
> - * all users. Users can query the media graph to determine whether they can
> - * support the device and, if they do, claim the device for exclusive use. Other
> - * users are then expected to skip over media devices in use as reported by the
> - * busy() function.
> - *
> - * Once claimed the device shall be released by its user when not needed anymore
> - * by calling the release() function.
> - *
> - * 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
> - * itself.
> - *
> - * \return true if the device was successfully claimed, or false if it was
> - * already in use
> - * \sa release(), busy()
> - */
> -bool MediaDevice::acquire()
> -{
> -	if (acquired_)
> -		return false;
> -
> -	if (open())
> -		return false;
> -
> -	acquired_ = true;
> -	return true;
> -}
> -
> -/**
> - * \brief Release a device previously claimed for exclusive use
> - * \sa acquire(), busy()
> - */
> -void MediaDevice::release()
> -{
> -	close();
> -	acquired_ = false;
> -}
>  
>  /**
>   * \fn MediaDevice::busy()
> @@ -420,6 +378,49 @@ void MediaDevice::close()
>  	fd_ = -1;
>  }
>  
> +/**
> + * \brief Claim a device for exclusive use
> + *
> + * The device claiming mechanism offers simple media device access arbitration
> + * between multiple users. When the media device is created, it is available to
> + * all users. Users can query the media graph to determine whether they can
> + * support the device and, if they do, claim the device for exclusive use. Other
> + * users are then expected to skip over media devices in use as reported by the
> + * busy() function.
> + *
> + * Once claimed the device shall be released by its user when not needed anymore
> + * by calling the release() function.
> + *
> + * 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
> + * itself.
> + *
> + * \return true if the device was successfully claimed, or false if it was
> + * already in use
> + * \sa release(), busy()
> + */
> +bool MediaDevice::acquire()
> +{
> +	if (acquired_)
> +		return false;
> +
> +	if (open())
> +		return false;
> +
> +	acquired_ = true;
> +	return true;
> +}
> +
> +/**
> + * \brief Release a device previously claimed for exclusive use
> + * \sa acquire(), busy()
> + */
> +void MediaDevice::release()
> +{
> +	close();
> +	acquired_ = false;
> +}
> +
>  /**
>   * \var MediaDevice::objects_
>   * \brief Global map of media objects (entities, pads, links) keyed by their
> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
> index 90e5f7a3ee0c0f2f..833038d56ea4631d 100644
> --- a/test/v4l2_device/v4l2_device_test.cpp
> +++ b/test/v4l2_device/v4l2_device_test.cpp
> @@ -46,9 +46,6 @@ int V4L2DeviceTest::init()
>  	if (!media_)
>  		return TestSkip;
>  
> -	if (!media_->acquire())
> -		return TestSkip;
> -
>  	MediaEntity *entity = media_->getEntityByName("vivid-000-vid-cap");
>  	if (!entity)
>  		return TestSkip;
> @@ -62,8 +59,6 @@ int V4L2DeviceTest::init()
>  
>  void V4L2DeviceTest::cleanup()
>  {
> -	media_->release();
> -
>  	capture_->streamOff();
>  	capture_->releaseBuffers();
>  	capture_->close();
> diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.cpp b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> index 31f3465a7ba3a5b1..0180698840cc2751 100644
> --- a/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> +++ b/test/v4l2_subdevice/v4l2_subdevice_test.cpp
> @@ -45,16 +45,9 @@ int V4L2SubdeviceTest::init()
>  		return TestSkip;
>  	}
>  
> -	if (!media_->acquire()) {
> -		cerr << "Unable to acquire media device: "
> -		     << media_->deviceNode() << endl;
> -		return TestSkip;
> -	}
> -
>  	MediaEntity *videoEntity = media_->getEntityByName("Scaler");
>  	if (!videoEntity) {
>  		cerr << "Unable to find media entity 'Scaler'" << endl;
> -		media_->release();
>  		return TestFail;
>  	}
>  
> @@ -63,7 +56,6 @@ int V4L2SubdeviceTest::init()
>  	if (ret) {
>  		cerr << "Unable to open video subdevice "
>  		     << scaler_->deviceNode() << endl;
> -		media_->release();
>  		return TestSkip;
>  	}
>  
> @@ -72,7 +64,5 @@ int V4L2SubdeviceTest::init()
>  
>  void V4L2SubdeviceTest::cleanup()
>  {
> -	media_->release();
> -
>  	delete scaler_;
>  }

-- 
Regards,

Laurent Pinchart