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

[Laurent Pinchart]

On Tue, Jul 02, 2024 at 02:59:32PM +0200, Jacopo Mondi wrote:
> On Tue, Jul 02, 2024 at 03:51:02PM GMT, Laurent Pinchart wrote:
> > Hi Stefan
> >
> > On Tue, Jul 02, 2024 at 09:53:21AM +0200, Jacopo Mondi wrote:
> > > On Mon, Jul 01, 2024 at 04:38:24PM GMT, Stefan Klug wrote:
> > > > 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 something like
> >
> > ----
> > Add black level values corresponding to the data pedestal for three
> > initial sensors. 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 | 18 ++++++++++++++++++
> > > >  src/ipa/libipa/camera_sensor_helper.h   |  6 ++++++
> > > >  2 files changed, 24 insertions(+)
> > > >
> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > index 782ff9904e81..6d8850eb25a5 100644
> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > @@ -47,6 +47,21 @@ namespace ipa {
> > > >   * function.
> > > >   */
> > > >
> > > > +/**
> > > > + * \brief Fetch the black levels of the sensor
> > > > + *
> > > > + * This function returns the black levels of the sensor with respect to a 16bit
> > > > + * pixel width in R, Gr, Gb, B order. If these are unknown an empty optional is
> > >
> > > s/with respect to a 16bit pixel width/expressed as four 16-bits pixel values/
> > >
> > > > + * returned.
> > > > + *
> > > > + * \return The black levels of the sensor
> > >
> > > , or std::nullopt if not initialized
> >
> > s/initialized/known/
> >
> > :-)
> >
> > > > + */
> > > > +std::optional<CameraSensorHelper::BlackLevels>
> > > > +CameraSensorHelper::blackLevels() const
> > > > +{
> > > > +	return blackLevels_;
> > > > +}
> > > > +
> > > >  /**
> > > >   * \brief Compute gain code from the analogue gain absolute value
> > > >   * \param[in] gain The real gain to pass
> > > > @@ -396,6 +411,7 @@ class CameraSensorHelperImx219 : public CameraSensorHelper
> > > >  public:
> > > >  	CameraSensorHelperImx219()
> > > >  	{
> > > > +		blackLevels_ = { 4096, 4096, 4096, 4096 };
> > > >  		gainType_ = AnalogueGainLinear;
> > > >  		gainConstants_.linear = { 0, 256, -1, 256 };
> > > >  	}
> > > > @@ -407,6 +423,7 @@ class CameraSensorHelperImx258 : public CameraSensorHelper
> > > >  public:
> > > >  	CameraSensorHelperImx258()
> > > >  	{
> > > > +		blackLevels_ = { 4096, 4096, 4096, 4096 };
> > > >  		gainType_ = AnalogueGainLinear;
> > > >  		gainConstants_.linear = { 0, 512, -1, 512 };
> > > >  	}
> > > > @@ -456,6 +473,7 @@ class CameraSensorHelperImx335 : public CameraSensorHelper
> > > >  public:
> > > >  	CameraSensorHelperImx335()
> > > >  	{
> > > > +		blackLevels_ = { 3200, 3200, 3200, 3200 };
> > >
> > > I trust you on these values, which I presume come from the sensor's
> > > datasheets ? If you have measured them instead, I would add them in a
> > > separate commit with the commit message reporting how these have been
> > > measured
> > >
> > > >  		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..f025727c06ee 100644
> > > > --- a/src/ipa/libipa/camera_sensor_helper.h
> > > > +++ b/src/ipa/libipa/camera_sensor_helper.h
> > > > @@ -9,7 +9,9 @@
> > > >
> > > >  #include <stdint.h>
> > > >
> > > > +#include <array>
> > > >  #include <memory>
> > > > +#include <optional>
> > > >  #include <string>
> > > >  #include <vector>
> > > >
> > > > @@ -22,9 +24,12 @@ namespace ipa {
> > > >  class CameraSensorHelper
> > > >  {
> > > >  public:
> > > > +	typedef std::array<int32_t, 4> BlackLevels;
> >
> > 	using BlackLevels = std::array<int32_t, 4>;
> >
> > >
> > > stupid question: are we sure we will always have 4 values only ? We
> > > tried not to assume a Bayer pattern in the design of the camera sensor
> > > helper classes.
> >
> > I would also have used 4 levels, but that's an interesting point. I
> > would be fine starting with 4, considering that sensors with less than 4
> > colour channels (the main case today is monochrome sensors) would use
> > the first elements, and figure out later what to do for sensors with
> > more than 4 channels. Or, given that we only deal with the pedestal
> > today, we could replace this with a single black level value. I'm
> > slightly tempted to go for the latter, as it's simpler.
> 
> Actually, from a sensor perspective, do we actually have one pedestal
> value for each color channel, or is the ISP that requires to express one
> value per channel ?

I suppose it's sensor-dependent, but in practice I don't really see a
reason why one would need per-channel values. There are certainly
sensors that program the data pedestal using a single register.

> > > > +
> > > >  	CameraSensorHelper() = default;
> > > >  	virtual ~CameraSensorHelper() = default;
> > > >
> > > > +	virtual std::optional<BlackLevels> blackLevels() const;
> > >
> > > Should this be virtual ? Is there any case when a derived class will
> > > have to override this instead of just initializing BlackLevels to the
> > > desired values ?
> >
> > Possibly, for instance the dark currents could be estimated based on the
> > sensor temperature. There's a high chance we'll never get there though,
> > so I'd start with a non-virtual function, and I would even make it
> > inline.
> >
> > > >  	virtual uint32_t gainCode(double gain) const;
> > > >  	virtual double gain(uint32_t gainCode) const;
> > > >
> > > > @@ -51,6 +56,7 @@ protected:
> > > >  		AnalogueGainExpConstants exp;
> > > >  	};
> > > >
> > > > +	std::optional<BlackLevels> blackLevels_;
> > > >  	AnalogueGainType gainType_;
> > > >  	AnalogueGainConstants gainConstants_;
> > > >

-- 
Regards,

Laurent Pinchart