[libcamera-devel] [PATCH v4 8/9] libcamera: pipeline: raspberrypi: Add more robust stream buffer logic
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 22 17:42:48 CEST 2020
Hi Naush,
Thank you for the patch.
On Tue, Jul 21, 2020 at 01:35:51PM +0100, Naushir Patuck wrote:
> On Tue, 21 Jul 2020 at 12:23, Kieran Bingham wrote:
> > On 20/07/2020 10:13, Naushir Patuck wrote:
> > > Add further queueing into the RPiStream object to ensure that we always
> > > follow the buffer ordering (be it internal or external) given by
> > > incoming Requests.
> > >
> > > This is essential, otherwise we risk dropping frames that are meant to
> > > be part of a Request, and can cause the pipeline to stall indefinitely.
> > > This also prevents any possibility of mismatched frame buffers going
> > > through the pipeline and out to the application.
> >
> > Sounds helpful... some questions below ... but I suspect I've just
> > misinterpreted something...
> >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > > .../pipeline/raspberrypi/rpi_stream.cpp | 70 ++++++++++++++++---
> > > .../pipeline/raspberrypi/rpi_stream.h | 12 +++-
> > > 2 files changed, 71 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > index 02f8d3e0..6635a751 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer)
> > > */
> > > if (!buffer) {
> > > if (availableBuffers_.empty()) {
> > > - LOG(RPISTREAM, Warning) << "No buffers available for "
> > > + LOG(RPISTREAM, Info) << "No buffers available for "
> > > << name_;
> > > - return -EINVAL;
> > > + /*
> > > + * Note that we need to requeue an internal buffer as soon
> > > + * as one becomes available.
> > > + */
> > > + requeueBuffers_.push(nullptr);
I'm having a hard time parsing this patch. "requeue" sounds like the
queue contains buffers that are completed and need to be given back to
the device, and I think that's not what is happening here. Or is it ?
It also seems that the buffers are queued to the device in
returnBuffers(), which is meant to signal completion. This makes the
code flow very opaque to me. I'm afraid I just can't provide meaningful
review comments as I don't understand what's going on.
> > > + return 0;
> > > }
> > >
> > > buffer = availableBuffers_.front();
> > > availableBuffers_.pop();
> > > }
> > >
> > > - LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> > > - << " for " << name_;
> > > + /*
> > > + * If no earlier requests are pending to be queued we can go ahead and
> > > + * queue the buffer into the device.
> > > + */
> > > + if (requeueBuffers_.empty())
> > > + return queueToDevice(buffer);
> > >
> > > - int ret = dev_->queueBuffer(buffer);
> > > - if (ret) {
> > > - LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> > > - << name_;
> > > - }
> > > + /*
> > > + * There are earlier buffers to be queued, so this bufferm ust go on
> >
> > s/bufferm ust/buffer must/
> >
> > That could be fixed up while applying. It's minor.
> >
> > > + * the waiting list.
> > > + */
> > > + requeueBuffers_.push(buffer);
> > >
> > > - return ret;
> > > + return 0;
> > > }
> > >
> > > void RPiStream::returnBuffer(FrameBuffer *buffer)
> > > @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer)
> > > /* This can only be called for external streams. */
> > > assert(external_);
> > >
> > > + /* Push this buffer back into the queue to be used again. */
> > > availableBuffers_.push(buffer);
> > > +
> > > + /*
> > > + * Do we have any buffers that are waiting to be queued?
> > > + * If so, do it now as availableBuffers_ will not be empty.
> > > + */
> > > + while (!requeueBuffers_.empty()) {
> > > + FrameBuffer *buffer = requeueBuffers_.front();
> > > +
> > > + if (!buffer) {
> > > + /*
> > > + * We want to queue an internal buffer, but none
> > > + * are available. Can't do anything, quit the loop.
> > > + */
> > > + if (availableBuffers_.empty())
> > > + break;
> >
> > Can this happen?
> >
> > This looks like:
> >
> > while(requeueBuffer exist)
> > buffer = first buffer from requeueBuffers
> > if (no buffer) {
> > /* unreachable code ? */
> > }
> >
>
> Yes it is possible, having a nullptr element in requeueBuffers_ means
> a Request did not provide a buffer for that stream, so we have to use
> a buffer from availableBuffers_ instead. This buffer will not be
> returned out to the applications. Hope that makes sense.
>
> > > +
> > > + /*
> > > + * We want to queue an internal buffer, and at least one
> > > + * is available.
> > > + */
> > > + buffer = availableBuffers_.front();
> > > + availableBuffers_.pop();
> > > + }
> > > +
> > > + requeueBuffers_.pop();
> > > + queueToDevice(buffer);
> > > + }
> > > }
> > >
> > > bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> > > @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> > > void RPiStream::clearBuffers()
> > > {
> > > availableBuffers_ = std::queue<FrameBuffer *>{};
> > > + requeueBuffers_ = std::queue<FrameBuffer *>{};
> > > internalBuffers_.clear();
> > > bufferList_.clear();
> > > }
> > >
> > > +int RPiStream::queueToDevice(FrameBuffer *buffer)
> > > +{
> > > + LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> > > + << " for " << name_;
> > > +
> > > + int ret = dev_->queueBuffer(buffer);
> > > + if (ret)
> > > + LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> > > + << name_;
> > > + return ret;
> > > +}
> > > +
> > > } /* namespace RPi */
> > >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > index af9c2ad2..6222c867 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > @@ -52,6 +52,7 @@ public:
> > >
> > > private:
> > > void clearBuffers();
> > > + int queueToDevice(FrameBuffer *buffer);
> > > /*
> > > * Indicates that this stream is active externally, i.e. the buffers
> > > * might be provided by (and returned to) the application.
> > > @@ -63,7 +64,7 @@ private:
> > > std::string name_;
> > > /* The actual device stream. */
> > > std::unique_ptr<V4L2VideoDevice> dev_;
> > > - /* All framebuffers associated with this device stream. */
> > > + /* All frame buffers associated with this device stream. */
> > > std::vector<FrameBuffer *> bufferList_;
> > > /*
> > > * List of frame buffer that we can use if none have been provided by
> > > @@ -71,6 +72,15 @@ private:
> > > * buffers exported internally.
> > > */
> > > std::queue<FrameBuffer *> availableBuffers_;
> > > + /*
> > > + * List of frame buffer that are to be re-queued into the device.
> >
> > "List of frame buffers ..."
> >
> > > + * A nullptr indicates any internal buffer can be used (from availableBuffers_),
> > > + * whereas a valid pointer indicates an external buffer to be queued.
> >
> > Oh ... are we pushing nullptrs on to the queues? ... perhaps that
> > answers my unreachable code question above ...
> >
> > > + *
> > > + * Ordering buffers to be queued is important here as it must match the
> > > + * requests coming from the application.
> > > + */
> > > + std::queue<FrameBuffer *> requeueBuffers_;
> > > /*
> > > * This is a list of buffers exported internally. Need to keep this around
> > > * as the stream needs to maintain ownership of these buffers.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list