[4/8] ipa: rkisp1: Add correction for exposure quantization

23171 diff mbox series

When WDR regulation is active, it can happen that exposure times get set
to very low values (Sometimes 2-3 lines). This intentionally introduced
underexposure is corrected by the GWDR module. As exposure time is
quantized by lines, the smalles possible change in exposure time now
results in a quite visible change in perceived brightness.

Mitigate that by applying a small global gain to account for the error
introduced by the exposure quantization.

ToDo: This needs perfect frame synchronous control of the sensor to work
properly which is not guaranteed in all cases. It still improves the
behavior with the current regulation.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp |  2 ++
 src/ipa/rkisp1/algorithms/awb.cpp | 12 ++++++++----
 src/ipa/rkisp1/ipa_context.h      |  2 ++
 3 files changed, 12 insertions(+), 4 deletions(-)

Hi Stefan,

Thanks for the patch.

Quoting Stefan Klug (2025-04-11 15:04:11)
> When WDR regulation is active, it can happen that exposure times get set
> to very low values (Sometimes 2-3 lines). This intentionally introduced
> underexposure is corrected by the GWDR module. As exposure time is
> quantized by lines, the smalles possible change in exposure time now

s/smalles/smallest/

> results in a quite visible change in perceived brightness.

I see, at very low exposure then gainLostInExposureQuantization becomes quite
significant. 

> Mitigate that by applying a small global gain to account for the error
> introduced by the exposure quantization.

And we want to set it globally in awb for lower latency as opposed to in the
sensor, ok. I suppose this is how we'd do digital gain eventually too maybe.

> ToDo: This needs perfect frame synchronous control of the sensor to work
> properly which is not guaranteed in all cases. It still improves the
> behavior with the current regulation.

Would this be good to capture as a comment? Or maybe it's fine because it's a
general pfc problem. Either way is fine.

> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp |  2 ++
>  src/ipa/rkisp1/algorithms/awb.cpp | 12 ++++++++----
>  src/ipa/rkisp1/ipa_context.h      |  2 ++
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 8e77455e7afd..0e4fd3663167 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -583,6 +583,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>         activeState.agc.automatic.exposure = newExposureTime / lineDuration;
>         activeState.agc.automatic.gain = aGain;
>  
> +       activeState.agc.automatic.gainLostInExposureQuantization =

Doesn't this need to be initialized to 1.0 in configure() ?


Paul

> +               newExposureTime / (activeState.agc.automatic.exposure * lineDuration);
>         /*
>          * Expand the target frame duration so that we do not run faster than
>          * the minimum frame duration when we have short exposures.
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 84aa1d29419d..cc78136a22c8 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -170,6 +170,7 @@ void Awb::queueRequest(IPAContext &context,
>         awbAlgo_->handleControls(controls);
>  
>         frameContext.awb.autoEnabled = awb.autoEnabled;
> +       frameContext.awb.additionalGain = 1.0;
>  
>         if (awb.autoEnabled)
>                 return;
> @@ -218,15 +219,18 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,
>                 auto &awb = context.activeState.awb;
>                 frameContext.awb.gains = awb.automatic.gains;
>                 frameContext.awb.temperatureK = awb.automatic.temperatureK;
> +               frameContext.awb.additionalGain =
> +                       context.activeState.agc.automatic.gainLostInExposureQuantization;
>         }
>  
> +       auto gains = frameContext.awb.gains * frameContext.awb.additionalGain;
>         auto gainConfig = params->block<BlockType::AwbGain>();
>         gainConfig.setEnabled(true);
>  
> -       gainConfig->gain_green_b = std::clamp<int>(256 * frameContext.awb.gains.g(), 0, 0x3ff);
> -       gainConfig->gain_blue = std::clamp<int>(256 * frameContext.awb.gains.b(), 0, 0x3ff);
> -       gainConfig->gain_red = std::clamp<int>(256 * frameContext.awb.gains.r(), 0, 0x3ff);
> -       gainConfig->gain_green_r = std::clamp<int>(256 * frameContext.awb.gains.g(), 0, 0x3ff);
> +       gainConfig->gain_green_b = std::clamp<int>(256 * gains.g(), 0, 0x3ff);
> +       gainConfig->gain_blue = std::clamp<int>(256 * gains.b(), 0, 0x3ff);
> +       gainConfig->gain_red = std::clamp<int>(256 * gains.r(), 0, 0x3ff);
> +       gainConfig->gain_green_r = std::clamp<int>(256 * gains.g(), 0, 0x3ff);
>  
>         /* If we have already set the AWB measurement parameters, return. */
>         if (frame > 0)
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 7ccc7b501aff..182203880ac9 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -76,6 +76,7 @@ struct IPAActiveState {
>                 } manual;
>                 struct {
>                         uint32_t exposure;
> +                       double gainLostInExposureQuantization;
>                         double gain;
>                 } automatic;
>  
> @@ -149,6 +150,7 @@ struct IPAFrameContext : public FrameContext {
>                 RGB<double> gains;
>                 bool autoEnabled;
>                 unsigned int temperatureK;
> +               double additionalGain;
>         } awb;
>  
>         struct {
> -- 
> 2.43.0
>