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

[Laurent Pinchart]

Hi Naush,

On Wed, Apr 21, 2021 at 09:58:07AM +0100, Naushir Patuck wrote:
> On Tue, 20 Apr 2021 at 23:33, Laurent Pinchart wrote:
> > 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.

My point was that the following pseudo-code would be invalid:

Foo::rawCompletionCallback(FrameBuffer *buffer)
{
	if (buffer->metadata().status == Cancelled) {
		Request *request = buffer->request();
		request->cancel();
		request->complete();
		return;
	}

	...
}

Foo::embeddedCompletionCallback(FrameBuffer *buffer)
{
	if (buffer->metadata().status == Cancelled) {
		Request *request = buffer->request();
		request->cancel();
		request->complete();
		return;
	}

	...
}

We would call cancel() and complete() twice. We also can't have the
cancellation handling in Foo::rawCompletionCallback() or
Foo::embeddedCompletionCallback() only, as we don't control in which
order those two will be called.

It's not a very complicated issue, and I'm sure we'll come up with a
nice API to handle request completion with cancellation support. My
point is that it requires a bit of careful API design.

> [1]
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482
> 
> > 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