[libcamera-devel] [PATCH v8 03/17] libcamera: pipeline: raspberrypi: Don't rely on bufferCount
Nícolas F. R. A. Prado
nfraprado at collabora.com
Tue Aug 24 21:56:22 CEST 2021
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>
---
Changes in v8:
- Reworked buffer allocation handling in the raspberrypi pipeline handler
- New
.../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++--------
.../pipeline/raspberrypi/rpi_stream.cpp | 28 +++++++++---
.../pipeline/raspberrypi/rpi_stream.h | 24 ++++++++--
3 files changed, 69 insertions(+), 28 deletions(-)
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 8a1040aa46be..a7c1cc1d5001 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -262,6 +262,25 @@ public:
bool match(DeviceEnumerator *enumerator) 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());
@@ -971,14 +990,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
return false;
/* Locate and open the unicam video streams. */
- data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));
- data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"));
+ data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"), kInternalRawBufferCount);
+ data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"), kInternalRawBufferCount);
/* Tag the ISP input stream as an import stream. */
- data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);
- data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));
- data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));
- data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));
+ data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), 0, kInternalRawBufferCount, true);
+ data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"), kInternalDropoutBufferCount);
+ data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"), kInternalDropoutBufferCount);
+ data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"), kInternalDropoutBufferCount);
/* Wire up all the buffer connections. */
data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);
@@ -1153,19 +1172,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
RPiCameraData *data = cameraData(camera);
int ret;
- /*
- * Decide how many internal buffers to allocate. For now, simply look
- * at how many external buffers will be provided. We'll need to improve
- * this logic. However, we really must have all streams allocate the same
- * number of buffers to simplify error handling in queueRequestDevice().
- */
- unsigned int maxBuffers = 0;
- for (const Stream *s : camera->streams())
- if (static_cast<const RPi::Stream *>(s)->isExternal())
- maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
-
+ /* Allocate internal buffers. */
for (auto const stream : data->streams_) {
- ret = stream->prepareBuffers(maxBuffers);
+ 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 b3265d0e8aab..98572abc2f61 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 = external_ ? 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;
@@ -102,12 +112,16 @@ int Stream::prepareBuffers(unsigned int count)
for (auto const &buffer : internalBuffers_)
availableBuffers_.push(buffer.get());
}
-
- /* 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_;
}
- 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 f1ac715f4221..7310ba1cc371 100644
--- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
@@ -36,9 +36,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_(ipa::RPi::MaskID)
+ dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(ipa::RPi::MaskID),
+ internalBufferCount_(internalBufferCount),
+ bufferSlotCount_(bufferSlotCount)
{
}
@@ -57,7 +61,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);
@@ -65,6 +69,8 @@ public:
void releaseBuffers();
private:
+ static constexpr unsigned int kRPIExternalBufferSlotCount = 16;
+
class IdGenerator
{
public:
@@ -127,6 +133,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.33.0
More information about the libcamera-devel
mailing list