ipa: rkisp1: Allow exposure time to be shorter than minimum frame duration limit

22881 diff mbox series

The minimum FrameDurationLimit also limits the min exposure time and
results in overly bright AE regulation. Remove the limit on the minimum
exposure time as the vertical blanking ensures the minimum frame
duration limit.

Fixes: f72c76eb6e06 ("rkisp1: Honor the FrameDurationLimits control")
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Quoting Stefan Klug (2025-02-26 15:59:26)
> The minimum FrameDurationLimit also limits the min exposure time and
> results in overly bright AE regulation. Remove the limit on the minimum
> exposure time as the vertical blanking ensures the minimum frame
> duration limit.
> 
> Fixes: f72c76eb6e06 ("rkisp1: Honor the FrameDurationLimits control")
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

Thanks, this is this is /very/ noticible here and the fix is very clear
(espcially thanks to the live graphs in camshark!)

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 4e0e3734117e..5a3ba0131cf1 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -526,9 +526,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>         double maxAnalogueGain;
>  
>         if (frameContext.agc.autoExposureEnabled) {
> -               minExposureTime = std::clamp(frameContext.agc.minFrameDuration,
> -                                            context.configuration.sensor.minExposureTime,
> -                                            context.configuration.sensor.maxExposureTime);
> +               minExposureTime = context.configuration.sensor.minExposureTime;
>                 maxExposureTime = std::clamp(frameContext.agc.maxFrameDuration,
>                                              context.configuration.sensor.minExposureTime,
>                                              context.configuration.sensor.maxExposureTime);
> -- 
> 2.43.0
>

Hi Stefan

On Wed, Feb 26, 2025 at 04:59:26PM +0100, Stefan Klug wrote:
> The minimum FrameDurationLimit also limits the min exposure time and
> results in overly bright AE regulation. Remove the limit on the minimum
> exposure time as the vertical blanking ensures the minimum frame
> duration limit.
>
> Fixes: f72c76eb6e06 ("rkisp1: Honor the FrameDurationLimits control")
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 4e0e3734117e..5a3ba0131cf1 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -526,9 +526,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  	double maxAnalogueGain;
>
>  	if (frameContext.agc.autoExposureEnabled) {
> -		minExposureTime = std::clamp(frameContext.agc.minFrameDuration,
> -					     context.configuration.sensor.minExposureTime,
> -					     context.configuration.sensor.maxExposureTime);
> +		minExposureTime = context.configuration.sensor.minExposureTime;
>  		maxExposureTime = std::clamp(frameContext.agc.maxFrameDuration,
>  					     context.configuration.sensor.minExposureTime,
>  					     context.configuration.sensor.maxExposureTime);
> --
> 2.43.0
>