[libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert pipelines complete all requests

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tue, Apr 20, 2021 at 02:07:36PM +0100, Kieran Bingham wrote:
> When the camera manager calls stop on a pipeline, it is expected that
> the pipeline handler guarantees all requests are returned back to the
> application before the camera has stopped.
> 
> Ensure that this guarantee is met by providing an accessor on the
> pipeline handler to validate that all pending requests are removed.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/pipeline_handler.h |  1 +
>  src/libcamera/camera.cpp                      |  2 ++
>  src/libcamera/pipeline_handler.cpp            | 15 +++++++++++++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index c6454db6b2c4..27d45d8ffc20 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -80,6 +80,7 @@ public:
>  
>  	virtual int start(Camera *camera, const ControlList *controls) = 0;
>  	virtual void stop(Camera *camera) = 0;
> +	bool active(const Camera *camera) const;
>  
>  	void queueRequest(Request *request);
>  
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 763f3b9926fd..c3fc3dd91bd6 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -1084,6 +1084,8 @@ int Camera::stop()
>  	d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
>  			       this);
>  
> +	ASSERT(!d->pipe_->active(this));
> +
>  	d->setState(Private::CameraConfigured);
>  
>  	return 0;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 3b3150bdbbf7..8629e3578f02 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const
>   * \context This function is called from the CameraManager thread.
>   */
>  
> +/**
> + * \brief Determine if the camera has any requests pending
> + * \param[in] camera The camera to check
> + *
> + * This method determines if there are any requests queued to the pipeline
> + * awaiting processing.
> + *
> + * \return True if there are pending requests, or false otherwise
> + */
> +bool PipelineHandler::active(const Camera *camera) const

How about naming the function hasPendingRequests() to make it more
explicit ?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +{
> +	const CameraData *data = cameraData(camera);
> +	return !data->queuedRequests_.empty();
> +}
> +
>  /**
>   * \fn PipelineHandler::queueRequest()
>   * \brief Queue a request

-- 
Regards,

Laurent Pinchart