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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart