[libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of exposure and gain limits

[Kieran Bingham]

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 at ideasonboard.com>
> >>>>>
> >>>>>> Signed-off-by: Umang Jain <umang.jain at 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
> >>>>>>