<div dir="ltr"><div dir="ltr">Hi Nicolas,<div><br></div><div>Thank you for your work!</div><div>The change looks good, just some (mostly cosmetic related) comments from me below.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 24 Aug 2021 at 20:57, Nícolas F. R. A. Prado <<a href="mailto:nfraprado@collabora.com">nfraprado@collabora.com</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">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>
<br>
---<br>
<br>
Changes in v8:<br>
- Reworked buffer allocation handling in the raspberrypi pipeline handler<br>
- New<br>
<br>
.../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++--------<br>
.../pipeline/raspberrypi/rpi_stream.cpp | 28 +++++++++---<br>
.../pipeline/raspberrypi/rpi_stream.h | 24 ++++++++--<br>
3 files changed, 69 insertions(+), 28 deletions(-)<br>
<br>
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
index 8a1040aa46be..a7c1cc1d5001 100644<br>
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
@@ -262,6 +262,25 @@ public:<br>
bool match(DeviceEnumerator *enumerator) 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></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+<br>
+ /*<br>
+ * The number of buffers to allocate internally for the external<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+ * 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></blockquote><div><br></div><div>s/external/ISP since the Unicam RAW image can also be an external stream.<br></div><div><br></div><div>Additionally, I would add that not only are these buffers used to drop the first few</div><div>frames, but they are also used when the user does not supply a buffer in the Request.</div><div>For example, if a low res is not requested by the application, we use an internal</div><div>buffer since low res images are used for our colour denoise algorithm.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ static constexpr unsigned int kInternalDropoutBufferCount = 2;<br>
+<br></blockquote><div><br></div><div>I would probably rename these variables like:</div><div><br></div><div>s/kInternalRawBufferCount/unicamInternalBufferCount/</div><div>s/kInternalDropoutBufferCount/ispInternalBufferCount/</div><div><br></div><div>I know that other libcamera source files use the "k" const prefix, but Raspberry Pi</div><div>source files don't, so, I would drop it to be consistent. The reason for the rename is</div><div>to more closely match the buffer usage as per the above comment.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
RPiCameraData *cameraData(Camera *camera)<br>
{<br>
return static_cast<RPiCameraData *>(camera->_d());<br>
@@ -971,14 +990,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
return false;<br>
<br>
/* Locate and open the unicam video streams. */<br>
- data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"));<br>
- data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"));<br>
+ data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicam_->getEntityByName("unicam-embedded"), kInternalRawBufferCount);<br>
+ data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicam_->getEntityByName("unicam-image"), kInternalRawBufferCount);<br>
<br>
/* Tag the ISP input stream as an import stream. */<br>
- data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), true);<br>
- data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"));<br>
- data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));<br>
- data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));<br>
+ data->isp_[Isp::Input] = RPi::Stream("ISP Input", isp_->getEntityByName("bcm2835-isp0-output0"), 0, kInternalRawBufferCount, true);<br>
+ data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", isp_->getEntityByName("bcm2835-isp0-capture1"), kInternalDropoutBufferCount);<br>
+ data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"), kInternalDropoutBufferCount);<br>
+ data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"), kInternalDropoutBufferCount);<br>
<br></blockquote><div><br></div><div>Some of these lines are over the 120 character limit and will need wrapping. </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">
/* Wire up all the buffer connections. */<br>
data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);<br>
@@ -1153,19 +1172,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)<br>
RPiCameraData *data = cameraData(camera);<br>
int ret;<br>
<br>
- /*<br>
- * Decide how many internal buffers to allocate. For now, simply look<br>
- * at how many external buffers will be provided. We'll need to improve<br>
- * this logic. However, we really must have all streams allocate the same<br>
- * number of buffers to simplify error handling in queueRequestDevice().<br>
- */<br>
- unsigned int maxBuffers = 0;<br>
- for (const Stream *s : camera->streams())<br>
- if (static_cast<const RPi::Stream *>(s)->isExternal())<br>
- maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);<br>
-<br>
+ /* Allocate internal buffers. */<br>
for (auto const stream : data->streams_) {<br>
- ret = stream->prepareBuffers(maxBuffers);<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 b3265d0e8aab..98572abc2f61 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 = external_ ? 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>
@@ -102,12 +112,16 @@ int Stream::prepareBuffers(unsigned int count)<br>
for (auto const &buffer : internalBuffers_)<br>
availableBuffers_.push(buffer.get());<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>
- 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 f1ac715f4221..7310ba1cc371 100644<br>
--- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h<br>
+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h<br>
@@ -36,9 +36,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_(ipa::RPi::MaskID)<br>
+ dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(ipa::RPi::MaskID),<br>
+ internalBufferCount_(internalBufferCount),<br>
+ bufferSlotCount_(bufferSlotCount)<br></blockquote><div><br></div><div>I wonder if we can simplify this constructor by removing "bool importOnly = false"?</div><div>In your patch, If a non-zero bufferSlotCount is provided, it implies importOnly == true.</div><div>Could we use that to remove the latter? Perhaps renaming bufferSlotCount to</div><div>importSlots to be more clear?</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>
}<br>
<br>
@@ -57,7 +61,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>
@@ -65,6 +69,8 @@ public:<br>
void releaseBuffers();<br>
<br>
private:<br>
+ static constexpr unsigned int kRPIExternalBufferSlotCount = 16;<br>
+<br></blockquote><div><br></div><div>Similar to earlier, I would remove the "k" prefix.</div><div><br></div><div>Regards,</div><div>Naush</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">
class IdGenerator<br>
{<br>
public:<br>
@@ -127,6 +133,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.33.0<br>
<br>
</blockquote></div></div>