[libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel pending requests correctly

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Thu, Apr 08, 2021 at 05:51:01PM +0900, Hirokazu Honda wrote:
> IPU3CameraData stores requests that are not queued yet to a
> camera node. They should be reported as cancel upon
> PipelineHandlerIPU3::stop().
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/buffer.h           |  3 +++
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 620f8a66..72c62144 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -58,6 +58,9 @@ private:
>  
>  	friend class V4L2VideoDevice; /* Needed to update metadata_. */
>  
> +	/*! HACK! */
> +	friend class IPU3CameraData; /* Needed to update metadata_. */

Looks like we need a way to handle this cleanly :-) It may relate to the
discussion we had earlier, about the rework of the buffer and requests
state handling. I think a redesign of that mechanism, with a cleaner API
for completion, would be nice. Making the Request class inherit from
Extensible, with Request::Private being defined in
include/libcamera/internal/request.h and offering public functions for
the pipeline handlers may be part of the solution. The "Private" name in
the our Extensible design pattern (which implements the p-impl design
pattern) means private to applications, in the public API, but it can be
accessed within libcamera.

The rest of the patch looks good.

> +
>  	std::vector<Plane> planes_;
>  
>  	Request *request_;
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 462a0d9b..f89b8f3f 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -66,6 +66,7 @@ public:
>  	void cio2BufferReady(FrameBuffer *buffer);
>  	void paramBufferReady(FrameBuffer *buffer);
>  	void statBufferReady(FrameBuffer *buffer);
> +	void cancelPendingRequests();
>  	int queuePendingRequests();
>  
>  	CIO2Device cio2_;
> @@ -768,7 +769,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  	IPU3CameraData *data = cameraData(camera);
>  	int ret = 0;
>  
> -	data->pendingRequests_ = {};
> +	data->cancelPendingRequests();
>  
>  	data->ipa_->stop();
>  
> @@ -780,6 +781,22 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  	freeBuffers(camera);
>  }
>  
> +void IPU3CameraData::cancelPendingRequests()
> +{
> +	while (!pendingRequests_.empty()) {
> +		Request *request = pendingRequests_.front();
> +
> +		for (auto it : request->buffers()) {
> +			FrameBuffer *buffer = it.second;
> +			buffer->metadata_.status = FrameMetadata::FrameCancelled;
> +			pipe_->completeBuffer(request, buffer);
> +		}
> +
> +		pipe_->completeRequest(request);
> +		pendingRequests_.pop();
> +	}
> +}
> +
>  int IPU3CameraData::queuePendingRequests()
>  {
>  	while (!pendingRequests_.empty()) {

-- 
Regards,

Laurent Pinchart