[libcamera-devel] [PATCH v4 4/6] libcamera: pipeline: ipu3: Cancel unused buffers
Naushir Patuck
naush at raspberrypi.com
Wed Apr 21 11:20:27 CEST 2021
Hi Laurent,
On Wed, 21 Apr 2021 at 10:09, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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.
>
I got it now! The RPi PH logic is slightly different to the above (and
probably
other PHs) in that there is a state-machine that checks for all buffers to
be complete before signalling request->complete() in only one place.
Naush
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210421/40bf1404/attachment.htm>
More information about the libcamera-devel
mailing list