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

[Jacopo Mondi]

Hello,

On Tue, May 11, 2021 at 10:26:55AM +0200, Jacopo Mondi 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; }
>
> }
>
> >
> >
> > > > +             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

Just to add that I made the method private and only accessible to
friend class for the moment for this specific reason.