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

[Laurent Pinchart]

Hi Jacopo,

On Wed, May 12, 2021 at 11:36:32AM +0200, Jacopo Mondi wrote:
> On Tue, May 11, 2021 at 02:00:32PM +0900, Hirokazu Honda wrote:
> > On Tue, May 11, 2021 at 5:28 AM Niklas Söderlund wrote:
> > > 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().
> 
> I don't think Camera::requestComplete() is the right place where to
> emit the buffer completion signal, as those are two separate actions.
> If we want to emit buffer complete for cancelled buffers we should
> make Camera::requestComplete() inspect the status of the request,
> something that doesn't seem to belong there.
> 
> OTOH emitting the buffer complete signal in Request::cancel() might be
> a better fit. Although this opens a whole new problem, which is the
> one of partial requests completion. If we allow requests to complete
> by pieces, we might want to allow ph to complete buffers which have
> been succesfull singularly, but signal that a request has been
> cancelled and has failed because not -all- of them have been
> completed.
> 
> In this specific case, where the request is cancelled because it has
> failed to queue it would make sense to emit buffers completion signal
> in Request::cancel() : no buffers have been queued, so none of them
> can be completed.
> 
> In the case the Request is cancelled later, by the ph, after one of
> the buffers has failed but other ones have completed, the ph should be
> in charge to emit buffer completion signals with the single buffers
> status.
> 
> IOW, I would for the time being, not tie cancelling the request to
> emit buffer completion. True, Request::cancel() sets all buffers state
> to error, but before this happen a buffer can be completed succesfully
> and application could have had access to it.
> 
> > > > +
> > > > +             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.
> 
> Good question, this has direct consequences on the libcamera API.
> 
> If queuing a Request fails, do we want to be notified immediately
> about it, or let the requests that have been queued before that one
> complete first ?
> 
> The typical application I can think of will re-use and re-queue a Request
> immediately in the request completion handler. If we delay reporting
> the queueing error there is a risk multiple requests are queued after
> the one that has failed. If we fail immediately, the application can
> handle the error condition immediately, and stop the re-queuing
> mechanism.
> 
> Laurent, what's your take here ?

Breaking the ordering guarantee is a really big change. I'm not
convinced at this point that it would be worth it. We can still consider
it, but it should then be considered very carefully, with all its
implications. It shouldn't be part of this series.

> > 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.
> >
> > > +     }
> > > >  }
> > > >
> > > >  /**

-- 
Regards,

Laurent Pinchart