[libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi: Pass controls to IPA on start

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Fri, Dec 04, 2020 at 03:31:21PM +0000, Naushir Patuck wrote:
> Forward any controls passed into the pipeline handler to the IPA.
> The IPA then sets up the Raspberry Pi controller with these settings
> appropriately, and passes back any V4L2 sensor controls that need
> to be applied.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.h           |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           | 54 ++++++++++++-------
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 13 ++++-
>  3 files changed, 47 insertions(+), 21 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 2b55dbfc..c91a14bd 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -21,6 +21,7 @@ enum ConfigParameters {
>  	IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
>  	IPA_CONFIG_SENSOR = (1 << 2),
>  	IPA_CONFIG_DROP_FRAMES = (1 << 3),
> +	IPA_CONFIG_STARTUP = (1 << 4)

Could you add a comma at the end of the line ?

>  };
>  
>  enum Operations {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index b8c0f955..ff95580e 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -78,8 +78,7 @@ public:
>  	}
>  
>  	int init(const IPASettings &settings) override;
> -	int start([[maybe_unused]] const IPAOperationData &data,
> -		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
> +	int start(const IPAOperationData &data, IPAOperationData *result) override;
>  	void stop() override {}
>  
>  	void configure(const CameraSensorInfo &sensorInfo,
> @@ -154,6 +153,36 @@ int IPARPi::init(const IPASettings &settings)
>  	return 0;
>  }
>  
> +int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)
> +{
> +	RPiController::Metadata metadata;
> +
> +	ASSERT(result);
> +	result->operation = 0;
> +	if (data.operation & RPi::IPA_CONFIG_STARTUP) {
> +		/* We have been given some controls to action before start. */
> +		queueRequest(data.controls[0]);

Not something to be addressed now, but do you foresee in the future a
need for algorithms to handle initial controls differently than the
controls passed through requests ? Requests are synchronized to frames,
and I could imagine algorithms potentially relying on that sequencing.
Reusing queueRequest() for initial controls would then interfere with
that mechanism.

> +	}
> +
> +	controller_.SwitchMode(mode_, &metadata);
> +
> +	/* SwitchMode may supply updated exposure/gain values to use. */
> +	AgcStatus agcStatus;
> +	agcStatus.shutter_time = 0.0;
> +	agcStatus.analogue_gain = 0.0;
> +
> +	/* SwitchMode may supply updated exposure/gain values to use. */
> +	metadata.Get("agc.status", agcStatus);
> +	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> +		ControlList ctrls(unicamCtrls_);
> +		applyAGC(&agcStatus, ctrls);
> +		result->controls.emplace_back(ctrls);
> +		result->operation |= RPi::IPA_CONFIG_SENSOR;
> +	}
> +
> +	return 0;
> +}
> +
>  void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
>  {
>  	mode_.bitdepth = sensorInfo.bitsPerPixel;
> @@ -229,7 +258,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		result->data.push_back(gainDelay);
>  		result->data.push_back(exposureDelay);
>  		result->data.push_back(sensorMetadata);
> -

Unintentional change ?

>  		result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
>  	}
>  
> @@ -285,11 +313,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	result->data.push_back(dropFrame);
>  	result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
>  
> -	/* These zero values mean not program anything (unless overwritten). */
> -	struct AgcStatus agcStatus;
> -	agcStatus.shutter_time = 0.0;
> -	agcStatus.analogue_gain = 0.0;
> -
>  	if (!controllerInit_) {
>  		/* Load the tuning file for this sensor. */
>  		controller_.Read(tuningFile_.c_str());
> @@ -297,20 +320,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		controllerInit_ = true;
>  
>  		/* Supply initial values for gain and exposure. */
> +		ControlList ctrls(unicamCtrls_);
> +		AgcStatus agcStatus;
>  		agcStatus.shutter_time = DefaultExposureTime;
>  		agcStatus.analogue_gain = DefaultAnalogueGain;
> -	}
> -
> -	RPiController::Metadata metadata;
> -	controller_.SwitchMode(mode_, &metadata);
> -
> -	/* SwitchMode may supply updated exposure/gain values to use. */
> -	metadata.Get("agc.status", agcStatus);
> -	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> -		ControlList ctrls(unicamCtrls_);
>  		applyAGC(&agcStatus, ctrls);
> -		result->controls.push_back(ctrls);
>  
> +		result->controls.emplace_back(ctrls);
>  		result->operation |= RPi::IPA_CONFIG_SENSOR;
>  	}

Should we skip this at configure() time now that we do it at start()
time ? Otherwise we'll set the controls twice.

>  
> @@ -824,7 +840,7 @@ void IPARPi::processStats(unsigned int bufferId)
>  
>  		IPAOperationData op;
>  		op.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;
> -		op.controls.push_back(ctrls);
> +		op.controls.emplace_back(ctrls);
>  		queueFrameAction.emit(0, op);
>  	}
>  }
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index bafd0b2d..bc7c56f8 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -753,8 +753,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>  
>  	/* Start the IPA. */
>  	IPAOperationData ipaData = {}, result = {};
> -	if (controls)
> +	if (controls) {
> +		ipaData.operation = RPi::IPA_CONFIG_STARTUP;
>  		ipaData.controls.emplace_back(*controls);
> +	}
>  	ret = data->ipa_->start(ipaData, &result);
>  	if (ret) {
>  		LOG(RPI, Error)
> @@ -763,6 +765,14 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>  		return ret;
>  	}
>  
> +	/* Apply any gain/exposure settings that the IPA may have passed back. */
> +	ASSERT(data->staggeredCtrl_);
> +	if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> +		const ControlList &ctrls = result.controls[0];
> +		if (!data->staggeredCtrl_.set(ctrls))
> +			LOG(RPI, Error) << "V4L2 staggered set failed";
> +	}
> +
>  	/*
>  	 * IPA configure may have changed the sensor flips - hence the bayer
>  	 * order. Get the sensor format and set the ISP input now.
> @@ -783,7 +793,6 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>  	 * starting. First check that the staggered ctrl has been initialised
>  	 * by configure().
>  	 */
> -	ASSERT(data->staggeredCtrl_);
>  	data->staggeredCtrl_.reset();
>  	data->staggeredCtrl_.write();
>  	data->expectedSequence_ = 0;

-- 
Regards,

Laurent Pinchart