[v9,21/26] ipa: libipa: module: Add createSelfEnumeratingAlgorithm
diff mbox series

Message ID 20251217100138.82525-22-bryan.odonoghue@linaro.org
State New
Headers show
Series
  • Add GLES 2.0 GPUISP to libcamera
Related show

Commit Message

Bryan O'Donoghue Dec. 17, 2025, 10:01 a.m. UTC 
Create an algorithm without having YAML data input.

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 src/ipa/libipa/module.cpp | 34 +++++++++++++++++++++++++++++
 src/ipa/libipa/module.h   | 46 +++++++++++++++++++++++++++++----------
 2 files changed, 68 insertions(+), 12 deletions(-)

Comments

Barnabás Pőcze Dec. 17, 2025, 4:19 p.m. UTC | #1 
Hi

2025. 12. 17. 11:01 keltezéssel, Bryan O'Donoghue írta:
> Create an algorithm without having YAML data input.
> 
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   src/ipa/libipa/module.cpp | 34 +++++++++++++++++++++++++++++
>   src/ipa/libipa/module.h   | 46 +++++++++++++++++++++++++++++----------
>   2 files changed, 68 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp
> index a95dca696..8e86ca823 100644
> --- a/src/ipa/libipa/module.cpp
> +++ b/src/ipa/libipa/module.cpp
> @@ -83,6 +83,40 @@ namespace ipa {
>    * \return The list of instantiated algorithms
>    */
> 
> +/**
> + * \fn int Module::createSelfEnumeratingAlgorithm(Context &context, const std::string &name)
> + * \brief Create and initialise a self-enumerating algorithm by name
> + *
> + * This function creates an algorithm instance from the registered algorithm
> + * factories using only the algorithm name, without requiring YAML configuration
> + * data.
> + *
> + * This is useful for algorithms that don't require external configuration
> + * parameters and can self-configure or use default values.
> + *
> + * \param[in] context The IPA context to pass to the algorithm's init function
> + * \param[in] name The name of the algorithm to instantiate
> + *
> + * \return 0 on success, negative errno value on failure:
> + *         -EINVAL if the algorithm is not found in the factory registry
> + *         Other negative values if algorithm initialisation fails
> + */
> +
> +/**
> + * \fn int Module::createAlgorithmCommon(Context &context, const YamlObject &algoData, const std::string &name)
> + * \brief Common helper fucntion to allow createSelfEnumeratingAlgorithm and createAlgorithm share code
> + *
> + * Worker method which allows sharing of common code in the Yaml and self-initialising algorithm case
> + *
> + * \param[in] context The IPA context to pass to the algorithm's init function
> + * \param[in] algoData Yaml object
> + * \param[in] name The name of the algorithm to instantiate
> + *
> + * \return 0 on success, negative errno value on failure:
> + *         -EINVAL if the algorithm is not found in the factory registry
> + *         Other negative values if algorithm initialisation fails
> + */
> +
>   /**
>    * \fn Module::createAlgorithms()
>    * \brief Create algorithms from YAML configuration data
> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h
> index c27af7718..d309606f2 100644
> --- a/src/ipa/libipa/module.h
> +++ b/src/ipa/libipa/module.h
> @@ -70,22 +70,25 @@ public:
>   		factories().push_back(factory);
>   	}
> 
> -private:
> -	int createAlgorithm(Context &context, const YamlObject &data)
> +	int createSelfEnumeratingAlgorithm(Context &context, const std::string &name)
>   	{
> -		const auto &[name, algoData] = *data.asDict().begin();
> +		YamlObject dummy;
> 
> -		/*
> -		 * Optionally, algorithms can be disabled via the tuning file
> -		 * by including enabled: false as a parameter within the
> -		 * algorithm tuning data. This is not an error, so we return 0.
> -		 */
> -		if (!algoData["enabled"].get<bool>(true)) {
> -			LOG(IPAModuleAlgo, Info)
> -				<< "Algorithm '" << name << "' disabled via tuning file";
> -			return 0;
> +		std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name);
> +		if (!algo) {
> +			LOG(IPAModuleAlgo, Error)
> +				<< "Algorithm '" << name << "' not found";
> +			return -EINVAL;
>   		}
> 
> +		context.selfInitialising = true;

The fact that such a variable is required in the `Context` type should be documented.
And which component is supposed to set it to false between calls?

But in any case, I think this is not the ideal approach. I feel like adding an `init()`
that does not take a `YamlObject` parameter might be better. (With a default implementation
in `Algorithm` returning `-ENOTSUP` or similar.)

And just to confirm, this is needed so that if the "Ccm" is not explicitly present in the
tuning file, then it is automatically loaded if the gpu is used (but not if the cpu is used)?


Regards,
Barnabás Pőcze


> +
> +		return createAlgorithmCommon(context, dummy, name);
> +	}
> +
> +private:
> +	int createAlgorithmCommon(Context &context, const YamlObject &algoData, const std::string &name)
> +	{
>   		std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name);
>   		if (!algo) {
>   			LOG(IPAModuleAlgo, Error)
> @@ -104,9 +107,28 @@ private:
>   			<< "Instantiated algorithm '" << name << "'";
> 
>   		algorithms_.push_back(std::move(algo));
> +
>   		return 0;
>   	}
> 
> +	int createAlgorithm(Context &context, const YamlObject &data)
> +	{
> +		const auto &[name, algoData] = *data.asDict().begin();
> +
> +		/*
> +		 * Optionally, algorithms can be disabled via the tuning file
> +		 * by including enabled: false as a parameter within the
> +		 * algorithm tuning data. This is not an error, so we return 0.
> +		 */
> +		if (!algoData["enabled"].get<bool>(true)) {
> +			LOG(IPAModuleAlgo, Info)
> +				<< "Algorithm '" << name << "' disabled via tuning file";
> +			return 0;
> +		}
> +
> +		return createAlgorithmCommon(context, algoData, name);
> +	}
> +
>   	static std::unique_ptr<Algorithm<Module>> createAlgorithm(const std::string &name)
>   	{
>   		for (const AlgorithmFactoryBase<Module> *factory : factories()) {
> --
> 2.52.0
>
Milan Zamazal Dec. 17, 2025, 4:54 p.m. UTC | #2 
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> Hi
>
> 2025. 12. 17. 11:01 keltezéssel, Bryan O'Donoghue írta:
>> Create an algorithm without having YAML data input.
>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   src/ipa/libipa/module.cpp | 34 +++++++++++++++++++++++++++++
>>   src/ipa/libipa/module.h   | 46 +++++++++++++++++++++++++++++----------
>>   2 files changed, 68 insertions(+), 12 deletions(-)
>> diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp
>> index a95dca696..8e86ca823 100644
>> --- a/src/ipa/libipa/module.cpp
>> +++ b/src/ipa/libipa/module.cpp
>> @@ -83,6 +83,40 @@ namespace ipa {
>>    * \return The list of instantiated algorithms
>>    */
>> +/**
>> + * \fn int Module::createSelfEnumeratingAlgorithm(Context &context, const std::string &name)
>> + * \brief Create and initialise a self-enumerating algorithm by name
>> + *
>> + * This function creates an algorithm instance from the registered algorithm
>> + * factories using only the algorithm name, without requiring YAML configuration
>> + * data.
>> + *
>> + * This is useful for algorithms that don't require external configuration
>> + * parameters and can self-configure or use default values.
>> + *
>> + * \param[in] context The IPA context to pass to the algorithm's init function
>> + * \param[in] name The name of the algorithm to instantiate
>> + *
>> + * \return 0 on success, negative errno value on failure:
>> + *         -EINVAL if the algorithm is not found in the factory registry
>> + *         Other negative values if algorithm initialisation fails
>> + */
>> +
>> +/**
>> + * \fn int Module::createAlgorithmCommon(Context &context, const YamlObject &algoData, const std::string &name)
>> + * \brief Common helper fucntion to allow createSelfEnumeratingAlgorithm and createAlgorithm share code
>> + *
>> + * Worker method which allows sharing of common code in the Yaml and self-initialising algorithm case
>> + *
>> + * \param[in] context The IPA context to pass to the algorithm's init function
>> + * \param[in] algoData Yaml object
>> + * \param[in] name The name of the algorithm to instantiate
>> + *
>> + * \return 0 on success, negative errno value on failure:
>> + *         -EINVAL if the algorithm is not found in the factory registry
>> + *         Other negative values if algorithm initialisation fails
>> + */
>> +
>>   /**
>>    * \fn Module::createAlgorithms()
>>    * \brief Create algorithms from YAML configuration data
>> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h
>> index c27af7718..d309606f2 100644
>> --- a/src/ipa/libipa/module.h
>> +++ b/src/ipa/libipa/module.h
>> @@ -70,22 +70,25 @@ public:
>>   		factories().push_back(factory);
>>   	}
>> -private:
>> -	int createAlgorithm(Context &context, const YamlObject &data)
>> +	int createSelfEnumeratingAlgorithm(Context &context, const std::string &name)
>>   	{
>> -		const auto &[name, algoData] = *data.asDict().begin();
>> +		YamlObject dummy;
>> -		/*
>> -		 * Optionally, algorithms can be disabled via the tuning file
>> -		 * by including enabled: false as a parameter within the
>> -		 * algorithm tuning data. This is not an error, so we return 0.
>> -		 */
>> -		if (!algoData["enabled"].get<bool>(true)) {
>> -			LOG(IPAModuleAlgo, Info)
>> -				<< "Algorithm '" << name << "' disabled via tuning file";
>> -			return 0;
>> +		std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name);
>> +		if (!algo) {
>> +			LOG(IPAModuleAlgo, Error)
>> +				<< "Algorithm '" << name << "' not found";
>> +			return -EINVAL;
>>   		}
>> +		context.selfInitialising = true;
>
> The fact that such a variable is required in the `Context` type should be documented.
> And which component is supposed to set it to false between calls?
>
> But in any case, I think this is not the ideal approach. I feel like adding an `init()`
> that does not take a `YamlObject` parameter might be better. (With a default implementation
> in `Algorithm` returning `-ENOTSUP` or similar.)
>
> And just to confirm, this is needed so that if the "Ccm" is not explicitly present in the
> tuning file, then it is automatically loaded if the gpu is used (but not if the cpu is used)?

Yes.  But we agreed at today's meeting that this and the related patches
can be dropped and the identity matrix can be specified in the default
uncalibrated.yaml instead.  This will slow down non-CCM CPU ISP but this
is not a big issue because GPU ISP will be the default, the default
matrix in uncalibrated.yaml can be commented out if needed and I'll work
on the simple IPA algorithms cleanup, which should avoid the need of
similar hacks in future.

>
> Regards,
> Barnabás Pőcze
>
>
>> +
>> +		return createAlgorithmCommon(context, dummy, name);
>> +	}
>> +
>> +private:
>> +	int createAlgorithmCommon(Context &context, const YamlObject &algoData, const std::string &name)
>> +	{
>>   		std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name);
>>   		if (!algo) {
>>   			LOG(IPAModuleAlgo, Error)
>> @@ -104,9 +107,28 @@ private:
>>   			<< "Instantiated algorithm '" << name << "'";
>>   		algorithms_.push_back(std::move(algo));
>> +
>>   		return 0;
>>   	}
>> +	int createAlgorithm(Context &context, const YamlObject &data)
>> +	{
>> +		const auto &[name, algoData] = *data.asDict().begin();
>> +
>> +		/*
>> +		 * Optionally, algorithms can be disabled via the tuning file
>> +		 * by including enabled: false as a parameter within the
>> +		 * algorithm tuning data. This is not an error, so we return 0.
>> +		 */
>> +		if (!algoData["enabled"].get<bool>(true)) {
>> +			LOG(IPAModuleAlgo, Info)
>> +				<< "Algorithm '" << name << "' disabled via tuning file";
>> +			return 0;
>> +		}
>> +
>> +		return createAlgorithmCommon(context, algoData, name);
>> +	}
>> +
>>   	static std::unique_ptr<Algorithm<Module>> createAlgorithm(const std::string &name)
>>   	{
>>   		for (const AlgorithmFactoryBase<Module> *factory : factories()) {
>> --
>> 2.52.0
>>

Patch
diff mbox series 

diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp
index a95dca696..8e86ca823 100644
--- a/src/ipa/libipa/module.cpp
+++ b/src/ipa/libipa/module.cpp
@@ -83,6 +83,40 @@  namespace ipa {
  * \return The list of instantiated algorithms
  */
 
+/**
+ * \fn int Module::createSelfEnumeratingAlgorithm(Context &context, const std::string &name)
+ * \brief Create and initialise a self-enumerating algorithm by name
+ *
+ * This function creates an algorithm instance from the registered algorithm
+ * factories using only the algorithm name, without requiring YAML configuration
+ * data.
+ *
+ * This is useful for algorithms that don't require external configuration
+ * parameters and can self-configure or use default values.
+ *
+ * \param[in] context The IPA context to pass to the algorithm's init function
+ * \param[in] name The name of the algorithm to instantiate
+ *
+ * \return 0 on success, negative errno value on failure:
+ *         -EINVAL if the algorithm is not found in the factory registry
+ *         Other negative values if algorithm initialisation fails
+ */
+
+/**
+ * \fn int Module::createAlgorithmCommon(Context &context, const YamlObject &algoData, const std::string &name)
+ * \brief Common helper fucntion to allow createSelfEnumeratingAlgorithm and createAlgorithm share code
+ *
+ * Worker method which allows sharing of common code in the Yaml and self-initialising algorithm case
+ *
+ * \param[in] context The IPA context to pass to the algorithm's init function
+ * \param[in] algoData Yaml object
+ * \param[in] name The name of the algorithm to instantiate
+ *
+ * \return 0 on success, negative errno value on failure:
+ *         -EINVAL if the algorithm is not found in the factory registry
+ *         Other negative values if algorithm initialisation fails
+ */
+
 /**
  * \fn Module::createAlgorithms()
  * \brief Create algorithms from YAML configuration data
diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h
index c27af7718..d309606f2 100644
--- a/src/ipa/libipa/module.h
+++ b/src/ipa/libipa/module.h
@@ -70,22 +70,25 @@  public:
 		factories().push_back(factory);
 	}
 
-private:
-	int createAlgorithm(Context &context, const YamlObject &data)
+	int createSelfEnumeratingAlgorithm(Context &context, const std::string &name)
 	{
-		const auto &[name, algoData] = *data.asDict().begin();
+		YamlObject dummy;
 
-		/*
-		 * Optionally, algorithms can be disabled via the tuning file
-		 * by including enabled: false as a parameter within the
-		 * algorithm tuning data. This is not an error, so we return 0.
-		 */
-		if (!algoData["enabled"].get<bool>(true)) {
-			LOG(IPAModuleAlgo, Info)
-				<< "Algorithm '" << name << "' disabled via tuning file";
-			return 0;
+		std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name);
+		if (!algo) {
+			LOG(IPAModuleAlgo, Error)
+				<< "Algorithm '" << name << "' not found";
+			return -EINVAL;
 		}
 
+		context.selfInitialising = true;
+
+		return createAlgorithmCommon(context, dummy, name);
+	}
+
+private:
+	int createAlgorithmCommon(Context &context, const YamlObject &algoData, const std::string &name)
+	{
 		std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name);
 		if (!algo) {
 			LOG(IPAModuleAlgo, Error)
@@ -104,9 +107,28 @@  private:
 			<< "Instantiated algorithm '" << name << "'";
 
 		algorithms_.push_back(std::move(algo));
+
 		return 0;
 	}
 
+	int createAlgorithm(Context &context, const YamlObject &data)
+	{
+		const auto &[name, algoData] = *data.asDict().begin();
+
+		/*
+		 * Optionally, algorithms can be disabled via the tuning file
+		 * by including enabled: false as a parameter within the
+		 * algorithm tuning data. This is not an error, so we return 0.
+		 */
+		if (!algoData["enabled"].get<bool>(true)) {
+			LOG(IPAModuleAlgo, Info)
+				<< "Algorithm '" << name << "' disabled via tuning file";
+			return 0;
+		}
+
+		return createAlgorithmCommon(context, algoData, name);
+	}
+
 	static std::unique_ptr<Algorithm<Module>> createAlgorithm(const std::string &name)
 	{
 		for (const AlgorithmFactoryBase<Module> *factory : factories()) {