[libcamera-devel] [PATCH 5/8] libcamera: camera: add state machine to control access from applications
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Feb 26 20:00:19 CET 2019
Hi Jacopo,
Thanks for your feedback.
On 2019-02-26 18:13:43 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> thanks for this big work
>
> On Tue, Feb 26, 2019 at 03:18:54AM +0100, Niklas Söderlund wrote:
> > There is a need to better control the order of operations an application
> > perform 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 four states are added; Disconnected, Free,
> > Acquired 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 can perform.
> >
> > * Disconnected
> > Is the lowest state a camera can be in. It indicates that the camera
> > have been disconnected from the system and the only operations an
> > application shall do at this point is clean up and release the camera so
> > it can be removed from libcamera as well.
> >
> > * Free
> > Is the base state of a camera, an application can inspect the properties
> > of the camera to determine if it wish to use it or not. If an
> > application wish to use a camera it should acquire it to proceed to the
> > next state.
> >
> > * Acquired
> > When an application have acquired a camera it have exclusive access to
> > it and can modify the cameras parameters to prepare it for capturing.
> > Once the application is done configure the camera it may be started to
> > progress to the running state. Once the camera is started it can not be
> > reconfigured until it's stopped.
> >
> > * Running
> > In this state the camera is running and able to process requests queued
> > to it by the application. Once an application finish capturing the
> > camera shall be stopped to change the state to acquired where it can be
> > reconfigured or released.
> >
>
> Before going into the details of the patch, I wonder if the states you
> have described here capture all the possible interactions with a
> camera object, in particulare regarding confgiurations of streams.
>
> I would have expected to see a "Configured" state, which unlock access
> to the stream start/stop methods and to buffer allocation, and which
> might be used to refuse other configurations, if there is an active
> one.
In my mind it captures all current interactions with the camera and
covers our discussions in Brussels. I agree it might be extended in the
future to also include a configured state. For now I feel this covers
the behavior discussed and agreed upon.
>
> My mental model of this is that once a camera's streams have been
> configured, the configuration should be deleted (by passing, say, an
> empty configuration vector) to make sure applications have an explicit
> "unconfigure" step to prevent overriding a configuration (and possibly
> centralize buffer release, which should not be done explicitly).
This seems messy and hard for applications to use. We might need it at
some point, but for now all this change is make the current behavior a
bit more strict. If we need this I say we should do this on top of this
change.
>
> It seems to me here a configuration could be replaced freely, and you
> can call free and allocte buffers on a camera which has just been
> acquired (but not configured). Is this intentional?
Yes. However with all patches in this series applied more checks are
added then just simply converting the existing ones to the state machine
logic. But as you point out above even more checks might be needed and
added in the future.
>
> Thanks
> j
>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/camera.h | 14 ++++-
> > src/libcamera/camera.cpp | 110 +++++++++++++++++++++++--------------
> > 2 files changed, 80 insertions(+), 44 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index bf70255a6a5ea364..8c8545b074e8ae13 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -55,20 +55,28 @@ public:
> > int stop();
> >
> > private:
> > + enum State {
> > + Disconnected,
> > + Free,
> > + Acquired,
> > + Running,
> > + };
> > +
> > Camera(PipelineHandler *pipe, const std::string &name);
> > ~Camera();
> >
> > + bool stateIs(State state) const;
> > + bool stateIsAtleast(State state) const;
> > +
> > friend class PipelineHandler;
> > void disconnect();
> > - int exclusiveAccess();
> >
> > std::shared_ptr<PipelineHandler> pipe_;
> > std::string name_;
> > std::vector<Stream *> streams_;
> > std::vector<Stream *> activeStreams_;
> >
> > - bool acquired_;
> > - bool disconnected_;
> > + State state_;
> > };
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index d4258fe3c7551af3..c50b14bbd904fc1c 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -116,17 +116,47 @@ 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), state_(Free)
> > {
> > }
> >
> > Camera::~Camera()
> > {
> > - if (acquired_)
> > + if (state_ > Free)
> > LOG(Camera, Error) << "Removing camera while still in use";
> > }
> >
> > +static const std::string stateNames[] = {
> > + "Disconnected",
> > + "Free",
> > + "Acquired",
> > + "Running",
> > +};
> > +
> > +bool Camera::stateIs(State state) const
> > +{
> > + if (state_ == state)
> > + return true;
> > +
> > + LOG(Camera, Error) << "Camera in " << stateNames[state_]
> > + << " state trying operation requiering "
> > + << stateNames[state] << " state";
> > +
> > + return false;
> > +}
> > +
> > +bool Camera::stateIsAtleast(State state) const
> > +{
> > + if (state_ >= state)
> > + return true;
> > +
> > + LOG(Camera, Error) << "Camera in " << stateNames[state_]
> > + << " state trying operation requiering at least "
> > + << stateNames[state] << " state";
> > +
> > + return false;
> > +}
> > +
> > /**
> > * \brief Notify camera disconnection
> > *
> > @@ -140,7 +170,7 @@ void Camera::disconnect()
> > {
> > LOG(Camera, Debug) << "Disconnecting camera " << name_;
> >
> > - disconnected_ = true;
> > + state_ = Disconnected;
> > disconnected.emit(this);
> > }
> >
> > @@ -162,10 +192,11 @@ void Camera::disconnect()
> > */
> > int Camera::acquire()
> > {
> > - if (acquired_)
> > + if (!stateIs(Free))
> > return -EBUSY;
> >
> > - acquired_ = true;
> > + state_ = Acquired;
> > +
> > return 0;
> > }
> >
> > @@ -177,7 +208,10 @@ int Camera::acquire()
> > */
> > void Camera::release()
> > {
> > - acquired_ = false;
> > + if (!stateIs(Acquired))
> > + return;
> > +
> > + state_ = Free;
> > }
> >
> > /**
> > @@ -191,6 +225,9 @@ void Camera::release()
> > */
> > const std::vector<Stream *> &Camera::streams() const
> > {
> > + if (!stateIsAtleast(Free))
> > + std::vector<Stream *>{};
> > +
> > return streams_;
> > }
> >
> > @@ -213,7 +250,7 @@ const std::vector<Stream *> &Camera::streams() const
> > std::map<Stream *, StreamConfiguration>
> > Camera::streamConfiguration(std::vector<Stream *> &streams)
> > {
> > - if (disconnected_ || !streams.size())
> > + if (!stateIsAtleast(Free) || !streams.size())
> > return std::map<Stream *, StreamConfiguration>{};
> >
> > return pipe_->streamConfiguration(this, streams);
> > @@ -244,9 +281,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > {
> > int ret;
> >
> > - ret = exclusiveAccess();
> > - if (ret)
> > - return ret;
> > + if (!stateIs(Acquired))
> > + return -EACCES;
> >
> > if (!config.size()) {
> > LOG(Camera, Error)
> > @@ -284,11 +320,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > */
> > int Camera::allocateBuffers()
> > {
> > - int ret;
> > -
> > - ret = exclusiveAccess();
> > - if (ret)
> > - return ret;
> > + if (!stateIs(Acquired))
> > + return -EACCES;
> >
> > if (activeStreams_.empty()) {
> > LOG(Camera, Error)
> > @@ -297,7 +330,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();
> > @@ -313,6 +346,9 @@ int Camera::allocateBuffers()
> > */
> > void Camera::freeBuffers()
> > {
> > + if (!stateIs(Acquired))
> > + return;
> > +
> > for (Stream *stream : activeStreams_) {
> > if (!stream->bufferPool().count())
> > continue;
> > @@ -336,7 +372,7 @@ void Camera::freeBuffers()
> > */
> > Request *Camera::createRequest()
> > {
> > - if (exclusiveAccess())
> > + if (!stateIsAtleast(Acquired))
> > return nullptr;
> >
> > return new Request(this);
> > @@ -358,13 +394,10 @@ Request *Camera::createRequest()
> > */
> > int Camera::queueRequest(Request *request)
> > {
> > - int ret;
> > + if (!stateIs(Running))
> > + return -EACCES;
> >
> > - ret = exclusiveAccess();
> > - if (ret)
> > - return ret;
> > -
> > - ret = request->prepare();
> > + int ret = request->prepare();
> > if (ret) {
> > LOG(Camera, Error) << "Failed to prepare request";
> > return ret;
> > @@ -385,13 +418,18 @@ int Camera::queueRequest(Request *request)
> > */
> > int Camera::start()
> > {
> > - int ret = exclusiveAccess();
> > - if (ret)
> > - return ret;
> > + if (!stateIs(Acquired))
> > + return -EACCES;
> >
> > LOG(Camera, Debug) << "Starting capture";
> >
> > - return pipe_->start(this);
> > + int ret = pipe_->start(this);
> > + if (ret)
> > + return ret;
> > +
> > + state_ = Running;
> > +
> > + return 0;
> > }
> >
> > /**
> > @@ -405,24 +443,14 @@ int Camera::start()
> > */
> > int Camera::stop()
> > {
> > - int ret = exclusiveAccess();
> > - if (ret)
> > - return ret;
> > + if (!stateIs(Running))
> > + return -EACCES;
> >
> > LOG(Camera, Debug) << "Stopping capture";
> >
> > pipe_->stop(this);
> >
> > - return 0;
> > -}
> > -
> > -int Camera::exclusiveAccess()
> > -{
> > - if (disconnected_)
> > - return -ENODEV;
> > -
> > - if (!acquired_)
> > - return -EACCES;
> > + state_ = Acquired;
> >
> > return 0;
> > }
> > --
> > 2.20.1
> >
> > _______________________________________________
> > 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