[libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3: Cancel unused buffers

[Naushir Patuck]

Hi Laurent and Kieran,


On Tue, 20 Apr 2021 at 23:33, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Kieran,
>
> Thank you for the patch.
>
> On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote:
> > On 20/04/2021 19:56, Naushir Patuck wrote:
> > > On Tue, 20 Apr 2021 at 14:07, Kieran Bingham wrote:
> > > >
> > > > When the CIO2 returns a cancelled buffer, we will not queue buffers
> > > > to the IMGU.
> > > >
> > > > These buffers should be explicitly marked as cancelled to ensure
> > > > the application knows there is no valid metadata or frame data
> > > > provided in the buffer.
> > > >
> > > > Provide a cancel() method on the FrameBuffer to allow explicitly
> > > > cancelling a buffer.
> > > >
> > > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > ---
> > > >  include/libcamera/buffer.h           | 2 ++
> > > >  src/libcamera/buffer.cpp             | 8 ++++++++
> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--
> > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > > > index 620f8a66f6a2..e0af00900409 100644
> > > > --- a/include/libcamera/buffer.h
> > > > +++ b/include/libcamera/buffer.h
> > > > @@ -53,6 +53,8 @@ public:
> > > >         unsigned int cookie() const { return cookie_; }
> > > >         void setCookie(unsigned int cookie) { cookie_ = cookie; }
> > > >
> > > > +       void cancel() { metadata_.status =
> FrameMetadata::FrameCancelled; }
> > > > +
> > > >  private:
> > > >         LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)
> > > >
> > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> > > > index 75b2693281a7..7635226b9045 100644
> > > > --- a/src/libcamera/buffer.cpp
> > > > +++ b/src/libcamera/buffer.cpp
> > > > @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const
> std::vector<Plane> &planes, unsigned int cookie)
> > > >   * core never modifies the buffer cookie.
> > > >   */
> > > >
> > > > +/**
> > > > + * \fn FrameBuffer::cancel()
> > > > + * \brief Marks the buffer as cancelled
> > > > + *
> > > > + * If a buffer is not used by a request, it shall be marked as
> cancelled to
> > > > + * indicate that the metadata is invalid.
> > > > + */
> > > > +
> > > >  /**
> > > >   * \class MappedBuffer
> > > >   * \brief Provide an interface to support managing memory mapped
> buffers
> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > index 51446fcf5bc1..73306cea6b37 100644
> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > @@ -1257,8 +1257,11 @@ void
> IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> > >
> > > >         /* If the buffer is cancelled force a complete of the whole
> request. */
> > > >         if (buffer->metadata().status ==
> FrameMetadata::FrameCancelled) {
> > > > -               for (auto it : request->buffers())
> > > > -                       pipe_->completeBuffer(request, it.second);
> > > > +               for (auto it : request->buffers()) {
> > > > +                       FrameBuffer *b = it.second;
> > > > +                       b->cancel();
> > > > +                       pipe_->completeBuffer(request, b);
> > > > +               }
> > >
> > > Would you consider adding a Request::cancel() method to the API that
> pipeline
> > > handlers can invoke to essentially perform the above logic?  I found
> it a bit
> > > confusing having to call completeBuffer on a cancelled buffer, and
> completeRequest
> > > to complete cancelling the request if you see what I mean?  This is
> probably a bit
> > > more relevant to the Raspberry Pi pipeline handler where we queue up
> requests
> > > if required.
> >
> > I think this could be helpful sure, but maybe as a private/internal call
> > only (not in the public API). That might be possible now that we have
> > Request as an Extensible class later in this series.
> >    (In fact, this Buffer->cancel() is public ... eep?)
> >
> > In this instance where the CIO2 (the receiver) has caused the
> > cancellation, a simple helper makes sense to clear the request entirely
> > and return it as we know that there are no other resources pending.
> >
> > But if this were on the completion path in the IMGU (the ISP), then I'd
> > be concerned about races with multiple completion handlers trying to
> > 'cancel' the request ...
> >
> > But if we can find ways to simplify handling of shutdown paths I'm all
> > for it ;-)
> >
> > Any thoughts?
>
> A Request::cancel() function would make sense I believe. It's certainly
> useful to cancel requests that have been put in an internal pipeline
> handler queue, but whose buffers haven't been queued to the device. It
> could also be used in this case I believe, as there should be no race.
> If the pipeline handler had queued more than one buffer from the request
> (including internal buffers) to the device, that would be a different
> question. For instance, if we queued a raw buffer and an embedded data
> buffer, they would complete (in the cancelled state) separately, in
> unknown order, so we couldn't call Request::cancel() sefely in that
> case.
>

Maybe I misunderstood the above, but I think you can tell in the above
example if the raw buffer had been completed as part of a request and
the embedded buffer had not.  This can be checked by looking at the
FrameBuffer::request() value which gets nulled on a
Request::completeBuffer().
The code in our pipeline handler to deal with cancelling requests [1] does
have to deal with this, so I assume it is valid.

[1]
https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482

Naush


>
> We could start with a first implementation the limits usage of
> Request::cancel() to the cases where no buffers or a single buffer has
> been queued to the device, and improve on top, but I'd be interested in
> hearing proposals about how this could be further improved.
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210421/2e06f6f8/attachment.htm>