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

[Paul Elder]

On Tue, Aug 24, 2021 at 04:56:25PM -0300, Nícolas F. R. A. Prado wrote:
> Currently the rkisp1 pipeline handler relies on bufferCount to decide on
> the number of parameter and statistics buffers to allocate internally
> 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/rkisp1/rkisp1.cpp      | 19 ++++++++++++-------
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  3 +--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  2 ++
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 853a0ef42ba3..03a8d1d26e30 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -188,6 +188,16 @@ private:
>  	std::queue<FrameBuffer *> availableStatBuffers_;
>  
>  	Camera *activeCamera_;
> +
> +	/*
> +	 * This many internal buffers (or rather parameter and statistics buffer
> +	 * pairs) ensures that the pipeline runs smoothly, without frame drops.
> +	 * This number considers:
> +	 * - three buffers queued to the driver, which is also the minimum
> +	 *   required to start streaming
> +	 * - one buffer being processed on the IPA
> +	 */
> +	static constexpr unsigned int kRkISP1InternalBufferCount = 4;
>  };
>  
>  RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> @@ -693,16 +703,11 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>  	unsigned int ipaBufferId = 1;
>  	int ret;
>  
> -	unsigned int maxCount = std::max({
> -		data->mainPathStream_.configuration().bufferCount,
> -		data->selfPathStream_.configuration().bufferCount,
> -	});
> -
> -	ret = param_->allocateBuffers(maxCount, &paramBuffers_);
> +	ret = param_->allocateBuffers(kRkISP1InternalBufferCount, &paramBuffers_);
>  	if (ret < 0)
>  		goto error;
>  
> -	ret = stat_->allocateBuffers(maxCount, &statBuffers_);
> +	ret = stat_->allocateBuffers(kRkISP1InternalBufferCount, &statBuffers_);
>  	if (ret < 0)
>  		goto error;
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 25f482eb8d8e..515f4be16d7e 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -171,8 +171,7 @@ int RkISP1Path::start()
>  	if (running_)
>  		return -EBUSY;
>  
> -	/* \todo Make buffer count user configurable. */
> -	ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
> +	ret = video_->importBuffers(kRkISP1BufferSlotCount);
>  	if (ret)
>  		return ret;
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index 91757600ccdc..267d8f988ace 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -69,6 +69,8 @@ private:
>  	std::unique_ptr<V4L2Subdevice> resizer_;
>  	std::unique_ptr<V4L2VideoDevice> video_;
>  	MediaLink *link_;
> +
> +	static constexpr unsigned int kRkISP1BufferSlotCount = 16;
>  };
>  
>  class RkISP1MainPath : public RkISP1Path
> -- 
> 2.33.0
>