[libcamera-devel,v1,1/7] ipa: ipu3: Move the AWB stats structures

13436 diff mbox series

The structure Ipu3AwbCell describes the AWB stats layout on the kernel
side. We will need it to be used by the AGC algorithm to be introduced
later, so let's make it visible from ipa::ipu3::algorithms and not only
for the AWB class.
This structure should probably go into the intel-ipu3.h file, whichs
means a kernel patch, let's keep it in mind for the moment.

The other structures RGB, IspStatsRegion and AwbStatus will also be used
elsewhere so move them at the same time.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/awb.h | 77 ++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 38 deletions(-)

Hi Jean-Michel,

Thank you for the patch.

On Mon, Aug 23, 2021 at 02:49:31PM +0200, Jean-Michel Hautbois wrote:
> The structure Ipu3AwbCell describes the AWB stats layout on the kernel
> side. We will need it to be used by the AGC algorithm to be introduced
> later, so let's make it visible from ipa::ipu3::algorithms and not only
> for the AWB class.

You need to either add a blank line here, or remove the line break.

> This structure should probably go into the intel-ipu3.h file, whichs
> means a kernel patch, let's keep it in mind for the moment.
> 
> The other structures RGB, IspStatsRegion and AwbStatus will also be used
> elsewhere so move them at the same time.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/awb.h | 77 ++++++++++++++++++-----------------
>  1 file changed, 39 insertions(+), 38 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> index a16dd68d..332652d0 100644
> --- a/src/ipa/ipu3/algorithms/awb.h
> +++ b/src/ipa/ipu3/algorithms/awb.h
> @@ -23,6 +23,45 @@ namespace ipa::ipu3::algorithms {
>  static constexpr uint32_t kAwbStatsSizeX = 16;
>  static constexpr uint32_t kAwbStatsSizeY = 12;
>  
> +/* \todo Move the cell layout into intel-ipu3.h kernel header */
> +struct Ipu3AwbCell {
> +	unsigned char greenRedAvg;
> +	unsigned char redAvg;
> +	unsigned char blueAvg;
> +	unsigned char greenBlueAvg;
> +	unsigned char satRatio;
> +	unsigned char padding[3];
> +};

Where's the packed attribute ?

> +
> +/* \todo Make these structs available to all the ISPs ? */
> +struct RGB {
> +	RGB(double _R = 0, double _G = 0, double _B = 0)
> +		: R(_R), G(_G), B(_B)
> +	{
> +	}
> +	double R, G, B;
> +	RGB &operator+=(RGB const &other)
> +	{
> +		R += other.R, G += other.G, B += other.B;
> +		return *this;
> +	}
> +};
> +
> +struct IspStatsRegion {
> +	unsigned int counted;
> +	unsigned int uncounted;
> +	unsigned long long rSum;
> +	unsigned long long gSum;
> +	unsigned long long bSum;
> +};
> +
> +struct AwbStatus {
> +	double temperatureK;
> +	double redGain;
> +	double greenGain;
> +	double blueGain;
> +};

Those are four different cases:

- Ipu3AwbCell describes the layout of statistics data produced by the
  ImgU, and should thus indeed be defined in intel-ipu3.h. I'm fine
  having it here for the time being.

- RGB is a more generic structure, it should be in a more generic
  header. It's however only used in the declaration of the
  AgcMetering::generateZones() function, which has no definition. You
  could thus leave it where it is for the time being.

- IspStatsRegion is also not AWB-specific, it doesn't belong to this
  header. It should also be renamed to a more explicit name.

- AwbStatus is only used outside of awb.c to store the red, green and
  blue gains in AgcMetering::awbStatus_, copying them from
  IPAFrameContext::awb::gains. There's no need to copy the data in
  AgcMetering::awbStatus_ in the first place, you could pass the
  IPAFrameContext to AgcMetering::computeInitialY() through
  AgcMetering::computeGain().

  This being said, if you think that AwbStatus in its current form would
  be a good model for AWB status regardless of the actual implementation
  of the algorithm (that is, if other parallel implementations of AWB in
  the future would use the same status structure), then the structure
  could be moved to ipa_context.h, and replace the awb field of
  IPAFrameContext. In that case I'd modigy AwbStatus as follows:

struct AwbStatus {
	double temperatureK;
	struct {
		double red;
		double green;
		double blue;
	} gains;
};

> +
>  class Awb : public Algorithm
>  {
>  public:
> @@ -32,44 +71,6 @@ public:
>  	void prepare(IPAContext &context, ipu3_uapi_params *params) override;
>  	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>  
> -	struct Ipu3AwbCell {
> -		unsigned char greenRedAvg;
> -		unsigned char redAvg;
> -		unsigned char blueAvg;
> -		unsigned char greenBlueAvg;
> -		unsigned char satRatio;
> -		unsigned char padding[3];
> -	} __attribute__((packed));
> -
> -	/* \todo Make these three structs available to all the ISPs ? */
> -	struct RGB {
> -		RGB(double _R = 0, double _G = 0, double _B = 0)
> -			: R(_R), G(_G), B(_B)
> -		{
> -		}
> -		double R, G, B;
> -		RGB &operator+=(RGB const &other)
> -		{
> -			R += other.R, G += other.G, B += other.B;
> -			return *this;
> -		}
> -	};
> -
> -	struct IspStatsRegion {
> -		unsigned int counted;
> -		unsigned int uncounted;
> -		unsigned long long rSum;
> -		unsigned long long gSum;
> -		unsigned long long bSum;
> -	};
> -
> -	struct AwbStatus {
> -		double temperatureK;
> -		double redGain;
> -		double greenGain;
> -		double blueGain;
> -	};
> -
>  private:
>  	void calculateWBGains(const ipu3_uapi_stats_3a *stats,
>  			      const ipu3_uapi_grid_config &grid);