[libcamera-devel] [PATCH v8 05/17] libcamera: pipeline: simple: Don't rely on bufferCount

[Paul Elder]

On Tue, Aug 24, 2021 at 04:56:24PM -0300, Nícolas F. R. A. Prado wrote:
> Currently the simple pipeline handler relies on bufferCount to decide on
> the number of buffers to allocate internally when a converter is in use
> 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 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/simple/converter.cpp | 12 ++++++-----
>  src/libcamera/pipeline/simple/converter.h   |  6 ++++--
>  src/libcamera/pipeline/simple/simple.cpp    | 22 ++++++++++++++++-----
>  3 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index b5e34c4cd0c5..3133b3dbda07 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -101,13 +101,14 @@ int SimpleConverter::Stream::exportBuffers(unsigned int count,
>  	return m2m_->capture()->exportBuffers(count, buffers);
>  }
>  
> -int SimpleConverter::Stream::start()
> +int SimpleConverter::Stream::start(unsigned int internalBufferCount,
> +				   unsigned int bufferSlotCount)
>  {
> -	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> +	int ret = m2m_->output()->importBuffers(internalBufferCount);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> +	ret = m2m_->capture()->importBuffers(bufferSlotCount);
>  	if (ret < 0) {
>  		stop();
>  		return ret;
> @@ -331,12 +332,13 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,
>  	return streams_[output].exportBuffers(count, buffers);
>  }
>  
> -int SimpleConverter::start()
> +int SimpleConverter::start(unsigned int internalBufferCount,
> +			   unsigned int bufferSlotCount)
>  {
>  	int ret;
>  
>  	for (Stream &stream : streams_) {
> -		ret = stream.start();
> +		ret = stream.start(internalBufferCount, bufferSlotCount);
>  		if (ret < 0) {
>  			stop();
>  			return ret;
> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> index 276a2a291c21..deb3df0d08df 100644
> --- a/src/libcamera/pipeline/simple/converter.h
> +++ b/src/libcamera/pipeline/simple/converter.h
> @@ -47,7 +47,8 @@ public:
>  	int exportBuffers(unsigned int ouput, unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>  
> -	int start();
> +	int start(unsigned int internalBufferCount,
> +		  unsigned int bufferSlotCount);
>  	void stop();
>  
>  	int queueBuffers(FrameBuffer *input,
> @@ -69,7 +70,8 @@ private:
>  		int exportBuffers(unsigned int count,
>  				  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>  
> -		int start();
> +		int start(unsigned int internalBufferCount,
> +			  unsigned int bufferSlotCount);
>  		void stop();
>  
>  		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 737815452bae..d0a658a23be8 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -253,6 +253,7 @@ protected:
>  
>  private:
>  	static constexpr unsigned int kNumInternalBuffers = 3;
> +	static constexpr unsigned int kSimpleBufferSlotCount = 16;
>  
>  	SimpleCameraData *cameraData(Camera *camera)
>  	{
> @@ -830,17 +831,27 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>  	V4L2VideoDevice *video = data->video_;
>  	int ret;
>  
> +	/*
> +	 * Number of internal buffers that will be used to move video capture
> +	 * device frames to the converter.
> +	 *
> +	 * \todo Make this depend on the driver in use instead of being
> +	 * hardcoded. In order to not drop frames, the realtime requirements for
> +	 * each device should be checked, so it may not be as simple as just
> +	 * using the minimum number of buffers required by the driver.
> +	 */
> +	static constexpr unsigned int internalBufferCount = 3;
> +
>  	if (data->useConverter_) {
>  		/*
>  		 * When using the converter allocate a fixed number of internal
>  		 * buffers.
>  		 */
> -		ret = video->allocateBuffers(kNumInternalBuffers,
> +		ret = video->allocateBuffers(internalBufferCount,
>  					     &data->converterBuffers_);
>  	} else {
> -		/* Otherwise, prepare for using buffers from the only stream. */
> -		Stream *stream = &data->streams_[0];
> -		ret = video->importBuffers(stream->configuration().bufferCount);
> +		/* Otherwise, prepare for using buffers. */
> +		ret = video->importBuffers(kSimpleBufferSlotCount);
>  	}
>  	if (ret < 0)
>  		return ret;
> @@ -852,7 +863,8 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>  	}
>  
>  	if (data->useConverter_) {
> -		ret = converter_->start();
> +		ret = converter_->start(internalBufferCount,
> +					kSimpleBufferSlotCount);
>  		if (ret < 0) {
>  			stop(camera);
>  			return ret;
> -- 
> 2.33.0
>