[libcamera-devel,v9,05/18] libcamera: pipeline: ipu3: Don't rely on bufferCount
diff mbox series

Message ID 20221216122939.256534-6-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • lc-compliance: Add test to queue more requests than hardware depth
Related show

Commit Message

Paul Elder Dec. 16, 2022, 12:29 p.m. UTC 
From: Nícolas F. R. A. Prado <nfraprado@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@collabora.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Paul Elder <paul.elder@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(-)

Comments

Jacopo Mondi Dec. 16, 2022, 2:52 p.m. UTC | #1 
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@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@collabora.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Paul Elder <paul.elder@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
>
Umang Jain Dec. 20, 2022, 1:24 p.m. UTC | #2 
Hi Jacopo,

On 12/16/22 8:22 PM, Jacopo Mondi via libcamera-devel wrote:
> 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@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@collabora.com>
>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>> Signed-off-by: Paul Elder <paul.elder@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 ?

I don't think so - I remember using only 1 parameter buffer while 
buffers > 1 for input and output nodes for standalone IMGU streaming

https://patchwork.libcamera.org/patch/16988/
>
>>   	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
>>
Umang Jain Dec. 21, 2022, 10:39 a.m. UTC | #3 
Hi Paul,

On 12/16/22 5:59 PM, Paul Elder via libcamera-devel wrote:
> From: Nícolas F. R. A. Prado <nfraprado@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@collabora.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Paul Elder <paul.elder@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;
>
Paul Elder Dec. 23, 2022, 10:28 p.m. UTC | #4 
Hi Umang,

On Wed, Dec 21, 2022 at 04:09:21PM +0530, Umang Jain wrote:
> Hi Paul,
> 
> On 12/16/22 5:59 PM, Paul Elder via libcamera-devel wrote:
> > From: Nícolas F. R. A. Prado <nfraprado@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.

Indeed I don't think there would be a situation where there would be
less internal buffers than MinimumRequests, but I think (also as is seen
throughout this series) it's possible that the number of internal
buffers could be greater than MinimumRequests.

I suppose it would be less duplication if the number of internal buffers
was defined based on MinimumRequests (eg. kIPU3BufferSlotCount =
MinimumRequests + 1). Well, not the property directly but the quantity
that is fed to the property.

> 
> > 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?

I don't think the IMGU and CIO2 internal buffers are defined off of
arbitrary constants; they're intentional constants with (documented)
rationales. But indeed I think there is benefit in defining them based
on MinimumRequests, such that they end up with the desired quantity.

> > thrashing dmabuf mappings, which would degrade performance.
> 
> Have you checked any performance metrics with increasing internal counts? 

I haven't yet. I don't have an IPU3 platform that I can test on anymore.
I'll test it on imx8mp at least.

> 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)


Thanks,

Paul

> > 
> > 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@collabora.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Paul Elder <paul.elder@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;
>

Patch
diff mbox series 

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;