[PATCH v2 2/3] camera: Use invokeMethod() for pipe_->acquire() and pipe_->release()

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tue, Aug 27, 2024 at 06:42:54PM +0200, Hans de Goede wrote:
> The uvcvideo driver needs to open / close its /dev/video# node from
> pipe_->acquireDevices() / pipe_->releaseDevices().
> 
> V4L2VideoDevice::open() creates an EventNotifier and this notifier needs
> to be created from the CameraManager thread.
> 
> Use invokeMethod() for pipe_->acquire() and pipe_->release() so that
> the EventNotifiers are created from the CameraManager thread context.
> 
> Running pipe_->acquire() and pipe_->release() from the CameraManager
> thread context serializes all calls to them. Drop PipelineHandler::lock_
> this now is no longer necessary and update the "\context" part of
> the documentation for acquire[Device]() and release[Device]() to match.
> 
> Note the delayed opening of /dev/video# is a special case because
> the kernel uvcvideo driver powers on the USB device as soon as /dev/video#
> is opened. This behavior should *not* be copied by other pipeline handlers.

The real reason why this shouldn't be copied is not so much that the
uvcvideo driver is a special case, but more that a proper API is needed
in libcamera. This text is fine for now though, let's keep the ball
rolling on the public API improvements, instead of nitpicking on commit
messages :-)

> Signed-off-by: Hans de Goede <hdegoede at redhat.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
> Changes in v2:
> - Add a note to the commit message that opening/closing /dev/video# from
>   acquire()/release() as the uvcvideo pipeline handler does is an exception
>   and that this behavior should not be copied by other pipeline handlers
> - Drop lock_, update "\context" in doxygen docs
> ---
>  include/libcamera/internal/pipeline_handler.h |  5 +----
>  src/libcamera/camera.cpp                      |  6 ++++--
>  src/libcamera/pipeline_handler.cpp            | 12 ++++++------
>  3 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 597f7c3e..c33cf715 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -14,7 +14,6 @@
>  #include <sys/types.h>
>  #include <vector>
>  
> -#include <libcamera/base/mutex.h>
>  #include <libcamera/base/object.h>
>  
>  #include <libcamera/controls.h>
> @@ -99,9 +98,7 @@ private:
>  	std::queue<Request *> waitingRequests_;
>  
>  	const char *name_;
> -
> -	Mutex lock_;
> -	unsigned int useCount_ LIBCAMERA_TSA_GUARDED_BY(lock_);
> +	unsigned int useCount_;
>  
>  	friend class PipelineHandlerFactoryBase;
>  };
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 4e393f89..61925e83 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -995,7 +995,8 @@ int Camera::acquire()
>  	if (ret < 0)
>  		return ret == -EACCES ? -EBUSY : ret;
>  
> -	if (!d->pipe_->acquire(this)) {
> +	if (!d->pipe_->invokeMethod(&PipelineHandler::acquire,
> +				    ConnectionTypeBlocking, this)) {
>  		LOG(Camera, Info)
>  			<< "Pipeline handler in use by another process";
>  		return -EBUSY;
> @@ -1030,7 +1031,8 @@ int Camera::release()
>  		return ret == -EACCES ? -EBUSY : ret;
>  
>  	if (d->isAcquired())
> -		d->pipe_->release(this);
> +		d->pipe_->invokeMethod(&PipelineHandler::release,
> +				       ConnectionTypeBlocking, this);
>  
>  	d->setState(Private::CameraAvailable);
>  
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 861815cb..b18b6d0b 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -157,7 +157,7 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
>   * Pipeline handlers shall not call this function directly as the Camera class
>   * handles access internally.
>   *
> - * \context This function is \threadsafe.
> + * \context This function is called from the CameraManager thread.
>   *
>   * \return True if the pipeline handler was acquired, false if another process
>   * has already acquired it
> @@ -165,8 +165,6 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
>   */
>  bool PipelineHandler::acquire(Camera *camera)
>  {
> -	MutexLocker locker(lock_);
> -
>  	if (useCount_ == 0) {
>  		for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
>  			if (!media->lock()) {
> @@ -199,14 +197,12 @@ bool PipelineHandler::acquire(Camera *camera)
>   * Pipeline handlers shall not call this function directly as the Camera class
>   * handles access internally.
>   *
> - * \context This function is \threadsafe.
> + * \context This function is called from the CameraManager thread.
>   *
>   * \sa acquire()
>   */
>  void PipelineHandler::release(Camera *camera)
>  {
> -	MutexLocker locker(lock_);
> -
>  	ASSERT(useCount_);
>  
>  	releaseDevice(camera);
> @@ -230,6 +226,8 @@ void PipelineHandler::release(Camera *camera)
>   * powers on the USB device as soon as /dev/video# is opened. This behavior
>   * should *not* be copied by other pipeline handlers.
>   *
> + * \context This function is called from the CameraManager thread.
> + *
>   * \return True on success, false on failure
>   * \sa releaseDevice()
>   */
> @@ -250,6 +248,8 @@ bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)
>   * release them until releaseDevice() has been called for all previously
>   * acquired cameras.
>   *
> + * \context This function is called from the CameraManager thread.
> + *
>   * \sa acquireDevice()
>   */
>  void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)

-- 
Regards,

Laurent Pinchart