[libcamera-devel,v3,3/8] libcamera: pipeline_handler: Cancel Request on queueing failure

12354 diff mbox series

Capture requests are queued by the PipelineHandler base class to each
pipeline handler implementation using the virtual queueRequestDevice()
function.

However, if the pipeline handler fails to queue the request to the
hardware, the request gets silently deleted from the list of queued
ones, without notifying application of the error.

Reporting to applications that a Request has failed to queue by
cancelling and then completing it allows applications to maintain their
request-tracking mechanism consistent with the one internal to the library.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline_handler.cpp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Hi Jacopo,

Thanks for your work.

On 2021-05-21 17:42:22 +0200, Jacopo Mondi wrote:
> Capture requests are queued by the PipelineHandler base class to each
> pipeline handler implementation using the virtual queueRequestDevice()
> function.
> 
> However, if the pipeline handler fails to queue the request to the
> hardware, the request gets silently deleted from the list of queued
> ones, without notifying application of the error.
> 
> Reporting to applications that a Request has failed to queue by
> cancelling and then completing it allows applications to maintain their
> request-tracking mechanism consistent with the one internal to the library.

I think this touches on a larger issue, PipelineHandler::queueRequest() 
should really return something to the caller about if it succeeded or 
not. We have had similar needs in the past. But that is above the intent 
of this change so,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline_handler.cpp | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index f41b7a7b3308..e507a8bba8a6 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const
>   * This method queues a capture request to the pipeline handler for processing.
>   * The request is first added to the internal list of queued requests, and
>   * then passed to the pipeline handler with a call to queueRequestDevice().
> + * If the pipeline handler fails in queuing the request to the hardware the
> + * request is cancelled.
>   *
>   * Keeping track of queued requests ensures automatic completion of all requests
>   * when the pipeline handler is stopped with stop(). Request completion shall be
> @@ -409,8 +411,10 @@ void PipelineHandler::queueRequest(Request *request)
>  	request->sequence_ = data->requestSequence_++;
>  
>  	int ret = queueRequestDevice(camera, request);
> -	if (ret)
> -		data->queuedRequests_.remove(request);
> +	if (ret) {
> +		request->cancel();
> +		completeRequest(request);
> +	}
>  }
>  
>  /**
> -- 
> 2.31.1
>

Hi Niklas and Jacopo,

On Sat, May 22, 2021 at 11:30:23AM +0200, Niklas Söderlund wrote:
> On 2021-05-21 17:42:22 +0200, Jacopo Mondi wrote:
> > Capture requests are queued by the PipelineHandler base class to each
> > pipeline handler implementation using the virtual queueRequestDevice()
> > function.
> > 
> > However, if the pipeline handler fails to queue the request to the
> > hardware, the request gets silently deleted from the list of queued
> > ones, without notifying application of the error.
> > 
> > Reporting to applications that a Request has failed to queue by
> > cancelling and then completing it allows applications to maintain their
> > request-tracking mechanism consistent with the one internal to the library.
> 
> I think this touches on a larger issue, PipelineHandler::queueRequest() 
> should really return something to the caller about if it succeeded or 
> not. We have had similar needs in the past. But that is above the intent 
> of this change so,

PipelineHandler::queueRequest() is called asynchronously, so it can't
return an error to Camera::queueRequest(). That's why errors have to be
handled internally.

> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/pipeline_handler.cpp | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index f41b7a7b3308..e507a8bba8a6 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const
> >   * This method queues a capture request to the pipeline handler for processing.
> >   * The request is first added to the internal list of queued requests, and
> >   * then passed to the pipeline handler with a call to queueRequestDevice().
> > + * If the pipeline handler fails in queuing the request to the hardware the
> > + * request is cancelled.

The Cancelled state was meant for requests cancelled when the camera
gets stopped. We may need a different state for errors at runtime. That
will be for another patch series though.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >   *
> >   * Keeping track of queued requests ensures automatic completion of all requests
> >   * when the pipeline handler is stopped with stop(). Request completion shall be
> > @@ -409,8 +411,10 @@ void PipelineHandler::queueRequest(Request *request)
> >  	request->sequence_ = data->requestSequence_++;
> >  
> >  	int ret = queueRequestDevice(camera, request);
> > -	if (ret)
> > -		data->queuedRequests_.remove(request);
> > +	if (ret) {
> > +		request->cancel();
> > +		completeRequest(request);
> > +	}
> >  }
> >  
> >  /**