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

[Laurent Pinchart]

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.

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

-- 
Regards,

Laurent Pinchart