[{"id":29674,"web_url":"https://patchwork.libcamera.org/comment/29674/","msgid":"<6b4icccczq7wma6td4lvbjge4jl7umfu25lmodnj56lkd3mtag@osrpvshhk6td>","date":"2024-05-30T14:23:19","subject":"Re: [PATCH v4] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nthanks for the fast update.\n\nOn Thu, May 30, 2024 at 09:39:34PM +0900, Paul Elder wrote:\n> Add helper functions for converting between floating point and fixed\n> point numbers. Also add tests for them.\n> \n> Signed-off-by: Paul Elder \n> ---\n> Changes in v4:\n> - relax the test case floating point precision\n> - optimize the fixed -> floating point converter\n> \n> Changes in v3:\n> - (used to be \"libcamera: utils: Add a helper to convert floating-point to fixed-point\")\n> - move the everything from libcamera global utils to the rkisp1 ipa\n> \n> Changes in v2:\n> - added the reverse conversion function (fixed -> floating)\n> - added tests\n> - make the conversion code cleaner\n> ---\n> src/ipa/rkisp1/meson.build | 1 +\n> src/ipa/rkisp1/utils.cpp | 40 +++++++++++++++\n> src/ipa/rkisp1/utils.h | 66 +++++++++++++++++++++++++\n> test/ipa/meson.build | 2 +\n> test/ipa/rkisp1/meson.build | 15 ++++++\n> test/ipa/rkisp1/rkisp1-utils.cpp | 84 ++++++++++++++++++++++++++++++++\n> 6 files changed, 208 insertions(+)\n> create mode 100644 src/ipa/rkisp1/utils.cpp\n> create mode 100644 src/ipa/rkisp1/utils.h\n> create mode 100644 test/ipa/rkisp1/meson.build\n> create mode 100644 test/ipa/rkisp1/rkisp1-utils.cpp\n> \n> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> index e813da53a..cf05cdb27 100644\n> --- a/src/ipa/rkisp1/meson.build\n> +++ b/src/ipa/rkisp1/meson.build\n> @@ -8,6 +8,7 @@ ipa_name = 'ipa_rkisp1'\n> rkisp1_ipa_sources = files([\n> 'ipa_context.cpp',\n> 'rkisp1.cpp',\n> + 'utils.cpp',\n> ])\n> \n> rkisp1_ipa_sources += rkisp1_ipa_algorithms\n> diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp\n> new file mode 100644\n> index 000000000..53186fdd6\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/utils.cpp\n> @@ -0,0 +1,40 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Miscellaneous utility functions specific to rkisp1\n> + */\n> +\n> +#include \"utils.h\"\n> +\n> +/**\n> + * \\file utils.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::utils {\n> +\n> +/**\n> + * \\fn R floatingToFixedPoint(T number)\n> + * \\brief Convert a floating point number to a fixed-point representation\n> + * \\tparam I Bit width of the integer part of the fixed-point\n> + * \\tparam F Bit width of the fractional part of the fixed-point\n> + * \\tparam R Return type of the fixed-point representation\n> + * \\tparam T Input type of the floating point representation\n> + * \\return The converted value\n> + */\n> +\n> +/**\n> + * \\fn R fixedToFloatingPoint(T number)\n> + * \\brief Convert a fixed-point number to a floating point representation\n> + * \\tparam I Bit width of the integer part of the fixed-point\n> + * \\tparam F Bit width of the fractional part of the fixed-point\n> + * \\tparam R Return type of the floating point representation\n> + * \\tparam T Input type of the fixed-point representation\n> + * \\return The converted value\n> + */\n> +\n> +} /* namespace ipa::rkisp1::utils */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/utils.h b/src/ipa/rkisp1/utils.h\n> new file mode 100644\n> index 000000000..450f22442\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/utils.h\n> @@ -0,0 +1,66 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Miscellaneous utility functions specific to rkisp1\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::utils {\n> +\n> +#ifndef __DOXYGEN__\n> +template +\t std::enable_if_t &&\n> +\t\t\t std::is_floating_point_v> * = nullptr>\n> +#else\n> +template\n> +#endif\n> +constexpr R floatingToFixedPoint(T number)\n> +{\n> +\tstatic_assert(sizeof(int) >= sizeof(R));\n> +\tstatic_assert(I + F <= sizeof(R) * 8);\n> +\n> +\t/*\n> +\t * The intermediate cast to int is needed on arm platforms to properly\n> +\t * cast negative values. See\n> +\t * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> +\t */\n> +\tR mask = (1 << (F + I)) - 1;\n> +\tR frac = static_cast(static_cast(std::round(number * (1 << F)))) & mask;\n> +\n> +\treturn frac;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template +\t std::enable_if_t &&\n> +\t\t\t std::is_integral_v> * = nullptr>\n> +#else\n> +template\n> +#endif\n> +constexpr R fixedToFloatingPoint(T number)\n> +{\n> +\tstatic_assert(sizeof(int) >= sizeof(T));\n> +\tstatic_assert(I + F <= sizeof(T) * 8);\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> +\t * by the same amount which keeps the sign bit in place.\n> +\t * This can be optimized by the compiler quite well.\n> +\t */\n> +\tint remaining_bits = sizeof(int) * 8 - (I + F);\n> +\tint t = static_cast(static_cast(number) << remaining_bits) >> remaining_bits;\n> +\treturn static_cast(t) / static_cast(1 << F);\n> +}\n> +\n> +} /* namespace ipa::rkisp1::utils */\n> +\n> +} /* namespace libcamera */\n> diff --git a/test/ipa/meson.build b/test/ipa/meson.build\n> index 180b0da0a..dc956284c 100644\n> --- a/test/ipa/meson.build\n> +++ b/test/ipa/meson.build\n> @@ -1,5 +1,7 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> +subdir('rkisp1')\n> +\n> ipa_test = [\n> {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> diff --git a/test/ipa/rkisp1/meson.build b/test/ipa/rkisp1/meson.build\n> new file mode 100644\n> index 000000000..5ffc5dd60\n> --- /dev/null\n> +++ b/test/ipa/rkisp1/meson.build\n> @@ -0,0 +1,15 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +rkisp1_ipa_test = [\n> + {'name': 'rkisp1-utils', 'sources': ['rkisp1-utils.cpp']},\n> +]\n> +\n> +foreach test : rkisp1_ipa_test\n> + exe = executable(test['name'], test['sources'], libcamera_generated_ipa_headers,\n> + dependencies : libcamera_private,\n> + link_with : [libipa, test_libraries],\n> + include_directories : [libipa_includes, test_includes_internal,\n> + '../../../src/ipa/rkisp1/'])\n> +\n> + test(test['name'], exe, suite : 'ipa')\n> +endforeach\n> diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp\n> new file mode 100644\n> index 000000000..d23d87e0f\n> --- /dev/null\n> +++ b/test/ipa/rkisp1/rkisp1-utils.cpp\n> @@ -0,0 +1,84 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Miscellaneous utility tests\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"../src/ipa/rkisp1/utils.h\"\n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +using namespace ipa::rkisp1;\n> +\n> +class RkISP1UtilsTest : public Test\n> +{\n> +protected:\n> +\ttemplate\n> +\tint testSingleFixedPoint(double input, T expected)\n> +\t{\n> +\t\tT ret = utils::floatingToFixedPoint(input);\n> +\t\tif (ret != expected) {\n> +\t\t\tcerr << \"Expected \" << input << \" to convert to \"\n> +\t\t\t << expected << \", got \" << ret << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * The precision check is fairly arbitrary but is based on what\n> +\t\t * the rkisp1 is capable of in the crosstalk module.\n> +\t\t */\n> +\t\tdouble f = utils::fixedToFloatingPoint(ret);\n> +\t\tif (std::abs(f - input) > 0.005) {\n> +\t\t\tcerr << \"Reverse conversion expected \" << ret\n> +\t\t\t << \" to convert to \" << input\n> +\t\t\t << \", got \" << f << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint testFixedPoint()\n> +\t{\n> +\t\t/* These are the only cases that we know for certain */\n\nNit: This comment no longer holds :-)\n\nNo need to send another patch.\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> +\t\tstd::map testCases = {\n> +\t\t\t{ 7.992, 0x3FF },\n> +\t\t\t{ 7.992, 0xBFF },\n> +\t\t\t{ 0.2, 0x01A },\n> +\t\t\t{ -0.2, 0x7E6 },\n> +\t\t\t{ -0.8, 0x79A },\n> +\t\t\t{ -0.4, 0x7CD },\n> +\t\t\t{ -1.4, 0x74D },\n> +\t\t\t{ -8, 0x400 },\n> +\t\t\t{ 0, 0 },\n> +\t\t};\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\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\treturn TestPass;\n> +\t}\n> +\n> +\tint run()\n> +\t{\n> +\t\t/* fixed point conversion test */\n> +\t\tif (testFixedPoint() != TestPass)\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +};\n> +\n> +TEST_REGISTER(RkISP1UtilsTest)\n> -- \n> 2.39.2\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 C97DEBD87C\n\tfor ;\n\tThu, 30 May 2024 14:23:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2F5C634AF;\n\tThu, 30 May 2024 16:23:24 +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 566DA61A43\n\tfor ;\n\tThu, 30 May 2024 16:23:22 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:741c:d46c:4301:49ad])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DE6ECA06;\n\tThu, 30 May 2024 16:23:17 +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=\"iEPsf8+m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717078997;\n\tbh=EVur0D2kK2g0vBuOVVKP/3ZHILbSGLX+lJgw/wNrR08=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iEPsf8+m5WQNnjDS63vmQbGR+xt54No6/K28fxiOjJX6QTHosnsL31WHqSuFP25qo\n\tmb9E9N1POhh+rYACELGskrC0J233gi9q5H7YUbF/cfUETbBxhzG8XCx7K6ywMfCkrv\n\tfGeHXajft4DGUJDFvDxdv5M9ptUonZNe6S8L+xTI=","Date":"Thu, 30 May 2024 16:23:19 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","Message-ID":"<6b4icccczq7wma6td4lvbjge4jl7umfu25lmodnj56lkd3mtag@osrpvshhk6td>","References":"<20240530123934.2325922-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240530123934.2325922-1-paul.elder@ideasonboard.com>","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":29685,"web_url":"https://patchwork.libcamera.org/comment/29685/","msgid":"<171710944394.372008.2276426537269290224@ping.linuxembedded.co.uk>","date":"2024-05-30T22:50:43","subject":"Re: [PATCH v4] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-05-30 15:23:19)\n> Hi Paul,\n> \n> thanks for the fast update.\n> \n> On Thu, May 30, 2024 at 09:39:34PM +0900, Paul Elder wrote:\n> > Add helper functions for converting between floating point and fixed\n> > point numbers. Also add tests for them.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > Changes in v4:\n> > - relax the test case floating point precision\n> > - optimize the fixed -> floating point converter\n> > \n> > Changes in v3:\n> > - (used to be \"libcamera: utils: Add a helper to convert floating-point to fixed-point\")\n> > - move the everything from libcamera global utils to the rkisp1 ipa\n> > \n> > Changes in v2:\n> > - added the reverse conversion function (fixed -> floating)\n> > - added tests\n> > - make the conversion code cleaner\n> > ---\n> > src/ipa/rkisp1/meson.build | 1 +\n> > src/ipa/rkisp1/utils.cpp | 40 +++++++++++++++\n> > src/ipa/rkisp1/utils.h | 66 +++++++++++++++++++++++++\n> > test/ipa/meson.build | 2 +\n> > test/ipa/rkisp1/meson.build | 15 ++++++\n> > test/ipa/rkisp1/rkisp1-utils.cpp | 84 ++++++++++++++++++++++++++++++++\n> > 6 files changed, 208 insertions(+)\n> > create mode 100644 src/ipa/rkisp1/utils.cpp\n> > create mode 100644 src/ipa/rkisp1/utils.h\n> > create mode 100644 test/ipa/rkisp1/meson.build\n> > create mode 100644 test/ipa/rkisp1/rkisp1-utils.cpp\n> > \n> > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> > index e813da53a..cf05cdb27 100644\n> > --- a/src/ipa/rkisp1/meson.build\n> > +++ b/src/ipa/rkisp1/meson.build\n> > @@ -8,6 +8,7 @@ ipa_name = 'ipa_rkisp1'\n> > rkisp1_ipa_sources = files([\n> > 'ipa_context.cpp',\n> > 'rkisp1.cpp',\n> > + 'utils.cpp',\n> > ])\n> > \n> > rkisp1_ipa_sources += rkisp1_ipa_algorithms\n> > diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp\n> > new file mode 100644\n> > index 000000000..53186fdd6\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/utils.cpp\n> > @@ -0,0 +1,40 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * Miscellaneous utility functions specific to rkisp1\n> > + */\n> > +\n> > +#include \"utils.h\"\n> > +\n> > +/**\n> > + * \\file utils.h\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::utils {\n> > +\n> > +/**\n> > + * \\fn R floatingToFixedPoint(T number)\n> > + * \\brief Convert a floating point number to a fixed-point representation\n> > + * \\tparam I Bit width of the integer part of the fixed-point\n> > + * \\tparam F Bit width of the fractional part of the fixed-point\n> > + * \\tparam R Return type of the fixed-point representation\n> > + * \\tparam T Input type of the floating point representation\n> > + * \\return The converted value\n> > + */\n> > +\n> > +/**\n> > + * \\fn R fixedToFloatingPoint(T number)\n> > + * \\brief Convert a fixed-point number to a floating point representation\n> > + * \\tparam I Bit width of the integer part of the fixed-point\n> > + * \\tparam F Bit width of the fractional part of the fixed-point\n> > + * \\tparam R Return type of the floating point representation\n> > + * \\tparam T Input type of the fixed-point representation\n> > + * \\return The converted value\n> > + */\n> > +\n> > +} /* namespace ipa::rkisp1::utils */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/utils.h b/src/ipa/rkisp1/utils.h\n> > new file mode 100644\n> > index 000000000..450f22442\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/utils.h\n> > @@ -0,0 +1,66 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * Miscellaneous utility functions specific to rkisp1\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::utils {\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template > + std::enable_if_t &&\n> > + std::is_floating_point_v> * = nullptr>\n> > +#else\n> > +template\n> > +#endif\n> > +constexpr R floatingToFixedPoint(T number)\n> > +{\n> > + static_assert(sizeof(int) >= sizeof(R));\n> > + static_assert(I + F <= sizeof(R) * 8);\n> > +\n> > + /*\n> > + * The intermediate cast to int is needed on arm platforms to properly\n> > + * cast negative values. See\n> > + * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> > + */\n> > + R mask = (1 << (F + I)) - 1;\n> > + R frac = static_cast(static_cast(std::round(number * (1 << F)))) & mask;\n> > +\n> > + return frac;\n> > +}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template > + std::enable_if_t &&\n> > + std::is_integral_v> * = nullptr>\n> > +#else\n> > +template\n> > +#endif\n> > +constexpr R fixedToFloatingPoint(T number)\n> > +{\n> > + static_assert(sizeof(int) >= sizeof(T));\n> > + static_assert(I + F <= sizeof(T) * 8);\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> > + * by the same amount which keeps the sign bit in place.\n> > + * This can be optimized by the compiler quite well.\n> > + */\n> > + int remaining_bits = sizeof(int) * 8 - (I + F);\n> > + int t = static_cast(static_cast(number) << remaining_bits) >> remaining_bits;\n> > + return static_cast(t) / static_cast(1 << F);\n> > +}\n> > +\n> > +} /* namespace ipa::rkisp1::utils */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/test/ipa/meson.build b/test/ipa/meson.build\n> > index 180b0da0a..dc956284c 100644\n> > --- a/test/ipa/meson.build\n> > +++ b/test/ipa/meson.build\n> > @@ -1,5 +1,7 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> > \n> > +subdir('rkisp1')\n> > +\n> > ipa_test = [\n> > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> > diff --git a/test/ipa/rkisp1/meson.build b/test/ipa/rkisp1/meson.build\n> > new file mode 100644\n> > index 000000000..5ffc5dd60\n> > --- /dev/null\n> > +++ b/test/ipa/rkisp1/meson.build\n> > @@ -0,0 +1,15 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +rkisp1_ipa_test = [\n> > + {'name': 'rkisp1-utils', 'sources': ['rkisp1-utils.cpp']},\n> > +]\n> > +\n> > +foreach test : rkisp1_ipa_test\n> > + exe = executable(test['name'], test['sources'], libcamera_generated_ipa_headers,\n> > + dependencies : libcamera_private,\n> > + link_with : [libipa, test_libraries],\n> > + include_directories : [libipa_includes, test_includes_internal,\n> > + '../../../src/ipa/rkisp1/'])\n> > +\n> > + test(test['name'], exe, suite : 'ipa')\n> > +endforeach\n> > diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp\n> > new file mode 100644\n> > index 000000000..d23d87e0f\n> > --- /dev/null\n> > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp\n> > @@ -0,0 +1,84 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * Miscellaneous utility tests\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"../src/ipa/rkisp1/utils.h\"\n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> > +using namespace libcamera;\n> > +using namespace ipa::rkisp1;\n> > +\n> > +class RkISP1UtilsTest : public Test\n> > +{\n> > +protected:\n> > + template\n> > + int testSingleFixedPoint(double input, T expected)\n> > + {\n> > + T ret = utils::floatingToFixedPoint(input);\n> > + if (ret != expected) {\n> > + cerr << \"Expected \" << input << \" to convert to \"\n> > + << expected << \", got \" << ret << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /*\n> > + * The precision check is fairly arbitrary but is based on what\n> > + * the rkisp1 is capable of in the crosstalk module.\n> > + */\n> > + double f = utils::fixedToFloatingPoint(ret);\n> > + if (std::abs(f - input) > 0.005) {\n> > + cerr << \"Reverse conversion expected \" << ret\n> > + << \" to convert to \" << input\n> > + << \", got \" << f << std::endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + return TestPass;\n> > + }\n> > +\n> > + int testFixedPoint()\n> > + {\n> > + /* These are the only cases that we know for certain */\n> \n> Nit: This comment no longer holds :-)\n> \n> No need to send another patch.\n> \n> Reviewed-by: Stefan Klug \n> \n> Cheers,\n> Stefan\n> \n> > + std::map testCases = {\n> > + { 7.992, 0x3FF },\n> > + { 7.992, 0xBFF },\n\nIs 7.992 really both expected values? How does this pass ... am I\nmissing something obvious?\n\nI really feel like I'm mis-understanding something here ... I ... assume\nthis passes the test right? If this passes, can you add a comment\nexplaining what this value is testing in both cases please?\n\n--\nKieran\n\n\n> > + { 0.2, 0x01A },\n> > + { -0.2, 0x7E6 },\n> > + { -0.8, 0x79A },\n> > + { -0.4, 0x7CD },\n> > + { -1.4, 0x74D },\n> > + { -8, 0x400 },\n> > + { 0, 0 },\n> > + };\n> > +\n> > + int ret;\n> > + for (const auto &testCase : testCases) {\n> > + ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> > + testCase.second);\n\n\n> > + if (ret != TestPass)\n> > + return ret;\n> > + }\n> > +\n> > + return TestPass;\n> > + }\n> > +\n> > + int run()\n> > + {\n> > + /* fixed point conversion test */\n> > + if (testFixedPoint() != TestPass)\n> > + return TestFail;\n> > +\n> > + return TestPass;\n> > + }\n> > +};\n> > +\n> > +TEST_REGISTER(RkISP1UtilsTest)\n> > -- \n> > 2.39.2\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 380CCBDE6B\n\tfor ;\n\tThu, 30 May 2024 22:50:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2DEB9634B7;\n\tFri, 31 May 2024 00:50:48 +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 66117634AF\n\tfor ;\n\tFri, 31 May 2024 00:50:47 +0200 (CEST)","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 CAEBFA06;\n\tFri, 31 May 2024 00:50:42 +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=\"U7rz+NaY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717109442;\n\tbh=T1tBUNfY4/xl5+v9DE0OvkpBGwu5UA90tz8dS/GiTbk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=U7rz+NaY5kpeNz0Dn0OtShd2JYgT89EgVVsp/BnMERFQwBVI00zLjjvXgAVJz5tcz\n\taQMJQX5dFQf7PHNueg4BvL1mT4UnWm9hKz0x4J1BfrPsZB4mFErfwz5KqiAbA/0RM2\n\ts92WCMS6w7lUGGMy8YH8zaz24Kv//20lztXt3cbQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<6b4icccczq7wma6td4lvbjge4jl7umfu25lmodnj56lkd3mtag@osrpvshhk6td>","References":"<20240530123934.2325922-1-paul.elder@ideasonboard.com>\n\t<6b4icccczq7wma6td4lvbjge4jl7umfu25lmodnj56lkd3mtag@osrpvshhk6td>","Subject":"Re: [PATCH v4] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Paul Elder ,\n\tStefan Klug ","Date":"Thu, 30 May 2024 23:50:43 +0100","Message-ID":"<171710944394.372008.2276426537269290224@ping.linuxembedded.co.uk>","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":29686,"web_url":"https://patchwork.libcamera.org/comment/29686/","msgid":"<171710958324.372008.5550756172027650934@ping.linuxembedded.co.uk>","date":"2024-05-30T22:53:03","subject":"Re: [PATCH v4] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2024-05-30 23:50:43)\n> Quoting Stefan Klug (2024-05-30 15:23:19)\n> > Hi Paul,\n> > \n> > thanks for the fast update.\n> > \n> > On Thu, May 30, 2024 at 09:39:34PM +0900, Paul Elder wrote:\n> > > Add helper functions for converting between floating point and fixed\n> > > point numbers. Also add tests for them.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > Changes in v4:\n> > > - relax the test case floating point precision\n> > > - optimize the fixed -> floating point converter\n> > > \n> > > Changes in v3:\n> > > - (used to be \"libcamera: utils: Add a helper to convert floating-point to fixed-point\")\n> > > - move the everything from libcamera global utils to the rkisp1 ipa\n> > > \n> > > Changes in v2:\n> > > - added the reverse conversion function (fixed -> floating)\n> > > - added tests\n> > > - make the conversion code cleaner\n> > > ---\n> > > src/ipa/rkisp1/meson.build | 1 +\n> > > src/ipa/rkisp1/utils.cpp | 40 +++++++++++++++\n> > > src/ipa/rkisp1/utils.h | 66 +++++++++++++++++++++++++\n> > > test/ipa/meson.build | 2 +\n> > > test/ipa/rkisp1/meson.build | 15 ++++++\n> > > test/ipa/rkisp1/rkisp1-utils.cpp | 84 ++++++++++++++++++++++++++++++++\n> > > 6 files changed, 208 insertions(+)\n> > > create mode 100644 src/ipa/rkisp1/utils.cpp\n> > > create mode 100644 src/ipa/rkisp1/utils.h\n> > > create mode 100644 test/ipa/rkisp1/meson.build\n> > > create mode 100644 test/ipa/rkisp1/rkisp1-utils.cpp\n> > > \n> > > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> > > index e813da53a..cf05cdb27 100644\n> > > --- a/src/ipa/rkisp1/meson.build\n> > > +++ b/src/ipa/rkisp1/meson.build\n> > > @@ -8,6 +8,7 @@ ipa_name = 'ipa_rkisp1'\n> > > rkisp1_ipa_sources = files([\n> > > 'ipa_context.cpp',\n> > > 'rkisp1.cpp',\n> > > + 'utils.cpp',\n> > > ])\n> > > \n> > > rkisp1_ipa_sources += rkisp1_ipa_algorithms\n> > > diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp\n> > > new file mode 100644\n> > > index 000000000..53186fdd6\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/utils.cpp\n> > > @@ -0,0 +1,40 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Paul Elder \n> > > + *\n> > > + * Miscellaneous utility functions specific to rkisp1\n> > > + */\n> > > +\n> > > +#include \"utils.h\"\n> > > +\n> > > +/**\n> > > + * \\file utils.h\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::utils {\n> > > +\n> > > +/**\n> > > + * \\fn R floatingToFixedPoint(T number)\n> > > + * \\brief Convert a floating point number to a fixed-point representation\n> > > + * \\tparam I Bit width of the integer part of the fixed-point\n> > > + * \\tparam F Bit width of the fractional part of the fixed-point\n> > > + * \\tparam R Return type of the fixed-point representation\n> > > + * \\tparam T Input type of the floating point representation\n> > > + * \\return The converted value\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn R fixedToFloatingPoint(T number)\n> > > + * \\brief Convert a fixed-point number to a floating point representation\n> > > + * \\tparam I Bit width of the integer part of the fixed-point\n> > > + * \\tparam F Bit width of the fractional part of the fixed-point\n> > > + * \\tparam R Return type of the floating point representation\n> > > + * \\tparam T Input type of the fixed-point representation\n> > > + * \\return The converted value\n> > > + */\n> > > +\n> > > +} /* namespace ipa::rkisp1::utils */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/rkisp1/utils.h b/src/ipa/rkisp1/utils.h\n> > > new file mode 100644\n> > > index 000000000..450f22442\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/utils.h\n> > > @@ -0,0 +1,66 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Paul Elder \n> > > + *\n> > > + * Miscellaneous utility functions specific to rkisp1\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::utils {\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +template > > + std::enable_if_t &&\n> > > + std::is_floating_point_v> * = nullptr>\n> > > +#else\n> > > +template\n> > > +#endif\n> > > +constexpr R floatingToFixedPoint(T number)\n> > > +{\n> > > + static_assert(sizeof(int) >= sizeof(R));\n> > > + static_assert(I + F <= sizeof(R) * 8);\n> > > +\n> > > + /*\n> > > + * The intermediate cast to int is needed on arm platforms to properly\n> > > + * cast negative values. See\n> > > + * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> > > + */\n> > > + R mask = (1 << (F + I)) - 1;\n> > > + R frac = static_cast(static_cast(std::round(number * (1 << F)))) & mask;\n> > > +\n> > > + return frac;\n> > > +}\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +template > > + std::enable_if_t &&\n> > > + std::is_integral_v> * = nullptr>\n> > > +#else\n> > > +template\n> > > +#endif\n> > > +constexpr R fixedToFloatingPoint(T number)\n> > > +{\n> > > + static_assert(sizeof(int) >= sizeof(T));\n> > > + static_assert(I + F <= sizeof(T) * 8);\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> > > + * by the same amount which keeps the sign bit in place.\n> > > + * This can be optimized by the compiler quite well.\n> > > + */\n> > > + int remaining_bits = sizeof(int) * 8 - (I + F);\n> > > + int t = static_cast(static_cast(number) << remaining_bits) >> remaining_bits;\n> > > + return static_cast(t) / static_cast(1 << F);\n> > > +}\n> > > +\n> > > +} /* namespace ipa::rkisp1::utils */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/test/ipa/meson.build b/test/ipa/meson.build\n> > > index 180b0da0a..dc956284c 100644\n> > > --- a/test/ipa/meson.build\n> > > +++ b/test/ipa/meson.build\n> > > @@ -1,5 +1,7 @@\n> > > # SPDX-License-Identifier: CC0-1.0\n> > > \n> > > +subdir('rkisp1')\n> > > +\n> > > ipa_test = [\n> > > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> > > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> > > diff --git a/test/ipa/rkisp1/meson.build b/test/ipa/rkisp1/meson.build\n> > > new file mode 100644\n> > > index 000000000..5ffc5dd60\n> > > --- /dev/null\n> > > +++ b/test/ipa/rkisp1/meson.build\n> > > @@ -0,0 +1,15 @@\n> > > +# SPDX-License-Identifier: CC0-1.0\n> > > +\n> > > +rkisp1_ipa_test = [\n> > > + {'name': 'rkisp1-utils', 'sources': ['rkisp1-utils.cpp']},\n> > > +]\n> > > +\n> > > +foreach test : rkisp1_ipa_test\n> > > + exe = executable(test['name'], test['sources'], libcamera_generated_ipa_headers,\n> > > + dependencies : libcamera_private,\n> > > + link_with : [libipa, test_libraries],\n> > > + include_directories : [libipa_includes, test_includes_internal,\n> > > + '../../../src/ipa/rkisp1/'])\n> > > +\n> > > + test(test['name'], exe, suite : 'ipa')\n> > > +endforeach\n> > > diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp\n> > > new file mode 100644\n> > > index 000000000..d23d87e0f\n> > > --- /dev/null\n> > > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp\n> > > @@ -0,0 +1,84 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Paul Elder \n> > > + *\n> > > + * Miscellaneous utility tests\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"../src/ipa/rkisp1/utils.h\"\n> > > +\n> > > +#include \"test.h\"\n> > > +\n> > > +using namespace std;\n> > > +using namespace libcamera;\n> > > +using namespace ipa::rkisp1;\n> > > +\n> > > +class RkISP1UtilsTest : public Test\n> > > +{\n> > > +protected:\n> > > + template\n> > > + int testSingleFixedPoint(double input, T expected)\n> > > + {\n> > > + T ret = utils::floatingToFixedPoint(input);\n> > > + if (ret != expected) {\n> > > + cerr << \"Expected \" << input << \" to convert to \"\n> > > + << expected << \", got \" << ret << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * The precision check is fairly arbitrary but is based on what\n> > > + * the rkisp1 is capable of in the crosstalk module.\n> > > + */\n> > > + double f = utils::fixedToFloatingPoint(ret);\n> > > + if (std::abs(f - input) > 0.005) {\n> > > + cerr << \"Reverse conversion expected \" << ret\n> > > + << \" to convert to \" << input\n> > > + << \", got \" << f << std::endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + return TestPass;\n> > > + }\n> > > +\n> > > + int testFixedPoint()\n> > > + {\n> > > + /* These are the only cases that we know for certain */\n> > \n> > Nit: This comment no longer holds :-)\n> > \n> > No need to send another patch.\n> > \n> > Reviewed-by: Stefan Klug \n> > \n> > Cheers,\n> > Stefan\n> > \n> > > + std::map testCases = {\n> > > + { 7.992, 0x3FF },\n> > > + { 7.992, 0xBFF },\n> \n> Is 7.992 really both expected values? How does this pass ... am I\n> missing something obvious?\n> \n> I really feel like I'm mis-understanding something here ... I ... assume\n> this passes the test right? If this passes, can you add a comment\n> explaining what this value is testing in both cases please?\n\nI see from v3 this is testing an unused bit doesn't affect the result.\nCertainly needs a comment explaining that I think..\n\nwith that (and the one rejected by Stefan above removed)\n\n\nReviewed-by: Kieran Bingham \n\n> \n> --\n> Kieran\n> \n> \n> > > + { 0.2, 0x01A },\n> > > + { -0.2, 0x7E6 },\n> > > + { -0.8, 0x79A },\n> > > + { -0.4, 0x7CD },\n> > > + { -1.4, 0x74D },\n> > > + { -8, 0x400 },\n> > > + { 0, 0 },\n> > > + };\n> > > +\n> > > + int ret;\n> > > + for (const auto &testCase : testCases) {\n> > > + ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> > > + testCase.second);\n> \n> \n> > > + if (ret != TestPass)\n> > > + return ret;\n> > > + }\n> > > +\n> > > + return TestPass;\n> > > + }\n> > > +\n> > > + int run()\n> > > + {\n> > > + /* fixed point conversion test */\n> > > + if (testFixedPoint() != TestPass)\n> > > + return TestFail;\n> > > +\n> > > + return TestPass;\n> > > + }\n> > > +};\n> > > +\n> > > +TEST_REGISTER(RkISP1UtilsTest)\n> > > -- \n> > > 2.39.2\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 48F5FBDE6B\n\tfor ;\n\tThu, 30 May 2024 22:53:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 14CB5634BA;\n\tFri, 31 May 2024 00:53:08 +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 48B6F634B5\n\tfor ;\n\tFri, 31 May 2024 00:53:06 +0200 (CEST)","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 8230DB53;\n\tFri, 31 May 2024 00:53:01 +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=\"BWz8u1pF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717109581;\n\tbh=U9h6r9gkKNJkjTJ+HIHD+5Og0hI2py8FGRGgMIpFLFA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=BWz8u1pFdB7fV8Pd1ze2ZPPHZO2hqEgbOiEWASQag/NIXQSAOGuiRPShckmjdz9Zf\n\t4ghKXk5cIMAMEKg2XcaG+DVILrzgRVHyNL3OBEaPY7qi1iUORb5ze7hUg0D9HD53xA\n\tMIplFvTUX42CHBQZoNfCe+ld7jDd51MJtzZvrbSg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<171710944394.372008.2276426537269290224@ping.linuxembedded.co.uk>","References":"<20240530123934.2325922-1-paul.elder@ideasonboard.com>\n\t<6b4icccczq7wma6td4lvbjge4jl7umfu25lmodnj56lkd3mtag@osrpvshhk6td>\n\t<171710944394.372008.2276426537269290224@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v4] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Paul Elder ,\n\tStefan Klug ","Date":"Thu, 30 May 2024 23:53:03 +0100","Message-ID":"<171710958324.372008.5550756172027650934@ping.linuxembedded.co.uk>","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\" "}}]