[libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3: Cancel unused buffers

[Naushir Patuck]

Hi all,


On Wed, 24 Mar 2021 at 20:34, Jean-Michel Hautbois <
jeanmichel.hautbois at ideasonboard.com> wrote:

> Hi Kieran, Laurent,
>
> On 24/03/2021 21:19, Laurent Pinchart wrote:
> > Hi Kieran,
> >
> > Thank you for the patch.
> >
> > On Wed, Mar 24, 2021 at 08:06:24PM +0000, 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           | 1 +
> >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--
> >>  2 files changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> >> index 302fe3d3e86b..6557fb1e59b5 100644
> >> --- a/include/libcamera/buffer.h
> >> +++ b/include/libcamera/buffer.h
> >> @@ -49,6 +49,7 @@ public:
> >>      Request *request() const { return request_; }
> >>      void setRequest(Request *request) { request_ = request; }
> >>      const FrameMetadata &metadata() const { return metadata_; }
> >> +    void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }
> >
> > Missing documentation ? I think we should document the function, but
> > also explained at a higher level in the FrameBuffer documentation how
> > it's meant to be used.
> >
> >>
> >>      unsigned int cookie() const { return cookie_; }
> >>      void setCookie(unsigned int cookie) { cookie_ = cookie; }
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index fc40dcb381ea..44bd5a9d042b 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -1256,8 +1256,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);
> >> +            }
> >>
> >>              frameInfos_.remove(info);
> >>              pipe_->completeRequest(request);
> >
> > Pipeline handlers never set the buffer status explicitly today, as this
> > is handled in V4L2VideoDevice. As you've found out, it causes issues we
> > requests whose buffers haven't been queued. Adding a
> > FrameBuffer::cancel() function makes the API a bit asymmetrical, which
> > bothers me a bit. There's also the issue that this function shouldn't be
> > visible to applications.
> >
> > I wonder, would it be better to create a Request::cancel() function
> > instead, that will cancel all buffers automatically and complete the
> > request ?
>

Thought I might chime in as I had to deal with exactly this scenario
for the Raspberry Pi pipeline handler.  My handling of incomplete
requests can be found at [1].  It's not the nicest bit of code, but it does
the job.

If there was a Request::cancel() method, that would significantly tidy
up the function at [1], so it has my vote.  I recall mentioning this ages
back when we first released our pipeline handler, and there was
agreement that such a function would eventually be needed.  That time
looks to be now :-)

Thanks,
Naush

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


>
> I like that latest idea better.
> The comment in cio2BufferReady() is "If the buffer is cancelled" => but
> isn't it indeed the request which is cancelled :-) ?
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210324/e6d6b777/attachment.htm>