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

[Laurent Pinchart]

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

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

Laurent Pinchart