[libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate requests are completed in Running state

[Jacopo Mondi]

Hi Laurent,

On Sun, Mar 14, 2021 at 01:00:04AM +0200, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Sat, Mar 13, 2021 at 10:35:49AM +0100, Jacopo Mondi wrote:
> > On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote:
> > > All requests must have completed before the Camera has fully stopped.
> > >
> > > Requests completing when the camera is not running represent an internal
> > > pipeline handler bug.
> > >
> > > Trap this event with a fatal error.
> >
> > Am I wrong or this is actually the other way around ? Marking the
> > Camera as Configured -after- the pipeline's stop() has been invoked
> > makes sure the requests that complete with error due to stop() are
> > refused.
> >
> > IOW stop() is invoked on the pipeline thread, which might complete
> > requests with error asynchronously from the camera state being
> > moved to 'CameraConfigured'. After the Camera state has changed,
> > all the requests completed with error will trigger a Fatal.
> > Am I missing something obvious ?
>
> The call to PipelineHandler::stop() is blocking, which means that the
> state will only be set back to CameraConfigured once the pipeline
> handler's stop() function returns. Pipeline handlers are required to
> complete all pending requests before returning from stop(), so the
> assertion shouldn't be triggered. Am I missing something ? :-)
>

I was missing the (obvious) fact that stop() is blocking it seems :)

Thanks for clearing this out!

> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > >  src/libcamera/camera.cpp | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 84edbb8f494d..19fb9eb415b0 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -1062,11 +1062,11 @@ int Camera::stop()
> > >
> > >  	LOG(Camera, Debug) << "Stopping capture";
> > >
> > > -	d->setState(Private::CameraConfigured);
> > > -
> > >  	d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
> > >  			       this);
> > >
> > > +	d->setState(Private::CameraConfigured);
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -1079,6 +1079,12 @@ int Camera::stop()
> > >   */
> > >  void Camera::requestComplete(Request *request)
> > >  {
> > > +	Private *const d = LIBCAMERA_D_PTR();
> > > +
> > > +	int ret = d->isAccessAllowed(Private::CameraRunning);
> > > +	if (ret < 0)
> > > +		LOG(Camera, Fatal) << "Trying to complete a request when Stopped";
>
> Nitpicking, as there's no Stopped state, I'd write "stopped".
>
> And you could also write
>
> 	if (d->isAccessAllowed(Private::CameraRunning) < 0)
> 		LOG(Camera, Fatal) << "Trying to complete a request when Stopped";
>
> I also wonder what then happens when a camera is unplugged. Could you
> try unplugging a UVC camera while streaming ?
>
> > > +
> > >  	requestCompleted.emit(request);
> > >  }
> > >
>
> --
> Regards,
>
> Laurent Pinchart