[libcamera-devel] [PATCH v3 02/13] ipa: rkisp1: Move shutter speed and analogue gain limits from agc to sensor

[Jacopo Mondi]

Hi Laurent

On Mon, Oct 24, 2022 at 03:03:45AM +0300, Laurent Pinchart via libcamera-devel wrote:
> The limits for the shutter speed and analogue gain are stored in
> IPASessionConfiguration::agc. While they're related to the AGC, they are
> properties of the sensor, and are stored in the session configuration by
> the IPA module, not the AGC algorithm. Move them to the
> IPASessionConfiguration::sensor structure where they belong.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Indeed makes sense!

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks

> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 11 ++++++-----
>  src/ipa/rkisp1/ipa_context.cpp    | 28 +++++++++++++++-------------
>  src/ipa/rkisp1/ipa_context.h      | 25 +++++++++++++------------
>  src/ipa/rkisp1/rkisp1.cpp         | 10 ++++++----
>  4 files changed, 40 insertions(+), 34 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 3fcbfa608467..169afe372803 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -74,7 +74,8 @@ Agc::Agc()
>  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  {
>  	/* Configure the default exposure and gain. */
> -	context.activeState.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> +	context.activeState.agc.gain = std::max(context.configuration.sensor.minAnalogueGain,
> +						kMinAnalogueGain);
>  	context.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;
>
>  	/*
> @@ -202,13 +203,13 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>  	/* Use the highest of the two gain estimates. */
>  	double evGain = std::max(yGain, iqMeanGain);
>
> -	utils::Duration minShutterSpeed = configuration.agc.minShutterSpeed;
> -	utils::Duration maxShutterSpeed = std::min(configuration.agc.maxShutterSpeed,
> +	utils::Duration minShutterSpeed = configuration.sensor.minShutterSpeed;
> +	utils::Duration maxShutterSpeed = std::min(configuration.sensor.maxShutterSpeed,
>  						   kMaxShutterSpeed);
>
> -	double minAnalogueGain = std::max(configuration.agc.minAnalogueGain,
> +	double minAnalogueGain = std::max(configuration.sensor.minAnalogueGain,
>  					  kMinAnalogueGain);
> -	double maxAnalogueGain = std::min(configuration.agc.maxAnalogueGain,
> +	double maxAnalogueGain = std::min(configuration.sensor.maxAnalogueGain,
>  					  kMaxAnalogueGain);
>
>  	/* Consider within 1% of the target as correctly exposed. */
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 3c14cf3476ce..7a987497bd03 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -28,21 +28,11 @@ namespace libcamera::ipa::rkisp1 {
>   * \var IPASessionConfiguration::agc
>   * \brief AGC parameters configuration of the IPA
>   *
> - * \var IPASessionConfiguration::agc.minShutterSpeed
> - * \brief Minimum shutter speed supported with the configured sensor
> - *
> - * \var IPASessionConfiguration::agc.maxShutterSpeed
> - * \brief Maximum shutter speed supported with the configured sensor
> - *
> - * \var IPASessionConfiguration::agc.minAnalogueGain
> - * \brief Minimum analogue gain supported with the configured sensor
> - *
> - * \var IPASessionConfiguration::agc.maxAnalogueGain
> - * \brief Maximum analogue gain supported with the configured sensor
> - *
>   * \var IPASessionConfiguration::agc.measureWindow
>   * \brief AGC measure window
> - *
> + */
> +
> +/**
>   * \var IPASessionConfiguration::hw
>   * \brief RkISP1-specific hardware information
>   *
> @@ -77,6 +67,18 @@ namespace libcamera::ipa::rkisp1 {
>   * \var IPASessionConfiguration::sensor
>   * \brief Sensor-specific configuration of the IPA
>   *
> + * \var IPASessionConfiguration::sensor.minShutterSpeed
> + * \brief Minimum shutter speed supported with the sensor
> + *
> + * \var IPASessionConfiguration::sensor.maxShutterSpeed
> + * \brief Maximum shutter speed supported with the sensor
> + *
> + * \var IPASessionConfiguration::sensor.minAnalogueGain
> + * \brief Minimum analogue gain supported with the sensor
> + *
> + * \var IPASessionConfiguration::sensor.maxAnalogueGain
> + * \brief Maximum analogue gain supported with the sensor
> + *
>   * \var IPASessionConfiguration::sensor.defVBlank
>   * \brief The default vblank value of the sensor
>   *
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 60e8a7e11196..bb60ab9eab72 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -21,24 +21,25 @@ namespace libcamera {
>  namespace ipa::rkisp1 {
>
>  struct IPASessionConfiguration {
> +	struct {
> +		struct rkisp1_cif_isp_window measureWindow;
> +	} agc;
> +
> +	struct {
> +		struct rkisp1_cif_isp_window measureWindow;
> +		bool enabled;
> +	} awb;
> +
> +	struct {
> +		bool enabled;
> +	} lsc;
> +
>  	struct {
>  		utils::Duration minShutterSpeed;
>  		utils::Duration maxShutterSpeed;
>  		double minAnalogueGain;
>  		double maxAnalogueGain;
> -		struct rkisp1_cif_isp_window measureWindow;
> -	} agc;
>
> -	struct {
> -		struct rkisp1_cif_isp_window measureWindow;
> -		bool enabled;
> -	} awb;
> -
> -	struct {
> -		bool enabled;
> -	} lsc;
> -
> -	struct {
>  		int32_t defVBlank;
>  		utils::Duration lineDuration;
>  		Size size;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 9061a189cbce..fcb9dacccc3c 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -257,10 +257,12 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>  	 *
>  	 * \todo take VBLANK into account for maximum shutter speed
>  	 */
> -	context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;
> -	context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
> -	context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
> -	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
> +	context_.configuration.sensor.minShutterSpeed =
> +		minExposure * context_.configuration.sensor.lineDuration;
> +	context_.configuration.sensor.maxShutterSpeed =
> +		maxExposure * context_.configuration.sensor.lineDuration;
> +	context_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);
> +	context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);
>
>  	for (auto const &algo : algorithms()) {
>  		int ret = algo->configure(context_, info);
> --
> Regards,
>
> Laurent Pinchart
>