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

[Kieran Bingham]

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?

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>

> 
> 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
> >