| Message ID | 20220301075919.201968-1-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Quoting Umang Jain (2022-03-01 07:59:19) > The exposure and gain limits are required for AGC configuration > handled in IPAIPU3::updateSessionConfiguration(), which is happening > already. Therefore the max/min private members in IPAIPU3 class for > exposure/gain serve no use except setting initial values of exposure_ > and gain_ members. > > Drop the max/min private members from IPAIPU3 class and set initial > gain_ and exposure_ in IPAIPU3::updateSessionConfiguration(). Great, even better to drop them if they aren't used. This looks like we're on the path to removing the private variables for controls from the IPAIPU3 now, as they should all be stored in the context if there's any relationship with the algorithms. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > Changes in v2: > - Split into separate patch > - Prepped over "v4 ipa: ipu3: Misc clean up" > - Don't include max/min exposure/gain members in > IPASessionConfiguration, as they aren't used anywhere. > --- > src/ipa/ipu3/ipu3.cpp | 26 ++------------------------ > 1 file changed, 2 insertions(+), 24 deletions(-) > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index 17324616..4b6852a7 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -167,11 +167,7 @@ private: > /* Camera sensor controls. */ > uint32_t defVBlank_; > uint32_t exposure_; > - uint32_t minExposure_; > - uint32_t maxExposure_; > uint32_t gain_; > - uint32_t minGain_; > - uint32_t maxGain_; What's required to get rid of gain_, exposure_ and defVBlank_ from here too? -- Kieran > > /* Interface to the Camera Helper */ > std::unique_ptr<CameraSensorHelper> camHelper_; > @@ -192,10 +188,12 @@ 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>(); > + 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>(); > + gain_ = minGain; > > /* > * When the AGC computes the new exposure values for a frame, it needs > @@ -429,32 +427,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>(); > > calculateBdsGrid(configInfo.bdsOutputSize); > -- > 2.31.1 >
Hi Kieran, On 3/3/22 17:17, Kieran Bingham wrote: > Quoting Umang Jain (2022-03-01 07:59:19) >> The exposure and gain limits are required for AGC configuration >> handled in IPAIPU3::updateSessionConfiguration(), which is happening >> already. Therefore the max/min private members in IPAIPU3 class for >> exposure/gain serve no use except setting initial values of exposure_ >> and gain_ members. >> >> Drop the max/min private members from IPAIPU3 class and set initial >> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration(). > > Great, even better to drop them if they aren't used. > > This looks like we're on the path to removing the private variables for > controls from the IPAIPU3 now, as they should all be stored in the > context if there's any relationship with the algorithms. yeah, we are on that path. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> Changes in v2: >> - Split into separate patch >> - Prepped over "v4 ipa: ipu3: Misc clean up" >> - Don't include max/min exposure/gain members in >> IPASessionConfiguration, as they aren't used anywhere. >> --- >> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------ >> 1 file changed, 2 insertions(+), 24 deletions(-) >> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index 17324616..4b6852a7 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -167,11 +167,7 @@ private: >> /* Camera sensor controls. */ >> uint32_t defVBlank_; >> uint32_t exposure_; >> - uint32_t minExposure_; >> - uint32_t maxExposure_; >> uint32_t gain_; >> - uint32_t minGain_; >> - uint32_t maxGain_; > What's required to get rid of gain_, exposure_ and defVBlank_ from here > too? we can drop gain_, exposure_ and replace them with local vars, no issue in that. defVBlank_ seems to be used IPAIPU3 class functions directly, but i think we can move the defVBlank_ to JM's introduction of IPASessionConfiguration.sensor structure With this plan i think we can drop these remaining private members from IPAIPU3. > > -- > Kieran > > >> >> /* Interface to the Camera Helper */ >> std::unique_ptr<CameraSensorHelper> camHelper_; >> @@ -192,10 +188,12 @@ 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>(); >> + 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>(); >> + gain_ = minGain; >> >> /* >> * When the AGC computes the new exposure values for a frame, it needs >> @@ -429,32 +427,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>(); >> >> calculateBdsGrid(configInfo.bdsOutputSize); >> -- >> 2.31.1 >>
Quoting Umang Jain (2022-03-03 12:15:44) > Hi Kieran, > > On 3/3/22 17:17, Kieran Bingham wrote: > > Quoting Umang Jain (2022-03-01 07:59:19) > >> The exposure and gain limits are required for AGC configuration > >> handled in IPAIPU3::updateSessionConfiguration(), which is happening > >> already. Therefore the max/min private members in IPAIPU3 class for > >> exposure/gain serve no use except setting initial values of exposure_ > >> and gain_ members. > >> > >> Drop the max/min private members from IPAIPU3 class and set initial > >> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration(). > > > > Great, even better to drop them if they aren't used. > > > > This looks like we're on the path to removing the private variables for > > controls from the IPAIPU3 now, as they should all be stored in the > > context if there's any relationship with the algorithms. > > > yeah, we are on that path. > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> Changes in v2: > >> - Split into separate patch > >> - Prepped over "v4 ipa: ipu3: Misc clean up" > >> - Don't include max/min exposure/gain members in > >> IPASessionConfiguration, as they aren't used anywhere. > >> --- > >> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------ > >> 1 file changed, 2 insertions(+), 24 deletions(-) > >> > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >> index 17324616..4b6852a7 100644 > >> --- a/src/ipa/ipu3/ipu3.cpp > >> +++ b/src/ipa/ipu3/ipu3.cpp > >> @@ -167,11 +167,7 @@ private: > >> /* Camera sensor controls. */ > >> uint32_t defVBlank_; > >> uint32_t exposure_; > >> - uint32_t minExposure_; > >> - uint32_t maxExposure_; > >> uint32_t gain_; > >> - uint32_t minGain_; > >> - uint32_t maxGain_; > > What's required to get rid of gain_, exposure_ and defVBlank_ from here > > too? > > > we can drop gain_, exposure_ and replace them with local vars, no issue > in that. > > defVBlank_ seems to be used IPAIPU3 class functions directly, but i > think we can move the defVBlank_ to JM's introduction of > IPASessionConfiguration.sensor structure > > With this plan i think we can drop these remaining private members from > IPAIPU3. Great, can easily be patches on top - doesn't need to be in this one. -- Kieran > > > > > -- > > Kieran > > > > > >> > >> /* Interface to the Camera Helper */ > >> std::unique_ptr<CameraSensorHelper> camHelper_; > >> @@ -192,10 +188,12 @@ 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>(); > >> + 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>(); > >> + gain_ = minGain; > >> > >> /* > >> * When the AGC computes the new exposure values for a frame, it needs > >> @@ -429,32 +427,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>(); > >> > >> calculateBdsGrid(configInfo.bdsOutputSize); > >> -- > >> 2.31.1 > >>
Hi, On 3/3/22 19:36, Kieran Bingham wrote: > Quoting Umang Jain (2022-03-03 12:15:44) >> Hi Kieran, >> >> On 3/3/22 17:17, Kieran Bingham wrote: >>> Quoting Umang Jain (2022-03-01 07:59:19) >>>> The exposure and gain limits are required for AGC configuration >>>> handled in IPAIPU3::updateSessionConfiguration(), which is happening >>>> already. Therefore the max/min private members in IPAIPU3 class for >>>> exposure/gain serve no use except setting initial values of exposure_ >>>> and gain_ members. >>>> >>>> Drop the max/min private members from IPAIPU3 class and set initial >>>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration(). >>> Great, even better to drop them if they aren't used. >>> >>> This looks like we're on the path to removing the private variables for >>> controls from the IPAIPU3 now, as they should all be stored in the >>> context if there's any relationship with the algorithms. >> >> yeah, we are on that path. >> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>> --- >>>> Changes in v2: >>>> - Split into separate patch >>>> - Prepped over "v4 ipa: ipu3: Misc clean up" >>>> - Don't include max/min exposure/gain members in >>>> IPASessionConfiguration, as they aren't used anywhere. >>>> --- >>>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------ >>>> 1 file changed, 2 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>> index 17324616..4b6852a7 100644 >>>> --- a/src/ipa/ipu3/ipu3.cpp >>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>> @@ -167,11 +167,7 @@ private: >>>> /* Camera sensor controls. */ >>>> uint32_t defVBlank_; >>>> uint32_t exposure_; >>>> - uint32_t minExposure_; >>>> - uint32_t maxExposure_; >>>> uint32_t gain_; >>>> - uint32_t minGain_; >>>> - uint32_t maxGain_; >>> What's required to get rid of gain_, exposure_ and defVBlank_ from here >>> too? >> >> we can drop gain_, exposure_ and replace them with local vars, no issue >> in that. >> >> defVBlank_ seems to be used IPAIPU3 class functions directly, but i >> think we can move the defVBlank_ to JM's introduction of >> IPASessionConfiguration.sensor structure >> >> With this plan i think we can drop these remaining private members from >> IPAIPU3. > Great, can easily be patches on top - doesn't need to be in this one. Ok. I have been thinking to introduce similar control-not-found error checks(as removed in this patch) in updateSessionConfiguration(), since we ll end up querying the exposure, gain, vblank controls there /only/ . Might as well introduce the control-not-found error checks there. > > -- > Kieran > > >>> -- >>> Kieran >>> >>> >>>> >>>> /* Interface to the Camera Helper */ >>>> std::unique_ptr<CameraSensorHelper> camHelper_; >>>> @@ -192,10 +188,12 @@ 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>(); >>>> + 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>(); >>>> + gain_ = minGain; >>>> >>>> /* >>>> * When the AGC computes the new exposure values for a frame, it needs >>>> @@ -429,32 +427,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>(); >>>> >>>> calculateBdsGrid(configInfo.bdsOutputSize); >>>> -- >>>> 2.31.1 >>>>
Quoting Umang Jain (2022-03-03 14:14:06) > Hi, > > On 3/3/22 19:36, Kieran Bingham wrote: > > Quoting Umang Jain (2022-03-03 12:15:44) > >> Hi Kieran, > >> > >> On 3/3/22 17:17, Kieran Bingham wrote: > >>> Quoting Umang Jain (2022-03-01 07:59:19) > >>>> The exposure and gain limits are required for AGC configuration > >>>> handled in IPAIPU3::updateSessionConfiguration(), which is happening > >>>> already. Therefore the max/min private members in IPAIPU3 class for > >>>> exposure/gain serve no use except setting initial values of exposure_ > >>>> and gain_ members. > >>>> > >>>> Drop the max/min private members from IPAIPU3 class and set initial > >>>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration(). > >>> Great, even better to drop them if they aren't used. > >>> > >>> This looks like we're on the path to removing the private variables for > >>> controls from the IPAIPU3 now, as they should all be stored in the > >>> context if there's any relationship with the algorithms. > >> > >> yeah, we are on that path. > >> > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> > >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>>> --- > >>>> Changes in v2: > >>>> - Split into separate patch > >>>> - Prepped over "v4 ipa: ipu3: Misc clean up" > >>>> - Don't include max/min exposure/gain members in > >>>> IPASessionConfiguration, as they aren't used anywhere. > >>>> --- > >>>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------ > >>>> 1 file changed, 2 insertions(+), 24 deletions(-) > >>>> > >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >>>> index 17324616..4b6852a7 100644 > >>>> --- a/src/ipa/ipu3/ipu3.cpp > >>>> +++ b/src/ipa/ipu3/ipu3.cpp > >>>> @@ -167,11 +167,7 @@ private: > >>>> /* Camera sensor controls. */ > >>>> uint32_t defVBlank_; > >>>> uint32_t exposure_; > >>>> - uint32_t minExposure_; > >>>> - uint32_t maxExposure_; > >>>> uint32_t gain_; > >>>> - uint32_t minGain_; > >>>> - uint32_t maxGain_; > >>> What's required to get rid of gain_, exposure_ and defVBlank_ from here > >>> too? > >> > >> we can drop gain_, exposure_ and replace them with local vars, no issue > >> in that. > >> > >> defVBlank_ seems to be used IPAIPU3 class functions directly, but i > >> think we can move the defVBlank_ to JM's introduction of > >> IPASessionConfiguration.sensor structure > >> > >> With this plan i think we can drop these remaining private members from > >> IPAIPU3. > > Great, can easily be patches on top - doesn't need to be in this one. > > > Ok. > > I have been thinking to introduce similar control-not-found error > checks(as removed in this patch) in updateSessionConfiguration(), since > we ll end up querying the exposure, gain, vblank controls there /only/ . > Might as well introduce the control-not-found error checks there. Aha, I assumed the checks were already there. We should certainly have checks that the controls are available, otherwise it can crash when it tries to access them. I wonder if we can skip the checks if they are Mandatory Controls as defined by the CameraSensor class though ... -- Kieran > > > > > -- > > Kieran > > > > > >>> -- > >>> Kieran > >>> > >>> > >>>> > >>>> /* Interface to the Camera Helper */ > >>>> std::unique_ptr<CameraSensorHelper> camHelper_; > >>>> @@ -192,10 +188,12 @@ 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>(); > >>>> + 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>(); > >>>> + gain_ = minGain; > >>>> > >>>> /* > >>>> * When the AGC computes the new exposure values for a frame, it needs > >>>> @@ -429,32 +427,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>(); > >>>> > >>>> calculateBdsGrid(configInfo.bdsOutputSize); > >>>> -- > >>>> 2.31.1 > >>>>
Hi, On 3/3/22 19:52, Kieran Bingham wrote: > Quoting Umang Jain (2022-03-03 14:14:06) >> Hi, >> >> On 3/3/22 19:36, Kieran Bingham wrote: >>> Quoting Umang Jain (2022-03-03 12:15:44) >>>> Hi Kieran, >>>> >>>> On 3/3/22 17:17, Kieran Bingham wrote: >>>>> Quoting Umang Jain (2022-03-01 07:59:19) >>>>>> The exposure and gain limits are required for AGC configuration >>>>>> handled in IPAIPU3::updateSessionConfiguration(), which is happening >>>>>> already. Therefore the max/min private members in IPAIPU3 class for >>>>>> exposure/gain serve no use except setting initial values of exposure_ >>>>>> and gain_ members. >>>>>> >>>>>> Drop the max/min private members from IPAIPU3 class and set initial >>>>>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration(). >>>>> Great, even better to drop them if they aren't used. >>>>> >>>>> This looks like we're on the path to removing the private variables for >>>>> controls from the IPAIPU3 now, as they should all be stored in the >>>>> context if there's any relationship with the algorithms. >>>> yeah, we are on that path. >>>> >>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>> >>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>>>> --- >>>>>> Changes in v2: >>>>>> - Split into separate patch >>>>>> - Prepped over "v4 ipa: ipu3: Misc clean up" >>>>>> - Don't include max/min exposure/gain members in >>>>>> IPASessionConfiguration, as they aren't used anywhere. >>>>>> --- >>>>>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------ >>>>>> 1 file changed, 2 insertions(+), 24 deletions(-) >>>>>> >>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>>>> index 17324616..4b6852a7 100644 >>>>>> --- a/src/ipa/ipu3/ipu3.cpp >>>>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>>>> @@ -167,11 +167,7 @@ private: >>>>>> /* Camera sensor controls. */ >>>>>> uint32_t defVBlank_; >>>>>> uint32_t exposure_; >>>>>> - uint32_t minExposure_; >>>>>> - uint32_t maxExposure_; >>>>>> uint32_t gain_; >>>>>> - uint32_t minGain_; >>>>>> - uint32_t maxGain_; >>>>> What's required to get rid of gain_, exposure_ and defVBlank_ from here >>>>> too? >>>> we can drop gain_, exposure_ and replace them with local vars, no issue >>>> in that. >>>> >>>> defVBlank_ seems to be used IPAIPU3 class functions directly, but i >>>> think we can move the defVBlank_ to JM's introduction of >>>> IPASessionConfiguration.sensor structure >>>> >>>> With this plan i think we can drop these remaining private members from >>>> IPAIPU3. >>> Great, can easily be patches on top - doesn't need to be in this one. >> >> Ok. >> >> I have been thinking to introduce similar control-not-found error >> checks(as removed in this patch) in updateSessionConfiguration(), since >> we ll end up querying the exposure, gain, vblank controls there /only/ . >> Might as well introduce the control-not-found error checks there. > Aha, I assumed the checks were already there. > > We should certainly have checks that the controls are available, > otherwise it can crash when it tries to access them. > > I wonder if we can skip the checks if they are Mandatory Controls as > defined by the CameraSensor class though ... Good point, like on a general level throughout the code-base should we add individuals checks for the mandatory controls ? or the mandatory control check in CameraSensor is a good gate-keeper to rely on.. By the way, I checked that V4L2_CID_GAIN isn't mandatory ... so I guess atleast for that, we need to. > > -- > Kieran > > >>> -- >>> Kieran >>> >>> >>>>> -- >>>>> Kieran >>>>> >>>>> >>>>>> >>>>>> /* Interface to the Camera Helper */ >>>>>> std::unique_ptr<CameraSensorHelper> camHelper_; >>>>>> @@ -192,10 +188,12 @@ 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>(); >>>>>> + 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>(); >>>>>> + gain_ = minGain; >>>>>> >>>>>> /* >>>>>> * When the AGC computes the new exposure values for a frame, it needs >>>>>> @@ -429,32 +427,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>(); >>>>>> >>>>>> calculateBdsGrid(configInfo.bdsOutputSize); >>>>>> -- >>>>>> 2.31.1 >>>>>>
Quoting Umang Jain (2022-03-03 14:55:34) > Hi, > > >>>> On 3/3/22 17:17, Kieran Bingham wrote: > >>>>> Quoting Umang Jain (2022-03-01 07:59:19) > >>>>>> The exposure and gain limits are required for AGC configuration > >>>>>> handled in IPAIPU3::updateSessionConfiguration(), which is happening > >>>>>> already. Therefore the max/min private members in IPAIPU3 class for > >>>>>> exposure/gain serve no use except setting initial values of exposure_ > >>>>>> and gain_ members. > >>>>>> > >>>>>> Drop the max/min private members from IPAIPU3 class and set initial > >>>>>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration(). > >>>>> Great, even better to drop them if they aren't used. > >>>>> > >>>>> This looks like we're on the path to removing the private variables for > >>>>> controls from the IPAIPU3 now, as they should all be stored in the > >>>>> context if there's any relationship with the algorithms. > >>>> yeah, we are on that path. > >>>> > >>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>>> > >>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>>>>> --- > >>>>>> Changes in v2: > >>>>>> - Split into separate patch > >>>>>> - Prepped over "v4 ipa: ipu3: Misc clean up" > >>>>>> - Don't include max/min exposure/gain members in > >>>>>> IPASessionConfiguration, as they aren't used anywhere. > >>>>>> --- > >>>>>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------ > >>>>>> 1 file changed, 2 insertions(+), 24 deletions(-) > >>>>>> > >>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >>>>>> index 17324616..4b6852a7 100644 > >>>>>> --- a/src/ipa/ipu3/ipu3.cpp > >>>>>> +++ b/src/ipa/ipu3/ipu3.cpp > >>>>>> @@ -167,11 +167,7 @@ private: > >>>>>> /* Camera sensor controls. */ > >>>>>> uint32_t defVBlank_; > >>>>>> uint32_t exposure_; > >>>>>> - uint32_t minExposure_; > >>>>>> - uint32_t maxExposure_; > >>>>>> uint32_t gain_; > >>>>>> - uint32_t minGain_; > >>>>>> - uint32_t maxGain_; > >>>>> What's required to get rid of gain_, exposure_ and defVBlank_ from here > >>>>> too? > >>>> we can drop gain_, exposure_ and replace them with local vars, no issue > >>>> in that. > >>>> > >>>> defVBlank_ seems to be used IPAIPU3 class functions directly, but i > >>>> think we can move the defVBlank_ to JM's introduction of > >>>> IPASessionConfiguration.sensor structure > >>>> > >>>> With this plan i think we can drop these remaining private members from > >>>> IPAIPU3. > >>> Great, can easily be patches on top - doesn't need to be in this one. > >> > >> Ok. > >> > >> I have been thinking to introduce similar control-not-found error > >> checks(as removed in this patch) in updateSessionConfiguration(), since > >> we ll end up querying the exposure, gain, vblank controls there /only/ . > >> Might as well introduce the control-not-found error checks there. > > Aha, I assumed the checks were already there. > > > > We should certainly have checks that the controls are available, > > otherwise it can crash when it tries to access them. > > > > I wonder if we can skip the checks if they are Mandatory Controls as > > defined by the CameraSensor class though ... > > > Good point, like on a general level throughout the code-base should we > add individuals checks for the mandatory controls ? or the mandatory > control check in CameraSensor is a good gate-keeper to rely on.. > > By the way, I checked that V4L2_CID_GAIN isn't mandatory ... so I guess > atleast for that, we need to. Ahhh ... I think some sensors provide V4L2_CID_GAIN, and some provide V4L2_CID_ANALOGUE_GAIN ... They /should/ provide V4L2_CID_ANALOGUE_GAIN ... but it's not consistent. which makes me think this needs to be handled in the CameraSensor class, which makes me further feel that we need to move to using libcamera controls between the IPA and the pipeline handler sooner. Then the translation from libcamera to v4l2 control would happen under the CameraSensor class, and deal with preferring ANALOGUE_GAIN, but falling back to GAIN if it exists.... -- Kieran > > > > > -- > > Kieran > > > > > >>> -- > >>> Kieran > >>> > >>> > >>>>> -- > >>>>> Kieran > >>>>> > >>>>> > >>>>>> > >>>>>> /* Interface to the Camera Helper */ > >>>>>> std::unique_ptr<CameraSensorHelper> camHelper_; > >>>>>> @@ -192,10 +188,12 @@ 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>(); > >>>>>> + 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>(); > >>>>>> + gain_ = minGain; > >>>>>> > >>>>>> /* > >>>>>> * When the AGC computes the new exposure values for a frame, it needs > >>>>>> @@ -429,32 +427,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>(); > >>>>>> > >>>>>> calculateBdsGrid(configInfo.bdsOutputSize); > >>>>>> -- > >>>>>> 2.31.1 > >>>>>>
Hi, On 3/3/22 20:49, Kieran Bingham wrote: > Quoting Umang Jain (2022-03-03 14:55:34) >> Hi, >> >>>>>> On 3/3/22 17:17, Kieran Bingham wrote: >>>>>>> Quoting Umang Jain (2022-03-01 07:59:19) >>>>>>>> The exposure and gain limits are required for AGC configuration >>>>>>>> handled in IPAIPU3::updateSessionConfiguration(), which is happening >>>>>>>> already. Therefore the max/min private members in IPAIPU3 class for >>>>>>>> exposure/gain serve no use except setting initial values of exposure_ >>>>>>>> and gain_ members. >>>>>>>> >>>>>>>> Drop the max/min private members from IPAIPU3 class and set initial >>>>>>>> gain_ and exposure_ in IPAIPU3::updateSessionConfiguration(). >>>>>>> Great, even better to drop them if they aren't used. >>>>>>> >>>>>>> This looks like we're on the path to removing the private variables for >>>>>>> controls from the IPAIPU3 now, as they should all be stored in the >>>>>>> context if there's any relationship with the algorithms. >>>>>> yeah, we are on that path. >>>>>> >>>>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>>>> >>>>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>>>>>> --- >>>>>>>> Changes in v2: >>>>>>>> - Split into separate patch >>>>>>>> - Prepped over "v4 ipa: ipu3: Misc clean up" >>>>>>>> - Don't include max/min exposure/gain members in >>>>>>>> IPASessionConfiguration, as they aren't used anywhere. >>>>>>>> --- >>>>>>>> src/ipa/ipu3/ipu3.cpp | 26 ++------------------------ >>>>>>>> 1 file changed, 2 insertions(+), 24 deletions(-) >>>>>>>> >>>>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>>>>>> index 17324616..4b6852a7 100644 >>>>>>>> --- a/src/ipa/ipu3/ipu3.cpp >>>>>>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>>>>>> @@ -167,11 +167,7 @@ private: >>>>>>>> /* Camera sensor controls. */ >>>>>>>> uint32_t defVBlank_; >>>>>>>> uint32_t exposure_; >>>>>>>> - uint32_t minExposure_; >>>>>>>> - uint32_t maxExposure_; >>>>>>>> uint32_t gain_; >>>>>>>> - uint32_t minGain_; >>>>>>>> - uint32_t maxGain_; >>>>>>> What's required to get rid of gain_, exposure_ and defVBlank_ from here >>>>>>> too? >>>>>> we can drop gain_, exposure_ and replace them with local vars, no issue >>>>>> in that. >>>>>> >>>>>> defVBlank_ seems to be used IPAIPU3 class functions directly, but i >>>>>> think we can move the defVBlank_ to JM's introduction of >>>>>> IPASessionConfiguration.sensor structure >>>>>> >>>>>> With this plan i think we can drop these remaining private members from >>>>>> IPAIPU3. >>>>> Great, can easily be patches on top - doesn't need to be in this one. >>>> Ok. >>>> >>>> I have been thinking to introduce similar control-not-found error >>>> checks(as removed in this patch) in updateSessionConfiguration(), since >>>> we ll end up querying the exposure, gain, vblank controls there /only/ . >>>> Might as well introduce the control-not-found error checks there. >>> Aha, I assumed the checks were already there. >>> >>> We should certainly have checks that the controls are available, >>> otherwise it can crash when it tries to access them. >>> >>> I wonder if we can skip the checks if they are Mandatory Controls as >>> defined by the CameraSensor class though ... >> >> Good point, like on a general level throughout the code-base should we >> add individuals checks for the mandatory controls ? or the mandatory >> control check in CameraSensor is a good gate-keeper to rely on.. >> >> By the way, I checked that V4L2_CID_GAIN isn't mandatory ... so I guess >> atleast for that, we need to. > Ahhh ... I think some sensors provide V4L2_CID_GAIN, and some provide > V4L2_CID_ANALOGUE_GAIN ... > > They /should/ provide V4L2_CID_ANALOGUE_GAIN ... but it's not > consistent. which makes me think this needs to be handled in the > CameraSensor class, which makes me further feel that we need to move to > using libcamera controls between the IPA and the pipeline handler > sooner. Then the translation from libcamera to v4l2 control would happen > under the CameraSensor class, and deal with preferring ANALOGUE_GAIN, > but falling back to GAIN if it exists.... Further discussion needed for this maybe... i.e. the gains For earlier part of this thread, I added 2 patches to summarise on this parent patch itself. > > -- > Kieran > > >>> -- >>> Kieran >>> >>> >>>>> -- >>>>> Kieran >>>>> >>>>> >>>>>>> -- >>>>>>> Kieran >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> /* Interface to the Camera Helper */ >>>>>>>> std::unique_ptr<CameraSensorHelper> camHelper_; >>>>>>>> @@ -192,10 +188,12 @@ 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>(); >>>>>>>> + 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>(); >>>>>>>> + gain_ = minGain; >>>>>>>> >>>>>>>> /* >>>>>>>> * When the AGC computes the new exposure values for a frame, it needs >>>>>>>> @@ -429,32 +427,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>(); >>>>>>>> >>>>>>>> calculateBdsGrid(configInfo.bdsOutputSize); >>>>>>>> -- >>>>>>>> 2.31.1 >>>>>>>>
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 17324616..4b6852a7 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -167,11 +167,7 @@ private: /* Camera sensor controls. */ uint32_t defVBlank_; uint32_t exposure_; - uint32_t minExposure_; - uint32_t maxExposure_; uint32_t gain_; - uint32_t minGain_; - uint32_t maxGain_; /* Interface to the Camera Helper */ std::unique_ptr<CameraSensorHelper> camHelper_; @@ -192,10 +188,12 @@ 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>(); + 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>(); + gain_ = minGain; /* * When the AGC computes the new exposure values for a frame, it needs @@ -429,32 +427,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>(); calculateBdsGrid(configInfo.bdsOutputSize);
The exposure and gain limits are required for AGC configuration handled in IPAIPU3::updateSessionConfiguration(), which is happening already. Therefore the max/min private members in IPAIPU3 class for exposure/gain serve no use except setting initial values of exposure_ and gain_ members. Drop the max/min private members from IPAIPU3 class and set initial gain_ and exposure_ in IPAIPU3::updateSessionConfiguration(). Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- Changes in v2: - Split into separate patch - Prepped over "v4 ipa: ipu3: Misc clean up" - Don't include max/min exposure/gain members in IPASessionConfiguration, as they aren't used anywhere. --- src/ipa/ipu3/ipu3.cpp | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-)