[libcamera-devel] [RFC PATCH 2/3] libcamera: pipeline: rkisp1: Allow bufferCount to be user-configurable

[Jacopo Mondi]

Hi Nicolas

On Fri, Apr 09, 2021 at 06:38:14PM -0300, Nícolas F. R. A. Prado wrote:
> Allow the StreamConfiguration's bufferCount to be changed by the user.
> This allows the user to, after increasing bufferCount, allocate more
> buffers through FrameBufferAllocator.

nit: I would avoid the ", allocate more" as this is a consequence of
bufferCount_ being increased, not something specifically about this
patch.

>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  4 ++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 12 +++++++-----
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  4 ++--
>  3 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 549f4a4e61a8..c7b93b2804c7 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -792,7 +792,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>  	}
>
>  	if (data->mainPath_->isEnabled()) {
> -		ret = mainPath_.start();
> +		ret = mainPath_.start(data->mainPathStream_.configuration());
>  		if (ret) {
>  			param_->streamOff();
>  			stat_->streamOff();
> @@ -803,7 +803,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>  	}
>
>  	if (data->selfPath_->isEnabled()) {
> -		ret = selfPath_.start();
> +		ret = selfPath_.start(data->selfPathStream_.configuration());

The patch is ok, however I would consider how all of this would look
like if we store in each 'path' (what an horrible name we chose!) the
StreamConfiguration passed in at RkISP1Path::configure() time and
re-use it here, instead of having to pass it in again at start() time.

>  		if (ret) {
>  			mainPath_.stop();
>  			param_->streamOff();
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 25f482eb8d8e..a03b5c23b87e 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -61,7 +61,7 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)
>  	StreamConfiguration cfg(formats);
>  	cfg.pixelFormat = formats::NV12;
>  	cfg.size = maxResolution;
> -	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> +	cfg.bufferCount = RKISP1_MIN_BUFFER_COUNT;
>
>  	return cfg;
>  }
> @@ -77,7 +77,10 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)
>
>  	cfg->size.boundTo(maxResolution_);
>  	cfg->size.expandTo(minResolution_);
> -	cfg->bufferCount = RKISP1_BUFFER_COUNT;
> +	if (cfg->bufferCount < RKISP1_MIN_BUFFER_COUNT) {
> +		cfg->bufferCount = RKISP1_MIN_BUFFER_COUNT;
> +		status = CameraConfiguration::Adjusted;
> +	}
>
>  	V4L2DeviceFormat format;
>  	format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);
> @@ -164,15 +167,14 @@ int RkISP1Path::configure(const StreamConfiguration &config,
>  	return 0;
>  }
>
> -int RkISP1Path::start()
> +int RkISP1Path::start(const StreamConfiguration &config)
>  {
>  	int ret;
>
>  	if (running_)
>  		return -EBUSY;
>
> -	/* \todo Make buffer count user configurable. */
> -	ret = video_->importBuffers(RKISP1_BUFFER_COUNT);
> +	ret = video_->importBuffers(config.bufferCount);
>  	if (ret)
>  		return ret;
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index 3b3e37d258d0..13291da89dc5 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -49,14 +49,14 @@ public:
>  		return video_->exportBuffers(bufferCount, buffers);
>  	}
>
> -	int start();
> +	int start(const StreamConfiguration &config);
>  	void stop();
>
>  	int queueBuffer(FrameBuffer *buffer) { return video_->queueBuffer(buffer); }
>  	Signal<FrameBuffer *> &bufferReady() { return video_->bufferReady; }
>
>  private:
> -	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> +	static constexpr unsigned int RKISP1_MIN_BUFFER_COUNT = 4;
>
>  	const char *name_;
>  	bool running_;
> --
> 2.31.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel