[libcamera-devel] [PATCH v3 09/11] libcamera: camera: Assert pipelines complete all requests

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Mar 25, 2021 at 01:42:29PM +0000, 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>
> 
> ---
> Note:
> 
> I wanted to make PipelineHandler::active() a const function, but then
> I'm unable to call it through invokeMethod(). Does invokeMethod support
> calling const functions? Or did I do something obviously wrong?

What's the compilation error ?

> ---
>  include/libcamera/internal/pipeline_handler.h |  1 +
>  src/libcamera/camera.cpp                      |  3 +++
>  src/libcamera/pipeline_handler.cpp            | 17 +++++++++++++++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 9bdda8f3b799..1410a06ebabe 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);
>  
>  	int queueRequest(Request *request);
>  
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 7f7956ba732f..99b01633ff5f 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -1086,6 +1086,9 @@ int Camera::stop()
>  	d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
>  			       this);
>  
> +	ASSERT(!d->pipe_->invokeMethod(&PipelineHandler::active,
> +				       ConnectionTypeBlocking, this));
> +

As the pipeline handler has stopped, there should be no race, can we
call

	ASSERT(!d->pipe_->active());

directly ? That would also solve your const problem :-)

>  	d->setState(Private::CameraConfigured);
>  
>  	return 0;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index e3d4975d9ffd..c1ea4374b445 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -357,6 +357,23 @@ 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.
> + *
> + * \context This function is called from the CameraManager thread.
> + *

This would need to be updated.

> + * \return True if there are pending requests, or false otherwise
> + */
> +bool PipelineHandler::active(const Camera *camera)
> +{
> +	const CameraData *data = cameraData(camera);
> +	return !data->queuedRequests_.empty();
> +}
> +
>  /**
>   * \fn PipelineHandler::queueRequest()
>   * \brief Queue a request

-- 
Regards,

Laurent Pinchart