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

[Jean-Michel Hautbois]

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 ?

>> +
>>  	/* 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)
>