[v2,1/2] test: ipa: libipa: Add CameraSensorHelper Gain Model tests

20098 diff mbox series

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.

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@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

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.

> 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@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
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
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.

> +
> +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 ?

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.

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.

> +		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,
    },

>      {'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

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@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

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@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

Quoting Laurent Pinchart (2024-06-03 18:10:24)
> 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@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 :-)

It's fine ... now we know ;-)

> > - 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 :-)

Great when that happens hey ;D

> > > > +
> > > > +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.

Yes, but I think we'll roughly know if that happens as we have a good
eye on the kernel.

I think defining the range of code values as you suggested in another
patch lets us make good progress here.


> > > 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.

Ok, that can be handled kernel side then (if/when anyone /needs/ that much
gain....)

> > > 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 :-)

Err .. Yeah - I'll leave all that to you. The graph on that gain model
already looks crazy enough ;-)

--
Kieran