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

[Hirokazu Honda]

Hi Jacopo, thank you for the patch.

On Tue, May 11, 2021 at 5:28 AM Niklas Söderlund <
niklas.soderlund at ragnatech.se> wrote:

> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2021-05-10 12:52:30 +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);
>
> I wonder if the emitting of bufferCompleted should be moved to
> Camera::requestComplete()? This would be a common thing for all requests
> that completes in the cancel state right?
>
>
I think this should be done in Request::cancel().


> > +
> > +             camera->requestComplete(request);
> >               data->queuedRequests_.remove(request);
>

I wonder if we should call completeRequest() to keep the request
return order same as the request queue order.

By the way, a discussion is needed about how to report the error code. cf.)
https://patchwork.libcamera.org/patch/11764/.
But this patch can go without it.

> +     }
> >  }
> >
> >  /**
> > --
> > 2.31.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210511/7c927342/attachment.htm>