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

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

Hi Laurent,

Em 2021-05-23 17:33, Laurent Pinchart escreveu:

> Hi Nícolas,
>
> Thank you for the patch.
>
> On Fri, May 21, 2021 at 10:30:50AM -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>
> > ---
> > 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 | 24 +++++++++++++++++++++---
> >  1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 1340c266cc5f..be0428ecbd46 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -347,6 +347,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,
> > @@ -388,6 +389,15 @@ static const char *const camera_state_names[] = {
> >  	"Running",
> >  };
> >  
> > +bool Camera::Private::isRunning() const
> > +{
> > +	State currentState = state_.load(std::memory_order_acquire);
> > +	if (currentState == CameraRunning)
> > +		return true;
> > +
> > +	return false;
>
> You could write this
>
> return state_.load(std::memory_order_acquire) == CameraRunning;
>
> If that's the only change needed (and assuming you agree) I can handle
> this when applying the series.

No need, I'll apply this for v5.

Thanks,
Nícolas

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +}
> > +
> >  int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,
> >  				     const char *from) const
> >  {
> > @@ -1061,9 +1071,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.
> >   *
> >   * \return 0 on success or a negative error code otherwise
> >   * \retval -ENODEV The camera has been disconnected from the system
> > @@ -1073,6 +1084,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;
>
> --
> Regards,
>
> Laurent Pinchart
>
> --
> To unsubscribe, send mail to kernel-unsubscribe at lists.collabora.co.uk.