[{"id":29755,"web_url":"https://patchwork.libcamera.org/comment/29755/","msgid":"<20240603163208.GA7043@pendragon.ideasonboard.com>","date":"2024-06-03T16:32:08","subject":"Re: [PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, May 24, 2024 at 02:52:55PM +0100, Kieran Bingham wrote:\n> Introduce a test that validates conversion of the gain model\n> in both Linear and Exponential models.\n> \n> It should be expected that if a gainCode produces a given gain\n> value, then converting that gain back should produce the same\n> result.\n\nI think we have a general agreement on this.\n\n> The test is known to fail and is marked as such. The test will be\n> updated with the corresponding fix.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> test/ipa/camera_sensor_helper.cpp | 101 ++++++++++++++++++++++++++++++\n> test/ipa/meson.build | 5 +-\n> 2 files changed, 105 insertions(+), 1 deletion(-)\n> create mode 100644 test/ipa/camera_sensor_helper.cpp\n> \n> diff --git a/test/ipa/camera_sensor_helper.cpp b/test/ipa/camera_sensor_helper.cpp\n> new file mode 100644\n> index 000000000000..d8a8c6850a04\n> --- /dev/null\n> +++ b/test/ipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,101 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy.\n> + */\n> +\n> +#include \"libipa/camera_sensor_helper.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +using namespace libcamera::ipa;\n> +\n> +/*\n> + * Helper function to compute the m parameter of the exponential gain model\n> + * when the gain code is expressed in dB.\n> + */\n> +static constexpr double expGainDb(double step)\n> +{\n> +\tconstexpr double log2_10 = 3.321928094887362;\n> +\n> +\t/*\n> + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps):\n> + *\n> + * G_code = G_dB/step = 20/step*log10(G_linear)\n> + *\n> + * Inverting the formula, we get\n> + *\n> + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code)\n> + */\n> +\treturn log2_10 * step / 20;\n> +}\n> +\n> +class CameraSensorHelperExponential : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperExponential()\n> +\t{\n> +\t\tgainType_ = AnalogueGainExponential;\n> +\t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> +\t}\n> +};\n> +\n> +class CameraSensorHelperLinear : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperLinear()\n> +\t{\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 0, 1024, -1, 1024 };\n> +\t}\n> +};\n\nCould we test all the helpers, instead of two particular implementations\n? CameraSensorHelperFactoryBase provides a list of factories. We would\nneed to expose their name through a const accessor to be able to call\ncreate(). Or do you think that would be overkill ? We have helpers that\ndon't use the two standard models, because someone decided to implement\nsomething more \"clever\" in the hardware. It would be useful to cover\nthose in the tests I think.\n\n> +\n> +class CameraSensorHelperTest : public Test\n\nAs this tests the gain models only, I'd name the class\nCameraSensorGainsTest or something similar. Same for the source file.\n\n> +{\n> +protected:\n> +\tint testGainModel(CameraSensorHelper &helper)\n> +\t{\n> +\t\tint ret = TestPass;\n> +\n> +\t\t/*\n> +\t\t * Arbitrarily test 255 code positions assuming 8 bit fields\n> +\t\t * are the normal maximum for a gain code register.\n> +\t\t */\n\nThis bothers me, mostly because the right solution would require\nextending the helpers to expose the min/max gains, and *that* is\nbothersome :-S So far we rely on kernel drivers to provide the\ninformation. Do you think we should bite the bullet ?\n\nOne issue to consider is that the gain code may not be a continuous\nvalue. A sensor I'm working on splits the analog gain in coarse and fine\ngains, stored in bits [6:4] and [3:0]. The coarse gain is an exponential\ngain, expressed as the exponent of a power of two, ranging from 2^0 to\n2^4. The fine gain is an inverse linear gain following the formula\n\n\t1 / (1 - fine / 32)\n\nThe fine gain value is quantized differently depending on the coarse\ngain. When the coarse gain is 2^0 or 2^2, the fine gain code can take\nany value in the [0, 15] range. When the coarse gain is 2^1 or 2^3, the\nfine gain code is restricted to even values. When the coarse gain is\n2^4, the fine gain code is restricted to multiples of 4.\n\nI believe the hardware will operate correctly when those constraints are\nnot met, but will ignore the LSB or 2 LSBs of the fine gain code\ndepending on the coarse gain. This leads to, for instance,\n\n\tcoarse = 2^1, fine = 4 -> gain = 2.2857142857142856\n\tcoarse = 2^1, fine = 5 -> gain = 2.2857142857142856\n\nThe code -> gain -> code conversion can lead to the same result for all\nvalid code values, but not for all code values in the [min, max] range.\nI wonder how to handle that in the test.\n\n> +\t\tfor (unsigned int i = 0; i < 255; i++) {\n> +\t\t\tfloat gain = helper.gain(i);\n> +\t\t\tuint32_t gainCode = helper.gainCode(gain);\n> +\n> +\t\t\tif (i != gainCode) {\n> +\t\t\t\tstd::cout << \"Gain conversions failed: \"\n> +\t\t\t\t\t << i << \" : \" << gain << \" : \"\n> +\t\t\t\t\t << gainCode << std::endl;\n> +\n> +\t\t\t\tret = TestFail;\n> +\t\t\t}\n> +\t\t};\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\tunsigned int failures = 0;\n> +\n> +\t\tCameraSensorHelperExponential exponential;\n> +\t\tCameraSensorHelperLinear linear;\n> +\n> +\t\tif (testGainModel(exponential) == TestFail)\n> +\t\t\tfailures++;\n> +\n> +\t\tif (testGainModel(linear) == TestFail)\n> +\t\t\tfailures++;\n> +\n> +\t\treturn failures ? TestFail : TestPass;\n> +\t}\n> +};\n> +\n> +TEST_REGISTER(CameraSensorHelperTest)\n> diff --git a/test/ipa/meson.build b/test/ipa/meson.build\n> index 180b0da0a51a..abc2456fc341 100644\n> --- a/test/ipa/meson.build\n> +++ b/test/ipa/meson.build\n> @@ -1,6 +1,8 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> ipa_test = [\n> + {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp'],\n> + 'should_fail': true},\n\nIf you make it multi-line, let's make it so properly :-)\n\n {\n 'name': 'camera_sensor_helper',\n 'sources': ['camera_sensor_helper.cpp'],\n 'should_fail': true,\n },\n\n> {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> ]\n> @@ -11,5 +13,6 @@ foreach test : ipa_test\n> link_with : [libipa, test_libraries],\n> include_directories : [libipa_includes, test_includes_internal])\n> \n> - test(test['name'], exe, suite : 'ipa')\n> + test(test['name'], exe, suite : 'ipa',\n> + should_fail : test.get('should_fail', false))\n> endforeach","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 D9436BDE6B\n\tfor ;\n\tMon, 3 Jun 2024 16:32:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8BB6634CA;\n\tMon, 3 Jun 2024 18:32:25 +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 9858961A3B\n\tfor ;\n\tMon, 3 Jun 2024 18:32:24 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A4F2908;\n\tMon, 3 Jun 2024 18:32: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=\"TljJnqrB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717432337;\n\tbh=CLa0UDvQr5krg8SoFn3iKAx8z8IksoiSFAbOJppiuuk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TljJnqrBQq7KXWetTSOpZH4NBLAU02sUtpPsbprCnmzPxzcDrqBFFkDLrxUFnM9Kt\n\tDYIJxUhC+lIDjYJ6RUzIZ7EAIZ7XlH0JNQZNTNsTa+rzHywXzakE4QYZiPhzgrfRgU\n\t93gpIf6JjuvdbIS1ytB5QGpPFG9z/XFKfwZEhi8A=","Date":"Mon, 3 Jun 2024 19:32:08 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera devel ","Subject":"Re: [PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","Message-ID":"<20240603163208.GA7043@pendragon.ideasonboard.com>","References":"<20240524135256.649406-1-kieran.bingham@ideasonboard.com>\n\t<20240524135256.649406-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240524135256.649406-2-kieran.bingham@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":29756,"web_url":"https://patchwork.libcamera.org/comment/29756/","msgid":"<171743379660.205609.9436321327379359062@ping.linuxembedded.co.uk>","date":"2024-06-03T16:56:36","subject":"Re: [PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-06-03 17:32:08)\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, May 24, 2024 at 02:52:55PM +0100, Kieran Bingham wrote:\n> > Introduce a test that validates conversion of the gain model\n> > in both Linear and Exponential models.\n> > \n> > It should be expected that if a gainCode produces a given gain\n> > value, then converting that gain back should produce the same\n> > result.\n> \n> I think we have a general agreement on this.\n\n\n \\o/ - I felt like was fighting a lonely corner over here for a while.\n\n\n> > The test is known to fail and is marked as such. The test will be\n> > updated with the corresponding fix.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > test/ipa/camera_sensor_helper.cpp | 101 ++++++++++++++++++++++++++++++\n> > test/ipa/meson.build | 5 +-\n> > 2 files changed, 105 insertions(+), 1 deletion(-)\n> > create mode 100644 test/ipa/camera_sensor_helper.cpp\n> > \n> > diff --git a/test/ipa/camera_sensor_helper.cpp b/test/ipa/camera_sensor_helper.cpp\n> > new file mode 100644\n> > index 000000000000..d8a8c6850a04\n> > --- /dev/null\n> > +++ b/test/ipa/camera_sensor_helper.cpp\n> > @@ -0,0 +1,101 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas on Board Oy.\n> > + */\n> > +\n> > +#include \"libipa/camera_sensor_helper.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> > +using namespace libcamera;\n> > +using namespace libcamera::ipa;\n> > +\n> > +/*\n> > + * Helper function to compute the m parameter of the exponential gain model\n> > + * when the gain code is expressed in dB.\n> > + */\n> > +static constexpr double expGainDb(double step)\n> > +{\n> > + constexpr double log2_10 = 3.321928094887362;\n> > +\n> > + /*\n> > + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps):\n> > + *\n> > + * G_code = G_dB/step = 20/step*log10(G_linear)\n> > + *\n> > + * Inverting the formula, we get\n> > + *\n> > + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code)\n> > + */\n> > + return log2_10 * step / 20;\n> > +}\n> > +\n> > +class CameraSensorHelperExponential : public CameraSensorHelper\n> > +{\n> > +public:\n> > + CameraSensorHelperExponential()\n> > + {\n> > + gainType_ = AnalogueGainExponential;\n> > + gainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > + }\n> > +};\n> > +\n> > +class CameraSensorHelperLinear : public CameraSensorHelper\n> > +{\n> > +public:\n> > + CameraSensorHelperLinear()\n> > + {\n> > + gainType_ = AnalogueGainLinear;\n> > + gainConstants_.linear = { 0, 1024, -1, 1024 };\n> > + }\n> > +};\n> \n> Could we test all the helpers, instead of two particular implementations\n> ? CameraSensorHelperFactoryBase provides a list of factories. We would\n\n! Please see my first iteration of this Test where I did exactly this.\n\nSomeone called 'Laurent' asked me to change the implementation to this\nmethod instead.... Could you have words with him please? ;-)\n\n- https://lists.libcamera.org/pipermail/libcamera-devel/2024-February/040629.html\n\n> need to expose their name through a const accessor to be able to call\n> create(). Or do you think that would be overkill ? We have helpers that\n\nYou mean ... exactly like this perhaps?:\n - https://lists.libcamera.org/pipermail/libcamera-devel/2024-February/040559.html\n - https://patchwork.libcamera.org/patch/19529/\n\nI don't think that's overkill. I implemented it ;-)\n\nI did also say:\n\n\"\"\"\nAn alternative here is to just allow direct access to the\ncreateInstance() from the test too, without having to re-search\nall of the factories each time. But I felt this was less\nof a de-restriction of the existing interfaces.\n\"\"\"\n\n> don't use the two standard models, because someone decided to implement\n> something more \"clever\" in the hardware. It would be useful to cover\n> those in the tests I think.\n\nI absolutely agree. that's why I wanted to iterate /all/ helpers and\ntest each one explicitly in https://patchwork.libcamera.org/patch/19530/\n\n> > +\n> > +class CameraSensorHelperTest : public Test\n> \n> As this tests the gain models only, I'd name the class\n> CameraSensorGainsTest or something similar. Same for the source file.\n> \n> > +{\n> > +protected:\n> > + int testGainModel(CameraSensorHelper &helper)\n> > + {\n> > + int ret = TestPass;\n> > +\n> > + /*\n> > + * Arbitrarily test 255 code positions assuming 8 bit fields\n> > + * are the normal maximum for a gain code register.\n> > + */\n> \n> This bothers me, mostly because the right solution would require\n> extending the helpers to expose the min/max gains, and *that* is\n> bothersome :-S So far we rely on kernel drivers to provide the\n> information. Do you think we should bite the bullet ?\n\nIn fact, I questioned this too in https://patchwork.libcamera.org/patch/19530/\n\n\n\"\"\"\nBecause that's what value the IMX283 uses. Yes - as I said above, and\nwhy this is an RFC, we don't have a way to get the max value from here.\nBut getting it is not so clear. Should we define the max code for every\ncamera sensor helper? Should it be defaulted to something supplied from\nthe driver? (How?) or are we then just duplicating and hardcoding values\nfrom v4l2 drivers here in the helpers... maybe that's fine ?\n\"\"\"\n\n> \n> One issue to consider is that the gain code may not be a continuous\n> value. A sensor I'm working on splits the analog gain in coarse and fine\n> gains, stored in bits [6:4] and [3:0]. The coarse gain is an exponential\n> gain, expressed as the exponent of a power of two, ranging from 2^0 to\n> 2^4. The fine gain is an inverse linear gain following the formula\n> \n> 1 / (1 - fine / 32)\n> \n> The fine gain value is quantized differently depending on the coarse\n> gain. When the coarse gain is 2^0 or 2^2, the fine gain code can take\n> any value in the [0, 15] range. When the coarse gain is 2^1 or 2^3, the\n> fine gain code is restricted to even values. When the coarse gain is\n> 2^4, the fine gain code is restricted to multiples of 4.\n\nWe do have sensors which encode both analog and digital gain into a\nsingle control too.\n\nHowever for those so far, we artificially limit the gain to be only the\nanalog component in the kernel drivers.\n\nSee:\n https://lore.kernel.org/all/20240414140621.167717-7-umang.jain@ideasonboard.com/\n\n> I believe the hardware will operate correctly when those constraints are\n> not met, but will ignore the LSB or 2 LSBs of the fine gain code\n> depending on the coarse gain. This leads to, for instance,\n> \n> coarse = 2^1, fine = 4 -> gain = 2.2857142857142856\n> coarse = 2^1, fine = 5 -> gain = 2.2857142857142856\n> \n> The code -> gain -> code conversion can lead to the same result for all\n> valid code values, but not for all code values in the [min, max] range.\n> I wonder how to handle that in the test.\n\nYikes....\n\n\n> \n> > + for (unsigned int i = 0; i < 255; i++) {\n> > + float gain = helper.gain(i);\n> > + uint32_t gainCode = helper.gainCode(gain);\n> > +\n> > + if (i != gainCode) {\n> > + std::cout << \"Gain conversions failed: \"\n> > + << i << \" : \" << gain << \" : \"\n> > + << gainCode << std::endl;\n> > +\n> > + ret = TestFail;\n> > + }\n> > + };\n> > +\n> > + return ret;\n> > + }\n> > +\n> > + int run() override\n> > + {\n> > + unsigned int failures = 0;\n> > +\n> > + CameraSensorHelperExponential exponential;\n> > + CameraSensorHelperLinear linear;\n> > +\n> > + if (testGainModel(exponential) == TestFail)\n> > + failures++;\n> > +\n> > + if (testGainModel(linear) == TestFail)\n> > + failures++;\n> > +\n> > + return failures ? TestFail : TestPass;\n> > + }\n> > +};\n> > +\n> > +TEST_REGISTER(CameraSensorHelperTest)\n> > diff --git a/test/ipa/meson.build b/test/ipa/meson.build\n> > index 180b0da0a51a..abc2456fc341 100644\n> > --- a/test/ipa/meson.build\n> > +++ b/test/ipa/meson.build\n> > @@ -1,6 +1,8 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> > \n> > ipa_test = [\n> > + {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp'],\n> > + 'should_fail': true},\n> \n> If you make it multi-line, let's make it so properly :-)\n> \n> {\n> 'name': 'camera_sensor_helper',\n> 'sources': ['camera_sensor_helper.cpp'],\n> 'should_fail': true,\n> },\n\nWell - I would (and did originally) except the very next patch removes\nit again, so I believed this was better as it was less visible churn.\n\n> \n> > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> > ]\n> > @@ -11,5 +13,6 @@ foreach test : ipa_test\n> > link_with : [libipa, test_libraries],\n> > include_directories : [libipa_includes, test_includes_internal])\n> > \n> > - test(test['name'], exe, suite : 'ipa')\n> > + test(test['name'], exe, suite : 'ipa',\n> > + should_fail : test.get('should_fail', false))\n> > endforeach\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 45F06BD87C\n\tfor ;\n\tMon, 3 Jun 2024 16:56:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F459634B2;\n\tMon, 3 Jun 2024 18:56:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B82961A3B\n\tfor ;\n\tMon, 3 Jun 2024 18:56:39 +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 0B77AC67;\n\tMon, 3 Jun 2024 18:56:32 +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=\"MXD6cTZF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717433792;\n\tbh=FTXbWxohEZyK+6h6ptb0hlzCQ2Ymxo/KH1cBgI8ojsY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=MXD6cTZFFBrq2VNrpPixX/5fIj389p6QPdF/hOGTwzr+IxCx7URJkkapvNTQLlTDG\n\tkZSrau8FBB1CzGwbXzyfhHAdU/FCoOm5LPjj7skPWCKt4hkXHRtNBzsDB8EUpV6RWJ\n\t+qklF4/gbqWHZGGJklaoj4kn5/27SHklBFE7sy7g=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240603163208.GA7043@pendragon.ideasonboard.com>","References":"<20240524135256.649406-1-kieran.bingham@ideasonboard.com>\n\t<20240524135256.649406-2-kieran.bingham@ideasonboard.com>\n\t<20240603163208.GA7043@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","From":"Kieran Bingham ","Cc":"libcamera devel ","To":"Laurent Pinchart ","Date":"Mon, 03 Jun 2024 17:56:36 +0100","Message-ID":"<171743379660.205609.9436321327379359062@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":29757,"web_url":"https://patchwork.libcamera.org/comment/29757/","msgid":"<20240603171024.GA5667@pendragon.ideasonboard.com>","date":"2024-06-03T17:10:24","subject":"Re: [PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jun 03, 2024 at 05:56:36PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-06-03 17:32:08)\n> > On Fri, May 24, 2024 at 02:52:55PM +0100, Kieran Bingham wrote:\n> > > Introduce a test that validates conversion of the gain model\n> > > in both Linear and Exponential models.\n> > > \n> > > It should be expected that if a gainCode produces a given gain\n> > > value, then converting that gain back should produce the same\n> > > result.\n> > \n> > I think we have a general agreement on this.\n> \n> \\o/ - I felt like was fighting a lonely corner over here for a while.\n> \n> > > The test is known to fail and is marked as such. The test will be\n> > > updated with the corresponding fix.\n> > > \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > test/ipa/camera_sensor_helper.cpp | 101 ++++++++++++++++++++++++++++++\n> > > test/ipa/meson.build | 5 +-\n> > > 2 files changed, 105 insertions(+), 1 deletion(-)\n> > > create mode 100644 test/ipa/camera_sensor_helper.cpp\n> > > \n> > > diff --git a/test/ipa/camera_sensor_helper.cpp b/test/ipa/camera_sensor_helper.cpp\n> > > new file mode 100644\n> > > index 000000000000..d8a8c6850a04\n> > > --- /dev/null\n> > > +++ b/test/ipa/camera_sensor_helper.cpp\n> > > @@ -0,0 +1,101 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas on Board Oy.\n> > > + */\n> > > +\n> > > +#include \"libipa/camera_sensor_helper.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"test.h\"\n> > > +\n> > > +using namespace std;\n> > > +using namespace libcamera;\n> > > +using namespace libcamera::ipa;\n> > > +\n> > > +/*\n> > > + * Helper function to compute the m parameter of the exponential gain model\n> > > + * when the gain code is expressed in dB.\n> > > + */\n> > > +static constexpr double expGainDb(double step)\n> > > +{\n> > > + constexpr double log2_10 = 3.321928094887362;\n> > > +\n> > > + /*\n> > > + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps):\n> > > + *\n> > > + * G_code = G_dB/step = 20/step*log10(G_linear)\n> > > + *\n> > > + * Inverting the formula, we get\n> > > + *\n> > > + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code)\n> > > + */\n> > > + return log2_10 * step / 20;\n> > > +}\n> > > +\n> > > +class CameraSensorHelperExponential : public CameraSensorHelper\n> > > +{\n> > > +public:\n> > > + CameraSensorHelperExponential()\n> > > + {\n> > > + gainType_ = AnalogueGainExponential;\n> > > + gainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > > + }\n> > > +};\n> > > +\n> > > +class CameraSensorHelperLinear : public CameraSensorHelper\n> > > +{\n> > > +public:\n> > > + CameraSensorHelperLinear()\n> > > + {\n> > > + gainType_ = AnalogueGainLinear;\n> > > + gainConstants_.linear = { 0, 1024, -1, 1024 };\n> > > + }\n> > > +};\n> > \n> > Could we test all the helpers, instead of two particular implementations\n> > ? CameraSensorHelperFactoryBase provides a list of factories. We would\n> \n> ! Please see my first iteration of this Test where I did exactly this.\n> \n> Someone called 'Laurent' asked me to change the implementation to this\n> method instead.... Could you have words with him please? ;-)\n\nWell, technically, I've only hinted a possible alternative, I didn't say\nit was the best solution, but... OK, I stand corrected, I was wrong :-)\n\n> - https://lists.libcamera.org/pipermail/libcamera-devel/2024-February/040629.html\n> \n> > need to expose their name through a const accessor to be able to call\n> > create(). Or do you think that would be overkill ? We have helpers that\n> \n> You mean ... exactly like this perhaps?:\n> - https://lists.libcamera.org/pipermail/libcamera-devel/2024-February/040559.html\n> - https://patchwork.libcamera.org/patch/19529/\n> \n> I don't think that's overkill. I implemented it ;-)\n> \n> I did also say:\n> \n> \"\"\"\n> An alternative here is to just allow direct access to the\n> createInstance() from the test too, without having to re-search\n> all of the factories each time. But I felt this was less\n> of a de-restriction of the existing interfaces.\n> \"\"\"\n> \n> > don't use the two standard models, because someone decided to implement\n> > something more \"clever\" in the hardware. It would be useful to cover\n> > those in the tests I think.\n> \n> I absolutely agree. that's why I wanted to iterate /all/ helpers and\n> test each one explicitly in https://patchwork.libcamera.org/patch/19530/\n\nAmazing, the work is already done :-)\n\n> > > +\n> > > +class CameraSensorHelperTest : public Test\n> > \n> > As this tests the gain models only, I'd name the class\n> > CameraSensorGainsTest or something similar. Same for the source file.\n> > \n> > > +{\n> > > +protected:\n> > > + int testGainModel(CameraSensorHelper &helper)\n> > > + {\n> > > + int ret = TestPass;\n> > > +\n> > > + /*\n> > > + * Arbitrarily test 255 code positions assuming 8 bit fields\n> > > + * are the normal maximum for a gain code register.\n> > > + */\n> > \n> > This bothers me, mostly because the right solution would require\n> > extending the helpers to expose the min/max gains, and *that* is\n> > bothersome :-S So far we rely on kernel drivers to provide the\n> > information. Do you think we should bite the bullet ?\n> \n> In fact, I questioned this too in https://patchwork.libcamera.org/patch/19530/\n> \n> \"\"\"\n> Because that's what value the IMX283 uses. Yes - as I said above, and\n> why this is an RFC, we don't have a way to get the max value from here.\n> But getting it is not so clear. Should we define the max code for every\n> camera sensor helper? Should it be defaulted to something supplied from\n> the driver? (How?) or are we then just duplicating and hardcoding values\n> from v4l2 drivers here in the helpers... maybe that's fine ?\n> \"\"\"\n\nSo... Biting the bullet ? We already hardcode the gain model in\nlibcamera, because sensor drivers don't provide any information about\nit. We could consider that the min/max values are part of the model. The\ndrawback is that we'll have problems if the kernel, for any reason,\ndecides to implement a different range.\n\n> > One issue to consider is that the gain code may not be a continuous\n> > value. A sensor I'm working on splits the analog gain in coarse and fine\n> > gains, stored in bits [6:4] and [3:0]. The coarse gain is an exponential\n> > gain, expressed as the exponent of a power of two, ranging from 2^0 to\n> > 2^4. The fine gain is an inverse linear gain following the formula\n> > \n> > 1 / (1 - fine / 32)\n> > \n> > The fine gain value is quantized differently depending on the coarse\n> > gain. When the coarse gain is 2^0 or 2^2, the fine gain code can take\n> > any value in the [0, 15] range. When the coarse gain is 2^1 or 2^3, the\n> > fine gain code is restricted to even values. When the coarse gain is\n> > 2^4, the fine gain code is restricted to multiples of 4.\n> \n> We do have sensors which encode both analog and digital gain into a\n> single control too.\n> \n> However for those so far, we artificially limit the gain to be only the\n> analog component in the kernel drivers.\n> \n> See:\n> https://lore.kernel.org/all/20240414140621.167717-7-umang.jain@ideasonboard.com/\n\nThe sensor may store the digital and analog gain values in the same\nregister, but they're still two separate gains, the kernel driver needs\nto split the value in two V4L2 controls. I'm not concerned here.\n\n> > I believe the hardware will operate correctly when those constraints are\n> > not met, but will ignore the LSB or 2 LSBs of the fine gain code\n> > depending on the coarse gain. This leads to, for instance,\n> > \n> > coarse = 2^1, fine = 4 -> gain = 2.2857142857142856\n> > coarse = 2^1, fine = 5 -> gain = 2.2857142857142856\n> > \n> > The code -> gain -> code conversion can lead to the same result for all\n> > valid code values, but not for all code values in the [min, max] range.\n> > I wonder how to handle that in the test.\n> \n> Yikes....\n\nAnd I haven't even mentioned that there's another fixed gain multiplier\nadded when the coarse gain is > 2^1 :-)\n\n> > > + for (unsigned int i = 0; i < 255; i++) {\n> > > + float gain = helper.gain(i);\n> > > + uint32_t gainCode = helper.gainCode(gain);\n> > > +\n> > > + if (i != gainCode) {\n> > > + std::cout << \"Gain conversions failed: \"\n> > > + << i << \" : \" << gain << \" : \"\n> > > + << gainCode << std::endl;\n> > > +\n> > > + ret = TestFail;\n> > > + }\n> > > + };\n> > > +\n> > > + return ret;\n> > > + }\n> > > +\n> > > + int run() override\n> > > + {\n> > > + unsigned int failures = 0;\n> > > +\n> > > + CameraSensorHelperExponential exponential;\n> > > + CameraSensorHelperLinear linear;\n> > > +\n> > > + if (testGainModel(exponential) == TestFail)\n> > > + failures++;\n> > > +\n> > > + if (testGainModel(linear) == TestFail)\n> > > + failures++;\n> > > +\n> > > + return failures ? TestFail : TestPass;\n> > > + }\n> > > +};\n> > > +\n> > > +TEST_REGISTER(CameraSensorHelperTest)\n> > > diff --git a/test/ipa/meson.build b/test/ipa/meson.build\n> > > index 180b0da0a51a..abc2456fc341 100644\n> > > --- a/test/ipa/meson.build\n> > > +++ b/test/ipa/meson.build\n> > > @@ -1,6 +1,8 @@\n> > > # SPDX-License-Identifier: CC0-1.0\n> > > \n> > > ipa_test = [\n> > > + {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp'],\n> > > + 'should_fail': true},\n> > \n> > If you make it multi-line, let's make it so properly :-)\n> > \n> > {\n> > 'name': 'camera_sensor_helper',\n> > 'sources': ['camera_sensor_helper.cpp'],\n> > 'should_fail': true,\n> > },\n> \n> Well - I would (and did originally) except the very next patch removes\n> it again, so I believed this was better as it was less visible churn.\n\nGood point. I'm fine with that.\n\n> > > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> > > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> > > ]\n> > > @@ -11,5 +13,6 @@ foreach test : ipa_test\n> > > link_with : [libipa, test_libraries],\n> > > include_directories : [libipa_includes, test_includes_internal])\n> > > \n> > > - test(test['name'], exe, suite : 'ipa')\n> > > + test(test['name'], exe, suite : 'ipa',\n> > > + should_fail : test.get('should_fail', false))\n> > > endforeach","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 03B86BDE6B\n\tfor ;\n\tMon, 3 Jun 2024 17:10:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F1AC634B2;\n\tMon, 3 Jun 2024 19:10:43 +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 65B3461A3B\n\tfor ;\n\tMon, 3 Jun 2024 19:10:41 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 03AFFA38;\n\tMon, 3 Jun 2024 19:10:33 +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=\"eJ9i3nEM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717434634;\n\tbh=r5r1KKf+VkqcO4V8d2XbduMiDqcNQoFZI8Zgv8UOXaM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eJ9i3nEMyNZyU89c7aQPorlmkUO2eBeLtgzNr0Ik4F4aYRlkqt8Zghzh3A9l06L6Y\n\tb/pOKocy5TzCCg+xr9LGUPOC4mGOycotWTivCJpnyyN6VxQT2X2kOJDsczFZdmcXvV\n\tlOHZBAX1oo6sRx1a48ymTJKKJkHZc5dWMGbxLVQU=","Date":"Mon, 3 Jun 2024 20:10:24 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera devel ","Subject":"Re: [PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","Message-ID":"<20240603171024.GA5667@pendragon.ideasonboard.com>","References":"<20240524135256.649406-1-kieran.bingham@ideasonboard.com>\n\t<20240524135256.649406-2-kieran.bingham@ideasonboard.com>\n\t<20240603163208.GA7043@pendragon.ideasonboard.com>\n\t<171743379660.205609.9436321327379359062@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171743379660.205609.9436321327379359062@ping.linuxembedded.co.uk>","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":29778,"web_url":"https://patchwork.libcamera.org/comment/29778/","msgid":"<171758031722.205609.7571558848372932514@ping.linuxembedded.co.uk>","date":"2024-06-05T09:38:37","subject":"Re: [PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-06-03 18:10:24)\n> Hi Kieran,\n> \n> On Mon, Jun 03, 2024 at 05:56:36PM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2024-06-03 17:32:08)\n> > > On Fri, May 24, 2024 at 02:52:55PM +0100, Kieran Bingham wrote:\n> > > > Introduce a test that validates conversion of the gain model\n> > > > in both Linear and Exponential models.\n> > > > \n> > > > It should be expected that if a gainCode produces a given gain\n> > > > value, then converting that gain back should produce the same\n> > > > result.\n> > > \n> > > I think we have a general agreement on this.\n> > \n> > \\o/ - I felt like was fighting a lonely corner over here for a while.\n> > \n> > > > The test is known to fail and is marked as such. The test will be\n> > > > updated with the corresponding fix.\n> > > > \n> > > > Signed-off-by: Kieran Bingham \n> > > > ---\n> > > > test/ipa/camera_sensor_helper.cpp | 101 ++++++++++++++++++++++++++++++\n> > > > test/ipa/meson.build | 5 +-\n> > > > 2 files changed, 105 insertions(+), 1 deletion(-)\n> > > > create mode 100644 test/ipa/camera_sensor_helper.cpp\n> > > > \n> > > > diff --git a/test/ipa/camera_sensor_helper.cpp b/test/ipa/camera_sensor_helper.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..d8a8c6850a04\n> > > > --- /dev/null\n> > > > +++ b/test/ipa/camera_sensor_helper.cpp\n> > > > @@ -0,0 +1,101 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Ideas on Board Oy.\n> > > > + */\n> > > > +\n> > > > +#include \"libipa/camera_sensor_helper.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"test.h\"\n> > > > +\n> > > > +using namespace std;\n> > > > +using namespace libcamera;\n> > > > +using namespace libcamera::ipa;\n> > > > +\n> > > > +/*\n> > > > + * Helper function to compute the m parameter of the exponential gain model\n> > > > + * when the gain code is expressed in dB.\n> > > > + */\n> > > > +static constexpr double expGainDb(double step)\n> > > > +{\n> > > > + constexpr double log2_10 = 3.321928094887362;\n> > > > +\n> > > > + /*\n> > > > + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps):\n> > > > + *\n> > > > + * G_code = G_dB/step = 20/step*log10(G_linear)\n> > > > + *\n> > > > + * Inverting the formula, we get\n> > > > + *\n> > > > + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code)\n> > > > + */\n> > > > + return log2_10 * step / 20;\n> > > > +}\n> > > > +\n> > > > +class CameraSensorHelperExponential : public CameraSensorHelper\n> > > > +{\n> > > > +public:\n> > > > + CameraSensorHelperExponential()\n> > > > + {\n> > > > + gainType_ = AnalogueGainExponential;\n> > > > + gainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > > > + }\n> > > > +};\n> > > > +\n> > > > +class CameraSensorHelperLinear : public CameraSensorHelper\n> > > > +{\n> > > > +public:\n> > > > + CameraSensorHelperLinear()\n> > > > + {\n> > > > + gainType_ = AnalogueGainLinear;\n> > > > + gainConstants_.linear = { 0, 1024, -1, 1024 };\n> > > > + }\n> > > > +};\n> > > \n> > > Could we test all the helpers, instead of two particular implementations\n> > > ? CameraSensorHelperFactoryBase provides a list of factories. We would\n> > \n> > ! Please see my first iteration of this Test where I did exactly this.\n> > \n> > Someone called 'Laurent' asked me to change the implementation to this\n> > method instead.... Could you have words with him please? ;-)\n> \n> Well, technically, I've only hinted a possible alternative, I didn't say\n> it was the best solution, but... OK, I stand corrected, I was wrong :-)\n\nIt's fine ... now we know ;-)\n\n> > - https://lists.libcamera.org/pipermail/libcamera-devel/2024-February/040629.html\n> > \n> > > need to expose their name through a const accessor to be able to call\n> > > create(). Or do you think that would be overkill ? We have helpers that\n> > \n> > You mean ... exactly like this perhaps?:\n> > - https://lists.libcamera.org/pipermail/libcamera-devel/2024-February/040559.html\n> > - https://patchwork.libcamera.org/patch/19529/\n> > \n> > I don't think that's overkill. I implemented it ;-)\n> > \n> > I did also say:\n> > \n> > \"\"\"\n> > An alternative here is to just allow direct access to the\n> > createInstance() from the test too, without having to re-search\n> > all of the factories each time. But I felt this was less\n> > of a de-restriction of the existing interfaces.\n> > \"\"\"\n> > \n> > > don't use the two standard models, because someone decided to implement\n> > > something more \"clever\" in the hardware. It would be useful to cover\n> > > those in the tests I think.\n> > \n> > I absolutely agree. that's why I wanted to iterate /all/ helpers and\n> > test each one explicitly in https://patchwork.libcamera.org/patch/19530/\n> \n> Amazing, the work is already done :-)\n\nGreat when that happens hey ;D\n\n> > > > +\n> > > > +class CameraSensorHelperTest : public Test\n> > > \n> > > As this tests the gain models only, I'd name the class\n> > > CameraSensorGainsTest or something similar. Same for the source file.\n> > > \n> > > > +{\n> > > > +protected:\n> > > > + int testGainModel(CameraSensorHelper &helper)\n> > > > + {\n> > > > + int ret = TestPass;\n> > > > +\n> > > > + /*\n> > > > + * Arbitrarily test 255 code positions assuming 8 bit fields\n> > > > + * are the normal maximum for a gain code register.\n> > > > + */\n> > > \n> > > This bothers me, mostly because the right solution would require\n> > > extending the helpers to expose the min/max gains, and *that* is\n> > > bothersome :-S So far we rely on kernel drivers to provide the\n> > > information. Do you think we should bite the bullet ?\n> > \n> > In fact, I questioned this too in https://patchwork.libcamera.org/patch/19530/\n> > \n> > \"\"\"\n> > Because that's what value the IMX283 uses. Yes - as I said above, and\n> > why this is an RFC, we don't have a way to get the max value from here.\n> > But getting it is not so clear. Should we define the max code for every\n> > camera sensor helper? Should it be defaulted to something supplied from\n> > the driver? (How?) or are we then just duplicating and hardcoding values\n> > from v4l2 drivers here in the helpers... maybe that's fine ?\n> > \"\"\"\n> \n> So... Biting the bullet ? We already hardcode the gain model in\n> libcamera, because sensor drivers don't provide any information about\n> it. We could consider that the min/max values are part of the model. The\n> drawback is that we'll have problems if the kernel, for any reason,\n> decides to implement a different range.\n\nYes, but I think we'll roughly know if that happens as we have a good\neye on the kernel.\n\nI think defining the range of code values as you suggested in another\npatch lets us make good progress here.\n\n\n> > > One issue to consider is that the gain code may not be a continuous\n> > > value. A sensor I'm working on splits the analog gain in coarse and fine\n> > > gains, stored in bits [6:4] and [3:0]. The coarse gain is an exponential\n> > > gain, expressed as the exponent of a power of two, ranging from 2^0 to\n> > > 2^4. The fine gain is an inverse linear gain following the formula\n> > > \n> > > 1 / (1 - fine / 32)\n> > > \n> > > The fine gain value is quantized differently depending on the coarse\n> > > gain. When the coarse gain is 2^0 or 2^2, the fine gain code can take\n> > > any value in the [0, 15] range. When the coarse gain is 2^1 or 2^3, the\n> > > fine gain code is restricted to even values. When the coarse gain is\n> > > 2^4, the fine gain code is restricted to multiples of 4.\n> > \n> > We do have sensors which encode both analog and digital gain into a\n> > single control too.\n> > \n> > However for those so far, we artificially limit the gain to be only the\n> > analog component in the kernel drivers.\n> > \n> > See:\n> > https://lore.kernel.org/all/20240414140621.167717-7-umang.jain@ideasonboard.com/\n> \n> The sensor may store the digital and analog gain values in the same\n> register, but they're still two separate gains, the kernel driver needs\n> to split the value in two V4L2 controls. I'm not concerned here.\n\nOk, that can be handled kernel side then (if/when anyone /needs/ that much\ngain....)\n\n> > > I believe the hardware will operate correctly when those constraints are\n> > > not met, but will ignore the LSB or 2 LSBs of the fine gain code\n> > > depending on the coarse gain. This leads to, for instance,\n> > > \n> > > coarse = 2^1, fine = 4 -> gain = 2.2857142857142856\n> > > coarse = 2^1, fine = 5 -> gain = 2.2857142857142856\n> > > \n> > > The code -> gain -> code conversion can lead to the same result for all\n> > > valid code values, but not for all code values in the [min, max] range.\n> > > I wonder how to handle that in the test.\n> > \n> > Yikes....\n> \n> And I haven't even mentioned that there's another fixed gain multiplier\n> added when the coarse gain is > 2^1 :-)\n\nErr .. Yeah - I'll leave all that to you. The graph on that gain model\nalready looks crazy enough ;-)\n\n--\nKieran","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 90E5BBD87C\n\tfor ;\n\tWed, 5 Jun 2024 09:38:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B925B6543E;\n\tWed, 5 Jun 2024 11:38:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F6C2634B5\n\tfor ;\n\tWed, 5 Jun 2024 11:38:40 +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 AB49F14B0;\n\tWed, 5 Jun 2024 11:38:31 +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=\"nt+ttQVc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717580311;\n\tbh=bZydRAFQrX550Caj0S+J4MMjuDHc2MwLA0R+BTlOyzo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=nt+ttQVcXnOvCT3V9tGhcVR5uPUTguRJUUCa+yGOjMf870DHcHRRbfDiOlWDrq/SW\n\t3RLw0h+TxnBHfI/Nh/d0tVrTu8ohzz4h8O0JoMS1q5S02N4KiZkoJVWqQcPjXMplp+\n\tU3J4IzE8lgUfGXQBl3jTJPnOhOjI5MQpeRaXEE94=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240603171024.GA5667@pendragon.ideasonboard.com>","References":"<20240524135256.649406-1-kieran.bingham@ideasonboard.com>\n\t<20240524135256.649406-2-kieran.bingham@ideasonboard.com>\n\t<20240603163208.GA7043@pendragon.ideasonboard.com>\n\t<171743379660.205609.9436321327379359062@ping.linuxembedded.co.uk>\n\t<20240603171024.GA5667@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","From":"Kieran Bingham ","Cc":"libcamera devel ","To":"Laurent Pinchart ","Date":"Wed, 05 Jun 2024 10:38:37 +0100","Message-ID":"<171758031722.205609.7571558848372932514@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\" "}}]