[libcamera-devel,v3,3/6] libcamera: camera: Add acquire() and release()

413 diff mbox series

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/camera.h |  5 +++++
 src/libcamera/camera.cpp   | 39 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletion(-)

Hi All,

On 27/01/2019 00:22, Niklas Söderlund wrote:
> From: Laurent Pinchart <laurent.pinchart@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.



> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@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  :-)


> + *
> + * \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 */
>

Hi Kieran,

On Mon, Jan 28, 2019 at 10:58:04AM +0000, Kieran Bingham wrote:
> On 27/01/2019 00:22, Niklas Söderlund wrote:
> > From: Laurent Pinchart <laurent.pinchart@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.

Sounds good to me.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@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  :-)

We'll have to investigate how to implement this. It will be fun :-)

> > + *
> > + * \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 */

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@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@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@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

On 2019-01-28 23:13:13 +0100, Niklas Söderlund wrote:
> Hi Laurent,

s/Laurent/Kieran/

Sorry about that :-)

> 
> 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@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@ideasonboard.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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

On 29/01/2019 01:39, Niklas Söderlund wrote:
> On 2019-01-28 23:13:13 +0100, Niklas Söderlund wrote:
>> Hi Laurent,
> 
> s/Laurent/Kieran/
> 
> Sorry about that :-)


It's fine, I'm used to it by now :P
(I think Laurent did the same between us, and I'm sure I'll manage to do
the same sometime hehe)