[{"id":29543,"web_url":"https://patchwork.libcamera.org/comment/29543/","msgid":"<20240515094218.omugezhzrhqp734c@jasper>","date":"2024-05-15T09:42:18","subject":"Re: [PATCH v3 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nthanks for the patch.\n\nOn Tue, May 07, 2024 at 03:10:16PM +0900, Paul Elder wrote:\n> Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> This patch depends on \"[PATCH] libcamera: utils: Add a helper to convert\n> floating-point to fixed-point\"\n> \n> Changes in v3:\n> - read ccm offsets from tuning data, and write these offsets to the\n> parameters buffer\n> - make parseYaml return void, as it should fill in default data if\n> unable to read, thus never failing\n> \n> Changes in v2:\n> - rename ctk to ccm\n> - reset the matrix interpolator to identity matrix if failed to read\n> from tuning file\n> ---\n> src/ipa/rkisp1/algorithms/ccm.cpp | 116 ++++++++++++++++++++++++++\n> src/ipa/rkisp1/algorithms/ccm.h | 44 ++++++++++\n> src/ipa/rkisp1/algorithms/meson.build | 1 +\n> 3 files changed, 161 insertions(+)\n> create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> new file mode 100644\n> index 00000000..11fa5ec2\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> @@ -0,0 +1,116 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * ccm.cpp - RkISP1 Cross Talk Correction control algorithm\n> + */\n> +\n> +#include \"ccm.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"libipa/matrix_interpolator.h\"\n> +\n> +/**\n> + * \\file ctk.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class Ccm\n> + * \\brief A cross talk correction algorithm\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> +\n> +void Ccm::parseYaml(const YamlObject &tuningData)\n> +{\n> +\tint ret = ccm_.readYaml(tuningData[\"ccms\"]);\n> +\tif (ret < 0) {\n> +\t\tLOG(RkISP1Ccm, Warning)\n> +\t\t\t<< \"Failed to parse 'ccms' \"\n> +\t\t\t<< \"parameter from tuning file; falling back to unit matrix\";\n> +\t\tccm_.reset();\n> +\t}\n> +\n> +\tret = offsets_.readYaml(tuningData[\"offsets\"]);\n\nAhh I should have read that patch first. This explains how you solved\nthe \"more data per temperature\" question. We can also postpone that for\nnow and see what we really need.\n\n> +\tif (ret < 0) {\n> +\t\tLOG(RkISP1Ccm, Warning)\n> +\t\t\t<< \"Failed to parse 'offsets' \"\n> +\t\t\t<< \"parameter from tuning file; falling back to zero offsets\";\n> +\t\tconst std::map> offsets = {\n> +\t\t\t{ 0, Matrix({ 0, 0, 0 }) }\n> +\t\t};\n\nOh, beeing able to use the interpolator for the offsets is nice. Having\nto do manual handling of the \"identity\" case doesn't feel too good. As\nthe identity matrix is only defined for square matrices what about\ninitializing the matrix to identity only if R==C in the constructor and\nto zero otherwise? Or is that too much magic?\n\n> +\t\tMatrixInterpolator interpolator(offsets);\n> +\t\toffsets_ = interpolator;\n> +\t}\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::init\n> + */\n> +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> +{\n> +\tparseYaml(tuningData);\n> +\treturn 0;\n> +}\n> +\n> +void Ccm::setParameters(rkisp1_params_cfg *params,\n> +\t\t\tconst Matrix &matrix,\n> +\t\t\tconst Matrix &offsets)\n> +{\n> +\tstruct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> +\n> +\t/*\n> +\t * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> +\t * +7.992 (0x3FF)\n> +\t */\n> +\tfor (unsigned int i = 0; i < 3; i++)\n> +\t\tfor (unsigned int j = 0; j < 3; j++)\n> +\t\t\tconfig.coeff[i][j] =\n> +\t\t\t\tutils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix.get(i, j));\n> +\n> +\tfor (unsigned int i = 0; i < 3; i++)\n> +\t\tconfig.ct_offset[i] = offsets.get(i, 0) & 0xFFF;\n\nIf we go for floats in the matrix, we should handle the offsets the same\nway.\n\n> +\n> +\tLOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix.toString();\n> +\tLOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets.toString();\n\nJust having to write \nLOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\nwould be beautiful :-)\n\nRegards,\nStefan\n\n> +\n> +\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> +\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> +\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> +\t\t rkisp1_params_cfg *params)\n> +{\n> +\tuint32_t ct = context.activeState.awb.temperatureK;\n> +\tMatrix ccm = ccm_.get(ct);\n> +\tMatrix offsets = offsets_.get(ct);\n> +\n> +\tsetParameters(params, ccm, offsets);\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> new file mode 100644\n> index 00000000..dcab5cd3\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> @@ -0,0 +1,44 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * ccm.h - RkISP1 Cross Talk Correction control algorithm\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"libipa/matrix.h\"\n> +#include \"libipa/matrix_interpolator.h\"\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class Ccm : public Algorithm\n> +{\n> +public:\n> +\tCcm(){};\n> +\t~Ccm() = default;\n> +\n> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> +\tvoid prepare(IPAContext &context, const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t rkisp1_params_cfg *params) override;\n> +\n> +private:\n> +\tvoid parseYaml(const YamlObject &tuningData);\n> +\tvoid setParameters(rkisp1_params_cfg *params,\n> +\t\t\t const Matrix &matrix,\n> +\t\t\t const Matrix &offsets);\n> +\n> +\tMatrixInterpolator ccm_;\n> +\tMatrixInterpolator offsets_;\n> +};\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> index 93a48329..16de7133 100644\n> --- a/src/ipa/rkisp1/algorithms/meson.build\n> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> 'agc.cpp',\n> 'awb.cpp',\n> 'blc.cpp',\n> + 'ccm.cpp',\n> 'cproc.cpp',\n> 'dpcc.cpp',\n> 'dpf.cpp',\n> -- \n> 2.39.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8086BBDE6B\n\tfor ;\n\tWed, 15 May 2024 09:42:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 786EA6347E;\n\tWed, 15 May 2024 11:42:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D4F8E63466\n\tfor ;\n\tWed, 15 May 2024 11:42:21 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:f83e:cbfd:31ad:3642])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1131E512;\n\tWed, 15 May 2024 11:42:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qcgVFSUS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715766134;\n\tbh=bIBFMLzvB+y/wBF8kCJqOrCI0y9EJsoNQfquhQaqIXw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qcgVFSUSGZHN7UZ5NeI01UUGcDzRlK6socQVdtFQVHAZRDH5xcQRPSqGFJw1ksSd8\n\tjoHpEm/U1WTEQhEFyxkocxoEl7pOl4QC7nAhIJTHP2RD6SiqEKJnOW00i09Jvb8miN\n\tOF00HYgexh8D+AZ4QgLCCzAeXtUAuBPArwRxhzR4=","Date":"Wed, 15 May 2024 11:42:18 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"<20240515094218.omugezhzrhqp734c@jasper>","References":"<20240507061016.1716450-1-paul.elder@ideasonboard.com>\n\t<20240507061016.1716450-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240507061016.1716450-4-paul.elder@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29546,"web_url":"https://patchwork.libcamera.org/comment/29546/","msgid":"","date":"2024-05-17T06:09:06","subject":"Re: [PATCH v3 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, May 15, 2024 at 11:42:18AM +0200, Stefan Klug wrote:\n> Hi Paul,\n> \n> thanks for the patch.\n> \n> On Tue, May 07, 2024 at 03:10:16PM +0900, Paul Elder wrote:\n> > Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > This patch depends on \"[PATCH] libcamera: utils: Add a helper to convert\n> > floating-point to fixed-point\"\n> > \n> > Changes in v3:\n> > - read ccm offsets from tuning data, and write these offsets to the\n> > parameters buffer\n> > - make parseYaml return void, as it should fill in default data if\n> > unable to read, thus never failing\n> > \n> > Changes in v2:\n> > - rename ctk to ccm\n> > - reset the matrix interpolator to identity matrix if failed to read\n> > from tuning file\n> > ---\n> > src/ipa/rkisp1/algorithms/ccm.cpp | 116 ++++++++++++++++++++++++++\n> > src/ipa/rkisp1/algorithms/ccm.h | 44 ++++++++++\n> > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > 3 files changed, 161 insertions(+)\n> > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > new file mode 100644\n> > index 00000000..11fa5ec2\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > @@ -0,0 +1,116 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + *\n> > + * ccm.cpp - RkISP1 Cross Talk Correction control algorithm\n> > + */\n> > +\n> > +#include \"ccm.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +#include \"libipa/matrix_interpolator.h\"\n> > +\n> > +/**\n> > + * \\file ctk.h\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +/**\n> > + * \\class Ccm\n> > + * \\brief A cross talk correction algorithm\n> > + */\n> > +\n> > +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > +\n> > +void Ccm::parseYaml(const YamlObject &tuningData)\n> > +{\n> > +\tint ret = ccm_.readYaml(tuningData[\"ccms\"]);\n> > +\tif (ret < 0) {\n> > +\t\tLOG(RkISP1Ccm, Warning)\n> > +\t\t\t<< \"Failed to parse 'ccms' \"\n> > +\t\t\t<< \"parameter from tuning file; falling back to unit matrix\";\n> > +\t\tccm_.reset();\n> > +\t}\n> > +\n> > +\tret = offsets_.readYaml(tuningData[\"offsets\"]);\n> \n> Ahh I should have read that patch first. This explains how you solved\n> the \"more data per temperature\" question. We can also postpone that for\n> now and see what we really need.\n> \n> > +\tif (ret < 0) {\n> > +\t\tLOG(RkISP1Ccm, Warning)\n> > +\t\t\t<< \"Failed to parse 'offsets' \"\n> > +\t\t\t<< \"parameter from tuning file; falling back to zero offsets\";\n> > +\t\tconst std::map> offsets = {\n> > +\t\t\t{ 0, Matrix({ 0, 0, 0 }) }\n> > +\t\t};\n> \n> Oh, beeing able to use the interpolator for the offsets is nice. Having\n> to do manual handling of the \"identity\" case doesn't feel too good. As\n> the identity matrix is only defined for square matrices what about\n> initializing the matrix to identity only if R==C in the constructor and\n> to zero otherwise? Or is that too much magic?\n\nOh, good idea. We can do that.\n\n> \n> > +\t\tMatrixInterpolator interpolator(offsets);\n> > +\t\toffsets_ = interpolator;\n> > +\t}\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::init\n> > + */\n> > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > +{\n> > +\tparseYaml(tuningData);\n> > +\treturn 0;\n> > +}\n> > +\n> > +void Ccm::setParameters(rkisp1_params_cfg *params,\n> > +\t\t\tconst Matrix &matrix,\n> > +\t\t\tconst Matrix &offsets)\n> > +{\n> > +\tstruct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> > +\n> > +\t/*\n> > +\t * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > +\t * +7.992 (0x3FF)\n> > +\t */\n> > +\tfor (unsigned int i = 0; i < 3; i++)\n> > +\t\tfor (unsigned int j = 0; j < 3; j++)\n> > +\t\t\tconfig.coeff[i][j] =\n> > +\t\t\t\tutils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix.get(i, j));\n> > +\n> > +\tfor (unsigned int i = 0; i < 3; i++)\n> > +\t\tconfig.ct_offset[i] = offsets.get(i, 0) & 0xFFF;\n> \n> If we go for floats in the matrix, we should handle the offsets the same\n> way.\n\nThe offsets are integers between (and including) -2048 to 2047 so the\nfloats don't really apply. The tuning tool with output these values\ndirectly.\n\n> \n> > +\n> > +\tLOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix.toString();\n> > +\tLOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets.toString();\n> \n> Just having to write \n> LOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> would be beautiful :-)\n\nIndeed :)\n\n\nThanks,\n\nPaul\n\n> \n> > +\n> > +\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > +\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> > +\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > + */\n> > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> > +\t\t rkisp1_params_cfg *params)\n> > +{\n> > +\tuint32_t ct = context.activeState.awb.temperatureK;\n> > +\tMatrix ccm = ccm_.get(ct);\n> > +\tMatrix offsets = offsets_.get(ct);\n> > +\n> > +\tsetParameters(params, ccm, offsets);\n> > +}\n> > +\n> > +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> > +\n> > +} /* namespace ipa::rkisp1::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > new file mode 100644\n> > index 00000000..dcab5cd3\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > @@ -0,0 +1,44 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + *\n> > + * ccm.h - RkISP1 Cross Talk Correction control algorithm\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \"libipa/matrix.h\"\n> > +#include \"libipa/matrix_interpolator.h\"\n> > +\n> > +#include \"algorithm.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +class Ccm : public Algorithm\n> > +{\n> > +public:\n> > +\tCcm(){};\n> > +\t~Ccm() = default;\n> > +\n> > +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> > +\tvoid prepare(IPAContext &context, const uint32_t frame,\n> > +\t\t IPAFrameContext &frameContext,\n> > +\t\t rkisp1_params_cfg *params) override;\n> > +\n> > +private:\n> > +\tvoid parseYaml(const YamlObject &tuningData);\n> > +\tvoid setParameters(rkisp1_params_cfg *params,\n> > +\t\t\t const Matrix &matrix,\n> > +\t\t\t const Matrix &offsets);\n> > +\n> > +\tMatrixInterpolator ccm_;\n> > +\tMatrixInterpolator offsets_;\n> > +};\n> > +\n> > +} /* namespace ipa::rkisp1::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > index 93a48329..16de7133 100644\n> > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> > 'agc.cpp',\n> > 'awb.cpp',\n> > 'blc.cpp',\n> > + 'ccm.cpp',\n> > 'cproc.cpp',\n> > 'dpcc.cpp',\n> > 'dpf.cpp',\n> > -- \n> > 2.39.2\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7A0D0BD78E\n\tfor ;\n\tFri, 17 May 2024 06:09:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 769016347E;\n\tFri, 17 May 2024 08:09:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D911B61A51\n\tfor ;\n\tFri, 17 May 2024 08:09:14 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C5930A9A;\n\tFri, 17 May 2024 08:09:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YxtO57Eu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715926145;\n\tbh=3EdHuDjk+mVD+JZUjGpd0NPswwW5t++jJvBh0GEAoUc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YxtO57EuQb/Gws0MRQVNBKdgbPFoL+Rxozdtf0Rqct9yEBP7kKpBzjlxIgoSTFvD7\n\tiCw0ahC/Zjah3c9RN5sXIBWbqFBtrQ/83LSLfMDhCeZoW6ZScdm/j6H6caVdT8qr8s\n\tqcOOJkbe1sCPMMj5gdIcXZLlfKwBIoB1PkBMA6jk=","Date":"Fri, 17 May 2024 15:09:06 +0900","From":"Paul Elder ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"","References":"<20240507061016.1716450-1-paul.elder@ideasonboard.com>\n\t<20240507061016.1716450-4-paul.elder@ideasonboard.com>\n\t<20240515094218.omugezhzrhqp734c@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240515094218.omugezhzrhqp734c@jasper>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29548,"web_url":"https://patchwork.libcamera.org/comment/29548/","msgid":"<20240517075911.eex6kzcuyvdx4qh4@jasper>","date":"2024-05-17T07:59:11","subject":"Re: [PATCH v3 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Fri, May 17, 2024 at 03:09:06PM +0900, Paul Elder wrote:\n> On Wed, May 15, 2024 at 11:42:18AM +0200, Stefan Klug wrote:\n> > Hi Paul,\n> > \n> > thanks for the patch.\n> > \n> > On Tue, May 07, 2024 at 03:10:16PM +0900, Paul Elder wrote:\n> > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > \n> > > ---\n> > > This patch depends on \"[PATCH] libcamera: utils: Add a helper to convert\n> > > floating-point to fixed-point\"\n> > > \n> > > Changes in v3:\n> > > - read ccm offsets from tuning data, and write these offsets to the\n> > > parameters buffer\n> > > - make parseYaml return void, as it should fill in default data if\n> > > unable to read, thus never failing\n> > > \n> > > Changes in v2:\n> > > - rename ctk to ccm\n> > > - reset the matrix interpolator to identity matrix if failed to read\n> > > from tuning file\n> > > ---\n> > > src/ipa/rkisp1/algorithms/ccm.cpp | 116 ++++++++++++++++++++++++++\n> > > src/ipa/rkisp1/algorithms/ccm.h | 44 ++++++++++\n> > > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > > 3 files changed, 161 insertions(+)\n> > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > new file mode 100644\n> > > index 00000000..11fa5ec2\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > @@ -0,0 +1,116 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas On Board\n> > > + *\n> > > + * ccm.cpp - RkISP1 Cross Talk Correction control algorithm\n> > > + */\n> > > +\n> > > +#include \"ccm.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > +\n> > > +#include \"libipa/matrix_interpolator.h\"\n> > > +\n> > > +/**\n> > > + * \\file ctk.h\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::algorithms {\n> > > +\n> > > +/**\n> > > + * \\class Ccm\n> > > + * \\brief A cross talk correction algorithm\n> > > + */\n> > > +\n> > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > > +\n> > > +void Ccm::parseYaml(const YamlObject &tuningData)\n> > > +{\n> > > +\tint ret = ccm_.readYaml(tuningData[\"ccms\"]);\n> > > +\tif (ret < 0) {\n> > > +\t\tLOG(RkISP1Ccm, Warning)\n> > > +\t\t\t<< \"Failed to parse 'ccms' \"\n> > > +\t\t\t<< \"parameter from tuning file; falling back to unit matrix\";\n> > > +\t\tccm_.reset();\n> > > +\t}\n> > > +\n> > > +\tret = offsets_.readYaml(tuningData[\"offsets\"]);\n> > \n> > Ahh I should have read that patch first. This explains how you solved\n> > the \"more data per temperature\" question. We can also postpone that for\n> > now and see what we really need.\n> > \n> > > +\tif (ret < 0) {\n> > > +\t\tLOG(RkISP1Ccm, Warning)\n> > > +\t\t\t<< \"Failed to parse 'offsets' \"\n> > > +\t\t\t<< \"parameter from tuning file; falling back to zero offsets\";\n> > > +\t\tconst std::map> offsets = {\n> > > +\t\t\t{ 0, Matrix({ 0, 0, 0 }) }\n> > > +\t\t};\n> > \n> > Oh, beeing able to use the interpolator for the offsets is nice. Having\n> > to do manual handling of the \"identity\" case doesn't feel too good. As\n> > the identity matrix is only defined for square matrices what about\n> > initializing the matrix to identity only if R==C in the constructor and\n> > to zero otherwise? Or is that too much magic?\n> \n> Oh, good idea. We can do that.\n> \n> > \n> > > +\t\tMatrixInterpolator interpolator(offsets);\n> > > +\t\toffsets_ = interpolator;\n> > > +\t}\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > + */\n> > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > > +{\n> > > +\tparseYaml(tuningData);\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +void Ccm::setParameters(rkisp1_params_cfg *params,\n> > > +\t\t\tconst Matrix &matrix,\n> > > +\t\t\tconst Matrix &offsets)\n> > > +{\n> > > +\tstruct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> > > +\n> > > +\t/*\n> > > +\t * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > > +\t * +7.992 (0x3FF)\n> > > +\t */\n> > > +\tfor (unsigned int i = 0; i < 3; i++)\n> > > +\t\tfor (unsigned int j = 0; j < 3; j++)\n> > > +\t\t\tconfig.coeff[i][j] =\n> > > +\t\t\t\tutils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix.get(i, j));\n> > > +\n> > > +\tfor (unsigned int i = 0; i < 3; i++)\n> > > +\t\tconfig.ct_offset[i] = offsets.get(i, 0) & 0xFFF;\n> > \n> > If we go for floats in the matrix, we should handle the offsets the same\n> > way.\n> \n> The offsets are integers between (and including) -2048 to 2047 so the\n> floats don't really apply. The tuning tool with output these values\n> directly.\n> \n\nOh I missed that. I actually thought these where also fixed point. Sorry\nfor the noise.\n\nCheers,\nStefan\n\n> > \n> > > +\n> > > +\tLOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix.toString();\n> > > +\tLOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets.toString();\n> > \n> > Just having to write \n> > LOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> > would be beautiful :-)\n> \n> Indeed :)\n> \n> \n> Thanks,\n> \n> Paul\n> \n> > \n> > > +\n> > > +\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > +\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > +\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > + */\n> > > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> > > +\t\t rkisp1_params_cfg *params)\n> > > +{\n> > > +\tuint32_t ct = context.activeState.awb.temperatureK;\n> > > +\tMatrix ccm = ccm_.get(ct);\n> > > +\tMatrix offsets = offsets_.get(ct);\n> > > +\n> > > +\tsetParameters(params, ccm, offsets);\n> > > +}\n> > > +\n> > > +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> > > +\n> > > +} /* namespace ipa::rkisp1::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > > new file mode 100644\n> > > index 00000000..dcab5cd3\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > > @@ -0,0 +1,44 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas On Board\n> > > + *\n> > > + * ccm.h - RkISP1 Cross Talk Correction control algorithm\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libipa/matrix.h\"\n> > > +#include \"libipa/matrix_interpolator.h\"\n> > > +\n> > > +#include \"algorithm.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::algorithms {\n> > > +\n> > > +class Ccm : public Algorithm\n> > > +{\n> > > +public:\n> > > +\tCcm(){};\n> > > +\t~Ccm() = default;\n> > > +\n> > > +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> > > +\tvoid prepare(IPAContext &context, const uint32_t frame,\n> > > +\t\t IPAFrameContext &frameContext,\n> > > +\t\t rkisp1_params_cfg *params) override;\n> > > +\n> > > +private:\n> > > +\tvoid parseYaml(const YamlObject &tuningData);\n> > > +\tvoid setParameters(rkisp1_params_cfg *params,\n> > > +\t\t\t const Matrix &matrix,\n> > > +\t\t\t const Matrix &offsets);\n> > > +\n> > > +\tMatrixInterpolator ccm_;\n> > > +\tMatrixInterpolator offsets_;\n> > > +};\n> > > +\n> > > +} /* namespace ipa::rkisp1::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > > index 93a48329..16de7133 100644\n> > > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> > > 'agc.cpp',\n> > > 'awb.cpp',\n> > > 'blc.cpp',\n> > > + 'ccm.cpp',\n> > > 'cproc.cpp',\n> > > 'dpcc.cpp',\n> > > 'dpf.cpp',\n> > > -- \n> > > 2.39.2\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 63D76BD78E\n\tfor ;\n\tFri, 17 May 2024 07:59:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1540C63484;\n\tFri, 17 May 2024 09:59:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94ADC6347C\n\tfor ;\n\tFri, 17 May 2024 09:59:14 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:d572:8aa2:3e8e:5b99])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 97812475;\n\tFri, 17 May 2024 09:59:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"G/o51jl7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715932745;\n\tbh=LJDA2hoflQOnfon9SbMqKirnLTEd0DgOHmb/LGhj8F4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G/o51jl730tCdm0vlIvFBNrqMxKB0X3q+UD/OE7RRLD7pkDbVPiDhQiqCmbsymotT\n\trf4O+RyA3964YyrsphPwH7wLrRwYkhgoX6MlxZ8QZ4iPyUYicmjTomrHc6NrSA5E1o\n\teFc0eliAyAQnrfmMwlaayVzR3Ly1vb9zvps6eMqI=","Date":"Fri, 17 May 2024 09:59:11 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"<20240517075911.eex6kzcuyvdx4qh4@jasper>","References":"<20240507061016.1716450-1-paul.elder@ideasonboard.com>\n\t<20240507061016.1716450-4-paul.elder@ideasonboard.com>\n\t<20240515094218.omugezhzrhqp734c@jasper>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]