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

[Hirokazu Honda]

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?


> >
> > -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/5bd79073/attachment-0001.htm>