[PATCH v3 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm

[Stefan Klug]

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?

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