[{"id":36834,"web_url":"https://patchwork.libcamera.org/comment/36834/","msgid":"","date":"2025-11-14T18:08:20","subject":"Re: [PATCH v4 05/21] ipa: libipa: fixedpoint: Fix unsigned usage","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta:\n> The fixedToFloatingPoint does not support unsigned Q types, and\n> incorrectly sign-extends all values which have the top most bit set in\n> the quantized values.\n\nAre none of the previously added tests affected?\n\n\n> \n> Fix this by ensuring that only signed types perform sign extension, and\n> simplify the calcuation for unsigned types.\n ^^^^^^^^^^\n calculation\n\n> \n> Convert the storage of the test cases to signed types to correctly\n> represent their intended purpose, to prevent test failures.\n\nI'm only seeing two uses of `fixedToFloatingPoint()` in total.\nBoth are in `src/ipa/mali-c55/algorithms/awb.cpp`. Those use `uint16_t`;\nare they affected by this change? The documentation just says `Q4.8`,\nbut I imagine they are unsigned actually.\n\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/libipa/fixedpoint.h | 3 +++\n> test/ipa/libipa/fixedpoint.cpp | 6 +++---\n> 2 files changed, 6 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h\n> index 17db8a026502..d97e6d6135ad 100644\n> --- a/src/ipa/libipa/fixedpoint.h\n> +++ b/src/ipa/libipa/fixedpoint.h\n> @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number)\n> \tstatic_assert(sizeof(int) >= sizeof(T));\n> \tstatic_assert(I + F <= sizeof(T) * 8);\n> \n> +\tif constexpr (std::is_unsigned_v)\n> +\t\treturn static_cast(number) / static_cast(1 << F);\n> +\n\nI'd add the rest to the `else` so as to avoid the \"instantiation\" of that part.\n\n> \t/*\n> \t * Recreate the upper bits in case of a negative number by shifting the sign\n> \t * bit from the fixed point to the first bit of the unsigned and then right shifting\n> diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp\n> index 935412771851..3f2d9ac97fe5 100644\n> --- a/test/ipa/libipa/fixedpoint.cpp\n> +++ b/test/ipa/libipa/fixedpoint.cpp\n> @@ -70,7 +70,7 @@ protected:\n> \t\t * The second 7.992 test is to test that unused bits don't\n> \t\t * affect the result.\n> \t\t */\n> -\t\tstd::map testCases = {\n> +\t\tstd::map testCases = {\n> \t\t\t{ 7.992, 0x3ff },\n> \t\t\t{ 0.2, 0x01a },\n> \t\t\t{ -0.2, 0x7e6 },\n> @@ -83,14 +83,14 @@ protected:\n> \n> \t\tint ret;\n> \t\tfor (const auto &testCase : testCases) {\n> -\t\t\tret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> +\t\t\tret = testSingleFixedPoint<4, 7, int16_t>(testCase.first,\n> \t\t\t\t\t\t\t\t testCase.second);\n> \t\t\tif (ret != TestPass)\n> \t\t\t\treturn ret;\n> \t\t}\n> \n> \t\t/* Special case with a superfluous one in the unused bits */\n> -\t\tret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992);\n> +\t\tret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992);\n> \t\tif (ret != TestPass)\n> \t\t\treturn ret;\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 C10F0C3263\n\tfor ;\n\tFri, 14 Nov 2025 18:08:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 724A260AA8;\n\tFri, 14 Nov 2025 19:08:26 +0100 (CET)","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 9335060A81\n\tfor ;\n\tFri, 14 Nov 2025 19:08:24 +0100 (CET)","from [192.168.33.35] (185.221.143.100.nat.pool.zt.hu\n\t[185.221.143.100])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B433397;\n\tFri, 14 Nov 2025 19:06:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oE/2yJ16\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763143583;\n\tbh=d3VATirOgvJrPGNIZvN7P/Sobf46uj6zWUfwajItxYI=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=oE/2yJ16RGFjGIfqTef33XrQ69vqZmMNvyHIP8bJbGudak2yKGkGMsOhLDt7jsgII\n\t12Oaugpo20qsM4Y7yp7UUoj2Ji3CUrbYp/KpBu4v2owCudKkXJFZzQhO+IpABtUUr4\n\tQVnKxBymxcp08vRib1QnhpirW/yEa7BKuslejFbs=","Message-ID":"","Date":"Fri, 14 Nov 2025 19:08:20 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 05/21] ipa: libipa: fixedpoint: Fix unsigned usage","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-6-kieran.bingham@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251114005428.90024-6-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":36871,"web_url":"https://patchwork.libcamera.org/comment/36871/","msgid":"<176346949960.880260.15723634492359864609@isaac-ThinkPad-T16-Gen-2>","date":"2025-11-18T12:38:19","subject":"Re: [PATCH v4 05/21] ipa: libipa: fixedpoint: Fix unsigned usage","submitter":{"id":215,"url":"https://patchwork.libcamera.org/api/people/215/","name":"Isaac Scott","email":"isaac.scott@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch!\n\nReviewed-by: Isaac Scott \n\nQuoting Kieran Bingham (2025-11-14 00:54:09)\n> The fixedToFloatingPoint does not support unsigned Q types, and\n> incorrectly sign-extends all values which have the top most bit set in\n> the quantized values.\n> \n> Fix this by ensuring that only signed types perform sign extension, and\n> simplify the calcuation for unsigned types.\n> \n> Convert the storage of the test cases to signed types to correctly\n> represent their intended purpose, to prevent test failures.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/libipa/fixedpoint.h | 3 +++\n> test/ipa/libipa/fixedpoint.cpp | 6 +++---\n> 2 files changed, 6 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h\n> index 17db8a026502..d97e6d6135ad 100644\n> --- a/src/ipa/libipa/fixedpoint.h\n> +++ b/src/ipa/libipa/fixedpoint.h\n> @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number)\n> static_assert(sizeof(int) >= sizeof(T));\n> static_assert(I + F <= sizeof(T) * 8);\n> \n> + if constexpr (std::is_unsigned_v)\n> + return static_cast(number) / static_cast(1 << F);\n> +\n> /*\n> * Recreate the upper bits in case of a negative number by shifting the sign\n> * bit from the fixed point to the first bit of the unsigned and then right shifting\n> diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp\n> index 935412771851..3f2d9ac97fe5 100644\n> --- a/test/ipa/libipa/fixedpoint.cpp\n> +++ b/test/ipa/libipa/fixedpoint.cpp\n> @@ -70,7 +70,7 @@ protected:\n> * The second 7.992 test is to test that unused bits don't\n> * affect the result.\n> */\n> - std::map testCases = {\n> + std::map testCases = {\n> { 7.992, 0x3ff },\n> { 0.2, 0x01a },\n> { -0.2, 0x7e6 },\n> @@ -83,14 +83,14 @@ protected:\n> \n> int ret;\n> for (const auto &testCase : testCases) {\n> - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> + ret = testSingleFixedPoint<4, 7, int16_t>(testCase.first,\n> testCase.second);\n> if (ret != TestPass)\n> return ret;\n> }\n> \n> /* Special case with a superfluous one in the unused bits */\n> - ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992);\n> + ret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992);\n> if (ret != TestPass)\n> return ret;\n> \n> -- \n> 2.51.1\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 BE69FBD80A\n\tfor ;\n\tTue, 18 Nov 2025 12:38:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B2BA60AA0;\n\tTue, 18 Nov 2025 13:38:24 +0100 (CET)","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 203FC606D5\n\tfor ;\n\tTue, 18 Nov 2025 13:38:22 +0100 (CET)","from thinkpad.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5BE7BD52;\n\tTue, 18 Nov 2025 13:36:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VlkPS1Xi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763469378;\n\tbh=L+LOlzJ19bDtjRd1PYUmkSLYYgL1f2qmE4pjA2byKDc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VlkPS1XijK5Mva9fh6uqFJaygyFc1qBt+GiDpe9rHferqs+JnrEEv/+vnvQ1qERAv\n\tRXD49wbzRbBaBDwttcULChg+NxvMQX+Tn9MArpG/oq+T3WfBS26dXyNLMpu/E/T4mJ\n\tFHBNeGGc3oK6EuSZZ+IpqTPwbKbZ3ilJgb4Sorfs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251114005428.90024-6-kieran.bingham@ideasonboard.com>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-6-kieran.bingham@ideasonboard.com>","Subject":"Re: [PATCH v4 05/21] ipa: libipa: fixedpoint: Fix unsigned usage","From":"Isaac Scott ","Cc":"Kieran Bingham ","To":"Kieran Bingham ,\n\tlibcamera devel ","Date":"Tue, 18 Nov 2025 12:38:19 +0000","Message-ID":"<176346949960.880260.15723634492359864609@isaac-ThinkPad-T16-Gen-2>","User-Agent":"alot/0.10","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":36891,"web_url":"https://patchwork.libcamera.org/comment/36891/","msgid":"<176350966690.3013501.13237551075260414385@ping.linuxembedded.co.uk>","date":"2025-11-18T23:47:46","subject":"Re: [PATCH v4 05/21] ipa: libipa: fixedpoint: Fix unsigned usage","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-11-14 18:08:20)\n> 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta:\n> > The fixedToFloatingPoint does not support unsigned Q types, and\n> > incorrectly sign-extends all values which have the top most bit set in\n> > the quantized values.\n> \n> Are none of the previously added tests affected?\n> \n\nThe addition of \n\n+ if constexpr (std::is_unsigned_v)\n+ return static_cast(number) / static_cast(1 << F);\n\nFixes any issues.\n\n\n> > Fix this by ensuring that only signed types perform sign extension, and\n> > simplify the calcuation for unsigned types.\n> ^^^^^^^^^^\n> calculation\n\nThanks\n\n\n> > Convert the storage of the test cases to signed types to correctly\n> > represent their intended purpose, to prevent test failures.\n> \n> I'm only seeing two uses of `fixedToFloatingPoint()` in total.\n> Both are in `src/ipa/mali-c55/algorithms/awb.cpp`. Those use `uint16_t`;\n> are they affected by this change? The documentation just says `Q4.8`,\n> but I imagine they are unsigned actually.\n> \n\nIndeed, they are signed types. The implementation prior to this series\nstores 'signed' types in an unsigned storage. That's why it has to do\ninternal signed bit extensions and was a big awkward part until I\nrealised ... I could now remove it all - because I use signed/unsigned\nstorage explicitly (which I much prefer).\n\nSee the patches towards the end of the series.\n\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/libipa/fixedpoint.h | 3 +++\n> > test/ipa/libipa/fixedpoint.cpp | 6 +++---\n> > 2 files changed, 6 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h\n> > index 17db8a026502..d97e6d6135ad 100644\n> > --- a/src/ipa/libipa/fixedpoint.h\n> > +++ b/src/ipa/libipa/fixedpoint.h\n> > @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number)\n> > static_assert(sizeof(int) >= sizeof(T));\n> > static_assert(I + F <= sizeof(T) * 8);\n> > \n> > + if constexpr (std::is_unsigned_v)\n> > + return static_cast(number) / static_cast(1 << F);\n> > +\n> \n> I'd add the rest to the `else` so as to avoid the \"instantiation\" of that part.\n\nDoes the compiler not know to skip it ?\nIt puts an undesirable extra level of indentation on the rest of the\ncode block :S\n\n\n> \n> > /*\n> > * Recreate the upper bits in case of a negative number by shifting the sign\n> > * bit from the fixed point to the first bit of the unsigned and then right shifting\n> > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp\n> > index 935412771851..3f2d9ac97fe5 100644\n> > --- a/test/ipa/libipa/fixedpoint.cpp\n> > +++ b/test/ipa/libipa/fixedpoint.cpp\n> > @@ -70,7 +70,7 @@ protected:\n> > * The second 7.992 test is to test that unused bits don't\n> > * affect the result.\n> > */\n> > - std::map testCases = {\n> > + std::map testCases = {\n> > { 7.992, 0x3ff },\n> > { 0.2, 0x01a },\n> > { -0.2, 0x7e6 },\n> > @@ -83,14 +83,14 @@ protected:\n> > \n> > int ret;\n> > for (const auto &testCase : testCases) {\n> > - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> > + ret = testSingleFixedPoint<4, 7, int16_t>(testCase.first,\n> > testCase.second);\n> > if (ret != TestPass)\n> > return ret;\n> > }\n> > \n> > /* Special case with a superfluous one in the unused bits */\n> > - ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992);\n> > + ret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992);\n> > if (ret != TestPass)\n> > return ret;\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 88A19BD80A\n\tfor ;\n\tTue, 18 Nov 2025 23:47:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2F9660A9D;\n\tWed, 19 Nov 2025 00:47:51 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C83860856\n\tfor ;\n\tWed, 19 Nov 2025 00:47:50 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 07471BCA;\n\tWed, 19 Nov 2025 00:45:45 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"p5VREfs9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763509546;\n\tbh=A3rCza7Igk8a7Ky2A36b3jV2VcMirOEK7gl1irzvqUE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=p5VREfs9WRkGhBva4RJFOzKb7y6M3LcJtTV8OAoV1zunlT4cBptsVSSHOGW+X/MCv\n\tUSNLNylOih3/O8BiBHKq5xD/vxHrbb/wbzlgtwWiCR4nr+YWsspb5ercu3e444hvUq\n\tN0XjTK/2egggsTJC9okulIZCwdRQcSRhWhvbpUNs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-6-kieran.bingham@ideasonboard.com>\n\t","Subject":"Re: [PATCH v4 05/21] ipa: libipa: fixedpoint: Fix unsigned usage","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera devel ","Date":"Tue, 18 Nov 2025 23:47:46 +0000","Message-ID":"<176350966690.3013501.13237551075260414385@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":36892,"web_url":"https://patchwork.libcamera.org/comment/36892/","msgid":"<176351567578.3013501.8787045411898357419@ping.linuxembedded.co.uk>","date":"2025-11-19T01:27:55","subject":"Re: [PATCH v4 05/21] ipa: libipa: fixedpoint: Fix unsigned usage","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2025-11-18 23:47:46)\n> Quoting Barnabás Pőcze (2025-11-14 18:08:20)\n> > 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta:\n> > > The fixedToFloatingPoint does not support unsigned Q types, and\n> > > incorrectly sign-extends all values which have the top most bit set in\n> > > the quantized values.\n> > \n> > Are none of the previously added tests affected?\n> > \n> \n> The addition of \n> \n> + if constexpr (std::is_unsigned_v)\n> + return static_cast(number) / static_cast(1 << F);\n> \n> Fixes any issues.\n\nHowever, now that I've added the static assert for min < max - it\ntriggers in the build history!\n\nhttps://gitlab.freedesktop.org/camera/libcamera/-/jobs/88124955#L1323\n\nSo good spot - it did break before here.\n\nI'll move this fix to before introduction of the FixedPointQ types.\n\n\n\n> \n> \n> > > Fix this by ensuring that only signed types perform sign extension, and\n> > > simplify the calcuation for unsigned types.\n> > ^^^^^^^^^^\n> > calculation\n> \n> Thanks\n> \n> \n> > > Convert the storage of the test cases to signed types to correctly\n> > > represent their intended purpose, to prevent test failures.\n> > \n> > I'm only seeing two uses of `fixedToFloatingPoint()` in total.\n> > Both are in `src/ipa/mali-c55/algorithms/awb.cpp`. Those use `uint16_t`;\n> > are they affected by this change? The documentation just says `Q4.8`,\n> > but I imagine they are unsigned actually.\n> > \n> \n> Indeed, they are signed types. The implementation prior to this series\n> stores 'signed' types in an unsigned storage. That's why it has to do\n> internal signed bit extensions and was a big awkward part until I\n> realised ... I could now remove it all - because I use signed/unsigned\n> storage explicitly (which I much prefer).\n> \n> See the patches towards the end of the series.\n> \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > src/ipa/libipa/fixedpoint.h | 3 +++\n> > > test/ipa/libipa/fixedpoint.cpp | 6 +++---\n> > > 2 files changed, 6 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h\n> > > index 17db8a026502..d97e6d6135ad 100644\n> > > --- a/src/ipa/libipa/fixedpoint.h\n> > > +++ b/src/ipa/libipa/fixedpoint.h\n> > > @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number)\n> > > static_assert(sizeof(int) >= sizeof(T));\n> > > static_assert(I + F <= sizeof(T) * 8);\n> > > \n> > > + if constexpr (std::is_unsigned_v)\n> > > + return static_cast(number) / static_cast(1 << F);\n> > > +\n> > \n> > I'd add the rest to the `else` so as to avoid the \"instantiation\" of that part.\n> \n> Does the compiler not know to skip it ?\n> It puts an undesirable extra level of indentation on the rest of the\n> code block :S\n> \n> \n> > \n> > > /*\n> > > * Recreate the upper bits in case of a negative number by shifting the sign\n> > > * bit from the fixed point to the first bit of the unsigned and then right shifting\n> > > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp\n> > > index 935412771851..3f2d9ac97fe5 100644\n> > > --- a/test/ipa/libipa/fixedpoint.cpp\n> > > +++ b/test/ipa/libipa/fixedpoint.cpp\n> > > @@ -70,7 +70,7 @@ protected:\n> > > * The second 7.992 test is to test that unused bits don't\n> > > * affect the result.\n> > > */\n> > > - std::map testCases = {\n> > > + std::map testCases = {\n> > > { 7.992, 0x3ff },\n> > > { 0.2, 0x01a },\n> > > { -0.2, 0x7e6 },\n> > > @@ -83,14 +83,14 @@ protected:\n> > > \n> > > int ret;\n> > > for (const auto &testCase : testCases) {\n> > > - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> > > + ret = testSingleFixedPoint<4, 7, int16_t>(testCase.first,\n> > > testCase.second);\n> > > if (ret != TestPass)\n> > > return ret;\n> > > }\n> > > \n> > > /* Special case with a superfluous one in the unused bits */\n> > > - ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992);\n> > > + ret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992);\n> > > if (ret != TestPass)\n> > > return ret;\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 A9725C0F1B\n\tfor ;\n\tWed, 19 Nov 2025 01:28:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB63A60A8B;\n\tWed, 19 Nov 2025 02:28:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F8D860856\n\tfor ;\n\tWed, 19 Nov 2025 02:27:59 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ADB6A195E;\n\tWed, 19 Nov 2025 02:25:54 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fjqjv0oq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763515554;\n\tbh=CXapCCKQmI1uBU81/0yDNHh1PtuAMA5S4mw4Yrx91tQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=fjqjv0oq7IjJW2jKc0wH6OBpywo92lsZD1FuUtleeXcGTxHA6OO/Z4qt4RiTzlaSl\n\tfIQeuQPypZ1htQ++e7oVsF0kmX3fbQqIhsDnIVody5KPjQmz0crmnDxO8K8E5yxE9R\n\tmvJf3/uoL6G5Cmw6c4SH5K/7b3CPigcfnd2dTc0k=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176350966690.3013501.13237551075260414385@ping.linuxembedded.co.uk>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-6-kieran.bingham@ideasonboard.com>\n\t\n\t<176350966690.3013501.13237551075260414385@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v4 05/21] ipa: libipa: fixedpoint: Fix unsigned usage","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera devel ","Date":"Wed, 19 Nov 2025 01:27:55 +0000","Message-ID":"<176351567578.3013501.8787045411898357419@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":36931,"web_url":"https://patchwork.libcamera.org/comment/36931/","msgid":"<1d190a4d-023c-4c25-8dce-fe39eb15fa57@ideasonboard.com>","date":"2025-11-20T09:39:42","subject":"Re: [PATCH v4 05/21] ipa: libipa: fixedpoint: Fix unsigned usage","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 11. 19. 0:47 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-11-14 18:08:20)\n>> 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta:\n>>> The fixedToFloatingPoint does not support unsigned Q types, and\n>>> incorrectly sign-extends all values which have the top most bit set in\n>>> the quantized values.\n>>\n>> Are none of the previously added tests affected?\n>>\n> \n> The addition of\n> \n> + if constexpr (std::is_unsigned_v)\n> + return static_cast(number) / static_cast(1 << F);\n> \n> Fixes any issues.\n> \n> \n>>> Fix this by ensuring that only signed types perform sign extension, and\n>>> simplify the calcuation for unsigned types.\n>> ^^^^^^^^^^\n>> calculation\n> \n> Thanks\n> \n> \n>>> Convert the storage of the test cases to signed types to correctly\n>>> represent their intended purpose, to prevent test failures.\n>>\n>> I'm only seeing two uses of `fixedToFloatingPoint()` in total.\n>> Both are in `src/ipa/mali-c55/algorithms/awb.cpp`. Those use `uint16_t`;\n>> are they affected by this change? The documentation just says `Q4.8`,\n>> but I imagine they are unsigned actually.\n>>\n> \n> Indeed, they are signed types. The implementation prior to this series\n> stores 'signed' types in an unsigned storage. That's why it has to do\n> internal signed bit extensions and was a big awkward part until I\n> realised ... I could now remove it all - because I use signed/unsigned\n> storage explicitly (which I much prefer).\n> \n> See the patches towards the end of the series.\n> \n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>> src/ipa/libipa/fixedpoint.h | 3 +++\n>>> test/ipa/libipa/fixedpoint.cpp | 6 +++---\n>>> 2 files changed, 6 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h\n>>> index 17db8a026502..d97e6d6135ad 100644\n>>> --- a/src/ipa/libipa/fixedpoint.h\n>>> +++ b/src/ipa/libipa/fixedpoint.h\n>>> @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number)\n>>> static_assert(sizeof(int) >= sizeof(T));\n>>> static_assert(I + F <= sizeof(T) * 8);\n>>> \n>>> + if constexpr (std::is_unsigned_v)\n>>> + return static_cast(number) / static_cast(1 << F);\n>>> +\n>>\n>> I'd add the rest to the `else` so as to avoid the \"instantiation\" of that part.\n> \n> Does the compiler not know to skip it ?\n> It puts an undesirable extra level of indentation on the rest of the\n> code block :S\n\nWell, the definition of `if constexpr` means that it will have a different effect.\nIn any case, it's not significant here; I was mainly just wondering if the two paths\ncould be more unified, but it's probably fine as is.\n\n\n\n> \n> \n>>\n>>> /*\n>>> * Recreate the upper bits in case of a negative number by shifting the sign\n>>> * bit from the fixed point to the first bit of the unsigned and then right shifting\n>>> diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp\n>>> index 935412771851..3f2d9ac97fe5 100644\n>>> --- a/test/ipa/libipa/fixedpoint.cpp\n>>> +++ b/test/ipa/libipa/fixedpoint.cpp\n>>> @@ -70,7 +70,7 @@ protected:\n>>> * The second 7.992 test is to test that unused bits don't\n>>> * affect the result.\n>>> */\n>>> - std::map testCases = {\n>>> + std::map testCases = {\n>>> { 7.992, 0x3ff },\n>>> { 0.2, 0x01a },\n>>> { -0.2, 0x7e6 },\n>>> @@ -83,14 +83,14 @@ protected:\n>>> \n>>> int ret;\n>>> for (const auto &testCase : testCases) {\n>>> - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n>>> + ret = testSingleFixedPoint<4, 7, int16_t>(testCase.first,\n>>> testCase.second);\n>>> if (ret != TestPass)\n>>> return ret;\n>>> }\n>>> \n>>> /* Special case with a superfluous one in the unused bits */\n>>> - ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992);\n>>> + ret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992);\n>>> if (ret != TestPass)\n>>> return ret;\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 DE6E8C0F1B\n\tfor ;\n\tThu, 20 Nov 2025 09:39:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6E7560A8B;\n\tThu, 20 Nov 2025 10:39:58 +0100 (CET)","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 21290606E6\n\tfor ;\n\tThu, 20 Nov 2025 10:39:57 +0100 (CET)","from [192.168.33.37] (185.221.143.100.nat.pool.zt.hu\n\t[185.221.143.100])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A1E28B3;\n\tThu, 20 Nov 2025 10:37:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jUF0YWIG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763631471;\n\tbh=c3zuVjAzEwTFyEy0ZqV9S4sqWzCM9weFWpR0S9MRcq0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=jUF0YWIG/kCrnu0dn1v1BxEMy1z7Ohsdv2/j2YM8LiVtbAU6mJNT6A0VxAY6COqCH\n\tPfO7sKPlKkslYPOXgBQDYQuUg5rJuZ4nG79RVCfHQE3p6nvl4Yi0rUTsuAqreyhre1\n\ttyZ9iPMEnkQK3O0w4ane/cWBzA1z/ha6AiYuVWhc=","Message-ID":"<1d190a4d-023c-4c25-8dce-fe39eb15fa57@ideasonboard.com>","Date":"Thu, 20 Nov 2025 10:39:42 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 05/21] ipa: libipa: fixedpoint: Fix unsigned usage","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-6-kieran.bingham@ideasonboard.com>\n\t\n\t<176350966690.3013501.13237551075260414385@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<176350966690.3013501.13237551075260414385@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]