[v1] ipa: simple: Fix `again10` value with sensor helper

26265 diff mbox series

`CameraSensorHelper::gain(uint32_t)` maps a gain code to the actual floating
point gain value. Calling it with `1.0` as the argument will simply get
the real gain for gain code 1. This is most likely not what was intended.

For example, in the case of the `ov2740` sensor, `againMin` is 1, but the
calculated `again10` (1 / 128 ~ 0.078) ends up being < 1, meaning that the
agc algorithm will never lower the exposure.

Fix that by using the maximum of the minimum gain and 1 as `again10`.

Fixes: 950ca85e8aa5 ("ipa: software_isp: AGC: Do not lower gain below 1.0")
---
 src/ipa/simple/soft_simple.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.53.0

Hi Barnabás,

thank you for the fix.

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> `CameraSensorHelper::gain(uint32_t)` maps a gain code to the actual floating
> point gain value. Calling it with `1.0` as the argument will simply get
> the real gain for gain code 1. This is most likely not what was intended.
>
> For example, in the case of the `ov2740` sensor, `againMin` is 1, but the
> calculated `again10` (1 / 128 ~ 0.078) ends up being < 1, meaning that the
> agc algorithm will never lower the exposure.
>
> Fix that by using the maximum of the minimum gain and 1 as `again10`.
>
> Fixes: 950ca85e8aa5 ("ipa: software_isp: AGC: Do not lower gain below 1.0")

Missing Signed-Off-By.

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

> ---
>  src/ipa/simple/soft_simple.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index c35277cfe..7d25bdd26 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -227,7 +227,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>  	if (camHelper_) {
>  		context_.configuration.agc.againMin = camHelper_->gain(againMin);
>  		context_.configuration.agc.againMax = camHelper_->gain(againMax);
> -		context_.configuration.agc.again10 = camHelper_->gain(1.0);
> +		context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0);
>  		context_.configuration.agc.againMinStep =
>  			(context_.configuration.agc.againMax -
>  			 context_.configuration.agc.againMin) /
> --
> 2.53.0

2026. 03. 06. 16:16 keltezéssel, Milan Zamazal írta:
> Hi Barnabás,
> 
> thank you for the fix.
> 
> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:
> 
>> `CameraSensorHelper::gain(uint32_t)` maps a gain code to the actual floating
>> point gain value. Calling it with `1.0` as the argument will simply get
>> the real gain for gain code 1. This is most likely not what was intended.
>>
>> For example, in the case of the `ov2740` sensor, `againMin` is 1, but the
>> calculated `again10` (1 / 128 ~ 0.078) ends up being < 1, meaning that the
>> agc algorithm will never lower the exposure.
>>
>> Fix that by using the maximum of the minimum gain and 1 as `again10`.
>>
>> Fixes: 950ca85e8aa5 ("ipa: software_isp: AGC: Do not lower gain below 1.0")
> 
> Missing Signed-Off-By.
> 
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

Oops, indeed.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


> 
>> ---
>>   src/ipa/simple/soft_simple.cpp | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index c35277cfe..7d25bdd26 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -227,7 +227,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>   	if (camHelper_) {
>>   		context_.configuration.agc.againMin = camHelper_->gain(againMin);
>>   		context_.configuration.agc.againMax = camHelper_->gain(againMax);
>> -		context_.configuration.agc.again10 = camHelper_->gain(1.0);
>> +		context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0);
>>   		context_.configuration.agc.againMinStep =
>>   			(context_.configuration.agc.againMax -
>>   			 context_.configuration.agc.againMin) /
>> --
>> 2.53.0
>

Hi,

On 6-Mar-26 3:42 PM, Barnabás Pőcze wrote:
> `CameraSensorHelper::gain(uint32_t)` maps a gain code to the actual floating
> point gain value. Calling it with `1.0` as the argument will simply get
> the real gain for gain code 1. This is most likely not what was intended.
> 
> For example, in the case of the `ov2740` sensor, `againMin` is 1, but the
> calculated `again10` (1 / 128 ~ 0.078) ends up being < 1, meaning that the
> agc algorithm will never lower the exposure.
> 
> Fix that by using the maximum of the minimum gain and 1 as `again10`.
> 
> Fixes: 950ca85e8aa5 ("ipa: software_isp: AGC: Do not lower gain below 1.0")
> ---
>  src/ipa/simple/soft_simple.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index c35277cfe..7d25bdd26 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -227,7 +227,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>  	if (camHelper_) {
>  		context_.configuration.agc.againMin = camHelper_->gain(againMin);
>  		context_.configuration.agc.againMax = camHelper_->gain(againMax);
> -		context_.configuration.agc.again10 = camHelper_->gain(1.0);
> +		context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0);

Good catch, patch looks good to me:

Fixes: 950ca85e8aa5 ("ipa: software_isp: AGC: Do not lower gain below 1.0")
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans






		context_.configuration.agc.again10 =
			std::max(context_.configuration.agc.againMin, 1.0);
>  		context_.configuration.agc.againMinStep =
>  			(context_.configuration.agc.againMax -
>  			 context_.configuration.agc.againMin) /
> --
> 2.53.0