[libcamera-devel] [PATCH v9 04/18] libcamera: pipeline: raspberrypi: Don't rely on bufferCount
Paul Elder
paul.elder at ideasonboard.com
Wed Dec 28 03:21:49 CET 2022
Hi Naush,
On Fri, Dec 16, 2022 at 01:39:23PM +0000, Naushir Patuck wrote:
> Hi Paul,
>
> On Fri, 16 Dec 2022 at 12:30, Paul Elder via libcamera-devel <
> libcamera-devel at lists.libcamera.org> wrote:
>
> From: Nícolas F. R. A. Prado <nfraprado at collabora.com>
>
> Currently the raspberrypi pipeline handler relies on bufferCount to
> decide on the number of buffers to allocate internally and for the
> number of V4L2 buffer slots to reserve. Instead, the number of internal
> buffers should be the minimum required by the pipeline to keep the
> requests flowing, in order to avoid wasting memory, while the number of
> V4L2 buffer slots should be greater than the expected number of requests
> queued, in order to avoid thrashing dmabuf mappings, which would degrade
> performance.
>
> Extend the Stream class to receive the number of internal buffers that
> should be allocated, and optionally the number of buffer slots to
> reserve. Use these new member variables to specify better suited numbers
> for internal buffers and buffer slots for each stream individually,
> which also allows us to remove bufferCount.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
>
>
> I'm afraid this is going to conflict badly with my changes in [1] where I've
> made substantial optimisations to the buffer allocation logic :-(
Oh... yeah.
>
> However, if the aim of this commit is to remove the use of bufferCount from
Yeah that's the goal.
> prepareBuffers(), then this ought to be a simple change on top of [1]:
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera
> /pipeline/raspberrypi/raspberrypi.cpp
> index ec416ab655c7..77f529ea0b46 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1511,7 +1511,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>
> for (Stream *s : camera->streams()) {
> if (s == &data->unicam_[Unicam::Image]) {
> - numRawBuffers = s->configuration().bufferCount;
> + numRawBuffers = data->unicam_[Unicam::Image].getBuffers
> ().size();
> /*
> * If the application provides a guarantees that Unicam
> * image buffers will always be provided for the RAW
> stream
Are you saying that this complex patch can just be reduced to that
hunk...?
I have a feeling that this series might make it in first.
> There is also the parallel discussion of using StreamConfig::bufferCout to
> provide applications with minimum buffer/requests required for the pipeline to
> run correctly. So we may not be able to remove this just yet...
This series introduces a MinimumRequests property which reports to
applications the minimum number of requests required for the pipeline to
run without frame drops, so I think this covers that.
Thanks,
Paul
>
> [1] https://patchwork.libcamera.org/project/libcamera/list/?series=3663
>
>
>
> ---
> Changes in v9:
> - rebased
> - I've decided that the buffer allocation decisions that Nícolas
> implemented covered the same cases that were added in
> PipelineHandlerRPi::prepareBuffers(), but in a slightly nicer way,
> especially considering that bufferCount is to be removed from
> StreamConfiguration in this series. Comments welcome, of course.
>
> Changes in v8:
> - Reworked buffer allocation handling in the raspberrypi pipeline handler
> - New
> ---
> .../pipeline/raspberrypi/raspberrypi.cpp | 83 +++++++------------
> .../pipeline/raspberrypi/rpi_stream.cpp | 39 +++++----
> .../pipeline/raspberrypi/rpi_stream.h | 24 +++++-
> 3 files changed, 71 insertions(+), 75 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/
> libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 4641c76f..72502c36 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -341,6 +341,25 @@ public:
> void releaseDevice(Camera *camera) override;
>
> private:
> + /*
> + * The number of buffers to allocate internally for transferring
> raw
> + * frames from the Unicam Image stream to the ISP Input stream. It
> is
> + * defined such that:
> + * - one buffer is being written to in Unicam Image
> + * - one buffer is being processed in ISP Input
> + * - one extra buffer is queued to Unicam Image
> + */
> + static constexpr unsigned int kInternalRawBufferCount = 3;
> +
> + /*
> + * The number of buffers to allocate internally for the external
> + * streams. They're used to drop the first few frames while the IPA
> + * algorithms converge. It is defined such that:
> + * - one buffer is being used on the stream
> + * - one extra buffer is queued
> + */
> + static constexpr unsigned int kInternalDropoutBufferCount = 2;
> +
> RPiCameraData *cameraData(Camera *camera)
> {
> return static_cast<RPiCameraData *>(camera->_d());
> @@ -1221,21 +1240,23 @@ int PipelineHandlerRPi::registerCamera(MediaDevice
> *unicam, MediaDevice *isp, Me
> return -ENOENT;
>
> /* Locate and open the unicam video streams. */
> - data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image",
> unicamImage);
> + data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image",
> unicamImage,
> +
> kInternalRawBufferCount);
>
> /* An embedded data node will not be present if the sensor does not
> support it. */
> MediaEntity *unicamEmbedded = unicam->getEntityByName
> ("unicam-embedded");
> if (unicamEmbedded) {
> - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam
> Embedded", unicamEmbedded);
> + data->unicam_[Unicam::Embedded] =
> + RPi::Stream("Unicam Embedded", unicamEmbedded,
> kInternalRawBufferCount);
> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect
> (data.get(),
>
> &RPiCameraData::unicamBufferDequeue);
> }
>
> /* Tag the ISP input stream as an import stream. */
> - data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0,
> true);
> - data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
> - data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2);
> - data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);
> + data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, 0,
> kInternalRawBufferCount, true);
> + data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1,
> kInternalDropoutBufferCount);
> + data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2,
> kInternalDropoutBufferCount);
> + data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3,
> kInternalDropoutBufferCount);
>
> /* Wire up all the buffer connections. */
> data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get
> (), &RPiCameraData::unicamTimeout);
> @@ -1428,57 +1449,11 @@ int PipelineHandlerRPi::queueAllBuffers(Camera
> *camera)
> int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> {
> RPiCameraData *data = cameraData(camera);
> - unsigned int numRawBuffers = 0;
> int ret;
>
> - for (Stream *s : camera->streams()) {
> - if (isRaw(s->configuration().pixelFormat)) {
> - numRawBuffers = s->configuration().bufferCount;
> - break;
> - }
> - }
> -
> - /* Decide how many internal buffers to allocate. */
> + /* Allocate internal buffers. */
> for (auto const stream : data->streams_) {
> - unsigned int numBuffers;
> - /*
> - * For Unicam, allocate a minimum of 4 buffers as we want
> - * to avoid any frame drops.
> - */
> - constexpr unsigned int minBuffers = 4;
> - if (stream == &data->unicam_[Unicam::Image]) {
> - /*
> - * If an application has configured a RAW stream,
> allocate
> - * additional buffers to make up the minimum, but
> ensure
> - * we have at least 2 sets of internal buffers to
> use to
> - * minimise frame drops.
> - */
> - numBuffers = std::max<int>(2, minBuffers -
> numRawBuffers);
> - } else if (stream == &data->isp_[Isp::Input]) {
> - /*
> - * ISP input buffers are imported from Unicam, so
> follow
> - * similar logic as above to count all the RAW
> buffers
> - * available.
> - */
> - numBuffers = numRawBuffers + std::max<int>(2,
> minBuffers - numRawBuffers);
> -
> - } else if (stream == &data->unicam_[Unicam::Embedded]) {
> - /*
> - * Embedded data buffers are (currently) for
> internal use,
> - * so allocate the minimum required to avoid frame
> drops.
> - */
> - numBuffers = minBuffers;
> - } else {
> - /*
> - * Since the ISP runs synchronous with the IPA and
> requests,
> - * we only ever need one set of internal buffers.
> Any buffers
> - * the application wants to hold onto will already
> be exported
> - * through PipelineHandlerRPi::exportFrameBuffers
> ().
> - */
> - numBuffers = 1;
> - }
> -
> - ret = stream->prepareBuffers(numBuffers);
> + ret = stream->prepareBuffers();
> if (ret < 0)
> return ret;
> }
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/
> libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 2bb10f25..cde04efa 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -84,14 +84,24 @@ void Stream::removeExternalBuffer(FrameBuffer *buffer)
> bufferMap_.erase(id);
> }
>
> -int Stream::prepareBuffers(unsigned int count)
> +int Stream::prepareBuffers()
> {
> + unsigned int slotCount;
> int ret;
>
> if (!importOnly_) {
> - if (count) {
> + /*
> + * External streams overallocate buffer slots in order to
> handle
> + * the buffers queued from applications without degrading
> + * performance. Internal streams only need to have enough
> buffer
> + * slots to have the internal buffers queued.
> + */
> + slotCount = isExternal() ? kRPIExternalBufferSlotCount
> + : internalBufferCount_;
> +
> + if (internalBufferCount_) {
> /* Export some frame buffers for internal use. */
> - ret = dev_->exportBuffers(count, &
> internalBuffers_);
> + ret = dev_->exportBuffers(internalBufferCount_, &
> internalBuffers_);
> if (ret < 0)
> return ret;
>
> @@ -100,23 +110,16 @@ int Stream::prepareBuffers(unsigned int count)
> resetBuffers();
> }
>
> - /* We must import all internal/external exported buffers. *
> /
> - count = bufferMap_.size();
> + } else {
> + /*
> + * An importOnly stream doesn't have its own internal
> buffers,
> + * so it needs to have the number of buffer slots to
> reserve
> + * explicitly declared.
> + */
> + slotCount = bufferSlotCount_;
> }
>
> - /*
> - * If this is an external stream, we must allocate slots for
> buffers that
> - * might be externally allocated. We have no indication of how many
> buffers
> - * may be used, so this might overallocate slots in the buffer
> cache.
> - * Similarly, if this stream is only importing buffers, we do the
> same.
> - *
> - * \todo Find a better heuristic, or, even better, an exact
> solution to
> - * this issue.
> - */
> - if (isExternal() || importOnly_)
> - count = count * 2;
> -
> - return dev_->importBuffers(count);
> + return dev_->importBuffers(slotCount);
> }
>
> int Stream::queueBuffer(FrameBuffer *buffer)
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/
> libcamera/pipeline/raspberrypi/rpi_stream.h
> index b8bd79cf..e63bf02b 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -42,9 +42,13 @@ public:
> {
> }
>
> - Stream(const char *name, MediaEntity *dev, bool importOnly = false)
> + Stream(const char *name, MediaEntity *dev,
> + unsigned int internalBufferCount,
> + unsigned int bufferSlotCount = 0, bool importOnly = false)
> : external_(false), importOnly_(importOnly), name_(name),
> - dev_(std::make_unique<V4L2VideoDevice>(dev)), id_
> (BufferMask::MaskID)
> + dev_(std::make_unique<V4L2VideoDevice>(dev)), id_
> (BufferMask::MaskID),
> + internalBufferCount_(internalBufferCount),
> + bufferSlotCount_(bufferSlotCount)
> {
> }
>
> @@ -63,7 +67,7 @@ public:
> void setExternalBuffer(FrameBuffer *buffer);
> void removeExternalBuffer(FrameBuffer *buffer);
>
> - int prepareBuffers(unsigned int count);
> + int prepareBuffers();
> int queueBuffer(FrameBuffer *buffer);
> void returnBuffer(FrameBuffer *buffer);
>
> @@ -71,6 +75,8 @@ public:
> void releaseBuffers();
>
> private:
> + static constexpr unsigned int kRPIExternalBufferSlotCount = 16;
> +
> class IdGenerator
> {
> public:
> @@ -133,6 +139,18 @@ private:
> /* All frame buffers associated with this device stream. */
> BufferMap bufferMap_;
>
> + /*
> + * The number of buffers that should be allocated internally for
> this
> + * stream.
> + */
> + unsigned int internalBufferCount_;
> +
> + /*
> + * The number of buffer slots that should be reserved for this
> stream.
> + * Only relevant for importOnly streams.
> + */
> + unsigned int bufferSlotCount_;
> +
> /*
> * List of frame buffers that we can use if none have been provided
> by
> * the application for external streams. This is populated by the
> --
> 2.35.1
>
>
More information about the libcamera-devel
mailing list