| Message ID | 20210303170426.189648-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, 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. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Personally I lean a bit towards Laurent's opinion that this should be rather be made fatal and pipeline handler fixed. Ater all this is not application facing and happens during pipeline development, so it won't disappoint any user but rather make sure developers notices this. If the two of you agree this is ok as an Error, plese add my tag Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > 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_) { > + LOG(V4L2, Error) << "No BufferCache available to queue."; > + return -ENOENT; > + } > + > ret = cache_->get(*buffer); > if (ret < 0) > return ret; > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@ideasonboard.com> > + return -ENOENT; > + } > + > ret = cache_->get(*buffer); > if (ret < 0) > return ret;
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@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@ideasonboard.com> > >> + return -ENOENT; >> + } >> + >> ret = cache_->get(*buffer); >> if (ret < 0) >> return ret; >
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_) { + LOG(V4L2, Error) << "No BufferCache available to queue."; + return -ENOENT; + } + ret = cache_->get(*buffer); if (ret < 0) return ret;
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. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/v4l2_videodevice.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+)