[libcamera-devel,12/12] ipa: ipu3: Replace ipa::ipu3::algorithms::Ipu3AwbCell
diff mbox series

Message ID 20210923081625.60276-13-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • Improve ImgU statistics usage
Related show

Commit Message

Jean-Michel Hautbois Sept. 23, 2021, 8:16 a.m. UTC 
The intel-ipu3.h public interface from the kernel does not define how to
parse the statistics for a cell. This had to be identified by a process
of reverse engineering, and later identifying the structures from [0]
leading to our custom definition of struct Ipu3AwbCell.

[0]
https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h

To improve the kernel interface, a proposal has been made to the
linux-kernel [1] to incorporate the memory layout for each cell into the
intel-ipu3 header directly.

[1]
https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/

Update our local copy of the intel-ipu3.h to match the proposal and
change the AGC and AWB algorithms to reference that structure directly,
allowing us to remove the deprecated custom Ipu3AwbCell definition.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/linux/intel-ipu3.h      | 28 ++++++++++++++++++++--
 src/ipa/ipu3/algorithms/agc.cpp | 14 +++++------
 src/ipa/ipu3/algorithms/awb.cpp | 41 ++++++---------------------------
 src/ipa/ipu3/algorithms/awb.h   | 10 --------
 4 files changed, 39 insertions(+), 54 deletions(-)

Comments

