[libcamera-devel] [PATCH] libcamera: Allow concurrent use of cameras from same pipeline handler
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Aug 10 17:07:48 CEST 2022
Hi Laurent,
Quoting Laurent Pinchart (2022-07-21 22:03:51)
> libcamera implements a pipeline handler locking mechanism based on
> advisory locks on media devices, to prevent concurrent access to cameras
> from the same pipeline handler from different processes (this only works
> between multiple libcamera instances, as other processes won't use
> advisory locks on media devices).
>
> A side effect of the implementation prevents multiple cameras created by
> the same pipeline handler from being used concurrently. Fix this by
> turning the PipelineHandler lock() and unlock() functions into acquire()
> and release(), with a use count to replace the boolean lock flag. The
> Camera class is updated accordingly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> This may help addressing the problem reported in "[RFC PATCH] qcam: Fix
> IPU3 camera switching".
This does indeed fix things for me on IPU3 testing with qcam.
It however seemed to introduce a regression in CTS (which perhaps
Harvey's patches will then resolve)
However, I think we'll hear from David that this would benefit the RPi
platform already...
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> include/libcamera/internal/camera.h | 1 +
> include/libcamera/internal/pipeline_handler.h | 8 ++-
> src/libcamera/camera.cpp | 12 +++-
> src/libcamera/pipeline_handler.cpp | 63 ++++++++++++-------
> 4 files changed, 55 insertions(+), 29 deletions(-)
>
> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> index 597426a6f9d2..38dd94ff8156 100644
> --- a/include/libcamera/internal/camera.h
> +++ b/include/libcamera/internal/camera.h
> @@ -50,6 +50,7 @@ private:
> CameraRunning,
> };
>
> + bool isAcquired() const;
> bool isRunning() const;
> int isAccessAllowed(State state, bool allowDisconnected = false,
> const char *from = __builtin_FUNCTION()) const;
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index c3e4c2587ecd..20f1cbb07fea 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -45,8 +45,8 @@ public:
> MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,
> const DeviceMatch &dm);
>
> - bool lock();
> - void unlock();
> + bool acquire();
> + void release();
>
> virtual CameraConfiguration *generateConfiguration(Camera *camera,
> const StreamRoles &roles) = 0;
> @@ -77,6 +77,8 @@ protected:
> CameraManager *manager_;
>
> private:
> + void unlockMediaDevices();
> +
> void mediaDeviceDisconnected(MediaDevice *media);
> virtual void disconnect();
>
> @@ -91,7 +93,7 @@ private:
> const char *name_;
>
> Mutex lock_;
> - bool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */
> + unsigned int useCount_ LIBCAMERA_TSA_GUARDED_BY(lock_);
>
> friend class PipelineHandlerFactory;
> };
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 2a8ef60e3862..9ce32db1ac78 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -508,6 +508,11 @@ static const char *const camera_state_names[] = {
> "Running",
> };
>
> +bool Camera::Private::isAcquired() const
> +{
> + return state_.load(std::memory_order_acquire) == CameraRunning;
> +}
> +
> bool Camera::Private::isRunning() const
> {
> return state_.load(std::memory_order_acquire) == CameraRunning;
> @@ -811,7 +816,7 @@ int Camera::exportFrameBuffers(Stream *stream,
> * not blocking, if the device has already been acquired (by the same or another
> * process) the -EBUSY error code is returned.
> *
> - * Acquiring a camera will limit usage of any other camera(s) provided by the
> + * Acquiring a camera may limit usage of any other camera(s) provided by the
> * same pipeline handler to the same instance of libcamera. The limit is in
> * effect until all cameras from the pipeline handler are released. Other
> * instances of libcamera can still list and examine the cameras but will fail
> @@ -839,7 +844,7 @@ int Camera::acquire()
> if (ret < 0)
> return ret == -EACCES ? -EBUSY : ret;
>
> - if (!d->pipe_->lock()) {
> + if (!d->pipe_->acquire()) {
> LOG(Camera, Info)
> << "Pipeline handler in use by another process";
> return -EBUSY;
> @@ -873,7 +878,8 @@ int Camera::release()
> if (ret < 0)
> return ret == -EACCES ? -EBUSY : ret;
>
> - d->pipe_->unlock();
> + if (d->isAcquired())
> + d->pipe_->release();
>
> d->setState(Private::CameraAvailable);
>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 675405330f45..7d2d00ef45e6 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -68,7 +68,7 @@ LOG_DEFINE_CATEGORY(Pipeline)
> * respective factories.
> */
> PipelineHandler::PipelineHandler(CameraManager *manager)
> - : manager_(manager), lockOwner_(false)
> + : manager_(manager), useCount_(0)
> {
> }
>
> @@ -143,58 +143,75 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
> }
>
> /**
> - * \brief Lock all media devices acquired by the pipeline
> + * \brief Acquire exclusive access to the pipeline handler for the process
> *
> - * This function shall not be called from pipeline handler implementation, as
> - * the Camera class handles locking directly.
> + * This function locks all the media devices used by the pipeline to ensure
> + * that no other process can access them concurrently.
> + *
> + * Access to a pipeline handler may be acquired recursively from within the
> + * same process. Every successful acquire() call shall be matched with a
> + * release() call. This allows concurrent access to the same pipeline handler
> + * from different cameras within the same process.
> + *
> + * Pipeline handlers shall not call this function directly as the Camera class
> + * handles access internally.
> *
> * \context This function is \threadsafe.
> *
> - * \return True if the devices could be locked, false otherwise
> - * \sa unlock()
> - * \sa MediaDevice::lock()
> + * \return True if the pipeline handler was acquired, false if another process
> + * has already acquired it
> + * \sa release()
> */
> -bool PipelineHandler::lock()
> +bool PipelineHandler::acquire()
> {
> MutexLocker locker(lock_);
>
> - /* Do not allow nested locking in the same libcamera instance. */
> - if (lockOwner_)
> - return false;
> + if (useCount_) {
> + ++useCount_;
> + return true;
> + }
>
> for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
> if (!media->lock()) {
> - unlock();
> + unlockMediaDevices();
> return false;
> }
> }
>
> - lockOwner_ = true;
> -
> + ++useCount_;
> return true;
> }
>
> /**
> - * \brief Unlock all media devices acquired by the pipeline
> + * \brief Release exclusive access to the pipeline handler
> *
> - * This function shall not be called from pipeline handler implementation, as
> - * the Camera class handles locking directly.
> + * This function releases access to the pipeline handler previously acquired by
> + * a call to acquire(). Every release() call shall match a previous successful
> + * acquire() call. Calling this function on a pipeline handler that hasn't been
> + * acquired results in undefined behaviour.
> + *
> + * Pipeline handlers shall not call this function directly as the Camera class
> + * handles access internally.
> *
> * \context This function is \threadsafe.
> *
> - * \sa lock()
> + * \sa acquire()
> */
> -void PipelineHandler::unlock()
> +void PipelineHandler::release()
> {
> MutexLocker locker(lock_);
>
> - if (!lockOwner_)
> - return;
> + ASSERT(useCount_);
>
> + unlockMediaDevices();
> +
> + --useCount_;
> +}
> +
> +void PipelineHandler::unlockMediaDevices()
> +{
> for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
> media->unlock();
> -
> - lockOwner_ = false;
> }
>
> /**
>
> base-commit: d4eee094e684806dbdb54fbe1bc02c9c590aa7f4
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list