[PATCH v3 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm
Stefan Klug
stefan.klug at ideasonboard.com
Fri May 17 09:59:11 CEST 2024
On Fri, May 17, 2024 at 03:09:06PM +0900, Paul Elder wrote:
> On Wed, May 15, 2024 at 11:42:18AM +0200, Stefan Klug wrote:
> > Hi Paul,
> >
> > thanks for the patch.
> >
> > On Tue, May 07, 2024 at 03:10:16PM +0900, Paul Elder wrote:
> > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > This patch depends on "[PATCH] libcamera: utils: Add a helper to convert
> > > floating-point to fixed-point"
> > >
> > > Changes in v3:
> > > - read ccm offsets from tuning data, and write these offsets to the
> > > parameters buffer
> > > - make parseYaml return void, as it should fill in default data if
> > > unable to read, thus never failing
> > >
> > > Changes in v2:
> > > - rename ctk to ccm
> > > - reset the matrix interpolator to identity matrix if failed to read
> > > from tuning file
> > > ---
> > > src/ipa/rkisp1/algorithms/ccm.cpp | 116 ++++++++++++++++++++++++++
> > > src/ipa/rkisp1/algorithms/ccm.h | 44 ++++++++++
> > > src/ipa/rkisp1/algorithms/meson.build | 1 +
> > > 3 files changed, 161 insertions(+)
> > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp
> > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > new file mode 100644
> > > index 00000000..11fa5ec2
> > > --- /dev/null
> > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > @@ -0,0 +1,116 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas On Board
> > > + *
> > > + * ccm.cpp - RkISP1 Cross Talk Correction control algorithm
> > > + */
> > > +
> > > +#include "ccm.h"
> > > +
> > > +#include <algorithm>
> > > +#include <chrono>
> > > +#include <cmath>
> > > +#include <tuple>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/base/log.h>
> > > +#include <libcamera/base/utils.h>
> > > +
> > > +#include <libcamera/ipa/core_ipa_interface.h>
> > > +
> > > +#include "libcamera/internal/yaml_parser.h"
> > > +
> > > +#include "libipa/matrix_interpolator.h"
> > > +
> > > +/**
> > > + * \file ctk.h
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa::rkisp1::algorithms {
> > > +
> > > +/**
> > > + * \class Ccm
> > > + * \brief A cross talk correction algorithm
> > > + */
> > > +
> > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > > +
> > > +void Ccm::parseYaml(const YamlObject &tuningData)
> > > +{
> > > + int ret = ccm_.readYaml(tuningData["ccms"]);
> > > + if (ret < 0) {
> > > + LOG(RkISP1Ccm, Warning)
> > > + << "Failed to parse 'ccms' "
> > > + << "parameter from tuning file; falling back to unit matrix";
> > > + ccm_.reset();
> > > + }
> > > +
> > > + ret = offsets_.readYaml(tuningData["offsets"]);
> >
> > Ahh I should have read that patch first. This explains how you solved
> > the "more data per temperature" question. We can also postpone that for
> > now and see what we really need.
> >
> > > + if (ret < 0) {
> > > + LOG(RkISP1Ccm, Warning)
> > > + << "Failed to parse 'offsets' "
> > > + << "parameter from tuning file; falling back to zero offsets";
> > > + const std::map<unsigned int, Matrix<int16_t, 3, 1>> offsets = {
> > > + { 0, Matrix<int16_t, 3, 1>({ 0, 0, 0 }) }
> > > + };
> >
> > Oh, beeing able to use the interpolator for the offsets is nice. Having
> > to do manual handling of the "identity" case doesn't feel too good. As
> > the identity matrix is only defined for square matrices what about
> > initializing the matrix to identity only if R==C in the constructor and
> > to zero otherwise? Or is that too much magic?
>
> Oh, good idea. We can do that.
>
> >
> > > + MatrixInterpolator<int16_t, 3, 1> interpolator(offsets);
> > > + offsets_ = interpolator;
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::init
> > > + */
> > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > > +{
> > > + parseYaml(tuningData);
> > > + return 0;
> > > +}
> > > +
> > > +void Ccm::setParameters(rkisp1_params_cfg *params,
> > > + const Matrix<double, 3, 3> &matrix,
> > > + const Matrix<int16_t, 3, 1> &offsets)
> > > +{
> > > + struct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;
> > > +
> > > + /*
> > > + * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to
> > > + * +7.992 (0x3FF)
> > > + */
> > > + for (unsigned int i = 0; i < 3; i++)
> > > + for (unsigned int j = 0; j < 3; j++)
> > > + config.coeff[i][j] =
> > > + utils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix.get(i, j));
> > > +
> > > + for (unsigned int i = 0; i < 3; i++)
> > > + config.ct_offset[i] = offsets.get(i, 0) & 0xFFF;
> >
> > If we go for floats in the matrix, we should handle the offsets the same
> > way.
>
> The offsets are integers between (and including) -2048 to 2047 so the
> floats don't really apply. The tuning tool with output these values
> directly.
>
Oh I missed that. I actually thought these where also fixed point. Sorry
for the noise.
Cheers,
Stefan
> >
> > > +
> > > + LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix.toString();
> > > + LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets.toString();
> >
> > Just having to write
> > LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> > would be beautiful :-)
>
> Indeed :)
>
>
> Thanks,
>
> Paul
>
> >
> > > +
> > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;
> > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;
> > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;
> > > +}
> > > +
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::prepare
> > > + */
> > > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > + [[maybe_unused]] IPAFrameContext &frameContext,
> > > + rkisp1_params_cfg *params)
> > > +{
> > > + uint32_t ct = context.activeState.awb.temperatureK;
> > > + Matrix<double, 3, 3> ccm = ccm_.get(ct);
> > > + Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> > > +
> > > + setParameters(params, ccm, offsets);
> > > +}
> > > +
> > > +REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
> > > +
> > > +} /* namespace ipa::rkisp1::algorithms */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h
> > > new file mode 100644
> > > index 00000000..dcab5cd3
> > > --- /dev/null
> > > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > > @@ -0,0 +1,44 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas On Board
> > > + *
> > > + * ccm.h - RkISP1 Cross Talk Correction control algorithm
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <linux/rkisp1-config.h>
> > > +
> > > +#include "libipa/matrix.h"
> > > +#include "libipa/matrix_interpolator.h"
> > > +
> > > +#include "algorithm.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa::rkisp1::algorithms {
> > > +
> > > +class Ccm : public Algorithm
> > > +{
> > > +public:
> > > + Ccm(){};
> > > + ~Ccm() = default;
> > > +
> > > + int init(IPAContext &context, const YamlObject &tuningData) override;
> > > + void prepare(IPAContext &context, const uint32_t frame,
> > > + IPAFrameContext &frameContext,
> > > + rkisp1_params_cfg *params) override;
> > > +
> > > +private:
> > > + void parseYaml(const YamlObject &tuningData);
> > > + void setParameters(rkisp1_params_cfg *params,
> > > + const Matrix<double, 3, 3> &matrix,
> > > + const Matrix<int16_t, 3, 1> &offsets);
> > > +
> > > + MatrixInterpolator<double, 3, 3> ccm_;
> > > + MatrixInterpolator<int16_t, 3, 1> offsets_;
> > > +};
> > > +
> > > +} /* namespace ipa::rkisp1::algorithms */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > > index 93a48329..16de7133 100644
> > > --- a/src/ipa/rkisp1/algorithms/meson.build
> > > +++ b/src/ipa/rkisp1/algorithms/meson.build
> > > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([
> > > 'agc.cpp',
> > > 'awb.cpp',
> > > 'blc.cpp',
> > > + 'ccm.cpp',
> > > 'cproc.cpp',
> > > 'dpcc.cpp',
> > > 'dpf.cpp',
> > > --
> > > 2.39.2
> > >
More information about the libcamera-devel
mailing list