| Message ID | 20210312054727.852622-6-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
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. > > 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 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; >
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@jmondi.org> > > > > 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 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, Thank you for the patch. On Fri, Mar 12, 2021 at 05:54:12AM +0000, Kieran Bingham wrote: > On 12/03/2021 05:47, Kieran Bingham wrote: > > The v4l2 buffer cache allows us to map incoming buffers to an instance s/v4l2/V4L2/ ? > > of the V4L2 Buffer required to actually queue. s/Buffer/buffer/ as there's no V4L2Buffer class ? > > 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. > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@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;
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;
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(+)