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