[libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler: Notify Request queueing failure
Jacopo Mondi
jacopo at jmondi.org
Wed May 12 11:36:32 CEST 2021
Hello,
On Tue, May 11, 2021 at 02:00:32PM +0900, Hirokazu Honda wrote:
> 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().
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 ?
>
> 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
> >
More information about the libcamera-devel
mailing list