[v7,3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm

20263 diff mbox series

Add an algorithm module to the rkisp1 IPA for crosstalk correction.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

---
Changes in v7:
- make offsets_ default to zero-matrices as opposed to identity matrices
- checkstyle
- populate metadata
  - add ccm to IPAFrameContext
- don't update the ccm if the color temperature didn't change

No change in v6

Changes in v5:
- clean up documentation
- coalesce parseYaml into init

Changes in v4:
- remove stray semicolons
- use the new matrix interpolator readYaml
- use the new matrix operator[] getter

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     | 140 ++++++++++++++++++++++++++
 src/ipa/rkisp1/algorithms/ccm.h       |  50 +++++++++
 src/ipa/rkisp1/algorithms/meson.build |   1 +
 src/ipa/rkisp1/ipa_context.h          |   5 +
 4 files changed, 196 insertions(+)
 create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp
 create mode 100644 src/ipa/rkisp1/algorithms/ccm.h

Quoting Paul Elder (2024-06-11 15:02:07)
> Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> Changes in v7:
> - make offsets_ default to zero-matrices as opposed to identity matrices
> - checkstyle
> - populate metadata
>   - add ccm to IPAFrameContext
> - don't update the ccm if the color temperature didn't change
> 
> No change in v6
> 
> Changes in v5:
> - clean up documentation
> - coalesce parseYaml into init
> 
> Changes in v4:
> - remove stray semicolons
> - use the new matrix interpolator readYaml
> - use the new matrix operator[] getter
> 
> 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     | 140 ++++++++++++++++++++++++++
>  src/ipa/rkisp1/algorithms/ccm.h       |  50 +++++++++
>  src/ipa/rkisp1/algorithms/meson.build |   1 +
>  src/ipa/rkisp1/ipa_context.h          |   5 +
>  4 files changed, 196 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 000000000000..09fe4b2aa1bc
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -0,0 +1,140 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + *
> + * RkISP1 Color Correction Matrix 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/control_ids.h>
> +
> +#include <libcamera/ipa/core_ipa_interface.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "../utils.h"
> +#include "libipa/matrix_interpolator.h"
> +
> +/**
> + * \file ccm.h
> + */
> +
> +namespace libcamera {
> +
> +namespace ipa::rkisp1::algorithms {
> +
> +/**
> + * \class Ccm
> + * \brief A color correction matrix algorithm
> + */
> +
> +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::init
> + */
> +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> +{
> +       int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> +       if (ret < 0) {
> +               LOG(RkISP1Ccm, Warning)
> +                       << "Failed to parse 'ccm' "
> +                       << "parameter from tuning file; falling back to unit matrix";
> +               ccm_.reset();
> +       }
> +
> +       ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> +       if (ret < 0) {
> +               LOG(RkISP1Ccm, Warning)
> +                       << "Failed to parse 'offsets' "
> +                       << "parameter from tuning file; falling back to zero offsets";
> +               /*
> +                * MatrixInterpolator::reset() resets to identity matrices
> +                * while here we need zero matrices so we need to construct it
> +                * ourselves.
> +                */
> +               Matrix<int16_t, 3, 1> m({ 0, 0, 0 });
> +               std::map<unsigned int, Matrix<int16_t, 3, 1>> matrices = { { 0, m } };
> +               offsets_ = MatrixInterpolator<int16_t, 3, 1>(matrices);
> +       }
> +
> +       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[i][j]);
> +
> +       for (unsigned int i = 0; i < 3; i++)
> +               config.ct_offset[i] = offsets[i][0] & 0xfff;
> +
> +       LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> +       LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets;
> +
> +       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,
> +                 IPAFrameContext &frameContext,
> +                 rkisp1_params_cfg *params)
> +{
> +       uint32_t ct = context.activeState.awb.temperatureK;
> +       if (ct == ct_)
> +               return;

Interestingly, I think this is fine (certainly for now) ... but this
means that the output request metadata will only write the CCM if it
changes.

I think that's actually 'fine' if we imply that any frame that doesn't
supply metadata is the same as the previous state. Though I think RPi
currently reports it for every frame regardless.

I'd be curious to see how Stefan's tooling handles this.

We can always update on top, and my comments are addressed so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> +
> +       ct_ = ct;
> +       Matrix<double, 3, 3> ccm = ccm_.get(ct);
> +       Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> +
> +       frameContext.ccm.ccm = ccm;
> +
> +       setParameters(params, ccm, offsets);
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::process
> + */
> +void process([[maybe_unused]] IPAContext &context,
> +            [[maybe_unused]] const uint32_t frame,
> +            IPAFrameContext &frameContext,
> +            [[maybe_unused]] const rkisp1_stat_buffer *stats,
> +            ControlList &metadata)
> +{
> +       float m[9];
> +       for (unsigned int i = 0; i < 3; i++)
> +               for (unsigned int j = 0; j < 3; j++)
> +                       m[i] = frameContext.ccm.ccm[i][j];
> +       metadata.set(controls::ColourCorrectionMatrix, m);
> +}
> +
> +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 000000000000..09a6801626b4
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/ccm.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + *
> + * RkISP1 Color Correction Matrix 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;
> +       void process([[maybe_unused]] IPAContext &context,
> +                    [[maybe_unused]] const uint32_t frame,
> +                    IPAFrameContext &frameContext,
> +                    [[maybe_unused]] const rkisp1_stat_buffer *stats,
> +                    ControlList &metadata) 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);
> +
> +       unsigned int ct_;
> +       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 6ee71a9b5da3..1734a6675f78 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',
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 2a994d81ae41..cfb1f9770870 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -16,6 +16,7 @@
>  #include <libcamera/geometry.h>
>  
>  #include <libipa/fc_queue.h>
> +#include <libipa/matrix.h>
>  
>  namespace libcamera {
>  
> @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {
>                 uint32_t exposure;
>                 double gain;
>         } sensor;
> +
> +       struct {
> +               Matrix<double, 3, 3> ccm;
> +       } ccm;
>  };
>  
>  struct IPAContext {
> -- 
> 2.39.2
>

Hi Paul,

Thank you for the patch.

On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2024-06-11 15:02:07)
> > Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > ---
> > Changes in v7:
> > - make offsets_ default to zero-matrices as opposed to identity matrices
> > - checkstyle
> > - populate metadata
> >   - add ccm to IPAFrameContext
> > - don't update the ccm if the color temperature didn't change
> > 
> > No change in v6
> > 
> > Changes in v5:
> > - clean up documentation
> > - coalesce parseYaml into init
> > 
> > Changes in v4:
> > - remove stray semicolons
> > - use the new matrix interpolator readYaml
> > - use the new matrix operator[] getter
> > 
> > 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     | 140 ++++++++++++++++++++++++++
> >  src/ipa/rkisp1/algorithms/ccm.h       |  50 +++++++++
> >  src/ipa/rkisp1/algorithms/meson.build |   1 +
> >  src/ipa/rkisp1/ipa_context.h          |   5 +
> >  4 files changed, 196 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 000000000000..09fe4b2aa1bc
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > @@ -0,0 +1,140 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 Color Correction Matrix 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/control_ids.h>
> > +
> > +#include <libcamera/ipa/core_ipa_interface.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "../utils.h"
> > +#include "libipa/matrix_interpolator.h"
> > +
> > +/**
> > + * \file ccm.h
> > + */
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +/**
> > + * \class Ccm
> > + * \brief A color correction matrix algorithm
> > + */
> > +
> > +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::init
> > + */
> > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > +{
> > +       int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> > +       if (ret < 0) {
> > +               LOG(RkISP1Ccm, Warning)
> > +                       << "Failed to parse 'ccm' "
> > +                       << "parameter from tuning file; falling back to unit matrix";
> > +               ccm_.reset();
> > +       }
> > +
> > +       ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > +       if (ret < 0) {
> > +               LOG(RkISP1Ccm, Warning)
> > +                       << "Failed to parse 'offsets' "
> > +                       << "parameter from tuning file; falling back to zero offsets";
> > +               /*
> > +                * MatrixInterpolator::reset() resets to identity matrices
> > +                * while here we need zero matrices so we need to construct it
> > +                * ourselves.
> > +                */
> > +               Matrix<int16_t, 3, 1> m({ 0, 0, 0 });
> > +               std::map<unsigned int, Matrix<int16_t, 3, 1>> matrices = { { 0, m } };
> > +               offsets_ = MatrixInterpolator<int16_t, 3, 1>(matrices);
> > +       }
> > +
> > +       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[i][j]);

Curly braces for outer loop.

> > +
> > +       for (unsigned int i = 0; i < 3; i++)
> > +               config.ct_offset[i] = offsets[i][0] & 0xfff;
> > +
> > +       LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> > +       LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets;
> > +
> > +       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,
> > +                 IPAFrameContext &frameContext,
> > +                 rkisp1_params_cfg *params)
> > +{
> > +       uint32_t ct = context.activeState.awb.temperatureK;
> > +       if (ct == ct_)

ct_ should be initialized/reset in configure(), to ensure that we'll
always set the CCM matrix on the first frame.

I expect temperatureK to be fairly noisy, should we add a threshold to
the comparison to avoid updating the CCM parameters on every frame in
practice ?

> > +               return;
> 
> Interestingly, I think this is fine (certainly for now) ... but this
> means that the output request metadata will only write the CCM if it
> changes.
> 
> I think that's actually 'fine' if we imply that any frame that doesn't
> supply metadata is the same as the previous state. Though I think RPi
> currently reports it for every frame regardless.
> 
> I'd be curious to see how Stefan's tooling handles this.
> 
> We can always update on top, and my comments are addressed so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +
> > +       ct_ = ct;
> > +       Matrix<double, 3, 3> ccm = ccm_.get(ct);
> > +       Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> > +
> > +       frameContext.ccm.ccm = ccm;
> > +
> > +       setParameters(params, ccm, offsets);
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::process
> > + */
> > +void process([[maybe_unused]] IPAContext &context,
> > +            [[maybe_unused]] const uint32_t frame,
> > +            IPAFrameContext &frameContext,
> > +            [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > +            ControlList &metadata)
> > +{
> > +       float m[9];
> > +       for (unsigned int i = 0; i < 3; i++)
> > +               for (unsigned int j = 0; j < 3; j++)
> > +                       m[i] = frameContext.ccm.ccm[i][j];

Curly braces for outer loop.

> > +       metadata.set(controls::ColourCorrectionMatrix, m);

It's interesting we're reporting the matrix but not the offsets. Not a
blocker for this series, but should this be addressed ?

> > +}
> > +
> > +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 000000000000..09a6801626b4
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > @@ -0,0 +1,50 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 Color Correction Matrix 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;
> > +       void process([[maybe_unused]] IPAContext &context,

[[maybe_unused]] is not needed in the function declaration.

> > +                    [[maybe_unused]] const uint32_t frame,
> > +                    IPAFrameContext &frameContext,
> > +                    [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > +                    ControlList &metadata) 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);
> > +
> > +       unsigned int ct_;
> > +       MatrixInterpolator<double, 3, 3> ccm_;

Given that the hardware representation is a 4.7 fixed point, would it be
enough to use floats instead of doubles (here and in the frame context)
?

> > +       MatrixInterpolator<int16_t, 3, 1> offsets_;

Why is the matrix stored in floating point values and the offsets as
integers ?

> > +};
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > index 6ee71a9b5da3..1734a6675f78 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',
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 2a994d81ae41..cfb1f9770870 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -16,6 +16,7 @@
> >  #include <libcamera/geometry.h>
> >  
> >  #include <libipa/fc_queue.h>
> > +#include <libipa/matrix.h>
> >  
> >  namespace libcamera {
> >  
> > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {
> >                 uint32_t exposure;
> >                 double gain;
> >         } sensor;
> > +
> > +       struct {
> > +               Matrix<double, 3, 3> ccm;
> > +       } ccm;
> >  };
> >  
> >  struct IPAContext {

On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:
> > Quoting Paul Elder (2024-06-11 15:02:07)
> > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > 
> > > ---
> > > Changes in v7:
> > > - make offsets_ default to zero-matrices as opposed to identity matrices
> > > - checkstyle
> > > - populate metadata
> > >   - add ccm to IPAFrameContext
> > > - don't update the ccm if the color temperature didn't change
> > > 
> > > No change in v6
> > > 
> > > Changes in v5:
> > > - clean up documentation
> > > - coalesce parseYaml into init
> > > 
> > > Changes in v4:
> > > - remove stray semicolons
> > > - use the new matrix interpolator readYaml
> > > - use the new matrix operator[] getter
> > > 
> > > 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     | 140 ++++++++++++++++++++++++++
> > >  src/ipa/rkisp1/algorithms/ccm.h       |  50 +++++++++
> > >  src/ipa/rkisp1/algorithms/meson.build |   1 +
> > >  src/ipa/rkisp1/ipa_context.h          |   5 +
> > >  4 files changed, 196 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 000000000000..09fe4b2aa1bc
> > > --- /dev/null
> > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > @@ -0,0 +1,140 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas On Board
> > > + *
> > > + * RkISP1 Color Correction Matrix 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/control_ids.h>
> > > +
> > > +#include <libcamera/ipa/core_ipa_interface.h>
> > > +
> > > +#include "libcamera/internal/yaml_parser.h"
> > > +
> > > +#include "../utils.h"
> > > +#include "libipa/matrix_interpolator.h"
> > > +
> > > +/**
> > > + * \file ccm.h
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa::rkisp1::algorithms {
> > > +
> > > +/**
> > > + * \class Ccm
> > > + * \brief A color correction matrix algorithm
> > > + */
> > > +
> > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > > +
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::init
> > > + */
> > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > > +{
> > > +       int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> > > +       if (ret < 0) {
> > > +               LOG(RkISP1Ccm, Warning)
> > > +                       << "Failed to parse 'ccm' "
> > > +                       << "parameter from tuning file; falling back to unit matrix";
> > > +               ccm_.reset();
> > > +       }
> > > +
> > > +       ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > > +       if (ret < 0) {
> > > +               LOG(RkISP1Ccm, Warning)
> > > +                       << "Failed to parse 'offsets' "
> > > +                       << "parameter from tuning file; falling back to zero offsets";
> > > +               /*
> > > +                * MatrixInterpolator::reset() resets to identity matrices
> > > +                * while here we need zero matrices so we need to construct it
> > > +                * ourselves.
> > > +                */
> > > +               Matrix<int16_t, 3, 1> m({ 0, 0, 0 });
> > > +               std::map<unsigned int, Matrix<int16_t, 3, 1>> matrices = { { 0, m } };
> > > +               offsets_ = MatrixInterpolator<int16_t, 3, 1>(matrices);
> > > +       }
> > > +
> > > +       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[i][j]);
> 
> Curly braces for outer loop.
> 
> > > +
> > > +       for (unsigned int i = 0; i < 3; i++)
> > > +               config.ct_offset[i] = offsets[i][0] & 0xfff;
> > > +
> > > +       LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> > > +       LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets;
> > > +
> > > +       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,
> > > +                 IPAFrameContext &frameContext,
> > > +                 rkisp1_params_cfg *params)
> > > +{
> > > +       uint32_t ct = context.activeState.awb.temperatureK;
> > > +       if (ct == ct_)
> 
> ct_ should be initialized/reset in configure(), to ensure that we'll
> always set the CCM matrix on the first frame.
> 
> I expect temperatureK to be fairly noisy, should we add a threshold to
> the comparison to avoid updating the CCM parameters on every frame in
> practice ?
> 
> > > +               return;
> > 
> > Interestingly, I think this is fine (certainly for now) ... but this
> > means that the output request metadata will only write the CCM if it
> > changes.

There was discussion before about "do we need to always write *all*
metadata" and iirc the conclusion was no so...

> > 
> > I think that's actually 'fine' if we imply that any frame that doesn't
> > supply metadata is the same as the previous state. Though I think RPi
> > currently reports it for every frame regardless.
> > 
> > I'd be curious to see how Stefan's tooling handles this.
> > 
> > We can always update on top, and my comments are addressed so:
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > > +
> > > +       ct_ = ct;
> > > +       Matrix<double, 3, 3> ccm = ccm_.get(ct);
> > > +       Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> > > +
> > > +       frameContext.ccm.ccm = ccm;
> > > +
> > > +       setParameters(params, ccm, offsets);
> > > +}
> > > +
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::process
> > > + */
> > > +void process([[maybe_unused]] IPAContext &context,
> > > +            [[maybe_unused]] const uint32_t frame,
> > > +            IPAFrameContext &frameContext,
> > > +            [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > +            ControlList &metadata)
> > > +{
> > > +       float m[9];
> > > +       for (unsigned int i = 0; i < 3; i++)
> > > +               for (unsigned int j = 0; j < 3; j++)
> > > +                       m[i] = frameContext.ccm.ccm[i][j];
> 
> Curly braces for outer loop.
> 
> > > +       metadata.set(controls::ColourCorrectionMatrix, m);
> 
> It's interesting we're reporting the matrix but not the offsets. Not a
> blocker for this series, but should this be addressed ?
> 

I wasn't sure if there was demand for reporting the offsets...

And actually I remember a link [1] that Stefan sent me a long time ago
that said something about them costing more computation and rarely
improving results...?

[1] https://www.imatest.com/docs/colormatrix/

> > > +}
> > > +
> > > +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 000000000000..09a6801626b4
> > > --- /dev/null
> > > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > > @@ -0,0 +1,50 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas On Board
> > > + *
> > > + * RkISP1 Color Correction Matrix 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;
> > > +       void process([[maybe_unused]] IPAContext &context,
> 
> [[maybe_unused]] is not needed in the function declaration.
> 
> > > +                    [[maybe_unused]] const uint32_t frame,
> > > +                    IPAFrameContext &frameContext,
> > > +                    [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > +                    ControlList &metadata) 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);
> > > +
> > > +       unsigned int ct_;
> > > +       MatrixInterpolator<double, 3, 3> ccm_;
> 
> Given that the hardware representation is a 4.7 fixed point, would it be
> enough to use floats instead of doubles (here and in the frame context)
> ?
> 
> > > +       MatrixInterpolator<int16_t, 3, 1> offsets_;
> 
> Why is the matrix stored in floating point values and the offsets as
> integers ?

Because the offsets are integers in hardware as well while the ccm is
fixed-point.


Paul

> 
> > > +};
> > > +
> > > +} /* namespace ipa::rkisp1::algorithms */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > > index 6ee71a9b5da3..1734a6675f78 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',
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 2a994d81ae41..cfb1f9770870 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -16,6 +16,7 @@
> > >  #include <libcamera/geometry.h>
> > >  
> > >  #include <libipa/fc_queue.h>
> > > +#include <libipa/matrix.h>
> > >  
> > >  namespace libcamera {
> > >  
> > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {
> > >                 uint32_t exposure;
> > >                 double gain;
> > >         } sensor;
> > > +
> > > +       struct {
> > > +               Matrix<double, 3, 3> ccm;
> > > +       } ccm;
> > >  };
> > >  
> > >  struct IPAContext {
>

On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:
> On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:
> > > Quoting Paul Elder (2024-06-11 15:02:07)
> > > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> > > > 
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > 
> > > > ---
> > > > Changes in v7:
> > > > - make offsets_ default to zero-matrices as opposed to identity matrices
> > > > - checkstyle
> > > > - populate metadata
> > > >   - add ccm to IPAFrameContext
> > > > - don't update the ccm if the color temperature didn't change
> > > > 
> > > > No change in v6
> > > > 
> > > > Changes in v5:
> > > > - clean up documentation
> > > > - coalesce parseYaml into init
> > > > 
> > > > Changes in v4:
> > > > - remove stray semicolons
> > > > - use the new matrix interpolator readYaml
> > > > - use the new matrix operator[] getter
> > > > 
> > > > 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     | 140 ++++++++++++++++++++++++++
> > > >  src/ipa/rkisp1/algorithms/ccm.h       |  50 +++++++++
> > > >  src/ipa/rkisp1/algorithms/meson.build |   1 +
> > > >  src/ipa/rkisp1/ipa_context.h          |   5 +
> > > >  4 files changed, 196 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 000000000000..09fe4b2aa1bc
> > > > --- /dev/null
> > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > > @@ -0,0 +1,140 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2024, Ideas On Board
> > > > + *
> > > > + * RkISP1 Color Correction Matrix 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/control_ids.h>
> > > > +
> > > > +#include <libcamera/ipa/core_ipa_interface.h>
> > > > +
> > > > +#include "libcamera/internal/yaml_parser.h"
> > > > +
> > > > +#include "../utils.h"
> > > > +#include "libipa/matrix_interpolator.h"
> > > > +
> > > > +/**
> > > > + * \file ccm.h
> > > > + */
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +namespace ipa::rkisp1::algorithms {
> > > > +
> > > > +/**
> > > > + * \class Ccm
> > > > + * \brief A color correction matrix algorithm
> > > > + */
> > > > +
> > > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > > > +
> > > > +/**
> > > > + * \copydoc libcamera::ipa::Algorithm::init
> > > > + */
> > > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > > > +{
> > > > +       int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> > > > +       if (ret < 0) {
> > > > +               LOG(RkISP1Ccm, Warning)
> > > > +                       << "Failed to parse 'ccm' "
> > > > +                       << "parameter from tuning file; falling back to unit matrix";
> > > > +               ccm_.reset();
> > > > +       }
> > > > +
> > > > +       ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > > > +       if (ret < 0) {
> > > > +               LOG(RkISP1Ccm, Warning)
> > > > +                       << "Failed to parse 'offsets' "
> > > > +                       << "parameter from tuning file; falling back to zero offsets";
> > > > +               /*
> > > > +                * MatrixInterpolator::reset() resets to identity matrices
> > > > +                * while here we need zero matrices so we need to construct it
> > > > +                * ourselves.
> > > > +                */
> > > > +               Matrix<int16_t, 3, 1> m({ 0, 0, 0 });
> > > > +               std::map<unsigned int, Matrix<int16_t, 3, 1>> matrices = { { 0, m } };
> > > > +               offsets_ = MatrixInterpolator<int16_t, 3, 1>(matrices);
> > > > +       }
> > > > +
> > > > +       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[i][j]);
> > 
> > Curly braces for outer loop.
> > 
> > > > +
> > > > +       for (unsigned int i = 0; i < 3; i++)
> > > > +               config.ct_offset[i] = offsets[i][0] & 0xfff;
> > > > +
> > > > +       LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> > > > +       LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets;
> > > > +
> > > > +       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,
> > > > +                 IPAFrameContext &frameContext,
> > > > +                 rkisp1_params_cfg *params)
> > > > +{
> > > > +       uint32_t ct = context.activeState.awb.temperatureK;
> > > > +       if (ct == ct_)
> > 
> > ct_ should be initialized/reset in configure(), to ensure that we'll
> > always set the CCM matrix on the first frame.
> > 
> > I expect temperatureK to be fairly noisy, should we add a threshold to
> > the comparison to avoid updating the CCM parameters on every frame in
> > practice ?
> > 
> > > > +               return;
> > > 
> > > Interestingly, I think this is fine (certainly for now) ... but this
> > > means that the output request metadata will only write the CCM if it
> > > changes.
> 
> There was discussion before about "do we need to always write *all*
> metadata" and iirc the conclusion was no so...

I'm sure we'll revisit the topic at some point.

> > > I think that's actually 'fine' if we imply that any frame that doesn't
> > > supply metadata is the same as the previous state. Though I think RPi
> > > currently reports it for every frame regardless.
> > > 
> > > I'd be curious to see how Stefan's tooling handles this.
> > > 
> > > We can always update on top, and my comments are addressed so:
> > > 
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > > +
> > > > +       ct_ = ct;
> > > > +       Matrix<double, 3, 3> ccm = ccm_.get(ct);
> > > > +       Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> > > > +
> > > > +       frameContext.ccm.ccm = ccm;
> > > > +
> > > > +       setParameters(params, ccm, offsets);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \copydoc libcamera::ipa::Algorithm::process
> > > > + */
> > > > +void process([[maybe_unused]] IPAContext &context,
> > > > +            [[maybe_unused]] const uint32_t frame,
> > > > +            IPAFrameContext &frameContext,
> > > > +            [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > +            ControlList &metadata)
> > > > +{
> > > > +       float m[9];
> > > > +       for (unsigned int i = 0; i < 3; i++)
> > > > +               for (unsigned int j = 0; j < 3; j++)
> > > > +                       m[i] = frameContext.ccm.ccm[i][j];
> > 
> > Curly braces for outer loop.
> > 
> > > > +       metadata.set(controls::ColourCorrectionMatrix, m);
> > 
> > It's interesting we're reporting the matrix but not the offsets. Not a
> > blocker for this series, but should this be addressed ?
> 
> I wasn't sure if there was demand for reporting the offsets...
> 
> And actually I remember a link [1] that Stefan sent me a long time ago
> that said something about them costing more computation and rarely
> improving results...?

That's what I was wondering too. I wonder if we should drop the offset.

> [1] https://www.imatest.com/docs/colormatrix/
> 
> > > > +}
> > > > +
> > > > +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 000000000000..09a6801626b4
> > > > --- /dev/null
> > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > > > @@ -0,0 +1,50 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2024, Ideas On Board
> > > > + *
> > > > + * RkISP1 Color Correction Matrix 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;
> > > > +       void process([[maybe_unused]] IPAContext &context,
> > 
> > [[maybe_unused]] is not needed in the function declaration.
> > 
> > > > +                    [[maybe_unused]] const uint32_t frame,
> > > > +                    IPAFrameContext &frameContext,
> > > > +                    [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > +                    ControlList &metadata) 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);
> > > > +
> > > > +       unsigned int ct_;
> > > > +       MatrixInterpolator<double, 3, 3> ccm_;
> > 
> > Given that the hardware representation is a 4.7 fixed point, would it be
> > enough to use floats instead of doubles (here and in the frame context)
> > ?
> > 
> > > > +       MatrixInterpolator<int16_t, 3, 1> offsets_;
> > 
> > Why is the matrix stored in floating point values and the offsets as
> > integers ?
> 
> Because the offsets are integers in hardware as well while the ccm is
> fixed-point.
> 
> > > > +};
> > > > +
> > > > +} /* namespace ipa::rkisp1::algorithms */
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > > > index 6ee71a9b5da3..1734a6675f78 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',
> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > index 2a994d81ae41..cfb1f9770870 100644
> > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > @@ -16,6 +16,7 @@
> > > >  #include <libcamera/geometry.h>
> > > >  
> > > >  #include <libipa/fc_queue.h>
> > > > +#include <libipa/matrix.h>
> > > >  
> > > >  namespace libcamera {
> > > >  
> > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {
> > > >                 uint32_t exposure;
> > > >                 double gain;
> > > >         } sensor;
> > > > +
> > > > +       struct {
> > > > +               Matrix<double, 3, 3> ccm;
> > > > +       } ccm;
> > > >  };
> > > >  
> > > >  struct IPAContext {

On Wed, Jun 12, 2024 at 12:38:34PM +0300, Laurent Pinchart wrote:
> On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:
> > On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:
> > > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:
> > > > Quoting Paul Elder (2024-06-11 15:02:07)
> > > > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> > > > > 
> > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > > 
> > > > > ---
> > > > > Changes in v7:
> > > > > - make offsets_ default to zero-matrices as opposed to identity matrices
> > > > > - checkstyle
> > > > > - populate metadata
> > > > >   - add ccm to IPAFrameContext
> > > > > - don't update the ccm if the color temperature didn't change
> > > > > 
> > > > > No change in v6
> > > > > 
> > > > > Changes in v5:
> > > > > - clean up documentation
> > > > > - coalesce parseYaml into init
> > > > > 
> > > > > Changes in v4:
> > > > > - remove stray semicolons
> > > > > - use the new matrix interpolator readYaml
> > > > > - use the new matrix operator[] getter
> > > > > 
> > > > > 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     | 140 ++++++++++++++++++++++++++
> > > > >  src/ipa/rkisp1/algorithms/ccm.h       |  50 +++++++++
> > > > >  src/ipa/rkisp1/algorithms/meson.build |   1 +
> > > > >  src/ipa/rkisp1/ipa_context.h          |   5 +
> > > > >  4 files changed, 196 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 000000000000..09fe4b2aa1bc
> > > > > --- /dev/null
> > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > > > @@ -0,0 +1,140 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2024, Ideas On Board
> > > > > + *
> > > > > + * RkISP1 Color Correction Matrix 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/control_ids.h>
> > > > > +
> > > > > +#include <libcamera/ipa/core_ipa_interface.h>
> > > > > +
> > > > > +#include "libcamera/internal/yaml_parser.h"
> > > > > +
> > > > > +#include "../utils.h"
> > > > > +#include "libipa/matrix_interpolator.h"
> > > > > +
> > > > > +/**
> > > > > + * \file ccm.h
> > > > > + */
> > > > > +
> > > > > +namespace libcamera {
> > > > > +
> > > > > +namespace ipa::rkisp1::algorithms {
> > > > > +
> > > > > +/**
> > > > > + * \class Ccm
> > > > > + * \brief A color correction matrix algorithm
> > > > > + */
> > > > > +
> > > > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > > > > +
> > > > > +/**
> > > > > + * \copydoc libcamera::ipa::Algorithm::init
> > > > > + */
> > > > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > > > > +{
> > > > > +       int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> > > > > +       if (ret < 0) {
> > > > > +               LOG(RkISP1Ccm, Warning)
> > > > > +                       << "Failed to parse 'ccm' "
> > > > > +                       << "parameter from tuning file; falling back to unit matrix";
> > > > > +               ccm_.reset();
> > > > > +       }
> > > > > +
> > > > > +       ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > > > > +       if (ret < 0) {
> > > > > +               LOG(RkISP1Ccm, Warning)
> > > > > +                       << "Failed to parse 'offsets' "
> > > > > +                       << "parameter from tuning file; falling back to zero offsets";
> > > > > +               /*
> > > > > +                * MatrixInterpolator::reset() resets to identity matrices
> > > > > +                * while here we need zero matrices so we need to construct it
> > > > > +                * ourselves.
> > > > > +                */
> > > > > +               Matrix<int16_t, 3, 1> m({ 0, 0, 0 });
> > > > > +               std::map<unsigned int, Matrix<int16_t, 3, 1>> matrices = { { 0, m } };
> > > > > +               offsets_ = MatrixInterpolator<int16_t, 3, 1>(matrices);
> > > > > +       }
> > > > > +
> > > > > +       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[i][j]);
> > > 
> > > Curly braces for outer loop.
> > > 
> > > > > +
> > > > > +       for (unsigned int i = 0; i < 3; i++)
> > > > > +               config.ct_offset[i] = offsets[i][0] & 0xfff;
> > > > > +
> > > > > +       LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> > > > > +       LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets;
> > > > > +
> > > > > +       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,
> > > > > +                 IPAFrameContext &frameContext,
> > > > > +                 rkisp1_params_cfg *params)
> > > > > +{
> > > > > +       uint32_t ct = context.activeState.awb.temperatureK;
> > > > > +       if (ct == ct_)
> > > 
> > > ct_ should be initialized/reset in configure(), to ensure that we'll
> > > always set the CCM matrix on the first frame.
> > > 
> > > I expect temperatureK to be fairly noisy, should we add a threshold to
> > > the comparison to avoid updating the CCM parameters on every frame in
> > > practice ?
> > > 
> > > > > +               return;
> > > > 
> > > > Interestingly, I think this is fine (certainly for now) ... but this
> > > > means that the output request metadata will only write the CCM if it
> > > > changes.
> > 
> > There was discussion before about "do we need to always write *all*
> > metadata" and iirc the conclusion was no so...
> 
> I'm sure we'll revisit the topic at some point.
> 
> > > > I think that's actually 'fine' if we imply that any frame that doesn't
> > > > supply metadata is the same as the previous state. Though I think RPi
> > > > currently reports it for every frame regardless.
> > > > 
> > > > I'd be curious to see how Stefan's tooling handles this.
> > > > 
> > > > We can always update on top, and my comments are addressed so:
> > > > 
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > 
> > > > > +
> > > > > +       ct_ = ct;
> > > > > +       Matrix<double, 3, 3> ccm = ccm_.get(ct);
> > > > > +       Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> > > > > +
> > > > > +       frameContext.ccm.ccm = ccm;
> > > > > +
> > > > > +       setParameters(params, ccm, offsets);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * \copydoc libcamera::ipa::Algorithm::process
> > > > > + */
> > > > > +void process([[maybe_unused]] IPAContext &context,
> > > > > +            [[maybe_unused]] const uint32_t frame,
> > > > > +            IPAFrameContext &frameContext,
> > > > > +            [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > > +            ControlList &metadata)
> > > > > +{
> > > > > +       float m[9];
> > > > > +       for (unsigned int i = 0; i < 3; i++)
> > > > > +               for (unsigned int j = 0; j < 3; j++)
> > > > > +                       m[i] = frameContext.ccm.ccm[i][j];
> > > 
> > > Curly braces for outer loop.
> > > 
> > > > > +       metadata.set(controls::ColourCorrectionMatrix, m);
> > > 
> > > It's interesting we're reporting the matrix but not the offsets. Not a
> > > blocker for this series, but should this be addressed ?
> > 
> > I wasn't sure if there was demand for reporting the offsets...
> > 
> > And actually I remember a link [1] that Stefan sent me a long time ago
> > that said something about them costing more computation and rarely
> > improving results...?
> 
> That's what I was wondering too. I wonder if we should drop the offset.
> 

I think they should be there to have the /option/ of being set.


Paul

> > [1] https://www.imatest.com/docs/colormatrix/
> > 
> > > > > +}
> > > > > +
> > > > > +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 000000000000..09a6801626b4
> > > > > --- /dev/null
> > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > > > > @@ -0,0 +1,50 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2024, Ideas On Board
> > > > > + *
> > > > > + * RkISP1 Color Correction Matrix 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;
> > > > > +       void process([[maybe_unused]] IPAContext &context,
> > > 
> > > [[maybe_unused]] is not needed in the function declaration.
> > > 
> > > > > +                    [[maybe_unused]] const uint32_t frame,
> > > > > +                    IPAFrameContext &frameContext,
> > > > > +                    [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > > +                    ControlList &metadata) 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);
> > > > > +
> > > > > +       unsigned int ct_;
> > > > > +       MatrixInterpolator<double, 3, 3> ccm_;
> > > 
> > > Given that the hardware representation is a 4.7 fixed point, would it be
> > > enough to use floats instead of doubles (here and in the frame context)
> > > ?
> > > 
> > > > > +       MatrixInterpolator<int16_t, 3, 1> offsets_;
> > > 
> > > Why is the matrix stored in floating point values and the offsets as
> > > integers ?
> > 
> > Because the offsets are integers in hardware as well while the ccm is
> > fixed-point.
> > 
> > > > > +};
> > > > > +
> > > > > +} /* namespace ipa::rkisp1::algorithms */
> > > > > +
> > > > > +} /* namespace libcamera */
> > > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > > > > index 6ee71a9b5da3..1734a6675f78 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',
> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > index 2a994d81ae41..cfb1f9770870 100644
> > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > @@ -16,6 +16,7 @@
> > > > >  #include <libcamera/geometry.h>
> > > > >  
> > > > >  #include <libipa/fc_queue.h>
> > > > > +#include <libipa/matrix.h>
> > > > >  
> > > > >  namespace libcamera {
> > > > >  
> > > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {
> > > > >                 uint32_t exposure;
> > > > >                 double gain;
> > > > >         } sensor;
> > > > > +
> > > > > +       struct {
> > > > > +               Matrix<double, 3, 3> ccm;
> > > > > +       } ccm;
> > > > >  };
> > > > >  
> > > > >  struct IPAContext {
>

On Thu, Jun 13, 2024 at 05:13:28PM +0900, Paul Elder wrote:
> On Wed, Jun 12, 2024 at 12:38:34PM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:
> > > On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:
> > > > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:
> > > > > Quoting Paul Elder (2024-06-11 15:02:07)
> > > > > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> > > > > > 
> > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > > > 
> > > > > > ---
> > > > > > Changes in v7:
> > > > > > - make offsets_ default to zero-matrices as opposed to identity matrices
> > > > > > - checkstyle
> > > > > > - populate metadata
> > > > > >   - add ccm to IPAFrameContext
> > > > > > - don't update the ccm if the color temperature didn't change
> > > > > > 
> > > > > > No change in v6
> > > > > > 
> > > > > > Changes in v5:
> > > > > > - clean up documentation
> > > > > > - coalesce parseYaml into init
> > > > > > 
> > > > > > Changes in v4:
> > > > > > - remove stray semicolons
> > > > > > - use the new matrix interpolator readYaml
> > > > > > - use the new matrix operator[] getter
> > > > > > 
> > > > > > 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     | 140 ++++++++++++++++++++++++++
> > > > > >  src/ipa/rkisp1/algorithms/ccm.h       |  50 +++++++++
> > > > > >  src/ipa/rkisp1/algorithms/meson.build |   1 +
> > > > > >  src/ipa/rkisp1/ipa_context.h          |   5 +
> > > > > >  4 files changed, 196 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 000000000000..09fe4b2aa1bc
> > > > > > --- /dev/null
> > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > > > > @@ -0,0 +1,140 @@
> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > +/*
> > > > > > + * Copyright (C) 2024, Ideas On Board
> > > > > > + *
> > > > > > + * RkISP1 Color Correction Matrix 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/control_ids.h>
> > > > > > +
> > > > > > +#include <libcamera/ipa/core_ipa_interface.h>
> > > > > > +
> > > > > > +#include "libcamera/internal/yaml_parser.h"
> > > > > > +
> > > > > > +#include "../utils.h"
> > > > > > +#include "libipa/matrix_interpolator.h"
> > > > > > +
> > > > > > +/**
> > > > > > + * \file ccm.h
> > > > > > + */
> > > > > > +
> > > > > > +namespace libcamera {
> > > > > > +
> > > > > > +namespace ipa::rkisp1::algorithms {
> > > > > > +
> > > > > > +/**
> > > > > > + * \class Ccm
> > > > > > + * \brief A color correction matrix algorithm
> > > > > > + */
> > > > > > +
> > > > > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > > > > > +
> > > > > > +/**
> > > > > > + * \copydoc libcamera::ipa::Algorithm::init
> > > > > > + */
> > > > > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > > > > > +{
> > > > > > +       int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> > > > > > +       if (ret < 0) {
> > > > > > +               LOG(RkISP1Ccm, Warning)
> > > > > > +                       << "Failed to parse 'ccm' "
> > > > > > +                       << "parameter from tuning file; falling back to unit matrix";
> > > > > > +               ccm_.reset();
> > > > > > +       }
> > > > > > +
> > > > > > +       ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > > > > > +       if (ret < 0) {
> > > > > > +               LOG(RkISP1Ccm, Warning)
> > > > > > +                       << "Failed to parse 'offsets' "
> > > > > > +                       << "parameter from tuning file; falling back to zero offsets";
> > > > > > +               /*
> > > > > > +                * MatrixInterpolator::reset() resets to identity matrices
> > > > > > +                * while here we need zero matrices so we need to construct it
> > > > > > +                * ourselves.
> > > > > > +                */
> > > > > > +               Matrix<int16_t, 3, 1> m({ 0, 0, 0 });
> > > > > > +               std::map<unsigned int, Matrix<int16_t, 3, 1>> matrices = { { 0, m } };
> > > > > > +               offsets_ = MatrixInterpolator<int16_t, 3, 1>(matrices);
> > > > > > +       }
> > > > > > +
> > > > > > +       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[i][j]);
> > > > 
> > > > Curly braces for outer loop.
> > > > 
> > > > > > +
> > > > > > +       for (unsigned int i = 0; i < 3; i++)
> > > > > > +               config.ct_offset[i] = offsets[i][0] & 0xfff;
> > > > > > +
> > > > > > +       LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> > > > > > +       LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets;
> > > > > > +
> > > > > > +       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,
> > > > > > +                 IPAFrameContext &frameContext,
> > > > > > +                 rkisp1_params_cfg *params)
> > > > > > +{
> > > > > > +       uint32_t ct = context.activeState.awb.temperatureK;
> > > > > > +       if (ct == ct_)
> > > > 
> > > > ct_ should be initialized/reset in configure(), to ensure that we'll
> > > > always set the CCM matrix on the first frame.
> > > > 
> > > > I expect temperatureK to be fairly noisy, should we add a threshold to
> > > > the comparison to avoid updating the CCM parameters on every frame in
> > > > practice ?
> > > > 
> > > > > > +               return;
> > > > > 
> > > > > Interestingly, I think this is fine (certainly for now) ... but this
> > > > > means that the output request metadata will only write the CCM if it
> > > > > changes.
> > > 
> > > There was discussion before about "do we need to always write *all*
> > > metadata" and iirc the conclusion was no so...
> > 
> > I'm sure we'll revisit the topic at some point.
> > 
> > > > > I think that's actually 'fine' if we imply that any frame that doesn't
> > > > > supply metadata is the same as the previous state. Though I think RPi
> > > > > currently reports it for every frame regardless.
> > > > > 
> > > > > I'd be curious to see how Stefan's tooling handles this.
> > > > > 
> > > > > We can always update on top, and my comments are addressed so:
> > > > > 
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > 
> > > > > > +
> > > > > > +       ct_ = ct;
> > > > > > +       Matrix<double, 3, 3> ccm = ccm_.get(ct);
> > > > > > +       Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> > > > > > +
> > > > > > +       frameContext.ccm.ccm = ccm;
> > > > > > +
> > > > > > +       setParameters(params, ccm, offsets);
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * \copydoc libcamera::ipa::Algorithm::process
> > > > > > + */
> > > > > > +void process([[maybe_unused]] IPAContext &context,
> > > > > > +            [[maybe_unused]] const uint32_t frame,
> > > > > > +            IPAFrameContext &frameContext,
> > > > > > +            [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > > > +            ControlList &metadata)
> > > > > > +{
> > > > > > +       float m[9];
> > > > > > +       for (unsigned int i = 0; i < 3; i++)
> > > > > > +               for (unsigned int j = 0; j < 3; j++)
> > > > > > +                       m[i] = frameContext.ccm.ccm[i][j];
> > > > 
> > > > Curly braces for outer loop.
> > > > 
> > > > > > +       metadata.set(controls::ColourCorrectionMatrix, m);
> > > > 
> > > > It's interesting we're reporting the matrix but not the offsets. Not a
> > > > blocker for this series, but should this be addressed ?
> > > 
> > > I wasn't sure if there was demand for reporting the offsets...
> > > 
> > > And actually I remember a link [1] that Stefan sent me a long time ago
> > > that said something about them costing more computation and rarely
> > > improving results...?
> > 
> > That's what I was wondering too. I wonder if we should drop the offset.
> 
> I think they should be there to have the /option/ of being set.

It costs a little bit of CPU time. Most likely not the end of the world,
but things do add up. I suppose we'll get more visibility once we have a
tuning tool in place to produce the data. If the tool hardcodes offsets
to 0, I'd rather drop them from the implementation.

> > > [1] https://www.imatest.com/docs/colormatrix/
> > > 
> > > > > > +}
> > > > > > +
> > > > > > +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 000000000000..09a6801626b4
> > > > > > --- /dev/null
> > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > > > > > @@ -0,0 +1,50 @@
> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > +/*
> > > > > > + * Copyright (C) 2024, Ideas On Board
> > > > > > + *
> > > > > > + * RkISP1 Color Correction Matrix 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;
> > > > > > +       void process([[maybe_unused]] IPAContext &context,
> > > > 
> > > > [[maybe_unused]] is not needed in the function declaration.
> > > > 
> > > > > > +                    [[maybe_unused]] const uint32_t frame,
> > > > > > +                    IPAFrameContext &frameContext,
> > > > > > +                    [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > > > +                    ControlList &metadata) 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);
> > > > > > +
> > > > > > +       unsigned int ct_;
> > > > > > +       MatrixInterpolator<double, 3, 3> ccm_;
> > > > 
> > > > Given that the hardware representation is a 4.7 fixed point, would it be
> > > > enough to use floats instead of doubles (here and in the frame context)
> > > > ?
> > > > 
> > > > > > +       MatrixInterpolator<int16_t, 3, 1> offsets_;
> > > > 
> > > > Why is the matrix stored in floating point values and the offsets as
> > > > integers ?
> > > 
> > > Because the offsets are integers in hardware as well while the ccm is
> > > fixed-point.
> > > 
> > > > > > +};
> > > > > > +
> > > > > > +} /* namespace ipa::rkisp1::algorithms */
> > > > > > +
> > > > > > +} /* namespace libcamera */
> > > > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > > > > > index 6ee71a9b5da3..1734a6675f78 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',
> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > > index 2a994d81ae41..cfb1f9770870 100644
> > > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > > @@ -16,6 +16,7 @@
> > > > > >  #include <libcamera/geometry.h>
> > > > > >  
> > > > > >  #include <libipa/fc_queue.h>
> > > > > > +#include <libipa/matrix.h>
> > > > > >  
> > > > > >  namespace libcamera {
> > > > > >  
> > > > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {
> > > > > >                 uint32_t exposure;
> > > > > >                 double gain;
> > > > > >         } sensor;
> > > > > > +
> > > > > > +       struct {
> > > > > > +               Matrix<double, 3, 3> ccm;
> > > > > > +       } ccm;
> > > > > >  };
> > > > > >  
> > > > > >  struct IPAContext {

Hi Paul,

I rebased my tuning work on this new series and got a undefined symbol
when loading the ipa. The definition of process() is missing Ccm::


On Tue, Jun 11, 2024 at 11:02:07PM +0900, Paul Elder wrote:
> Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> Changes in v7:
> - make offsets_ default to zero-matrices as opposed to identity matrices
> - checkstyle
> - populate metadata
>   - add ccm to IPAFrameContext
> - don't update the ccm if the color temperature didn't change
> 
> No change in v6
> 
> Changes in v5:
> - clean up documentation
> - coalesce parseYaml into init
> 
> Changes in v4:
> - remove stray semicolons
> - use the new matrix interpolator readYaml
> - use the new matrix operator[] getter
> 
> 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     | 140 ++++++++++++++++++++++++++
>  src/ipa/rkisp1/algorithms/ccm.h       |  50 +++++++++
>  src/ipa/rkisp1/algorithms/meson.build |   1 +
>  src/ipa/rkisp1/ipa_context.h          |   5 +
>  4 files changed, 196 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 000000000000..09fe4b2aa1bc
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -0,0 +1,140 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + *
> + * RkISP1 Color Correction Matrix 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/control_ids.h>
> +
> +#include <libcamera/ipa/core_ipa_interface.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include "../utils.h"
> +#include "libipa/matrix_interpolator.h"
> +
> +/**
> + * \file ccm.h
> + */
> +
> +namespace libcamera {
> +
> +namespace ipa::rkisp1::algorithms {
> +
> +/**
> + * \class Ccm
> + * \brief A color correction matrix algorithm
> + */
> +
> +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::init
> + */
> +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> +{
> +	int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> +	if (ret < 0) {
> +		LOG(RkISP1Ccm, Warning)
> +			<< "Failed to parse 'ccm' "
> +			<< "parameter from tuning file; falling back to unit matrix";
> +		ccm_.reset();
> +	}
> +
> +	ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> +	if (ret < 0) {
> +		LOG(RkISP1Ccm, Warning)
> +			<< "Failed to parse 'offsets' "
> +			<< "parameter from tuning file; falling back to zero offsets";
> +		/*
> +		 * MatrixInterpolator::reset() resets to identity matrices
> +		 * while here we need zero matrices so we need to construct it
> +		 * ourselves.
> +		 */
> +		Matrix<int16_t, 3, 1> m({ 0, 0, 0 });
> +		std::map<unsigned int, Matrix<int16_t, 3, 1>> matrices = { { 0, m } };
> +		offsets_ = MatrixInterpolator<int16_t, 3, 1>(matrices);
> +	}
> +
> +	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[i][j]);
> +
> +	for (unsigned int i = 0; i < 3; i++)
> +		config.ct_offset[i] = offsets[i][0] & 0xfff;
> +
> +	LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> +	LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets;
> +
> +	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,
> +		  IPAFrameContext &frameContext,
> +		  rkisp1_params_cfg *params)
> +{
> +	uint32_t ct = context.activeState.awb.temperatureK;
> +	if (ct == ct_)
> +		return;
> +
> +	ct_ = ct;
> +	Matrix<double, 3, 3> ccm = ccm_.get(ct);
> +	Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> +
> +	frameContext.ccm.ccm = ccm;
> +
> +	setParameters(params, ccm, offsets);
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::process
> + */
> +void process([[maybe_unused]] IPAContext &context,

This needs to be Ccm::process.

Cheers,
Stefan

> +	     [[maybe_unused]] const uint32_t frame,
> +	     IPAFrameContext &frameContext,
> +	     [[maybe_unused]] const rkisp1_stat_buffer *stats,
> +	     ControlList &metadata)
> +{
> +	float m[9];
> +	for (unsigned int i = 0; i < 3; i++)
> +		for (unsigned int j = 0; j < 3; j++)
> +			m[i] = frameContext.ccm.ccm[i][j];
> +	metadata.set(controls::ColourCorrectionMatrix, m);
> +}
> +
> +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 000000000000..09a6801626b4
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/ccm.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + *
> + * RkISP1 Color Correction Matrix 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;
> +	void process([[maybe_unused]] IPAContext &context,
> +		     [[maybe_unused]] const uint32_t frame,
> +		     IPAFrameContext &frameContext,
> +		     [[maybe_unused]] const rkisp1_stat_buffer *stats,
> +		     ControlList &metadata) 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);
> +
> +	unsigned int ct_;
> +	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 6ee71a9b5da3..1734a6675f78 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',
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 2a994d81ae41..cfb1f9770870 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -16,6 +16,7 @@
>  #include <libcamera/geometry.h>
>  
>  #include <libipa/fc_queue.h>
> +#include <libipa/matrix.h>
>  
>  namespace libcamera {
>  
> @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {
>  		uint32_t exposure;
>  		double gain;
>  	} sensor;
> +
> +	struct {
> +		Matrix<double, 3, 3> ccm;
> +	} ccm;
>  };
>  
>  struct IPAContext {
> -- 
> 2.39.2
>

Quoting Laurent Pinchart (2024-06-13 13:04:53)
> On Thu, Jun 13, 2024 at 05:13:28PM +0900, Paul Elder wrote:
> > On Wed, Jun 12, 2024 at 12:38:34PM +0300, Laurent Pinchart wrote:
> > > On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:
> > > > On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:
> > > > > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:
> > > > > > Quoting Paul Elder (2024-06-11 15:02:07)
> > > > > > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> > > > > > > 
> > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > > > > 
> > > > > > > ---
> > > > > > > Changes in v7:
> > > > > > > - make offsets_ default to zero-matrices as opposed to identity matrices
> > > > > > > - checkstyle
> > > > > > > - populate metadata
> > > > > > >   - add ccm to IPAFrameContext
> > > > > > > - don't update the ccm if the color temperature didn't change
> > > > > > > 
> > > > > > > No change in v6
> > > > > > > 
> > > > > > > Changes in v5:
> > > > > > > - clean up documentation
> > > > > > > - coalesce parseYaml into init
> > > > > > > 
> > > > > > > Changes in v4:
> > > > > > > - remove stray semicolons
> > > > > > > - use the new matrix interpolator readYaml
> > > > > > > - use the new matrix operator[] getter
> > > > > > > 
> > > > > > > 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     | 140 ++++++++++++++++++++++++++
> > > > > > >  src/ipa/rkisp1/algorithms/ccm.h       |  50 +++++++++
> > > > > > >  src/ipa/rkisp1/algorithms/meson.build |   1 +
> > > > > > >  src/ipa/rkisp1/ipa_context.h          |   5 +
> > > > > > >  4 files changed, 196 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 000000000000..09fe4b2aa1bc
> > > > > > > --- /dev/null
> > > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > > > > > @@ -0,0 +1,140 @@
> > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > > +/*
> > > > > > > + * Copyright (C) 2024, Ideas On Board
> > > > > > > + *
> > > > > > > + * RkISP1 Color Correction Matrix 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/control_ids.h>
> > > > > > > +
> > > > > > > +#include <libcamera/ipa/core_ipa_interface.h>
> > > > > > > +
> > > > > > > +#include "libcamera/internal/yaml_parser.h"
> > > > > > > +
> > > > > > > +#include "../utils.h"
> > > > > > > +#include "libipa/matrix_interpolator.h"
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \file ccm.h
> > > > > > > + */
> > > > > > > +
> > > > > > > +namespace libcamera {
> > > > > > > +
> > > > > > > +namespace ipa::rkisp1::algorithms {
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \class Ccm
> > > > > > > + * \brief A color correction matrix algorithm
> > > > > > > + */
> > > > > > > +
> > > > > > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \copydoc libcamera::ipa::Algorithm::init
> > > > > > > + */
> > > > > > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > > > > > > +{
> > > > > > > +       int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> > > > > > > +       if (ret < 0) {
> > > > > > > +               LOG(RkISP1Ccm, Warning)
> > > > > > > +                       << "Failed to parse 'ccm' "
> > > > > > > +                       << "parameter from tuning file; falling back to unit matrix";
> > > > > > > +               ccm_.reset();
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > > > > > > +       if (ret < 0) {
> > > > > > > +               LOG(RkISP1Ccm, Warning)
> > > > > > > +                       << "Failed to parse 'offsets' "
> > > > > > > +                       << "parameter from tuning file; falling back to zero offsets";
> > > > > > > +               /*
> > > > > > > +                * MatrixInterpolator::reset() resets to identity matrices
> > > > > > > +                * while here we need zero matrices so we need to construct it
> > > > > > > +                * ourselves.
> > > > > > > +                */
> > > > > > > +               Matrix<int16_t, 3, 1> m({ 0, 0, 0 });
> > > > > > > +               std::map<unsigned int, Matrix<int16_t, 3, 1>> matrices = { { 0, m } };
> > > > > > > +               offsets_ = MatrixInterpolator<int16_t, 3, 1>(matrices);
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       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[i][j]);
> > > > > 
> > > > > Curly braces for outer loop.
> > > > > 
> > > > > > > +
> > > > > > > +       for (unsigned int i = 0; i < 3; i++)
> > > > > > > +               config.ct_offset[i] = offsets[i][0] & 0xfff;
> > > > > > > +
> > > > > > > +       LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> > > > > > > +       LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets;
> > > > > > > +
> > > > > > > +       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,
> > > > > > > +                 IPAFrameContext &frameContext,
> > > > > > > +                 rkisp1_params_cfg *params)
> > > > > > > +{
> > > > > > > +       uint32_t ct = context.activeState.awb.temperatureK;
> > > > > > > +       if (ct == ct_)
> > > > > 
> > > > > ct_ should be initialized/reset in configure(), to ensure that we'll
> > > > > always set the CCM matrix on the first frame.
> > > > > 
> > > > > I expect temperatureK to be fairly noisy, should we add a threshold to
> > > > > the comparison to avoid updating the CCM parameters on every frame in
> > > > > practice ?
> > > > > 
> > > > > > > +               return;
> > > > > > 
> > > > > > Interestingly, I think this is fine (certainly for now) ... but this
> > > > > > means that the output request metadata will only write the CCM if it
> > > > > > changes.
> > > > 
> > > > There was discussion before about "do we need to always write *all*
> > > > metadata" and iirc the conclusion was no so...
> > > 
> > > I'm sure we'll revisit the topic at some point.
> > > 
> > > > > > I think that's actually 'fine' if we imply that any frame that doesn't
> > > > > > supply metadata is the same as the previous state. Though I think RPi
> > > > > > currently reports it for every frame regardless.
> > > > > > 
> > > > > > I'd be curious to see how Stefan's tooling handles this.
> > > > > > 
> > > > > > We can always update on top, and my comments are addressed so:
> > > > > > 
> > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > > 
> > > > > > > +
> > > > > > > +       ct_ = ct;
> > > > > > > +       Matrix<double, 3, 3> ccm = ccm_.get(ct);
> > > > > > > +       Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> > > > > > > +
> > > > > > > +       frameContext.ccm.ccm = ccm;
> > > > > > > +
> > > > > > > +       setParameters(params, ccm, offsets);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \copydoc libcamera::ipa::Algorithm::process
> > > > > > > + */
> > > > > > > +void process([[maybe_unused]] IPAContext &context,
> > > > > > > +            [[maybe_unused]] const uint32_t frame,
> > > > > > > +            IPAFrameContext &frameContext,
> > > > > > > +            [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > > > > +            ControlList &metadata)
> > > > > > > +{
> > > > > > > +       float m[9];
> > > > > > > +       for (unsigned int i = 0; i < 3; i++)
> > > > > > > +               for (unsigned int j = 0; j < 3; j++)
> > > > > > > +                       m[i] = frameContext.ccm.ccm[i][j];
> > > > > 
> > > > > Curly braces for outer loop.
> > > > > 
> > > > > > > +       metadata.set(controls::ColourCorrectionMatrix, m);
> > > > > 
> > > > > It's interesting we're reporting the matrix but not the offsets. Not a
> > > > > blocker for this series, but should this be addressed ?
> > > > 
> > > > I wasn't sure if there was demand for reporting the offsets...
> > > > 
> > > > And actually I remember a link [1] that Stefan sent me a long time ago
> > > > that said something about them costing more computation and rarely
> > > > improving results...?
> > > 
> > > That's what I was wondering too. I wonder if we should drop the offset.
> > 
> > I think they should be there to have the /option/ of being set.
> 
> It costs a little bit of CPU time. Most likely not the end of the world,
> but things do add up. I suppose we'll get more visibility once we have a

Yes, but if we don't have the ability to control we won't be able to
experiement.  I'd rather keep this in. The cost is low. Unless we're
really worried about the cost of 3 integers...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> tuning tool in place to produce the data. If the tool hardcodes offsets
> to 0, I'd rather drop them from the implementation.
> 
> > > > [1] https://www.imatest.com/docs/colormatrix/
> > > > 
> > > > > > > +}
> > > > > > > +
> > > > > > > +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 000000000000..09a6801626b4
> > > > > > > --- /dev/null
> > > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > > > > > > @@ -0,0 +1,50 @@
> > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > > +/*
> > > > > > > + * Copyright (C) 2024, Ideas On Board
> > > > > > > + *
> > > > > > > + * RkISP1 Color Correction Matrix 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;
> > > > > > > +       void process([[maybe_unused]] IPAContext &context,
> > > > > 
> > > > > [[maybe_unused]] is not needed in the function declaration.
> > > > > 
> > > > > > > +                    [[maybe_unused]] const uint32_t frame,
> > > > > > > +                    IPAFrameContext &frameContext,
> > > > > > > +                    [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > > > > +                    ControlList &metadata) 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);
> > > > > > > +
> > > > > > > +       unsigned int ct_;
> > > > > > > +       MatrixInterpolator<double, 3, 3> ccm_;
> > > > > 
> > > > > Given that the hardware representation is a 4.7 fixed point, would it be
> > > > > enough to use floats instead of doubles (here and in the frame context)
> > > > > ?
> > > > > 
> > > > > > > +       MatrixInterpolator<int16_t, 3, 1> offsets_;
> > > > > 
> > > > > Why is the matrix stored in floating point values and the offsets as
> > > > > integers ?
> > > > 
> > > > Because the offsets are integers in hardware as well while the ccm is
> > > > fixed-point.
> > > > 
> > > > > > > +};
> > > > > > > +
> > > > > > > +} /* namespace ipa::rkisp1::algorithms */
> > > > > > > +
> > > > > > > +} /* namespace libcamera */
> > > > > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > > > > > > index 6ee71a9b5da3..1734a6675f78 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',
> > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > > > index 2a994d81ae41..cfb1f9770870 100644
> > > > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > > > @@ -16,6 +16,7 @@
> > > > > > >  #include <libcamera/geometry.h>
> > > > > > >  
> > > > > > >  #include <libipa/fc_queue.h>
> > > > > > > +#include <libipa/matrix.h>
> > > > > > >  
> > > > > > >  namespace libcamera {
> > > > > > >  
> > > > > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {
> > > > > > >                 uint32_t exposure;
> > > > > > >                 double gain;
> > > > > > >         } sensor;
> > > > > > > +
> > > > > > > +       struct {
> > > > > > > +               Matrix<double, 3, 3> ccm;
> > > > > > > +       } ccm;
> > > > > > >  };
> > > > > > >  
> > > > > > >  struct IPAContext {
> 
> -- 
> Regards,
> 
> Laurent Pinchart

On Fri, Jun 14, 2024 at 09:20:35AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-13 13:04:53)
> > On Thu, Jun 13, 2024 at 05:13:28PM +0900, Paul Elder wrote:
> > > On Wed, Jun 12, 2024 at 12:38:34PM +0300, Laurent Pinchart wrote:
> > > > On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:
> > > > > On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:
> > > > > > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:
> > > > > > > Quoting Paul Elder (2024-06-11 15:02:07)
> > > > > > > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> > > > > > > > 
> > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > > > > > 
> > > > > > > > ---
> > > > > > > > Changes in v7:
> > > > > > > > - make offsets_ default to zero-matrices as opposed to identity matrices
> > > > > > > > - checkstyle
> > > > > > > > - populate metadata
> > > > > > > >   - add ccm to IPAFrameContext
> > > > > > > > - don't update the ccm if the color temperature didn't change
> > > > > > > > 
> > > > > > > > No change in v6
> > > > > > > > 
> > > > > > > > Changes in v5:
> > > > > > > > - clean up documentation
> > > > > > > > - coalesce parseYaml into init
> > > > > > > > 
> > > > > > > > Changes in v4:
> > > > > > > > - remove stray semicolons
> > > > > > > > - use the new matrix interpolator readYaml
> > > > > > > > - use the new matrix operator[] getter
> > > > > > > > 
> > > > > > > > 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     | 140 ++++++++++++++++++++++++++
> > > > > > > >  src/ipa/rkisp1/algorithms/ccm.h       |  50 +++++++++
> > > > > > > >  src/ipa/rkisp1/algorithms/meson.build |   1 +
> > > > > > > >  src/ipa/rkisp1/ipa_context.h          |   5 +
> > > > > > > >  4 files changed, 196 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 000000000000..09fe4b2aa1bc
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > > > > > > @@ -0,0 +1,140 @@
> > > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > > > +/*
> > > > > > > > + * Copyright (C) 2024, Ideas On Board
> > > > > > > > + *
> > > > > > > > + * RkISP1 Color Correction Matrix 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/control_ids.h>
> > > > > > > > +
> > > > > > > > +#include <libcamera/ipa/core_ipa_interface.h>
> > > > > > > > +
> > > > > > > > +#include "libcamera/internal/yaml_parser.h"
> > > > > > > > +
> > > > > > > > +#include "../utils.h"
> > > > > > > > +#include "libipa/matrix_interpolator.h"
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \file ccm.h
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +namespace libcamera {
> > > > > > > > +
> > > > > > > > +namespace ipa::rkisp1::algorithms {
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \class Ccm
> > > > > > > > + * \brief A color correction matrix algorithm
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \copydoc libcamera::ipa::Algorithm::init
> > > > > > > > + */
> > > > > > > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > > > > > > > +{
> > > > > > > > +       int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> > > > > > > > +       if (ret < 0) {
> > > > > > > > +               LOG(RkISP1Ccm, Warning)
> > > > > > > > +                       << "Failed to parse 'ccm' "
> > > > > > > > +                       << "parameter from tuning file; falling back to unit matrix";
> > > > > > > > +               ccm_.reset();
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > > > > > > > +       if (ret < 0) {
> > > > > > > > +               LOG(RkISP1Ccm, Warning)
> > > > > > > > +                       << "Failed to parse 'offsets' "
> > > > > > > > +                       << "parameter from tuning file; falling back to zero offsets";
> > > > > > > > +               /*
> > > > > > > > +                * MatrixInterpolator::reset() resets to identity matrices
> > > > > > > > +                * while here we need zero matrices so we need to construct it
> > > > > > > > +                * ourselves.
> > > > > > > > +                */
> > > > > > > > +               Matrix<int16_t, 3, 1> m({ 0, 0, 0 });
> > > > > > > > +               std::map<unsigned int, Matrix<int16_t, 3, 1>> matrices = { { 0, m } };
> > > > > > > > +               offsets_ = MatrixInterpolator<int16_t, 3, 1>(matrices);
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       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[i][j]);
> > > > > > 
> > > > > > Curly braces for outer loop.
> > > > > > 
> > > > > > > > +
> > > > > > > > +       for (unsigned int i = 0; i < 3; i++)
> > > > > > > > +               config.ct_offset[i] = offsets[i][0] & 0xfff;
> > > > > > > > +
> > > > > > > > +       LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> > > > > > > > +       LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets;
> > > > > > > > +
> > > > > > > > +       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,
> > > > > > > > +                 IPAFrameContext &frameContext,
> > > > > > > > +                 rkisp1_params_cfg *params)
> > > > > > > > +{
> > > > > > > > +       uint32_t ct = context.activeState.awb.temperatureK;
> > > > > > > > +       if (ct == ct_)
> > > > > > 
> > > > > > ct_ should be initialized/reset in configure(), to ensure that we'll
> > > > > > always set the CCM matrix on the first frame.
> > > > > > 
> > > > > > I expect temperatureK to be fairly noisy, should we add a threshold to
> > > > > > the comparison to avoid updating the CCM parameters on every frame in
> > > > > > practice ?
> > > > > > 
> > > > > > > > +               return;
> > > > > > > 
> > > > > > > Interestingly, I think this is fine (certainly for now) ... but this
> > > > > > > means that the output request metadata will only write the CCM if it
> > > > > > > changes.
> > > > > 
> > > > > There was discussion before about "do we need to always write *all*
> > > > > metadata" and iirc the conclusion was no so...
> > > > 
> > > > I'm sure we'll revisit the topic at some point.
> > > > 
> > > > > > > I think that's actually 'fine' if we imply that any frame that doesn't
> > > > > > > supply metadata is the same as the previous state. Though I think RPi
> > > > > > > currently reports it for every frame regardless.
> > > > > > > 
> > > > > > > I'd be curious to see how Stefan's tooling handles this.
> > > > > > > 
> > > > > > > We can always update on top, and my comments are addressed so:
> > > > > > > 
> > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > > > 
> > > > > > > > +
> > > > > > > > +       ct_ = ct;
> > > > > > > > +       Matrix<double, 3, 3> ccm = ccm_.get(ct);
> > > > > > > > +       Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> > > > > > > > +
> > > > > > > > +       frameContext.ccm.ccm = ccm;
> > > > > > > > +
> > > > > > > > +       setParameters(params, ccm, offsets);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \copydoc libcamera::ipa::Algorithm::process
> > > > > > > > + */
> > > > > > > > +void process([[maybe_unused]] IPAContext &context,
> > > > > > > > +            [[maybe_unused]] const uint32_t frame,
> > > > > > > > +            IPAFrameContext &frameContext,
> > > > > > > > +            [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > > > > > +            ControlList &metadata)
> > > > > > > > +{
> > > > > > > > +       float m[9];
> > > > > > > > +       for (unsigned int i = 0; i < 3; i++)
> > > > > > > > +               for (unsigned int j = 0; j < 3; j++)
> > > > > > > > +                       m[i] = frameContext.ccm.ccm[i][j];
> > > > > > 
> > > > > > Curly braces for outer loop.
> > > > > > 
> > > > > > > > +       metadata.set(controls::ColourCorrectionMatrix, m);
> > > > > > 
> > > > > > It's interesting we're reporting the matrix but not the offsets. Not a
> > > > > > blocker for this series, but should this be addressed ?
> > > > > 
> > > > > I wasn't sure if there was demand for reporting the offsets...
> > > > > 
> > > > > And actually I remember a link [1] that Stefan sent me a long time ago
> > > > > that said something about them costing more computation and rarely
> > > > > improving results...?
> > > > 
> > > > That's what I was wondering too. I wonder if we should drop the offset.
> > > 
> > > I think they should be there to have the /option/ of being set.
> > 
> > It costs a little bit of CPU time. Most likely not the end of the world,
> > but things do add up. I suppose we'll get more visibility once we have a
> 
> Yes, but if we don't have the ability to control we won't be able to
> experiement.  I'd rather keep this in. The cost is low. Unless we're
> really worried about the cost of 3 integers...

It's more the CPU cost that we should try to save. We can revisit this
later after experimenting with the tuning.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > tuning tool in place to produce the data. If the tool hardcodes offsets
> > to 0, I'd rather drop them from the implementation.
> > 
> > > > > [1] https://www.imatest.com/docs/colormatrix/
> > > > > 
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +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 000000000000..09a6801626b4
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > > > > > > > @@ -0,0 +1,50 @@
> > > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > > > +/*
> > > > > > > > + * Copyright (C) 2024, Ideas On Board
> > > > > > > > + *
> > > > > > > > + * RkISP1 Color Correction Matrix 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;
> > > > > > > > +       void process([[maybe_unused]] IPAContext &context,
> > > > > > 
> > > > > > [[maybe_unused]] is not needed in the function declaration.
> > > > > > 
> > > > > > > > +                    [[maybe_unused]] const uint32_t frame,
> > > > > > > > +                    IPAFrameContext &frameContext,
> > > > > > > > +                    [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > > > > > +                    ControlList &metadata) 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);
> > > > > > > > +
> > > > > > > > +       unsigned int ct_;
> > > > > > > > +       MatrixInterpolator<double, 3, 3> ccm_;
> > > > > > 
> > > > > > Given that the hardware representation is a 4.7 fixed point, would it be
> > > > > > enough to use floats instead of doubles (here and in the frame context)
> > > > > > ?
> > > > > > 
> > > > > > > > +       MatrixInterpolator<int16_t, 3, 1> offsets_;
> > > > > > 
> > > > > > Why is the matrix stored in floating point values and the offsets as
> > > > > > integers ?
> > > > > 
> > > > > Because the offsets are integers in hardware as well while the ccm is
> > > > > fixed-point.
> > > > > 
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +} /* namespace ipa::rkisp1::algorithms */
> > > > > > > > +
> > > > > > > > +} /* namespace libcamera */
> > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > > > > > > > index 6ee71a9b5da3..1734a6675f78 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',
> > > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > > > > index 2a994d81ae41..cfb1f9770870 100644
> > > > > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > > > > @@ -16,6 +16,7 @@
> > > > > > > >  #include <libcamera/geometry.h>
> > > > > > > >  
> > > > > > > >  #include <libipa/fc_queue.h>
> > > > > > > > +#include <libipa/matrix.h>
> > > > > > > >  
> > > > > > > >  namespace libcamera {
> > > > > > > >  
> > > > > > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {
> > > > > > > >                 uint32_t exposure;
> > > > > > > >                 double gain;
> > > > > > > >         } sensor;
> > > > > > > > +
> > > > > > > > +       struct {
> > > > > > > > +               Matrix<double, 3, 3> ccm;
> > > > > > > > +       } ccm;
> > > > > > > >  };
> > > > > > > >  
> > > > > > > >  struct IPAContext {
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart