[{"id":28739,"web_url":"https://patchwork.libcamera.org/comment/28739/","msgid":"<3tkwx4mm4ywycesruo735cqiripjhcj3yynpq7fuv7doxrqdpa@ykuajrtrb3ok>","date":"2024-02-26T14:50:46","subject":"Re: [RFC PATCH 2/3] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Fri, Feb 23, 2024 at 03:59:53PM +0000, Kieran Bingham wrote:\n> Introduce a test that validates conversion of the gain model\n> in each CameraSensorHelper.\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> This test fails on the following CameraSensorHelpers:\n> - imx219 imx258 imx283 imx290 imx296 imx327 imx335 imx477\n>\n> Signed-off-by: Kieran Bingham \n> ---\n>\n> This test also fails on the ar0521 helper as that has a defined code\n> limit of '63', which the test does not yet have a way to infer.\n>\n> Adding a 'maxCode' helper to the base class may be the way to go here,\n> and would also let us define a maximum value for other helpers directly.\n>\n> test/ipa/camera_sensor_helper.cpp | 69 +++++++++++++++++++++++++++++++\n> test/ipa/meson.build | 1 +\n> 2 files changed, 70 insertions(+)\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..2d37628f87c4\n> --- /dev/null\n> +++ b/test/ipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,69 @@\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> +class CameraSensorHelperTest : public Test\n> +{\n> +protected:\n> +\tint testGainModel(std::string model)\n> +\t{\n> +\t\tint ret = TestPass;\n> +\n> +\t\tstd::unique_ptr camHelper_;\n> +\n> +\t\tcamHelper_ = CameraSensorHelperFactoryBase::create(model);\n> +\t\tif (!camHelper_) {\n> +\t\t\tstd::cout\n> +\t\t\t\t<< \"Failed to create camera sensor helper for \"\n> +\t\t\t\t<< model;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tfor (unsigned int i = 0; i < 240; i++) {\n\nHow did you establish 0 and 240 as the min/max ? As you suggested\nabove, isn't it better if the helper advertise these ?\n\n> +\t\t\tfloat gain = camHelper_->gain(i);\n> +\t\t\tuint32_t gainCode = camHelper_->gainCode(gain);\n> +\n> +\t\t\tif (i != gainCode) {\n> +\t\t\t\tstd::cout << model << \": 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\tstd::vector factories;\n> +\n> +\t\tfor (auto factory : CameraSensorHelperFactoryBase::factories()) {\n> +\t\t\tconst std::string &model = factory->name();\n> +\n> +\t\t\tcout << \"Testing CameraSensorHelper for \" << model << endl;\n> +\n> +\t\t\tif (testGainModel(factory->name()) == TestFail)\n> +\t\t\t\tfailures++;\n> +\t\t}\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..c99385a7cb8b 100644\n> --- a/test/ipa/meson.build\n> +++ b/test/ipa/meson.build\n> @@ -1,6 +1,7 @@\n> # SPDX-License-Identifier: CC0-1.0\n>\n> ipa_test = [\n> + {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp']},\n> {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> ]\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EC3C7BD160\n\tfor ;\n\tMon, 26 Feb 2024 14:50:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2BF7262879;\n\tMon, 26 Feb 2024 15:50:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5ECBE62871\n\tfor ;\n\tMon, 26 Feb 2024 15:50:50 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C4FD9CD1;\n\tMon, 26 Feb 2024 15:50:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fkQTKpLB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1708959038;\n\tbh=eqBhAOtFEe0quF7IJo5g6Jnv9n52BLtABEMJkvX4Xwg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fkQTKpLBsax/fyjLZf8gMc1Wbpt8cDK+Wrt8wAmbihXvI0oGHfTtJqOgTnTwfU3Y6\n\t/DVvbmZFjmJj+vUZCY5+9dpljQiefktfldKdTqUy3HDa3agTVK/SScK5tgrcMOsrFz\n\t7FLbU81GC8+RRXlbWHFF73r22YeOYloZlWX1Yw2o=","Date":"Mon, 26 Feb 2024 15:50:46 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Subject":"Re: [RFC PATCH 2/3] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","Message-ID":"<3tkwx4mm4ywycesruo735cqiripjhcj3yynpq7fuv7doxrqdpa@ykuajrtrb3ok>","References":"<20240223155954.4139705-1-kieran.bingham@ideasonboard.com>\n\t<20240223155954.4139705-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240223155954.4139705-3-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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28742,"web_url":"https://patchwork.libcamera.org/comment/28742/","msgid":"<170896646747.2629073.14066332392457792174@ping.linuxembedded.co.uk>","date":"2024-02-26T16:54:27","subject":"Re: [RFC PATCH 2/3] 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 Jacopo Mondi (2024-02-26 14:50:46)\n> Hi Kieran\n> \n> On Fri, Feb 23, 2024 at 03:59:53PM +0000, Kieran Bingham wrote:\n> > Introduce a test that validates conversion of the gain model\n> > in each CameraSensorHelper.\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> > This test fails on the following CameraSensorHelpers:\n> > - imx219 imx258 imx283 imx290 imx296 imx327 imx335 imx477\n> >\n> > Signed-off-by: Kieran Bingham \n> > ---\n> >\n> > This test also fails on the ar0521 helper as that has a defined code\n> > limit of '63', which the test does not yet have a way to infer.\n> >\n> > Adding a 'maxCode' helper to the base class may be the way to go here,\n> > and would also let us define a maximum value for other helpers directly.\n> >\n> > test/ipa/camera_sensor_helper.cpp | 69 +++++++++++++++++++++++++++++++\n> > test/ipa/meson.build | 1 +\n> > 2 files changed, 70 insertions(+)\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..2d37628f87c4\n> > --- /dev/null\n> > +++ b/test/ipa/camera_sensor_helper.cpp\n> > @@ -0,0 +1,69 @@\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> > +class CameraSensorHelperTest : public Test\n> > +{\n> > +protected:\n> > + int testGainModel(std::string model)\n> > + {\n> > + int ret = TestPass;\n> > +\n> > + std::unique_ptr camHelper_;\n> > +\n> > + camHelper_ = CameraSensorHelperFactoryBase::create(model);\n> > + if (!camHelper_) {\n> > + std::cout\n> > + << \"Failed to create camera sensor helper for \"\n> > + << model;\n> > + return TestFail;\n> > + }\n> > +\n> > + for (unsigned int i = 0; i < 240; i++) {\n> \n> How did you establish 0 and 240 as the min/max ? As you suggested\n> above, isn't it better if the helper advertise these ?\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--\nKieran\n\n\n> \n> > + float gain = camHelper_->gain(i);\n> > + uint32_t gainCode = camHelper_->gainCode(gain);\n> > +\n> > + if (i != gainCode) {\n> > + std::cout << model << \": 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> > + std::vector factories;\n> > +\n> > + for (auto factory : CameraSensorHelperFactoryBase::factories()) {\n> > + const std::string &model = factory->name();\n> > +\n> > + cout << \"Testing CameraSensorHelper for \" << model << endl;\n> > +\n> > + if (testGainModel(factory->name()) == TestFail)\n> > + failures++;\n> > + }\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..c99385a7cb8b 100644\n> > --- a/test/ipa/meson.build\n> > +++ b/test/ipa/meson.build\n> > @@ -1,6 +1,7 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> >\n> > ipa_test = [\n> > + {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp']},\n> > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> > ]\n> > --\n> > 2.34.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 068E8BD160\n\tfor ;\n\tMon, 26 Feb 2024 16:54:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50CA26285F;\n\tMon, 26 Feb 2024 17:54:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 08AB661C98\n\tfor ;\n\tMon, 26 Feb 2024 17:54:30 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 67C06CD1;\n\tMon, 26 Feb 2024 17:54:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"li7uUprz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1708966458;\n\tbh=WsB9pDryp/cWSR4pOb2cvH81hm1yi5b37E3DmxOPSEU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=li7uUprzYHotwCt0DTatrV+Abf7D3DFdISyjMPksWI31GoBYD99PkqR8sUgMvP577\n\tSEz8ER3T2/U6WLtitwVO4HnZqp5urym8y73XsuAF0oC37S1gv1p0aoR7yVXEL+we6g\n\tVMl6jIfwvisZ6/RAmp98LY7AnA3VJwsNxHI9vFRE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<3tkwx4mm4ywycesruo735cqiripjhcj3yynpq7fuv7doxrqdpa@ykuajrtrb3ok>","References":"<20240223155954.4139705-1-kieran.bingham@ideasonboard.com>\n\t<20240223155954.4139705-3-kieran.bingham@ideasonboard.com>\n\t<3tkwx4mm4ywycesruo735cqiripjhcj3yynpq7fuv7doxrqdpa@ykuajrtrb3ok>","Subject":"Re: [RFC PATCH 2/3] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","From":"Kieran Bingham ","To":"Jacopo Mondi ","Date":"Mon, 26 Feb 2024 16:54:27 +0000","Message-ID":"<170896646747.2629073.14066332392457792174@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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28743,"web_url":"https://patchwork.libcamera.org/comment/28743/","msgid":"","date":"2024-02-26T17:08:58","subject":"Re: [RFC PATCH 2/3] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Mon, Feb 26, 2024 at 04:54:27PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2024-02-26 14:50:46)\n> > Hi Kieran\n> >\n> > On Fri, Feb 23, 2024 at 03:59:53PM +0000, Kieran Bingham wrote:\n> > > Introduce a test that validates conversion of the gain model\n> > > in each CameraSensorHelper.\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> > > This test fails on the following CameraSensorHelpers:\n> > > - imx219 imx258 imx283 imx290 imx296 imx327 imx335 imx477\n> > >\n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > >\n> > > This test also fails on the ar0521 helper as that has a defined code\n> > > limit of '63', which the test does not yet have a way to infer.\n> > >\n> > > Adding a 'maxCode' helper to the base class may be the way to go here,\n> > > and would also let us define a maximum value for other helpers directly.\n> > >\n> > > test/ipa/camera_sensor_helper.cpp | 69 +++++++++++++++++++++++++++++++\n> > > test/ipa/meson.build | 1 +\n> > > 2 files changed, 70 insertions(+)\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..2d37628f87c4\n> > > --- /dev/null\n> > > +++ b/test/ipa/camera_sensor_helper.cpp\n> > > @@ -0,0 +1,69 @@\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> > > +class CameraSensorHelperTest : public Test\n> > > +{\n> > > +protected:\n> > > + int testGainModel(std::string model)\n> > > + {\n> > > + int ret = TestPass;\n> > > +\n> > > + std::unique_ptr camHelper_;\n> > > +\n> > > + camHelper_ = CameraSensorHelperFactoryBase::create(model);\n> > > + if (!camHelper_) {\n> > > + std::cout\n> > > + << \"Failed to create camera sensor helper for \"\n> > > + << model;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + for (unsigned int i = 0; i < 240; i++) {\n> >\n> > How did you establish 0 and 240 as the min/max ? As you suggested\n> > above, isn't it better if the helper advertise these ?\n\nThis also assume the gain codes range is continuous, but we anyway\nassume that in our helpers..\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\nJust to throw more options in the mix, we also have the min/max v4l2\ncontrol values. Not accessible by this test though...\n\nDuplicating those values in the camera sensor helpers might require we\nare really really careful and keep in sync the kernel driver and the\nhelper, but we already have many things to keep synchronized, so\nthat's maybe not that bad ?\n\nAnyway, you here use the gain codes and from there you get to the gain\nvalue and then back to the code. Isn't it better to do the other way\naround and test in a 'safe' 1x-16x range ?\n\nUsing gain code is 'risky' different sensors might express gains with\ndifferent Q formats, with non-continuous ranges etc.. True, we're only\ntesting the helpers without going to the device here..\n\n>\n> --\n> Kieran\n>\n>\n> >\n> > > + float gain = camHelper_->gain(i);\n> > > + uint32_t gainCode = camHelper_->gainCode(gain);\n> > > +\n> > > + if (i != gainCode) {\n> > > + std::cout << model << \": 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> > > + std::vector factories;\n> > > +\n> > > + for (auto factory : CameraSensorHelperFactoryBase::factories()) {\n> > > + const std::string &model = factory->name();\n> > > +\n> > > + cout << \"Testing CameraSensorHelper for \" << model << endl;\n> > > +\n> > > + if (testGainModel(factory->name()) == TestFail)\n> > > + failures++;\n> > > + }\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..c99385a7cb8b 100644\n> > > --- a/test/ipa/meson.build\n> > > +++ b/test/ipa/meson.build\n> > > @@ -1,6 +1,7 @@\n> > > # SPDX-License-Identifier: CC0-1.0\n> > >\n> > > ipa_test = [\n> > > + {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp']},\n> > > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> > > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> > > ]\n> > > --\n> > > 2.34.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 79ECBBD80A\n\tfor ;\n\tMon, 26 Feb 2024 17:09:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E896D61C9A;\n\tMon, 26 Feb 2024 18:09:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE0FA61C98\n\tfor ;\n\tMon, 26 Feb 2024 18:09:01 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 04123673;\n\tMon, 26 Feb 2024 18:08:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ciDZlNSl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1708967330;\n\tbh=BuSZlNZolw6leOYob356FkGYv4dbmXtTTXDEFccrmYQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ciDZlNSlOlncR/048jfc+pcYEOiR2mDxfGx8A/Srq6m5Zf8D7ZD40DUts7pJA0TxR\n\txuziF2cr34TPTIp7CV9DEIpHAs3S2499mrtwuJalA4r7c4SCtDOI7hplHcWtmmDhQd\n\tbSNkhRSLcch3sVTRE+vzCLVNWnZvPJ44J/t1xcMM=","Date":"Mon, 26 Feb 2024 18:08:58 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Subject":"Re: [RFC PATCH 2/3] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","Message-ID":"","References":"<20240223155954.4139705-1-kieran.bingham@ideasonboard.com>\n\t<20240223155954.4139705-3-kieran.bingham@ideasonboard.com>\n\t<3tkwx4mm4ywycesruo735cqiripjhcj3yynpq7fuv7doxrqdpa@ykuajrtrb3ok>\n\t<170896646747.2629073.14066332392457792174@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<170896646747.2629073.14066332392457792174@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","Cc":"Jacopo Mondi ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28764,"web_url":"https://patchwork.libcamera.org/comment/28764/","msgid":"<20240227150551.GB28713@pendragon.ideasonboard.com>","date":"2024-02-27T15:05:51","subject":"Re: [RFC PATCH 2/3] 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, Feb 26, 2024 at 03:50:46PM +0100, Jacopo Mondi wrote:\n> On Fri, Feb 23, 2024 at 03:59:53PM +0000, Kieran Bingham wrote:\n> > Introduce a test that validates conversion of the gain model\n> > in each CameraSensorHelper.\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> > This test fails on the following CameraSensorHelpers:\n> > - imx219 imx258 imx283 imx290 imx296 imx327 imx335 imx477\n> >\n> > Signed-off-by: Kieran Bingham \n> > ---\n> >\n> > This test also fails on the ar0521 helper as that has a defined code\n> > limit of '63', which the test does not yet have a way to infer.\n\nWhere does that value come from ? I don't see it in the code.\n\n> > Adding a 'maxCode' helper to the base class may be the way to go here,\n> > and would also let us define a maximum value for other helpers directly.\n\nOne option would be to retrieve the maximum by calling gainCode(very\nhigh value). Or that would be an option if the helpers actually\nimplemented limits, which is currently not the case. If I recall\ncorrectly, we get the gain code limits dynamically from the kernel\ndriver. Hardcoding limits in libcamera could be problematic.\n\nHow about creating subclasses of the camera sensor class in this test,\nwith different models and coefficients ? That would also remove the\nrequirement of exposing the factory names.\n\n> > test/ipa/camera_sensor_helper.cpp | 69 +++++++++++++++++++++++++++++++\n> > test/ipa/meson.build | 1 +\n> > 2 files changed, 70 insertions(+)\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..2d37628f87c4\n> > --- /dev/null\n> > +++ b/test/ipa/camera_sensor_helper.cpp\n> > @@ -0,0 +1,69 @@\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> > +class CameraSensorHelperTest : public Test\n> > +{\n> > +protected:\n> > +\tint testGainModel(std::string model)\n> > +\t{\n> > +\t\tint ret = TestPass;\n> > +\n> > +\t\tstd::unique_ptr camHelper_;\n> > +\n> > +\t\tcamHelper_ = CameraSensorHelperFactoryBase::create(model);\n> > +\t\tif (!camHelper_) {\n> > +\t\t\tstd::cout\n> > +\t\t\t\t<< \"Failed to create camera sensor helper for \"\n> > +\t\t\t\t<< model;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tfor (unsigned int i = 0; i < 240; i++) {\n> \n> How did you establish 0 and 240 as the min/max ? As you suggested\n> above, isn't it better if the helper advertise these ?\n> \n> > +\t\t\tfloat gain = camHelper_->gain(i);\n> > +\t\t\tuint32_t gainCode = camHelper_->gainCode(gain);\n> > +\n> > +\t\t\tif (i != gainCode) {\n> > +\t\t\t\tstd::cout << model << \": 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\tstd::vector factories;\n> > +\n> > +\t\tfor (auto factory : CameraSensorHelperFactoryBase::factories()) {\n> > +\t\t\tconst std::string &model = factory->name();\n> > +\n> > +\t\t\tcout << \"Testing CameraSensorHelper for \" << model << endl;\n> > +\n> > +\t\t\tif (testGainModel(factory->name()) == TestFail)\n> > +\t\t\t\tfailures++;\n> > +\t\t}\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..c99385a7cb8b 100644\n> > --- a/test/ipa/meson.build\n> > +++ b/test/ipa/meson.build\n> > @@ -1,6 +1,7 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> >\n> > ipa_test = [\n> > + {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp']},\n> > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\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 A16FFBD160\n\tfor ;\n\tTue, 27 Feb 2024 15:05:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C2266285F;\n\tTue, 27 Feb 2024 16:05:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5B1662806\n\tfor ;\n\tTue, 27 Feb 2024 16:05:49 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 91FE08D4;\n\tTue, 27 Feb 2024 16:05:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Z2vnBXLA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709046337;\n\tbh=kXfztaeeMcW1BK0R7GcOsNmE369A6RcGO+ard4095dY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z2vnBXLAQyU+Dw6Yv5JtoLmQs+lKOYFkwumvU1V4yTBCnPg865l65pKutxUIaCBQJ\n\tOdXKT8eR7FzRGyMMW2DQUEhHIi4okV2CayzKNBB79vX95LaTr0uoHFCTKqLtnVu+Mg\n\tb9zTbZLDu0qxGrBsC9AFmHj15J5gBbzMFW/eNnes=","Date":"Tue, 27 Feb 2024 17:05:51 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Subject":"Re: [RFC PATCH 2/3] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","Message-ID":"<20240227150551.GB28713@pendragon.ideasonboard.com>","References":"<20240223155954.4139705-1-kieran.bingham@ideasonboard.com>\n\t<20240223155954.4139705-3-kieran.bingham@ideasonboard.com>\n\t<3tkwx4mm4ywycesruo735cqiripjhcj3yynpq7fuv7doxrqdpa@ykuajrtrb3ok>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<3tkwx4mm4ywycesruo735cqiripjhcj3yynpq7fuv7doxrqdpa@ykuajrtrb3ok>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28769,"web_url":"https://patchwork.libcamera.org/comment/28769/","msgid":"<170905395246.1406778.4189083197152054540@ping.linuxembedded.co.uk>","date":"2024-02-27T17:12:32","subject":"Re: [RFC PATCH 2/3] 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-02-27 15:05:51)\n> Hi Kieran\n> \n> On Mon, Feb 26, 2024 at 03:50:46PM +0100, Jacopo Mondi wrote:\n> > On Fri, Feb 23, 2024 at 03:59:53PM +0000, Kieran Bingham wrote:\n> > > Introduce a test that validates conversion of the gain model\n> > > in each CameraSensorHelper.\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> > > This test fails on the following CameraSensorHelpers:\n> > > - imx219 imx258 imx283 imx290 imx296 imx327 imx335 imx477\n> > >\n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > >\n> > > This test also fails on the ar0521 helper as that has a defined code\n> > > limit of '63', which the test does not yet have a way to infer.\n> \n> Where does that value come from ? I don't see it in the code.\n\nIt's what the test outputs as the maximum value. All values after this\nfail, and only 63 is returned for any value above this.\n\n\nI expect it's this:\n\n \nuint32_t CameraSensorHelperAr0521::gainCode(double gain) const\n{\n\tgain = std::clamp(gain, 1.0, 15.5);\n\t...\n}\n\n\n> > > Adding a 'maxCode' helper to the base class may be the way to go here,\n> > > and would also let us define a maximum value for other helpers directly.\n> \n> One option would be to retrieve the maximum by calling gainCode(very\n> high value). Or that would be an option if the helpers actually\n> implemented limits, which is currently not the case. If I recall\n> correctly, we get the gain code limits dynamically from the kernel\n> driver. Hardcoding limits in libcamera could be problematic.\n> \n> How about creating subclasses of the camera sensor class in this test,\n> with different models and coefficients ? That would also remove the\n> requirement of exposing the factory names.\n\nDo you mean only validate 3 instances? Instead of validating every\nCameraSensorHelper?\n\nI liked that this gives us the opportunity to validate every\nCameraSensorHelper that gets added, especially as they can be\noverridden.\n\nI was imagining that there would be other camera specific overrides that\nmay warrant additional tests in the future but maybe that's too early to\nidentify, or the other parts that will be added will be too simple to\ntest, unlike these calculations.\n\n--\nKieran\n\n> > > test/ipa/camera_sensor_helper.cpp | 69 +++++++++++++++++++++++++++++++\n> > > test/ipa/meson.build | 1 +\n> > > 2 files changed, 70 insertions(+)\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..2d37628f87c4\n> > > --- /dev/null\n> > > +++ b/test/ipa/camera_sensor_helper.cpp\n> > > @@ -0,0 +1,69 @@\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> > > +class CameraSensorHelperTest : public Test\n> > > +{\n> > > +protected:\n> > > + int testGainModel(std::string model)\n> > > + {\n> > > + int ret = TestPass;\n> > > +\n> > > + std::unique_ptr camHelper_;\n> > > +\n> > > + camHelper_ = CameraSensorHelperFactoryBase::create(model);\n> > > + if (!camHelper_) {\n> > > + std::cout\n> > > + << \"Failed to create camera sensor helper for \"\n> > > + << model;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + for (unsigned int i = 0; i < 240; i++) {\n> > \n> > How did you establish 0 and 240 as the min/max ? As you suggested\n> > above, isn't it better if the helper advertise these ?\n> > \n> > > + float gain = camHelper_->gain(i);\n> > > + uint32_t gainCode = camHelper_->gainCode(gain);\n> > > +\n> > > + if (i != gainCode) {\n> > > + std::cout << model << \": 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> > > + std::vector factories;\n> > > +\n> > > + for (auto factory : CameraSensorHelperFactoryBase::factories()) {\n> > > + const std::string &model = factory->name();\n> > > +\n> > > + cout << \"Testing CameraSensorHelper for \" << model << endl;\n> > > +\n> > > + if (testGainModel(factory->name()) == TestFail)\n> > > + failures++;\n> > > + }\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..c99385a7cb8b 100644\n> > > --- a/test/ipa/meson.build\n> > > +++ b/test/ipa/meson.build\n> > > @@ -1,6 +1,7 @@\n> > > # SPDX-License-Identifier: CC0-1.0\n> > >\n> > > ipa_test = [\n> > > + {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp']},\n> > > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> > > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> > > ]\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 633BFBD160\n\tfor ;\n\tTue, 27 Feb 2024 17:12:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7DF0662867;\n\tTue, 27 Feb 2024 18:12:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93EB062806\n\tfor ;\n\tTue, 27 Feb 2024 18:12:35 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4A5E18D4;\n\tTue, 27 Feb 2024 18:12:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"c/kiAJff\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709053943;\n\tbh=QbYYiVKrkMSBIcWM5x3ta4YF0mFWoDz4joSf3BRDvtQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=c/kiAJffgxWkdFiC+r1c9qAqKsOmFeEzrFq7friUBubAvZHKwWRmlSnTcoOo560Vu\n\tJ8chGu4oi658ju7oLuq/e6k4kt0LGck7b0L0RFnj+55ffHGSkLGMaDKiGEX69uyF6F\n\tFX90nwVFnNz/VJ8IloID/HmdysXaBA70WkOUb5dk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240227150551.GB28713@pendragon.ideasonboard.com>","References":"<20240223155954.4139705-1-kieran.bingham@ideasonboard.com>\n\t<20240223155954.4139705-3-kieran.bingham@ideasonboard.com>\n\t<3tkwx4mm4ywycesruo735cqiripjhcj3yynpq7fuv7doxrqdpa@ykuajrtrb3ok>\n\t<20240227150551.GB28713@pendragon.ideasonboard.com>","Subject":"Re: [RFC PATCH 2/3] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","From":"Kieran Bingham ","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Date":"Tue, 27 Feb 2024 17:12:32 +0000","Message-ID":"<170905395246.1406778.4189083197152054540@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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29612,"web_url":"https://patchwork.libcamera.org/comment/29612/","msgid":"<171655523206.1456831.10537703510799197002@ping.linuxembedded.co.uk>","date":"2024-05-24T12:53:52","subject":"Re: [RFC PATCH 2/3] 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 Jacopo Mondi (2024-02-26 17:08:58)\n> Hi Kieran\n> \n> On Mon, Feb 26, 2024 at 04:54:27PM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2024-02-26 14:50:46)\n> > > Hi Kieran\n> > >\n> > > On Fri, Feb 23, 2024 at 03:59:53PM +0000, Kieran Bingham wrote:\n> > > > Introduce a test that validates conversion of the gain model\n> > > > in each CameraSensorHelper.\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> > > > This test fails on the following CameraSensorHelpers:\n> > > > - imx219 imx258 imx283 imx290 imx296 imx327 imx335 imx477\n> > > >\n> > > > Signed-off-by: Kieran Bingham \n> > > > ---\n> > > >\n> > > > This test also fails on the ar0521 helper as that has a defined code\n> > > > limit of '63', which the test does not yet have a way to infer.\n> > > >\n> > > > Adding a 'maxCode' helper to the base class may be the way to go here,\n> > > > and would also let us define a maximum value for other helpers directly.\n> > > >\n> > > > test/ipa/camera_sensor_helper.cpp | 69 +++++++++++++++++++++++++++++++\n> > > > test/ipa/meson.build | 1 +\n> > > > 2 files changed, 70 insertions(+)\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..2d37628f87c4\n> > > > --- /dev/null\n> > > > +++ b/test/ipa/camera_sensor_helper.cpp\n> > > > @@ -0,0 +1,69 @@\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> > > > +class CameraSensorHelperTest : public Test\n> > > > +{\n> > > > +protected:\n> > > > + int testGainModel(std::string model)\n> > > > + {\n> > > > + int ret = TestPass;\n> > > > +\n> > > > + std::unique_ptr camHelper_;\n> > > > +\n> > > > + camHelper_ = CameraSensorHelperFactoryBase::create(model);\n> > > > + if (!camHelper_) {\n> > > > + std::cout\n> > > > + << \"Failed to create camera sensor helper for \"\n> > > > + << model;\n> > > > + return TestFail;\n> > > > + }\n> > > > +\n> > > > + for (unsigned int i = 0; i < 240; i++) {\n> > >\n> > > How did you establish 0 and 240 as the min/max ? As you suggested\n> > > above, isn't it better if the helper advertise these ?\n> \n> This also assume the gain codes range is continuous, but we anyway\n> assume that in our helpers..\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> Just to throw more options in the mix, we also have the min/max v4l2\n> control values. Not accessible by this test though...\n> \n> Duplicating those values in the camera sensor helpers might require we\n> are really really careful and keep in sync the kernel driver and the\n> helper, but we already have many things to keep synchronized, so\n> that's maybe not that bad ?\n> \n> Anyway, you here use the gain codes and from there you get to the gain\n> value and then back to the code. Isn't it better to do the other way\n> around and test in a 'safe' 1x-16x range ?\n\nBut what steps should we use on the gain? How do we validate a 1-1\nmapping when we don't know the gain code?\n\nThe advantage of comparing a gain code to a gain and back again is that\nit verifies the exactness of the conversion (which is what the patch for\n3/3 corrects).\n\n\n> Using gain code is 'risky' different sensors might express gains with\n> different Q formats, with non-continuous ranges etc.. True, we're only\n> testing the helpers without going to the device here..\n\nYes, and right now our helpers are ... wrong! Which this test\nhighlights.\n\nMaybe should I concentrate on trying to merge the fix without the test?\nIt seems testing this seems to be controversial :-(\n\n--\nKieran\n\n\n> >\n> > --\n> > Kieran\n> >\n> >\n> > >\n> > > > + float gain = camHelper_->gain(i);\n> > > > + uint32_t gainCode = camHelper_->gainCode(gain);\n> > > > +\n> > > > + if (i != gainCode) {\n> > > > + std::cout << model << \": 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> > > > + std::vector factories;\n> > > > +\n> > > > + for (auto factory : CameraSensorHelperFactoryBase::factories()) {\n> > > > + const std::string &model = factory->name();\n> > > > +\n> > > > + cout << \"Testing CameraSensorHelper for \" << model << endl;\n> > > > +\n> > > > + if (testGainModel(factory->name()) == TestFail)\n> > > > + failures++;\n> > > > + }\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..c99385a7cb8b 100644\n> > > > --- a/test/ipa/meson.build\n> > > > +++ b/test/ipa/meson.build\n> > > > @@ -1,6 +1,7 @@\n> > > > # SPDX-License-Identifier: CC0-1.0\n> > > >\n> > > > ipa_test = [\n> > > > + {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp']},\n> > > > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> > > > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> > > > ]\n> > > > --\n> > > > 2.34.1\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3A0CABDE6B\n\tfor ;\n\tFri, 24 May 2024 12:53:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38751634AC;\n\tFri, 24 May 2024 14:53:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A630D61A49\n\tfor ;\n\tFri, 24 May 2024 14:53:55 +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 5FEFF9C1;\n\tFri, 24 May 2024 14:53:41 +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=\"D8wIWirC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716555221;\n\tbh=FftRG5HfbsH4dfFIauzqWtmH3E3ejRbCnPXr4GwU1KU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=D8wIWirCtxUB3c7OgcGsGO5W5Ih00mlPZ0fN0F1tC6nzUV0e20dlca1mBZ8YBSb4T\n\tnlK5WuU0nroOs2B7hJ8WOhE3zpy2JVyVQ0mwojIKNxXC0jWwHusu0aBPv96Gc5eq7f\n\tkdOrWmwMd4f/ft9EDtS/9ui6ZAu3+IiaHMRxt/Ok=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240223155954.4139705-1-kieran.bingham@ideasonboard.com>\n\t<20240223155954.4139705-3-kieran.bingham@ideasonboard.com>\n\t<3tkwx4mm4ywycesruo735cqiripjhcj3yynpq7fuv7doxrqdpa@ykuajrtrb3ok>\n\t<170896646747.2629073.14066332392457792174@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [RFC PATCH 2/3] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","From":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tlibcamera devel ","To":"Jacopo Mondi ","Date":"Fri, 24 May 2024 13:53:52 +0100","Message-ID":"<171655523206.1456831.10537703510799197002@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":29613,"web_url":"https://patchwork.libcamera.org/comment/29613/","msgid":"<171655540682.1456831.8159450558984092472@ping.linuxembedded.co.uk>","date":"2024-05-24T12:56:46","subject":"Re: [RFC PATCH 2/3] 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 Kieran Bingham (2024-02-27 17:12:32)\n> Quoting Laurent Pinchart (2024-02-27 15:05:51)\n> > Hi Kieran\n> > \n> > On Mon, Feb 26, 2024 at 03:50:46PM +0100, Jacopo Mondi wrote:\n> > > On Fri, Feb 23, 2024 at 03:59:53PM +0000, Kieran Bingham wrote:\n> > > > Introduce a test that validates conversion of the gain model\n> > > > in each CameraSensorHelper.\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> > > > This test fails on the following CameraSensorHelpers:\n> > > > - imx219 imx258 imx283 imx290 imx296 imx327 imx335 imx477\n> > > >\n> > > > Signed-off-by: Kieran Bingham \n> > > > ---\n> > > >\n> > > > This test also fails on the ar0521 helper as that has a defined code\n> > > > limit of '63', which the test does not yet have a way to infer.\n> > \n> > Where does that value come from ? I don't see it in the code.\n> \n> It's what the test outputs as the maximum value. All values after this\n> fail, and only 63 is returned for any value above this.\n> \n> \n> I expect it's this:\n> \n> \n> uint32_t CameraSensorHelperAr0521::gainCode(double gain) const\n> {\n> gain = std::clamp(gain, 1.0, 15.5);\n> ...\n> }\n> \n> \n> > > > Adding a 'maxCode' helper to the base class may be the way to go here,\n> > > > and would also let us define a maximum value for other helpers directly.\n> > \n> > One option would be to retrieve the maximum by calling gainCode(very\n> > high value). Or that would be an option if the helpers actually\n> > implemented limits, which is currently not the case. If I recall\n> > correctly, we get the gain code limits dynamically from the kernel\n> > driver. Hardcoding limits in libcamera could be problematic.\n> > \n> > How about creating subclasses of the camera sensor class in this test,\n> > with different models and coefficients ? That would also remove the\n> > requirement of exposing the factory names.\n\nI'll give it a quick go now.\n\nIt means we won't catch issues in future camera sensor helpers - but\nthat already requires extending the current camera sensor helpers with\nrepetetive data anyway so maybe this is sufficient for now.\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 81E85BDE6B\n\tfor ;\n\tFri, 24 May 2024 12:56:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C0C1634AC;\n\tFri, 24 May 2024 14:56:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A39C761A49\n\tfor ;\n\tFri, 24 May 2024 14:56:49 +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 500259C1;\n\tFri, 24 May 2024 14:56:35 +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=\"WgjgRHuF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716555395;\n\tbh=O3XDSZXighic6TowMjpu7DuoXGFF49Egf0CeL89hH6E=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=WgjgRHuFOtFdAxn89q8w6Hfh2Wbo2DRPXoNpVx1HlK6eSGviXbCcSlf5WZpAAUg2Z\n\teEcrVu4dLp4qwfoo9TTl5M0AegNKQzjyEKKmeVKPe2I+glxQikkGJRZABrEBjyOCCA\n\tKqX0nij9tyNTYuihzOG73qFD5moE6BfLiNSiOKNo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<170905395246.1406778.4189083197152054540@ping.linuxembedded.co.uk>","References":"<20240223155954.4139705-1-kieran.bingham@ideasonboard.com>\n\t<20240223155954.4139705-3-kieran.bingham@ideasonboard.com>\n\t<3tkwx4mm4ywycesruo735cqiripjhcj3yynpq7fuv7doxrqdpa@ykuajrtrb3ok>\n\t<20240227150551.GB28713@pendragon.ideasonboard.com>\n\t<170905395246.1406778.4189083197152054540@ping.linuxembedded.co.uk>","Subject":"Re: [RFC PATCH 2/3] test: ipa: libipa: Add CameraSensorHelper Gain\n\tModel tests","From":"Kieran Bingham ","Cc":"libcamera devel ","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Date":"Fri, 24 May 2024 13:56:46 +0100","Message-ID":"<171655540682.1456831.8159450558984092472@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\" "}}]