[libcamera-devel] [PATCH v1 3/7] ipa: ipu3: Use a common IPU3 header for the constants

[Jean-Michel Hautbois]

Hello Kieran, Laurent,

On 08/07/2021 14:55, Laurent Pinchart wrote:
> Hello,
> 
> On Wed, Jul 07, 2021 at 01:45:17PM +0100, Kieran Bingham wrote:
>> On 28/06/2021 21:22, Jean-Michel Hautbois wrote:
>>> Every constants used can be needed by both AWB and AGC (for the moment).
>>> Use a common header to simplify their usage.
>>>
>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>> ---
>>>  src/ipa/ipu3/ipu3_agc.h    |  2 ++
>>>  src/ipa/ipu3/ipu3_awb.h    | 11 ++-------
>>>  src/ipa/ipu3/ipu3_common.h | 49 ++++++++++++++++++++++++++++++++++++++
>>
>> the ipu3_ in ipu3/ipu3_common seems redundant, but we already have
>> ipu3_agc, ipu3_awb ...
>>
>>>  3 files changed, 53 insertions(+), 9 deletions(-)
>>>  create mode 100644 src/ipa/ipu3/ipu3_common.h
>>>
>>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
>>> index 6ca9af8e..774c8385 100644
>>> --- a/src/ipa/ipu3/ipu3_agc.h
>>> +++ b/src/ipa/ipu3/ipu3_agc.h
>>> @@ -7,6 +7,8 @@
>>>  #ifndef __LIBCAMERA_IPU3_AGC_H__
>>>  #define __LIBCAMERA_IPU3_AGC_H__
>>>  
>>> +#include "ipu3_common.h"
>>> +
> 
> This doesn't seem needed.
> 

It is used in the new AGC algorithm which is patch 7/7. The include
should then go there, you are right :-).

>>>  #include <array>
>>>  #include <unordered_map>
>>>  
>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>>> index 6ae111fd..f4100f4a 100644
>>> --- a/src/ipa/ipu3/ipu3_awb.h
>>> +++ b/src/ipa/ipu3/ipu3_awb.h
>>> @@ -7,6 +7,8 @@
>>>  #ifndef __LIBCAMERA_IPU3_AWB_H__
>>>  #define __LIBCAMERA_IPU3_AWB_H__
>>>  
>>> +#include "ipu3_common.h"
>>> +
>>>  #include <vector>
>>>  
>>>  #include <linux/intel-ipu3.h>
>>> @@ -34,15 +36,6 @@ public:
>>>  	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>>>  	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
>>>  
>>> -	struct Ipu3AwbCell {
>>> -		unsigned char greenRedAvg;
>>> -		unsigned char redAvg;
>>> -		unsigned char blueAvg;
>>> -		unsigned char greenBlueAvg;
>>> -		unsigned char satRatio;
>>> -		unsigned char padding[3];
>>> -	} __attribute__((packed));
>>> -
>>>  private:
>>>  	void generateZones(std::vector<RGB> &zones);
>>>  	void generateAwbStats(const ipu3_uapi_stats_3a *stats);
>>> diff --git a/src/ipa/ipu3/ipu3_common.h b/src/ipa/ipu3/ipu3_common.h
>>> new file mode 100644
>>> index 00000000..38ef49ce
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/ipu3_common.h
>>> @@ -0,0 +1,49 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2021, Ideas On Board
>>> + *
>>> + * isp.h - ISP statistics interface
>>
>> isp.h?
>>
>>> + */
>>> +#ifndef __LIBCAMERA_IPA_IPU3_COMMON_H__
>>> +#define __LIBCAMERA_IPA_IPU3_COMMON_H__
>>> +
>>> +#include <stdint.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +namespace ipa {
>>> +
>>> +/* Region size for the statistics generation algorithm */
>>> +static constexpr uint32_t kAwbStatsSizeX = 16;
>>> +static constexpr uint32_t kAwbStatsSizeY = 12;
>>> +static constexpr uint32_t kAwbStatsSize = kAwbStatsSizeX * kAwbStatsSizeY + 1;
>>> +
>>> +static constexpr uint32_t kAgcStatsSizeX = 7;
>>> +static constexpr uint32_t kAgcStatsSizeY = 5;
>>> +static constexpr uint32_t kAgcStatsSize = kAgcStatsSizeX * kAgcStatsSizeY + 1;
>>> +static constexpr uint32_t kNumAgcWeightedZones = 15;
>>> +static constexpr double kCenteredWeights[kNumAgcWeightedZones] = { 3, 3, 3, 2, 2, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0 };
>>> +static constexpr uint32_t kAgcStatsRegions[kAgcStatsSize] = {
>>> +	11, 9, 9, 9, 9, 9, 12,
>>> +	7, 5, 3, 3, 3, 6, 8,
>>> +	7, 5, 1, 0, 2, 6, 8,
>>> +	7, 5, 4, 4, 4, 6, 8,
>>> +	13, 10, 10, 10, 10, 10, 14
>>> +};
>>
>> What makes these all better in common? It's not clear yet why these will
>> be used elsewhere - but perhaps that's likely to come later in the series.
>>
>> If they are common, are they still specific to AwbStatsSize? Is this the
>> maximum - or a declaration of the size used?
>>
>> Are these structures used by the algorithms, or by the kernel specific
>> structures?
> 
> The constants should also be documented.
> 
>>> +
>>> +static constexpr uint32_t kMinGreenLevelInZone = 16;
>>> +
>>> +struct Ipu3AwbCell {
>>> +	unsigned char greenRedAvg;
>>> +	unsigned char redAvg;
>>> +	unsigned char blueAvg;
>>> +	unsigned char greenBlueAvg;
>>> +	unsigned char satRatio;
>>> +	unsigned char padding[3];
>>> +} __attribute__((packed));
>>
>> Seeing __attribute__((packed)) implies that this is used by a kernel
>> structure or hardware directly. Is that true?
>>
>> Is this defined in an existing interface file? or is this one of the
>> ones we had to discover ourselves.
>>
>> Either way, it's lacking some documentation to state what it references
>> and why the layout is important.

The structure is describing what is in the IPU3 stats structure for AWB.
I will document it when getting it back into ipu3_awb.cpp.
The __attribute__((packed)) is a "C" reflex and probably not useful here
(it is used to cast and move from one cell to another).

> 
> Also, as the structure seems to be AWB-specific, why isn't it better
> placed in ipu3_awb.h ?
> 
> Same for the constants above, shouldn't they go to ipu3_agc.h or
> ipu3_awb.h ? Or better, if they're only used in the implementation, in
> a .cpp file ?
> 

Part of the constants and structures are used by awb, others by agc.
It sounds a bit overkill to have them in a separate header indeed, so I
will just drop this patch, and get back to ipu3_agc.cpp/ipu3_awb.cpp
implementation.

>>> +
>>> +} /* namespace ipa */
>>> +
>>> +} /* namespace libcamera */
>>> +
>>> +#endif /* __LIBCAMERA_IPA_IPU3_COMMON_H__ */
>>>
>