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

[Naushir Patuck]

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.


> > +             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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220323/e48e5e89/attachment-0001.htm>