[libcamera-devel] [PATCH v2 3/8] libcamera: pipeline_handler: Notify Request queueing failure

[Laurent Pinchart]

Hi Jacopo,

On Mon, May 17, 2021 at 09:17:32AM +0200, Jacopo Mondi wrote:
> On Mon, May 17, 2021 at 01:48:59PM +0900, Hirokazu Honda wrote:
> > On Mon, May 17, 2021 at 11:45 AM Laurent Pinchart wrote:
> > > On Thu, May 13, 2021 at 11:22:41AM +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.
> > > >
> > > > Allowing applications to know if a Request has failed to queue
> > > > by emitting the Camera::bufferCompleted and Camera::requestCompleted
> > > > signals allows them to maintain a state consistent with the one
> > > > internal to the library.
> > > >
> > > > To do so, if queuing a request to the device fails, cancel the request
> > > > and emit the completion signal to report the failure to applications.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++-
> > > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > > index f41b7a7b3308..260a52018a4d 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 immediately completed with error.
> > > >   *
> > > >   * 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,19 @@ void PipelineHandler::queueRequest(Request *request)
> > > >       request->sequence_ = data->requestSequence_++;
> > > >
> > > >       int ret = queueRequestDevice(camera, request);
> > > > -     if (ret)
> > > > +     if (ret) {
> > > > +             /*
> > > > +              * Cancel the request and notify completion of its buffers in
> > > > +              * error state. Then complete the cancelled request and remove
> > > > +              * it from the queued requests list.
> > > > +              */
> > > > +             request->cancel();
> > > > +             for (auto bufferMap : request->buffers())
> > > > +                     camera->bufferCompleted.emit(request, bufferMap.second);
> > > > +
> > > > +             camera->requestComplete(request);
> > >
> > > This breaks the request completion ordering. I think you could just call
> > > PipelineHandler::completeRequest() instead.
> > >
> >
> > I had the same comment in the previous version.
> > https://patchwork.libcamera.org/patch/12243/
> > We were waiting for your reply. Alright we got the reply now.
> >
> 
> Yes, I've cc-ed Laurent in that patch to know his opinion.
> 
> Laurent, could you please read my reply and let me know if it add
> anything to the discussion ?

Done. Sorry for missing it in the first place.

> > > >               data->queuedRequests_.remove(request);
> > > > +     }
> > > >  }
> > > >
> > > >  /**

-- 
Regards,

Laurent Pinchart