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