[libcamera-devel,6/7] ipa: raspberrypi: Rationalise parameters to ipa::start()

11605 diff mbox series

Separate out the in and out parameters in ipa::start() as they are not
the same. This function now takes in a ControlList and returns out a
struct StartConfig which holds a ControlList and drop frame count for
the pipeline handler to action.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.mojom          |  4 ++--
 src/ipa/raspberrypi/raspberrypi.cpp              | 16 +++++++---------
 .../pipeline/raspberrypi/raspberrypi.cpp         | 16 +++++++---------
 3 files changed, 16 insertions(+), 20 deletions(-)

Hi Naush,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

On Wed, Mar 17, 2021 at 10:02:10AM +0000, Naushir Patuck wrote:
> Separate out the in and out parameters in ipa::start() as they are not
> the same. This function now takes in a ControlList and returns out a
> struct StartConfig which holds a ControlList and drop frame count for
> the pipeline handler to action.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom          |  4 ++--
>  src/ipa/raspberrypi/raspberrypi.cpp              | 16 +++++++---------
>  .../pipeline/raspberrypi/raspberrypi.cpp         | 16 +++++++---------
>  3 files changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index a713c88eee19..7549397a27c4 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -40,14 +40,14 @@ struct ConfigOutput {
>  	ControlList controls;
>  };
>  
> -struct StartControls {
> +struct StartConfig {
>  	ControlList controls;
>  	int32 dropFrameCount;
>  };
>  
>  interface IPARPiInterface {
>  	init(IPASettings settings) => (int32 ret, SensorConfig sensorConfig);
> -	start(StartControls controls) => (StartControls result);
> +	start(ControlList controls) => (StartConfig startConfig);
>  	stop();
>  
>  	/**
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 2ca64bcdb80a..417a7922c3bd 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -79,8 +79,7 @@ public:
>  	}
>  
>  	int init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) override;
> -	void start(const ipa::RPi::StartControls &data,
> -		   ipa::RPi::StartControls *result) override;
> +	void start(const ControlList &controls, ipa::RPi::StartConfig *startConfig) override;
>  	void stop() override {}
>  
>  	int configure(const CameraSensorInfo &sensorInfo,
> @@ -192,15 +191,14 @@ int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConf
>  	return 0;
>  }
>  
> -void IPARPi::start(const ipa::RPi::StartControls &data,
> -		   ipa::RPi::StartControls *result)
> +void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConfig)
>  {
>  	RPiController::Metadata metadata;
>  
> -	ASSERT(result);
> -	if (!data.controls.empty()) {
> +	ASSERT(startConfig);
> +	if (!controls.empty()) {
>  		/* We have been given some controls to action before start. */
> -		queueRequest(data.controls);
> +		queueRequest(controls);
>  	}
>  
>  	controller_.SwitchMode(mode_, &metadata);
> @@ -215,7 +213,7 @@ void IPARPi::start(const ipa::RPi::StartControls &data,
>  	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
>  		ControlList ctrls(sensorCtrls_);
>  		applyAGC(&agcStatus, ctrls);
> -		result->controls = std::move(ctrls);
> +		startConfig->controls = std::move(ctrls);
>  	}
>  
>  	/*
> @@ -262,7 +260,7 @@ void IPARPi::start(const ipa::RPi::StartControls &data,
>  		mistrustCount_ = helper_->MistrustFramesModeSwitch();
>  	}
>  
> -	result->dropFrameCount = dropFrame;
> +	startConfig->dropFrameCount = dropFrame;
>  
>  	firstStart_ = false;
>  }
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index cd8a10a5747f..16568d56f31c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -815,20 +815,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  		data->applyScalerCrop(*controls);
>  
>  	/* Start the IPA. */
> -	ipa::RPi::StartControls ipaData;
> -	ipa::RPi::StartControls result;
> +	ControlList startControls;
> +	ipa::RPi::StartConfig startConfig;
>  	if (controls)
> -		ipaData.controls = *controls;
> -	data->ipa_->start(ipaData, &result);
> +		startControls = *controls;
> +	data->ipa_->start(startControls, &startConfig);

I think you don't need to copy controls anymore:

	ipa::RPi::StartConfig startConfig;
	data->ipa_->start(controls ? *controls : ControlList{}, &startConfig);

Overall this reads much better, it's a nice cleanup.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	/* Apply any gain/exposure settings that the IPA may have passed back. */
> -	if (!result.controls.empty()) {
> -		ControlList &ctrls = result.controls;
> -		data->unicam_[Unicam::Image].dev()->setControls(&ctrls);
> -	}
> +	if (!startConfig.controls.empty())
> +		data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);
>  
>  	/* Configure the number of dropped frames required on startup. */
> -	data->dropFrameCount_ = result.dropFrameCount;
> +	data->dropFrameCount_ = startConfig.dropFrameCount;
>  
>  	/* We need to set the dropFrameCount_ before queueing buffers. */
>  	ret = queueAllBuffers(camera);