[libcamera-devel] [PATCH v2] ipa: ipu3: Consolidate querying of exposure and gain limits
Umang Jain
umang.jain at ideasonboard.com
Thu Mar 3 16:55:11 CET 2022
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 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....
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
>>>>>>>>
More information about the libcamera-devel
mailing list