[libcamera-devel] [PATCH] ipa: ipu3: Replace ipa::ipu3::algorithms::Ipu3AwbCell
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 2 20:52:33 CEST 2021
Hi Jean-Michel,
On Thu, Sep 02, 2021 at 06:08:00PM +0200, Jean-Michel Hautbois wrote:
> On 02/09/2021 17:46, Kieran Bingham wrote:
> > On 02/09/2021 14:29, Jean-Michel Hautbois wrote:
> >> This patch was proposed in:
> >> https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/
> >
> > That's not accurate, and it dives off to an external reference without
> > explaining why. /This/ patch is not proposed for linux-media.
> >
> >
> > Try to introduce your patches, and think about your commit message in
> > distinct sections such as:
> >
> > - Describe the issue
> > - Describe any context
> > - Describe solution
> >
> >
> > - the issue
> > """
> > 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
> > https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h
> > leading to our custom definition of struct Ipu3AwbCell.
> >
> > """
> >
> >
> > - context
> > """
> > To improve the kernel interface, a proposal has been made to the
> > linux-kernel [0] to incorporate the memory layout for each cell into the
> > intel-ipu3 header directly.
> >
> > [0]
> > https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/
> > """
> >
> >
> > - solution / What this patch does
> > """
> > 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.
> > """
> >
> > The only issue is that the proposal to linux-media has not yet been
> > reviewed or approved/integrated, so until then this patch would cause us
> > to diverge from the kernel version of the header and would require
> > special attention.
> >
> > That may not be an issue in this case, as we can track the progress
> > upstream - but normally we would expect to keep our linux headers
> > matching the kernel unmodified.
>
> Thanks a lot for this really better description, I sent a v2 with it.
If you really want to thank Kieran, try to send further patches with
commit messages that follow that structure :-) It will save him review
time, I'm sure he'll appreciate.
> I know we would normally keep the headers matching the kernel, but it
> could take some time before we can do that, can we afford it ?
Not an issue in this case, we can track the change as it progresses
upstream and update this header accordingly. The important part is that
we're trying to land this change upstream. If it wasn't submitted
upstream, or had no hope of getting merged there, it would be a
different story.
> >> It introduces the AWB metadata layout as a structure in the kernel
> >> header, to make it clear for userspace applications. As intel-ipu3.h now
> >> has a defined AWB layout, use that one instead of the
> >> ipa::ipu3::algorithms::Ipu3AwbCell.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >> ---
> >> include/linux/intel-ipu3.h | 38 +++++++++++++++++++++++++--------
> >> src/ipa/ipu3/algorithms/agc.cpp | 7 +++---
> >> src/ipa/ipu3/algorithms/awb.cpp | 10 ++++-----
> >> src/ipa/ipu3/algorithms/awb.h | 10 ---------
> >> 4 files changed, 38 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h
> >> index ee0e6d0e..0633e04e 100644
> >> --- a/include/linux/intel-ipu3.h
> >> +++ b/include/linux/intel-ipu3.h
> >> @@ -59,20 +59,40 @@ 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.
> >> */
> >> #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_SPARE_FOR_BUBBLES \
> >> - (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> >> - IPU3_UAPI_AWB_MD_ITEM_SIZE)
> >> +#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET 160
> >> +/* Based on max grid height + Spare for bubbles */
> >> +#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
> >> + (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))
> >> + AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET
> >>
> >>
> >> /**
> >> @@ -82,7 +102,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..36f648be 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 + 4 + 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 ea567f96..db0a22d3 100644
> >> --- a/src/ipa/ipu3/algorithms/awb.cpp
> >> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> >> @@ -207,14 +207,14 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
> >> 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 23f3e872..a9320b4c 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;
> >> unsigned int total;
> >>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list