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

[Laurent Pinchart]

On Wed, Jul 03, 2024 at 01:32:04PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-07-03 13:28:17)
> > On Wed, Jul 03, 2024 at 01:11:39PM +0100, Kieran Bingham wrote:
> > > 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

s/use them/use it/ or s/helper now has/helpers now have/

> > > > file. But the direction is to remove them from the tuning files and move

s/file. But the/file. The/ or s/file. But/file, but/

> > > > 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?
> > 
> > Yes, that is actually the reason. I was undecided here. The metadata is
> > int32_t, the tuning files used to read int16_t, the rkisp1 allows
> > negative black levels for whatever reason... So I thought int16_t is the
> > bitwidth we always mention as reference and the maximum value beeing 50%
> > of the uint16_t range should be more than sufficient for the black
> > level.

I think it's sufficient indeed :-) We could use uint16_t if desired
though.

> > > > +       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?
> > 
> > Oh damn you are right. I'm just in the final real test, so that would
> > have been caught. But yes, that is wrong.
> > 
> > Thanks for catching it.
> 
> No worries - with that corrected:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

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

-- 
Regards,

Laurent Pinchart