[libcamera-devel] [PATCH v9 04/18] libcamera: pipeline: raspberrypi: Don't rely on bufferCount
Jacopo Mondi
jacopo at jmondi.org
Fri Dec 16 15:46:33 CET 2022
Hi Paul
On Fri, Dec 16, 2022 at 09:29:25PM +0900, Paul Elder via libcamera-devel 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>
>
> ---
> 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);
For Unicam the old code took into account the external RAW buffers,
and if none was requested 4 buffers where allocated. So it seems a
number between 2 and 4 was possible. That's why you chose 3 in
static constexpr unsigned int kInternalRawBufferCount = 3;
> - } 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;
This was 4 and now it's 3
The rest of the code seems to work as it used to, but it would be
great to have a confirmation from RPi and a test run on the platform
Thanks
j
> - } 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