[{"id":19299,"web_url":"https://patchwork.libcamera.org/comment/19299/","msgid":"<1cf179cb-03ae-a779-e360-edc807a66509@ideasonboard.com>","date":"2021-09-02T15:46:49","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Replace\n\tipa::ipu3::algorithms::Ipu3AwbCell","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 02/09/2021 14:29, Jean-Michel Hautbois wrote:\n> This patch was proposed in:\n> https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/\n\nThat's not accurate, and it dives off to an external reference without\nexplaining why. /This/ patch is not proposed for linux-media.\n\n\nTry to introduce your patches, and think about your commit message in\ndistinct sections such as:\n\n - Describe the issue\n - Describe any context\n - Describe solution\n\n\n - the issue\n\"\"\"\nThe intel-ipu3.h public interface from the kernel does not define how to\nparse the statistics for a cell. This had to be identified by a process\nof reverse engineering, and later identifying the structures from\nhttps://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h\nleading to our custom definition of struct Ipu3AwbCell.\n\n\"\"\"\n\n\n - context\n\"\"\"\nTo improve the kernel interface, a proposal has been made to the\nlinux-kernel [0] to incorporate the memory layout for each cell into the\nintel-ipu3 header directly.\n\n[0]\nhttps://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/\n\"\"\"\n\n\n - solution / What this patch does\n\"\"\"\nUpdate our local copy of the intel-ipu3.h to match the proposal and\nchange the AGC and AWB algorithms to reference that structure directly,\nallowing us to remove the deprecated custom Ipu3AwbCell definition.\n\"\"\"\n\nThe only issue is that the proposal to linux-media has not yet been\nreviewed or approved/integrated, so until then this patch would cause us\nto diverge from the kernel version of the header and would require\nspecial attention.\n\nThat may not be an issue in this case, as we can track the progress\nupstream - but normally we would expect to keep our linux headers\nmatching the kernel unmodified.\n\n\n\n\n> It introduces the AWB metadata layout as a structure in the kernel\n> header, to make it clear for userspace applications. As intel-ipu3.h now\n> has a defined AWB layout, use that one instead of the\n> ipa::ipu3::algorithms::Ipu3AwbCell.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> include/linux/intel-ipu3.h | 38 +++++++++++++++++++++++++--------\n> src/ipa/ipu3/algorithms/agc.cpp | 7 +++---\n> src/ipa/ipu3/algorithms/awb.cpp | 10 ++++-----\n> src/ipa/ipu3/algorithms/awb.h | 10 ---------\n> 4 files changed, 38 insertions(+), 27 deletions(-)\n> \n> diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h\n> index ee0e6d0e..0633e04e 100644\n> --- a/include/linux/intel-ipu3.h\n> +++ b/include/linux/intel-ipu3.h\n> @@ -59,20 +59,40 @@ struct ipu3_uapi_grid_config {\n> \t__u16 y_end;\n> } __attribute__((packed));\n> \n> +/**\n> + * struct ipu3_uapi_awb_set_item - Memory layout for each cell in AWB\n> + *\n> + * @Gr_avg:\tGreen average for red lines in the cell.\n> + * @R_avg:\tRed average in the cell.\n> + * @B_avg:\tBlue average in the cell.\n> + * @Gb_avg:\tGreen average for blue lines in the cell.\n> + * @sat_ratio: Saturation ratio in the cell.\n> + * @padding0: Unused byte for padding.\n> + * @padding1: Unused byte for padding.\n> + * @padding2: Unused byte for padding.\n> + */\n> +struct ipu3_uapi_awb_set_item {\n> +\tunsigned char Gr_avg;\n> +\tunsigned char R_avg;\n> +\tunsigned char B_avg;\n> +\tunsigned char Gb_avg;\n> +\tunsigned char sat_ratio;\n> +\tunsigned char padding0;\n> +\tunsigned char padding1;\n> +\tunsigned char padding2;\n> +} __attribute__((packed));\n> +\n> /*\n> * The grid based data is divided into \"slices\" called set, each slice of setX\n> * refers to ipu3_uapi_grid_config width * height_per_slice.\n> */\n> #define IPU3_UAPI_AWB_MAX_SETS\t\t\t\t60\n> -/* Based on grid size 80 * 60 and cell size 16 x 16 */\n> -#define IPU3_UAPI_AWB_SET_SIZE\t\t\t\t1280\n> -#define IPU3_UAPI_AWB_MD_ITEM_SIZE\t\t\t8\n> -#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \\\n> -\t(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \\\n> -\t IPU3_UAPI_AWB_MD_ITEM_SIZE)\n> +#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET\t\t\t160\n> +/* Based on max grid height + Spare for bubbles */\n> +#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \\\n> +\t(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)\n> #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \\\n> -\t(IPU3_UAPI_AWB_MAX_SETS * \\\n> -\t (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))\n> + AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET\n> \n> \n> /**\n> @@ -82,7 +102,7 @@ struct ipu3_uapi_grid_config {\n> *\t\tthe average values for each color channel.\n> */\n> struct ipu3_uapi_awb_raw_buffer {\n> -\t__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]\n> +\tstruct ipu3_uapi_awb_set_item meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]\n> \t\t__attribute__((aligned(32)));\n> } __attribute__((packed));\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 5ff50f4a..36f648be 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -96,9 +96,10 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\t\t * We observed a bit shift which makes the value 160 to be 32 in the stats grid.\n> \t\t\t * Use the one passed at init time.\n> \t\t\t */\n> -\t\t\tif (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {\n> -\t\t\t\tuint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];\n> -\t\t\t\tuint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];\n> +\t\t\tconst ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width];\n> +\t\t\tif (currentCell->sat_ratio == 0) {\n> +\t\t\t\tuint8_t Gr = currentCell->Gr_avg;\n> +\t\t\t\tuint8_t Gb = currentCell->Gb_avg;\n> \t\t\t\thist[(Gr + Gb) / 2]++;\n> \t\t\t\tcount++;\n> \t\t\t}\n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index ea567f96..db0a22d3 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -207,14 +207,14 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> \t\t\tcellPosition *= 8;\n> \n> \t\t\t/* Cast the initial IPU3 structure to simplify the reading */\n> -\t\t\tIpu3AwbCell *currentCell = reinterpret_cast(const_cast(&stats->awb_raw_buffer.meta_data[cellPosition]));\n> -\t\t\tif (currentCell->satRatio == 0) {\n> +\t\t\tconst ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[cellPosition];\n> +\t\t\tif (currentCell->sat_ratio == 0) {\n> \t\t\t\t/* The cell is not saturated, use the current cell */\n> \t\t\t\tawbStats_[awbRegionPosition].counted++;\n> -\t\t\t\tuint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;\n> +\t\t\t\tuint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;\n> \t\t\t\tawbStats_[awbRegionPosition].sum.green += greenValue / 2;\n> -\t\t\t\tawbStats_[awbRegionPosition].sum.red += currentCell->redAvg;\n> -\t\t\t\tawbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;\n> +\t\t\t\tawbStats_[awbRegionPosition].sum.red += currentCell->R_avg;\n> +\t\t\t\tawbStats_[awbRegionPosition].sum.blue += currentCell->B_avg;\n> \t\t\t}\n> \t\t}\n> \t}\n> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> index 23f3e872..a9320b4c 100644\n> --- a/src/ipa/ipu3/algorithms/awb.h\n> +++ b/src/ipa/ipu3/algorithms/awb.h\n> @@ -23,16 +23,6 @@ namespace ipa::ipu3::algorithms {\n> static constexpr uint32_t kAwbStatsSizeX = 16;\n> static constexpr uint32_t kAwbStatsSizeY = 12;\n> \n> -/* \\todo Move the cell layout into intel-ipu3.h kernel header */\n> -struct Ipu3AwbCell {\n> -\tunsigned char greenRedAvg;\n> -\tunsigned char redAvg;\n> -\tunsigned char blueAvg;\n> -\tunsigned char greenBlueAvg;\n> -\tunsigned char satRatio;\n> -\tunsigned char padding[3];\n> -} __attribute__((packed));\n> -\n> struct Accumulator {\n> \tunsigned int counted;\n> \tunsigned int total;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 56E12BD87D\n\tfor ;\n\tThu, 2 Sep 2021 15:46:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A13EF69166;\n\tThu, 2 Sep 2021 17:46:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6427A60255\n\tfor ;\n\tThu, 2 Sep 2021 17:46:52 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DA56545E;\n\tThu, 2 Sep 2021 17:46:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"R5L0bmHV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630597612;\n\tbh=9hertrFz2XsT3xgVe4wqiPpfOzH5PxNFKsUYgHNkCF4=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=R5L0bmHVw3d2xNRpA1BiC2jIF1L7Z4dTtNtO98ZDZPcUVbze4POB30C5clvBLKnzM\n\trTitsZmjoLvvZbgmoGHdR+8PzESrCQMHi63coOwzv6MWdlt5kX2Tn7Rrni1Vk+KJhL\n\t5GvJChi2jd1RVAsIG7cyxYMi4TemPEWbwpTh3Y9s=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210902132930.76137-1-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<1cf179cb-03ae-a779-e360-edc807a66509@ideasonboard.com>","Date":"Thu, 2 Sep 2021 16:46:49 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210902132930.76137-1-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Replace\n\tipa::ipu3::algorithms::Ipu3AwbCell","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19300,"web_url":"https://patchwork.libcamera.org/comment/19300/","msgid":"<773ca75c-41e1-b283-9a85-6629a8db9dcf@ideasonboard.com>","date":"2021-09-02T16:08:00","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Replace\n\tipa::ipu3::algorithms::Ipu3AwbCell","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 02/09/2021 17:46, Kieran Bingham wrote:\n> On 02/09/2021 14:29, Jean-Michel Hautbois wrote:\n>> This patch was proposed in:\n>> https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/\n> \n> That's not accurate, and it dives off to an external reference without\n> explaining why. /This/ patch is not proposed for linux-media.\n> \n> \n> Try to introduce your patches, and think about your commit message in\n> distinct sections such as:\n> \n> - Describe the issue\n> - Describe any context\n> - Describe solution\n> \n> \n> - the issue\n> \"\"\"\n> The intel-ipu3.h public interface from the kernel does not define how to\n> parse the statistics for a cell. This had to be identified by a process\n> of reverse engineering, and later identifying the structures from\n> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h\n> leading to our custom definition of struct Ipu3AwbCell.\n> \n> \"\"\"\n> \n> \n> - context\n> \"\"\"\n> To improve the kernel interface, a proposal has been made to the\n> linux-kernel [0] to incorporate the memory layout for each cell into the\n> intel-ipu3 header directly.\n> \n> [0]\n> https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/\n> \"\"\"\n> \n> \n> - solution / What this patch does\n> \"\"\"\n> Update our local copy of the intel-ipu3.h to match the proposal and\n> change the AGC and AWB algorithms to reference that structure directly,\n> allowing us to remove the deprecated custom Ipu3AwbCell definition.\n> \"\"\"\n> \n> The only issue is that the proposal to linux-media has not yet been\n> reviewed or approved/integrated, so until then this patch would cause us\n> to diverge from the kernel version of the header and would require\n> special attention.\n> \n> That may not be an issue in this case, as we can track the progress\n> upstream - but normally we would expect to keep our linux headers\n> matching the kernel unmodified.\n> \n\nThanks a lot for this really better description, I sent a v2 with it.\nI know we would normally keep the headers matching the kernel, but it\ncould take some time before we can do that, can we afford it ?\n\n> \n> \n> \n>> It introduces the AWB metadata layout as a structure in the kernel\n>> header, to make it clear for userspace applications. As intel-ipu3.h now\n>> has a defined AWB layout, use that one instead of the\n>> ipa::ipu3::algorithms::Ipu3AwbCell.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> include/linux/intel-ipu3.h | 38 +++++++++++++++++++++++++--------\n>> src/ipa/ipu3/algorithms/agc.cpp | 7 +++---\n>> src/ipa/ipu3/algorithms/awb.cpp | 10 ++++-----\n>> src/ipa/ipu3/algorithms/awb.h | 10 ---------\n>> 4 files changed, 38 insertions(+), 27 deletions(-)\n>>\n>> diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h\n>> index ee0e6d0e..0633e04e 100644\n>> --- a/include/linux/intel-ipu3.h\n>> +++ b/include/linux/intel-ipu3.h\n>> @@ -59,20 +59,40 @@ struct ipu3_uapi_grid_config {\n>> \t__u16 y_end;\n>> } __attribute__((packed));\n>> \n>> +/**\n>> + * struct ipu3_uapi_awb_set_item - Memory layout for each cell in AWB\n>> + *\n>> + * @Gr_avg:\tGreen average for red lines in the cell.\n>> + * @R_avg:\tRed average in the cell.\n>> + * @B_avg:\tBlue average in the cell.\n>> + * @Gb_avg:\tGreen average for blue lines in the cell.\n>> + * @sat_ratio: Saturation ratio in the cell.\n>> + * @padding0: Unused byte for padding.\n>> + * @padding1: Unused byte for padding.\n>> + * @padding2: Unused byte for padding.\n>> + */\n>> +struct ipu3_uapi_awb_set_item {\n>> +\tunsigned char Gr_avg;\n>> +\tunsigned char R_avg;\n>> +\tunsigned char B_avg;\n>> +\tunsigned char Gb_avg;\n>> +\tunsigned char sat_ratio;\n>> +\tunsigned char padding0;\n>> +\tunsigned char padding1;\n>> +\tunsigned char padding2;\n>> +} __attribute__((packed));\n>> +\n>> /*\n>> * The grid based data is divided into \"slices\" called set, each slice of setX\n>> * refers to ipu3_uapi_grid_config width * height_per_slice.\n>> */\n>> #define IPU3_UAPI_AWB_MAX_SETS\t\t\t\t60\n>> -/* Based on grid size 80 * 60 and cell size 16 x 16 */\n>> -#define IPU3_UAPI_AWB_SET_SIZE\t\t\t\t1280\n>> -#define IPU3_UAPI_AWB_MD_ITEM_SIZE\t\t\t8\n>> -#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \\\n>> -\t(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \\\n>> -\t IPU3_UAPI_AWB_MD_ITEM_SIZE)\n>> +#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET\t\t\t160\n>> +/* Based on max grid height + Spare for bubbles */\n>> +#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \\\n>> +\t(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)\n>> #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \\\n>> -\t(IPU3_UAPI_AWB_MAX_SETS * \\\n>> -\t (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))\n>> + AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET\n>> \n>> \n>> /**\n>> @@ -82,7 +102,7 @@ struct ipu3_uapi_grid_config {\n>> *\t\tthe average values for each color channel.\n>> */\n>> struct ipu3_uapi_awb_raw_buffer {\n>> -\t__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]\n>> +\tstruct ipu3_uapi_awb_set_item meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]\n>> \t\t__attribute__((aligned(32)));\n>> } __attribute__((packed));\n>> \n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 5ff50f4a..36f648be 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -96,9 +96,10 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>> \t\t\t * We observed a bit shift which makes the value 160 to be 32 in the stats grid.\n>> \t\t\t * Use the one passed at init time.\n>> \t\t\t */\n>> -\t\t\tif (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {\n>> -\t\t\t\tuint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];\n>> -\t\t\t\tuint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];\n>> +\t\t\tconst ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width];\n>> +\t\t\tif (currentCell->sat_ratio == 0) {\n>> +\t\t\t\tuint8_t Gr = currentCell->Gr_avg;\n>> +\t\t\t\tuint8_t Gb = currentCell->Gb_avg;\n>> \t\t\t\thist[(Gr + Gb) / 2]++;\n>> \t\t\t\tcount++;\n>> \t\t\t}\n>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n>> index ea567f96..db0a22d3 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.cpp\n>> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n>> @@ -207,14 +207,14 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n>> \t\t\tcellPosition *= 8;\n>> \n>> \t\t\t/* Cast the initial IPU3 structure to simplify the reading */\n>> -\t\t\tIpu3AwbCell *currentCell = reinterpret_cast(const_cast(&stats->awb_raw_buffer.meta_data[cellPosition]));\n>> -\t\t\tif (currentCell->satRatio == 0) {\n>> +\t\t\tconst ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[cellPosition];\n>> +\t\t\tif (currentCell->sat_ratio == 0) {\n>> \t\t\t\t/* The cell is not saturated, use the current cell */\n>> \t\t\t\tawbStats_[awbRegionPosition].counted++;\n>> -\t\t\t\tuint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;\n>> +\t\t\t\tuint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;\n>> \t\t\t\tawbStats_[awbRegionPosition].sum.green += greenValue / 2;\n>> -\t\t\t\tawbStats_[awbRegionPosition].sum.red += currentCell->redAvg;\n>> -\t\t\t\tawbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;\n>> +\t\t\t\tawbStats_[awbRegionPosition].sum.red += currentCell->R_avg;\n>> +\t\t\t\tawbStats_[awbRegionPosition].sum.blue += currentCell->B_avg;\n>> \t\t\t}\n>> \t\t}\n>> \t}\n>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n>> index 23f3e872..a9320b4c 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.h\n>> +++ b/src/ipa/ipu3/algorithms/awb.h\n>> @@ -23,16 +23,6 @@ namespace ipa::ipu3::algorithms {\n>> static constexpr uint32_t kAwbStatsSizeX = 16;\n>> static constexpr uint32_t kAwbStatsSizeY = 12;\n>> \n>> -/* \\todo Move the cell layout into intel-ipu3.h kernel header */\n>> -struct Ipu3AwbCell {\n>> -\tunsigned char greenRedAvg;\n>> -\tunsigned char redAvg;\n>> -\tunsigned char blueAvg;\n>> -\tunsigned char greenBlueAvg;\n>> -\tunsigned char satRatio;\n>> -\tunsigned char padding[3];\n>> -} __attribute__((packed));\n>> -\n>> struct Accumulator {\n>> \tunsigned int counted;\n>> \tunsigned int total;\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 04AC2BDC71\n\tfor ;\n\tThu, 2 Sep 2021 16:08:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C8F76916A;\n\tThu, 2 Sep 2021 18:08:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6E88560255\n\tfor ;\n\tThu, 2 Sep 2021 18:08:02 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:43b0:6633:e0bd:5dc6])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0703D45E;\n\tThu, 2 Sep 2021 18:08:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"phHDtZGh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630598882;\n\tbh=bUj/iVmncpnrnBqvoasY3RMh4Sr9VC8K2KBX+wh0FhQ=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=phHDtZGhUNTgvEuYbhd13TJ/khHPr/MMCn4Bj6OTQneUcEOdvTgZT3oeRCBOncphJ\n\tSBQj3f4tQEiccIlOWx6/eye1PhU7hq3WG6md3R9dyumVj/xfr+8sUtKRtasR8dZeYp\n\tCBasgrWwS66QFHcjFw5BtIeZ952l8IWmU2pTWI8M=","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210902132930.76137-1-jeanmichel.hautbois@ideasonboard.com>\n\t<1cf179cb-03ae-a779-e360-edc807a66509@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"<773ca75c-41e1-b283-9a85-6629a8db9dcf@ideasonboard.com>","Date":"Thu, 2 Sep 2021 18:08:00 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"<1cf179cb-03ae-a779-e360-edc807a66509@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Replace\n\tipa::ipu3::algorithms::Ipu3AwbCell","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19310,"web_url":"https://patchwork.libcamera.org/comment/19310/","msgid":"","date":"2021-09-02T18:52:33","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Replace\n\tipa::ipu3::algorithms::Ipu3AwbCell","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Thu, Sep 02, 2021 at 06:08:00PM +0200, Jean-Michel Hautbois wrote:\n> On 02/09/2021 17:46, Kieran Bingham wrote:\n> > On 02/09/2021 14:29, Jean-Michel Hautbois wrote:\n> >> This patch was proposed in:\n> >> https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/\n> > \n> > That's not accurate, and it dives off to an external reference without\n> > explaining why. /This/ patch is not proposed for linux-media.\n> > \n> > \n> > Try to introduce your patches, and think about your commit message in\n> > distinct sections such as:\n> > \n> > - Describe the issue\n> > - Describe any context\n> > - Describe solution\n> > \n> > \n> > - the issue\n> > \"\"\"\n> > The intel-ipu3.h public interface from the kernel does not define how to\n> > parse the statistics for a cell. This had to be identified by a process\n> > of reverse engineering, and later identifying the structures from\n> > https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h\n> > leading to our custom definition of struct Ipu3AwbCell.\n> > \n> > \"\"\"\n> > \n> > \n> > - context\n> > \"\"\"\n> > To improve the kernel interface, a proposal has been made to the\n> > linux-kernel [0] to incorporate the memory layout for each cell into the\n> > intel-ipu3 header directly.\n> > \n> > [0]\n> > https://lore.kernel.org/linux-media/20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com/\n> > \"\"\"\n> > \n> > \n> > - solution / What this patch does\n> > \"\"\"\n> > Update our local copy of the intel-ipu3.h to match the proposal and\n> > change the AGC and AWB algorithms to reference that structure directly,\n> > allowing us to remove the deprecated custom Ipu3AwbCell definition.\n> > \"\"\"\n> > \n> > The only issue is that the proposal to linux-media has not yet been\n> > reviewed or approved/integrated, so until then this patch would cause us\n> > to diverge from the kernel version of the header and would require\n> > special attention.\n> > \n> > That may not be an issue in this case, as we can track the progress\n> > upstream - but normally we would expect to keep our linux headers\n> > matching the kernel unmodified.\n> \n> Thanks a lot for this really better description, I sent a v2 with it.\n\nIf you really want to thank Kieran, try to send further patches with\ncommit messages that follow that structure :-) It will save him review\ntime, I'm sure he'll appreciate.\n\n> I know we would normally keep the headers matching the kernel, but it\n> could take some time before we can do that, can we afford it ?\n\nNot an issue in this case, we can track the change as it progresses\nupstream and update this header accordingly. The important part is that\nwe're trying to land this change upstream. If it wasn't submitted\nupstream, or had no hope of getting merged there, it would be a\ndifferent story.\n\n> >> It introduces the AWB metadata layout as a structure in the kernel\n> >> header, to make it clear for userspace applications. As intel-ipu3.h now\n> >> has a defined AWB layout, use that one instead of the\n> >> ipa::ipu3::algorithms::Ipu3AwbCell.\n> >>\n> >> Signed-off-by: Jean-Michel Hautbois \n> >> ---\n> >> include/linux/intel-ipu3.h | 38 +++++++++++++++++++++++++--------\n> >> src/ipa/ipu3/algorithms/agc.cpp | 7 +++---\n> >> src/ipa/ipu3/algorithms/awb.cpp | 10 ++++-----\n> >> src/ipa/ipu3/algorithms/awb.h | 10 ---------\n> >> 4 files changed, 38 insertions(+), 27 deletions(-)\n> >>\n> >> diff --git a/include/linux/intel-ipu3.h b/include/linux/intel-ipu3.h\n> >> index ee0e6d0e..0633e04e 100644\n> >> --- a/include/linux/intel-ipu3.h\n> >> +++ b/include/linux/intel-ipu3.h\n> >> @@ -59,20 +59,40 @@ struct ipu3_uapi_grid_config {\n> >> \t__u16 y_end;\n> >> } __attribute__((packed));\n> >> \n> >> +/**\n> >> + * struct ipu3_uapi_awb_set_item - Memory layout for each cell in AWB\n> >> + *\n> >> + * @Gr_avg:\tGreen average for red lines in the cell.\n> >> + * @R_avg:\tRed average in the cell.\n> >> + * @B_avg:\tBlue average in the cell.\n> >> + * @Gb_avg:\tGreen average for blue lines in the cell.\n> >> + * @sat_ratio: Saturation ratio in the cell.\n> >> + * @padding0: Unused byte for padding.\n> >> + * @padding1: Unused byte for padding.\n> >> + * @padding2: Unused byte for padding.\n> >> + */\n> >> +struct ipu3_uapi_awb_set_item {\n> >> +\tunsigned char Gr_avg;\n> >> +\tunsigned char R_avg;\n> >> +\tunsigned char B_avg;\n> >> +\tunsigned char Gb_avg;\n> >> +\tunsigned char sat_ratio;\n> >> +\tunsigned char padding0;\n> >> +\tunsigned char padding1;\n> >> +\tunsigned char padding2;\n> >> +} __attribute__((packed));\n> >> +\n> >> /*\n> >> * The grid based data is divided into \"slices\" called set, each slice of setX\n> >> * refers to ipu3_uapi_grid_config width * height_per_slice.\n> >> */\n> >> #define IPU3_UAPI_AWB_MAX_SETS\t\t\t\t60\n> >> -/* Based on grid size 80 * 60 and cell size 16 x 16 */\n> >> -#define IPU3_UAPI_AWB_SET_SIZE\t\t\t\t1280\n> >> -#define IPU3_UAPI_AWB_MD_ITEM_SIZE\t\t\t8\n> >> -#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \\\n> >> -\t(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \\\n> >> -\t IPU3_UAPI_AWB_MD_ITEM_SIZE)\n> >> +#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET\t\t\t160\n> >> +/* Based on max grid height + Spare for bubbles */\n> >> +#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \\\n> >> +\t(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)\n> >> #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \\\n> >> -\t(IPU3_UAPI_AWB_MAX_SETS * \\\n> >> -\t (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))\n> >> + AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET\n> >> \n> >> \n> >> /**\n> >> @@ -82,7 +102,7 @@ struct ipu3_uapi_grid_config {\n> >> *\t\tthe average values for each color channel.\n> >> */\n> >> struct ipu3_uapi_awb_raw_buffer {\n> >> -\t__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]\n> >> +\tstruct ipu3_uapi_awb_set_item meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]\n> >> \t\t__attribute__((aligned(32)));\n> >> } __attribute__((packed));\n> >> \n> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> >> index 5ff50f4a..36f648be 100644\n> >> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> >> @@ -96,9 +96,10 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> >> \t\t\t * We observed a bit shift which makes the value 160 to be 32 in the stats grid.\n> >> \t\t\t * Use the one passed at init time.\n> >> \t\t\t */\n> >> -\t\t\tif (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {\n> >> -\t\t\t\tuint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];\n> >> -\t\t\t\tuint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];\n> >> +\t\t\tconst ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width];\n> >> +\t\t\tif (currentCell->sat_ratio == 0) {\n> >> +\t\t\t\tuint8_t Gr = currentCell->Gr_avg;\n> >> +\t\t\t\tuint8_t Gb = currentCell->Gb_avg;\n> >> \t\t\t\thist[(Gr + Gb) / 2]++;\n> >> \t\t\t\tcount++;\n> >> \t\t\t}\n> >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> >> index ea567f96..db0a22d3 100644\n> >> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> >> @@ -207,14 +207,14 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> >> \t\t\tcellPosition *= 8;\n> >> \n> >> \t\t\t/* Cast the initial IPU3 structure to simplify the reading */\n> >> -\t\t\tIpu3AwbCell *currentCell = reinterpret_cast(const_cast(&stats->awb_raw_buffer.meta_data[cellPosition]));\n> >> -\t\t\tif (currentCell->satRatio == 0) {\n> >> +\t\t\tconst ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[cellPosition];\n> >> +\t\t\tif (currentCell->sat_ratio == 0) {\n> >> \t\t\t\t/* The cell is not saturated, use the current cell */\n> >> \t\t\t\tawbStats_[awbRegionPosition].counted++;\n> >> -\t\t\t\tuint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;\n> >> +\t\t\t\tuint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;\n> >> \t\t\t\tawbStats_[awbRegionPosition].sum.green += greenValue / 2;\n> >> -\t\t\t\tawbStats_[awbRegionPosition].sum.red += currentCell->redAvg;\n> >> -\t\t\t\tawbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;\n> >> +\t\t\t\tawbStats_[awbRegionPosition].sum.red += currentCell->R_avg;\n> >> +\t\t\t\tawbStats_[awbRegionPosition].sum.blue += currentCell->B_avg;\n> >> \t\t\t}\n> >> \t\t}\n> >> \t}\n> >> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> >> index 23f3e872..a9320b4c 100644\n> >> --- a/src/ipa/ipu3/algorithms/awb.h\n> >> +++ b/src/ipa/ipu3/algorithms/awb.h\n> >> @@ -23,16 +23,6 @@ namespace ipa::ipu3::algorithms {\n> >> static constexpr uint32_t kAwbStatsSizeX = 16;\n> >> static constexpr uint32_t kAwbStatsSizeY = 12;\n> >> \n> >> -/* \\todo Move the cell layout into intel-ipu3.h kernel header */\n> >> -struct Ipu3AwbCell {\n> >> -\tunsigned char greenRedAvg;\n> >> -\tunsigned char redAvg;\n> >> -\tunsigned char blueAvg;\n> >> -\tunsigned char greenBlueAvg;\n> >> -\tunsigned char satRatio;\n> >> -\tunsigned char padding[3];\n> >> -} __attribute__((packed));\n> >> -\n> >> struct Accumulator {\n> >> \tunsigned int counted;\n> >> \tunsigned int total;\n> >>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CE91FBDC71\n\tfor ;\n\tThu, 2 Sep 2021 18:52:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3125D69166;\n\tThu, 2 Sep 2021 20:52:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67C3860255\n\tfor ;\n\tThu, 2 Sep 2021 20:52:50 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CAEE545E;\n\tThu, 2 Sep 2021 20:52:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uzs7QJvf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630608770;\n\tbh=BJOn6E3/FwYmnzZoJpfCpam4jSfXof+X6wUNvZ2WCe4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uzs7QJvfGuvqvrun6oKBml5UFUHS4q7SWBlBzUImZObjQbgDxqMuEXmE85cdR8fvx\n\tPplyGRmOf3OQYCyryeJt4u3CZ+MUeMXmsNAdo2NFnRLtRmYwS0TjmK328BKpcsYcQz\n\tuuXpWmmi4fZlZCRaRGgRqqGGqKtOU+2p3tICD7U0=","Date":"Thu, 2 Sep 2021 21:52:33 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210902132930.76137-1-jeanmichel.hautbois@ideasonboard.com>\n\t<1cf179cb-03ae-a779-e360-edc807a66509@ideasonboard.com>\n\t<773ca75c-41e1-b283-9a85-6629a8db9dcf@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<773ca75c-41e1-b283-9a85-6629a8db9dcf@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: Replace\n\tipa::ipu3::algorithms::Ipu3AwbCell","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]