[libcamera-devel] [PATCH 1/3] pipeline: raspberrypi: Allow Stream::returnBuffer() to handle internal buffers

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.

On Mon, 7 Feb 2022 at 23:36, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Feb 07, 2022 at 03:12:12PM +0000, Naushir Patuck wrote:
> > If Stream::returnBuffer() gets passed an internally allocated buffer, it
> now
> > simply re-queues it back to the device. With this change, the pipeline
> handler
> > code can be simplified slightly as it does not need multiple code paths
> for
> > internally allocated and non-internally allocated buffers.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 53 +++++++++----------
> >  .../pipeline/raspberrypi/rpi_stream.cpp       |  7 ++-
> >  2 files changed, 29 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 49af56edc1f9..0755de84c70c 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1877,34 +1877,29 @@ void RPiCameraData::clearIncompleteRequests()
> >
> >  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream
> *stream)
> >  {
> > -     if (stream->isExternal()) {
> > +     /*
> > +      * It is possible to be here without a pending request, so check
> > +      * that we actually have one to action, otherwise we just return
> > +      * buffer back to the stream.
> > +      */
> > +     Request *request = requestQueue_.empty() ? nullptr :
> requestQueue_.front();
> > +     if (!dropFrameCount_ && request && request->findBuffer(stream) ==
> buffer) {
> >               /*
> > -              * It is possible to be here without a pending request, so
> check
> > -              * that we actually have one to action, otherwise we just
> return
> > -              * buffer back to the stream.
> > +              * Check if this is an externally provided buffer, and if
> > +              * so, we must stop tracking it in the pipeline handler.
> >                */
> > -             Request *request = requestQueue_.empty() ? nullptr :
> requestQueue_.front();
> > -             if (!dropFrameCount_ && request &&
> request->findBuffer(stream) == buffer) {
> > -                     /*
> > -                      * Check if this is an externally provided buffer,
> and if
> > -                      * so, we must stop tracking it in the pipeline
> handler.
> > -                      */
> > -                     handleExternalBuffer(buffer, stream);
> > -                     /*
> > -                      * Tag the buffer as completed, returning it to the
> > -                      * application.
> > -                      */
> > -                     pipe()->completeBuffer(request, buffer);
> > -             } else {
> > -                     /*
> > -                      * This buffer was not part of the Request, or
> there is no
> > -                      * pending request, so we can recycle it.
> > -                      */
> > -                     stream->returnBuffer(buffer);
> > -             }
> > +             handleExternalBuffer(buffer, stream);
> > +             /*
> > +              * Tag the buffer as completed, returning it to the
> > +              * application.
> > +              */
> > +             pipe()->completeBuffer(request, buffer);
> >       } else {
> > -             /* Simply re-queue the buffer to the requested stream. */
> > -             stream->queueBuffer(buffer);
> > +             /*
> > +              * This buffer was not part of the Request, or there is no
> > +              * pending request, so we can recycle it.
>
> As buffer handling code isn't necessarily straightforward to follow, I'd
> expand this to
>
>                  * This buffer was not part of the Request (which happens
> if an
>                  * internal buffer was used for an external stream, or
>                  * unconditionally for internal streams), or there is no
> pending
>                  * request, so we can recycle it.
>

Sure, I can change this.  Or would it be possible  to update before merging
provided there are no further changes needed?

Regards,
Naush


> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +              */
> > +             stream->returnBuffer(buffer);
> >       }
> >  }
> >
> > @@ -2104,7 +2099,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame
> &bayerFrame, FrameBuffer *&em
> >                       FrameBuffer *b = embeddedQueue_.front();
> >                       if (!unicam_[Unicam::Embedded].isExternal() &&
> b->metadata().timestamp < ts) {
> >                               embeddedQueue_.pop();
> > -                             unicam_[Unicam::Embedded].queueBuffer(b);
> > +                             unicam_[Unicam::Embedded].returnBuffer(b);
> >                               embeddedRequeueCount++;
> >                               LOG(RPI, Warning) << "Dropping unmatched
> input frame in stream "
> >                                                 <<
> unicam_[Unicam::Embedded].name();
> > @@ -2140,7 +2135,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame
> &bayerFrame, FrameBuffer *&em
> >                                * the front of the queue. This buffer is
> now orphaned, so requeue
> >                                * it back to the device.
> >                                */
> > -
>  unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);
> > +
>  unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);
> >                               bayerQueue_.pop();
> >                               bayerRequeueCount++;
> >                               LOG(RPI, Warning) << "Dropping unmatched
> input frame in stream "
> > @@ -2158,7 +2153,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame
> &bayerFrame, FrameBuffer *&em
> >
> >                               LOG(RPI, Warning) << "Flushing bayer
> stream!";
> >                               while (!bayerQueue_.empty()) {
> > -
>  unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);
> > +
>  unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);
> >                                       bayerQueue_.pop();
> >                               }
> >                               flushedBuffers = true;
> > @@ -2175,7 +2170,7 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame
> &bayerFrame, FrameBuffer *&em
> >
> >                               LOG(RPI, Warning) << "Flushing embedded
> data stream!";
> >                               while (!embeddedQueue_.empty()) {
> > -
>  unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> > +
>  unicam_[Unicam::Embedded].returnBuffer(embeddedQueue_.front());
> >                                       embeddedQueue_.pop();
> >                               }
> >                               flushedBuffers = true;
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > index a421ad09ba50..f446e1ce66c7 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > @@ -163,8 +163,11 @@ int Stream::queueBuffer(FrameBuffer *buffer)
> >
> >  void Stream::returnBuffer(FrameBuffer *buffer)
> >  {
> > -     /* This can only be called for external streams. */
> > -     ASSERT(external_);
> > +     if (!external_) {
> > +             /* For internal buffers, simply requeue back to the
> device. */
> > +             queueToDevice(buffer);
> > +             return;
> > +     }
> >
> >       /* Push this buffer back into the queue to be used again. */
> >       availableBuffers_.push(buffer);
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220208/d616b88e/attachment-0001.htm>