Laurent Pinchart Sept. 29, 2021, 1:05 p.m. UTC | #1 
On Thu, Sep 23, 2021 at 10:16:25AM +0200, Jean-Michel Hautbois wrote:
> The intel-ipu3.h public interface from the kernel does not define how to
> parse the statistics for a cell. This had to be identified by a process
> of reverse engineering, and later identifying the structures from [0]
> leading to our custom definition of struct Ipu3AwbCell.
> 
> [0]
> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h
> 
> To improve the kernel interface, a proposal has been made to the
> linux-kernel [1] to incorporate the memory layout for each cell into the
> intel-ipu3 header directly.
> 
> [1]
> https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/
> 
> Update our local copy of the intel-ipu3.h to match the proposal and
> change the AGC and AWB algorithms to reference that structure directly,
> allowing us to remove the deprecated custom Ipu3AwbCell definition.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/linux/intel-ipu3.h      | 28 ++++++++++++++++++++--
>  src/ipa/ipu3/algorithms/agc.cpp | 14 +++++------
>  src/ipa/ipu3/algorithms/awb.cpp | 41 ++++++---------------------------
>  src/ipa/ipu3/algorithms/awb.h   | 10 --------
>  4 files changed, 39 insertions(+), 54 deletions(-)
> 
> diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h
> index ee0e6d0e..a9de8c11 100644
> --- a/include/linux/intel-ipu3.h
> +++ b/include/linux/intel-ipu3.h
> @@ -59,6 +59,29 @@ struct ipu3_uapi_grid_config {
>  	__u16 y_end;
>  } __attribute__((packed));
>  
> +/**
> + * struct ipu3_uapi_awb_set_item - Memory layout for each cell in AWB
> + *
> + * @Gr_avg:	Green average for red lines in the cell.
> + * @R_avg:	Red average in the cell.
> + * @B_avg:	Blue average in the cell.
> + * @Gb_avg:	Green average for blue lines in the cell.
> + * @sat_ratio:  Saturation ratio in the cell.
> + * @padding0:   Unused byte for padding.
> + * @padding1:   Unused byte for padding.
> + * @padding2:   Unused byte for padding.
> + */
> +struct ipu3_uapi_awb_set_item {
> +	unsigned char Gr_avg;
> +	unsigned char R_avg;
> +	unsigned char B_avg;
> +	unsigned char Gb_avg;
> +	unsigned char sat_ratio;
> +	unsigned char padding0;
> +	unsigned char padding1;
> +	unsigned char padding2;
> +} __attribute__((packed));
> +
>  /*
>   * The grid based data is divided into "slices" called set, each slice of setX
>   * refers to ipu3_uapi_grid_config width * height_per_slice.
> @@ -72,7 +95,8 @@ struct ipu3_uapi_grid_config {
>  	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
>  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
>  	(IPU3_UAPI_AWB_MAX_SETS * \
> -	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
> +	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES)) / \
> +	 sizeof(ipu3_uapi_awb_set_item)

Missing parentheses.

This doesn't match [1], is it intentional ?

>  
>  /**
> @@ -82,7 +106,7 @@ struct ipu3_uapi_grid_config {
>   *		the average values for each color channel.
>   */
>  struct ipu3_uapi_awb_raw_buffer {
> -	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> +	struct ipu3_uapi_awb_set_item meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
>  		__attribute__((aligned(32)));
>  } __attribute__((packed));
>  
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index ebbc5676..b04c0170 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -6,7 +6,6 @@
>   */
>  
>  #include "agc.h"
> -#include "awb.h"
>  
>  #include <algorithm>
>  #include <chrono>
> @@ -76,18 +75,17 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>  
>  	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
>  		for (unsigned int cellX = 0; cellX < stride_; cellX++) {
> -			uint32_t cellPosition = cellY * stride_ + cellX
> -					      * sizeof(Ipu3AwbCell);
> +			uint32_t cellPosition = cellY * stride_ + cellX;
>  
>  			/* Cast the initial IPU3 structure to simplify the reading */
> -			const Ipu3AwbCell *cell =
> -				reinterpret_cast<const Ipu3AwbCell *>(
> +			const ipu3_uapi_awb_set_item *cell =
> +				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>  					&stats->awb_raw_buffer.meta_data[cellPosition]
>  				);
>  
> -			if (cell->satRatio == 0) {
> -				uint8_t gr = cell->greenRedAvg;
> -				uint8_t gb = cell->greenBlueAvg;
> +			if (cell->sat_ratio == 0) {
> +				uint8_t gr = cell->Gr_avg;
> +				uint8_t gb = cell->Gb_avg;
>  				hist[(gr + gb) / 2]++;
>  			}
>  		}
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index 3013870b..06570b60 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -104,32 +104,6 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>   * \brief Gain calculated for the blue channel
>   */
>  
> -/**
> - * \struct Ipu3AwbCell
> - * \brief Memory layout for each cell in AWB metadata
> - *
> - * The Ipu3AwbCell structure is used to get individual values
> - * such as red average or saturation ratio in a particular cell.
> - *
> - * \var Ipu3AwbCell::greenRedAvg
> - * \brief Green average for red lines in the cell
> - *
> - * \var Ipu3AwbCell::redAvg
> - * \brief Red average in the cell
> - *
> - * \var Ipu3AwbCell::blueAvg
> - * \brief blue average in the cell
> - *
> - * \var Ipu3AwbCell::greenBlueAvg
> - * \brief Green average for blue lines
> - *
> - * \var Ipu3AwbCell::satRatio
> - * \brief Saturation ratio in the cell
> - *
> - * \var Ipu3AwbCell::padding
> - * \brief array of unused bytes for padding
> - */
> -
>  /* Default settings for Bayer noise reduction replicated from the Kernel */
>  static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
>  	.wb_gains = { 16, 16, 16, 16 },
> @@ -244,25 +218,24 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>  	 */
>  	for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) {
>  		for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) {
> -			uint32_t cellPosition = (cellY * stride_ + cellX)
> -					      * sizeof(Ipu3AwbCell);
> +			uint32_t cellPosition = cellY * stride_ + cellX;
>  			uint32_t zoneX = cellX / cellsPerZoneX_;
>  			uint32_t zoneY = cellY / cellsPerZoneY_;
>  
>  			uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX;
>  
>  			/* Cast the initial IPU3 structure to simplify the reading */
> -			const Ipu3AwbCell *currentCell =
> -				reinterpret_cast<const Ipu3AwbCell *>(
> +			const ipu3_uapi_awb_set_item *currentCell =
> +				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>  					&stats->awb_raw_buffer.meta_data[cellPosition]
>  				);
> -			if (currentCell->satRatio == 0) {
> +			if (currentCell->sat_ratio == 0) {
>  				/* The cell is not saturated, use the current cell */
>  				awbStats_[awbZonePosition].counted++;
> -				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
> +				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
>  				awbStats_[awbZonePosition].sum.green += greenValue / 2;
> -				awbStats_[awbZonePosition].sum.red += currentCell->redAvg;
> -				awbStats_[awbZonePosition].sum.blue += currentCell->blueAvg;
> +				awbStats_[awbZonePosition].sum.red += currentCell->R_avg;
> +				awbStats_[awbZonePosition].sum.blue += currentCell->B_avg;
>  			}
>  		}
>  	}
> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> index b3e0ad82..677384ed 100644
> --- a/src/ipa/ipu3/algorithms/awb.h
> +++ b/src/ipa/ipu3/algorithms/awb.h
> @@ -23,16 +23,6 @@ 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];
> -} __attribute__((packed));
> -
>  struct Accumulator {
>  	unsigned int counted;
>  	struct {
Jean-Michel Hautbois Sept. 29, 2021, 4:26 p.m. UTC | #2 
Hi Laurent,

On 29/09/2021 15:05, Laurent Pinchart wrote:
> On Thu, Sep 23, 2021 at 10:16:25AM +0200, Jean-Michel Hautbois wrote:
>> The intel-ipu3.h public interface from the kernel does not define how to
>> parse the statistics for a cell. This had to be identified by a process
>> of reverse engineering, and later identifying the structures from [0]
>> leading to our custom definition of struct Ipu3AwbCell.
>>
>> [0]
>> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h
>>
>> To improve the kernel interface, a proposal has been made to the
>> linux-kernel [1] to incorporate the memory layout for each cell into the
>> intel-ipu3 header directly.
>>
>> [1]
>> https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/
>>
>> Update our local copy of the intel-ipu3.h to match the proposal and
>> change the AGC and AWB algorithms to reference that structure directly,
>> allowing us to remove the deprecated custom Ipu3AwbCell definition.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>  include/linux/intel-ipu3.h      | 28 ++++++++++++++++++++--
>>  src/ipa/ipu3/algorithms/agc.cpp | 14 +++++------
>>  src/ipa/ipu3/algorithms/awb.cpp | 41 ++++++---------------------------
>>  src/ipa/ipu3/algorithms/awb.h   | 10 --------
>>  4 files changed, 39 insertions(+), 54 deletions(-)
>>
>> diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h
>> index ee0e6d0e..a9de8c11 100644
>> --- a/include/linux/intel-ipu3.h
>> +++ b/include/linux/intel-ipu3.h
>> @@ -59,6 +59,29 @@ struct ipu3_uapi_grid_config {
>>  	__u16 y_end;
>>  } __attribute__((packed));
>>  
>> +/**
>> + * struct ipu3_uapi_awb_set_item - Memory layout for each cell in AWB
>> + *
>> + * @Gr_avg:	Green average for red lines in the cell.
>> + * @R_avg:	Red average in the cell.
>> + * @B_avg:	Blue average in the cell.
>> + * @Gb_avg:	Green average for blue lines in the cell.
>> + * @sat_ratio:  Saturation ratio in the cell.
>> + * @padding0:   Unused byte for padding.
>> + * @padding1:   Unused byte for padding.
>> + * @padding2:   Unused byte for padding.
>> + */
>> +struct ipu3_uapi_awb_set_item {
>> +	unsigned char Gr_avg;
>> +	unsigned char R_avg;
>> +	unsigned char B_avg;
>> +	unsigned char Gb_avg;
>> +	unsigned char sat_ratio;
>> +	unsigned char padding0;
>> +	unsigned char padding1;
>> +	unsigned char padding2;
>> +} __attribute__((packed));
>> +
>>  /*
>>   * The grid based data is divided into "slices" called set, each slice of setX
>>   * refers to ipu3_uapi_grid_config width * height_per_slice.
>> @@ -72,7 +95,8 @@ struct ipu3_uapi_grid_config {
>>  	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
>>  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
>>  	(IPU3_UAPI_AWB_MAX_SETS * \
>> -	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
>> +	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES)) / \
>> +	 sizeof(ipu3_uapi_awb_set_item)
> 
> Missing parentheses.
> 
> This doesn't match [1], is it intentional ?
> 

It is not, I need to push this version instead of the one in [1]. I
should do that in a short notice ;-).

What parentheses are missing, I'm sorry, I can't see it...

>>  
>>  /**
>> @@ -82,7 +106,7 @@ struct ipu3_uapi_grid_config {
>>   *		the average values for each color channel.
>>   */
>>  struct ipu3_uapi_awb_raw_buffer {
>> -	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
>> +	struct ipu3_uapi_awb_set_item meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
>>  		__attribute__((aligned(32)));
>>  } __attribute__((packed));
>>  
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index ebbc5676..b04c0170 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -6,7 +6,6 @@
>>   */
>>  
>>  #include "agc.h"
>> -#include "awb.h"
>>  
>>  #include <algorithm>
>>  #include <chrono>
>> @@ -76,18 +75,17 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>>  
>>  	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
>>  		for (unsigned int cellX = 0; cellX < stride_; cellX++) {
>> -			uint32_t cellPosition = cellY * stride_ + cellX
>> -					      * sizeof(Ipu3AwbCell);
>> +			uint32_t cellPosition = cellY * stride_ + cellX;
>>  
>>  			/* Cast the initial IPU3 structure to simplify the reading */
>> -			const Ipu3AwbCell *cell =
>> -				reinterpret_cast<const Ipu3AwbCell *>(
>> +			const ipu3_uapi_awb_set_item *cell =
>> +				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>>  					&stats->awb_raw_buffer.meta_data[cellPosition]
>>  				);
>>  
>> -			if (cell->satRatio == 0) {
>> -				uint8_t gr = cell->greenRedAvg;
>> -				uint8_t gb = cell->greenBlueAvg;
>> +			if (cell->sat_ratio == 0) {
>> +				uint8_t gr = cell->Gr_avg;
>> +				uint8_t gb = cell->Gb_avg;
>>  				hist[(gr + gb) / 2]++;
>>  			}
>>  		}
>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
>> index 3013870b..06570b60 100644
>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>> @@ -104,32 +104,6 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>>   * \brief Gain calculated for the blue channel
>>   */
>>  
>> -/**
>> - * \struct Ipu3AwbCell
>> - * \brief Memory layout for each cell in AWB metadata
>> - *
>> - * The Ipu3AwbCell structure is used to get individual values
>> - * such as red average or saturation ratio in a particular cell.
>> - *
>> - * \var Ipu3AwbCell::greenRedAvg
>> - * \brief Green average for red lines in the cell
>> - *
>> - * \var Ipu3AwbCell::redAvg
>> - * \brief Red average in the cell
>> - *
>> - * \var Ipu3AwbCell::blueAvg
>> - * \brief blue average in the cell
>> - *
>> - * \var Ipu3AwbCell::greenBlueAvg
>> - * \brief Green average for blue lines
>> - *
>> - * \var Ipu3AwbCell::satRatio
>> - * \brief Saturation ratio in the cell
>> - *
>> - * \var Ipu3AwbCell::padding
>> - * \brief array of unused bytes for padding
>> - */
>> -
>>  /* Default settings for Bayer noise reduction replicated from the Kernel */
>>  static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
>>  	.wb_gains = { 16, 16, 16, 16 },
>> @@ -244,25 +218,24 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>>  	 */
>>  	for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) {
>>  		for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) {
>> -			uint32_t cellPosition = (cellY * stride_ + cellX)
>> -					      * sizeof(Ipu3AwbCell);
>> +			uint32_t cellPosition = cellY * stride_ + cellX;
>>  			uint32_t zoneX = cellX / cellsPerZoneX_;
>>  			uint32_t zoneY = cellY / cellsPerZoneY_;
>>  
>>  			uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX;
>>  
>>  			/* Cast the initial IPU3 structure to simplify the reading */
>> -			const Ipu3AwbCell *currentCell =
>> -				reinterpret_cast<const Ipu3AwbCell *>(
>> +			const ipu3_uapi_awb_set_item *currentCell =
>> +				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>>  					&stats->awb_raw_buffer.meta_data[cellPosition]
>>  				);
>> -			if (currentCell->satRatio == 0) {
>> +			if (currentCell->sat_ratio == 0) {
>>  				/* The cell is not saturated, use the current cell */
>>  				awbStats_[awbZonePosition].counted++;
>> -				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
>> +				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
>>  				awbStats_[awbZonePosition].sum.green += greenValue / 2;
>> -				awbStats_[awbZonePosition].sum.red += currentCell->redAvg;
>> -				awbStats_[awbZonePosition].sum.blue += currentCell->blueAvg;
>> +				awbStats_[awbZonePosition].sum.red += currentCell->R_avg;
>> +				awbStats_[awbZonePosition].sum.blue += currentCell->B_avg;
>>  			}
>>  		}
>>  	}
>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
>> index b3e0ad82..677384ed 100644
>> --- a/src/ipa/ipu3/algorithms/awb.h
>> +++ b/src/ipa/ipu3/algorithms/awb.h
>> @@ -23,16 +23,6 @@ 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];
>> -} __attribute__((packed));
>> -
>>  struct Accumulator {
>>  	unsigned int counted;
>>  	struct {
>
Laurent Pinchart Sept. 29, 2021, 4:40 p.m. UTC | #3 
On Wed, Sep 29, 2021 at 06:26:12PM +0200, Jean-Michel Hautbois wrote:
> On 29/09/2021 15:05, Laurent Pinchart wrote:
> > On Thu, Sep 23, 2021 at 10:16:25AM +0200, Jean-Michel Hautbois wrote:
> >> The intel-ipu3.h public interface from the kernel does not define how to
> >> parse the statistics for a cell. This had to be identified by a process
> >> of reverse engineering, and later identifying the structures from [0]
> >> leading to our custom definition of struct Ipu3AwbCell.
> >>
> >> [0]
> >> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h
> >>
> >> To improve the kernel interface, a proposal has been made to the
> >> linux-kernel [1] to incorporate the memory layout for each cell into the
> >> intel-ipu3 header directly.
> >>
> >> [1]
> >> https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/
> >>
> >> Update our local copy of the intel-ipu3.h to match the proposal and
> >> change the AGC and AWB algorithms to reference that structure directly,
> >> allowing us to remove the deprecated custom Ipu3AwbCell definition.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> ---
> >>  include/linux/intel-ipu3.h      | 28 ++++++++++++++++++++--
> >>  src/ipa/ipu3/algorithms/agc.cpp | 14 +++++------
> >>  src/ipa/ipu3/algorithms/awb.cpp | 41 ++++++---------------------------
> >>  src/ipa/ipu3/algorithms/awb.h   | 10 --------
> >>  4 files changed, 39 insertions(+), 54 deletions(-)
> >>
> >> diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h
> >> index ee0e6d0e..a9de8c11 100644
> >> --- a/include/linux/intel-ipu3.h
> >> +++ b/include/linux/intel-ipu3.h
> >> @@ -59,6 +59,29 @@ struct ipu3_uapi_grid_config {
> >>  	__u16 y_end;
> >>  } __attribute__((packed));
> >>  
> >> +/**
> >> + * struct ipu3_uapi_awb_set_item - Memory layout for each cell in AWB
> >> + *
> >> + * @Gr_avg:	Green average for red lines in the cell.
> >> + * @R_avg:	Red average in the cell.
> >> + * @B_avg:	Blue average in the cell.
> >> + * @Gb_avg:	Green average for blue lines in the cell.
> >> + * @sat_ratio:  Saturation ratio in the cell.
> >> + * @padding0:   Unused byte for padding.
> >> + * @padding1:   Unused byte for padding.
> >> + * @padding2:   Unused byte for padding.
> >> + */
> >> +struct ipu3_uapi_awb_set_item {
> >> +	unsigned char Gr_avg;
> >> +	unsigned char R_avg;
> >> +	unsigned char B_avg;
> >> +	unsigned char Gb_avg;
> >> +	unsigned char sat_ratio;
> >> +	unsigned char padding0;
> >> +	unsigned char padding1;
> >> +	unsigned char padding2;
> >> +} __attribute__((packed));
> >> +
> >>  /*
> >>   * The grid based data is divided into "slices" called set, each slice of setX
> >>   * refers to ipu3_uapi_grid_config width * height_per_slice.
> >> @@ -72,7 +95,8 @@ struct ipu3_uapi_grid_config {
> >>  	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
> >>  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
> >>  	(IPU3_UAPI_AWB_MAX_SETS * \
> >> -	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
> >> +	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES)) / \
> >> +	 sizeof(ipu3_uapi_awb_set_item)
> > 
> > Missing parentheses.
> > 
> > This doesn't match [1], is it intentional ?
> 
> It is not, I need to push this version instead of the one in [1]. I
> should do that in a short notice ;-).

Have you determined why the sizes are different ?

> What parentheses are missing, I'm sorry, I can't see it...

Outer parentheses around the whole expression.

> >>  
> >>  /**
> >> @@ -82,7 +106,7 @@ struct ipu3_uapi_grid_config {
> >>   *		the average values for each color channel.
> >>   */
> >>  struct ipu3_uapi_awb_raw_buffer {
> >> -	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> >> +	struct ipu3_uapi_awb_set_item meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> >>  		__attribute__((aligned(32)));
> >>  } __attribute__((packed));
> >>  
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> >> index ebbc5676..b04c0170 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >> @@ -6,7 +6,6 @@
> >>   */
> >>  
> >>  #include "agc.h"
> >> -#include "awb.h"
> >>  
> >>  #include <algorithm>
> >>  #include <chrono>
> >> @@ -76,18 +75,17 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
> >>  
> >>  	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> >>  		for (unsigned int cellX = 0; cellX < stride_; cellX++) {
> >> -			uint32_t cellPosition = cellY * stride_ + cellX
> >> -					      * sizeof(Ipu3AwbCell);
> >> +			uint32_t cellPosition = cellY * stride_ + cellX;
> >>  
> >>  			/* Cast the initial IPU3 structure to simplify the reading */
> >> -			const Ipu3AwbCell *cell =
> >> -				reinterpret_cast<const Ipu3AwbCell *>(
> >> +			const ipu3_uapi_awb_set_item *cell =
> >> +				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
> >>  					&stats->awb_raw_buffer.meta_data[cellPosition]
> >>  				);
> >>  
> >> -			if (cell->satRatio == 0) {
> >> -				uint8_t gr = cell->greenRedAvg;
> >> -				uint8_t gb = cell->greenBlueAvg;
> >> +			if (cell->sat_ratio == 0) {
> >> +				uint8_t gr = cell->Gr_avg;
> >> +				uint8_t gb = cell->Gb_avg;
> >>  				hist[(gr + gb) / 2]++;
> >>  			}
> >>  		}
> >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> >> index 3013870b..06570b60 100644
> >> --- a/src/ipa/ipu3/algorithms/awb.cpp
> >> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> >> @@ -104,32 +104,6 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
> >>   * \brief Gain calculated for the blue channel
> >>   */
> >>  
> >> -/**
> >> - * \struct Ipu3AwbCell
> >> - * \brief Memory layout for each cell in AWB metadata
> >> - *
> >> - * The Ipu3AwbCell structure is used to get individual values
> >> - * such as red average or saturation ratio in a particular cell.
> >> - *
> >> - * \var Ipu3AwbCell::greenRedAvg
> >> - * \brief Green average for red lines in the cell
> >> - *
> >> - * \var Ipu3AwbCell::redAvg
> >> - * \brief Red average in the cell
> >> - *
> >> - * \var Ipu3AwbCell::blueAvg
> >> - * \brief blue average in the cell
> >> - *
> >> - * \var Ipu3AwbCell::greenBlueAvg
> >> - * \brief Green average for blue lines
> >> - *
> >> - * \var Ipu3AwbCell::satRatio
> >> - * \brief Saturation ratio in the cell
> >> - *
> >> - * \var Ipu3AwbCell::padding
> >> - * \brief array of unused bytes for padding
> >> - */
> >> -
> >>  /* Default settings for Bayer noise reduction replicated from the Kernel */
> >>  static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
> >>  	.wb_gains = { 16, 16, 16, 16 },
> >> @@ -244,25 +218,24 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
> >>  	 */
> >>  	for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) {
> >>  		for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) {
> >> -			uint32_t cellPosition = (cellY * stride_ + cellX)
> >> -					      * sizeof(Ipu3AwbCell);
> >> +			uint32_t cellPosition = cellY * stride_ + cellX;
> >>  			uint32_t zoneX = cellX / cellsPerZoneX_;
> >>  			uint32_t zoneY = cellY / cellsPerZoneY_;
> >>  
> >>  			uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX;
> >>  
> >>  			/* Cast the initial IPU3 structure to simplify the reading */
> >> -			const Ipu3AwbCell *currentCell =
> >> -				reinterpret_cast<const Ipu3AwbCell *>(
> >> +			const ipu3_uapi_awb_set_item *currentCell =
> >> +				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
> >>  					&stats->awb_raw_buffer.meta_data[cellPosition]
> >>  				);
> >> -			if (currentCell->satRatio == 0) {
> >> +			if (currentCell->sat_ratio == 0) {
> >>  				/* The cell is not saturated, use the current cell */
> >>  				awbStats_[awbZonePosition].counted++;
> >> -				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
> >> +				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
> >>  				awbStats_[awbZonePosition].sum.green += greenValue / 2;
> >> -				awbStats_[awbZonePosition].sum.red += currentCell->redAvg;
> >> -				awbStats_[awbZonePosition].sum.blue += currentCell->blueAvg;
> >> +				awbStats_[awbZonePosition].sum.red += currentCell->R_avg;
> >> +				awbStats_[awbZonePosition].sum.blue += currentCell->B_avg;
> >>  			}
> >>  		}
> >>  	}
> >> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> >> index b3e0ad82..677384ed 100644
> >> --- a/src/ipa/ipu3/algorithms/awb.h
> >> +++ b/src/ipa/ipu3/algorithms/awb.h
> >> @@ -23,16 +23,6 @@ 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];
> >> -} __attribute__((packed));
> >> -
> >>  struct Accumulator {
> >>  	unsigned int counted;
> >>  	struct {

Patch
diff mbox series 

diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h
index ee0e6d0e..a9de8c11 100644
--- a/include/linux/intel-ipu3.h
+++ b/include/linux/intel-ipu3.h
@@ -59,6 +59,29 @@  struct ipu3_uapi_grid_config {
 	__u16 y_end;
 } __attribute__((packed));
 
+/**
+ * struct ipu3_uapi_awb_set_item - Memory layout for each cell in AWB
+ *
+ * @Gr_avg:	Green average for red lines in the cell.
+ * @R_avg:	Red average in the cell.
+ * @B_avg:	Blue average in the cell.
+ * @Gb_avg:	Green average for blue lines in the cell.
+ * @sat_ratio:  Saturation ratio in the cell.
+ * @padding0:   Unused byte for padding.
+ * @padding1:   Unused byte for padding.
+ * @padding2:   Unused byte for padding.
+ */
+struct ipu3_uapi_awb_set_item {
+	unsigned char Gr_avg;
+	unsigned char R_avg;
+	unsigned char B_avg;
+	unsigned char Gb_avg;
+	unsigned char sat_ratio;
+	unsigned char padding0;
+	unsigned char padding1;
+	unsigned char padding2;
+} __attribute__((packed));
+
 /*
  * The grid based data is divided into "slices" called set, each slice of setX
  * refers to ipu3_uapi_grid_config width * height_per_slice.
@@ -72,7 +95,8 @@  struct ipu3_uapi_grid_config {
 	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
 #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
 	(IPU3_UAPI_AWB_MAX_SETS * \
-	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
+	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES)) / \
+	 sizeof(ipu3_uapi_awb_set_item)
 
 
 /**
@@ -82,7 +106,7 @@  struct ipu3_uapi_grid_config {
  *		the average values for each color channel.
  */
 struct ipu3_uapi_awb_raw_buffer {
-	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
+	struct ipu3_uapi_awb_set_item meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
 		__attribute__((aligned(32)));
 } __attribute__((packed));
 
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index ebbc5676..b04c0170 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -6,7 +6,6 @@ 
  */
 
 #include "agc.h"
-#include "awb.h"
 
 #include <algorithm>
 #include <chrono>
@@ -76,18 +75,17 @@  void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
 
 	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
 		for (unsigned int cellX = 0; cellX < stride_; cellX++) {
-			uint32_t cellPosition = cellY * stride_ + cellX
-					      * sizeof(Ipu3AwbCell);
+			uint32_t cellPosition = cellY * stride_ + cellX;
 
 			/* Cast the initial IPU3 structure to simplify the reading */
-			const Ipu3AwbCell *cell =
-				reinterpret_cast<const Ipu3AwbCell *>(
+			const ipu3_uapi_awb_set_item *cell =
+				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
 					&stats->awb_raw_buffer.meta_data[cellPosition]
 				);
 
-			if (cell->satRatio == 0) {
-				uint8_t gr = cell->greenRedAvg;
-				uint8_t gb = cell->greenBlueAvg;
+			if (cell->sat_ratio == 0) {
+				uint8_t gr = cell->Gr_avg;
+				uint8_t gb = cell->Gb_avg;
 				hist[(gr + gb) / 2]++;
 			}
 		}
diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
index 3013870b..06570b60 100644
--- a/src/ipa/ipu3/algorithms/awb.cpp
+++ b/src/ipa/ipu3/algorithms/awb.cpp
@@ -104,32 +104,6 @@  static constexpr uint32_t kMinGreenLevelInZone = 32;
  * \brief Gain calculated for the blue channel
  */
 
-/**
- * \struct Ipu3AwbCell
- * \brief Memory layout for each cell in AWB metadata
- *
- * The Ipu3AwbCell structure is used to get individual values
- * such as red average or saturation ratio in a particular cell.
- *
- * \var Ipu3AwbCell::greenRedAvg
- * \brief Green average for red lines in the cell
- *
- * \var Ipu3AwbCell::redAvg
- * \brief Red average in the cell
- *
- * \var Ipu3AwbCell::blueAvg
- * \brief blue average in the cell
- *
- * \var Ipu3AwbCell::greenBlueAvg
- * \brief Green average for blue lines
- *
- * \var Ipu3AwbCell::satRatio
- * \brief Saturation ratio in the cell
- *
- * \var Ipu3AwbCell::padding
- * \brief array of unused bytes for padding
- */
-
 /* Default settings for Bayer noise reduction replicated from the Kernel */
 static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
 	.wb_gains = { 16, 16, 16, 16 },
@@ -244,25 +218,24 @@  void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
 	 */
 	for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) {
 		for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) {
-			uint32_t cellPosition = (cellY * stride_ + cellX)
-					      * sizeof(Ipu3AwbCell);
+			uint32_t cellPosition = cellY * stride_ + cellX;
 			uint32_t zoneX = cellX / cellsPerZoneX_;
 			uint32_t zoneY = cellY / cellsPerZoneY_;
 
 			uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX;
 
 			/* Cast the initial IPU3 structure to simplify the reading */
-			const Ipu3AwbCell *currentCell =
-				reinterpret_cast<const Ipu3AwbCell *>(
+			const ipu3_uapi_awb_set_item *currentCell =
+				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
 					&stats->awb_raw_buffer.meta_data[cellPosition]
 				);
-			if (currentCell->satRatio == 0) {
+			if (currentCell->sat_ratio == 0) {
 				/* The cell is not saturated, use the current cell */
 				awbStats_[awbZonePosition].counted++;
-				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
+				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
 				awbStats_[awbZonePosition].sum.green += greenValue / 2;
-				awbStats_[awbZonePosition].sum.red += currentCell->redAvg;
-				awbStats_[awbZonePosition].sum.blue += currentCell->blueAvg;
+				awbStats_[awbZonePosition].sum.red += currentCell->R_avg;
+				awbStats_[awbZonePosition].sum.blue += currentCell->B_avg;
 			}
 		}
 	}
diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
index b3e0ad82..677384ed 100644
--- a/src/ipa/ipu3/algorithms/awb.h
+++ b/src/ipa/ipu3/algorithms/awb.h
@@ -23,16 +23,6 @@  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];
-} __attribute__((packed));
-
 struct Accumulator {
 	unsigned int counted;
 	struct {