[libcamera-devel] [PATCH v4 6/8] libcamera: v4l2_videodevice: Do not allow buffer queueing in stopping state

[Naushir Patuck]

On Wed, 23 Mar 2022 at 09:54, Kieran Bingham <
kieran.bingham at 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 at 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 at 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 at 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
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220323/dd518600/attachment.htm>