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

[Kieran Bingham]

Hi Naush,

On 24/03/2021 21:44, Naushir Patuck wrote:
> Hi all,
> 
> 
> On Wed, 24 Mar 2021 at 20:34, Jean-Michel Hautbois
> <jeanmichel.hautbois at ideasonboard.com
> <mailto: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
>     <mailto: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 for chiming in! I'm afraid to admit, I hadn't really looked at
the RPi pipeline handler in detail to check how this was handled there,
and I should have indeed.

And clearly this is going to be a repeating pattern for pipeline
handlers so indeed ... I suspect I know what I'm doing tomorrow.

I think for the purposes of the other patches in this series, I'm going
to split this thread out.

The others in this series all fix issues directly experienced on the
IPU3 pipeline handler, where as this specific issue (not to reduce the
value of it) is 'only' an uninitialised variable access warning from
valgrind.

But if we can clear up the clean-up/cancellation paths for all pipeline
handlers with common code, hopefully that will lead to well used/tested
code paths ...

I'm a bit fearful of how the internal buffers may get tracked / handled
on cancellation, and what races that might cause - but that's a tomorrow
problem. Now if only tomorrow was more than 2 hours away ;-)

--
Kieran



> 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
>     <mailto:libcamera-devel at lists.libcamera.org>
>     https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran