[PATCH v2 1/2] test: ipa: libipa: Add CameraSensorHelper Gain Model tests
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jun 5 11:38:37 CEST 2024
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 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 :-)
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
More information about the libcamera-devel
mailing list