[libcamera-devel] [PATCH v8 03/17] libcamera: pipeline: raspberrypi: Don't rely on bufferCount
Naushir Patuck
naush at raspberrypi.com
Wed Aug 25 11:26:39 CEST 2021
Hi Nicolas,
Thank you for your work!
The change looks good, just some (mostly cosmetic related) comments from me
below.
On Tue, 24 Aug 2021 at 20:57, Nícolas F. R. A. Prado <
nfraprado at collabora.com> wrote:
> 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
> + */
>
s/external/ISP since the Unicam RAW image can also be an external stream.
Additionally, I would add that not only are these buffers used to drop the
first few
frames, but they are also used when the user does not supply a buffer in
the Request.
For example, if a low res is not requested by the application, we use an
internal
buffer since low res images are used for our colour denoise algorithm.
> + static constexpr unsigned int kInternalDropoutBufferCount = 2;
> +
>
I would probably rename these variables like:
s/kInternalRawBufferCount/unicamInternalBufferCount/
s/kInternalDropoutBufferCount/ispInternalBufferCount/
I know that other libcamera source files use the "k" const prefix, but
Raspberry Pi
source files don't, so, I would drop it to be consistent. The reason for
the rename is
to more closely match the buffer usage as per the above comment.
> 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);
>
>
Some of these lines are over the 120 character limit and will need
wrapping.
> /* 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)
>
I wonder if we can simplify this constructor by removing "bool importOnly =
false"?
In your patch, If a non-zero bufferSlotCount is provided, it implies
importOnly == true.
Could we use that to remove the latter? Perhaps renaming bufferSlotCount to
importSlots to be more clear?
> {
> }
>
> @@ -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;
> +
>
Similar to earlier, I would remove the "k" prefix.
Regards,
Naush
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210825/c8c168ba/attachment-0001.htm>
More information about the libcamera-devel
mailing list