<div dir="ltr"><div dir="ltr">Hi Laurent and Kieran,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 20 Apr 2021 at 23:33, 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 Kieran,<br>
<br>
Thank you for the patch.<br>
<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></blockquote><div><br></div><div>Maybe I misunderstood the above, but I think you can tell in the above</div><div>example if the raw buffer had been completed as part of a request and</div><div>the embedded buffer had not. This can be checked by looking at the</div><div>FrameBuffer::request() value which gets nulled on a Request::completeBuffer().</div><div>The code in our pipeline handler to deal with cancelling requests [1] does</div><div>have to deal with this, so I assume it is valid.</div><div><br></div><div>[1] <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482</a></div><div><br></div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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>