[libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3: Don't rely on bufferCount

[Jacopo Mondi]

Hi Paul

On Fri, Dec 16, 2022 at 09:29:26PM +0900, Paul Elder via libcamera-devel wrote:
> From: Nícolas F. R. A. Prado <nfraprado at collabora.com>
>
> 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>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v9:
> - rebase
>
> Changes in v8:
> - New
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp |  4 ++--
>  src/libcamera/pipeline/ipu3/cio2.h   | 16 +++++++++++++++-
>  src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------
>  src/libcamera/pipeline/ipu3/imgu.h   | 15 ++++++++++++++-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++------------
>  5 files changed, 45 insertions(+), 22 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index d4e523af..feb69991 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -335,13 +335,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(kBufferCount, &buffers_);
>  	if (ret < 0)
>  		return ret;
>
> -	ret = output_->importBuffers(kBufferCount);
> +	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 68504a2d..8ed208d3 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -30,6 +30,20 @@ struct StreamConfiguration;
>  class CIO2Device
>  {
>  public:
> +	/*
> +	 * 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 kBufferCount = 4;
>
>  	CIO2Device();
> @@ -48,7 +62,7 @@ public:
>  	V4L2SubdeviceFormat getSensorFormat(const std::vector<unsigned int> &mbusCodes,
>  					    const Size &size) const;
>
> -	int start();
> +	int start(unsigned int bufferSlotCount);
>  	int stop();
>
>  	CameraSensor *sensor() { return sensor_.get(); }
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 531879f1..fa920d87 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -576,22 +576,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, &paramBuffers_);
> +	ret = param_->allocateBuffers(kImgUInternalBufferCount, &paramBuffers_);

shouldn't stats and parameters use the same number of buffers
allocated in the input and output devices ? If you run short of stats
or param you will stall the pipeline, won't you ?

>  	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;
> @@ -602,13 +602,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 0af4dd8a..85873961 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.h
> +++ b/src/libcamera/pipeline/ipu3/imgu.h
> @@ -84,7 +84,7 @@ public:
>  					    outputFormat);
>  	}
>
> -	int allocateBuffers(unsigned int bufferCount);
> +	int allocateBuffers(unsigned int bufferSlotCount);
>  	void freeBuffers();
>
>  	int start();
> @@ -119,6 +119,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 bab2db65..4d8fcfeb 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -160,7 +160,7 @@ private:
>  	int updateControls(IPU3CameraData *data);
>  	int registerCameras();
>
> -	int allocateBuffers(Camera *camera);
> +	int allocateBuffers(Camera *camera, unsigned int bufferSlotCount);
>  	int freeBuffers(Camera *camera);
>
>  	ImgUDevice imgu0_;
> @@ -169,6 +169,8 @@ private:
>  	MediaDevice *imguMediaDev_;
>
>  	std::vector<IPABuffer> ipaBuffers_;
> +
> +	static constexpr unsigned int kIPU3BufferSlotCount = 16;
>  };
>
>  IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data)
> @@ -710,20 +712,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;
>
> @@ -781,7 +777,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
>  		return ret;
>
>  	/* Allocate buffers for internal pipeline usage. */
> -	ret = allocateBuffers(camera);
> +	ret = allocateBuffers(camera, kIPU3BufferSlotCount);
>  	if (ret)
>  		return ret;
>
> @@ -795,7 +791,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.35.1
>