[PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain Model tests
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 3 19:10:24 CEST 2024
Hi Kieran,
On Mon, Jun 03, 2024 at 05:56:36PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-03 17:32:08)
> > On Fri, May 24, 2024 at 02:52:55PM +0100, Kieran Bingham wrote:
> > > Introduce a test that validates conversion of the gain model
> > > in both Linear and Exponential models.
> > >
> > > It should be expected that if a gainCode produces a given gain
> > > value, then converting that gain back should produce the same
> > > result.
> >
> > I think we have a general agreement on this.
>
> \o/ - I felt like was fighting a lonely corner over here for a while.
>
> > > The test is known to fail and is marked as such. The test will be
> > > updated with the corresponding fix.
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > > test/ipa/camera_sensor_helper.cpp | 101 ++++++++++++++++++++++++++++++
> > > test/ipa/meson.build | 5 +-
> > > 2 files changed, 105 insertions(+), 1 deletion(-)
> > > create mode 100644 test/ipa/camera_sensor_helper.cpp
> > >
> > > diff --git a/test/ipa/camera_sensor_helper.cpp b/test/ipa/camera_sensor_helper.cpp
> > > new file mode 100644
> > > index 000000000000..d8a8c6850a04
> > > --- /dev/null
> > > +++ b/test/ipa/camera_sensor_helper.cpp
> > > @@ -0,0 +1,101 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas on Board Oy.
> > > + */
> > > +
> > > +#include "libipa/camera_sensor_helper.h"
> > > +
> > > +#include <iostream>
> > > +#include <string.h>
> > > +
> > > +#include "test.h"
> > > +
> > > +using namespace std;
> > > +using namespace libcamera;
> > > +using namespace libcamera::ipa;
> > > +
> > > +/*
> > > + * Helper function to compute the m parameter of the exponential gain model
> > > + * when the gain code is expressed in dB.
> > > + */
> > > +static constexpr double expGainDb(double step)
> > > +{
> > > + constexpr double log2_10 = 3.321928094887362;
> > > +
> > > + /*
> > > + * The gain code is expressed in step * dB (e.g. in 0.1 dB steps):
> > > + *
> > > + * G_code = G_dB/step = 20/step*log10(G_linear)
> > > + *
> > > + * Inverting the formula, we get
> > > + *
> > > + * G_linear = 10^(step/20*G_code) = 2^(log2(10)*step/20*G_code)
> > > + */
> > > + return log2_10 * step / 20;
> > > +}
> > > +
> > > +class CameraSensorHelperExponential : public CameraSensorHelper
> > > +{
> > > +public:
> > > + CameraSensorHelperExponential()
> > > + {
> > > + gainType_ = AnalogueGainExponential;
> > > + gainConstants_.exp = { 1.0, expGainDb(0.3) };
> > > + }
> > > +};
> > > +
> > > +class CameraSensorHelperLinear : public CameraSensorHelper
> > > +{
> > > +public:
> > > + CameraSensorHelperLinear()
> > > + {
> > > + gainType_ = AnalogueGainLinear;
> > > + gainConstants_.linear = { 0, 1024, -1, 1024 };
> > > + }
> > > +};
> >
> > Could we test all the helpers, instead of two particular implementations
> > ? CameraSensorHelperFactoryBase provides a list of factories. We would
>
> ! Please see my first iteration of this Test where I did exactly this.
>
> Someone called 'Laurent' asked me to change the implementation to this
> method instead.... Could you have words with him please? ;-)
Well, technically, I've only hinted a possible alternative, I didn't say
it was the best solution, but... OK, I stand corrected, I was wrong :-)
> - https://lists.libcamera.org/pipermail/libcamera-devel/2024-February/040629.html
>
> > need to expose their name through a const accessor to be able to call
> > create(). Or do you think that would be overkill ? We have helpers that
>
> You mean ... exactly like this perhaps?:
> - https://lists.libcamera.org/pipermail/libcamera-devel/2024-February/040559.html
> - https://patchwork.libcamera.org/patch/19529/
>
> I don't think that's overkill. I implemented it ;-)
>
> I did also say:
>
> """
> An alternative here is to just allow direct access to the
> createInstance() from the test too, without having to re-search
> all of the factories each time. But I felt this was less
> of a de-restriction of the existing interfaces.
> """
>
> > don't use the two standard models, because someone decided to implement
> > something more "clever" in the hardware. It would be useful to cover
> > those in the tests I think.
>
> I absolutely agree. that's why I wanted to iterate /all/ helpers and
> test each one explicitly in https://patchwork.libcamera.org/patch/19530/
Amazing, the work is already done :-)
> > > +
> > > +class CameraSensorHelperTest : public Test
> >
> > As this tests the gain models only, I'd name the class
> > CameraSensorGainsTest or something similar. Same for the source file.
> >
> > > +{
> > > +protected:
> > > + int testGainModel(CameraSensorHelper &helper)
> > > + {
> > > + int ret = TestPass;
> > > +
> > > + /*
> > > + * Arbitrarily test 255 code positions assuming 8 bit fields
> > > + * are the normal maximum for a gain code register.
> > > + */
> >
> > This bothers me, mostly because the right solution would require
> > extending the helpers to expose the min/max gains, and *that* is
> > bothersome :-S So far we rely on kernel drivers to provide the
> > information. Do you think we should bite the bullet ?
>
> In fact, I questioned this too in https://patchwork.libcamera.org/patch/19530/
>
> """
> Because that's what value the IMX283 uses. Yes - as I said above, and
> why this is an RFC, we don't have a way to get the max value from here.
> But getting it is not so clear. Should we define the max code for every
> camera sensor helper? Should it be defaulted to something supplied from
> the driver? (How?) or are we then just duplicating and hardcoding values
> from v4l2 drivers here in the helpers... maybe that's fine ?
> """
So... Biting the bullet ? We already hardcode the gain model in
libcamera, because sensor drivers don't provide any information about
it. We could consider that the min/max values are part of the model. The
drawback is that we'll have problems if the kernel, for any reason,
decides to implement a different range.
> > One issue to consider is that the gain code may not be a continuous
> > value. A sensor I'm working on splits the analog gain in coarse and fine
> > gains, stored in bits [6:4] and [3:0]. The coarse gain is an exponential
> > gain, expressed as the exponent of a power of two, ranging from 2^0 to
> > 2^4. The fine gain is an inverse linear gain following the formula
> >
> > 1 / (1 - fine / 32)
> >
> > The fine gain value is quantized differently depending on the coarse
> > gain. When the coarse gain is 2^0 or 2^2, the fine gain code can take
> > any value in the [0, 15] range. When the coarse gain is 2^1 or 2^3, the
> > fine gain code is restricted to even values. When the coarse gain is
> > 2^4, the fine gain code is restricted to multiples of 4.
>
> We do have sensors which encode both analog and digital gain into a
> single control too.
>
> However for those so far, we artificially limit the gain to be only the
> analog component in the kernel drivers.
>
> See:
> https://lore.kernel.org/all/20240414140621.167717-7-umang.jain@ideasonboard.com/
The sensor may store the digital and analog gain values in the same
register, but they're still two separate gains, the kernel driver needs
to split the value in two V4L2 controls. I'm not concerned here.
> > I believe the hardware will operate correctly when those constraints are
> > not met, but will ignore the LSB or 2 LSBs of the fine gain code
> > depending on the coarse gain. This leads to, for instance,
> >
> > coarse = 2^1, fine = 4 -> gain = 2.2857142857142856
> > coarse = 2^1, fine = 5 -> gain = 2.2857142857142856
> >
> > The code -> gain -> code conversion can lead to the same result for all
> > valid code values, but not for all code values in the [min, max] range.
> > I wonder how to handle that in the test.
>
> Yikes....
And I haven't even mentioned that there's another fixed gain multiplier
added when the coarse gain is > 2^1 :-)
> > > + for (unsigned int i = 0; i < 255; i++) {
> > > + float gain = helper.gain(i);
> > > + uint32_t gainCode = helper.gainCode(gain);
> > > +
> > > + if (i != gainCode) {
> > > + std::cout << "Gain conversions failed: "
> > > + << i << " : " << gain << " : "
> > > + << gainCode << std::endl;
> > > +
> > > + ret = TestFail;
> > > + }
> > > + };
> > > +
> > > + return ret;
> > > + }
> > > +
> > > + int run() override
> > > + {
> > > + unsigned int failures = 0;
> > > +
> > > + CameraSensorHelperExponential exponential;
> > > + CameraSensorHelperLinear linear;
> > > +
> > > + if (testGainModel(exponential) == TestFail)
> > > + failures++;
> > > +
> > > + if (testGainModel(linear) == TestFail)
> > > + failures++;
> > > +
> > > + return failures ? TestFail : TestPass;
> > > + }
> > > +};
> > > +
> > > +TEST_REGISTER(CameraSensorHelperTest)
> > > diff --git a/test/ipa/meson.build b/test/ipa/meson.build
> > > index 180b0da0a51a..abc2456fc341 100644
> > > --- a/test/ipa/meson.build
> > > +++ b/test/ipa/meson.build
> > > @@ -1,6 +1,8 @@
> > > # SPDX-License-Identifier: CC0-1.0
> > >
> > > ipa_test = [
> > > + {'name': 'camera_sensor_helper', 'sources': ['camera_sensor_helper.cpp'],
> > > + 'should_fail': true},
> >
> > If you make it multi-line, let's make it so properly :-)
> >
> > {
> > 'name': 'camera_sensor_helper',
> > 'sources': ['camera_sensor_helper.cpp'],
> > 'should_fail': true,
> > },
>
> Well - I would (and did originally) except the very next patch removes
> it again, so I believed this was better as it was less visible churn.
Good point. I'm fine with that.
> > > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},
> > > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},
> > > ]
> > > @@ -11,5 +13,6 @@ foreach test : ipa_test
> > > link_with : [libipa, test_libraries],
> > > include_directories : [libipa_includes, test_includes_internal])
> > >
> > > - test(test['name'], exe, suite : 'ipa')
> > > + test(test['name'], exe, suite : 'ipa',
> > > + should_fail : test.get('should_fail', false))
> > > endforeach
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list