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

Message ID 20210909082516.26055-5-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • Move and improve AWB structures
Related show

Commit Message

Jean-Michel Hautbois Sept. 9, 2021, 8:25 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>
---
 include/linux/intel-ipu3.h      | 28 ++++++++++++++++++++++++++--
 src/ipa/ipu3/algorithms/agc.cpp |  7 ++++---
 src/ipa/ipu3/algorithms/awb.cpp | 11 +++++------
 src/ipa/ipu3/algorithms/awb.h   | 10 ----------
 4 files changed, 35 insertions(+), 21 deletions(-)

Comments

Kieran Bingham Sept. 9, 2021, 10:59 a.m. UTC | #1 
On 09/09/2021 09:25, 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>
> ---
>  include/linux/intel-ipu3.h      | 28 ++++++++++++++++++++++++++--
>  src/ipa/ipu3/algorithms/agc.cpp |  7 ++++---
>  src/ipa/ipu3/algorithms/awb.cpp | 11 +++++------
>  src/ipa/ipu3/algorithms/awb.h   | 10 ----------
>  4 files changed, 35 insertions(+), 21 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 {

Ahh now we have a new term too. A 'set' is a 'cell' ...

Is this name from something external? or defined by us? (or the
CrOS/Intel awb_public.h?)

> +	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 5ff50f4a..8740dcdf 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -96,9 +96,10 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>  			 * We observed a bit shift which makes the value 160 to be 32 in the stats grid.
>  			 * Use the one passed at init time.
>  			 */
> -			if (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {
> -				uint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];
> -				uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];
> +			const ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[i + j * grid.width];
> +			if (currentCell->sat_ratio == 0) {
> +				uint8_t Gr = currentCell->Gr_avg;
> +				uint8_t Gb = currentCell->Gb_avg;
>  				hist[(Gr + Gb) / 2]++;
>  				count++;
>  			}
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index bc0aa6fe..5d6fa4d4 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -201,17 +201,16 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>  			uint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;

Now that we're definitely talking about cells, I would rename
regionWidth and regionHeight to be cellWidth and cellHeight to match the
naming of the structure, and the fact that we already use cellPosition.

(but do this in a separate patch on top).

>  
>  			uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
> -			cellPosition *= 8;
>  
>  			/* Cast the initial IPU3 structure to simplify the reading */
> -			Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
> -			if (currentCell->satRatio == 0) {
> +			const ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[cellPosition];
> +			if (currentCell->sat_ratio == 0) {
>  				/* The cell is not saturated, use the current cell */
>  				awbStats_[awbRegionPosition].counted++;

Like wise, here the awbRegionPosition is ... I presume the zonePosition?

Can you refactor this to make it all consistent in the terminology please?

 (again, not in this patch, definitely on top).

So for this patch which just moves the structure and naming accordingly:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



> -				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
> +				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
>  				awbStats_[awbRegionPosition].sum.green += greenValue / 2;
> -				awbStats_[awbRegionPosition].sum.red += currentCell->redAvg;
> -				awbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;
> +				awbStats_[awbRegionPosition].sum.red += currentCell->R_avg;
> +				awbStats_[awbRegionPosition].sum.blue += currentCell->B_avg;
>  			}
>  		}
>  	}
> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> index 3385ebe7..17905ae8 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. 9, 2021, 1:32 p.m. UTC | #2 
On 09/09/2021 12:59, Kieran Bingham wrote:
> On 09/09/2021 09:25, 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>
>> ---
>>  include/linux/intel-ipu3.h      | 28 ++++++++++++++++++++++++++--
>>  src/ipa/ipu3/algorithms/agc.cpp |  7 ++++---
>>  src/ipa/ipu3/algorithms/awb.cpp | 11 +++++------
>>  src/ipa/ipu3/algorithms/awb.h   | 10 ----------
>>  4 files changed, 35 insertions(+), 21 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 {
> 
> Ahh now we have a new term too. A 'set' is a 'cell' ...
> 
> Is this name from something external? or defined by us? (or the
> CrOS/Intel awb_public.h?)
> 

This is defined in the CrOS/Intel awb_public.h file...


>> +	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));
>> +

