[{"id":18016,"web_url":"https://patchwork.libcamera.org/comment/18016/","msgid":"<2905ae70-c166-1b48-13a0-28223174783f@ideasonboard.com>","date":"2021-07-07T12:45:17","subject":"Re: [libcamera-devel] [PATCH v1 3/7] ipa: ipu3: Use a common IPU3\n\theader for the constants","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi JM,\n\nOn 28/06/2021 21:22, Jean-Michel Hautbois wrote:\n> Every constants used can be needed by both AWB and AGC (for the moment).\n> Use a common header to simplify their usage.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3_agc.h | 2 ++\n> src/ipa/ipu3/ipu3_awb.h | 11 ++-------\n> src/ipa/ipu3/ipu3_common.h | 49 ++++++++++++++++++++++++++++++++++++++\n\nthe ipu3_ in ipu3/ipu3_common seems redundant, but we already have\nipu3_agc, ipu3_awb ...\n\n\n\n> 3 files changed, 53 insertions(+), 9 deletions(-)\n> create mode 100644 src/ipa/ipu3/ipu3_common.h\n> \n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> index 6ca9af8e..774c8385 100644\n> --- a/src/ipa/ipu3/ipu3_agc.h\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -7,6 +7,8 @@\n> #ifndef __LIBCAMERA_IPU3_AGC_H__\n> #define __LIBCAMERA_IPU3_AGC_H__\n> \n> +#include \"ipu3_common.h\"\n> +\n> #include \n> #include \n> \n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> index 6ae111fd..f4100f4a 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/ipu3_awb.h\n> @@ -7,6 +7,8 @@\n> #ifndef __LIBCAMERA_IPU3_AWB_H__\n> #define __LIBCAMERA_IPU3_AWB_H__\n> \n> +#include \"ipu3_common.h\"\n> +\n> #include \n> \n> #include \n> @@ -34,15 +36,6 @@ public:\n> \tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> \tvoid updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma);\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> private:\n> \tvoid generateZones(std::vector &zones);\n> \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n> diff --git a/src/ipa/ipu3/ipu3_common.h b/src/ipa/ipu3/ipu3_common.h\n> new file mode 100644\n> index 00000000..38ef49ce\n> --- /dev/null\n> +++ b/src/ipa/ipu3/ipu3_common.h\n\n\n\n> @@ -0,0 +1,49 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * isp.h - ISP statistics interface\n\nisp.h?\n\n\n> + */\n> +#ifndef __LIBCAMERA_IPA_IPU3_COMMON_H__\n> +#define __LIBCAMERA_IPA_IPU3_COMMON_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +/* Region size for the statistics generation algorithm */\n> +static constexpr uint32_t kAwbStatsSizeX = 16;\n> +static constexpr uint32_t kAwbStatsSizeY = 12;\n> +static constexpr uint32_t kAwbStatsSize = kAwbStatsSizeX * kAwbStatsSizeY + 1;\n> +\n> +static constexpr uint32_t kAgcStatsSizeX = 7;\n> +static constexpr uint32_t kAgcStatsSizeY = 5;\n> +static constexpr uint32_t kAgcStatsSize = kAgcStatsSizeX * kAgcStatsSizeY + 1;\n> +static constexpr uint32_t kNumAgcWeightedZones = 15;\n> +static constexpr double kCenteredWeights[kNumAgcWeightedZones] = { 3, 3, 3, 2, 2, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0 };\n> +static constexpr uint32_t kAgcStatsRegions[kAgcStatsSize] = {\n> +\t11, 9, 9, 9, 9, 9, 12,\n> +\t7, 5, 3, 3, 3, 6, 8,\n> +\t7, 5, 1, 0, 2, 6, 8,\n> +\t7, 5, 4, 4, 4, 6, 8,\n> +\t13, 10, 10, 10, 10, 10, 14\n> +};\n\nWhat makes these all better in common? It's not clear yet why these will\nbe used elsewhere - but perhaps that's likely to come later in the series.\n\n\nIf they are common, are they still specific to AwbStatsSize? Is this the\nmaximum - or a declaration of the size used?\n\nAre these structures used by the algorithms, or by the kernel specific\nstructures?\n\n\n> +\n> +static constexpr uint32_t kMinGreenLevelInZone = 16;\n> +\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\nSeeing __attribute__((packed)) implies that this is used by a kernel\nstructure or hardware directly. Is that true?\n\nIs this defined in an existing interface file? or is this one of the\nones we had to discover ourselves.\n\nEither way, it's lacking some documentation to state what it references\nand why the layout is important.\n\n\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_IPU3_COMMON_H__ */\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 529D0C3224\n\tfor ;\n\tWed, 7 Jul 2021 12:45:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF34C60284;\n\tWed, 7 Jul 2021 14:45:20 +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 B7C0160284\n\tfor ;\n\tWed, 7 Jul 2021 14:45:19 +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 3F00A2E4;\n\tWed, 7 Jul 2021 14:45:19 +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=\"QpEdqCyl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625661919;\n\tbh=lOPwYAr5d7JghPtCq6LFEjx5VUkxSlSpO27ltLSfIe8=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=QpEdqCyls5YUtJYWoTYGQkTMxHgJtA/BPu2upMvDUcpjAmuYYeIMw+m7b4z4vN89N\n\tdT27gKHHwEsF1pTaq9grrfqCKqR2G7VwJTUfjRrH1E6+ELdcRPf0KW3jUFNKZapLN0\n\twlav77t+oORdCYvTGzbrR8OcZ2Ovr24vhVJVc1G8=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210628202255.138874-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210628202255.138874-4-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<2905ae70-c166-1b48-13a0-28223174783f@ideasonboard.com>","Date":"Wed, 7 Jul 2021 13:45:17 +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":"<20210628202255.138874-4-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 3/7] ipa: ipu3: Use a common IPU3\n\theader for the constants","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":18030,"web_url":"https://patchwork.libcamera.org/comment/18030/","msgid":"","date":"2021-07-08T12:55:23","subject":"Re: [libcamera-devel] [PATCH v1 3/7] ipa: ipu3: Use a common IPU3\n\theader for the constants","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Jul 07, 2021 at 01:45:17PM +0100, Kieran Bingham wrote:\n> On 28/06/2021 21:22, Jean-Michel Hautbois wrote:\n> > Every constants used can be needed by both AWB and AGC (for the moment).\n> > Use a common header to simplify their usage.\n> > \n> > Signed-off-by: Jean-Michel Hautbois \n> > ---\n> > src/ipa/ipu3/ipu3_agc.h | 2 ++\n> > src/ipa/ipu3/ipu3_awb.h | 11 ++-------\n> > src/ipa/ipu3/ipu3_common.h | 49 ++++++++++++++++++++++++++++++++++++++\n> \n> the ipu3_ in ipu3/ipu3_common seems redundant, but we already have\n> ipu3_agc, ipu3_awb ...\n> \n> > 3 files changed, 53 insertions(+), 9 deletions(-)\n> > create mode 100644 src/ipa/ipu3/ipu3_common.h\n> > \n> > diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> > index 6ca9af8e..774c8385 100644\n> > --- a/src/ipa/ipu3/ipu3_agc.h\n> > +++ b/src/ipa/ipu3/ipu3_agc.h\n> > @@ -7,6 +7,8 @@\n> > #ifndef __LIBCAMERA_IPU3_AGC_H__\n> > #define __LIBCAMERA_IPU3_AGC_H__\n> > \n> > +#include \"ipu3_common.h\"\n> > +\n\nThis doesn't seem needed.\n\n> > #include \n> > #include \n> > \n> > diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> > index 6ae111fd..f4100f4a 100644\n> > --- a/src/ipa/ipu3/ipu3_awb.h\n> > +++ b/src/ipa/ipu3/ipu3_awb.h\n> > @@ -7,6 +7,8 @@\n> > #ifndef __LIBCAMERA_IPU3_AWB_H__\n> > #define __LIBCAMERA_IPU3_AWB_H__\n> > \n> > +#include \"ipu3_common.h\"\n> > +\n> > #include \n> > \n> > #include \n> > @@ -34,15 +36,6 @@ public:\n> > \tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> > \tvoid updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma);\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> > private:\n> > \tvoid generateZones(std::vector &zones);\n> > \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n> > diff --git a/src/ipa/ipu3/ipu3_common.h b/src/ipa/ipu3/ipu3_common.h\n> > new file mode 100644\n> > index 00000000..38ef49ce\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/ipu3_common.h\n> > @@ -0,0 +1,49 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Ideas On Board\n> > + *\n> > + * isp.h - ISP statistics interface\n> \n> isp.h?\n> \n> > + */\n> > +#ifndef __LIBCAMERA_IPA_IPU3_COMMON_H__\n> > +#define __LIBCAMERA_IPA_IPU3_COMMON_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +/* Region size for the statistics generation algorithm */\n> > +static constexpr uint32_t kAwbStatsSizeX = 16;\n> > +static constexpr uint32_t kAwbStatsSizeY = 12;\n> > +static constexpr uint32_t kAwbStatsSize = kAwbStatsSizeX * kAwbStatsSizeY + 1;\n> > +\n> > +static constexpr uint32_t kAgcStatsSizeX = 7;\n> > +static constexpr uint32_t kAgcStatsSizeY = 5;\n> > +static constexpr uint32_t kAgcStatsSize = kAgcStatsSizeX * kAgcStatsSizeY + 1;\n> > +static constexpr uint32_t kNumAgcWeightedZones = 15;\n> > +static constexpr double kCenteredWeights[kNumAgcWeightedZones] = { 3, 3, 3, 2, 2, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0 };\n> > +static constexpr uint32_t kAgcStatsRegions[kAgcStatsSize] = {\n> > +\t11, 9, 9, 9, 9, 9, 12,\n> > +\t7, 5, 3, 3, 3, 6, 8,\n> > +\t7, 5, 1, 0, 2, 6, 8,\n> > +\t7, 5, 4, 4, 4, 6, 8,\n> > +\t13, 10, 10, 10, 10, 10, 14\n> > +};\n> \n> What makes these all better in common? It's not clear yet why these will\n> be used elsewhere - but perhaps that's likely to come later in the series.\n> \n> If they are common, are they still specific to AwbStatsSize? Is this the\n> maximum - or a declaration of the size used?\n> \n> Are these structures used by the algorithms, or by the kernel specific\n> structures?\n\nThe constants should also be documented.\n\n> > +\n> > +static constexpr uint32_t kMinGreenLevelInZone = 16;\n> > +\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> Seeing __attribute__((packed)) implies that this is used by a kernel\n> structure or hardware directly. Is that true?\n> \n> Is this defined in an existing interface file? or is this one of the\n> ones we had to discover ourselves.\n> \n> Either way, it's lacking some documentation to state what it references\n> and why the layout is important.\n\nAlso, as the structure seems to be AWB-specific, why isn't it better\nplaced in ipu3_awb.h ?\n\nSame for the constants above, shouldn't they go to ipu3_agc.h or\nipu3_awb.h ? Or better, if they're only used in the implementation, in\na .cpp file ?\n\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_IPA_IPU3_COMMON_H__ */\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 84D5FC3224\n\tfor ;\n\tThu, 8 Jul 2021 12:56:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB13B68513;\n\tThu, 8 Jul 2021 14:56:09 +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 05AD668506\n\tfor ;\n\tThu, 8 Jul 2021 14:56:08 +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 61385E7;\n\tThu, 8 Jul 2021 14:56:07 +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=\"e/r60vhK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625748967;\n\tbh=YDlcVn78Y2KjEFaUwuo+zivn4Qo6nwFLwAIijJ7tbHE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e/r60vhK+Pn3Jp/NXTPGEA05d+cj1cVYB9MEKmofV1jyeR+5VSqlbalFh56Doqn2A\n\tX9W56oRckSSGKOnq3mXOQ5Ac7otQ5pekSgv6OaibpncQEYGWGogo7IcV6bavvEODIJ\n\tG0nIzQC7ggQYb5jwREKtl5JBPjoBZpv+MoHvF68I=","Date":"Thu, 8 Jul 2021 15:55:23 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210628202255.138874-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210628202255.138874-4-jeanmichel.hautbois@ideasonboard.com>\n\t<2905ae70-c166-1b48-13a0-28223174783f@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<2905ae70-c166-1b48-13a0-28223174783f@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 3/7] ipa: ipu3: Use a common IPU3\n\theader for the constants","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\" "}},{"id":18037,"web_url":"https://patchwork.libcamera.org/comment/18037/","msgid":"<634698a1-6ccc-216a-9732-691d1d9a0ee6@ideasonboard.com>","date":"2021-07-09T06:04:07","subject":"Re: [libcamera-devel] [PATCH v1 3/7] ipa: ipu3: Use a common IPU3\n\theader for the constants","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hello Kieran, Laurent,\n\nOn 08/07/2021 14:55, Laurent Pinchart wrote:\n> Hello,\n> \n> On Wed, Jul 07, 2021 at 01:45:17PM +0100, Kieran Bingham wrote:\n>> On 28/06/2021 21:22, Jean-Michel Hautbois wrote:\n>>> Every constants used can be needed by both AWB and AGC (for the moment).\n>>> Use a common header to simplify their usage.\n>>>\n>>> Signed-off-by: Jean-Michel Hautbois \n>>> ---\n>>> src/ipa/ipu3/ipu3_agc.h | 2 ++\n>>> src/ipa/ipu3/ipu3_awb.h | 11 ++-------\n>>> src/ipa/ipu3/ipu3_common.h | 49 ++++++++++++++++++++++++++++++++++++++\n>>\n>> the ipu3_ in ipu3/ipu3_common seems redundant, but we already have\n>> ipu3_agc, ipu3_awb ...\n>>\n>>> 3 files changed, 53 insertions(+), 9 deletions(-)\n>>> create mode 100644 src/ipa/ipu3/ipu3_common.h\n>>>\n>>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n>>> index 6ca9af8e..774c8385 100644\n>>> --- a/src/ipa/ipu3/ipu3_agc.h\n>>> +++ b/src/ipa/ipu3/ipu3_agc.h\n>>> @@ -7,6 +7,8 @@\n>>> #ifndef __LIBCAMERA_IPU3_AGC_H__\n>>> #define __LIBCAMERA_IPU3_AGC_H__\n>>> \n>>> +#include \"ipu3_common.h\"\n>>> +\n> \n> This doesn't seem needed.\n> \n\nIt is used in the new AGC algorithm which is patch 7/7. The include\nshould then go there, you are right :-).\n\n>>> #include \n>>> #include \n>>> \n>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n>>> index 6ae111fd..f4100f4a 100644\n>>> --- a/src/ipa/ipu3/ipu3_awb.h\n>>> +++ b/src/ipa/ipu3/ipu3_awb.h\n>>> @@ -7,6 +7,8 @@\n>>> #ifndef __LIBCAMERA_IPU3_AWB_H__\n>>> #define __LIBCAMERA_IPU3_AWB_H__\n>>> \n>>> +#include \"ipu3_common.h\"\n>>> +\n>>> #include \n>>> \n>>> #include \n>>> @@ -34,15 +36,6 @@ public:\n>>> \tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n>>> \tvoid updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma);\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>>> private:\n>>> \tvoid generateZones(std::vector &zones);\n>>> \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n>>> diff --git a/src/ipa/ipu3/ipu3_common.h b/src/ipa/ipu3/ipu3_common.h\n>>> new file mode 100644\n>>> index 00000000..38ef49ce\n>>> --- /dev/null\n>>> +++ b/src/ipa/ipu3/ipu3_common.h\n>>> @@ -0,0 +1,49 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2021, Ideas On Board\n>>> + *\n>>> + * isp.h - ISP statistics interface\n>>\n>> isp.h?\n>>\n>>> + */\n>>> +#ifndef __LIBCAMERA_IPA_IPU3_COMMON_H__\n>>> +#define __LIBCAMERA_IPA_IPU3_COMMON_H__\n>>> +\n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +namespace ipa {\n>>> +\n>>> +/* Region size for the statistics generation algorithm */\n>>> +static constexpr uint32_t kAwbStatsSizeX = 16;\n>>> +static constexpr uint32_t kAwbStatsSizeY = 12;\n>>> +static constexpr uint32_t kAwbStatsSize = kAwbStatsSizeX * kAwbStatsSizeY + 1;\n>>> +\n>>> +static constexpr uint32_t kAgcStatsSizeX = 7;\n>>> +static constexpr uint32_t kAgcStatsSizeY = 5;\n>>> +static constexpr uint32_t kAgcStatsSize = kAgcStatsSizeX * kAgcStatsSizeY + 1;\n>>> +static constexpr uint32_t kNumAgcWeightedZones = 15;\n>>> +static constexpr double kCenteredWeights[kNumAgcWeightedZones] = { 3, 3, 3, 2, 2, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0 };\n>>> +static constexpr uint32_t kAgcStatsRegions[kAgcStatsSize] = {\n>>> +\t11, 9, 9, 9, 9, 9, 12,\n>>> +\t7, 5, 3, 3, 3, 6, 8,\n>>> +\t7, 5, 1, 0, 2, 6, 8,\n>>> +\t7, 5, 4, 4, 4, 6, 8,\n>>> +\t13, 10, 10, 10, 10, 10, 14\n>>> +};\n>>\n>> What makes these all better in common? It's not clear yet why these will\n>> be used elsewhere - but perhaps that's likely to come later in the series.\n>>\n>> If they are common, are they still specific to AwbStatsSize? Is this the\n>> maximum - or a declaration of the size used?\n>>\n>> Are these structures used by the algorithms, or by the kernel specific\n>> structures?\n> \n> The constants should also be documented.\n> \n>>> +\n>>> +static constexpr uint32_t kMinGreenLevelInZone = 16;\n>>> +\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>> Seeing __attribute__((packed)) implies that this is used by a kernel\n>> structure or hardware directly. Is that true?\n>>\n>> Is this defined in an existing interface file? or is this one of the\n>> ones we had to discover ourselves.\n>>\n>> Either way, it's lacking some documentation to state what it references\n>> and why the layout is important.\n\nThe structure is describing what is in the IPU3 stats structure for AWB.\nI will document it when getting it back into ipu3_awb.cpp.\nThe __attribute__((packed)) is a \"C\" reflex and probably not useful here\n(it is used to cast and move from one cell to another).\n\n> \n> Also, as the structure seems to be AWB-specific, why isn't it better\n> placed in ipu3_awb.h ?\n> \n> Same for the constants above, shouldn't they go to ipu3_agc.h or\n> ipu3_awb.h ? Or better, if they're only used in the implementation, in\n> a .cpp file ?\n> \n\nPart of the constants and structures are used by awb, others by agc.\nIt sounds a bit overkill to have them in a separate header indeed, so I\nwill just drop this patch, and get back to ipu3_agc.cpp/ipu3_awb.cpp\nimplementation.\n\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>> +\n>>> +#endif /* __LIBCAMERA_IPA_IPU3_COMMON_H__ */\n>>>\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 028B3C3224\n\tfor ;\n\tFri, 9 Jul 2021 06:04:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4DD446851F;\n\tFri, 9 Jul 2021 08:04:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA86C6059F\n\tfor ;\n\tFri, 9 Jul 2021 08:04:10 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:d47a:9456:f481:a0f8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4B471E7;\n\tFri, 9 Jul 2021 08:04:10 +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=\"l5JSN1Fq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625810650;\n\tbh=fCdzUnekLqM653zY/+0OOOOZwiiw1eaKfcLqkvBsNng=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=l5JSN1Fqg9Epo+E51end9PDQJyUdUkEOFix1muJzVKHZis5oQ0CGuTSCrH8dxzgUd\n\tsEjefhqPOXO/GFmgeLaIZFyjMfvF3KVfO1nRXseIS5Mr2Ap2Oth4WMbnne1nQrFqgU\n\tDJ3BGwMfrI9oe3VKZZi0HvkFEwJ6dTM9zjochUyw=","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20210628202255.138874-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210628202255.138874-4-jeanmichel.hautbois@ideasonboard.com>\n\t<2905ae70-c166-1b48-13a0-28223174783f@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<634698a1-6ccc-216a-9732-691d1d9a0ee6@ideasonboard.com>","Date":"Fri, 9 Jul 2021 08:04:07 +0200","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":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v1 3/7] ipa: ipu3: Use a common IPU3\n\theader for the constants","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\" "}}]