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

11558 diff mbox series

The Pipeline handlers are responsible for making sure that all work
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@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.

 src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

Hi Kieran

On Fri, Mar 12, 2021 at 05:47:26AM +0000, Kieran Bingham wrote:
> The Pipeline handlers are responsible for making sure that all work
> 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@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 ?
>
>  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.";
> +
>  	freeBuffers(camera);
>  }
>
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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);
> >  }
> >

Hi Laurent,

On 13/03/2021 23:09, Laurent Pinchart wrote:
> 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/ ?
> 

Changed.

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

Yes, that's why I've left it as a todo for now. I really don't like the
idea of splitting to the base class just yet.

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

It may be that we need to extend the Camera state to incorporate a
stopping state as well to ensure any requests that come in while we're
calling stop get rejected, (while still allowing active ones to
complete, or be cancelled) during the Camera::requestComplete() call.

I've now created a patch to add this state, which will be included as
part of the improvements series on top of this.

With that it may be safer to allow an accessor to validate that the
data->queuedRequests_ is empty from the Camera class.

Which is also now done, and will be included in the improvements series.

And that would then make this patch redundant, and possible to drop.

<dropped>


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