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

18021 diff mbox series

From: Nícolas F. R. A. Prado <nfraprado@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@collabora.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Paul Elder <paul.elder@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(-)

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