<div dir="ltr"><div>Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 11, 2021 at 7:44 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">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 06:20:30PM +0900, Hirokazu Honda wrote:<br>
> Hi Jacopo,<br>
><br>
> On Tue, May 11, 2021 at 5:26 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
><br>
> > 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<br>
> > 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<br>
> > 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<br>
> > 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 !=<br>
> > 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>
> ><br>
> Ah, Ack.<br>
><br>
><br>
> > ><br>
> > ><br>
> > > > > + buffer = pending_.erase(buffer);<br>
> > > > > + }<br>
> > > ><br>
> > > > I wonder how this works if a buffer have been queued to hardware but<br>
> > 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>
> ><br>
> I think so. IPU3 is implemented so, isn't it?<br>
<br>
IPU3 should be ok, as well as RPi as their state machine is IPA<br>
driven, so they queue the raw frame and wait for the IPA to compute<br>
parameters before queuing buffers to the ISP.<br>
<br>
Other pipelines might require special attention to exit from a failed<br>
queueRequestDevice() in a clean state. But that's very ph specific.<br>
Looking at how you're changing the ipu3 one, do you think using<br>
Request::cancel() could be useful ?<br>
<br></blockquote><div><br></div><div>Yes, I did the same thing in the change. I will replace the code with Request::cancel().</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>
> > > -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>
> ><br>
</blockquote></div></div>