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

[Niklas Söderlund]

Hi Kieran,

Thanks for your feedback.

On 2019-01-29 09:51:54 +0000, Kieran Bingham wrote:
> Hi Niklas, (/Laurent,)
> 
> On 29/01/2019 02:00, Niklas Söderlund wrote:
> > From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > 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 at ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> With capitalisation comments considered,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

I have collected your tag while leaving the capitalisation as is for now 
:-) Lets discuss this in patch the first patch in this series and reach 
a conclusion if we should capitalise or not. I think whatever we decide 
there is an opportunity to align this in the existing code.

> 
> 
> 
> > ---
> >  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
> 
> Back on my Doxygen Capitalisation Front....
> 
> I think we should be capitalising any use of a class name so that
> Doxygen links the types and makes it clear that we are referencing an
> object of that type.
> 
> 
> > + *
> > + * 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.
> > + *
> > + * \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
> 
> s/camera/Camera/
> 
> > + *
> > + * Releasing the camera device allows other users to acquire exclusive access
> 
> s/camera/Camera/
> 
> > + * with the acquire() function.
> > + */
> > +void Camera::release()
> > +{
> > +	acquired_ = false;
> > +}
> > +
> >  } /* namespace libcamera */
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund