[v2,12/13] ipa: rkisp1: cproc: Provide a Hue control

24900 diff mbox series

The RKISP1 supports a configurable Hue as part of the colour processing
unit (cproc).

This is implemented as a phase shift of the chrominance values between
-90 and +87.188 degrees according to the datasheet however the type
itself would imply that this is a range between -90 and 89.2969.

Implement the new control converting to the hardware scale accordingly
and report the applied control in the completed request metadata.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/cproc.cpp | 18 ++++++++++++++++++
 src/ipa/rkisp1/ipa_context.h        |  3 +++
 2 files changed, 21 insertions(+)

Quoting Kieran Bingham (2025-10-29 17:24:37)
> The RKISP1 supports a configurable Hue as part of the colour processing
> unit (cproc).
> 
> This is implemented as a phase shift of the chrominance values between
> -90 and +87.188 degrees according to the datasheet however the type
> itself would imply that this is a range between -90 and 89.2969.
> 
> Implement the new control converting to the hardware scale accordingly
> and report the applied control in the completed request metadata.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/cproc.cpp | 18 ++++++++++++++++++
>  src/ipa/rkisp1/ipa_context.h        |  3 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index 7475e6c1b609..5389882c1685 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -39,6 +39,7 @@ namespace {
>  
>  constexpr float kDefaultBrightness = 0.0f;
>  constexpr float kDefaultContrast = 1.0f;
> +constexpr float kDefaultHue = 0.0f;
>  constexpr float kDefaultSaturation = 1.0f;
>  
>  } /* namespace */
> @@ -53,6 +54,8 @@ int ColorProcessing::init(IPAContext &context,
>  
>         cmap[&controls::Brightness] = ControlInfo(-1.0f, 0.993f, kDefaultBrightness);
>         cmap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, kDefaultContrast);
> +       cmap[&controls::Hue] = ControlInfo(HueQ::TraitsType::min,
> +                                          HueQ::TraitsType::max, kDefaultHue);

I've only used this style on the Hue addition because I want to know if
it is more generally preferred to pass through the type min/max or
define them like in Brightness/Contrast/Saturation.

I think setting the control info from the range of the HueQ is quite
good - but then obfuscates the values here in the code a little. Still
they could be commented with the expected values perhaps.

I also wonder about adding

 <typename Traits>
 struct Quantized {
+   Traits::quantized_type min = Traits::min;
+   Traist::quantized_type max = Traits::max;
 ...
 }

But that would impose / force all Traits to set / define a min/max - and
I'm not sure if we would want to do that for gain code conversions
(generically). Maybe we could ... but I think we read min/max from the
driver so we couldn't do that constexpr.


>         cmap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, kDefaultSaturation);
>  
>         return 0;
> @@ -68,6 +71,7 @@ int ColorProcessing::configure(IPAContext &context,
>  
>         cproc.brightness = BrightnessQ(kDefaultBrightness);
>         cproc.contrast = ContrastQ(kDefaultContrast);
> +       cproc.hue = HueQ(kDefaultHue);
>         cproc.saturation = SaturationQ(kDefaultSaturation);
>  
>         return 0;
> @@ -109,6 +113,17 @@ void ColorProcessing::queueRequest(IPAContext &context,
>                 LOG(RkISP1CProc, Debug) << "Set contrast to " << value.value();
>         }
>  
> +       const auto &hue = controls.get(controls::Hue);
> +       if (hue) {
> +               HueQ value = *hue;
> +               if (cproc.hue != value) {
> +                       cproc.hue = value;
> +                       update = true;
> +               }
> +
> +               LOG(RkISP1CProc, Debug) << "Set hue to " << value.value();
> +       }
> +
>         const auto saturation = controls.get(controls::Saturation);
>         if (saturation) {
>                 SaturationQ value = *saturation;
> @@ -122,6 +137,7 @@ void ColorProcessing::queueRequest(IPAContext &context,
>  
>         frameContext.cproc.brightness = cproc.brightness;
>         frameContext.cproc.contrast = cproc.contrast;
> +       frameContext.cproc.hue = cproc.hue;
>         frameContext.cproc.saturation = cproc.saturation;
>         frameContext.cproc.update = update;
>  }
> @@ -142,6 +158,7 @@ void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
>         config.setEnabled(true);
>         config->brightness = frameContext.cproc.brightness.quantized();
>         config->contrast = frameContext.cproc.contrast.quantized();
> +       config->hue = frameContext.cproc.hue.quantized();
>         config->sat = frameContext.cproc.saturation.quantized();
>  }
>  
> @@ -154,6 +171,7 @@ void ColorProcessing::process([[maybe_unused]] IPAContext &context, [[maybe_unus
>  {
>         metadata.set(controls::Brightness, frameContext.cproc.brightness.value());
>         metadata.set(controls::Contrast, frameContext.cproc.contrast.value());
> +       metadata.set(controls::Hue, frameContext.cproc.hue.value());
>         metadata.set(controls::Saturation, frameContext.cproc.saturation.value());
>  }
>  
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 0a5fb2769d31..a4badbfc0c36 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -37,6 +37,7 @@ namespace ipa::rkisp1 {
>  /* Fixed point types used by CPROC */
>  using BrightnessQ = Q1_7;
>  using ContrastQ = UQ1_7;
> +using HueQ = Quantized<ScaledFixedPointQTraits<Q1_7::TraitsType, 90>>;
>  using SaturationQ = UQ1_7;
>  
>  struct IPAHwSettings {
> @@ -124,6 +125,7 @@ struct IPAActiveState {
>         struct {
>                 BrightnessQ brightness;
>                 ContrastQ contrast;
> +               HueQ hue;
>                 SaturationQ saturation;
>         } cproc;
>  
> @@ -178,6 +180,7 @@ struct IPAFrameContext : public FrameContext {
>         struct {
>                 BrightnessQ brightness;
>                 ContrastQ contrast;
> +               HueQ hue;
>                 SaturationQ saturation;
>  
>                 bool update;
> -- 
> 2.50.1
>