[4/5] ipa: software_isp: AGC: Only use integers for exposure calculations
diff mbox series

Message ID 20250923190657.21453-5-hansg@kernel.org
State Superseded
Headers show
Series
  • ipa: software_isp: AGC: Fox AGC oscillation bug
Related show

Commit Message

Hans de Goede Sept. 23, 2025, 7:06 p.m. UTC 
Exposure is an integer, instead of re-using the "double next" used
for again calculations, doing intermediate calculations with double
precision, use a local next variable of an integer type.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 src/ipa/simple/algorithms/agc.cpp | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Milan Zamazal Sept. 24, 2025, 8:13 a.m. UTC | #1 
Hi Hans,

thank you for the patch.

Hans de Goede <hansg@kernel.org> writes:

> Exposure is an integer, instead of re-using the "double next" used
> for again calculations, doing intermediate calculations with double
> precision, use a local next variable of an integer type.

Using separate locals is cleaner anyway.

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  src/ipa/simple/algorithms/agc.cpp | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
> index 230616e62..cdde56ba2 100644
> --- a/src/ipa/simple/algorithms/agc.cpp
> +++ b/src/ipa/simple/algorithms/agc.cpp
> @@ -51,19 +51,18 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>  	static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
>  	static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
>  
> -	double next;
>  	int32_t &exposure = frameContext.sensor.exposure;
>  	double &again = frameContext.sensor.gain;
>  
>  	if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
>  		if (exposure < context.configuration.agc.exposureMax) {
> -			next = exposure * kExpNumeratorUp / kExpDenominator;
> +			int32_t next = exposure * kExpNumeratorUp / kExpDenominator;
>  			if (next - exposure < 1)
>  				exposure += 1;
>  			else
>  				exposure = next;
>  		} else {
> -			next = again * kExpNumeratorUp / kExpDenominator;
> +			double next = again * kExpNumeratorUp / kExpDenominator;
>  			if (next - again < context.configuration.agc.againMinStep)
>  				again += context.configuration.agc.againMinStep;
>  			else
> @@ -73,13 +72,13 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>  
>  	if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
>  		if (again > context.configuration.agc.againDef) {
> -			next = again * kExpNumeratorDown / kExpDenominator;
> +			double next = again * kExpNumeratorDown / kExpDenominator;
>  			if (again - next < context.configuration.agc.againMinStep)
>  				again -= context.configuration.agc.againMinStep;
>  			else
>  				again = next;
>  		} else {
> -			next = exposure * kExpNumeratorDown / kExpDenominator;
> +			int32_t next = exposure * kExpNumeratorDown / kExpDenominator;
>  			if (exposure - next < 1)
>  				exposure -= 1;
>  			else
Isaac Scott Sept. 25, 2025, 10:05 a.m. UTC | #2 
Hi Hans,

Thank you for the patch!

Quoting Hans de Goede (2025-09-23 20:06:56)
> Exposure is an integer, instead of re-using the "double next" used
> for again calculations, doing intermediate calculations with double
> precision, use a local next variable of an integer type.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  src/ipa/simple/algorithms/agc.cpp | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
> index 230616e62..cdde56ba2 100644
> --- a/src/ipa/simple/algorithms/agc.cpp
> +++ b/src/ipa/simple/algorithms/agc.cpp
> @@ -51,19 +51,18 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>         static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
>         static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
>  
> -       double next;
>         int32_t &exposure = frameContext.sensor.exposure;
>         double &again = frameContext.sensor.gain;
>  
>         if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
>                 if (exposure < context.configuration.agc.exposureMax) {
> -                       next = exposure * kExpNumeratorUp / kExpDenominator;
> +                       int32_t next = exposure * kExpNumeratorUp / kExpDenominator;
>                         if (next - exposure < 1)
>                                 exposure += 1;
>                         else
>                                 exposure = next;
>                 } else {
> -                       next = again * kExpNumeratorUp / kExpDenominator;
> +                       double next = again * kExpNumeratorUp / kExpDenominator;

Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>

Best wishes,
Isaac

>                         if (next - again < context.configuration.agc.againMinStep)
>                                 again += context.configuration.agc.againMinStep;
>                         else
> @@ -73,13 +72,13 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
>  
>         if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
>                 if (again > context.configuration.agc.againDef) {
> -                       next = again * kExpNumeratorDown / kExpDenominator;
> +                       double next = again * kExpNumeratorDown / kExpDenominator;
>                         if (again - next < context.configuration.agc.againMinStep)
>                                 again -= context.configuration.agc.againMinStep;
>                         else
>                                 again = next;
>                 } else {
> -                       next = exposure * kExpNumeratorDown / kExpDenominator;
> +                       int32_t next = exposure * kExpNumeratorDown / kExpDenominator;
>                         if (exposure - next < 1)
>                                 exposure -= 1;
>                         else
> -- 
> 2.51.0
>

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
index 230616e62..cdde56ba2 100644
--- a/src/ipa/simple/algorithms/agc.cpp
+++ b/src/ipa/simple/algorithms/agc.cpp
@@ -51,19 +51,18 @@  void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
 	static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
 	static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
 
-	double next;
 	int32_t &exposure = frameContext.sensor.exposure;
 	double &again = frameContext.sensor.gain;
 
 	if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
 		if (exposure < context.configuration.agc.exposureMax) {
-			next = exposure * kExpNumeratorUp / kExpDenominator;
+			int32_t next = exposure * kExpNumeratorUp / kExpDenominator;
 			if (next - exposure < 1)
 				exposure += 1;
 			else
 				exposure = next;
 		} else {
-			next = again * kExpNumeratorUp / kExpDenominator;
+			double next = again * kExpNumeratorUp / kExpDenominator;
 			if (next - again < context.configuration.agc.againMinStep)
 				again += context.configuration.agc.againMinStep;
 			else
@@ -73,13 +72,13 @@  void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou
 
 	if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
 		if (again > context.configuration.agc.againDef) {
-			next = again * kExpNumeratorDown / kExpDenominator;
+			double next = again * kExpNumeratorDown / kExpDenominator;
 			if (again - next < context.configuration.agc.againMinStep)
 				again -= context.configuration.agc.againMinStep;
 			else
 				again = next;
 		} else {
-			next = exposure * kExpNumeratorDown / kExpDenominator;
+			int32_t next = exposure * kExpNumeratorDown / kExpDenominator;
 			if (exposure - next < 1)
 				exposure -= 1;
 			else