<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 21 Apr 2021 at 10:09, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Wed, Apr 21, 2021 at 09:58:07AM +0100, Naushir Patuck wrote:<br>
> On Tue, 20 Apr 2021 at 23:33, Laurent Pinchart wrote:<br>
> > On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote:<br>
> > > On 20/04/2021 19:56, Naushir Patuck wrote:<br>
> > > > On Tue, 20 Apr 2021 at 14:07, Kieran Bingham wrote:<br>
> > > > ><br>
> > > > > When the CIO2 returns a cancelled buffer, we will not queue buffers<br>
> > > > > to the IMGU.<br>
> > > > ><br>
> > > > > These buffers should be explicitly marked as cancelled to ensure<br>
> > > > > the application knows there is no valid metadata or frame data<br>
> > > > > provided in the buffer.<br>
> > > > ><br>
> > > > > Provide a cancel() method on the FrameBuffer to allow explicitly<br>
> > > > > cancelling a buffer.<br>
> > > > ><br>
> > > > > Signed-off-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> > > > > ---<br>
> > > > > include/libcamera/buffer.h | 2 ++<br>
> > > > > src/libcamera/buffer.cpp | 8 ++++++++<br>
> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--<br>
> > > > > 3 files changed, 15 insertions(+), 2 deletions(-)<br>
> > > > ><br>
> > > > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h<br>
> > > > > index 620f8a66f6a2..e0af00900409 100644<br>
> > > > > --- a/include/libcamera/buffer.h<br>
> > > > > +++ b/include/libcamera/buffer.h<br>
> > > > > @@ -53,6 +53,8 @@ public:<br>
> > > > > unsigned int cookie() const { return cookie_; }<br>
> > > > > void setCookie(unsigned int cookie) { cookie_ = cookie; }<br>
> > > > ><br>
> > > > > + void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }<br>
> > > > > +<br>
> > > > > private:<br>
> > > > > LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)<br>
> > > > ><br>
> > > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp<br>
> > > > > index 75b2693281a7..7635226b9045 100644<br>
> > > > > --- a/src/libcamera/buffer.cpp<br>
> > > > > +++ b/src/libcamera/buffer.cpp<br>
> > > > > @@ -226,6 +226,14 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)<br>
> > > > > * core never modifies the buffer cookie.<br>
> > > > > */<br>
> > > > ><br>
> > > > > +/**<br>
> > > > > + * \fn FrameBuffer::cancel()<br>
> > > > > + * \brief Marks the buffer as cancelled<br>
> > > > > + *<br>
> > > > > + * If a buffer is not used by a request, it shall be marked as cancelled to<br>
> > > > > + * indicate that the metadata is invalid.<br>
> > > > > + */<br>
> > > > > +<br>
> > > > > /**<br>
> > > > > * \class MappedBuffer<br>
> > > > > * \brief Provide an interface to support managing memory mapped buffers<br>
> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> > > > > index 51446fcf5bc1..73306cea6b37 100644<br>
> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> > > > > @@ -1257,8 +1257,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)<br>
> > > > ><br>
> > > > > /* If the buffer is cancelled force a complete of the whole request. */<br>
> > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {<br>
> > > > > - for (auto it : request->buffers())<br>
> > > > > - pipe_->completeBuffer(request, it.second);<br>
> > > > > + for (auto it : request->buffers()) {<br>
> > > > > + FrameBuffer *b = it.second;<br>
> > > > > + b->cancel();<br>
> > > > > + pipe_->completeBuffer(request, b);<br>
> > > > > + }<br>
> > > ><br>
> > > > Would you consider adding a Request::cancel() method to the API that pipeline<br>
> > > > handlers can invoke to essentially perform the above logic? I found it a bit<br>
> > > > confusing having to call completeBuffer on a cancelled buffer, and completeRequest<br>
> > > > to complete cancelling the request if you see what I mean? This is probably a bit<br>
> > > > more relevant to the Raspberry Pi pipeline handler where we queue up requests<br>
> > > > if required.<br>
> > ><br>
> > > I think this could be helpful sure, but maybe as a private/internal call<br>
> > > only (not in the public API). That might be possible now that we have<br>
> > > Request as an Extensible class later in this series.<br>
> > > (In fact, this Buffer->cancel() is public ... eep?)<br>
> > ><br>
> > > In this instance where the CIO2 (the receiver) has caused the<br>
> > > cancellation, a simple helper makes sense to clear the request entirely<br>
> > > and return it as we know that there are no other resources pending.<br>
> > ><br>
> > > But if this were on the completion path in the IMGU (the ISP), then I'd<br>
> > > be concerned about races with multiple completion handlers trying to<br>
> > > 'cancel' the request ...<br>
> > ><br>
> > > But if we can find ways to simplify handling of shutdown paths I'm all<br>
> > > for it ;-)<br>
> > ><br>
> > > Any thoughts?<br>
> ><br>
> > A Request::cancel() function would make sense I believe. It's certainly<br>
> > useful to cancel requests that have been put in an internal pipeline<br>
> > handler queue, but whose buffers haven't been queued to the device. It<br>
> > could also be used in this case I believe, as there should be no race.<br>
> > If the pipeline handler had queued more than one buffer from the request<br>
> > (including internal buffers) to the device, that would be a different<br>
> > question. For instance, if we queued a raw buffer and an embedded data<br>
> > buffer, they would complete (in the cancelled state) separately, in<br>
> > unknown order, so we couldn't call Request::cancel() sefely in that<br>
> > case.<br>
> <br>
> Maybe I misunderstood the above, but I think you can tell in the above<br>
> example if the raw buffer had been completed as part of a request and<br>
> the embedded buffer had not. This can be checked by looking at the<br>
> FrameBuffer::request() value which gets nulled on a<br>
> Request::completeBuffer().<br>
> The code in our pipeline handler to deal with cancelling requests [1] does<br>
> have to deal with this, so I assume it is valid.<br>
<br>
My point was that the following pseudo-code would be invalid:<br>
<br>
Foo::rawCompletionCallback(FrameBuffer *buffer)<br>
{<br>
if (buffer->metadata().status == Cancelled) {<br>
Request *request = buffer->request();<br>
request->cancel();<br>
request->complete();<br>
return;<br>
}<br>
<br>
...<br>
}<br>
<br>
Foo::embeddedCompletionCallback(FrameBuffer *buffer)<br>
{<br>
if (buffer->metadata().status == Cancelled) {<br>
Request *request = buffer->request();<br>
request->cancel();<br>
request->complete();<br>
return;<br>
}<br>
<br>
...<br>
}<br>
<br>
We would call cancel() and complete() twice. We also can't have the<br>
cancellation handling in Foo::rawCompletionCallback() or<br>
Foo::embeddedCompletionCallback() only, as we don't control in which<br>
order those two will be called.<br></blockquote><div><br></div><div>I got it now! The RPi PH logic is slightly different to the above (and probably</div><div>other PHs) in that there is a state-machine that checks for all buffers to</div><div>be complete before signalling request->complete() in only one place.</div><div><br></div><div>Naush</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
It's not a very complicated issue, and I'm sure we'll come up with a<br>
nice API to handle request completion with cancellation support. My<br>
point is that it requires a bit of careful API design.<br>
<br>
> [1]<br>
> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482</a><br>
> <br>
> > We could start with a first implementation the limits usage of<br>
> > Request::cancel() to the cases where no buffers or a single buffer has<br>
> > been queued to the device, and improve on top, but I'd be interested in<br>
> > hearing proposals about how this could be further improved.<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>