[RFC/PATCH] libcamera: libipa: camera_sensor: Add onsemi AR0144 sensor properties

20197 diff mbox series

Provide the onsemi AR0144 camera sensor properties and registration with
libipa for the gain code helpers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
I'm working on a driver for the AR0144 camera sensor, which implements
an exotic gain model. While I would normally post the kernel driver
before submitting the libcamera integration patch, I thought reversing
the order would be useful for discussion purpose, as we recently
resurected the topic of gain rounding (see
https://patchwork.libcamera.org/cover/20097/).

The approach I've taken here is to ensure that the code -> gain -> code
roundtrip conversion always results to the same gain code, for all valid
gain codes. To achieve this, I had to increase the multiple m in the
gain() function by the machine epsilon. In theory increasing other
factors and terms in the formula may have been needed, but this proved
enough in practice by running the roundtrip conversion on all possible
code values.

One challenge to test this in unit tests is that the valid gain code
ranges is not contiguous. The sensor rounds the fine gain differently
depending on the coarse gain, which creates holes in the range. It would
be feasible (and is considered) to extend the helpers to expose the
[min, max] range of valid codes, but exposing arbitrary holes is more
problematic. One option would be to specify that the gain() function
should return NaN when the gain code is invalid. To make this safe, we
would need to prove that the gainCode() function will never an invalid
gain code, ideally formally but possibly by brute force.
---
 src/ipa/libipa/camera_sensor_helper.cpp       | 87 +++++++++++++++++++
 .../sensor/camera_sensor_properties.cpp       |  9 ++
 2 files changed, 96 insertions(+)


base-commit: 6cd17515ffeb67fb38ffcc4d57aadf9732b54800

Quoting Laurent Pinchart (2024-06-04 00:03:00)
> Provide the onsemi AR0144 camera sensor properties and registration with
> libipa for the gain code helpers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> I'm working on a driver for the AR0144 camera sensor, which implements
> an exotic gain model. While I would normally post the kernel driver
> before submitting the libcamera integration patch, I thought reversing
> the order would be useful for discussion purpose, as we recently
> resurected the topic of gain rounding (see
> https://patchwork.libcamera.org/cover/20097/).
> 
> The approach I've taken here is to ensure that the code -> gain -> code
> roundtrip conversion always results to the same gain code, for all valid
> gain codes. To achieve this, I had to increase the multiple m in the
> gain() function by the machine epsilon. In theory increasing other
> factors and terms in the formula may have been needed, but this proved
> enough in practice by running the roundtrip conversion on all possible
> code values.
> 
> One challenge to test this in unit tests is that the valid gain code
> ranges is not contiguous. The sensor rounds the fine gain differently
> depending on the coarse gain, which creates holes in the range. It would
> be feasible (and is considered) to extend the helpers to expose the
> [min, max] range of valid codes, but exposing arbitrary holes is more
> problematic. One option would be to specify that the gain() function
> should return NaN when the gain code is invalid. To make this safe, we
> would need to prove that the gainCode() function will never an invalid
> gain code, ideally formally but possibly by brute force.

I like this proposal. I'm very much in favour of us trying to validate
each helper independently. And likely brute force (as it's relatively
cheap in reality). Otherwise we won't be able to test 'all helpers'.

I think adding a min()/max() accessor to the helpers is reasonable to
achieve this even if it means hardcoding additional properties of a
sensor in the helpers.

--
Kieran


> ---
>  src/ipa/libipa/camera_sensor_helper.cpp       | 87 +++++++++++++++++++
>  .../sensor/camera_sensor_properties.cpp       |  9 ++
>  2 files changed, 96 insertions(+)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index 782ff9904e81..c2be011300b1 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -8,6 +8,7 @@
>  #include "camera_sensor_helper.h"
>  
>  #include <cmath>
> +#include <limits>
>  
>  #include <libcamera/base/log.h>
>  
> @@ -366,6 +367,92 @@ static constexpr double expGainDb(double step)
>         return log2_10 * step / 20;
>  }
>  
> +class CameraSensorHelperAr0144 : public CameraSensorHelper
> +{
> +public:
> +       uint32_t gainCode(double gain) const override
> +       {
> +               /* The recommended minimum gain is 1.6842 to avoid artifacts. */
> +               gain = std::clamp(gain, 1.0 / (1.0 - 13.0 / 32.0), 18.45);
> +
> +               /*
> +                * The analogue gain is made of a coarse exponential gain in
> +                * the range [2^0, 2^4] and a fine inversely linear gain in the
> +                * range [1.0, 2.0[. There is an additional fixed 1.153125
> +                * multiplier when the coarse gain reaches 2^2.
> +                */
> +
> +               if (gain > 4.0)
> +                       gain /= 1.153125;
> +
> +               unsigned int coarse = std::log2(gain);
> +               unsigned int fine = (1 - (1 << coarse) / gain) * 32;
> +
> +               /* The fine gain rounding depends on the coarse gain. */
> +               if (coarse == 1 || coarse == 3)
> +                       fine &= ~1;
> +               else if (coarse == 4)
> +                       fine &= ~3;
> +
> +               return (coarse << 4) | (fine & 0xf);
> +       }
> +
> +       double gain(uint32_t gainCode) const override
> +       {
> +               unsigned int coarse = gainCode >> 4;
> +               unsigned int fine = gainCode & 0xf;
> +               unsigned int d1;
> +               double d2, m;
> +
> +               switch (coarse) {
> +               case 0:
> +                       d1 = 1;
> +                       d2 = 32.0;
> +                       m = 1.0;
> +                       break;
> +               case 1:
> +                       d1 = 2;
> +                       d2 = 16.0;
> +                       m = 1.0;
> +                       break;
> +               case 2:
> +                       d1 = 1;
> +                       d2 = 32.0;
> +                       m = 1.153125;
> +                       break;
> +               case 3:
> +                       d1 = 2;
> +                       d2 = 16.0;
> +                       m = 1.153125;
> +                       break;
> +               case 4:
> +                       d1 = 4;
> +                       d2 = 8.0;
> +                       m = 1.153125;
> +                       break;
> +               }
> +
> +               /*
> +                * With infinite precision, the calculated gain would be exact,
> +                * and the reverse conversion with gainCode() would produce the
> +                * same gain code. In the real world, rounding errors may cause
> +                * the calculated gain to be lower by an amount negligible for
> +                * all purposes, except for the reverse conversion. Converting
> +                * the gain to a gain code could then return the quantized value
> +                * just lower than the original gain code. To avoid this, tests
> +                * showed that adding the machine epsilon to the multiplier m is
> +                * sufficient.
> +                */
> +               m += std::numeric_limits<decltype(m)>::epsilon();
> +
> +               return m * (1 << coarse) / (1.0 - (fine / d1) / d2);
> +       }
> +
> +private:
> +       static constexpr double kStep_ = 16;
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("ar0144", CameraSensorHelperAr0144)
> +
>  class CameraSensorHelperAr0521 : public CameraSensorHelper
>  {
>  public:
> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> index b18524d85b37..4e5217ab51ef 100644
> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> @@ -52,6 +52,15 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)
>  const CameraSensorProperties *CameraSensorProperties::get(const std::string &sensor)
>  {
>         static const std::map<std::string, const CameraSensorProperties> sensorProps = {
> +               { "ar0144", {
> +                       .unitCellSize = { 3000, 3000 },
> +                       .testPatternModes = {
> +                               { controls::draft::TestPatternModeOff, 0 },
> +                               { controls::draft::TestPatternModeSolidColor, 1 },
> +                               { controls::draft::TestPatternModeColorBars, 2 },
> +                               { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> +                       },
> +               } },
>                 { "ar0521", {
>                         .unitCellSize = { 2200, 2200 },
>                         .testPatternModes = {
> 
> base-commit: 6cd17515ffeb67fb38ffcc4d57aadf9732b54800
> -- 
> Regards,
> 
> Laurent Pinchart
>

On Tue, Jun 04, 2024 at 10:51:04AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-04 00:03:00)
> > Provide the onsemi AR0144 camera sensor properties and registration with
> > libipa for the gain code helpers.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > I'm working on a driver for the AR0144 camera sensor, which implements
> > an exotic gain model. While I would normally post the kernel driver
> > before submitting the libcamera integration patch, I thought reversing
> > the order would be useful for discussion purpose, as we recently
> > resurected the topic of gain rounding (see
> > https://patchwork.libcamera.org/cover/20097/).
> > 
> > The approach I've taken here is to ensure that the code -> gain -> code
> > roundtrip conversion always results to the same gain code, for all valid
> > gain codes. To achieve this, I had to increase the multiple m in the
> > gain() function by the machine epsilon. In theory increasing other
> > factors and terms in the formula may have been needed, but this proved
> > enough in practice by running the roundtrip conversion on all possible
> > code values.
> > 
> > One challenge to test this in unit tests is that the valid gain code
> > ranges is not contiguous. The sensor rounds the fine gain differently
> > depending on the coarse gain, which creates holes in the range. It would
> > be feasible (and is considered) to extend the helpers to expose the
> > [min, max] range of valid codes, but exposing arbitrary holes is more
> > problematic. One option would be to specify that the gain() function
> > should return NaN when the gain code is invalid. To make this safe, we
> > would need to prove that the gainCode() function will never an invalid
> > gain code, ideally formally but possibly by brute force.
> 
> I like this proposal. I'm very much in favour of us trying to validate
> each helper independently. And likely brute force (as it's relatively
> cheap in reality). Otherwise we won't be able to test 'all helpers'.

We can't brute-force the proof for the generic helpers unfortunately.
All we can brute-force is each subclass. That's fine as a first step,
but formally proving the generic helpers would be nicer.

> I think adding a min()/max() accessor to the helpers is reasonable to
> achieve this even if it means hardcoding additional properties of a
> sensor in the helpers.

Or a range() function that returns the min and max, as callers will
likely need them both. Sounds good to me.

> > ---
> >  src/ipa/libipa/camera_sensor_helper.cpp       | 87 +++++++++++++++++++
> >  .../sensor/camera_sensor_properties.cpp       |  9 ++
> >  2 files changed, 96 insertions(+)
> > 
> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > index 782ff9904e81..c2be011300b1 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -8,6 +8,7 @@
> >  #include "camera_sensor_helper.h"
> >  
> >  #include <cmath>
> > +#include <limits>
> >  
> >  #include <libcamera/base/log.h>
> >  
> > @@ -366,6 +367,92 @@ static constexpr double expGainDb(double step)
> >         return log2_10 * step / 20;
> >  }
> >  
> > +class CameraSensorHelperAr0144 : public CameraSensorHelper
> > +{
> > +public:
> > +       uint32_t gainCode(double gain) const override
> > +       {
> > +               /* The recommended minimum gain is 1.6842 to avoid artifacts. */
> > +               gain = std::clamp(gain, 1.0 / (1.0 - 13.0 / 32.0), 18.45);
> > +
> > +               /*
> > +                * The analogue gain is made of a coarse exponential gain in
> > +                * the range [2^0, 2^4] and a fine inversely linear gain in the
> > +                * range [1.0, 2.0[. There is an additional fixed 1.153125
> > +                * multiplier when the coarse gain reaches 2^2.
> > +                */
> > +
> > +               if (gain > 4.0)
> > +                       gain /= 1.153125;
> > +
> > +               unsigned int coarse = std::log2(gain);
> > +               unsigned int fine = (1 - (1 << coarse) / gain) * 32;
> > +
> > +               /* The fine gain rounding depends on the coarse gain. */
> > +               if (coarse == 1 || coarse == 3)
> > +                       fine &= ~1;
> > +               else if (coarse == 4)
> > +                       fine &= ~3;
> > +
> > +               return (coarse << 4) | (fine & 0xf);
> > +       }
> > +
> > +       double gain(uint32_t gainCode) const override
> > +       {
> > +               unsigned int coarse = gainCode >> 4;
> > +               unsigned int fine = gainCode & 0xf;
> > +               unsigned int d1;
> > +               double d2, m;
> > +
> > +               switch (coarse) {
> > +               case 0:
> > +                       d1 = 1;
> > +                       d2 = 32.0;
> > +                       m = 1.0;
> > +                       break;
> > +               case 1:
> > +                       d1 = 2;
> > +                       d2 = 16.0;
> > +                       m = 1.0;
> > +                       break;
> > +               case 2:
> > +                       d1 = 1;
> > +                       d2 = 32.0;
> > +                       m = 1.153125;
> > +                       break;
> > +               case 3:
> > +                       d1 = 2;
> > +                       d2 = 16.0;
> > +                       m = 1.153125;
> > +                       break;
> > +               case 4:
> > +                       d1 = 4;
> > +                       d2 = 8.0;
> > +                       m = 1.153125;
> > +                       break;
> > +               }
> > +
> > +               /*
> > +                * With infinite precision, the calculated gain would be exact,
> > +                * and the reverse conversion with gainCode() would produce the
> > +                * same gain code. In the real world, rounding errors may cause
> > +                * the calculated gain to be lower by an amount negligible for
> > +                * all purposes, except for the reverse conversion. Converting
> > +                * the gain to a gain code could then return the quantized value
> > +                * just lower than the original gain code. To avoid this, tests
> > +                * showed that adding the machine epsilon to the multiplier m is
> > +                * sufficient.
> > +                */
> > +               m += std::numeric_limits<decltype(m)>::epsilon();
> > +
> > +               return m * (1 << coarse) / (1.0 - (fine / d1) / d2);
> > +       }
> > +
> > +private:
> > +       static constexpr double kStep_ = 16;
> > +};
> > +REGISTER_CAMERA_SENSOR_HELPER("ar0144", CameraSensorHelperAr0144)
> > +
> >  class CameraSensorHelperAr0521 : public CameraSensorHelper
> >  {
> >  public:
> > diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> > index b18524d85b37..4e5217ab51ef 100644
> > --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> > +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> > @@ -52,6 +52,15 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)
> >  const CameraSensorProperties *CameraSensorProperties::get(const std::string &sensor)
> >  {
> >         static const std::map<std::string, const CameraSensorProperties> sensorProps = {
> > +               { "ar0144", {
> > +                       .unitCellSize = { 3000, 3000 },
> > +                       .testPatternModes = {
> > +                               { controls::draft::TestPatternModeOff, 0 },
> > +                               { controls::draft::TestPatternModeSolidColor, 1 },
> > +                               { controls::draft::TestPatternModeColorBars, 2 },
> > +                               { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > +                       },
> > +               } },
> >                 { "ar0521", {
> >                         .unitCellSize = { 2200, 2200 },
> >                         .testPatternModes = {
> > 
> > base-commit: 6cd17515ffeb67fb38ffcc4d57aadf9732b54800