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

[Kieran Bingham]

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 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"
> +
>  #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__ */
>