| Message ID | 20220228132250.116211-3-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
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@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? 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. 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 >
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@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 >>
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@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? Re-looking the patch now, and discussion from yesterday, I guess we can drop these maximums and minimums members from IPAIPU3. And no, I don't think we should add them to context_.configuration.sensor (yet) because I don't see max/min used anywhere else. I'll prep a follow-up patch > > 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. > > 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 >>
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_; 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)
The exposure and gain limits are queried as part of IPAIPU3::configure(). Consolidate it. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/ipa/ipu3/ipu3.cpp | 44 +++++++++++++------------------------------ 1 file changed, 13 insertions(+), 31 deletions(-)