[libcamera-devel] [PATCH v8 04/17] libcamera: pipeline: ipu3: Don't rely on bufferCount
Paul Elder
paul.elder at ideasonboard.com
Thu Dec 1 13:28:33 CET 2022
On Tue, Aug 24, 2021 at 04:56:23PM -0300, Nícolas F. R. A. Prado wrote:
> Currently the ipu3 pipeline handler relies on bufferCount to decide on
> the number of V4L2 buffer slots to reserve as well as for the number of
> buffers to allocate internally for the CIO2 raw buffers and
> parameter-statistics ImgU buffer pairs. Instead, the number of internal
> buffers should be the minimum required by the pipeline to keep the
> requests flowing without frame drops, in order to avoid wasting memory,
> while the number of V4L2 buffer slots should be greater than the
> expected number of requests queued by the application, in order to avoid
> thrashing dmabuf mappings, which would degrade performance.
>
> Stop relying on bufferCount for these numbers and instead set them to
> appropriate, and independent, constants.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
>
> Changes in v8:
> - New
>
> src/libcamera/pipeline/ipu3/cio2.cpp | 6 +++---
> src/libcamera/pipeline/ipu3/cio2.h | 18 +++++++++++++++++-
> src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------
> src/libcamera/pipeline/ipu3/imgu.h | 15 ++++++++++++++-
> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++------------
> 5 files changed, 48 insertions(+), 23 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 1bcd580e251c..b940a0f6d7d6 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -225,13 +225,13 @@ int CIO2Device::exportBuffers(unsigned int count,
> return output_->exportBuffers(count, buffers);
> }
>
> -int CIO2Device::start()
> +int CIO2Device::start(unsigned int bufferSlotCount)
> {
> - int ret = output_->exportBuffers(CIO2_BUFFER_COUNT, &buffers_);
> + int ret = output_->exportBuffers(kCIO2InternalBufferCount, &buffers_);
> if (ret < 0)
> return ret;
>
> - ret = output_->importBuffers(CIO2_BUFFER_COUNT);
> + ret = output_->importBuffers(bufferSlotCount);
> if (ret)
> LOG(IPU3, Error) << "Failed to import CIO2 buffers";
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index f28e9f1ddf42..ab915b6a16fa 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -45,7 +45,7 @@ public:
> int exportBuffers(unsigned int count,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> - int start();
> + int start(unsigned int bufferSlotCount);
> int stop();
>
> CameraSensor *sensor() { return sensor_.get(); }
> @@ -69,6 +69,22 @@ private:
>
> std::vector<std::unique_ptr<FrameBuffer>> buffers_;
> std::queue<FrameBuffer *> availableBuffers_;
> +
> + /*
> + * This many internal buffers for the CIO2 ensures that the pipeline
> + * runs smoothly, without frame drops. This number considers:
> + * - one buffer being DMA'ed to in CIO2
> + * - one buffer programmed by the CIO2 as the next buffer
> + * - one buffer under processing in ImgU
> + * - one extra idle buffer queued to CIO2, to account for possible
> + * delays in requeuing the buffer from ImgU back to CIO2
> + *
> + * Transient situations can arise when one of the parts, CIO2 or ImgU,
> + * finishes its processing first and experiences a lack of buffers, but
> + * they will shortly after return to the state described above as the
> + * other part catches up.
> + */
> + static constexpr unsigned int kCIO2InternalBufferCount = 4;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 317e482a1498..3484b378c189 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -593,22 +593,22 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> /**
> * \brief Allocate buffers for all the ImgU video devices
> */
> -int ImgUDevice::allocateBuffers(unsigned int bufferCount)
> +int ImgUDevice::allocateBuffers(unsigned int bufferSlotCount)
> {
> /* Share buffers between CIO2 output and ImgU input. */
> - int ret = input_->importBuffers(bufferCount);
> + int ret = input_->importBuffers(bufferSlotCount);
> if (ret) {
> LOG(IPU3, Error) << "Failed to import ImgU input buffers";
> return ret;
> }
>
> - ret = param_->allocateBuffers(bufferCount, ¶mBuffers_);
> + ret = param_->allocateBuffers(kImgUInternalBufferCount, ¶mBuffers_);
> if (ret < 0) {
> LOG(IPU3, Error) << "Failed to allocate ImgU param buffers";
> goto error;
> }
>
> - ret = stat_->allocateBuffers(bufferCount, &statBuffers_);
> + ret = stat_->allocateBuffers(kImgUInternalBufferCount, &statBuffers_);
> if (ret < 0) {
> LOG(IPU3, Error) << "Failed to allocate ImgU stat buffers";
> goto error;
> @@ -619,13 +619,13 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)
> * corresponding stream is active or inactive, as the driver needs
> * buffers to be requested on the V4L2 devices in order to operate.
> */
> - ret = output_->importBuffers(bufferCount);
> + ret = output_->importBuffers(bufferSlotCount);
> if (ret < 0) {
> LOG(IPU3, Error) << "Failed to import ImgU output buffers";
> goto error;
> }
>
> - ret = viewfinder_->importBuffers(bufferCount);
> + ret = viewfinder_->importBuffers(bufferSlotCount);
> if (ret < 0) {
> LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers";
> goto error;
> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> index 9d4915116087..61ccd17c2e31 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.h
> +++ b/src/libcamera/pipeline/ipu3/imgu.h
> @@ -61,7 +61,7 @@ public:
> outputFormat);
> }
>
> - int allocateBuffers(unsigned int bufferCount);
> + int allocateBuffers(unsigned int bufferSlotCount);
> void freeBuffers();
>
> int start();
> @@ -96,6 +96,19 @@ private:
>
> std::string name_;
> MediaDevice *media_;
> +
> + /*
> + * This many internal buffers (or rather parameter and statistics buffer
> + * pairs) for the ImgU ensures that the pipeline runs smoothly, without
> + * frame drops. This number considers:
> + * - three buffers queued to the CIO2 (Since these buffers are bound to
> + * CIO2 buffers before queuing to the CIO2)
> + * - one buffer under processing in ImgU
> + *
> + * \todo Update this number when we make these buffers only get added to
> + * the FrameInfo after the raw buffers are dequeued from CIO2.
> + */
> + static constexpr unsigned int kImgUInternalBufferCount = 4;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index eaa503a33208..cc519ae6adbe 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -156,7 +156,7 @@ private:
> int initControls(IPU3CameraData *data);
> int registerCameras();
>
> - int allocateBuffers(Camera *camera);
> + int allocateBuffers(Camera *camera, unsigned int bufferSlotCount);
> int freeBuffers(Camera *camera);
>
> ImgUDevice imgu0_;
> @@ -165,6 +165,8 @@ private:
> MediaDevice *imguMediaDev_;
>
> std::vector<IPABuffer> ipaBuffers_;
> +
> + static constexpr unsigned int kIPU3BufferSlotCount = 16;
> };
>
> IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data)
> @@ -693,20 +695,14 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> * In order to be able to start the 'viewfinder' and 'stat' nodes, we need
> * memory to be reserved.
> */
> -int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> +int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
> + unsigned int bufferSlotCount)
> {
> IPU3CameraData *data = cameraData(camera);
> ImgUDevice *imgu = data->imgu_;
> - unsigned int bufferCount;
> int ret;
>
> - bufferCount = std::max({
> - data->outStream_.configuration().bufferCount,
> - data->vfStream_.configuration().bufferCount,
> - data->rawStream_.configuration().bufferCount,
> - });
> -
> - ret = imgu->allocateBuffers(bufferCount);
> + ret = imgu->allocateBuffers(bufferSlotCount);
> if (ret < 0)
> return ret;
>
> @@ -758,7 +754,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
> int ret;
>
> /* Allocate buffers for internal pipeline usage. */
> - ret = allocateBuffers(camera);
> + ret = allocateBuffers(camera, kIPU3BufferSlotCount);
> if (ret)
> return ret;
>
> @@ -770,7 +766,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
> * Start the ImgU video devices, buffers will be queued to the
> * ImgU output and viewfinder when requests will be queued.
> */
> - ret = cio2->start();
> + ret = cio2->start(kIPU3BufferSlotCount);
> if (ret)
> goto error;
>
> --
> 2.33.0
>
More information about the libcamera-devel
mailing list