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

[Jacopo Mondi]

Hi Stefan

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

> + */
> +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;

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.

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

Thanks
  j

>  	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_;
>
> --
> 2.43.0
>