[libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler: Notify Request queueing failure
Hirokazu Honda
hiroh at chromium.org
Thu May 13 04:37:23 CEST 2021
Hi Jacopo,
On Wed, May 12, 2021 at 6:35 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> 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.
>
>
Sounds good to me.
> >
> >
> > > > +
> > > > + 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
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210513/99100034/attachment-0001.htm>
More information about the libcamera-devel
mailing list