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

[Jacopo Mondi]

Hi Paul

On Fri, Dec 16, 2022 at 09:29:28PM +0900, Paul Elder via libcamera-devel wrote:
> From: Nícolas F. R. A. Prado <nfraprado at collabora.com>
>
> 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>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v9:
> - rebased
>
> 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 8fe37c4f..5028ef1a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -199,6 +199,16 @@ private:
>  	Camera *activeCamera_;
>
>  	const MediaPad *ispSink_;
> +
> +	/*
> +	 * 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;

I indeed see
drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c:#define RKISP1_MIN_BUFFERS_NEEDED 3

This seems quite right
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j


>  };
>
>  RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> @@ -835,17 +845,12 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>  	unsigned int ipaBufferId = 1;
>  	int ret;
>
> -	unsigned int maxCount = std::max({
> -		data->mainPathStream_.configuration().bufferCount,
> -		data->selfPathStream_.configuration().bufferCount,
> -	});
> -
>  	if (!isRaw_) {
> -		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 5079b268..a168e0ad 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -370,8 +370,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 bdf3f95b..5b53783c 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -76,6 +76,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.35.1
>