[libcamera-devel] [PATCH 16/19] libcamera: camera: Implement the threading model
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jan 22 17:36:14 CET 2020
Hi Laurent,
Thanks for your work.
On 2020-01-20 02:24:34 +0200, Laurent Pinchart wrote:
> Document the threading model of the Camera class and implement it.
> Selected functions become thread-safe, and require a few functions of
> the PipelineHandler class to be called through cross-thread invocation
> as the pipeline handlers live in the camera manager thread, while the
> Camera class is mostly accessed from the application thread. The
> PipelineHandler is made to inherit from the Object class to support
> this.
>
> Disconnection is currently left out as it is not implemented in pipeline
> handlers, and isn't fully supported in the Camera class either. This
> will be revisited when implementing proper hotplug support.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/camera.cpp | 101 +++++++++++++++++------
> src/libcamera/include/pipeline_handler.h | 4 +-
> 2 files changed, 78 insertions(+), 27 deletions(-)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 83c2249b8897..f281a4cd346b 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -8,6 +8,7 @@
> #include <libcamera/camera.h>
>
> #include <array>
> +#include <atomic>
> #include <iomanip>
>
> #include <libcamera/framebuffer_allocator.h>
> @@ -282,7 +283,7 @@ public:
>
> private:
> bool disconnected_;
> - State state_;
> + std::atomic<State> state_;
> };
>
> Camera::Private::Private(PipelineHandler *pipe, const std::string &name,
> @@ -294,7 +295,7 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name,
>
> Camera::Private::~Private()
> {
> - if (state_ != Private::CameraAvailable)
> + if (state_.load(std::memory_order_acquire) != Private::CameraAvailable)
> LOG(Camera, Error) << "Removing camera while still in use";
> }
>
> @@ -310,12 +311,13 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const
> if (!allowDisconnected && disconnected_)
> return -ENODEV;
>
> - if (state_ == state)
> + State currentState = state_.load(std::memory_order_acquire);
> + if (currentState == state)
> return 0;
>
> ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
>
> - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> + LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState]
> << " state trying operation requiring state "
> << camera_state_names[state];
>
> @@ -328,13 +330,14 @@ int Camera::Private::isAccessAllowed(State low, State high,
> if (!allowDisconnected && disconnected_)
> return -ENODEV;
>
> - if (state_ >= low && state_ <= high)
> + State currentState = state_.load(std::memory_order_acquire);
> + if (currentState >= low && currentState <= high)
> return 0;
>
> ASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&
> static_cast<unsigned int>(high) < camera_state_names.size());
>
> - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> + LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState]
> << " state trying operation requiring state between "
> << camera_state_names[low] << " and "
> << camera_state_names[high];
> @@ -349,15 +352,15 @@ void Camera::Private::disconnect()
> * state to Configured state to allow applications to free resources
> * and call release() before deleting the camera.
> */
> - if (state_ == Private::CameraRunning)
> - state_ = Private::CameraConfigured;
> + if (state_.load(std::memory_order_acquire) == Private::CameraRunning)
> + state_.store(Private::CameraConfigured, std::memory_order_release);
>
> disconnected_ = true;
> }
>
> void Camera::Private::setState(State state)
> {
> - state_ = state;
> + state_.store(state, std::memory_order_release);
> }
>
> /**
> @@ -389,12 +392,17 @@ void Camera::Private::setState(State state)
> * An application may start and stop a camera multiple times as long as it is
> * not released. The camera may also be reconfigured.
> *
> + * Functions that affect the camera state as defined below are generally not
> + * synchronized with each other by the Camera class. The caller is responsible
> + * for ensuring their synchronization if necessary.
> + *
> * \subsection Camera States
> *
> * To help manage the sequence of operations needed to control the camera a set
> * of states are defined. Each state describes which operations may be performed
> - * on the camera. Operations not listed in the state diagram are allowed in all
> - * states.
> + * on the camera. Performing an operation not allowed in the camera state
> + * results in undefined behaviour. Operations not listed at all in the state
> + * diagram are allowed in all states.
> *
> * \dot
> * digraph camera_state_machine {
> @@ -467,6 +475,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
>
> /**
> * \brief Retrieve the name of the camera
> + * \context This function is \threadsafe.
> * \return Name of the camera device
> */
> const std::string &Camera::name() const
> @@ -540,7 +549,9 @@ int Camera::exportFrameBuffers(Stream *stream,
> if (p_->activeStreams_.find(stream) == p_->activeStreams_.end())
> return -EINVAL;
>
> - return p_->pipe_->exportFrameBuffers(this, stream, buffers);
> + return p_->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,
> + ConnectionTypeBlocking, this, stream,
> + buffers);
> }
>
> int Camera::freeFrameBuffers(Stream *stream)
> @@ -549,7 +560,8 @@ int Camera::freeFrameBuffers(Stream *stream)
> if (ret < 0)
> return ret;
>
> - p_->pipe_->freeFrameBuffers(this, stream);
> + p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers,
> + ConnectionTypeBlocking, this, stream);
>
> return 0;
> }
> @@ -571,7 +583,8 @@ int Camera::freeFrameBuffers(Stream *stream)
> * Once exclusive access isn't needed anymore, the device should be released
> * with a call to the release() function.
> *
> - * This function affects the state of the camera, see \ref camera_operation.
> + * \context This function is \threadsafe. It may only be called when the camera
> + * is in the Available state as defined in \ref camera_operation.
> *
> * \return 0 on success or a negative error code otherwise
> * \retval -ENODEV The camera has been disconnected from the system
> @@ -579,6 +592,10 @@ int Camera::freeFrameBuffers(Stream *stream)
> */
> int Camera::acquire()
> {
> + /*
> + * No manual locking is required as PipelineHandler::lock() is
> + * thread-safe.
> + */
> int ret = p_->isAccessAllowed(Private::CameraAvailable);
> if (ret < 0)
> return ret == -EACCES ? -EBUSY : ret;
> @@ -600,7 +617,10 @@ int Camera::acquire()
> * Releasing the camera device allows other users to acquire exclusive access
> * with the acquire() function.
> *
> - * This function affects the state of the camera, see \ref camera_operation.
> + * \context This function may only be called when the camera is in the
> + * Available or Configured state as defined in \ref camera_operation, and shall
> + * be synchronized by the caller with other functions that affect the camera
> + * state.
> *
> * \return 0 on success or a negative error code otherwise
> * \retval -EBUSY The camera is running and can't be released
> @@ -634,6 +654,8 @@ int Camera::release()
> *
> * Camera controls remain constant through the lifetime of the camera.
> *
> + * \context This function is \threadsafe.
> + *
> * \return A ControlInfoMap listing the controls supported by the camera
> */
> const ControlInfoMap &Camera::controls()
> @@ -648,6 +670,8 @@ const ControlInfoMap &Camera::controls()
> * information describes among other things how many streams the camera
> * supports and the capabilities of each stream.
> *
> + * \context This function is \threadsafe.
> + *
> * \return An array of all the camera's streams.
> */
> const std::set<Stream *> &Camera::streams() const
> @@ -665,6 +689,8 @@ const std::set<Stream *> &Camera::streams() const
> * empty list of roles is valid, and will generate an empty configuration that
> * can be filled by the caller.
> *
> + * \context This function is \threadsafe.
> + *
> * \return A CameraConfiguration if the requested roles can be satisfied, or a
> * null pointer otherwise. The ownership of the returned configuration is
> * passed to the caller.
> @@ -715,7 +741,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
> * Exclusive access to the camera shall be ensured by a call to acquire() prior
> * to calling this function, otherwise an -EACCES error will be returned.
> *
> - * This function affects the state of the camera, see \ref camera_operation.
> + * \context This function may only be called when the camera is in the Acquired
> + * or Configured state as defined in \ref camera_operation, and shall be
> + * synchronized by the caller with other functions that affect the camera
> + * state.
> *
> * Upon return the StreamConfiguration entries in \a config are associated with
> * Stream instances which can be retrieved with StreamConfiguration::stream().
> @@ -754,7 +783,8 @@ int Camera::configure(CameraConfiguration *config)
>
> LOG(Camera, Info) << msg.str();
>
> - ret = p_->pipe_->configure(this, config);
> + ret = p_->pipe_->invokeMethod(&PipelineHandler::configure,
> + ConnectionTypeBlocking, this, config);
> if (ret)
> return ret;
>
> @@ -789,8 +819,8 @@ int Camera::configure(CameraConfiguration *config)
> * The ownership of the returned request is passed to the caller, which is
> * responsible for either queueing the request or deleting it.
> *
> - * This function shall only be called when the camera is in the Configured
> - * or Running state, see \ref camera_operation.
> + * \context This function is \threadsafe. It may only be called when the camera
> + * is in the Configured or Running state as defined in \ref camera_operation.
> *
> * \return A pointer to the newly created request, or nullptr on error
> */
> @@ -820,6 +850,9 @@ Request *Camera::createRequest(uint64_t cookie)
> * Ownership of the request is transferred to the camera. It will be deleted
> * automatically after it completes.
> *
> + * \context This function is \threadsafe. It may only be called when the camera
> + * is in the Running state as defined in \ref camera_operation.
> + *
> * \return 0 on success or a negative error code otherwise
> * \retval -ENODEV The camera has been disconnected from the system
> * \retval -EACCES The camera is not running so requests can't be queued
> @@ -832,6 +865,12 @@ int Camera::queueRequest(Request *request)
> if (ret < 0)
> return ret;
>
> + /*
> + * The camera state may chance until the end of the function. No locking
> + * is however needed as PipelineHandler::queueRequest() will handle
> + * this.
> + */
> +
> if (request->buffers().empty()) {
> LOG(Camera, Error) << "Request contains no buffers";
> return -EINVAL;
> @@ -846,7 +885,8 @@ int Camera::queueRequest(Request *request)
> }
> }
>
> - return p_->pipe_->queueRequest(this, request);
> + return p_->pipe_->invokeMethod(&PipelineHandler::queueRequest,
> + ConnectionTypeQueued, this, request);
> }
>
> /**
> @@ -856,7 +896,10 @@ int Camera::queueRequest(Request *request)
> * can queue requests to the camera to process and return to the application
> * until the capture session is terminated with \a stop().
> *
> - * This function affects the state of the camera, see \ref camera_operation.
> + * \context This function may only be called when the camera is in the
> + * Configured state as defined in \ref camera_operation, and shall be
> + * synchronized by the caller with other functions that affect the camera
> + * state.
> *
> * \return 0 on success or a negative error code otherwise
> * \retval -ENODEV The camera has been disconnected from the system
> @@ -874,10 +917,12 @@ int Camera::start()
> if (allocator_ && !allocator_->buffers(stream).empty())
> continue;
>
> - p_->pipe_->importFrameBuffers(this, stream);
> + p_->pipe_->invokeMethod(&PipelineHandler::importFrameBuffers,
> + ConnectionTypeDirect, this, stream);
> }
>
> - ret = p_->pipe_->start(this);
> + ret = p_->pipe_->invokeMethod(&PipelineHandler::start,
> + ConnectionTypeBlocking, this);
> if (ret)
> return ret;
>
> @@ -892,7 +937,9 @@ int Camera::start()
> * This method stops capturing and processing requests immediately. All pending
> * requests are cancelled and complete synchronously in an error state.
> *
> - * This function affects the state of the camera, see \ref camera_operation.
> + * \context This function may only be called when the camera is in the Running
> + * state as defined in \ref camera_operation, and shall be synchronized by the
> + * caller with other functions that affect the camera state.
> *
> * \return 0 on success or a negative error code otherwise
> * \retval -ENODEV The camera has been disconnected from the system
> @@ -908,13 +955,15 @@ int Camera::stop()
>
> p_->setState(Private::CameraConfigured);
>
> - p_->pipe_->stop(this);
> + p_->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
> + this);
>
> for (Stream *stream : p_->activeStreams_) {
> if (allocator_ && !allocator_->buffers(stream).empty())
> continue;
>
> - p_->pipe_->freeFrameBuffers(this, stream);
> + p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers,
> + ConnectionTypeBlocking, this, stream);
> }
>
> return 0;
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index a6c1e1fbae38..db0ec6ac00d7 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -17,6 +17,7 @@
>
> #include <ipa/ipa_interface.h>
> #include <libcamera/controls.h>
> +#include <libcamera/object.h>
> #include <libcamera/stream.h>
>
> namespace libcamera {
> @@ -51,7 +52,8 @@ private:
> CameraData &operator=(const CameraData &) = delete;
> };
>
> -class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>
> +class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>,
> + public Object
> {
> public:
> PipelineHandler(CameraManager *manager);
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list