[libcamera-devel] [PATCH 2/8] libcamera: request: Add Request::cancel()

[Jacopo Mondi]

Hi Hiro,

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

IPU3 should be ok, as well as RPi as their state machine is IPA
driven, so they queue the raw frame and wait for the IPA to compute
parameters before queuing buffers to the ISP.

Other pipelines might require special attention to exit from a failed
queueRequestDevice() in a clean state. But that's very ph specific.
Looking at how you're changing the ipu3 one, do you think using
Request::cancel() could be useful ?

>
>
> > >
> > > -Hiro
> > >
> > >
> > > > > +
> > > > > +     cancelled_ = true;
> > > > > +     complete();
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * \brief Complete a buffer for the request
> > > > >   * \param[in] buffer The buffer that has completed
> > > > > --
> > > > > 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
> > > >
> >