[v2,11/17] ipa: rkisp1: Implement manual ColourCorrectionMatrix control
diff mbox series

Message ID 20250319161152.63625-12-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • Some rkisp1 awb improvements
Related show

Commit Message

Stefan Klug March 19, 2025, 4:11 p.m. UTC 
Add a manual ColourCorrectionMatrix control. This was already discussed
while implementing manual colour temperature but was never implemented.
The control allows to manually specify the CCM when AwbEnable is false.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v2:
- None
---
 src/ipa/rkisp1/algorithms/ccm.cpp | 59 ++++++++++++++++++++++++++++---
 src/ipa/rkisp1/algorithms/ccm.h   |  6 ++++
 src/ipa/rkisp1/ipa_context.h      |  3 +-
 3 files changed, 62 insertions(+), 6 deletions(-)

Comments

Kieran Bingham March 31, 2025, 5:39 p.m. UTC | #1 
Quoting Stefan Klug (2025-03-19 16:11:16)
> Add a manual ColourCorrectionMatrix control. This was already discussed
> while implementing manual colour temperature but was never implemented.
> The control allows to manually specify the CCM when AwbEnable is false.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - None
> ---
>  src/ipa/rkisp1/algorithms/ccm.cpp | 59 ++++++++++++++++++++++++++++---
>  src/ipa/rkisp1/algorithms/ccm.h   |  6 ++++
>  src/ipa/rkisp1/ipa_context.h      |  3 +-
>  3 files changed, 62 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> index 2e5e91006b55..303ac3dd2fe2 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -36,17 +36,25 @@ namespace ipa::rkisp1::algorithms {
>  
>  LOG_DEFINE_CATEGORY(RkISP1Ccm)
>  
> +constexpr Matrix<float, 3, 3> kIdentity3x3 = Matrix<float, 3, 3>::identity();
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::init
>   */
>  int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
>  {
> +       auto &cmap = context.ctrlMap;
> +       cmap[&controls::ColourCorrectionMatrix] = ControlInfo(
> +               ControlValue(-8.0f),
> +               ControlValue(7.993f),
> +               ControlValue(kIdentity3x3.data()));
> +
>         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_.setData({ { 0, Matrix<float, 3, 3>::identity() } });
> +               ccm_.setData({ { 0, kIdentity3x3 } });
>         }
>  
>         ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> @@ -61,13 +69,48 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
>         return 0;
>  }
>  
> +/**
> + * \copydoc libcamera::ipa::Algorithm::configure
> + */
> +int Ccm::configure(IPAContext &context,
> +                  [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> +{
> +       auto &as = context.activeState;
> +       as.ccm.manual = kIdentity3x3;
> +       as.ccm.automatic = ccm_.getInterpolated(as.awb.automatic.temperatureK);
> +       LOG(RkISP1Ccm, Debug) << "init matrix " << as.ccm.manual;

I'm not sure that debug line adds much value...

But aside from that, it seems to match the control documentation.


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


> +       return 0;
> +}
> +
> +void Ccm::queueRequest(IPAContext &context,
> +                      [[maybe_unused]] const uint32_t frame,
> +                      IPAFrameContext &frameContext,
> +                      const ControlList &controls)
> +{
> +       /* Nothing to do here, the ccm will be calculated in prepare() */
> +       if (frameContext.awb.autoEnabled)
> +               return;
> +
> +       auto &ccm = context.activeState.ccm;
> +
> +       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> +       const auto &ccmMatrix = controls.get(controls::ColourCorrectionMatrix);
> +       if (ccmMatrix)
> +               ccm.manual = Matrix<float, 3, 3>(*ccmMatrix);
> +       else if (colourTemperature)
> +               ccm.manual = ccm_.getInterpolated(*colourTemperature);
> +
> +       LOG(RkISP1Ccm, Debug) << "queueRequest matrix " << ccm.manual;
> +       frameContext.ccm.ccm = ccm.manual;
> +}
> +
>  void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
>                         const Matrix<float, 3, 3> &matrix,
>                         const Matrix<int16_t, 3, 1> &offsets)
>  {
>         /*
>          * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to
> -        * +7.992 (0x3ff)
> +        * +7.9921875 (0x3ff)
>          */
>         for (unsigned int i = 0; i < 3; i++) {
>                 for (unsigned int j = 0; j < 3; j++)
> @@ -88,14 +131,20 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
>  void Ccm::prepare(IPAContext &context, const uint32_t frame,
>                   IPAFrameContext &frameContext, RkISP1Params *params)
>  {
> -       uint32_t ct = frameContext.awb.temperatureK;
> +       if (!frameContext.awb.autoEnabled) {
> +               auto config = params->block<BlockType::Ctk>();
> +               config.setEnabled(true);
> +               setParameters(*config, frameContext.ccm.ccm, Matrix<int16_t, 3, 1>());
> +               return;
> +       }
>  
> +       uint32_t ct = frameContext.awb.temperatureK;
>         /*
>          * \todo The colour temperature will likely be noisy, add filtering to
>          * avoid updating the CCM matrix all the time.
>          */
>         if (frame > 0 && ct == ct_) {
> -               frameContext.ccm.ccm = context.activeState.ccm.ccm;
> +               frameContext.ccm.ccm = context.activeState.ccm.automatic;
>                 return;
>         }
>  
> @@ -103,7 +152,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>         Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct);
>         Matrix<int16_t, 3, 1> offsets = offsets_.getInterpolated(ct);
>  
> -       context.activeState.ccm.ccm = ccm;
> +       context.activeState.ccm.automatic = ccm;
>         frameContext.ccm.ccm = ccm;
>  
>         auto config = params->block<BlockType::Ctk>();
> diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h
> index a5d9a9a45e5d..c301e6e531c8 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.h
> +++ b/src/ipa/rkisp1/algorithms/ccm.h
> @@ -26,6 +26,12 @@ public:
>         ~Ccm() = default;
>  
>         int init(IPAContext &context, const YamlObject &tuningData) override;
> +       int configure(IPAContext &context,
> +                     const IPACameraSensorInfo &configInfo) override;
> +       void queueRequest(IPAContext &context,
> +                         const uint32_t frame,
> +                         IPAFrameContext &frameContext,
> +                         const ControlList &controls) override;
>         void prepare(IPAContext &context, const uint32_t frame,
>                      IPAFrameContext &frameContext,
>                      RkISP1Params *params) override;
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 769e9f114e23..f0d504215d34 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -101,7 +101,8 @@ struct IPAActiveState {
>         } awb;
>  
>         struct {
> -               Matrix<float, 3, 3> ccm;
> +               Matrix<float, 3, 3> manual;
> +               Matrix<float, 3, 3> automatic;
>         } ccm;
>  
>         struct {
> -- 
> 2.43.0
>
Laurent Pinchart April 1, 2025, 9:30 p.m. UTC | #2 
On Mon, Mar 31, 2025 at 06:39:48PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2025-03-19 16:11:16)
> > Add a manual ColourCorrectionMatrix control. This was already discussed
> > while implementing manual colour temperature but was never implemented.
> > The control allows to manually specify the CCM when AwbEnable is false.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - None
> > ---
> >  src/ipa/rkisp1/algorithms/ccm.cpp | 59 ++++++++++++++++++++++++++++---
> >  src/ipa/rkisp1/algorithms/ccm.h   |  6 ++++
> >  src/ipa/rkisp1/ipa_context.h      |  3 +-
> >  3 files changed, 62 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> > index 2e5e91006b55..303ac3dd2fe2 100644
> > --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > @@ -36,17 +36,25 @@ namespace ipa::rkisp1::algorithms {
> >  
> >  LOG_DEFINE_CATEGORY(RkISP1Ccm)
> >  
> > +constexpr Matrix<float, 3, 3> kIdentity3x3 = Matrix<float, 3, 3>::identity();
> > +
> >  /**
> >   * \copydoc libcamera::ipa::Algorithm::init
> >   */
> >  int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> >  {
> > +       auto &cmap = context.ctrlMap;
> > +       cmap[&controls::ColourCorrectionMatrix] = ControlInfo(
> > +               ControlValue(-8.0f),
> > +               ControlValue(7.993f),
> > +               ControlValue(kIdentity3x3.data()));
> > +
> >         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_.setData({ { 0, Matrix<float, 3, 3>::identity() } });
> > +               ccm_.setData({ { 0, kIdentity3x3 } });
> >         }
> >  
> >         ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > @@ -61,13 +69,48 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
> >         return 0;
> >  }
> >  
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::configure
> > + */
> > +int Ccm::configure(IPAContext &context,
> > +                  [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> > +{
> > +       auto &as = context.activeState;
> > +       as.ccm.manual = kIdentity3x3;
> > +       as.ccm.automatic = ccm_.getInterpolated(as.awb.automatic.temperatureK);
> > +       LOG(RkISP1Ccm, Debug) << "init matrix " << as.ccm.manual;
> 
> I'm not sure that debug line adds much value...
> 
> But aside from that, it seems to match the control documentation.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> > +       return 0;
> > +}
> > +
> > +void Ccm::queueRequest(IPAContext &context,
> > +                      [[maybe_unused]] const uint32_t frame,
> > +                      IPAFrameContext &frameContext,
> > +                      const ControlList &controls)
> > +{
> > +       /* Nothing to do here, the ccm will be calculated in prepare() */
> > +       if (frameContext.awb.autoEnabled)
> > +               return;
> > +
> > +       auto &ccm = context.activeState.ccm;
> > +
> > +       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > +       const auto &ccmMatrix = controls.get(controls::ColourCorrectionMatrix);
> > +       if (ccmMatrix)
> > +               ccm.manual = Matrix<float, 3, 3>(*ccmMatrix);
> > +       else if (colourTemperature)
> > +               ccm.manual = ccm_.getInterpolated(*colourTemperature);
> > +
> > +       LOG(RkISP1Ccm, Debug) << "queueRequest matrix " << ccm.manual;

To make the log less noisy, should this be restricted to when a CT or
CCM matrix control was set ? Or maybe also indicate where the CCM came
from ?

	if (ccmMatrix) {
		ccm.manual = Matrix<float, 3, 3>(*ccmMatrix);
 		LOG(RkISP1Ccm, Debug)
			<< "Setting manual CCM from CCM control to " << ccm.manual;
	} else if (colourTemperature) {
		ccm.manual = ccm_.getInterpolated(*colourTemperature);
 		LOG(RkISP1Ccm, Debug)
			<< "Setting manual CCM from CT control to " << ccm.manual;
	}

or something similar.

> > +       frameContext.ccm.ccm = ccm.manual;
> > +}
> > +
> >  void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
> >                         const Matrix<float, 3, 3> &matrix,
> >                         const Matrix<int16_t, 3, 1> &offsets)
> >  {
> >         /*
> >          * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to
> > -        * +7.992 (0x3ff)
> > +        * +7.9921875 (0x3ff)

Someone likes precision :-)

> >          */
> >         for (unsigned int i = 0; i < 3; i++) {
> >                 for (unsigned int j = 0; j < 3; j++)
> > @@ -88,14 +131,20 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
> >  void Ccm::prepare(IPAContext &context, const uint32_t frame,
> >                   IPAFrameContext &frameContext, RkISP1Params *params)
> >  {
> > -       uint32_t ct = frameContext.awb.temperatureK;
> > +       if (!frameContext.awb.autoEnabled) {
> > +               auto config = params->block<BlockType::Ctk>();
> > +               config.setEnabled(true);
> > +               setParameters(*config, frameContext.ccm.ccm, Matrix<int16_t, 3, 1>());
> > +               return;
> > +       }
> >  
> > +       uint32_t ct = frameContext.awb.temperatureK;
> >         /*
> >          * \todo The colour temperature will likely be noisy, add filtering to
> >          * avoid updating the CCM matrix all the time.
> >          */
> >         if (frame > 0 && ct == ct_) {
> > -               frameContext.ccm.ccm = context.activeState.ccm.ccm;
> > +               frameContext.ccm.ccm = context.activeState.ccm.automatic;
> >                 return;
> >         }
> >  
> > @@ -103,7 +152,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
> >         Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct);
> >         Matrix<int16_t, 3, 1> offsets = offsets_.getInterpolated(ct);
> >  
> > -       context.activeState.ccm.ccm = ccm;
> > +       context.activeState.ccm.automatic = ccm;
> >         frameContext.ccm.ccm = ccm;
> >  
> >         auto config = params->block<BlockType::Ctk>();
> > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h
> > index a5d9a9a45e5d..c301e6e531c8 100644
> > --- a/src/ipa/rkisp1/algorithms/ccm.h
> > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > @@ -26,6 +26,12 @@ public:
> >         ~Ccm() = default;
> >  
> >         int init(IPAContext &context, const YamlObject &tuningData) override;
> > +       int configure(IPAContext &context,
> > +                     const IPACameraSensorInfo &configInfo) override;
> > +       void queueRequest(IPAContext &context,
> > +                         const uint32_t frame,
> > +                         IPAFrameContext &frameContext,
> > +                         const ControlList &controls) override;
> >         void prepare(IPAContext &context, const uint32_t frame,
> >                      IPAFrameContext &frameContext,
> >                      RkISP1Params *params) override;
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 769e9f114e23..f0d504215d34 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -101,7 +101,8 @@ struct IPAActiveState {
> >         } awb;
> >  
> >         struct {
> > -               Matrix<float, 3, 3> ccm;
> > +               Matrix<float, 3, 3> manual;
> > +               Matrix<float, 3, 3> automatic;

Missing documentation updates.

> >         } ccm;
> >  
> >         struct {
Stefan Klug April 2, 2025, 2:45 p.m. UTC | #3 
Hi Laurent,

Thank you for the review. 

On Wed, Apr 02, 2025 at 12:30:05AM +0300, Laurent Pinchart wrote:
> On Mon, Mar 31, 2025 at 06:39:48PM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2025-03-19 16:11:16)
> > > Add a manual ColourCorrectionMatrix control. This was already discussed
> > > while implementing manual colour temperature but was never implemented.
> > > The control allows to manually specify the CCM when AwbEnable is false.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Changes in v2:
> > > - None
> > > ---
> > >  src/ipa/rkisp1/algorithms/ccm.cpp | 59 ++++++++++++++++++++++++++++---
> > >  src/ipa/rkisp1/algorithms/ccm.h   |  6 ++++
> > >  src/ipa/rkisp1/ipa_context.h      |  3 +-
> > >  3 files changed, 62 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > index 2e5e91006b55..303ac3dd2fe2 100644
> > > --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > @@ -36,17 +36,25 @@ namespace ipa::rkisp1::algorithms {
> > >  
> > >  LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > >  
> > > +constexpr Matrix<float, 3, 3> kIdentity3x3 = Matrix<float, 3, 3>::identity();
> > > +
> > >  /**
> > >   * \copydoc libcamera::ipa::Algorithm::init
> > >   */
> > >  int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > >  {
> > > +       auto &cmap = context.ctrlMap;
> > > +       cmap[&controls::ColourCorrectionMatrix] = ControlInfo(
> > > +               ControlValue(-8.0f),
> > > +               ControlValue(7.993f),
> > > +               ControlValue(kIdentity3x3.data()));
> > > +
> > >         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_.setData({ { 0, Matrix<float, 3, 3>::identity() } });
> > > +               ccm_.setData({ { 0, kIdentity3x3 } });
> > >         }
> > >  
> > >         ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > > @@ -61,13 +69,48 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
> > >         return 0;
> > >  }
> > >  
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::configure
> > > + */
> > > +int Ccm::configure(IPAContext &context,
> > > +                  [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> > > +{
> > > +       auto &as = context.activeState;
> > > +       as.ccm.manual = kIdentity3x3;
> > > +       as.ccm.automatic = ccm_.getInterpolated(as.awb.automatic.temperatureK);
> > > +       LOG(RkISP1Ccm, Debug) << "init matrix " << as.ccm.manual;
> > 
> > I'm not sure that debug line adds much value...
> > 
> > But aside from that, it seems to match the control documentation.
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > 
> > > +       return 0;
> > > +}
> > > +
> > > +void Ccm::queueRequest(IPAContext &context,
> > > +                      [[maybe_unused]] const uint32_t frame,
> > > +                      IPAFrameContext &frameContext,
> > > +                      const ControlList &controls)
> > > +{
> > > +       /* Nothing to do here, the ccm will be calculated in prepare() */
> > > +       if (frameContext.awb.autoEnabled)
> > > +               return;
> > > +
> > > +       auto &ccm = context.activeState.ccm;
> > > +
> > > +       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > > +       const auto &ccmMatrix = controls.get(controls::ColourCorrectionMatrix);
> > > +       if (ccmMatrix)
> > > +               ccm.manual = Matrix<float, 3, 3>(*ccmMatrix);
> > > +       else if (colourTemperature)
> > > +               ccm.manual = ccm_.getInterpolated(*colourTemperature);
> > > +
> > > +       LOG(RkISP1Ccm, Debug) << "queueRequest matrix " << ccm.manual;
> 
> To make the log less noisy, should this be restricted to when a CT or
> CCM matrix control was set ? Or maybe also indicate where the CCM came
> from ?
> 
> 	if (ccmMatrix) {
> 		ccm.manual = Matrix<float, 3, 3>(*ccmMatrix);
>  		LOG(RkISP1Ccm, Debug)
> 			<< "Setting manual CCM from CCM control to " << ccm.manual;
> 	} else if (colourTemperature) {
> 		ccm.manual = ccm_.getInterpolated(*colourTemperature);
>  		LOG(RkISP1Ccm, Debug)
> 			<< "Setting manual CCM from CT control to " << ccm.manual;
> 	}

Yes, that is better. Integrated.

> 
> or something similar.
> 
> > > +       frameContext.ccm.ccm = ccm.manual;
> > > +}
> > > +
> > >  void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
> > >                         const Matrix<float, 3, 3> &matrix,
> > >                         const Matrix<int16_t, 3, 1> &offsets)
> > >  {
> > >         /*
> > >          * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to
> > > -        * +7.992 (0x3ff)
> > > +        * +7.9921875 (0x3ff)
> 
> Someone likes precision :-)

I wanted a way to explain why I used 7.993 as control limit :-)

> 
> > >          */
> > >         for (unsigned int i = 0; i < 3; i++) {
> > >                 for (unsigned int j = 0; j < 3; j++)
> > > @@ -88,14 +131,20 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
> > >  void Ccm::prepare(IPAContext &context, const uint32_t frame,
> > >                   IPAFrameContext &frameContext, RkISP1Params *params)
> > >  {
> > > -       uint32_t ct = frameContext.awb.temperatureK;
> > > +       if (!frameContext.awb.autoEnabled) {
> > > +               auto config = params->block<BlockType::Ctk>();
> > > +               config.setEnabled(true);
> > > +               setParameters(*config, frameContext.ccm.ccm, Matrix<int16_t, 3, 1>());
> > > +               return;
> > > +       }
> > >  
> > > +       uint32_t ct = frameContext.awb.temperatureK;
> > >         /*
> > >          * \todo The colour temperature will likely be noisy, add filtering to
> > >          * avoid updating the CCM matrix all the time.
> > >          */
> > >         if (frame > 0 && ct == ct_) {
> > > -               frameContext.ccm.ccm = context.activeState.ccm.ccm;
> > > +               frameContext.ccm.ccm = context.activeState.ccm.automatic;
> > >                 return;
> > >         }
> > >  
> > > @@ -103,7 +152,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
> > >         Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct);
> > >         Matrix<int16_t, 3, 1> offsets = offsets_.getInterpolated(ct);
> > >  
> > > -       context.activeState.ccm.ccm = ccm;
> > > +       context.activeState.ccm.automatic = ccm;
> > >         frameContext.ccm.ccm = ccm;
> > >  
> > >         auto config = params->block<BlockType::Ctk>();
> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h
> > > index a5d9a9a45e5d..c301e6e531c8 100644
> > > --- a/src/ipa/rkisp1/algorithms/ccm.h
> > > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > > @@ -26,6 +26,12 @@ public:
> > >         ~Ccm() = default;
> > >  
> > >         int init(IPAContext &context, const YamlObject &tuningData) override;
> > > +       int configure(IPAContext &context,
> > > +                     const IPACameraSensorInfo &configInfo) override;
> > > +       void queueRequest(IPAContext &context,
> > > +                         const uint32_t frame,
> > > +                         IPAFrameContext &frameContext,
> > > +                         const ControlList &controls) override;
> > >         void prepare(IPAContext &context, const uint32_t frame,
> > >                      IPAFrameContext &frameContext,
> > >                      RkISP1Params *params) override;
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 769e9f114e23..f0d504215d34 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -101,7 +101,8 @@ struct IPAActiveState {
> > >         } awb;
> > >  
> > >         struct {
> > > -               Matrix<float, 3, 3> ccm;
> > > +               Matrix<float, 3, 3> manual;
> > > +               Matrix<float, 3, 3> automatic;
> 
> Missing documentation updates.

Ahrgh, we should really add rkisp1 to doxygen. But that is a bit of
pain....

Regards,
Stefan

> 
> > >         } ccm;
> > >  
> > >         struct {
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
index 2e5e91006b55..303ac3dd2fe2 100644
--- a/src/ipa/rkisp1/algorithms/ccm.cpp
+++ b/src/ipa/rkisp1/algorithms/ccm.cpp
@@ -36,17 +36,25 @@  namespace ipa::rkisp1::algorithms {
 
 LOG_DEFINE_CATEGORY(RkISP1Ccm)
 
+constexpr Matrix<float, 3, 3> kIdentity3x3 = Matrix<float, 3, 3>::identity();
+
 /**
  * \copydoc libcamera::ipa::Algorithm::init
  */
 int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
 {
+	auto &cmap = context.ctrlMap;
+	cmap[&controls::ColourCorrectionMatrix] = ControlInfo(
+		ControlValue(-8.0f),
+		ControlValue(7.993f),
+		ControlValue(kIdentity3x3.data()));
+
 	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_.setData({ { 0, Matrix<float, 3, 3>::identity() } });
+		ccm_.setData({ { 0, kIdentity3x3 } });
 	}
 
 	ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
@@ -61,13 +69,48 @@  int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
 	return 0;
 }
 
+/**
+ * \copydoc libcamera::ipa::Algorithm::configure
+ */
+int Ccm::configure(IPAContext &context,
+		   [[maybe_unused]] const IPACameraSensorInfo &configInfo)
+{
+	auto &as = context.activeState;
+	as.ccm.manual = kIdentity3x3;
+	as.ccm.automatic = ccm_.getInterpolated(as.awb.automatic.temperatureK);
+	LOG(RkISP1Ccm, Debug) << "init matrix " << as.ccm.manual;
+	return 0;
+}
+
+void Ccm::queueRequest(IPAContext &context,
+		       [[maybe_unused]] const uint32_t frame,
+		       IPAFrameContext &frameContext,
+		       const ControlList &controls)
+{
+	/* Nothing to do here, the ccm will be calculated in prepare() */
+	if (frameContext.awb.autoEnabled)
+		return;
+
+	auto &ccm = context.activeState.ccm;
+
+	const auto &colourTemperature = controls.get(controls::ColourTemperature);
+	const auto &ccmMatrix = controls.get(controls::ColourCorrectionMatrix);
+	if (ccmMatrix)
+		ccm.manual = Matrix<float, 3, 3>(*ccmMatrix);
+	else if (colourTemperature)
+		ccm.manual = ccm_.getInterpolated(*colourTemperature);
+
+	LOG(RkISP1Ccm, Debug) << "queueRequest matrix " << ccm.manual;
+	frameContext.ccm.ccm = ccm.manual;
+}
+
 void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
 			const Matrix<float, 3, 3> &matrix,
 			const Matrix<int16_t, 3, 1> &offsets)
 {
 	/*
 	 * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to
-	 * +7.992 (0x3ff)
+	 * +7.9921875 (0x3ff)
 	 */
 	for (unsigned int i = 0; i < 3; i++) {
 		for (unsigned int j = 0; j < 3; j++)
@@ -88,14 +131,20 @@  void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
 void Ccm::prepare(IPAContext &context, const uint32_t frame,
 		  IPAFrameContext &frameContext, RkISP1Params *params)
 {
-	uint32_t ct = frameContext.awb.temperatureK;
+	if (!frameContext.awb.autoEnabled) {
+		auto config = params->block<BlockType::Ctk>();
+		config.setEnabled(true);
+		setParameters(*config, frameContext.ccm.ccm, Matrix<int16_t, 3, 1>());
+		return;
+	}
 
+	uint32_t ct = frameContext.awb.temperatureK;
 	/*
 	 * \todo The colour temperature will likely be noisy, add filtering to
 	 * avoid updating the CCM matrix all the time.
 	 */
 	if (frame > 0 && ct == ct_) {
-		frameContext.ccm.ccm = context.activeState.ccm.ccm;
+		frameContext.ccm.ccm = context.activeState.ccm.automatic;
 		return;
 	}
 
@@ -103,7 +152,7 @@  void Ccm::prepare(IPAContext &context, const uint32_t frame,
 	Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct);
 	Matrix<int16_t, 3, 1> offsets = offsets_.getInterpolated(ct);
 
-	context.activeState.ccm.ccm = ccm;
+	context.activeState.ccm.automatic = ccm;
 	frameContext.ccm.ccm = ccm;
 
 	auto config = params->block<BlockType::Ctk>();
diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h
index a5d9a9a45e5d..c301e6e531c8 100644
--- a/src/ipa/rkisp1/algorithms/ccm.h
+++ b/src/ipa/rkisp1/algorithms/ccm.h
@@ -26,6 +26,12 @@  public:
 	~Ccm() = default;
 
 	int init(IPAContext &context, const YamlObject &tuningData) override;
+	int configure(IPAContext &context,
+		      const IPACameraSensorInfo &configInfo) override;
+	void queueRequest(IPAContext &context,
+			  const uint32_t frame,
+			  IPAFrameContext &frameContext,
+			  const ControlList &controls) override;
 	void prepare(IPAContext &context, const uint32_t frame,
 		     IPAFrameContext &frameContext,
 		     RkISP1Params *params) override;
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 769e9f114e23..f0d504215d34 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -101,7 +101,8 @@  struct IPAActiveState {
 	} awb;
 
 	struct {
-		Matrix<float, 3, 3> ccm;
+		Matrix<float, 3, 3> manual;
+		Matrix<float, 3, 3> automatic;
 	} ccm;
 
 	struct {