Message ID | 20210923081625.60276-13-jeanmichel.hautbois@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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 {
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 { >
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 {
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 {