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

[Laurent Pinchart]

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.

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