[{"id":19002,"web_url":"https://patchwork.libcamera.org/comment/19002/","msgid":"","date":"2021-08-23T15:58:14","subject":"Re: [libcamera-devel] [PATCH v1 1/7] ipa: ipu3: Move the AWB stats\n\tstructures","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Mon, Aug 23, 2021 at 02:49:31PM +0200, Jean-Michel Hautbois wrote:\n> The structure Ipu3AwbCell describes the AWB stats layout on the kernel\n> side. We will need it to be used by the AGC algorithm to be introduced\n> later, so let's make it visible from ipa::ipu3::algorithms and not only\n> for the AWB class.\n\nYou need to either add a blank line here, or remove the line break.\n\n> This structure should probably go into the intel-ipu3.h file, whichs\n> means a kernel patch, let's keep it in mind for the moment.\n> \n> The other structures RGB, IspStatsRegion and AwbStatus will also be used\n> elsewhere so move them at the same time.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/awb.h | 77 ++++++++++++++++++-----------------\n> 1 file changed, 39 insertions(+), 38 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> index a16dd68d..332652d0 100644\n> --- a/src/ipa/ipu3/algorithms/awb.h\n> +++ b/src/ipa/ipu3/algorithms/awb.h\n> @@ -23,6 +23,45 @@ 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> +};\n\nWhere's the packed attribute ?\n\n> +\n> +/* \\todo Make these structs available to all the ISPs ? */\n> +struct RGB {\n> +\tRGB(double _R = 0, double _G = 0, double _B = 0)\n> +\t\t: R(_R), G(_G), B(_B)\n> +\t{\n> +\t}\n> +\tdouble R, G, B;\n> +\tRGB &operator+=(RGB const &other)\n> +\t{\n> +\t\tR += other.R, G += other.G, B += other.B;\n> +\t\treturn *this;\n> +\t}\n> +};\n> +\n> +struct IspStatsRegion {\n> +\tunsigned int counted;\n> +\tunsigned int uncounted;\n> +\tunsigned long long rSum;\n> +\tunsigned long long gSum;\n> +\tunsigned long long bSum;\n> +};\n> +\n> +struct AwbStatus {\n> +\tdouble temperatureK;\n> +\tdouble redGain;\n> +\tdouble greenGain;\n> +\tdouble blueGain;\n> +};\n\nThose are four different cases:\n\n- Ipu3AwbCell describes the layout of statistics data produced by the\n ImgU, and should thus indeed be defined in intel-ipu3.h. I'm fine\n having it here for the time being.\n\n- RGB is a more generic structure, it should be in a more generic\n header. It's however only used in the declaration of the\n AgcMetering::generateZones() function, which has no definition. You\n could thus leave it where it is for the time being.\n\n- IspStatsRegion is also not AWB-specific, it doesn't belong to this\n header. It should also be renamed to a more explicit name.\n\n- AwbStatus is only used outside of awb.c to store the red, green and\n blue gains in AgcMetering::awbStatus_, copying them from\n IPAFrameContext::awb::gains. There's no need to copy the data in\n AgcMetering::awbStatus_ in the first place, you could pass the\n IPAFrameContext to AgcMetering::computeInitialY() through\n AgcMetering::computeGain().\n\n This being said, if you think that AwbStatus in its current form would\n be a good model for AWB status regardless of the actual implementation\n of the algorithm (that is, if other parallel implementations of AWB in\n the future would use the same status structure), then the structure\n could be moved to ipa_context.h, and replace the awb field of\n IPAFrameContext. In that case I'd modigy AwbStatus as follows:\n\nstruct AwbStatus {\n\tdouble temperatureK;\n\tstruct {\n\t\tdouble red;\n\t\tdouble green;\n\t\tdouble blue;\n\t} gains;\n};\n\n> +\n> class Awb : public Algorithm\n> {\n> public:\n> @@ -32,44 +71,6 @@ public:\n> \tvoid prepare(IPAContext &context, ipu3_uapi_params *params) override;\n> \tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> \n> -\tstruct Ipu3AwbCell {\n> -\t\tunsigned char greenRedAvg;\n> -\t\tunsigned char redAvg;\n> -\t\tunsigned char blueAvg;\n> -\t\tunsigned char greenBlueAvg;\n> -\t\tunsigned char satRatio;\n> -\t\tunsigned char padding[3];\n> -\t} __attribute__((packed));\n> -\n> -\t/* \\todo Make these three structs available to all the ISPs ? */\n> -\tstruct RGB {\n> -\t\tRGB(double _R = 0, double _G = 0, double _B = 0)\n> -\t\t\t: R(_R), G(_G), B(_B)\n> -\t\t{\n> -\t\t}\n> -\t\tdouble R, G, B;\n> -\t\tRGB &operator+=(RGB const &other)\n> -\t\t{\n> -\t\t\tR += other.R, G += other.G, B += other.B;\n> -\t\t\treturn *this;\n> -\t\t}\n> -\t};\n> -\n> -\tstruct IspStatsRegion {\n> -\t\tunsigned int counted;\n> -\t\tunsigned int uncounted;\n> -\t\tunsigned long long rSum;\n> -\t\tunsigned long long gSum;\n> -\t\tunsigned long long bSum;\n> -\t};\n> -\n> -\tstruct AwbStatus {\n> -\t\tdouble temperatureK;\n> -\t\tdouble redGain;\n> -\t\tdouble greenGain;\n> -\t\tdouble blueGain;\n> -\t};\n> -\n> private:\n> \tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid);","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 9E691BD87D\n\tfor ;\n\tMon, 23 Aug 2021 15:58:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CEEB688A3;\n\tMon, 23 Aug 2021 17:58:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B781D6025B\n\tfor ;\n\tMon, 23 Aug 2021 17:58:24 +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 3B63B2A5;\n\tMon, 23 Aug 2021 17:58:24 +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=\"cPMqHbDR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629734304;\n\tbh=CejNbh98vmfKvIXp/kgIHRsFalZpAD/EqbVZAi6TxXw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cPMqHbDRc9p4Q4eo30ZK5B3ExfXQqP83VvOe5nVm8BuqFIT3YBX5gikvb0p2tT2Jv\n\t1Xx2tavACIo3Kria7rcjna0Y6fvfuCT7F2QWRb26Tlf42DuwXJ8K2cA/dXah016K/u\n\tQa7qTxQSE7cgEWoWW+gCA1htmMwrY1mfpE7uQgRI=","Date":"Mon, 23 Aug 2021 18:58:14 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210823124937.253539-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210823124937.253539-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210823124937.253539-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 1/7] ipa: ipu3: Move the AWB stats\n\tstructures","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\" "}}]