[RFC PATCH 4/4] pipeline: rkisp1: Prperly handle the bufferCount set in the stream configuration

[Sven Püschel]

Hi Stefan,

just noticed a typo in the commit title Prperly -> Properly

On 5/26/25 23:42, Stefan Klug wrote:
> The bufferCount is reset to a hardcoded value of 4 in RkISP1Path::validate().
> Keep the default value of 4 but do not reset it, if it was changed.
>
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 5 +++--
>   src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 ++----
>   src/libcamera/pipeline/rkisp1/rkisp1_path.h   | 4 +---
>   3 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index af9117c83630..ea94bccd252d 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -791,6 +791,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>   			return nullptr;
>   
>   		cfg.colorSpace = colorSpace;
> +		cfg.bufferCount = kPipelineDepth;
>   		config->addConfiguration(cfg);
>   	}
>   
> @@ -1124,14 +1125,14 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>   	}
>   
>   	if (data->mainPath_->isEnabled()) {
> -		ret = mainPath_.start();
> +		ret = mainPath_.start(maxQueuedRequestsDevice());
>   		if (ret)
>   			return ret;
>   		actions += [&]() { mainPath_.stop(); };
>   	}
>   
>   	if (hasSelfPath_ && data->selfPath_->isEnabled()) {
> -		ret = selfPath_.start();
> +		ret = selfPath_.start(maxQueuedRequestsDevice());
>   		if (ret)
>   			return ret;
>   	}
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 64018dc5b2f4..2f482fcc1834 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -249,7 +249,6 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
>   	StreamConfiguration cfg(formats);
>   	cfg.pixelFormat = format;
>   	cfg.size = streamSize;
> -	cfg.bufferCount = RKISP1_BUFFER_COUNT;
>   
>   	return cfg;
>   }
> @@ -383,7 +382,6 @@ RkISP1Path::validate(const CameraSensor *sensor,
>   
>   	cfg->size.boundTo(maxResolution);
>   	cfg->size.expandTo(minResolution);
> -	cfg->bufferCount = RKISP1_BUFFER_COUNT;
>   
>   	V4L2DeviceFormat format;
>   	format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);
> @@ -480,7 +478,7 @@ int RkISP1Path::configure(const StreamConfiguration &config,
>   	return 0;
>   }
>   
> -int RkISP1Path::start()
> +int RkISP1Path::start(unsigned int bufferCount)
>   {
>   	int ret;
>   
> @@ -488,7 +486,7 @@ int RkISP1Path::start()
>   		return -EBUSY;
>   
>   	/* \todo Make buffer count user configurable. */
> -	ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
> +	ret = video_->importBuffers(bufferCount);

The todo can also be removed with this change.


Sincerely

     Sven

>   	if (ret)
>   		return ret;
>   
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index 430181d371a7..0b60c499ac64 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -58,7 +58,7 @@ public:
>   		return video_->exportBuffers(bufferCount, buffers);
>   	}
>   
> -	int start();
> +	int start(unsigned int bufferCount);
>   	void stop();
>   
>   	int queueBuffer(FrameBuffer *buffer) { return video_->queueBuffer(buffer); }
> @@ -69,8 +69,6 @@ private:
>   	void populateFormats();
>   	Size filterSensorResolution(const CameraSensor *sensor);
>   
> -	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> -
>   	const char *name_;
>   	bool running_;
>