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

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

Commit Message

Paul Elder May 7, 2024, 6:10 a.m. UTC 
Add an algorithm module to the rkisp1 IPA for crosstalk correction.

Signed-off-by: Paul Elder <paul.elder@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

Comments

Stefan Klug May 15, 2024, 9:42 a.m. UTC | #1 
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@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?

> +		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.

> +
> +	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 :-)

Regards,
Stefan

> +
> +	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
>
Paul Elder May 17, 2024, 6:09 a.m. UTC | #2 
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@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.

> 
> > +
> > +	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
> >
Stefan Klug May 17, 2024, 7:59 a.m. UTC | #3 
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@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
> > >

Patch
diff mbox series 

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"]);
+	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 }) }
+		};
+		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;
+
+	LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix.toString();
+	LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets.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 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',