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

[Stefan Klug]

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