[libcamera-devel] [PATCH 3/4] ipa: ipu3: Copy IPACameraSensorInfo for future usage

[Umang Jain]

Hi JM.

On 6/8/21 11:01 AM, Jean-Michel Hautbois wrote:
> Hi Laurent, Umang,
>
> On 08/06/2021 01:00, Laurent Pinchart wrote:
>> Hi Umang,
>>
>> Thank you for the patch.
>>
>> On Wed, Jun 02, 2021 at 03:53:25PM +0530, Umang Jain wrote:
>>> IPACameraSensorInfo members will be needed at various places in the
>>> IPAIPU3 class, in subsequent commits. Hence, it seems trivial to copy
>>> this structure for wider availability throughout the class.
>>>
>>> This commit does not introduce any functional changes.
>>>
>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>> ---
>>>   src/ipa/ipu3/ipu3.cpp | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>> index 2496b0a0..97ddb863 100644
>>> --- a/src/ipa/ipu3/ipu3.cpp
>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>> @@ -63,6 +63,8 @@ private:
>>>   
>>>   	ControlInfoMap ctrls_;
>>>   
>>> +	IPACameraSensorInfo sensorInfo_;
>> This is OK for now, but in the not too distant future I think we should
>> store fields individually as we should probably convert them. For
>> instance, the line duration should be converted to a time unit when we
>> receive it, and then used as such, instead of converting between pixels
>> and time in multiple places.
>>
>> Jean-Michel, how about moving IPU3Agc to handling exposure time in time
>> units, and doing the conversion in ipu3.cpp ? Is this something you have
>> considered, or maybe you're already planning it ?
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> I indeed have a local branch where exposure is a time unit and the line
> duration is calculated in the agcAlgo_->initialise() call.
> Those ctrls disappear from ipu3.cpp except for setting those in
> setControls().
> There is no need for a local variable in IPAIPU3 as IPU3Agc receives the
> configInfo reference directly.
> Would you prefer that new patch instead ? Or we can go with this one for
> the moment and I will fix it when I will post the new agc algorithm series ?


I plan to keep this as is for now assuming that the context of the 
rework, will be better when you post your agc algorithm series.

>
>>> +
>>>   	/* Camera sensor controls. */
>>>   	uint32_t exposure_;
>>>   	uint32_t minExposure_;
>>> @@ -144,6 +146,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)
>>>   	if (configInfo.entityControls.empty())
>>>   		return;
>>>   
>>> +	sensorInfo_ = configInfo.sensorInfo;
>>> +
>>>   	ctrls_ = configInfo.entityControls.at(0);
>>>   
>>>   	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>>> @@ -174,7 +178,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)
>>>   	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
>>>   
>>>   	agcAlgo_ = std::make_unique<IPU3Agc>();
>>> -	agcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo);
>>> +	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
>>>   }
>>>   
>>>   void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)