<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 12, 2021 at 6:35 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br>
<br>
On Tue, May 11, 2021 at 02:00:32PM +0900, Hirokazu Honda wrote:<br>
> Hi Jacopo, thank you for the patch.<br>
><br>
> On Tue, May 11, 2021 at 5:28 AM Niklas Söderlund <<br>
> <a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</a>> wrote:<br>
><br>
> > Hi Jacopo,<br>
> ><br>
> > Thanks for your patch.<br>
> ><br>
> > On 2021-05-10 12:52:30 +0200, Jacopo Mondi wrote:<br>
> > > Capture requests are queued by the PipelineHandler base class to each<br>
> > > pipeline handler implementation using the virtual queueRequestDevice()<br>
> > > function.<br>
> > ><br>
> > > However, if the pipeline handler fails to queue the request to the<br>
> > > hardware, the request gets silently deleted from the list of queued<br>
> > > ones, without notifying application of the error.<br>
> > ><br>
> > > Allowing applications to know if a Request has failed to queue<br>
> > > by emitting the Camera::bufferCompleted and Camera::requestCompleted<br>
> > > signals allows them to maintain a state consistent with the one<br>
> > > internal to the library.<br>
> > ><br>
> > > To do so, if queuing a request to the device fails, cancel the request<br>
> > > and emit the completion signal to report the failure to applications.<br>
> > ><br>
> > > Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> > > ---<br>
> > >  src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++-<br>
> > >  1 file changed, 14 insertions(+), 1 deletion(-)<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline_handler.cpp<br>
> > b/src/libcamera/pipeline_handler.cpp<br>
> > > index f41b7a7b3308..260a52018a4d 100644<br>
> > > --- a/src/libcamera/pipeline_handler.cpp<br>
> > > +++ b/src/libcamera/pipeline_handler.cpp<br>
> > > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const<br>
> > Camera *camera) const<br>
> > >   * This method queues a capture request to the pipeline handler for<br>
> > processing.<br>
> > >   * The request is first added to the internal list of queued requests,<br>
> > and<br>
> > >   * then passed to the pipeline handler with a call to<br>
> > queueRequestDevice().<br>
> > > + * If the pipeline handler fails in queuing the request to the hardware<br>
> > the<br>
> > > + * request is immediately completed with error.<br>
> > >   *<br>
> > >   * Keeping track of queued requests ensures automatic completion of all<br>
> > requests<br>
> > >   * when the pipeline handler is stopped with stop(). Request completion<br>
> > shall be<br>
> > > @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request)<br>
> > >       request->sequence_ = data->requestSequence_++;<br>
> > ><br>
> > >       int ret = queueRequestDevice(camera, request);<br>
> > > -     if (ret)<br>
> > > +     if (ret) {<br>
> > > +             /*<br>
> > > +              * Cancel the request and notify completion of its buffers<br>
> > in<br>
> > > +              * error state. Then complete the cancelled request and<br>
> > remove<br>
> > > +              * it from the queued requests list.<br>
> > > +              */<br>
> > > +             request->cancel();<br>
> > > +             for (auto bufferMap : request->buffers())<br>
> > > +                     camera->bufferCompleted.emit(request,<br>
> > bufferMap.second);<br>
> ><br>
> > I wonder if the emitting of bufferCompleted should be moved to<br>
> > Camera::requestComplete()? This would be a common thing for all requests<br>
> > that completes in the cancel state right?<br>
> ><br>
> ><br>
> I think this should be done in Request::cancel().<br>
<br>
I don't think Camera::requestComplete() is the right place where to<br>
emit the buffer completion signal, as those are two separate actions.<br>
If we want to emit buffer complete for cancelled buffers we should<br>
make Camera::requestComplete() inspect the status of the request,<br>
something that doesn't seem to belong there.<br>
<br>
OTOH emitting the buffer complete signal in Request::cancel() might be<br>
a better fit. Although this opens a whole new problem, which is the<br>
one of partial requests completion. If we allow requests to complete<br>
by pieces, we might want to allow ph to complete buffers which have<br>
been succesfull singularly, but signal that a request has been<br>
cancelled and has failed because not -all- of them have been<br>
completed.<br>
<br>
In this specific case, where the request is cancelled because it has<br>
failed to queue it would make sense to emit buffers completion signal<br>
in Request::cancel() : no buffers have been queued, so none of them<br>
can be completed.<br>
<br>
In the case the Request is cancelled later, by the ph, after one of<br>
the buffers has failed but other ones have completed, the ph should be<br>
in charge to emit buffer completion signals with the single buffers<br>
status.<br>
<br>
IOW, I would for the time being, not tie cancelling the request to<br>
emit buffer completion. True, Request::cancel() sets all buffers state<br>
to error, but before this happen a buffer can be completed succesfully<br>
and application could have had access to it.<br>
<br></blockquote><div><br></div><div>Sounds good to me.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
><br>
> > > +<br>
> > > +             camera->requestComplete(request);<br>
> > >               data->queuedRequests_.remove(request);<br>
> ><br>
><br>
> I wonder if we should call completeRequest() to keep the request<br>
> return order same as the request queue order.<br>
<br>
Good question, this has direct consequences on the libcamera API.<br>
<br>
If queuing a Request fails, do we want to be notified immediately<br>
about it, or let the requests that have been queued before that one<br>
complete first ?<br>
<br>
The typical application I can think of will re-use and re-queue a Request<br>
immediately in the request completion handler. If we delay reporting<br>
the queueing error there is a risk multiple requests are queued after<br>
the one that has failed. If we fail immediately, the application can<br>
handle the error condition immediately, and stop the re-queuing<br>
mechanism.<br>
<br>
Laurent, what's your take here ?<br>
<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">><br>
> By the way, a discussion is needed about how to report the error code. cf.)<br>
> <a href="https://patchwork.libcamera.org/patch/11764/" rel="noreferrer" target="_blank">https://patchwork.libcamera.org/patch/11764/</a>.<br>
> But this patch can go without it.<br>
><br>
> > +     }<br>
> > >  }<br>
> > ><br>
> > >  /**<br>
> > > --<br>
> > > 2.31.1<br>
> > ><br>
> > > _______________________________________________<br>
> > > libcamera-devel mailing list<br>
> > > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> ><br>
> > --<br>
> > Regards,<br>
> > Niklas Söderlund<br>
> > _______________________________________________<br>
> > libcamera-devel mailing list<br>
> > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> ><br>
</blockquote></div></div>