[PATCH v2 1/6] ipa: libipa: Add black levels to camera sensor helper

[Kieran Bingham]

Quoting Kieran Bingham (2024-07-03 13:05:13)
> Quoting Stefan Klug (2024-07-03 11:39:48)
> > For a proper tuning process we need to know the sensor black levels. In
> > most cases these are fixed and not reported by the kernel driver. Store
> > them inside the sensor helpers for later retrieval by the algorithms.
> > 
> > Add black level value corresponding to the data pedestal for three
> > initial sensors as documented in the datasheets. More should be added,
> > eventually filling the gaps for all supported sensors.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  src/ipa/libipa/camera_sensor_helper.cpp | 27 +++++++++++++++++++++++++
> >  src/ipa/libipa/camera_sensor_helper.h   |  3 +++
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > index 782ff9904e81..3d0e756927f0 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -47,6 +47,30 @@ namespace ipa {
> >   * function.
> >   */
> >  
> > +/**
> > + * \fn CameraSensorHelper::blackLevel()
> > + * \brief Fetch the black level of the sensor
> > + *
> > + * This function returns the black level of the sensor scaled to a 16bit pixel
> > + * width. If it is unknown an empty optional is returned.
> > + *
> > + * Black levels are typically the result of the following phenomena:
> > + * - Pedestal added by the sensor to pixel values. They are typically fixed,
> > + *   sometimes programmable and should be reported in datasheets (but
> > + *   documentation is not always available).
> > + * - Dark currents and other physical effects that add charge to pixels in the
> > + *   absence of light. Those can depend on the integration time and the sensor
> > + *   die temperature, and their contribution to pixel values depend on the
> > + *   sensor gains.
> > + *
> > + * The pedestal is usually the value with the biggest contribution to the
> > + * overall black level. In most cases it is either known before or in rare cases
> > + * (There is not a single driver with such a control in the linux kernel) can be
> > + * queried from the sensor. This function shall provide that fixed, known value.
> > + *
> > + * \return The black level of the sensor, or std::nullopt if not known
> > + */
> > +
> >  /**
> >   * \brief Compute gain code from the analogue gain absolute value
> >   * \param[in] gain The real gain to pass
> > @@ -396,6 +420,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper
> >  public:
> >         CameraSensorHelperImx219()
> >         {
> > +               blackLevel_ = 4096;
> >                 gainType_ = AnalogueGainLinear;
> >                 gainConstants_.linear = { 0, 256, -1, 256 };
> >         }
> > @@ -407,6 +432,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper
> >  public:
> >         CameraSensorHelperImx258()
> >         {
> > +               blackLevel_ = 4096;
> >                 gainType_ = AnalogueGainLinear;
> >                 gainConstants_.linear = { 0, 512, -1, 512 };
> >         }
> > @@ -456,6 +482,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper
> >  public:
> >         CameraSensorHelperImx335()
> >         {
> > +               blackLevel_ = 3200;
> >                 gainType_ = AnalogueGainExponential;
> >                 gainConstants_.exp = { 1.0, expGainDb(0.3) };
> >         }
> > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h
> > index 0d99073bea82..ac276e27f523 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.h
> > +++ b/src/ipa/libipa/camera_sensor_helper.h
> > @@ -10,6 +10,7 @@
> >  #include <stdint.h>
> >  
> >  #include <memory>
> > +#include <optional>
> >  #include <string>
> >  #include <vector>
> >  
> > @@ -25,6 +26,7 @@ public:
> >         CameraSensorHelper() = default;
> >         virtual ~CameraSensorHelper() = default;
> >  
> > +       std::optional<int16_t> blackLevel() const { return blackLevel_; }
> >         virtual uint32_t gainCode(double gain) const;
> >         virtual double gain(uint32_t gainCode) const;
> >  
> > @@ -51,6 +53,7 @@ protected:
> >                 AnalogueGainExpConstants exp;
> >         };
> >  
> > +       std::optional<int16_t> blackLevel_;
> 
> Shouldn't the black level be a uint16_t ? Otherwise, this would then be
> limited to 15 bit max ? Can we expect a blacklevel above 32767?
> 

Given it's a known stipulation that we're using int16_t to read from the
yaml files currently:


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
> 
> >         AnalogueGainType gainType_;
> >         AnalogueGainConstants gainConstants_;
> >  
> > -- 
> > 2.43.0
> >