[libcamera-devel] [PATCH v3 08/11] libcamera: camera: Extend with a Stopping state

[Laurent Pinchart]

Hi Kieran,

On Fri, Mar 26, 2021 at 03:48:07PM +0000, Kieran Bingham wrote:
> On 26/03/2021 02:36, Laurent Pinchart wrote:
> > On Thu, Mar 25, 2021 at 01:42:28PM +0000, Kieran Bingham wrote:
> >> When the camera is being stop()ped, active requests will complete. These
> >> may trigger an application to re-queue those requests to the camera but
> >> that is not permitted.
> > 
> > Maybe ", and is an error in the application." to be clear ?
> 
> Yes,
> 
> >> Extend the camera state to include a stopping state which is entered as
> >> soon as a call to stop() is made. At this point, any request queued will
> >> be rejected with a warning, while any pending requests are either
> >> successfully completed or cancelled.
> > 
> > Should it be an error instead of a warning ?
> 
> The warning will come from the isAccessAllowed log level.
> 
> I don't want to duplicate an error message, but it would probably be
> considered an error.
> 
> We could try to pass in a log level to the isAccessAllowed, but I think
> we're overloading those operators enough already ...
> 
> Perhaps we could actually raise the log from isAccessAllowed further
> from Debug to Warning, to all the way to Error?
> 
> If an operation is made in the wrong state - it's an error isn't it ?

Yes, I think so.

> >> When the pipeline handler has finished stopping, the camera state will
> >> transition to the CameraConfigured state where it can begin to accept
> >> requests again, and be restarted.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  src/libcamera/camera.cpp | 18 ++++++++++++++++--
> >>  1 file changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> >> index 336ab8695ab3..7f7956ba732f 100644
> >> --- a/src/libcamera/camera.cpp
> >> +++ b/src/libcamera/camera.cpp
> >> @@ -339,6 +339,7 @@ public:
> >>  		CameraAvailable,
> >>  		CameraAcquired,
> >>  		CameraConfigured,
> >> +		CameraStopping,
> >>  		CameraRunning,
> >>  	};
> >>  
> >> @@ -382,6 +383,7 @@ static const char *const camera_state_names[] = {
> >>  	"Available",
> >>  	"Acquired",
> >>  	"Configured",
> >> +	"Stopping",
> >>  	"Running",
> >>  };
> >>  
> >> @@ -492,6 +494,7 @@ void Camera::Private::setState(State state)
> >>   *   node [shape = doublecircle ]; Available;
> >>   *   node [shape = circle ]; Acquired;
> >>   *   node [shape = circle ]; Configured;
> >> + *   node [shape = circle ]; Stopping;
> >>   *   node [shape = circle ]; Running;
> >>   *
> >>   *   Available -> Available [label = "release()"];
> >> @@ -504,7 +507,8 @@ void Camera::Private::setState(State state)
> >>   *   Configured -> Configured [label = "configure(), createRequest()"];
> >>   *   Configured -> Running [label = "start()"];
> >>   *
> >> - *   Running -> Configured [label = "stop()"];
> >> + *   Running -> Stopping [label = "stop()"];
> >> + *   Stopping -> Configured;
> >>   *   Running -> Running [label = "createRequest(), queueRequest()"];
> >>   * }
> >>   * \enddot
> >> @@ -524,6 +528,12 @@ void Camera::Private::setState(State state)
> >>   * release() the camera and to get back to the Available state or start()
> >>   * it to progress to the Running state.
> >>   *
> >> + * \subsubsection Stopping
> >> + * The camera has been asked to stop. Pending reqeusts are being completed or
> > 
> > s/reqeusts/requests/
> 
> Fixed, thanks.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> >> + * cancelled, and no new requests are permitted to be queued. The camera will
> >> + * transition to the Configured state when all queued requests have been
> >> + * returned to the application.
> >> + *
> >>   * \subsubsection Running
> >>   * The camera is running and ready to process requests queued by the
> >>   * application. The camera remains in this state until it is stopped and moved
> >> @@ -1071,6 +1081,8 @@ int Camera::stop()
> >>  
> >>  	LOG(Camera, Debug) << "Stopping capture";
> >>  
> >> +	d->setState(Private::CameraStopping);
> >> +
> >>  	d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
> >>  			       this);
> >>  
> >> @@ -1091,7 +1103,9 @@ void Camera::requestComplete(Request *request)
> >>  	Private *const d = LIBCAMERA_D_PTR();
> >>  
> >>  	/* Disconnected cameras are still able to complete requests. */
> >> -	int ret = d->isAccessAllowed(__FUNCTION__, Private::CameraRunning);
> >> +	int ret = d->isAccessAllowed(__FUNCTION__,
> >> +				     Private::CameraStopping,
> >> +				     Private::CameraRunning);
> >>  	if (ret < 0 && ret != -ENODEV)
> >>  		LOG(Camera, Fatal) << "Trying to complete a request when stopped";
> >>  

-- 
Regards,

Laurent Pinchart