[libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3: Cancel unused buffers

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wed, Mar 24, 2021 at 08:06:24PM +0000, Kieran Bingham wrote:
> When the CIO2 returns a cancelled buffer, we will not queue buffers
> to the IMGU.
> 
> These buffers should be explicitly marked as cancelled to ensure
> the application knows there is no valid metadata or frame data
> provided in the buffer.
> 
> Provide a cancel() method on the FrameBuffer to allow explicitly
> cancelling a buffer.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/buffer.h           | 1 +
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 302fe3d3e86b..6557fb1e59b5 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -49,6 +49,7 @@ public:
>  	Request *request() const { return request_; }
>  	void setRequest(Request *request) { request_ = request; }
>  	const FrameMetadata &metadata() const { return metadata_; }
> +	void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }

Missing documentation ? I think we should document the function, but
also explained at a higher level in the FrameBuffer documentation how
it's meant to be used.

>  
>  	unsigned int cookie() const { return cookie_; }
>  	void setCookie(unsigned int cookie) { cookie_ = cookie; }
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index fc40dcb381ea..44bd5a9d042b 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1256,8 +1256,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>  
>  	/* If the buffer is cancelled force a complete of the whole request. */
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> -		for (auto it : request->buffers())
> -			pipe_->completeBuffer(request, it.second);
> +		for (auto it : request->buffers()) {
> +			FrameBuffer *b = it.second;
> +			b->cancel();
> +			pipe_->completeBuffer(request, b);
> +		}
>  
>  		frameInfos_.remove(info);
>  		pipe_->completeRequest(request);

Pipeline handlers never set the buffer status explicitly today, as this
is handled in V4L2VideoDevice. As you've found out, it causes issues we
requests whose buffers haven't been queued. Adding a
FrameBuffer::cancel() function makes the API a bit asymmetrical, which
bothers me a bit. There's also the issue that this function shouldn't be
visible to applications.

I wonder, would it be better to create a Request::cancel() function
instead, that will cancel all buffers automatically and complete the
request ?

-- 
Regards,

Laurent Pinchart