[libcamera-devel,v3,03/11] ipa: Do not modify the sensor limits
diff mbox series

Message ID 20211123150423.125524-4-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • Introduce AGC for RkISP1
Related show

Commit Message

Jean-Michel Hautbois Nov. 23, 2021, 3:04 p.m. UTC 
The driver is responsible of setting the proper limits for its controls.
For instance, imx219 has an analogue gain of 1.0 when the gain code is
set to 0. The minimum analogue is forced to be at least 1, which for
imx219 sets it to 1.00329. Rework this for both IPU3 and RkISP1.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp     | 4 ++--
 src/ipa/rkisp1/rkisp1.cpp | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Kieran Bingham Nov. 23, 2021, 3:22 p.m. UTC | #1 
Quoting Jean-Michel Hautbois (2021-11-23 15:04:15)
> The driver is responsible of setting the proper limits for its controls.

s/of/for/

> For instance, imx219 has an analogue gain of 1.0 when the gain code is
> set to 0. The minimum analogue is forced to be at least 1, which for

The minimum ... exposure? (Is analogue really correct here?)

Also

The minimum [exposure] is currently forced to be
                          ^^^^^^^^^

That just helps readability.

With that,


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

> imx219 sets it to 1.00329. Rework this for both IPU3 and RkISP1.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp     | 4 ++--
>  src/ipa/rkisp1/rkisp1.cpp | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index a8d54a5d..b0c75541 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -444,11 +444,11 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>                 return -EINVAL;
>         }
>  
> -       minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
> +       minExposure_ = itExp->second.min().get<int32_t>();
>         maxExposure_ = itExp->second.max().get<int32_t>();
>         exposure_ = minExposure_;
>  
> -       minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
> +       minGain_ = itGain->second.min().get<int32_t>();
>         maxGain_ = itGain->second.max().get<int32_t>();
>         gain_ = minGain_;
>  
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 7ecbf8ae..910ad952 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -139,11 +139,11 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>  
>         autoExposure_ = true;
>  
> -       minExposure_ = std::max<uint32_t>(itExp->second.min().get<int32_t>(), 1);
> +       minExposure_ = itExp->second.min().get<int32_t>();
>         maxExposure_ = itExp->second.max().get<int32_t>();
>         exposure_ = minExposure_;
>  
> -       minGain_ = std::max<uint32_t>(itGain->second.min().get<int32_t>(), 1);
> +       minGain_ = itGain->second.min().get<int32_t>();
>         maxGain_ = itGain->second.max().get<int32_t>();
>         gain_ = minGain_;
>  
> -- 
> 2.32.0
>
Jean-Michel Hautbois Nov. 23, 2021, 3:23 p.m. UTC | #2 
On 23/11/2021 16:22, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-11-23 15:04:15)
>> The driver is responsible of setting the proper limits for its controls.
> 
> s/of/for/
> 
>> For instance, imx219 has an analogue gain of 1.0 when the gain code is
>> set to 0. The minimum analogue is forced to be at least 1, which for
> 
> The minimum ... exposure? (Is analogue really correct here?)

It is the analogue gain for this imx219 example.

> 
> Also
> 
> The minimum [exposure] is currently forced to be
>                            ^^^^^^^^^
> 
> That just helps readability.
> 
> With that,
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> imx219 sets it to 1.00329. Rework this for both IPU3 and RkISP1.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/ipu3.cpp     | 4 ++--
>>   src/ipa/rkisp1/rkisp1.cpp | 4 ++--
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index a8d54a5d..b0c75541 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -444,11 +444,11 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>                  return -EINVAL;
>>          }
>>   
>> -       minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
>> +       minExposure_ = itExp->second.min().get<int32_t>();
>>          maxExposure_ = itExp->second.max().get<int32_t>();
>>          exposure_ = minExposure_;
>>   
>> -       minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>> +       minGain_ = itGain->second.min().get<int32_t>();
>>          maxGain_ = itGain->second.max().get<int32_t>();
>>          gain_ = minGain_;
>>   
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 7ecbf8ae..910ad952 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -139,11 +139,11 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>>   
>>          autoExposure_ = true;
>>   
>> -       minExposure_ = std::max<uint32_t>(itExp->second.min().get<int32_t>(), 1);
>> +       minExposure_ = itExp->second.min().get<int32_t>();
>>          maxExposure_ = itExp->second.max().get<int32_t>();
>>          exposure_ = minExposure_;
>>   
>> -       minGain_ = std::max<uint32_t>(itGain->second.min().get<int32_t>(), 1);
>> +       minGain_ = itGain->second.min().get<int32_t>();
>>          maxGain_ = itGain->second.max().get<int32_t>();
>>          gain_ = minGain_;
>>   
>> -- 
>> 2.32.0
>>
Kieran Bingham Nov. 23, 2021, 4:12 p.m. UTC | #3 
Quoting Jean-Michel Hautbois (2021-11-23 15:23:42)
> 
> 
> On 23/11/2021 16:22, Kieran Bingham wrote:
> > Quoting Jean-Michel Hautbois (2021-11-23 15:04:15)
> >> The driver is responsible of setting the proper limits for its controls.
> > 
> > s/of/for/
> > 
> >> For instance, imx219 has an analogue gain of 1.0 when the gain code is
> >> set to 0. The minimum analogue is forced to be at least 1, which for
> > 
> > The minimum ... exposure? (Is analogue really correct here?)
> 
> It is the analogue gain for this imx219 example.

