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

[Laurent Pinchart]

Hi Kieran,

On Wed, Mar 24, 2021 at 12:49:38PM +0000, Kieran Bingham wrote:
> On 13/03/2021 23:00, Laurent Pinchart wrote:
> > 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";
> 
> As we don't otherwise need this ret value I agree, until ...
> 
> > I also wonder what then happens when a camera is unplugged. Could you
> > try unplugging a UVC camera while streaming ?
> 
> Hrm ... indeed this is a race probably.
> 
> When a camera is disconnected, it should set disconnected_, which will
> then causes isAccessAllowed() to return -ENODEV, which would be entirely
> permittable here to still complete the request and pass it back to the
> application.
> 
> How about:
> 
> /* Disconnected cameras are still able to complete requests. */
> ret = d->isAccessAllowed(Private::CameraRunning);
> if (ret < 0 && ret != -ENODEV)
>    LOG(Camera, Fatal) << "Trying to complete a request when stopped";

Seems better to me. Another option would be to pas true as the second
argument of isAccessAllowed().

> >>> +
> >>>  	requestCompleted.emit(request);
> >>>  }
> >>>

-- 
Regards,

Laurent Pinchart