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

[Laurent Pinchart]

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 ? :-)

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