[libcamera-devel] [PATCH 09/12] ipa: ipu3: awb: Use the line stride for the stats

[Jean-Michel Hautbois]

Hi Laurent,

On 29/09/2021 14:44, Laurent Pinchart wrote:
> Hello,
> 
> On Tue, Sep 28, 2021 at 05:02:42PM +0100, Kieran Bingham wrote:
>> On Thu, Sep 23, 2021 at 10:16:22AM +0200, Jean-Michel Hautbois wrote:
>>> The statistics buffer 'ipu3_uapi_awb_raw_buffer' stores the ImgU
>>> calculation results in a buffer aligned to a multiple of 4. The AWB loop
> 
> s/aligned to a multiple of 4/aligned horizontally to a multiple of 4 cells/
> 
>>> should take care of it to add the proper offset between lines and avoid
>>> any staircase effect.
>>>
>>> It is now no more required to pass the grid configuration context to the
>>
>> "It is no longer required to ..."
>>
>> That's a nice perk indeed.
>>
>>> private functions called from process() which simplifies the code flow.
>>>
>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>> ---
>>>  src/ipa/ipu3/algorithms/awb.cpp | 16 ++++++++--------
>>>  src/ipa/ipu3/algorithms/awb.h   |  7 +++----
>>>  2 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
>>> index 8a926691..a5391653 100644
>>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>>> @@ -172,8 +172,10 @@ int Awb::configure(IPAContext &context,
>>>  		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>>>  {
>>>  	const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;
>>> +	/* The grid is aligned to the next multiple of 4 */
> 
> s/grid/grid width/
> 
>>> +	stride_ = (grid.width + 3) / 4 * 4;
>>
>> Feels like something that could have been handled by an alignedUp macro.
>> We have that in geometry.h but for Size ...
>>
>> So perhaps we can just open code it (unless anyone knows of a
>> std:: macro that does this?)
> 
> utils::alignUp()
> 
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>> -	cellsPerZoneX_ = round(grid.width / static_cast<double>(kAwbStatsSizeX));
>>> +	cellsPerZoneX_ = round(stride_ / static_cast<double>(kAwbStatsSizeX));
> 
> This isn't right, you will end up using the padding cells in your
> calculations.
> 

The padding cell is all black, so it will not be taken into account in
Awb::generateZones() as the green value will be lower than
kMinGreenLevelInZone.

>>>  	cellsPerZoneY_ = round(grid.height / static_cast<double>(kAwbStatsSizeY));
>>>  
>>>  	/*
>>> @@ -234,8 +236,7 @@ void Awb::generateZones(std::vector<RGB> &zones)
>>>  }
>>>  
>>>  /* Translate the IPU3 statistics into the default statistics zone array */
>>> -void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>>> -			   const ipu3_uapi_grid_config &grid)
>>> +void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>>>  {
>>>  	/*
>>>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>>> @@ -243,7 +244,7 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>>>  	 */
>>>  	for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) {
>>>  		for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) {
>>> -			uint32_t cellPosition = (cellY * grid.width + cellX)
>>> +			uint32_t cellPosition = (cellY * stride_ + cellX)
>>>  					      * sizeof(Ipu3AwbCell);
>>>  			uint32_t zoneX = cellX / cellsPerZoneX_;
>>>  			uint32_t zoneY = cellY / cellsPerZoneY_;
>>> @@ -318,13 +319,12 @@ void Awb::awbGreyWorld()
>>>  	asyncResults_.blueGain = blueGain;
>>>  }
>>>  
>>> -void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,
>>> -			   const ipu3_uapi_grid_config &grid)
>>> +void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
>>>  {
>>>  	ASSERT(stats->stats_3a_status.awb_en);
>>>  	zones_.clear();
>>>  	clearAwbStats();
>>> -	generateAwbStats(stats, grid);
>>> +	generateAwbStats(stats);
>>>  	generateZones(zones_);
>>>  	LOG(IPU3Awb, Debug) << "Valid zones: " << zones_.size();
>>>  	if (zones_.size() > 10) {
>>> @@ -336,7 +336,7 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,
>>>  
>>>  void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>>>  {
>>> -	calculateWBGains(stats, context.configuration.grid.bdsGrid);
>>> +	calculateWBGains(stats);
>>>  
>>>  	/*
>>>  	 * Gains are only recalculated if enough zones were detected.
>>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
>>> index 681d8c2b..b3e0ad82 100644
>>> --- a/src/ipa/ipu3/algorithms/awb.h
>>> +++ b/src/ipa/ipu3/algorithms/awb.h
>>> @@ -74,11 +74,9 @@ public:
>>>  	};
>>>  
>>>  private:
>>> -	void calculateWBGains(const ipu3_uapi_stats_3a *stats,
>>> -			      const ipu3_uapi_grid_config &grid);
>>> +	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>>>  	void generateZones(std::vector<RGB> &zones);
>>> -	void generateAwbStats(const ipu3_uapi_stats_3a *stats,
>>> -			      const ipu3_uapi_grid_config &grid);
>>> +	void generateAwbStats(const ipu3_uapi_stats_3a *stats);
>>>  	void clearAwbStats();
>>>  	void awbGreyWorld();
>>>  	uint32_t estimateCCT(double red, double green, double blue);
>>> @@ -87,6 +85,7 @@ private:
>>>  	Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
>>>  	AwbStatus asyncResults_;
>>>  
>>> +	uint32_t stride_;
>>>  	uint32_t cellsPerZoneX_;
>>>  	uint32_t cellsPerZoneY_;
>>>  	uint32_t cellsPerZoneThreshold_;
>