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

[paul.elder at ideasonboard.com]

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.


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
>