[3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm
diff mbox series

Message ID 20240405084050.1919105-4-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: rkisp1: Add crosstalk algorithm
Related show

Commit Message

Paul Elder April 5, 2024, 8:40 a.m. UTC 
Add an algorithm module to the rkisp1 IPA for crosstalk correction.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/ctk.cpp     | 98 +++++++++++++++++++++++++++
 src/ipa/rkisp1/algorithms/ctk.h       | 41 +++++++++++
 src/ipa/rkisp1/algorithms/meson.build |  1 +
 3 files changed, 140 insertions(+)
 create mode 100644 src/ipa/rkisp1/algorithms/ctk.cpp
 create mode 100644 src/ipa/rkisp1/algorithms/ctk.h

Comments

Stefan Klug April 10, 2024, 10:11 p.m. UTC | #1 
Hi Paul,

thank you for the patch.

On Fri, Apr 05, 2024 at 05:40:50PM +0900, Paul Elder wrote:
> Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/ctk.cpp     | 98 +++++++++++++++++++++++++++
>  src/ipa/rkisp1/algorithms/ctk.h       | 41 +++++++++++
>  src/ipa/rkisp1/algorithms/meson.build |  1 +
>  3 files changed, 140 insertions(+)
>  create mode 100644 src/ipa/rkisp1/algorithms/ctk.cpp
>  create mode 100644 src/ipa/rkisp1/algorithms/ctk.h
> 
> diff --git a/src/ipa/rkisp1/algorithms/ctk.cpp b/src/ipa/rkisp1/algorithms/ctk.cpp
> new file mode 100644
> index 00000000..76f5da93
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/ctk.cpp
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + *
> + * ctk.cpp - RkISP1 Cross Talk Correction control algorithm
> + */
> +
> +#include "ctk.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 Ctk
> + * \brief A cross talk correction algorithm
> + */

Maybe it's just my bubble, but I wouldn't look for ctk, but for
ColorCorrection or CC in the algorithms. Maybe we should agree on one
name libcamera wide (In rpi it's called ccm which feels natural to me
although technically one could argue that the algorithm is only cc).

> +
> +LOG_DEFINE_CATEGORY(RkISP1Ctk)
> +
> +int Ctk::parseYaml(const YamlObject &tuningData, const char *prop)
> +{
> +	int ret = 0;
> +
> +	const YamlObject &yaml = tuningData[prop];
> +	ret = ccm_.readYaml(yaml);
> +	if (ret < 0) {
> +		LOG(RkISP1Ctk, Error)
> +			<< "Failed to parse '" << prop << "' parameter from tuning file";
> +	}

In the error case I would expect it to fallback to a unit matrix.

> +
> +	return ret;
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::init
> + */
> +int Ctk::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> +{
> +	return parseYaml(tuningData, "ctms");

That's another name for the same thing. Why not ccms?

> +}
> +
> +void Ctk::setParameters(rkisp1_params_cfg *params, const Matrix<double> &matrix)
> +{
> +	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] = static_cast<uint16_t>(matrix.data[i * 3 + j]);

I think the conversion to fixed point should be done here and the yaml
should contain floats, to keep the hardware specifics out of the yaml.

> +
> +	LOG(RkISP1Ctk, Debug) << "Setting matrix " << matrix.toString();
> +
> +	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 Ctk::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 ccm = ccm_.get(ct);
> +
> +	setParameters(params, ccm);
> +}
> +
> +REGISTER_IPA_ALGORITHM(Ctk, "Ctk")
> +
> +} /* namespace ipa::rkisp1::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/rkisp1/algorithms/ctk.h b/src/ipa/rkisp1/algorithms/ctk.h
> new file mode 100644
> index 00000000..4f429df4
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/ctk.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + *
> + * ctk.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 Ctk : public Algorithm
> +{
> +public:
> +	Ctk(){};
> +	~Ctk() = 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:
> +	int parseYaml(const YamlObject &tuningData, const char *prop);
> +	void setParameters(rkisp1_params_cfg *params, const Matrix<double> &matrix);
> +
> +	MatrixInterpolator<double, 3, 3> ccm_;
> +};
> +
> +} /* namespace ipa::rkisp1::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> index 93a48329..c9891e87 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',
> +    'ctk.cpp',

This should be sorted.

Best regards,
Stefan

>      'cproc.cpp',
>      'dpcc.cpp',
>      'dpf.cpp',
> -- 
> 2.39.2
>
Kieran Bingham April 10, 2024, 10:33 p.m. UTC | #2 
Quoting Stefan Klug (2024-04-10 23:11:13)
> Hi Paul,
> 
> thank you for the patch.
> 
> On Fri, Apr 05, 2024 at 05:40:50PM +0900, Paul Elder wrote:
> > Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/ctk.cpp     | 98 +++++++++++++++++++++++++++
> >  src/ipa/rkisp1/algorithms/ctk.h       | 41 +++++++++++
> >  src/ipa/rkisp1/algorithms/meson.build |  1 +
> >  3 files changed, 140 insertions(+)
> >  create mode 100644 src/ipa/rkisp1/algorithms/ctk.cpp
> >  create mode 100644 src/ipa/rkisp1/algorithms/ctk.h
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/ctk.cpp b/src/ipa/rkisp1/algorithms/ctk.cpp
> > new file mode 100644
> > index 00000000..76f5da93
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/ctk.cpp
> > @@ -0,0 +1,98 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * ctk.cpp - RkISP1 Cross Talk Correction control algorithm
> > + */
> > +
> > +#include "ctk.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 Ctk
> > + * \brief A cross talk correction algorithm
> > + */
> 
> Maybe it's just my bubble, but I wouldn't look for ctk, but for
> ColorCorrection or CC in the algorithms. Maybe we should agree on one
> name libcamera wide (In rpi it's called ccm which feels natural to me
> although technically one could argue that the algorithm is only cc).
> 
> > +
> > +LOG_DEFINE_CATEGORY(RkISP1Ctk)
> > +
> > +int Ctk::parseYaml(const YamlObject &tuningData, const char *prop)
> > +{
> > +     int ret = 0;
> > +
> > +     const YamlObject &yaml = tuningData[prop];
> > +     ret = ccm_.readYaml(yaml);
> > +     if (ret < 0) {
> > +             LOG(RkISP1Ctk, Error)
> > +                     << "Failed to parse '" << prop << "' parameter from tuning file";
> > +     }
> 
> In the error case I would expect it to fallback to a unit matrix.

I think this was probably intended to be called the 'identity' matrix.
But I think I agree here, as we should expect without any better
information to just pass through the data in this component.

I don't think we need the braces around the LOG either...

> 
> > +
> > +     return ret;
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::init
> > + */
> > +int Ctk::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > +{
> > +     return parseYaml(tuningData, "ctms");
> 
> That's another name for the same thing. Why not ccms?

Except the isp structure is called rkisp1_cif_isp_ctk_config below which
would lead me to believe this should be reading in the 'ctks' ? But
they're not crosstalks ... they color correction matrices ... eeek.

Tough to name things, do we name it to match the component we're
populating (ctk), or the type of data we're filling it with (ccm)

> > +}
> > +
> > +void Ctk::setParameters(rkisp1_params_cfg *params, const Matrix<double> &matrix)
> > +{
> > +     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] = static_cast<uint16_t>(matrix.data[i * 3 + j]);
> 
> I think the conversion to fixed point should be done here and the yaml
> should contain floats, to keep the hardware specifics out of the yaml.

I think we've discussed this too and already agreed on this, so indeed I
agree here.

> > +     LOG(RkISP1Ctk, Debug) << "Setting matrix " << matrix.toString();
> > +
> > +     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 Ctk::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 ccm = ccm_.get(ct);

Wow. I thought this class wasn't doing anything, but this is actaully
/awesome/ and shows how much code is factored into the helpers.

I might bikeshed and wonder if this should be 'ccm_.interpolate(ct);' to
be clearer on what will happen, but I don't really care. It should be
clear that the ccms are stored in an interpolater ;-)

