[v3,2/2] ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls
diff mbox series

Message ID 20251010113332.3030598-3-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • Fix ControlSerializer deserializing array controls
Related show

Commit Message

Paul Elder Oct. 10, 2025, 11:33 a.m. UTC 
The ControlInfos of non-scalar controls that are reported in controls()
must have 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>
---
No change in v3

No change in v2
---
 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(-)

Comments

Barnabás Pőcze Oct. 10, 2025, 12:44 p.m. UTC | #1 
Hi

2025. 10. 10. 13:33 keltezéssel, Paul Elder írta:
> The ControlInfos of non-scalar controls that are reported in controls()
> must have 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.

Sorry, maybe I am missing something, but doesn't the previous change fix that?
If so, then I think the justification should mention that the goal is (at least
as far as I am aware) that applications are able to take the default value of a
control (if there is any), and set it "as is". Which implies that array-like controls
need array-like default values with the same type (and same size if fixed size).


Regards,
Barnabás Pőcze


> 
> Fix the scalarness of these controls where relevant.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> No change in v3
> 
> No change in v2
> ---
>   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),
Paul Elder Oct. 11, 2025, 5:01 a.m. UTC | #2 
Quoting Barnabás Pőcze (2025-10-10 21:44:52)
> Hi
> 
> 2025. 10. 10. 13:33 keltezéssel, Paul Elder írta:
> > The ControlInfos of non-scalar controls that are reported in controls()
> > must have 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.
> 
> Sorry, maybe I am missing something, but doesn't the previous change fix that?

Right, it does.

> If so, then I think the justification should mention that the goal is (at least
> as far as I am aware) that applications are able to take the default value of a
> control (if there is any), and set it "as is". Which implies that array-like controls
> need array-like default values with the same type (and same size if fixed size).

Yes that is right.


Thanks,

Paul

> 
> 
> Regards,
> Barnabás Pőcze
> 
> 
> > 
> > Fix the scalarness of these controls where relevant.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> > No change in v3
> > 
> > No change in v2
> > ---
> >   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),
>

Patch
diff mbox series 

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),