[libcamera-devel] [PATCH v2 5/8] libcamera: v4l2_videodevice: Prevent queueing buffers without a cache

[Jacopo Mondi]

Hi Kieran,

On Fri, Mar 12, 2021 at 05:54:12AM +0000, Kieran Bingham wrote:
> Hi Kieran,
>
> On 12/03/2021 05:47, 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.
>
> Perhaps you forgot to update this commit message.
>
> I'd recommend change the last paragraph to the following when/if committing:
>
> Catch any invalid queueing of buffers to the V4L2 video device when
> resources have been released by adding a Fatal log message to highlight
> the error during development.
>

With the commit message updated, and Niklas' concern about other
pipelines not triggering this verified
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

> >
> > 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 cb52d4cea3f6..12c09dc7578d 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1391,6 +1391,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_) {
> > +		LOG(V4L2, Fatal) << "No BufferCache available to queue.";
> > +		return -ENOENT;
> > +	}
> > +
> >  	ret = cache_->get(*buffer);
> >  	if (ret < 0)
> >  		return ret;
> >
>
> --
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel