[2/3] ipa: rkisp1: Refer to integration time rather than shutter speed
diff mbox series

Message ID 20240418124632.652163-3-dan.scally@ideasonboard.com
State New
Headers show
Series
  • Minor AGC fixes
Related show

Commit Message

Dan Scally April 18, 2024, 12:46 p.m. UTC 
Multiple variables referencing shutter speed in the RkISP1 IPA module
are in fact calculated and used as integration time. The discrepancy
is problematic given the minimum shutter speed would produce the max
integration time.

Replace references to shutter speed with integration time.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 4 ++--
 src/ipa/rkisp1/ipa_context.cpp    | 8 ++++----
 src/ipa/rkisp1/ipa_context.h      | 4 ++--
 src/ipa/rkisp1/rkisp1.cpp         | 8 ++++----
 4 files changed, 12 insertions(+), 12 deletions(-)

Comments

Paul Elder April 19, 2024, 3 a.m. UTC | #1 
On Thu, Apr 18, 2024 at 01:46:31PM +0100, Daniel Scally wrote:
> Multiple variables referencing shutter speed in the RkISP1 IPA module
> are in fact calculated and used as integration time. The discrepancy
> is problematic given the minimum shutter speed would produce the max
> integration time.
> 
> Replace references to shutter speed with integration time.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 4 ++--
>  src/ipa/rkisp1/ipa_context.cpp    | 8 ++++----
>  src/ipa/rkisp1/ipa_context.h      | 4 ++--
>  src/ipa/rkisp1/rkisp1.cpp         | 8 ++++----
>  4 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 27b6f2c1..13f54398 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -95,8 +95,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
>  
>  	/* \todo Run this again when FrameDurationLimits is passed in */
> -	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
> -				     context.configuration.sensor.maxShutterSpeed,
> +	configureExposureModeHelpers(context.configuration.sensor.minIntegrationTime,
> +				     context.configuration.sensor.maxIntegrationTime,
>  				     context.configuration.sensor.minAnalogueGain,
>  				     context.configuration.sensor.maxAnalogueGain);
>  
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 070834fa..991ca1c2 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -78,11 +78,11 @@ 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.minIntegrationTime
> + * \brief Minimum integration time supported with the sensor
>   *
> - * \var IPASessionConfiguration::sensor.maxShutterSpeed
> - * \brief Maximum shutter speed supported with the sensor
> + * \var IPASessionConfiguration::sensor.maxIntegrationTime
> + * \brief Maximum integration time supported with the sensor
>   *
>   * \var IPASessionConfiguration::sensor.minAnalogueGain
>   * \brief Minimum analogue gain supported with the sensor
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 256b75eb..3405a260 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -43,8 +43,8 @@ struct IPASessionConfiguration {
>  	} lsc;
>  
>  	struct {
> -		utils::Duration minShutterSpeed;
> -		utils::Duration maxShutterSpeed;
> +		utils::Duration minIntegrationTime;
> +		utils::Duration maxIntegrationTime;
>  		double minAnalogueGain;
>  		double maxAnalogueGain;
>  
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index d8610095..15919d3f 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -245,14 +245,14 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
>  
>  	/*
>  	 * When the AGC computes the new exposure values for a frame, it needs
> -	 * to know the limits for shutter speed and analogue gain.
> +	 * to know the limits for integration time and analogue gain.
>  	 * As it depends on the sensor, update it with the controls.
>  	 *
> -	 * \todo take VBLANK into account for maximum shutter speed
> +	 * \todo take VBLANK into account for maximum integration time
>  	 */
> -	context_.configuration.sensor.minShutterSpeed =
> +	context_.configuration.sensor.minIntegrationTime =
>  		minExposure * context_.configuration.sensor.lineDuration;
> -	context_.configuration.sensor.maxShutterSpeed =
> +	context_.configuration.sensor.maxIntegrationTime =
>  		maxExposure * context_.configuration.sensor.lineDuration;
>  	context_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);
>  	context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);
> -- 
> 2.34.1
>
Laurent Pinchart May 8, 2024, 1:34 p.m. UTC | #2 
Hi Dan,

Thank you for the patch.

On Thu, Apr 18, 2024 at 01:46:31PM +0100, Daniel Scally wrote:
> Multiple variables referencing shutter speed in the RkISP1 IPA module
> are in fact calculated and used as integration time. The discrepancy
> is problematic given the minimum shutter speed would produce the max
> integration time.
> 
> Replace references to shutter speed with integration time.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

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

> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 4 ++--
>  src/ipa/rkisp1/ipa_context.cpp    | 8 ++++----
>  src/ipa/rkisp1/ipa_context.h      | 4 ++--
>  src/ipa/rkisp1/rkisp1.cpp         | 8 ++++----
>  4 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 27b6f2c1..13f54398 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -95,8 +95,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
>  
>  	/* \todo Run this again when FrameDurationLimits is passed in */
> -	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
> -				     context.configuration.sensor.maxShutterSpeed,
> +	configureExposureModeHelpers(context.configuration.sensor.minIntegrationTime,
> +				     context.configuration.sensor.maxIntegrationTime,
>  				     context.configuration.sensor.minAnalogueGain,
>  				     context.configuration.sensor.maxAnalogueGain);
>  
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 070834fa..991ca1c2 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -78,11 +78,11 @@ 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.minIntegrationTime
> + * \brief Minimum integration time supported with the sensor
>   *
> - * \var IPASessionConfiguration::sensor.maxShutterSpeed
> - * \brief Maximum shutter speed supported with the sensor
> + * \var IPASessionConfiguration::sensor.maxIntegrationTime
> + * \brief Maximum integration time supported with the sensor
>   *
>   * \var IPASessionConfiguration::sensor.minAnalogueGain
>   * \brief Minimum analogue gain supported with the sensor
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 256b75eb..3405a260 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -43,8 +43,8 @@ struct IPASessionConfiguration {
>  	} lsc;
>  
>  	struct {
> -		utils::Duration minShutterSpeed;
> -		utils::Duration maxShutterSpeed;
> +		utils::Duration minIntegrationTime;
> +		utils::Duration maxIntegrationTime;
>  		double minAnalogueGain;
>  		double maxAnalogueGain;
>  
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index d8610095..15919d3f 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -245,14 +245,14 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
>  
>  	/*
>  	 * When the AGC computes the new exposure values for a frame, it needs
> -	 * to know the limits for shutter speed and analogue gain.
> +	 * to know the limits for integration time and analogue gain.
>  	 * As it depends on the sensor, update it with the controls.
>  	 *
> -	 * \todo take VBLANK into account for maximum shutter speed
> +	 * \todo take VBLANK into account for maximum integration time
>  	 */
> -	context_.configuration.sensor.minShutterSpeed =
> +	context_.configuration.sensor.minIntegrationTime =
>  		minExposure * context_.configuration.sensor.lineDuration;
> -	context_.configuration.sensor.maxShutterSpeed =
> +	context_.configuration.sensor.maxIntegrationTime =
>  		maxExposure * context_.configuration.sensor.lineDuration;
>  	context_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);
>  	context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 27b6f2c1..13f54398 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -95,8 +95,8 @@  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
 	context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
 
 	/* \todo Run this again when FrameDurationLimits is passed in */
-	configureExposureModeHelpers(context.configuration.sensor.minShutterSpeed,
-				     context.configuration.sensor.maxShutterSpeed,
+	configureExposureModeHelpers(context.configuration.sensor.minIntegrationTime,
+				     context.configuration.sensor.maxIntegrationTime,
 				     context.configuration.sensor.minAnalogueGain,
 				     context.configuration.sensor.maxAnalogueGain);
 
diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
index 070834fa..991ca1c2 100644
--- a/src/ipa/rkisp1/ipa_context.cpp
+++ b/src/ipa/rkisp1/ipa_context.cpp
@@ -78,11 +78,11 @@  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.minIntegrationTime
+ * \brief Minimum integration time supported with the sensor
  *
- * \var IPASessionConfiguration::sensor.maxShutterSpeed
- * \brief Maximum shutter speed supported with the sensor
+ * \var IPASessionConfiguration::sensor.maxIntegrationTime
+ * \brief Maximum integration time supported with the sensor
  *
  * \var IPASessionConfiguration::sensor.minAnalogueGain
  * \brief Minimum analogue gain supported with the sensor
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 256b75eb..3405a260 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -43,8 +43,8 @@  struct IPASessionConfiguration {
 	} lsc;
 
 	struct {
-		utils::Duration minShutterSpeed;
-		utils::Duration maxShutterSpeed;
+		utils::Duration minIntegrationTime;
+		utils::Duration maxIntegrationTime;
 		double minAnalogueGain;
 		double maxAnalogueGain;
 
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index d8610095..15919d3f 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -245,14 +245,14 @@  int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
 
 	/*
 	 * When the AGC computes the new exposure values for a frame, it needs
-	 * to know the limits for shutter speed and analogue gain.
+	 * to know the limits for integration time and analogue gain.
 	 * As it depends on the sensor, update it with the controls.
 	 *
-	 * \todo take VBLANK into account for maximum shutter speed
+	 * \todo take VBLANK into account for maximum integration time
 	 */
-	context_.configuration.sensor.minShutterSpeed =
+	context_.configuration.sensor.minIntegrationTime =
 		minExposure * context_.configuration.sensor.lineDuration;
-	context_.configuration.sensor.maxShutterSpeed =
+	context_.configuration.sensor.maxIntegrationTime =
 		maxExposure * context_.configuration.sensor.lineDuration;
 	context_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);
 	context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);