[libcamera-devel] [PATCH 12/12] ipa: ipu3: Replace ipa::ipu3::algorithms::Ipu3AwbCell
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 29 18:40:34 CEST 2021
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 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 | 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 {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list