[libcamera-devel] [PATCH v10 1/5] libcamera: camera: Make stop() idempotent

[Nícolas F. R. A. Prado]

Hi Paul,

On Wed, Jun 30, 2021 at 01:27:20PM +0900, paul.elder at ideasonboard.com wrote:
> Hello Nicolas,
> 
> On Tue, Jun 29, 2021 at 12:12:14PM -0300, Nícolas F. R. A. Prado wrote:
> > Make Camera::stop() idempotent so that it can be called in any state and
> > consecutive times. When called in any state other than CameraRunning, it
> > is a no-op. This simplifies the cleanup path for applications.
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > No changes in v10
> > 
> > No changes in v9
> > 
> > No changes in v8
> > 
> > No changes in v7
> > 
> > No changes in v6
> > 
> > Changes in v5:
> > - Thanks to Laurent:
> >   - Simplified isRunning()
> > 
> > Changes in v4:
> > - Thanks to Jacopo:
> >   - Added \todo in Camera::stop()
> > 
> > No changes in v3
> > 
> > Changes in v2:
> > - Suggested by Kieran:
> >   - Add new isRunning() function instead of relying on isAccessAllowed()
> > - Suggested by Laurent:
> >   - Make stop()'s description clearer
> > 
> >  src/libcamera/camera.cpp | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index de0123aeafca..29f2d91d05d3 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -348,6 +348,7 @@ public:
> >  		const std::set<Stream *> &streams);
> >  	~Private();
> >  
> > +	bool isRunning() const;
> >  	int isAccessAllowed(State state, bool allowDisconnected = false,
> >  			    const char *from = __builtin_FUNCTION()) const;
> >  	int isAccessAllowed(State low, State high,
> > @@ -389,6 +390,11 @@ static const char *const camera_state_names[] = {
> >  	"Running",
> >  };
> >  
> > +bool Camera::Private::isRunning() const
> > +{
> > +	return state_.load(std::memory_order_acquire) == CameraRunning;
> > +}
> > +
> >  int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,
> >  				     const char *from) const
> >  {
> > @@ -1062,9 +1068,10 @@ int Camera::start(const ControlList *controls)
> >   * This method stops capturing and processing requests immediately. All pending
> >   * requests are cancelled and complete synchronously in an error state.
> >   *
> > - * \context This function may only be called when the camera is in the Running
> > - * state as defined in \ref camera_operation, and shall be synchronized by the
> > - * caller with other functions that affect the camera state.
> > + * \context This function may be called in any camera state as defined in \ref
> > + * camera_operation, and shall be synchronized by the caller with other
> > + * functions that affect the camera state. If called when the camera isn't
> > + * running, it is a no-op.
> 
> The camera statemachine test is failing with this patch. I see that it
> is intended behavior, in which case the test needs to be updated
> accordingly.

Thanks for the feedback. I'll have that fixed for v11.

Nícolas

> 
> 
> Paul
> 
> >   *
> >   * \return 0 on success or a negative error code otherwise
> >   * \retval -ENODEV The camera has been disconnected from the system
> > @@ -1074,6 +1081,13 @@ int Camera::stop()
> >  {
> >  	Private *const d = LIBCAMERA_D_PTR();
> >  
> > +	/*
> > +	 * \todo Make calling stop() when not in 'Running' part of the state
> > +	 * machine rather than take this shortcut
> > +	 */
> > +	if (!d->isRunning())
> > +		return 0;
> > +
> >  	int ret = d->isAccessAllowed(Private::CameraRunning);
> >  	if (ret < 0)
> >  		return ret;
> > -- 
> > 2.32.0
> >