[v2,2/3] ipa: libipa: agc_mean_luminance: Error out when effectiveExposureValue is zero
diff mbox series

Message ID 20250326134727.279393-3-stefan.klug@ideasonboard.com
State Accepted
Commit 03bae6b9248280a3edd9350061cc307d95ba1cd0
Headers show
Series
  • rkisp1: Reduce oscillations on startup
Related show

Commit Message

Stefan Klug March 26, 2025, 1:47 p.m. UTC
In a proper system it never happens that the effectiveExposureValue
drops to zero. If that still happens due to a bug outside of
agc_mean_luminance, the calculated gain goes towards infinity but the
newExposureValue is still 0 because it is the result of multiplying the
effectiveExposureTime with the gain, leading to wild oscillations.

Catch that condition, print an error message and set the new effective
exposure value to an arbitrary 10ms.

Note that in any case the underlying problem must be fixed. The
important change is the added error message to be able to detect such a
situation.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

---

Changes in v2:
- Collected tags
- Improved log message wording
- Added comment on the arbitrary exposure time
---
 src/ipa/libipa/agc_mean_luminance.cpp | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Laurent Pinchart March 26, 2025, 3:05 p.m. UTC | #1
Hi Stefan,

Thank you for the patch.

On Wed, Mar 26, 2025 at 02:47:21PM +0100, Stefan Klug wrote:
> In a proper system it never happens that the effectiveExposureValue
> drops to zero. If that still happens due to a bug outside of
> agc_mean_luminance, the calculated gain goes towards infinity but the
> newExposureValue is still 0 because it is the result of multiplying the
> effectiveExposureTime with the gain, leading to wild oscillations.
> 
> Catch that condition, print an error message and set the new effective
> exposure value to an arbitrary 10ms.
> 
> Note that in any case the underlying problem must be fixed. The
> important change is the added error message to be able to detect such a
> situation.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

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

> ---
> 
> Changes in v2:
> - Collected tags
> - Improved log message wording
> - Added comment on the arbitrary exposure time
> ---
>  src/ipa/libipa/agc_mean_luminance.cpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 02555a44d271..f617fde81101 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -541,6 +541,18 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
>  	std::shared_ptr<ExposureModeHelper> exposureModeHelper =
>  		exposureModeHelpers_.at(exposureModeIndex);
>  
> +	if (effectiveExposureValue == 0s) {
> +		LOG(AgcMeanLuminance, Error)
> +			<< "Effective exposure value is 0. This is a bug in AGC "
> +			   "and must be fixed for proper operation.";
> +		/*
> +		 * Return an arbitrary exposure time > 0 to ensure regulation
> +		 * doesn't get stuck with 0 in case the sensor driver allows a
> +		 * min exposure of 0.
> +		 */
> +		return exposureModeHelper->splitExposure(10ms);
> +	}
> +
>  	double gain = estimateInitialGain();
>  	gain = constraintClampGain(constraintModeIndex, yHist, gain);
>

Patch
diff mbox series

diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
index 02555a44d271..f617fde81101 100644
--- a/src/ipa/libipa/agc_mean_luminance.cpp
+++ b/src/ipa/libipa/agc_mean_luminance.cpp
@@ -541,6 +541,18 @@  AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
 	std::shared_ptr<ExposureModeHelper> exposureModeHelper =
 		exposureModeHelpers_.at(exposureModeIndex);
 
+	if (effectiveExposureValue == 0s) {
+		LOG(AgcMeanLuminance, Error)
+			<< "Effective exposure value is 0. This is a bug in AGC "
+			   "and must be fixed for proper operation.";
+		/*
+		 * Return an arbitrary exposure time > 0 to ensure regulation
+		 * doesn't get stuck with 0 in case the sensor driver allows a
+		 * min exposure of 0.
+		 */
+		return exposureModeHelper->splitExposure(10ms);
+	}
+
 	double gain = estimateInitialGain();
 	gain = constraintClampGain(constraintModeIndex, yHist, gain);