[{"id":29672,"web_url":"https://patchwork.libcamera.org/comment/29672/","msgid":"<4xnvnxksfbpoh3tojvwjcdd5k47dmercwpcoata5ms7u6veido@cqsfpuowsw3p>","date":"2024-05-30T11:16:56","subject":"Re: [PATCH v3] 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 patch.\n\nOn Thu, May 30, 2024 at 04:21:44AM +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 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 | 67 +++++++++++++++++++++++++++\n> test/ipa/meson.build | 2 +\n> test/ipa/rkisp1/meson.build | 15 ++++++\n> test/ipa/rkisp1/rkisp1-utils.cpp | 78 ++++++++++++++++++++++++++++++++\n> 6 files changed, 203 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..7d647d6d7\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/utils.h\n> @@ -0,0 +1,67 @@\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 * Create a number with all upper bits set including the sign bit\n> +\t * of the fixed point number. We need an extra cast as the compiler\n> +\t * won't let us left-shift negative numbers.\n> +\t */\n> +\tint upper_ones = static_cast(std::numeric_limits::max() << (I + F - 1));\n> +\tif (number & upper_ones)\n> +\t\treturn static_cast(upper_ones | number) / static_cast(1 << F);\n> +\n> +\treturn static_cast(number) / static_cast(1 << F);\n\nSorry for beeing a pest here. This fails in some cases. I wrote that\nhere https://patchwork.libcamera.org/patch/19935/#29340 with a fixed\n(and possibly faster) version.\n\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..c0c05dc68\n> --- /dev/null\n> +++ b/test/ipa/rkisp1/rkisp1-utils.cpp\n> @@ -0,0 +1,78 @@\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.001) {\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> +\t\tstd::map testCases = {\n> +\t\t\t{ 7.992, 0x3FF },\n> +\t\t\t{ -8, 0x400 },\n> +\t\t\t{ 0, 0 },\n\nCould you include the two cases that led to additional rewrites of the\nfunction? So (-0.4, -1.4) and one test, where the value is positive\nbut has a 1 in the uneeded bits (e.g. 0xBFF should result in 7.992)\n\nIf ever someone touches that code again, we shouldn't fall into these\ntraps.\n\nCheers,\nStefan\n\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 3A93DBDE6B\n\tfor ;\n\tThu, 30 May 2024 11:17:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F72E634B6;\n\tThu, 30 May 2024 13:17:02 +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 3FCA261A43\n\tfor ;\n\tThu, 30 May 2024 13:17:00 +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 E32B4FF1;\n\tThu, 30 May 2024 13:16:55 +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=\"ahcZkmbX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717067816;\n\tbh=jKdImbbopBdaksec0Mm/4Q7/5LeFlr+4umg6CmTKJFs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ahcZkmbXro2wR97Qe870Le47T4TEKgfHrQ7oLYT9EQ2s8HZ2uE7dJ/mkYIQV630aS\n\tbJJl8OIPpUezGI8zZfC/0ZNiZKwHxEBb9abxt2In54zIOf1JobhCG0oxmHX4mtpLc7\n\tAQwQNHrOGlvNXUmTny7kThPLJge14clOgQU6kYqE=","Date":"Thu, 30 May 2024 13:16:56 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","Message-ID":"<4xnvnxksfbpoh3tojvwjcdd5k47dmercwpcoata5ms7u6veido@cqsfpuowsw3p>","References":"<20240529192144.801432-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240529192144.801432-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":29673,"web_url":"https://patchwork.libcamera.org/comment/29673/","msgid":"","date":"2024-05-30T12:23:27","subject":"Re: [PATCH v3] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, May 30, 2024 at 01:16:56PM +0200, Stefan Klug wrote:\n> Hi Paul,\n> \n> thanks for the patch.\n> \n> On Thu, May 30, 2024 at 04:21:44AM +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 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 | 67 +++++++++++++++++++++++++++\n> > test/ipa/meson.build | 2 +\n> > test/ipa/rkisp1/meson.build | 15 ++++++\n> > test/ipa/rkisp1/rkisp1-utils.cpp | 78 ++++++++++++++++++++++++++++++++\n> > 6 files changed, 203 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..7d647d6d7\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/utils.h\n> > @@ -0,0 +1,67 @@\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 * Create a number with all upper bits set including the sign bit\n> > +\t * of the fixed point number. We need an extra cast as the compiler\n> > +\t * won't let us left-shift negative numbers.\n> > +\t */\n> > +\tint upper_ones = static_cast(std::numeric_limits::max() << (I + F - 1));\n> > +\tif (number & upper_ones)\n> > +\t\treturn static_cast(upper_ones | number) / static_cast(1 << F);\n> > +\n> > +\treturn static_cast(number) / static_cast(1 << F);\n> \n> Sorry for beeing a pest here. This fails in some cases. I wrote that\n> here https://patchwork.libcamera.org/patch/19935/#29340 with a fixed\n> (and possibly faster) version.\n\nThis version works fine for 0xBFF that you mention below, but indeed\nyour version there is more optimizable so I'll go with that.\n\n> \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..c0c05dc68\n> > --- /dev/null\n> > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp\n> > @@ -0,0 +1,78 @@\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.001) {\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> > +\t\tstd::map testCases = {\n> > +\t\t\t{ 7.992, 0x3FF },\n> > +\t\t\t{ -8, 0x400 },\n> > +\t\t\t{ 0, 0 },\n> \n> Could you include the two cases that led to additional rewrites of the\n> function? So (-0.4, -1.4) and one test, where the value is positive\n> but has a 1 in the uneeded bits (e.g. 0xBFF should result in 7.992)\n> \n\nAaand I can't get -1.4 to pass the test...\n\n\nPaul\n\n> If ever someone touches that code again, we shouldn't fall into these\n> traps.\n> \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 23A4FBD87C\n\tfor ;\n\tThu, 30 May 2024 12:23:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56BC6634B6;\n\tThu, 30 May 2024 14:23:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DE1C61A43\n\tfor ;\n\tThu, 30 May 2024 14:23:35 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1BE3F29F;\n\tThu, 30 May 2024 14:23:29 +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=\"C/R6eBy1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717071811;\n\tbh=DWC5I3+Xj6PU5J353kI0fHJSdUFdU37jtLsARP76gy4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C/R6eBy1f63jZFTUvAJ/p8FqfFrUg7fwC9fgZU2rn2/rH1uhYDZ5neDV/zis+020j\n\tpjFs/wft7ereOSEke5f3/YmW0YYNMCXi2Gtd80I9vJqPFRiizB/tbqMcdC83PMWv81\n\tdKn/crnLCFvZHSQAnKFLr7cpwVRJryrJpHEvpZzE=","Date":"Thu, 30 May 2024 21:23:27 +0900","From":"Paul Elder ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","Message-ID":"","References":"<20240529192144.801432-1-paul.elder@ideasonboard.com>\n\t<4xnvnxksfbpoh3tojvwjcdd5k47dmercwpcoata5ms7u6veido@cqsfpuowsw3p>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<4xnvnxksfbpoh3tojvwjcdd5k47dmercwpcoata5ms7u6veido@cqsfpuowsw3p>","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\" "}}]