[{"id":19492,"web_url":"https://patchwork.libcamera.org/comment/19492/","msgid":"<3816220a-2f1c-0eb7-13d1-6f6fe42144c2@ideasonboard.com>","date":"2021-09-07T06:44:13","subject":"Re: [libcamera-devel] [PATCH v3 4/4] 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 JM,\n\nOn 06/09/2021 21:47, Jean-Michel Hautbois wrote:\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> 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> 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> 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 | 11 +++++-----\n> src/ipa/ipu3/algorithms/awb.h | 10 ---------\n> 4 files changed, 38 insertions(+), 28 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\nThe idea is not bad, but you did not test it before sending, and forgot\none little thing: you need to align the kernel on it, because the sizes\ndefined here are not the same as before !\nSo, you could keep the old sizes defined (don't forget to remove the\nIPU3_UAPI_AWB_MD_ITEM_SIZE multiplication though) or don't apply that\npatch now...\n\n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 5ff50f4a..8740dcdf 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 + 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 bc0aa6fe..5d6fa4d4 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -201,17 +201,16 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> \t\t\tuint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;\n> \n> \t\t\tuint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;\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 3385ebe7..17905ae8 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> \tstruct {\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 4D142BE175\n\tfor ;\n\tTue, 7 Sep 2021 06:44:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CAA666916A;\n\tTue, 7 Sep 2021 08:44:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94AE369168\n\tfor ;\n\tTue, 7 Sep 2021 08:44:16 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:e0a9:3439:7934:19a5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D057499\n\tfor ;\n\tTue, 7 Sep 2021 08:44:16 +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=\"FQCwn6K5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630997056;\n\tbh=haJDt9xD4ZJAd3/+WnPTLhUKCK0kxdFZPWIJFE2bgzY=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=FQCwn6K5zGcrbmBmNU7Tg26fPh69BcsW6N0MjFVtACEG9qSXTISRjAhadl8EdUjet\n\tKfA6oErLhuKgcoCqi++Q6hi5zbONJbj+zgG/f/ocF/WyNAUDPr10nhwmn3bxsXiDed\n\t9HwP8FPEchVqgU8eABBRYgnbDBl2u3np9H2tUKfc=","To":"libcamera-devel@lists.libcamera.org","References":"<20210906194755.106953-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210906194755.106953-5-jeanmichel.hautbois@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"<3816220a-2f1c-0eb7-13d1-6f6fe42144c2@ideasonboard.com>","Date":"Tue, 7 Sep 2021 08:44:13 +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":"<20210906194755.106953-5-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] 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\" "}}]