[libcamera-devel] [PATCH v2 12/12] ipa: ipu3: Replace ipa::ipu3::algorithms::Ipu3AwbCell
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 5 03:33:12 CEST 2021
Hi Jean-Michel,
Thank you for the patch.
On Thu, Sep 30, 2021 at 11:37:15AM +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/20210930092021.65741-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 at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/linux/intel-ipu3.h | 31 +++++++++++++++++++++----
> src/ipa/ipu3/algorithms/agc.cpp | 14 +++++------
> src/ipa/ipu3/algorithms/awb.cpp | 41 ++++++---------------------------
> src/ipa/ipu3/algorithms/awb.h | 10 --------
> 4 files changed, 40 insertions(+), 56 deletions(-)
>
> diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h
> index ee0e6d0e..a5a71ca8 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.
> @@ -71,9 +94,9 @@ struct ipu3_uapi_grid_config {
> (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> 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_MAX_SETS * \
> + (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES)) / \
> + sizeof(struct ipu3_uapi_awb_set_item))
I've reviewed the corresponding kernel patch, copying the comment here
as libcamera-devel wasn't CC'ed:
We'll really have to figure out what the bubbles are... Not in this
patch though.
Given that IPU3_UAPI_AWB_MD_ITEM_SIZE is equal to the size of one item,
how about this ?
diff --git a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
index ee0e6d0e4b2c..a18e9228ed07 100644
--- a/include/linux/intel-ipu3.h
+++ b/include/linux/intel-ipu3.h
@@ -65,11 +65,9 @@ struct ipu3_uapi_grid_config {
*/
#define IPU3_UAPI_AWB_MAX_SETS 60
/* Based on grid size 80 * 60 and cell size 16 x 16 */
-#define IPU3_UAPI_AWB_SET_SIZE 1280
-#define IPU3_UAPI_AWB_MD_ITEM_SIZE 8
+#define IPU3_UAPI_AWB_SET_SIZE 160
#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
- (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
- IPU3_UAPI_AWB_MD_ITEM_SIZE)
+ (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)
#define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
(IPU3_UAPI_AWB_MAX_SETS * \
(IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
> /**
> * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
> @@ -82,7 +105,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 46e01fc4..44008632 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>
> @@ -73,18 +72,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 < grid.width; 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 fcd1469f..4558f85a 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 },
> @@ -243,25 +217,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 {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list