| Message ID | 20220322092257.2713521-7-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Tue, Mar 22, 2022 at 09:22:55AM +0000, Naushir Patuck via libcamera-devel wrote: > If the device is in the process of being stopped (i.e. Stopping state), any > call to queueBuffer() must fail. This is to ensure the integrity of the buffer > queue, as it gets cleared at the end of streamOff. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/v4l2_videodevice.cpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 9cea6a608660..1516be129d73 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1491,6 +1491,9 @@ int V4L2VideoDevice::releaseBuffers() > * The best available V4L2 buffer is picked for \a buffer using the V4L2 buffer > * cache. > * > + * Note that \a queueBuffer will fail if the device is in the process of being If you write it queueBuffer() doxygen should recognize the function call and generate proper links, you can drop the \a. Same below. > + * stopped from a streaming state through the \a streamOff function. > + * > * \return 0 on success or a negative error code otherwise > */ > int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) > @@ -1499,6 +1502,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) > struct v4l2_buffer buf = {}; > int ret; > > + if (state_ == State::Stopping) { > + LOG(V4L2, Error) << "Device is in a stopping state."; Does this currently happen ? I assume it does, causing the unit test failures in v3 of the series. Is it a bug on the application side, or a problem elsewhere ? Are the cam, qcam and simple-cam applications affected ? We should not have any error message printed under normal operation conditions, so I'd like to at least know what needs to be fixed. > + return -EPERM; I'm tempted to use -ESHUTDOWN as a more precise alternative. Any opinion from anyone ? > + } > + > /* > * Pipeline handlers should not requeue buffers after releasing the > * buffers on the device. Any occurence of this error should be fixed
Hi Laurent, Thank you for your feedback. On Tue, 22 Mar 2022 at 20:16, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Tue, Mar 22, 2022 at 09:22:55AM +0000, Naushir Patuck via > libcamera-devel wrote: > > If the device is in the process of being stopped (i.e. Stopping state), > any > > call to queueBuffer() must fail. This is to ensure the integrity of the > buffer > > queue, as it gets cleared at the end of streamOff. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/v4l2_videodevice.cpp | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp > b/src/libcamera/v4l2_videodevice.cpp > > index 9cea6a608660..1516be129d73 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -1491,6 +1491,9 @@ int V4L2VideoDevice::releaseBuffers() > > * The best available V4L2 buffer is picked for \a buffer using the > V4L2 buffer > > * cache. > > * > > + * Note that \a queueBuffer will fail if the device is in the process > of being > > If you write it queueBuffer() doxygen should recognize the function call > and generate proper links, you can drop the \a. Same below. > > > + * stopped from a streaming state through the \a streamOff function. > > + * > > * \return 0 on success or a negative error code otherwise > > */ > > int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) > > @@ -1499,6 +1502,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer > *buffer) > > struct v4l2_buffer buf = {}; > > int ret; > > > > + if (state_ == State::Stopping) { > > + LOG(V4L2, Error) << "Device is in a stopping state."; > > Does this currently happen ? I assume it does, causing the unit test > failures in v3 of the series. Is it a bug on the application side, or a > problem elsewhere ? Are the cam, qcam and simple-cam applications > affected ? We should not have any error message printed under normal > operation conditions, so I'd like to at least know what needs to be > fixed. > This does occur, but only in the v4l2_m2mdevice test (that I've found). Applications are not directly affected, as this is on the interface between the pipeline handlers and the v4l2 device. The RPi pipeline handler does not re-queue buffers into the device when we are stopping. I've had a brief look at the ipu3 and rkisp ones, and I don't think they do either, but it would be good for someone else to confirm. Kieran and I briefly talked about what to do with the v4l2_m2mdevice test, it would be an easy fix to correct the behavior and get rid of the above log messages popping out with this change. > > + return -EPERM; > > I'm tempted to use -ESHUTDOWN as a more precise alternative. Any opinion > from anyone ? > I am fine with ESHUTDOWN if others think it is more appropriate. Regards, Naush > > > + } > > + > > /* > > * Pipeline handlers should not requeue buffers after releasing the > > * buffers on the device. Any occurence of this error should be > fixed > > -- > Regards, > > Laurent Pinchart >
Quoting Naushir Patuck via libcamera-devel (2022-03-23 09:03:44) > Hi Laurent, > > Thank you for your feedback. > > On Tue, 22 Mar 2022 at 20:16, Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > > > Hi Naush, > > > > Thank you for the patch. > > > > On Tue, Mar 22, 2022 at 09:22:55AM +0000, Naushir Patuck via > > libcamera-devel wrote: > > > If the device is in the process of being stopped (i.e. Stopping state), > > any > > > call to queueBuffer() must fail. This is to ensure the integrity of the > > buffer > > > queue, as it gets cleared at the end of streamOff. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/v4l2_videodevice.cpp | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp > > b/src/libcamera/v4l2_videodevice.cpp > > > index 9cea6a608660..1516be129d73 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -1491,6 +1491,9 @@ int V4L2VideoDevice::releaseBuffers() > > > * The best available V4L2 buffer is picked for \a buffer using the > > V4L2 buffer > > > * cache. > > > * > > > + * Note that \a queueBuffer will fail if the device is in the process > > of being > > > > If you write it queueBuffer() doxygen should recognize the function call > > and generate proper links, you can drop the \a. Same below. > > > > > + * stopped from a streaming state through the \a streamOff function. > > > + * > > > * \return 0 on success or a negative error code otherwise > > > */ > > > int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) > > > @@ -1499,6 +1502,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer > > *buffer) > > > struct v4l2_buffer buf = {}; > > > int ret; > > > > > > + if (state_ == State::Stopping) { > > > + LOG(V4L2, Error) << "Device is in a stopping state."; > > > > Does this currently happen ? I assume it does, causing the unit test > > failures in v3 of the series. Is it a bug on the application side, or a > > problem elsewhere ? Are the cam, qcam and simple-cam applications > > affected ? We should not have any error message printed under normal > > operation conditions, so I'd like to at least know what needs to be > > fixed. > > > > This does occur, but only in the v4l2_m2mdevice test (that I've found). > Applications > are not directly affected, as this is on the interface between the pipeline > handlers > and the v4l2 device. The RPi pipeline handler does not re-queue buffers > into the > device when we are stopping. I've had a brief look at the ipu3 and rkisp > ones, and > I don't think they do either, but it would be good for someone else to > confirm. > > Kieran and I briefly talked about what to do with the v4l2_m2mdevice test, > it would > be an easy fix to correct the behavior and get rid of the above log > messages popping > out with this change. Doesn't it happen on the async capture test too? Indeed as I understand it - this isn't something that can happen from a 'libcamera' application as it is already guarded by the stopping state on the Camera state machine. But ... it is an issue if someone uses the V4L2VideoDevice directly (such as our tests), and any future expansion of the V4L2VideoDevice class - so I would rather see this fix/check here to be sure it doesn't cause issues with future users of the V4L2VideoDevice. > > > > > + return -EPERM; > > > > I'm tempted to use -ESHUTDOWN as a more precise alternative. Any opinion > > from anyone ? > > > > I am fine with ESHUTDOWN if others think it is more appropriate. ESHUTDOWN 108 Cannot send after transport endpoint shutdown Sounds reasonable enough to me. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Regards, > Naush > > > > > > > + } > > > + > > > /* > > > * Pipeline handlers should not requeue buffers after releasing the > > > * buffers on the device. Any occurence of this error should be > > fixed > > > > -- > > Regards, > > > > Laurent Pinchart > >
On Wed, 23 Mar 2022 at 09:54, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck via libcamera-devel (2022-03-23 09:03:44) > > Hi Laurent, > > > > Thank you for your feedback. > > > > On Tue, 22 Mar 2022 at 20:16, Laurent Pinchart < > > laurent.pinchart@ideasonboard.com> wrote: > > > > > Hi Naush, > > > > > > Thank you for the patch. > > > > > > On Tue, Mar 22, 2022 at 09:22:55AM +0000, Naushir Patuck via > > > libcamera-devel wrote: > > > > If the device is in the process of being stopped (i.e. Stopping > state), > > > any > > > > call to queueBuffer() must fail. This is to ensure the integrity of > the > > > buffer > > > > queue, as it gets cleared at the end of streamOff. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/libcamera/v4l2_videodevice.cpp | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp > > > b/src/libcamera/v4l2_videodevice.cpp > > > > index 9cea6a608660..1516be129d73 100644 > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > @@ -1491,6 +1491,9 @@ int V4L2VideoDevice::releaseBuffers() > > > > * The best available V4L2 buffer is picked for \a buffer using the > > > V4L2 buffer > > > > * cache. > > > > * > > > > + * Note that \a queueBuffer will fail if the device is in the > process > > > of being > > > > > > If you write it queueBuffer() doxygen should recognize the function > call > > > and generate proper links, you can drop the \a. Same below. > > > > > > > + * stopped from a streaming state through the \a streamOff function. > > > > + * > > > > * \return 0 on success or a negative error code otherwise > > > > */ > > > > int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) > > > > @@ -1499,6 +1502,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer > > > *buffer) > > > > struct v4l2_buffer buf = {}; > > > > int ret; > > > > > > > > + if (state_ == State::Stopping) { > > > > + LOG(V4L2, Error) << "Device is in a stopping state."; > > > > > > Does this currently happen ? I assume it does, causing the unit test > > > failures in v3 of the series. Is it a bug on the application side, or a > > > problem elsewhere ? Are the cam, qcam and simple-cam applications > > > affected ? We should not have any error message printed under normal > > > operation conditions, so I'd like to at least know what needs to be > > > fixed. > > > > > > > This does occur, but only in the v4l2_m2mdevice test (that I've found). > > Applications > > are not directly affected, as this is on the interface between the > pipeline > > handlers > > and the v4l2 device. The RPi pipeline handler does not re-queue buffers > > into the > > device when we are stopping. I've had a brief look at the ipu3 and rkisp > > ones, and > > I don't think they do either, but it would be good for someone else to > > confirm. > > > > Kieran and I briefly talked about what to do with the v4l2_m2mdevice > test, > > it would > > be an easy fix to correct the behavior and get rid of the above log > > messages popping > > out with this change. > > Doesn't it happen on the async capture test too? > Sorry, capture_async does also behave in the same way, so we do get log messages on that run. > > Indeed as I understand it - this isn't something that can happen from a > 'libcamera' application as it is already guarded by the stopping state > on the Camera state machine. > > But ... it is an issue if someone uses the V4L2VideoDevice directly > (such as our tests), and any future expansion of the V4L2VideoDevice > class - so I would rather see this fix/check here to be sure it doesn't > cause issues with future users of the V4L2VideoDevice. > > > > > > > > > > + return -EPERM; > > > > > > I'm tempted to use -ESHUTDOWN as a more precise alternative. Any > opinion > > > from anyone ? > > > > > > > I am fine with ESHUTDOWN if others think it is more appropriate. > > ESHUTDOWN 108 Cannot send after transport endpoint shutdown > > Sounds reasonable enough to me. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Happy to change to ESHUTDOWN. Do you want me to submit a new version, or can this (as well as the doxygen change suggested by Laurent) be fixed while applying? I think this patch needs one more R-B tag for the series to be merged. Regards, Naush > > > > > Regards, > > Naush > > > > > > > > > > > + } > > > > + > > > > /* > > > > * Pipeline handlers should not requeue buffers after > releasing the > > > > * buffers on the device. Any occurence of this error should be > > > fixed > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart > > > >
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 9cea6a608660..1516be129d73 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -1491,6 +1491,9 @@ int V4L2VideoDevice::releaseBuffers() * The best available V4L2 buffer is picked for \a buffer using the V4L2 buffer * cache. * + * Note that \a queueBuffer will fail if the device is in the process of being + * stopped from a streaming state through the \a streamOff function. + * * \return 0 on success or a negative error code otherwise */ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) @@ -1499,6 +1502,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer) struct v4l2_buffer buf = {}; int ret; + if (state_ == State::Stopping) { + LOG(V4L2, Error) << "Device is in a stopping state."; + return -EPERM; + } + /* * Pipeline handlers should not requeue buffers after releasing the * buffers on the device. Any occurence of this error should be fixed
If the device is in the process of being stopped (i.e. Stopping state), any call to queueBuffer() must fail. This is to ensure the integrity of the buffer queue, as it gets cleared at the end of streamOff. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/v4l2_videodevice.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)