I wonder if we should cache the interpolation though so that we don't
recalculate/reinterpolate for every frame, and instead only
re-interpolate if the ct changes by more than ... X ?

--
Kieran



> > +
> > +     setParameters(params, ccm);
> > +}
> > +
> > +REGISTER_IPA_ALGORITHM(Ctk, "Ctk")
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/ctk.h b/src/ipa/rkisp1/algorithms/ctk.h
> > new file mode 100644
> > index 00000000..4f429df4
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/ctk.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * ctk.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 Ctk : public Algorithm
> > +{
> > +public:
> > +     Ctk(){};
> > +     ~Ctk() = 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:
> > +     int parseYaml(const YamlObject &tuningData, const char *prop);
> > +     void setParameters(rkisp1_params_cfg *params, const Matrix<double> &matrix);
> > +
> > +     MatrixInterpolator<double, 3, 3> ccm_;
> > +};
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > index 93a48329..c9891e87 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',
> > +    'ctk.cpp',
> 
> This should be sorted.
> 
> Best regards,
> Stefan
> 
> >      'cproc.cpp',
> >      'dpcc.cpp',
> >      'dpf.cpp',
> > -- 
> > 2.39.2
> >

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/ctk.cpp b/src/ipa/rkisp1/algorithms/ctk.cpp
new file mode 100644
index 00000000..76f5da93
--- /dev/null
+++ b/src/ipa/rkisp1/algorithms/ctk.cpp
@@ -0,0 +1,98 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2024, Ideas On Board
+ *
+ * ctk.cpp - RkISP1 Cross Talk Correction control algorithm
+ */
+
+#include "ctk.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 Ctk
+ * \brief A cross talk correction algorithm
+ */
+
+LOG_DEFINE_CATEGORY(RkISP1Ctk)
+
+int Ctk::parseYaml(const YamlObject &tuningData, const char *prop)
+{
+	int ret = 0;
+
+	const YamlObject &yaml = tuningData[prop];
+	ret = ccm_.readYaml(yaml);
+	if (ret < 0) {
+		LOG(RkISP1Ctk, Error)
+			<< "Failed to parse '" << prop << "' parameter from tuning file";
+	}
+
+	return ret;
+}
+
+/**
+ * \copydoc libcamera::ipa::Algorithm::init
+ */
+int Ctk::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
+{
+	return parseYaml(tuningData, "ctms");
+}
+
+void Ctk::setParameters(rkisp1_params_cfg *params, const Matrix<double> &matrix)
+{
+	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] = static_cast<uint16_t>(matrix.data[i * 3 + j]);
+
+	LOG(RkISP1Ctk, Debug) << "Setting matrix " << matrix.toString();
+
+	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 Ctk::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 ccm = ccm_.get(ct);
+
+	setParameters(params, ccm);
+}
+
+REGISTER_IPA_ALGORITHM(Ctk, "Ctk")
+
+} /* namespace ipa::rkisp1::algorithms */
+
+} /* namespace libcamera */
diff --git a/src/ipa/rkisp1/algorithms/ctk.h b/src/ipa/rkisp1/algorithms/ctk.h
new file mode 100644
index 00000000..4f429df4
--- /dev/null
+++ b/src/ipa/rkisp1/algorithms/ctk.h
@@ -0,0 +1,41 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2024, Ideas On Board
+ *
+ * ctk.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 Ctk : public Algorithm
+{
+public:
+	Ctk(){};
+	~Ctk() = 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:
+	int parseYaml(const YamlObject &tuningData, const char *prop);
+	void setParameters(rkisp1_params_cfg *params, const Matrix<double> &matrix);
+
+	MatrixInterpolator<double, 3, 3> ccm_;
+};
+
+} /* namespace ipa::rkisp1::algorithms */
+
+} /* namespace libcamera */
diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
index 93a48329..c9891e87 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',
+    'ctk.cpp',
     'cproc.cpp',
     'dpcc.cpp',
     'dpf.cpp',