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

12738 diff mbox series

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@ideasonboard.com>
---
 src/ipa/ipu3/ipu3_agc.h    |  2 ++
 src/ipa/ipu3/ipu3_awb.h    | 11 ++-------
 src/ipa/ipu3/ipu3_common.h | 49 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 9 deletions(-)
 create mode 100644 src/ipa/ipu3/ipu3_common.h

Hi JM,

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@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"
> +
>  #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?


> +
> +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.


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

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@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.

> >  #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.

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 ?

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

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@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__ */
>>>
>