[libcamera-devel] [PATCH v3 3/3] libcamera: camera: add state machine to control access from applications
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Mar 1 00:37:29 CET 2019
Hi Laurent,
Thanks for your feedback.
On 2019-02-28 23:39:47 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Thu, Feb 28, 2019 at 07:51:26PM +0100, Niklas Söderlund wrote:
> > There is a need to better control the order of operations an application
> > performs on a camera for it to function correctly. Add a basic state
> > machine to ensure applications perform operations on the camera in good
> > order.
> >
> > Internal to the Camera states are added; Available, Acquired,
> > Configured, Prepared and Running. Each state represents a higher state
> > of configuration of the camera ultimately leading to the highest state
> > where the camera is capturing frames. Each state supports a subset of
> > operations the application may perform.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/camera.h | 18 ++-
> > src/libcamera/camera.cpp | 266 +++++++++++++++++++++++++++++++------
> > 2 files changed, 238 insertions(+), 46 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 9c8ae01ed5c607f1..e5212cf05d221279 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -39,7 +39,7 @@ public:
> > Signal<Camera *> disconnected;
> >
> > int acquire();
> > - void release();
> > + int release();
> >
> > const std::set<Stream *> &streams() const;
> > std::map<Stream *, StreamConfiguration>
> > @@ -47,7 +47,7 @@ public:
> > int configureStreams(std::map<Stream *, StreamConfiguration> &config);
> >
> > int allocateBuffers();
> > - void freeBuffers();
> > + int freeBuffers();
> >
> > Request *createRequest();
> > int queueRequest(Request *request);
> > @@ -56,20 +56,30 @@ public:
> > int stop();
> >
> > private:
> > + enum State {
> > + CameraAvailable,
> > + CameraAcquired,
> > + CameraConfigured,
> > + CameraPrepared,
> > + CameraRunning,
> > + };
> > +
> > Camera(PipelineHandler *pipe, const std::string &name);
> > ~Camera();
> >
> > + bool stateBetween(State low, State high) const;
> > + bool stateIs(State state) const;
> > +
> > friend class PipelineHandler;
> > void disconnect();
> > - int exclusiveAccess();
> >
> > std::shared_ptr<PipelineHandler> pipe_;
> > std::string name_;
> > std::set<Stream *> streams_;
> > std::set<Stream *> activeStreams_;
> >
> > - bool acquired_;
> > bool disconnected_;
> > + State state_;
> > };
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 84b97b5c2ce94ecf..0b06c0d838b356f3 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -11,6 +11,7 @@
> >
> > #include "log.h"
> > #include "pipeline_handler.h"
> > +#include "utils.h"
> >
> > /**
> > * \file camera.h
> > @@ -42,6 +43,9 @@ LOG_DECLARE_CATEGORY(Camera)
> > * \class Camera
> > * \brief Camera device
> > *
> > + * \todo Add documentation for camera start timings. What exactly does the
> > + * camera expect the pipeline handler to do when start() is called?
> > + *
> > * The Camera class models a camera capable of producing one or more image
> > * streams from a single image source. It provides the main interface to
> > * configuring and controlling the device, and capturing image streams. It is
> > @@ -52,6 +56,78 @@ LOG_DECLARE_CATEGORY(Camera)
> > * created with the create() function which returns a shared pointer. The
> > * Camera constructors and destructor are private, to prevent instances from
> > * being constructed and destroyed manually.
> > + *
> > + * \section camera_operation Operating the Camera
> > + *
> > + * An application needs to perform a sequence of operations on a camera before
> > + * it is ready to process requests. The camera needs to be acquired, configured
> > + * and resources allocated or imported to prepare the camera for capture. Once
> > + * started the camera can process requests until it is stopped. When an
> > + * application is done with a camera all resources allocated needs to be freed
>
> s/needs/need/
>
> > + * and the camera released.
> > + *
> > + * An application may start and stop a camera multiple times as long as it is
> > + * not released. The camera may also be reconfigured provided that all
> > + * resources allocated are freed prior to the reconfiguration.
> > + *
> > + * \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.
> > + *
> > + * \dot
> > + * digraph camera_state_machine {
> > + * node [shape = doublecircle ]; Available;
> > + * node [shape = circle ]; Acquired;
> > + * node [shape = circle ]; Configured;
> > + * node [shape = circle ]; Prepared;
> > + * node [shape = circle ]; Running;
> > + *
> > + * Available -> Available [label = "release()"];
> > + * Available -> Acquired [label = "acquire()"];
> > + *
> > + * Acquired -> Available [label = "release()"];
> > + * Acquired -> Configured [label = "configureStreams()"];
> > + *
> > + * Configured -> Available [label = "release()"];
> > + * Configured -> Configured [label = "configureStreams()"];
> > + * Configured -> Prepared [label = "allocateBuffers()"];
> > + *
> > + * Prepared -> Configured [label = "freeBuffers()"];
> > + * Prepared -> Prepared [label = "createRequest()"];
> > + * Prepared -> Running [label = "start()"];
> > + *
> > + * Running -> Prepared [label = "stop()"];
> > + * Running -> Running [label = "createRequest(), queueRequest()"];
> > + * }
> > + * \enddot
> > + *
> > + * \subsubsection Available
> > + * The base state of a camera, an application can inspect the properties of the
> > + * camera to determine if it wishes to use it. If an application wishes to use
> > + * a camera it should acquire() it to proceed to the Acquired state.
> > + *
> > + * \subsubsection Acquired
> > + * In the acquired state an application has exclusive access to the camera and
> > + * may modify the camera's parameters to configure it and proceed to the
> > + * Configured state.
> > + *
> > + * \subsubsection Configured
> > + * The camera is configured and ready for the application to prepare it with
> > + * resources. The camera may be reconfigured multiple times until resources
> > + * are provided and the state progresses to Prepared.
> > + *
> > + * \subsubsection Prepared
> > + * The camera has been configured and provided with resources and is ready to be
> > + * started. The application may free the camera's resources to get back to the
> > + * Configured state or start it to progress to the Running state.
>
> s/start/start()/
>
> > + *
> > + * \subsubsection Running
> > + * The camera is running and ready to process requests queued by the
> > + * application. The camera remains in this state until it is stopped and moved
> > + * to the Prepared state.
> > */
> >
> > /**
> > @@ -116,17 +192,55 @@ const std::string &Camera::name() const
> > */
> >
> > Camera::Camera(PipelineHandler *pipe, const std::string &name)
> > - : pipe_(pipe->shared_from_this()), name_(name), acquired_(false),
> > - disconnected_(false)
> > + : pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),
> > + state_(CameraAvailable)
> > {
> > }
> >
> > Camera::~Camera()
> > {
> > - if (acquired_)
> > + if (!stateIs(CameraAvailable))
> > LOG(Camera, Error) << "Removing camera while still in use";
> > }
> >
> > +static const char *const camera_state_name[] = {
>
> Maybe camera_state_names ?
>
> > + "Available",
> > + "Acquired",
> > + "Configured",
> > + "Prepared",
> > + "Running",
> > +};
> > +
> > +bool Camera::stateBetween(State low, State high) const
> > +{
> > + if (state_ >= low && state_ <= high)
> > + return true;
> > +
> > + ASSERT(static_cast<unsigned int>(low) < ARRAY_SIZE(camera_state_name) &&
> > + static_cast<unsigned int>(high) < ARRAY_SIZE(camera_state_name));
> > +
> > + LOG(Camera, Debug) << "Camera in " << camera_state_name[state_]
> > + << " state trying operation requiring state between "
> > + << camera_state_name[low] << " and "
> > + << camera_state_name[high];
> > +
> > + return false;
> > +}
> > +
> > +bool Camera::stateIs(State state) const
> > +{
> > + if (state_ == state)
> > + return true;
> > +
> > + ASSERT(static_cast<unsigned int>(state) < ARRAY_SIZE(camera_state_name));
> > +
> > + LOG(Camera, Debug) << "Camera in " << camera_state_name[state_]
> > + << " state trying operation requiring state "
> > + << camera_state_name[state];
> > +
> > + return false;
> > +}
> > +
> > /**
> > * \brief Notify camera disconnection
> > *
> > @@ -135,11 +249,24 @@ Camera::~Camera()
> > * instance notifies the application by emitting the #disconnected signal, and
> > * ensures that all new calls to the application-facing Camera API return an
> > * error immediately.
> > + *
> > + * \todo: Deal with pending requests if the camera is disconnected in a
> > + * running state.
> > + * \todo: Update comment about Running state when importing buffers as well as
> > + * allocating them are supported.
>
> No need for colons after any of the \todo tags.
>
> > */
> > void Camera::disconnect()
> > {
> > LOG(Camera, Debug) << "Disconnecting camera " << name_;
> >
> > + /*
> > + * If the camera was running when the hardware was removed force the
> > + * state to Prepared to allow applications to call freeBuffers() and
> > + * release() before deleting the camera.
> > + */
> > + if (state_ == CameraRunning)
> > + state_ = CameraPrepared;
> > +
> > disconnected_ = true;
> > disconnected.emit(this);
> > }
> > @@ -155,16 +282,24 @@ void Camera::disconnect()
> > * Once exclusive access isn't needed anymore, the device should be released
> > * with a call to the release() function.
> > *
> > + * This function effects the state of the camera, see \ref camera_operation.
>
> s/effects/affects/ (through the whole file)
>
> > + *
> > * \todo Implement exclusive access across processes.
> > *
> > * \return 0 on success or a negative error code otherwise
> > + * \retval -ENODEV The camera has been disconnected from the system
> > + * \retval -EBUSY The camera is not free and can't be acquired by the caller
> > */
> > int Camera::acquire()
> > {
> > - if (acquired_)
> > + if (disconnected_)
> > + return -ENODEV;
> > +
> > + if (!stateIs(CameraAvailable))
> > return -EBUSY;
> >
> > - acquired_ = true;
> > + state_ = CameraAcquired;
> > +
> > return 0;
> > }
> >
> > @@ -173,10 +308,20 @@ int Camera::acquire()
> > *
> > * Releasing the camera device allows other users to acquire exclusive access
> > * with the acquire() function.
> > + *
> > + * This function effects the state of the camera, see \ref camera_operation.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EBUSY The camera is running and can't be released
> > */
> > -void Camera::release()
> > +int Camera::release()
> > {
> > - acquired_ = false;
> > + if (!stateBetween(CameraAvailable, CameraConfigured))
> > + return -EBUSY;
> > +
> > + state_ = CameraAvailable;
> > +
> > + return 0;
> > }
> >
> > /**
> > @@ -235,18 +380,22 @@ Camera::streamConfiguration(std::set<Stream *> &streams)
> > * 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 effects the state of the camera, see \ref camera_operation.
> > + *
> > * \return 0 on success or a negative error code otherwise
> > - * \retval -ENODEV The camera is not connected to any hardware
> > - * \retval -EACCES The user has not acquired exclusive access to the camera
> > + * \retval -ENODEV The camera has been disconnected from the system
> > + * \retval -EACCES The camera is not in a state where it can be configured
> > * \retval -EINVAL The configuration is not valid
> > */
> > int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > {
> > int ret;
> >
> > - ret = exclusiveAccess();
> > - if (ret)
> > - return ret;
> > + if (disconnected_)
> > + return -ENODEV;
> > +
> > + if (!stateBetween(CameraAvailable, CameraConfigured))
> > + return -EACCES;
> >
> > if (!config.size()) {
> > LOG(Camera, Error)
> > @@ -273,20 +422,28 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > stream->bufferPool().createBuffers(cfg.bufferCount);
> > }
> >
> > + state_ = CameraConfigured;
> > +
> > return 0;
> > }
> >
> > /**
> > * \brief Allocate buffers for all configured streams
> > + *
> > + * This function effects the state of the camera, see \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 in a state where buffers can be allocated
> > + * \retval -EINVAL The configuration is not valid
> > */
> > int Camera::allocateBuffers()
> > {
> > - int ret;
> > + if (disconnected_)
> > + return -ENODEV;
> >
> > - ret = exclusiveAccess();
> > - if (ret)
> > - return ret;
> > + if (!stateIs(CameraConfigured))
> > + return -EACCES;
> >
> > if (activeStreams_.empty()) {
> > LOG(Camera, Error)
> > @@ -295,7 +452,7 @@ int Camera::allocateBuffers()
> > }
> >
> > for (Stream *stream : activeStreams_) {
> > - ret = pipe_->allocateBuffers(this, stream);
> > + int ret = pipe_->allocateBuffers(this, stream);
> > if (ret) {
> > LOG(Camera, Error) << "Failed to allocate buffers";
> > freeBuffers();
> > @@ -303,14 +460,24 @@ int Camera::allocateBuffers()
> > }
> > }
> >
> > + state_ = CameraPrepared;
> > +
> > return 0;
> > }
> >
> > /**
> > * \brief Release all buffers from allocated pools in each stream
> > + *
> > + * This function effects the state of the camera, see \ref camera_operation.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EACCES The camera is not in a state where buffers can be freed
> > */
> > -void Camera::freeBuffers()
> > +int Camera::freeBuffers()
> > {
> > + if (!stateIs(CameraPrepared))
> > + return -EACCES;
> > +
> > for (Stream *stream : activeStreams_) {
> > if (!stream->bufferPool().count())
> > continue;
> > @@ -318,6 +485,10 @@ void Camera::freeBuffers()
> > pipe_->freeBuffers(this, stream);
> > stream->bufferPool().destroyBuffers();
> > }
> > +
> > + state_ = CameraConfigured;
> > +
> > + return 0;
> > }
> >
> > /**
> > @@ -333,7 +504,7 @@ void Camera::freeBuffers()
> > */
> > Request *Camera::createRequest()
>
> Should you update the documentation of this function ?
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Thanks, I have addressed all your comments, collected your tag and
pushed. Thanks for all your hard work reviewing, especially the
documentation bits.
>
> > {
> > - if (exclusiveAccess())
> > + if (disconnected_ || !stateBetween(CameraPrepared, CameraRunning))
> > return nullptr;
> >
> > return new Request(this);
> > @@ -351,16 +522,18 @@ Request *Camera::createRequest()
> > * automatically after it completes.
> > *
> > * \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
> > */
> > int Camera::queueRequest(Request *request)
> > {
> > - int ret;
> > + if (disconnected_)
> > + return -ENODEV;
> >
> > - ret = exclusiveAccess();
> > - if (ret)
> > - return ret;
> > + if (!stateIs(CameraRunning))
> > + return -EACCES;
> >
> > - ret = request->prepare();
> > + int ret = request->prepare();
> > if (ret) {
> > LOG(Camera, Error) << "Failed to prepare request";
> > return ret;
> > @@ -376,17 +549,29 @@ 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 effects the state of the camera, see \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 in a state where it can be started
> > */
> > int Camera::start()
> > {
> > - int ret = exclusiveAccess();
> > - if (ret)
> > - return ret;
> > + if (disconnected_)
> > + return -ENODEV;
> > +
> > + if (!stateIs(CameraPrepared))
> > + return -EACCES;
> >
> > LOG(Camera, Debug) << "Starting capture";
> >
> > - return pipe_->start(this);
> > + int ret = pipe_->start(this);
> > + if (ret)
> > + return ret;
> > +
> > + state_ = CameraRunning;
> > +
> > + return 0;
> > }
> >
> > /**
> > @@ -395,30 +580,27 @@ 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 effects the state of the camera, see \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 can't be stopped
> > */
> > int Camera::stop()
> > {
> > - int ret = exclusiveAccess();
> > - if (ret)
> > - return ret;
> > + if (disconnected_)
> > + return -ENODEV;
> > +
> > + if (!stateIs(CameraRunning))
> > + return -EACCES;
> >
> > LOG(Camera, Debug) << "Stopping capture";
> >
> > + state_ = CameraPrepared;
> > +
> > pipe_->stop(this);
> >
> > return 0;
> > }
> >
> > -int Camera::exclusiveAccess()
> > -{
> > - if (disconnected_)
> > - return -ENODEV;
> > -
> > - if (!acquired_)
> > - return -EACCES;
> > -
> > - return 0;
> > -}
> > -
> > } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list