[libcamera-devel] [PATCH 3/3] libcamera: v4l2_videodevice: Prevent queueing buffers without a cache

[Kieran Bingham]

Hi Laurent,

On 08/03/2021 15:05, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Mar 03, 2021 at 05:04:26PM +0000, Kieran Bingham wrote:
>> The v4l2 buffer cache allows us to map incoming buffers to an instance
>> of the V4L2 Buffer required to actually queue.
>>
>> If the cache_ is not available, then the buffers required to allow
>> queuing to a device have been released, and this indicates an issue at
>> the pipeline handler.
>>
>> This could be a common mistake, as it could happen if a pipeline handler
>> always requeues buffers to the device after they complete, without
>> checking if they are cancelled.
>>
>> That can then lead to trying to queue a buffer to a device which no
>> longer expects buffers, so add a loud message to indicate what has
>> happened but return an error without fatally crashing.
> 
> As discussed in replies to the cover letter, this should be a fatal
> error. I'm fine relaxing to check with an error instead until we fix the
> IPU3 pipeline handler to avoid blocking IPA development. Could the

I don't mind it being fatal (and indeed, we've long ago discussed
letting FATAL continue in non-debug) ... so that's fine too.


> commit message be reworded to explain that this is a fatal error as it
> denotes a bug in the pipeline handler that can also likely result in
> other undefined behaviour, but that it's currently downgraded to a
> normal error to avoid blocking development ?

It looks like the IPU3 Pipeline handler is stopping the IPA /after/
stopping the video devices.

Taht's a bug there, so patch to hit the list soon, and JM is just
testing it on his side.

If that resolves this, I'll simply make a v2 of this one as Fatal.

--
Kieran


> 
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/libcamera/v4l2_videodevice.cpp | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>> index c77e1aff7978..6129ce529afc 100644
>> --- a/src/libcamera/v4l2_videodevice.cpp
>> +++ b/src/libcamera/v4l2_videodevice.cpp
>> @@ -1390,6 +1390,16 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>>  	struct v4l2_buffer buf = {};
>>  	int ret;
>>  
>> +	/*
>> +	 * Pipeline handlers should not requeue buffers after releasing the
>> +	 * buffers on the device. Any occurence of this error should be fixed
>> +	 * in the pipeline handler directly.
>> +	 */
>> +	if (!cache_) {
> 
> 		/* \todo Make this a Fatal error */
> 
> ?
> 
>> +		LOG(V4L2, Error) << "No BufferCache available to queue.";
> 
> I'd write "No buffers allocated", or "Can't queue a buffer with no
> buffers allocated", as that's what is visible to pipeline handlers (the
> cache is an internal implementation detail).
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> +		return -ENOENT;
>> +	}
>> +
>>  	ret = cache_->get(*buffer);
>>  	if (ret < 0)
>>  		return ret;
> 

-- 
Regards
--
Kieran