<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 11, 2021 at 5:26 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">Hi Hiro,<br>
<br>
On Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote:<br>
> Hi Jacopo, thank you for the patch.<br>
><br>
> On Tue, May 11, 2021 at 5:25 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 work.<br>
> ><br>
> > On 2021-05-10 12:52:29 +0200, Jacopo Mondi wrote:<br>
> > > Add a cancel() function to the Request class that allows to forcefully<br>
> > > complete the request and its associated buffers in error state.<br>
> > ><br>
> > > Only pending requests can be forcefully cancelled. Enforce that<br>
> > > by asserting the request state to be RequestPending.<br>
> > ><br>
> > > Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> > > ---<br>
> > >  include/libcamera/request.h |  1 +<br>
> > >  src/libcamera/request.cpp   | 30 ++++++++++++++++++++++++++++++<br>
> > >  2 files changed, 31 insertions(+)<br>
> > ><br>
> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h<br>
> > > index 4cf5ff3f7d3b..5596901ddd8e 100644<br>
> > > --- a/include/libcamera/request.h<br>
> > > +++ b/include/libcamera/request.h<br>
> > > @@ -65,6 +65,7 @@ private:<br>
> > >       friend class PipelineHandler;<br>
> > ><br>
> > >       void complete();<br>
> > > +     void cancel();<br>
> > ><br>
> > >       bool completeBuffer(FrameBuffer *buffer);<br>
> > ><br>
> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp<br>
> > > index ce2dd7b17f10..fc5e25199112 100644<br>
> > > --- a/src/libcamera/request.cpp<br>
> > > +++ b/src/libcamera/request.cpp<br>
> > > @@ -292,6 +292,36 @@ void Request::complete()<br>
> > >       LIBCAMERA_TRACEPOINT(request_complete, this);<br>
> > >  }<br>
> > ><br>
> > > +/**<br>
> > > + * \brief Cancel a queued request<br>
> > > + *<br>
> > > + * Mark the request and its associated buffers as cancelled and<br>
> > complete it.<br>
> > > + *<br>
> > > + * Set the status of each buffer in the request to the frame cancelled<br>
> > state and<br>
> > > + * remove them from the pending buffer queue before completing the<br>
> > request with<br>
> > > + * error.<br>
> > > + */<br>
> > > +void Request::cancel()<br>
> > > +{<br>
> > > +     LIBCAMERA_TRACEPOINT(request_cancel, this);<br>
> > > +<br>
> > > +     ASSERT(status_ == RequestPending);<br>
> > > +<br>
> > > +     /*<br>
> > > +      * We can't simply loop and call completeBuffer() as erase()<br>
> > invalidates<br>
> > > +      * pointers and iterators, so we have to manually cancel the<br>
> > buffer and<br>
> > > +      * erase it from the pending buffers list.<br>
> > > +      */<br>
> > > +     for (auto buffer = pending_.begin(); buffer != pending_.end();) {<br>
> > > +             (*buffer)->cancel();<br>
> > > +             (*buffer)->setRequest(nullptr);<br>
> ><br>
><br>
> Shall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled?<br>
<br>
That's exactly what (*buffer)->cancel() does :)<br>
<br>
class FrameBuffer<br>
{<br>
        ....<br>
<br>
        void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }<br>
<br>
}<br>
<br></blockquote><div><br></div><div>Ah, Ack.</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>
> > > +             buffer = pending_.erase(buffer);<br>
> > > +     }<br>
> ><br>
> > I wonder how this works if a buffer have been queued to hardware but not<br>
> > yet completed? Do we need a new Request status RequestProcessing(?) to<br>
> > deal with such a corner case or maybe it's not a problem?<br>
> ><br>
> ><br>
> I think Request::cancel() should be called when a request and its buffers<br>
> are not in the hardware.<br>
<br>
Ideally yes, but there's one issue. Once we add this method it becomes<br>
application visible, and application can call it, at any time, beside<br>
the fact that ph should be diligent and keep the state clean. Should<br>
then queueRequestDevice look like:<br>
<br>
        ret = csi2.queueRawBuffer(raw)<br>
        if (ret)<br>
                return ret;<br>
<br>
        ret = isp.queueViewfinderBuffer(buffer);<br>
        if (ret) {<br>
                dequeueRawBuffer()<br>
                return ret;<br>
        }<br>
<br>
is this even possible ?<br>
<br></blockquote><div><br></div><div>I think so. IPU3 is implemented so, isn't it?</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>
> -Hiro<br>
><br>
><br>
> > > +<br>
> > > +     cancelled_ = true;<br>
> > > +     complete();<br>
> > > +}<br>
> > > +<br>
> > >  /**<br>
> > >   * \brief Complete a buffer for the request<br>
> > >   * \param[in] buffer The buffer that has completed<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>