| Message ID | 20210420130741.236848-5-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thanks for the patch ! On 20/04/2021 15:07, Kieran Bingham wrote: > When the CIO2 returns a cancelled buffer, we will not queue buffers > to the IMGU. This is my personal preference: s/IMGU/ImgU but do what you want ;-p > 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@ideasonboard.com> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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); > + } > > frameInfos_.remove(info); > pipe_->completeRequest(request); >
Hi Kieran, On Tue, 20 Apr 2021 at 14:07, Kieran Bingham < kieran.bingham@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@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. Regards, Naush > frameInfos_.remove(info); > pipe_->completeRequest(request); > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
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@ideasonboard.com > <mailto:kieran.bingham@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@ideasonboard.com > <mailto:kieran.bingham@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@lists.libcamera.org > <mailto:libcamera-devel@lists.libcamera.org> > https://lists.libcamera.org/listinfo/libcamera-devel > <https://lists.libcamera.org/listinfo/libcamera-devel> >
Hi Kieran, Thank you for the patch. On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote: > On 20/04/2021 19:56, Naushir Patuck wrote: > > On Tue, 20 Apr 2021 at 14:07, 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@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? A Request::cancel() function would make sense I believe. It's certainly useful to cancel requests that have been put in an internal pipeline handler queue, but whose buffers haven't been queued to the device. It could also be used in this case I believe, as there should be no race. If the pipeline handler had queued more than one buffer from the request (including internal buffers) to the device, that would be a different question. For instance, if we queued a raw buffer and an embedded data buffer, they would complete (in the cancelled state) separately, in unknown order, so we couldn't call Request::cancel() sefely in that case. We could start with a first implementation the limits usage of Request::cancel() to the cases where no buffers or a single buffer has been queued to the device, and improve on top, but I'd be interested in hearing proposals about how this could be further improved.
Hi Kieran, Thanks for the patch. On Wed, Apr 21, 2021 at 7:33 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Kieran, > > Thank you for the patch. > > On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote: > > On 20/04/2021 19:56, Naushir Patuck wrote: > > > On Tue, 20 Apr 2021 at 14:07, 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@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? > > A Request::cancel() function would make sense I believe. It's certainly > useful to cancel requests that have been put in an internal pipeline > handler queue, but whose buffers haven't been queued to the device. It > could also be used in this case I believe, as there should be no race. > If the pipeline handler had queued more than one buffer from the request > (including internal buffers) to the device, that would be a different > question. For instance, if we queued a raw buffer and an embedded data > buffer, they would complete (in the cancelled state) separately, in > unknown order, so we couldn't call Request::cancel() sefely in that > case. > > We could start with a first implementation the limits usage of > Request::cancel() to the cases where no buffers or a single buffer has > been queued to the device, and improve on top, but I'd be interested in > hearing proposals about how this could be further improved. > As this patch does a correct thing, Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent and Kieran, On Tue, 20 Apr 2021 at 23:33, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Kieran, > > Thank you for the patch. > > On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote: > > On 20/04/2021 19:56, Naushir Patuck wrote: > > > On Tue, 20 Apr 2021 at 14:07, 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@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? > > A Request::cancel() function would make sense I believe. It's certainly > useful to cancel requests that have been put in an internal pipeline > handler queue, but whose buffers haven't been queued to the device. It > could also be used in this case I believe, as there should be no race. > If the pipeline handler had queued more than one buffer from the request > (including internal buffers) to the device, that would be a different > question. For instance, if we queued a raw buffer and an embedded data > buffer, they would complete (in the cancelled state) separately, in > unknown order, so we couldn't call Request::cancel() sefely in that > case. > Maybe I misunderstood the above, but I think you can tell in the above example if the raw buffer had been completed as part of a request and the embedded buffer had not. This can be checked by looking at the FrameBuffer::request() value which gets nulled on a Request::completeBuffer(). The code in our pipeline handler to deal with cancelling requests [1] does have to deal with this, so I assume it is valid. [1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482 Naush > > We could start with a first implementation the limits usage of > Request::cancel() to the cases where no buffers or a single buffer has > been queued to the device, and improve on top, but I'd be interested in > hearing proposals about how this could be further improved. > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Wed, Apr 21, 2021 at 09:58:07AM +0100, Naushir Patuck wrote: > On Tue, 20 Apr 2021 at 23:33, Laurent Pinchart wrote: > > On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote: > > > On 20/04/2021 19:56, Naushir Patuck wrote: > > > > On Tue, 20 Apr 2021 at 14:07, 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@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? > > > > A Request::cancel() function would make sense I believe. It's certainly > > useful to cancel requests that have been put in an internal pipeline > > handler queue, but whose buffers haven't been queued to the device. It > > could also be used in this case I believe, as there should be no race. > > If the pipeline handler had queued more than one buffer from the request > > (including internal buffers) to the device, that would be a different > > question. For instance, if we queued a raw buffer and an embedded data > > buffer, they would complete (in the cancelled state) separately, in > > unknown order, so we couldn't call Request::cancel() sefely in that > > case. > > Maybe I misunderstood the above, but I think you can tell in the above > example if the raw buffer had been completed as part of a request and > the embedded buffer had not. This can be checked by looking at the > FrameBuffer::request() value which gets nulled on a > Request::completeBuffer(). > The code in our pipeline handler to deal with cancelling requests [1] does > have to deal with this, so I assume it is valid. My point was that the following pseudo-code would be invalid: Foo::rawCompletionCallback(FrameBuffer *buffer) { if (buffer->metadata().status == Cancelled) { Request *request = buffer->request(); request->cancel(); request->complete(); return; } ... } Foo::embeddedCompletionCallback(FrameBuffer *buffer) { if (buffer->metadata().status == Cancelled) { Request *request = buffer->request(); request->cancel(); request->complete(); return; } ... } We would call cancel() and complete() twice. We also can't have the cancellation handling in Foo::rawCompletionCallback() or Foo::embeddedCompletionCallback() only, as we don't control in which order those two will be called. It's not a very complicated issue, and I'm sure we'll come up with a nice API to handle request completion with cancellation support. My point is that it requires a bit of careful API design. > [1] > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482 > > > We could start with a first implementation the limits usage of > > Request::cancel() to the cases where no buffers or a single buffer has > > been queued to the device, and improve on top, but I'd be interested in > > hearing proposals about how this could be further improved.
Hi Laurent, On Wed, 21 Apr 2021 at 10:09, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Wed, Apr 21, 2021 at 09:58:07AM +0100, Naushir Patuck wrote: > > On Tue, 20 Apr 2021 at 23:33, Laurent Pinchart wrote: > > > On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote: > > > > On 20/04/2021 19:56, Naushir Patuck wrote: > > > > > On Tue, 20 Apr 2021 at 14:07, 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@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? > > > > > > A Request::cancel() function would make sense I believe. It's certainly > > > useful to cancel requests that have been put in an internal pipeline > > > handler queue, but whose buffers haven't been queued to the device. It > > > could also be used in this case I believe, as there should be no race. > > > If the pipeline handler had queued more than one buffer from the > request > > > (including internal buffers) to the device, that would be a different > > > question. For instance, if we queued a raw buffer and an embedded data > > > buffer, they would complete (in the cancelled state) separately, in > > > unknown order, so we couldn't call Request::cancel() sefely in that > > > case. > > > > Maybe I misunderstood the above, but I think you can tell in the above > > example if the raw buffer had been completed as part of a request and > > the embedded buffer had not. This can be checked by looking at the > > FrameBuffer::request() value which gets nulled on a > > Request::completeBuffer(). > > The code in our pipeline handler to deal with cancelling requests [1] > does > > have to deal with this, so I assume it is valid. > > My point was that the following pseudo-code would be invalid: > > Foo::rawCompletionCallback(FrameBuffer *buffer) > { > if (buffer->metadata().status == Cancelled) { > Request *request = buffer->request(); > request->cancel(); > request->complete(); > return; > } > > ... > } > > Foo::embeddedCompletionCallback(FrameBuffer *buffer) > { > if (buffer->metadata().status == Cancelled) { > Request *request = buffer->request(); > request->cancel(); > request->complete(); > return; > } > > ... > } > > We would call cancel() and complete() twice. We also can't have the > cancellation handling in Foo::rawCompletionCallback() or > Foo::embeddedCompletionCallback() only, as we don't control in which > order those two will be called. > I got it now! The RPi PH logic is slightly different to the above (and probably other PHs) in that there is a state-machine that checks for all buffers to be complete before signalling request->complete() in only one place. Naush > It's not a very complicated issue, and I'm sure we'll come up with a > nice API to handle request completion with cancellation support. My > point is that it requires a bit of careful API design. > > > [1] > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482 > > > > > We could start with a first implementation the limits usage of > > > Request::cancel() to the cases where no buffers or a single buffer has > > > been queued to the device, and improve on top, but I'd be interested in > > > hearing proposals about how this could be further improved. > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Wed, Apr 21, 2021 at 10:20:27AM +0100, Naushir Patuck wrote: > On Wed, 21 Apr 2021 at 10:09, Laurent Pinchart wrote: > > On Wed, Apr 21, 2021 at 09:58:07AM +0100, Naushir Patuck wrote: > > > On Tue, 20 Apr 2021 at 23:33, Laurent Pinchart wrote: > > > > On Tue, Apr 20, 2021 at 08:13:25PM +0100, Kieran Bingham wrote: > > > > > On 20/04/2021 19:56, Naushir Patuck wrote: > > > > > > On Tue, 20 Apr 2021 at 14:07, 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@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? > > > > > > > > A Request::cancel() function would make sense I believe. It's certainly > > > > useful to cancel requests that have been put in an internal pipeline > > > > handler queue, but whose buffers haven't been queued to the device. It > > > > could also be used in this case I believe, as there should be no race. > > > > If the pipeline handler had queued more than one buffer from the request > > > > (including internal buffers) to the device, that would be a different > > > > question. For instance, if we queued a raw buffer and an embedded data > > > > buffer, they would complete (in the cancelled state) separately, in > > > > unknown order, so we couldn't call Request::cancel() sefely in that > > > > case. > > > > > > Maybe I misunderstood the above, but I think you can tell in the above > > > example if the raw buffer had been completed as part of a request and > > > the embedded buffer had not. This can be checked by looking at the > > > FrameBuffer::request() value which gets nulled on a > > > Request::completeBuffer(). > > > The code in our pipeline handler to deal with cancelling requests [1] does > > > have to deal with this, so I assume it is valid. > > > > My point was that the following pseudo-code would be invalid: > > > > Foo::rawCompletionCallback(FrameBuffer *buffer) > > { > > if (buffer->metadata().status == Cancelled) { > > Request *request = buffer->request(); > > request->cancel(); > > request->complete(); > > return; > > } > > > > ... > > } > > > > Foo::embeddedCompletionCallback(FrameBuffer *buffer) > > { > > if (buffer->metadata().status == Cancelled) { > > Request *request = buffer->request(); > > request->cancel(); > > request->complete(); > > return; > > } > > > > ... > > } > > > > We would call cancel() and complete() twice. We also can't have the > > cancellation handling in Foo::rawCompletionCallback() or > > Foo::embeddedCompletionCallback() only, as we don't control in which > > order those two will be called. > > I got it now! The RPi PH logic is slightly different to the above (and probably > other PHs) in that there is a state-machine that checks for all buffers to > be complete before signalling request->complete() in only one place. Now that we have the same need implemented in different pipeline handlers with different logics, I think we're reached the point where we have enough data to refactor that and create common helpers. That's on my todo list, and I don't think it should be a blocker to already improve the request completion API for some use cases. > > It's not a very complicated issue, and I'm sure we'll come up with a > > nice API to handle request completion with cancellation support. My > > point is that it requires a bit of careful API design. > > > > > [1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1482 > > > > > > > We could start with a first implementation the limits usage of > > > > Request::cancel() to the cases where no buffers or a single buffer has > > > > been queued to the device, and improve on top, but I'd be interested in > > > > hearing proposals about how this could be further improved.
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); + } frameInfos_.remove(info); pipe_->completeRequest(request);
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@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(-)