Ok, so something like:

"""
The driver is responsible for setting the proper limits for its
controls.

The IMX219 has an analogue gain of 1.0 when the gain code is set to 0,
therefore we can not clamp to a minimum gain code of 1.

Rework this for both IPU3 and RkISP1, for both Exposure and Gain
controls.
"""

> > Also
> > 
> > The minimum [exposure] is currently forced to be
> >                            ^^^^^^^^^
> > 
> > That just helps readability.
> > 
> > With that,
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> >> imx219 sets it to 1.00329. Rework this for both IPU3 and RkISP1.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> ---
> >>   src/ipa/ipu3/ipu3.cpp     | 4 ++--
> >>   src/ipa/rkisp1/rkisp1.cpp | 4 ++--
> >>   2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index a8d54a5d..b0c75541 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -444,11 +444,11 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >>                  return -EINVAL;
> >>          }
> >>   
> >> -       minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
> >> +       minExposure_ = itExp->second.min().get<int32_t>();
> >>          maxExposure_ = itExp->second.max().get<int32_t>();
> >>          exposure_ = minExposure_;
> >>   
> >> -       minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
> >> +       minGain_ = itGain->second.min().get<int32_t>();
> >>          maxGain_ = itGain->second.max().get<int32_t>();
> >>          gain_ = minGain_;
> >>   
> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >> index 7ecbf8ae..910ad952 100644
> >> --- a/src/ipa/rkisp1/rkisp1.cpp
> >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >> @@ -139,11 +139,11 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> >>   
> >>          autoExposure_ = true;
> >>   
> >> -       minExposure_ = std::max<uint32_t>(itExp->second.min().get<int32_t>(), 1);
> >> +       minExposure_ = itExp->second.min().get<int32_t>();
> >>          maxExposure_ = itExp->second.max().get<int32_t>();
> >>          exposure_ = minExposure_;
> >>   
> >> -       minGain_ = std::max<uint32_t>(itGain->second.min().get<int32_t>(), 1);
> >> +       minGain_ = itGain->second.min().get<int32_t>();
> >>          maxGain_ = itGain->second.max().get<int32_t>();
> >>          gain_ = minGain_;
> >>   
> >> -- 
> >> 2.32.0
> >>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index a8d54a5d..b0c75541 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -444,11 +444,11 @@  int IPAIPU3::configure(const IPAConfigInfo &configInfo,
 		return -EINVAL;
 	}
 
-	minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
+	minExposure_ = itExp->second.min().get<int32_t>();
 	maxExposure_ = itExp->second.max().get<int32_t>();
 	exposure_ = minExposure_;
 
-	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
+	minGain_ = itGain->second.min().get<int32_t>();
 	maxGain_ = itGain->second.max().get<int32_t>();
 	gain_ = minGain_;
 
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 7ecbf8ae..910ad952 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -139,11 +139,11 @@  int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
 
 	autoExposure_ = true;
 
-	minExposure_ = std::max<uint32_t>(itExp->second.min().get<int32_t>(), 1);
+	minExposure_ = itExp->second.min().get<int32_t>();
 	maxExposure_ = itExp->second.max().get<int32_t>();
 	exposure_ = minExposure_;
 
-	minGain_ = std::max<uint32_t>(itGain->second.min().get<int32_t>(), 1);
+	minGain_ = itGain->second.min().get<int32_t>();
 	maxGain_ = itGain->second.max().get<int32_t>();
 	gain_ = minGain_;