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

[Jacopo Mondi]

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
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 ?

>
> -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
> >