[libcamera-devel] [PATCH v3 1/4] libcamera: camera: Make stop() idempotent

[Jacopo Mondi]

Hi Nicolas,

On Fri, May 14, 2021 at 10:16:49AM -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.

Sorry for the late reply and the contradictory advices you've been
given during the review of the previous iterations.

I've managed to discuss this with Laurent and Kieran and I hope to be
able to here summarize my understanding.

In a few words: please do not shortcut the camera state machine. It's
fine to allow stop() to be called in more states than just 'Running'
(probably on all states above 'Acquired') but that should be made part
of the state machine.

Probably adding these would be sufficient

         *   Acquired -> Available [label = "release()"];
         *   Acquired -> Configured [label = "configure()"];
       + *   Acquired -> Acquired [label = "stop()"];
         *
         *   Configured -> Available [label = "release()"];
         *   Configured -> Configured [label = "configure(), createRequest()"];
       + *   Configured -> Configured [label = "stop()"];
         *   Configured -> Running [label = "start()"];
         *
         *   Running -> Stopping [label = "stop()"];
         *   Stopping -> Configured;
       + *   Stopping -> Stopping [label = "stop()"];
         *   Running -> Running [label = "createRequest(), queueRequest()"];

The motivation is that allowing stop() to be called in any state and
return silently allows calls on a !acquired camera, and that
maintaining the camera state machine in place allows to later
re-introduce debug statements if deemed useful for developers (ie
"calling stop() on an already stopped camera").

All in all, there's a bit of work it might not be fair to require to
get the series in, so if you don't feel like biting the bullet, a
\todo comment will do.

>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
> 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 1340c266cc5f..1c6c76c7c01e 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;
> +}
> +
>  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,9 @@ int Camera::stop()
>  {
>  	Private *const d = LIBCAMERA_D_PTR();
>
> +	if (!d->isRunning())
> +		return 0;
> +
>  	int ret = d->isAccessAllowed(Private::CameraRunning);
>  	if (ret < 0)
>  		return ret;
> --
> 2.31.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel