[libcamera-devel] [PATCH v2 7/8] libcamera: pipeline: ipu3: Ensure no requests are pending at stop()

[Laurent Pinchart]

Hi Kieran and Jacopo,

Thank you for the patch, and for the review :-)

On Sat, Mar 13, 2021 at 10:40:27AM +0100, Jacopo Mondi wrote:
> On Fri, Mar 12, 2021 at 05:47:26AM +0000, Kieran Bingham wrote:
> > The Pipeline handlers are responsible for making sure that all work

s/Pipeline/pipeline/ ?

> > is completed during the stop call, and all requests and resources
> > should be released back to the application.
> >
> > Add an assertion to guarantee that there are no pending requests
> > before returning from ::stop()
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > ---
> > RFC? Ideally this should be handled by the base pipeline handler so that
> > the same guarantee applies to all pipeline handlers. But we don't
> > currently call into the base class during stop.
> 
> I wonder if this plays a role with my comment on making requests
> completed when the Camera is moved to Configured triggering a Fatal.
> 
> More generally, how can we guarantee the queue of requests have been
> emptied when stop() is called ? I don't think it's that unlikely that
> the requests queue is !empty when applications call stop(), isn't it ?

The pipeline handler is responsible for cancelling all pending requests
in its stop() implementation:

/**
 * \fn PipelineHandler::stop()
 * \brief Stop capturing from all running streams
 * \param[in] camera The camera to stop
 *
 * This method stops capturing and processing requests immediately. All pending
 * requests are cancelled and complete immediately in an error state.
 *
 * \context This function is called from the CameraManager thread.
 */

> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 7ab3a5bfdccb..c5facaea16dd 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -771,6 +771,14 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> >  	if (ret)
> >  		LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
> >
> > +	/*
> > +	 * All requests must have completed before releaseing buffers.
> > +	 * \todo: Ensure all pipeline handlers guarantee queuedRequests is emtpy
> > +	 * at the end of stop().
> > +	 */
> > +	if (!data->queuedRequests_.empty())
> > +		LOG(IPU3, Fatal) << "There are still requests to complete.";
> > +

Is this something we could check in the PipelineHandler or Camera class
instead ? The former would be a bit more difficult, as the
PipelineHandler base class isn't involved in processing the stop()
function, and I'm not very keen on splitting stop() between stop() and
stopDevice() like we did for queueRequest(). The latter would involve
either keeping track of queued requests in the Camera class, or, perhaps
better to avoid the duplication, adding a function to the
PipelineHandler base class to check if the queue is empty and calling it
at the end of Camera::stop(). Regardless of which option you pick,
please make sure to consider race conditions between the application
thread in which Camera::stop() runs and the pipeline handler thread.

> >  	freeBuffers(camera);
> >  }
> >

-- 
Regards,

Laurent Pinchart