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

[Laurent Pinchart]

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 at ragnatech.se>
> 
> > Signed-off-by: Jacopo Mondi <jacopo at 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 at 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);
> > +	}
> >  }
> >  
> >  /**

-- 
Regards,

Laurent Pinchart