[libcamera-devel] [PATCH 2/2] ipa: ipu3: Consolidate querying exposure and gain limits

[Umang Jain]

Hi Kieran,

On 2/28/22 19:03, Kieran Bingham wrote:
> Hi Umang,
>
> Quoting Umang Jain (2022-02-28 13:22:50)
>> The exposure and gain limits are queried as part of
>> IPAIPU3::configure(). Consolidate it.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>   src/ipa/ipu3/ipu3.cpp | 44 +++++++++++++------------------------------
>>   1 file changed, 13 insertions(+), 31 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 2fab4ee0..33fcf59b 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -196,12 +196,14 @@ private:
>>   void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>>   {
>>          const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
>> -       int32_t minExposure = v4l2Exposure.min().get<int32_t>();
>> -       int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
>> +       minExposure_ = v4l2Exposure.min().get<int32_t>();
>> +       maxExposure_ = v4l2Exposure.max().get<int32_t>();
>> +       exposure_ = minExposure_;
> Are the class members used elsewhere at all? (I.e. exposure_ etc?) or
> could they be removed leaving the state stored only in
> context_.configuration?


The class members dealing with min/max (i.e. static values) could be 
brought under there, yes. I don't think the class members exposure_ and 
gain_ can be brought under the umbrella of IPASessionConfiguration since 
they reflect the latest values computed by IPA

>
> I don't think it's in yet, but I'm sure one of JM's patches adds state
> to a IPASessionConfiguration.sensor structure to keep sensor specfic
> state tied together in the context.


yes, I see [PATCH v4 3/4] ipa: ipu3: agc: Introduce lineDuration in 
IPASessionConfiguration

We can have min/max exposure/gain under them I think

>
> These properties might really belong in there.
>
>>          const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;
>> -       int32_t minGain = v4l2Gain.min().get<int32_t>();
>> -       int32_t maxGain = v4l2Gain.max().get<int32_t>();
>> +       minGain_ = v4l2Gain.min().get<int32_t>();
>> +       maxGain_ = v4l2Gain.max().get<int32_t>();
>> +       gain_ = minGain_;
>>   
>>          /*
>>           * When the AGC computes the new exposure values for a frame, it needs
>> @@ -210,10 +212,10 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
>>           *
>>           * \todo take VBLANK into account for maximum shutter speed
>>           */
>> -       context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_;
>> -       context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_;
>> -       context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
>> -       context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
>> +       context_.configuration.agc.minShutterSpeed = minExposure_ * lineDuration_;
>> +       context_.configuration.agc.maxShutterSpeed = maxExposure_ * lineDuration_;
>> +       context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain_);
>> +       context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain_);
>>   }
>>   
>>   /**
>> @@ -430,32 +432,12 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>           */
>>          ctrls_ = configInfo.sensorControls;
>>   
>> -       const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>> -       if (itExp == ctrls_.end()) {
>> -               LOG(IPAIPU3, Error) << "Can't find exposure control";
>> -               return -EINVAL;
>> -       }
>> -
>> -       const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>> -       if (itGain == ctrls_.end()) {
>> -               LOG(IPAIPU3, Error) << "Can't find gain control";
>> -               return -EINVAL;
>> -       }
>> -
>>          const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);
>>          if (itVBlank == ctrls_.end()) {
>>                  LOG(IPAIPU3, Error) << "Can't find VBLANK control";
>>                  return -EINVAL;
>>          }
>>   
>> -       minExposure_ = itExp->second.min().get<int32_t>();
>> -       maxExposure_ = itExp->second.max().get<int32_t>();
>> -       exposure_ = minExposure_;
>> -
>> -       minGain_ = itGain->second.min().get<int32_t>();
>> -       maxGain_ = itGain->second.max().get<int32_t>();
>> -       gain_ = minGain_;
>> -
>>          defVBlank_ = itVBlank->second.def().get<int32_t>();
>>   
>>          /* Clean context at configuration */
>> @@ -465,12 +447,12 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>   
>>          lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;
>>   
>> -       /* Update the camera controls using the new sensor settings. */
>> -       updateControls(sensorInfo_, ctrls_, ipaControls);
>> -
>>          /* Update the IPASessionConfiguration using the sensor settings. */
>>          updateSessionConfiguration(ctrls_);
>>   
>> +       /* Update the camera controls using the new sensor settings. */
>> +       updateControls(sensorInfo_, ctrls_, ipaControls);
>> +
>>          for (auto const &algo : algorithms_) {
>>                  int ret = algo->configure(context_, configInfo);
>>                  if (ret)
>> -- 
>> 2.31.1
>>