[PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain Model tests
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jun 3 18:56:36 CEST 2024
Quoting Laurent Pinchart (2024-06-03 17:32:08)
> Hi Kieran,
>
> Thank you for the patch.
>
> 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? ;-)
- 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/
> > +
> > +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 ?
"""
>
> 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/
> 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....
>
> > + 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.
>
> > {'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