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

[Hirokazu Honda]

Hi Jacopo,

On Tue, May 11, 2021 at 7:44 PM Jacopo Mondi <jacopo at jmondi.org> wrote:

> 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 ?
>
>
Yes, I did the same thing in the change. I will replace the code with
Request::cancel().


> >
> >
> > > >
> > > > -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
> > > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210511/b312f2af/attachment.htm>