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

[Kieran Bingham]

Hi Naush,

On 20/04/2021 19:56, Naushir Patuck wrote:
> Hi Kieran,
> 
> 
> On Tue, 20 Apr 2021 at 14:07, Kieran Bingham
> <kieran.bingham at ideasonboard.com
> <mailto:kieran.bingham at ideasonboard.com>> 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
>     <mailto: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?

--
Kieran


> 
> Regards,
> Naush
> 
> 
>                     frameInfos_.remove(info);
>                     pipe_->completeRequest(request);
>     -- 
>     2.25.1
> 
>     _______________________________________________
>     libcamera-devel mailing list
>     libcamera-devel at lists.libcamera.org
>     <mailto:libcamera-devel at lists.libcamera.org>
>     https://lists.libcamera.org/listinfo/libcamera-devel
>     <https://lists.libcamera.org/listinfo/libcamera-devel>
> 

-- 
Regards
--
Kieran