[v2,5/5] libcamera: pipeline: rkisp1: Don't rely on bufferCount

23748 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.
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.

Stop relying on bufferCount for these numbers and instead set them to
kPipelineDepth, as this already limits the number of buffers queued
to the driver.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Sven Püschel <s.pueschel@pengutronix.de>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Quoting Stefan Klug (2025-07-07 08:53:39)
> 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.
> 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.
> 
> Stop relying on bufferCount for these numbers and instead set them to
> kPipelineDepth, as this already limits the number of buffers queued
> to the driver.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Sven Püschel <s.pueschel@pengutronix.de>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index bdaeb935ee06..8591e4868e6f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -157,6 +157,10 @@ private:
>  
>  namespace {
>  
> +/*
> + * This many internal buffers (or rather parameter and statistics buffer
> + * pairs) ensures that the pipeline runs smoothly, without frame drops.
> + */
>  const unsigned int kPipelineDepth = 4;
>  
>  } // namespace
> @@ -990,24 +994,19 @@ 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(kPipelineDepth, &paramBuffers_);
>                 if (ret < 0)
>                         goto error;
>  
> -               ret = stat_->allocateBuffers(maxCount, &statBuffers_);
> +               ret = stat_->allocateBuffers(kPipelineDepth, &statBuffers_);
>                 if (ret < 0)
>                         goto error;
>         }
>  
>         /* If the dewarper is being used, allocate internal buffers for ISP. */
>         if (useDewarper_) {
> -               ret = mainPath_.exportBuffers(maxCount, &mainPathBuffers_);
> +               ret = mainPath_.exportBuffers(kPipelineDepth, &mainPathBuffers_);
>                 if (ret < 0)
>                         goto error;
>  
> -- 
> 2.48.1
>

On Mon, Jul 07, 2025 at 09:53:39AM +0200, Stefan Klug 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.
> 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.
> 

The commit message should be adapted to reflect the usage of
maxQueuedRequestsDevice_.

```
The RkISP1 pipeline handler currently relies on CameraConfiguration::bufferCount
to allocate internal buffers for parameters and statistics buffers.

However, only maxQueuedRequestsDevice_ requests can be queued to the
underlying hardware to saturate the pipeline, hence allocate buffers
pairs upto maxQueuedRequestsDevice_, for parameters and statistics.
```

> Stop relying on bufferCount for these numbers and instead set them to
> kPipelineDepth, as this already limits the number of buffers queued
> to the driver.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Sven Püschel <s.pueschel@pengutronix.de>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index bdaeb935ee06..8591e4868e6f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -157,6 +157,10 @@ private:
>  
>  namespace {
>  
> +/*
> + * This many internal buffers (or rather parameter and statistics buffer
> + * pairs) ensures that the pipeline runs smoothly, without frame drops.
> + */

nit: With the kPipelineDepth naming changed, this comment would rather be
obvious to understand, hence can be considered to drop.

Reviewed-by: Umang Jain <uajain@igalia.com>

>  const unsigned int kPipelineDepth = 4;
>  
>  } // namespace
> @@ -990,24 +994,19 @@ 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(kPipelineDepth, &paramBuffers_);
>  		if (ret < 0)
>  			goto error;
>  
> -		ret = stat_->allocateBuffers(maxCount, &statBuffers_);
> +		ret = stat_->allocateBuffers(kPipelineDepth, &statBuffers_);
>  		if (ret < 0)
>  			goto error;
>  	}
>  
>  	/* If the dewarper is being used, allocate internal buffers for ISP. */
>  	if (useDewarper_) {
> -		ret = mainPath_.exportBuffers(maxCount, &mainPathBuffers_);
> +		ret = mainPath_.exportBuffers(kPipelineDepth, &mainPathBuffers_);
>  		if (ret < 0)
>  			goto error;
>  
> -- 
> 2.48.1
>