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

[Umang Jain]

Hi Paul,

On 12/16/22 5:59 PM, 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,

This is now sounding like the number of internal buffers should be 
allocated based on MinimumRequests property we just defined? Can it be 
then pivoted to Minimum requests property ? I don't see possibility 
where  internal buffers needed to be allocated comes out to be less than 
the camera's MinimumRequests to avoid frame drops.

> while the number of V4L2 buffer slots should be greater than the
> expected number of requests queued by the application, in order to avoid

s/expected number of requests queued by the application/Minimum Requests 
needed to be queued by the application/

I think in this case we want a mutiplier to MinimumRequests so allocate 
larger V4L2 slots, For e.g. in context of this patch,

     kIPU3BufferSlotCount  =  4 * minimumRequests;

My goal is that, instead of defining arbitrary constants for both IMGU 
and CIO2 internal buffers , we better pivot the buffers counts to 
MinimumRequests so it would be more cleaner and logical ?  What do you 
think?
> thrashing dmabuf mappings, which would degrade performance.

Have you checked any performance metrics with increasing internal 
counts?  The comments  (and this series) overwhelming  says "without 
frame drops" throughout - but I am not sure if that's the case in 
reality. But the direction is more or less correct in my opinion.

(The feedback applies to other platforms as well, regarding pivoting 
constants the MinimumRequests property, so I rather not comment on them 
individually)
>
> 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_);
>   	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;
>