[libcamera-devel] [PATCH v3 3/6] libcamera: camera: Add acquire() and release()
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Jan 28 23:13:13 CET 2019
Hi Laurent,
Thanks for your feedback.
On 2019-01-28 10:58:04 +0000, Kieran Bingham wrote:
> Hi All,
>
> On 27/01/2019 00:22, Niklas Söderlund wrote:
> > From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Can we have at least a brief excerpt from the brief below here in the
> commit message so that our git-log is valid and readable independently?
>
> How about:
>
> Exclusive access must be obtained before performing operations that
> change the device state. Define an internal flag to track ownership and
> provide a means of protecting functions that change device configuration.
I agree with you that adding this to the commit messages adds value,
added per your suggestion. Thanks!
>
>
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/camera.h | 5 +++++
> > src/libcamera/camera.cpp | 39 +++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index a2ded62de94814c4..7e358f8c0aa093cf 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -29,6 +29,9 @@ public:
> >
> > Signal<Camera *> disconnected;
> >
> > + int acquire();
> > + void release();
> > +
> > private:
> > Camera(PipelineHandler *pipe, const std::string &name);
> > ~Camera();
> > @@ -38,6 +41,8 @@ private:
> >
> > std::shared_ptr<PipelineHandler> pipe_;
> > std::string name_;
> > +
> > + bool acquired_;
> > };
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 9cec289282e4797b..500976b237bcbd2d 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -102,12 +102,14 @@ const std::string &Camera::name() const
> > */
> >
> > Camera::Camera(PipelineHandler *pipe, const std::string &name)
> > - : pipe_(pipe->shared_from_this()), name_(name)
> > + : pipe_(pipe->shared_from_this()), name_(name), acquired_(false)
> > {
> > }
> >
> > Camera::~Camera()
> > {
> > + if (acquired_)
> > + LOG(Camera, Error) << "Removing camera while still in use";
> > }
> >
> > /**
> > @@ -127,4 +129,39 @@ void Camera::disconnect()
> > disconnected.emit(this);
> > }
> >
> > +/**
> > + * \brief Acquire the camera device for exclusive access
> > + *
> > + * After opening the device with open(), exclusive access must be obtained
> > + * before performing operations that change the device state. This function is
> > + * not blocking, if the device has already been acquired (by the same or another
> > + * process) the -EBUSY error code is returned.
> > + *
> > + * Once exclusive access isn't needed anymore, the device should be released
> > + * with a call to the release() function.
> > + *
> > + * \todo Implement exclusive access across processes.
>
> Aha - ok - I was going to say "but this code doesn't protect the device"
> ... and now I see the todo - so I'll let it pass.
>
> When we implement this - it would be nice to store some information
> about the process who has acquired the device so that we can report
> "who" has the device if a request fails.
>
> I expect this will be useful when we debug the 'Who has locked the
> device and not released it' issues ... or if a process hangs with the
> acquire lock.
>
> Ideally we want to make this work in some way that if something kills
> the process the lock is also released - as it will affect other processes.
>
> Perhaps if the 'owner' doesn't exist any more an acquire might succeed.
> Who knows :-)
I'm both scared and thrilled to tackle this issue, sometime in the
future. But yes storing the owner who locked it I think is a good idea
to aid future debugging.
Thinking out loud, maybe we can store the pid of the owner inside the
lock file. Then we can 'check' if a lock is valid by verifying that the
pid is still alive. Well lets cross this bridge once we get there.
>
>
> > + *
> > + * \return 0 on success or a negative error code on error.
> > + */
> > +int Camera::acquire()
> > +{
> > + if (acquired_)
> > + return -EBUSY;
> > +
> > + acquired_ = true;
> > + return 0;
> > +}
> > +
> > +/**
> > + * \brief Release exclusive access to the camera device
> > + *
> > + * Releasing the camera device allows other users to acquire exclusive access
> > + * with the acquire() function.
> > + */
> > +void Camera::release()
> > +{
> > + acquired_ = false;
> > +}
> > +
> > } /* namespace libcamera */
> >
>
> --
> Regards
> --
> Kieran
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list