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

[Kieran Bingham]

Hi Laurent,

On 24/03/2021 20: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.

Well, I'll do the documentation when I know the patch won't get thrown
away ;-)

> 
>>  
>>  	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

Yes, which wasn't a problem when the Buffer is initialised to a state
which means it must be explicitly marked as successful.


> 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 ?

If one buffer is cancelled, should all buffers be cancelled?

What happens to other pending events like buffers which are still being
returned and may have succeeded, or will be cancelled.

Cancelling and automatically completing the request sounds like it would
cause some difficulties ... Just as we've seen with the IPU3 Frames
completing too soon...

-- 
Regards
--
Kieran