Bingbu asked to change it from unsigned char to __u8...

>>  /*
>>   * 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 5ff50f4a..8740dcdf 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -96,9 +96,10 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>>  			 * We observed a bit shift which makes the value 160 to be 32 in the stats grid.
>>  			 * Use the one passed at init time.
>>  			 */
>> -			if (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {
>> -				uint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];
>> -				uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];
>> +			const ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[i + j * grid.width];
>> +			if (currentCell->sat_ratio == 0) {
>> +				uint8_t Gr = currentCell->Gr_avg;
>> +				uint8_t Gb = currentCell->Gb_avg;
>>  				hist[(Gr + Gb) / 2]++;
>>  				count++;
>>  			}
>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
>> index bc0aa6fe..5d6fa4d4 100644
>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>> @@ -201,17 +201,16 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>>  			uint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;
> 
> Now that we're definitely talking about cells, I would rename
> regionWidth and regionHeight to be cellWidth and cellHeight to match the
> naming of the structure, and the fact that we already use cellPosition.
> 
> (but do this in a separate patch on top).
> 
>>  
>>  			uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
>> -			cellPosition *= 8;
>>  
>>  			/* Cast the initial IPU3 structure to simplify the reading */
>> -			Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
>> -			if (currentCell->satRatio == 0) {
>> +			const ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[cellPosition];
>> +			if (currentCell->sat_ratio == 0) {
>>  				/* The cell is not saturated, use the current cell */
>>  				awbStats_[awbRegionPosition].counted++;
> 
> Like wise, here the awbRegionPosition is ... I presume the zonePosition?
> 
> Can you refactor this to make it all consistent in the terminology please?
> 
>  (again, not in this patch, definitely on top).

Yes, it deserves its own patch, thanks.

> 
> So for this patch which just moves the structure and naming accordingly:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> 
>> -				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
>> +				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
>>  				awbStats_[awbRegionPosition].sum.green += greenValue / 2;
>> -				awbStats_[awbRegionPosition].sum.red += currentCell->redAvg;
>> -				awbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;
>> +				awbStats_[awbRegionPosition].sum.red += currentCell->R_avg;
>> +				awbStats_[awbRegionPosition].sum.blue += currentCell->B_avg;
>>  			}
>>  		}
>>  	}
>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
>> index 3385ebe7..17905ae8 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 5ff50f4a..8740dcdf 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -96,9 +96,10 @@  void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
 			 * We observed a bit shift which makes the value 160 to be 32 in the stats grid.
 			 * Use the one passed at init time.
 			 */
-			if (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {
-				uint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];
-				uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];
+			const ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[i + j * grid.width];
+			if (currentCell->sat_ratio == 0) {
+				uint8_t Gr = currentCell->Gr_avg;
+				uint8_t Gb = currentCell->Gb_avg;
 				hist[(Gr + Gb) / 2]++;
 				count++;
 			}
diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
index bc0aa6fe..5d6fa4d4 100644
--- a/src/ipa/ipu3/algorithms/awb.cpp
+++ b/src/ipa/ipu3/algorithms/awb.cpp
@@ -201,17 +201,16 @@  void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
 			uint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;
 
 			uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
-			cellPosition *= 8;
 
 			/* Cast the initial IPU3 structure to simplify the reading */
-			Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
-			if (currentCell->satRatio == 0) {
+			const ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[cellPosition];
+			if (currentCell->sat_ratio == 0) {
 				/* The cell is not saturated, use the current cell */
 				awbStats_[awbRegionPosition].counted++;
-				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
+				uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;
 				awbStats_[awbRegionPosition].sum.green += greenValue / 2;
-				awbStats_[awbRegionPosition].sum.red += currentCell->redAvg;
-				awbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;
+				awbStats_[awbRegionPosition].sum.red += currentCell->R_avg;
+				awbStats_[awbRegionPosition].sum.blue += currentCell->B_avg;
 			}
 		}
 	}
diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
index 3385ebe7..17905ae8 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 {