[libcamera-devel] [PATCH v9 04/18] libcamera: pipeline: raspberrypi: Don't rely on bufferCount
Paul Elder
paul.elder at ideasonboard.com
Fri Dec 16 13:29:25 CET 2022
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);
- } 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