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

Message ID 20210909135449.68017-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, 1:54 p.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>
---
 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

Laurent Pinchart Sept. 10, 2021, 3:20 p.m. UTC | #1
Hi Jean-Michel,

Thank you for the patch.

On Thu, Sep 09, 2021 at 03:54:48PM +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 |  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 {
> +	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 6bf114ac..cf97208f 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -236,17 +236,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 {

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 6bf114ac..cf97208f 100644
--- a/src/ipa/ipu3/algorithms/awb.cpp
+++ b/src/ipa/ipu3/algorithms/awb.cpp
@@ -236,17 +236,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 {