[libcamera-devel] [PATCH v5 4/4] ipa: ipu3: Replace ipa::ipu3::algorithms::Ipu3AwbCell
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Thu Sep 9 15:32:15 CEST 2021
On 09/09/2021 12:59, Kieran Bingham wrote:
> On 09/09/2021 09:25, 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>
>> ---
>> 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 {
>
> Ahh now we have a new term too. A 'set' is a 'cell' ...
>
> Is this name from something external? or defined by us? (or the
> CrOS/Intel awb_public.h?)
>
This is defined in the CrOS/Intel awb_public.h file...
>> + 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));
>> +
Bingbu asked to change it from unsigned char to __u8...
>> /*
>> * 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 bc0aa6fe..5d6fa4d4 100644
>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>> @@ -201,17 +201,16 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>> uint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;
>
> Now that we're definitely talking about cells, I would rename
> regionWidth and regionHeight to be cellWidth and cellHeight to match the
> naming of the structure, and the fact that we already use cellPosition.
>
> (but do this in a separate patch on top).
>
>>
>> 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++;
>
> Like wise, here the awbRegionPosition is ... I presume the zonePosition?
>
> Can you refactor this to make it all consistent in the terminology please?
>
> (again, not in this patch, definitely on top).
Yes, it deserves its own patch, thanks.
>
> So for this patch which just moves the structure and naming accordingly:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>
>> - 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 {
>>
More information about the libcamera-devel
mailing list