| Message ID | 20240703104004.184783-4-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
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 > > 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@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. > > > + 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. Best regards, Stefan > > > > > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS; > > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS; > > -- > > 2.43.0 > >
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 > > > 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@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. > > > > > > + 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@ideasonboard.com> > > Best regards, > Stefan > > > > > > > > > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS; > > > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS; > > > -- > > > 2.43.0 > > >
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@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@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS; > > > > params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;
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>(); + 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; params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS; params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;
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@ideasonboard.com> --- src/ipa/rkisp1/algorithms/blc.cpp | 54 ++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 8 deletions(-)