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

[Jean-Michel Hautbois]

Hi Kieran, Laurent,

On 24/03/2021 21:19, Laurent Pinchart wrote:
> 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 ?

I like that latest idea better.
The comment in cio2BufferReady() is "If the buffer is cancelled" => but
isn't it indeed the request which is cancelled :-) ?