[PATCH v2 3/6] ipa: rkisp1: blc: Query black levels from camera sensor helper

[Kieran Bingham]

Quoting Stefan Klug (2024-07-03 11:39:50)
> As the camera sensor helper now has the ability to provide the black
> level, use them. Black levels can still be overwritten by the tuning
> file. But the direction is to remove them from the tuning files and move
> them into the sensor helpers.
> 
> Additionally interpret all values based on 16bits. The conversion to the
> scale required by the hardware is done in process(). It ensures all the
> values inside libcamera are the same scale and is in preparation for the
> i.MX8MP where black levels are based on a 20bit scale. Note that this
> breaks existing tuning files. The tuning files distributed with
> libcamera will be fixed in a later patch.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/blc.cpp | 54 ++++++++++++++++++++++++++-----
>  1 file changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> index d2e743541c99..87025e4f8c72 100644
> --- a/src/ipa/rkisp1/algorithms/blc.cpp
> +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> @@ -46,10 +46,47 @@ BlackLevelCorrection::BlackLevelCorrection()
>  int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,
>                                const YamlObject &tuningData)
>  {
> -       blackLevelRed_ = tuningData["R"].get<int16_t>(256);
> -       blackLevelGreenR_ = tuningData["Gr"].get<int16_t>(256);
> -       blackLevelGreenB_ = tuningData["Gb"].get<int16_t>(256);
> -       blackLevelBlue_ = tuningData["B"].get<int16_t>(256);
> +       std::optional<int16_t> levelRed = tuningData["R"].get<int16_t>();
> +       std::optional<int16_t> levelGreenR = tuningData["Gr"].get<int16_t>();
> +       std::optional<int16_t> levelGreenB = tuningData["Gb"].get<int16_t>();
> +       std::optional<int16_t> levelBlue = tuningData["B"].get<int16_t>();

Is this why we're using int16_t instead of uint16_t ... can we
distinguise int vs uint in the yaml parsers?

> +       bool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;
> +
> +       auto blackLevel = context.camHelper->blackLevel();
> +       if (!blackLevel) {
> +               /*
> +                * Not all camera sensor helpers have been updated with black
> +                * levels. Print a warning and fall back to the levels from the
> +                * tuning data to preserve backward compatibility. This should
> +                * be removed once all helpers provide the data.
> +                */
> +               LOG(RkISP1Blc, Warning)
> +                       << "No black levels provided by camera sensor helper"
> +                       << ", please fix";
> +
> +               blackLevelRed_ = levelRed.value_or(4096);
> +               blackLevelGreenR_ = levelGreenR.value_or(4096);
> +               blackLevelGreenB_ = levelGreenB.value_or(4096);
> +               blackLevelBlue_ = levelBlue.value_or(4096);
> +       } else if (tuningHasLevels) {
> +               /*
> +                * If black levels are provided in the tuning file, use them to
> +                * avoid breaking existing camera tuning. This is deprecated and
> +                * will be removed.
> +                */
> +               LOG(RkISP1Blc, Warning)
> +                       << "Deprecated: black levels overwritten by tuning file";
> +
> +               blackLevelRed_ = *levelRed;
> +               blackLevelGreenR_ = *levelGreenR;
> +               blackLevelGreenB_ = *levelGreenB;
> +               blackLevelBlue_ = *levelBlue;
> +       } else {
> +               blackLevelRed_ = *blackLevel;
> +               blackLevelGreenR_ = *blackLevel;
> +               blackLevelGreenB_ = *blackLevel;
> +               blackLevelBlue_ = *blackLevel;
> +       }
>  
>         tuningParameters_ = true;
>  
> @@ -77,10 +114,11 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
>                 return;
>  
>         params->others.bls_config.enable_auto = 0;
> -       params->others.bls_config.fixed_val.r = blackLevelRed_;
> -       params->others.bls_config.fixed_val.gr = blackLevelGreenR_;
> -       params->others.bls_config.fixed_val.gb = blackLevelGreenB_;
> -       params->others.bls_config.fixed_val.b = blackLevelBlue_;
> +       /* The rkisp1 uses 12bit based black levels. Scale down accordingly. */
> +       params->others.bls_config.fixed_val.r = blackLevelRed_ << 4;
> +       params->others.bls_config.fixed_val.gr = blackLevelGreenR_ << 4;
> +       params->others.bls_config.fixed_val.gb = blackLevelGreenB_ << 4;
> +       params->others.bls_config.fixed_val.b = blackLevelBlue_ << 4;

Erm, Should those be >> 4 or did I miss something obvious?

>  
>         params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS;
>         params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;
> -- 
> 2.43.0
>