[v2,2/2] ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls

24567 diff mbox series

The ControlInfos of non-scalar controls that are reported in controls()
must have scalar min/max and non-scalar default values for controls that
have a defined size. Currently this is relevant to the following
controls:
- ColourGains
- ColourCorrectionMatrix
- FrameDurationLimits

A mismatch of scalarness in these fields causes deserialization errors
in ControlSerializer which manifest when IPAs are run in isolation.

Fix the scalarness of these controls where relevant.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp             | 4 +++-
 src/ipa/mali-c55/mali-c55.cpp     | 5 ++++-
 src/ipa/rkisp1/algorithms/awb.cpp | 5 ++++-
 src/ipa/rkisp1/rkisp1.cpp         | 4 +++-
 src/ipa/rpi/common/ipa_base.cpp   | 7 ++++++-
 5 files changed, 20 insertions(+), 5 deletions(-)

Hi me,

Quoting Paul Elder (2025-10-07 19:27:44)
> The ControlInfos of non-scalar controls that are reported in controls()
> must have scalar min/max and non-scalar default values for controls that

min/max don't have this restriction; only the default value has this
restriction. The code is fine though, it's just the commit message.

We should upgrade the documentation (where?) though to mention that min/max can
be scalar or non-scalar depending on what makes sense for the control, and that
min/max are not designed to be set as a control value directly whereas default is
designed to be able to be set directly.


Paul

> have a defined size. Currently this is relevant to the following
> controls:
> - ColourGains
> - ColourCorrectionMatrix
> - FrameDurationLimits
> 
> A mismatch of scalarness in these fields causes deserialization errors
> in ControlSerializer which manifest when IPAs are run in isolation.
> 
> Fix the scalarness of these controls where relevant.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp             | 4 +++-
>  src/ipa/mali-c55/mali-c55.cpp     | 5 ++++-
>  src/ipa/rkisp1/algorithms/awb.cpp | 5 ++++-
>  src/ipa/rkisp1/rkisp1.cpp         | 4 +++-
>  src/ipa/rpi/common/ipa_base.cpp   | 7 ++++++-
>  5 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 1cae08bf255f..e71639a16522 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -20,6 +20,7 @@
>  
>  #include <libcamera/base/file.h>
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/span.h>
>  #include <libcamera/base/utils.h>
>  
>  #include <libcamera/control_ids.h>
> @@ -280,10 +281,11 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>                 uint64_t frameSize = lineLength * frameHeights[i];
>                 frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
>         }
> +       std::array<int64_t, 2> defFrameDurations = { frameDurations[2], frameDurations[2] };
>  
>         controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
>                                                                frameDurations[1],
> -                                                              frameDurations[2]);
> +                                                              Span<const int64_t, 2>{ defFrameDurations });
>  
>         controls.merge(context_.ctrlMap);
>         *ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp
> index 7d45e7310aec..c63d3b2bb7be 100644
> --- a/src/ipa/mali-c55/mali-c55.cpp
> +++ b/src/ipa/mali-c55/mali-c55.cpp
> @@ -5,6 +5,7 @@
>   * Mali-C55 ISP image processing algorithms
>   */
>  
> +#include <array>
>  #include <map>
>  #include <string.h>
>  #include <vector>
> @@ -14,6 +15,7 @@
>  
>  #include <libcamera/base/file.h>
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/span.h>
>  
>  #include <libcamera/control_ids.h>
>  #include <libcamera/ipa/ipa_interface.h>
> @@ -234,9 +236,10 @@ void IPAMaliC55::updateControls(const IPACameraSensorInfo &sensorInfo,
>                 frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
>         }
>  
> +       std::array<int64_t, 2> defFrameDurations = { frameDurations[2], frameDurations[2] };
>         ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
>                                                               frameDurations[1],
> -                                                             frameDurations[2]);
> +                                                             Span<const int64_t, 2>{ defFrameDurations });
>  
>         /*
>          * Compute exposure time limits from the V4L2_CID_EXPOSURE control
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 399fb51be414..a664011a9f0d 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -91,7 +91,10 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)
>                                                          kMaxColourTemperature,
>                                                          kDefaultColourTemperature);
>         cmap[&controls::AwbEnable] = ControlInfo(false, true);
> -       cmap[&controls::ColourGains] = ControlInfo(0.0f, 3.996f, 1.0f);
> +
> +       std::array<float, 2> defColourGains = { 1.0f, 1.0f };
> +       cmap[&controls::ColourGains] = ControlInfo(0.0f, 3.996f,
> +                                                  Span<const float, 2>{ defColourGains });
>  
>         if (!tuningData.contains("algorithm"))
>                 LOG(RkISP1Awb, Info) << "No AWB algorithm specified."
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index fa22bfc34904..3f098610a06a 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -436,10 +436,12 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>                 uint64_t frameSize = lineLength * frameHeights[i];
>                 frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
>         }
> +       std::array<int64_t, 2> defFrameDurations = { frameDurations[2], frameDurations[2] };
>  
>         /* \todo Move this (and other agc-related controls) to agc */
>         context_.ctrlMap[&controls::FrameDurationLimits] =
> -               ControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]);
> +               ControlInfo(frameDurations[0], frameDurations[1],
> +                           ControlValue(Span<const int64_t, 2>{ defFrameDurations }));
>  
>         ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());
>         *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 8dfe35cc3267..67dab6cd970c 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -7,6 +7,7 @@
>  
>  #include "ipa_base.h"
>  
> +#include <array>
>  #include <cmath>
>  
>  #include <libcamera/base/log.h>
> @@ -243,10 +244,14 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa
>          * based on the current sensor mode.
>          */
>         ControlInfoMap::Map ctrlMap = ipaControls;
> +       std::array<int64_t, 2> defFrameDurations = {
> +               static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()),
> +               static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>())
> +       };
>         ctrlMap[&controls::FrameDurationLimits] =
>                 ControlInfo(static_cast<int64_t>(mode_.minFrameDuration.get<std::micro>()),
>                             static_cast<int64_t>(mode_.maxFrameDuration.get<std::micro>()),
> -                           static_cast<int64_t>(defaultMinFrameDuration.get<std::micro>()));
> +                           Span<const int64_t, 2>{ defFrameDurations });
>  
>         ctrlMap[&controls::AnalogueGain] =
>                 ControlInfo(static_cast<float>(mode_.minAnalogueGain),
> -- 
> 2.47.2
>