[libcamera-devel,v3,2/6] libcamera: ipa_manager: Split path handling
diff mbox series

Message ID 20200220165704.23600-3-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • Support loading IPAs from the build tree
Related show

Commit Message

Kieran Bingham Feb. 20, 2020, 4:57 p.m. UTC 
Move the iteration of a colon-separated path to its own function.
This prepares for parsing extra path variables.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/include/ipa_manager.h |  1 +
 src/libcamera/ipa_manager.cpp       | 35 ++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 8 deletions(-)

Comments

Laurent Pinchart Feb. 20, 2020, 8:27 p.m. UTC | #1 
Hi Kieran,

Thank you for the patch.

On Thu, Feb 20, 2020 at 04:57:00PM +0000, Kieran Bingham wrote:
> Move the iteration of a colon-separated path to its own function.
> This prepares for parsing extra path variables.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/include/ipa_manager.h |  1 +
>  src/libcamera/ipa_manager.cpp       | 35 ++++++++++++++++++++++-------
>  2 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
> index 126f8babbc8f..94d35d9062e4 100644
> --- a/src/libcamera/include/ipa_manager.h
> +++ b/src/libcamera/include/ipa_manager.h
> @@ -33,6 +33,7 @@ private:
>  	~IPAManager();
>  
>  	int addDir(const char *libDir);
> +	int addPath(const char *path);
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 4ffbdd712ac2..d87f2221b00b 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -110,14 +110,7 @@ IPAManager::IPAManager()
>  		return;
>  	}
>  
> -	for (const auto &dir : utils::split(modulePaths, ":")) {
> -		if (dir.empty())
> -			continue;
> -
> -		int ret = addDir(dir.c_str());
> -		if (ret > 0)
> -			ipaCount += ret;
> -	}
> +	ipaCount += addPath(modulePaths);
>  
>  	if (!ipaCount)
>  		LOG(IPAManager, Warning)
> @@ -197,6 +190,32 @@ int IPAManager::addDir(const char *libDir)
>  	return count;
>  }
>  
> +/**
> + * \brief Load IPA modules from a search path
> + * \param[in] path The colon-separated list of directories to load IPA modules from
> + *
> + * This method tries to create an IPAModule instance for every shared object
> + * found in the directories listed in \a path.
> + *
> + * \return Number of modules loaded by this call, or a negative error code
> + * otherwise

The function never returns a negative error code. I would drop the part
after the comma, and change the function return type and the ipaCount to
be unsigned int.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + */
> +int IPAManager::addPath(const char *path)
> +{
> +	int ipaCount = 0;
> +
> +	for (const auto &dir : utils::split(path, ":")) {
> +		if (dir.empty())
> +			continue;
> +
> +		int ret = addDir(dir.c_str());
> +		if (ret > 0)
> +			ipaCount += ret;
> +	}
> +
> +	return ipaCount;
> +}
> +
>  /**
>   * \brief Create an IPA interface that matches a given pipeline handler
>   * \param[in] pipe The pipeline handler that wants a matching IPA interface
Kieran Bingham Feb. 21, 2020, 11:52 a.m. UTC | #2 
Hi Laurent,

On 20/02/2020 20:27, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Feb 20, 2020 at 04:57:00PM +0000, Kieran Bingham wrote:
>> Move the iteration of a colon-separated path to its own function.
>> This prepares for parsing extra path variables.
>>

In fact, we no longer parse 'extra' path variables.

Also - once addDir is changed to not return errors, this functionality
really is just simple enough to be inlined and avoid all the extra
documentation and layer required for little gain.

Very likely dropping this patch.

--

Kieran

>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/libcamera/include/ipa_manager.h |  1 +
>>  src/libcamera/ipa_manager.cpp       | 35 ++++++++++++++++++++++-------
>>  2 files changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
>> index 126f8babbc8f..94d35d9062e4 100644
>> --- a/src/libcamera/include/ipa_manager.h
>> +++ b/src/libcamera/include/ipa_manager.h
>> @@ -33,6 +33,7 @@ private:
>>  	~IPAManager();
>>  
>>  	int addDir(const char *libDir);
>> +	int addPath(const char *path);
>>  };
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index 4ffbdd712ac2..d87f2221b00b 100644
>> --- a/src/libcamera/ipa_manager.cpp
>> +++ b/src/libcamera/ipa_manager.cpp
>> @@ -110,14 +110,7 @@ IPAManager::IPAManager()
>>  		return;
>>  	}
>>  
>> -	for (const auto &dir : utils::split(modulePaths, ":")) {
>> -		if (dir.empty())
>> -			continue;
>> -
>> -		int ret = addDir(dir.c_str());
>> -		if (ret > 0)
>> -			ipaCount += ret;
>> -	}
>> +	ipaCount += addPath(modulePaths);
>>  
>>  	if (!ipaCount)
>>  		LOG(IPAManager, Warning)
>> @@ -197,6 +190,32 @@ int IPAManager::addDir(const char *libDir)
>>  	return count;
>>  }
>>  
>> +/**
>> + * \brief Load IPA modules from a search path
>> + * \param[in] path The colon-separated list of directories to load IPA modules from
>> + *
>> + * This method tries to create an IPAModule instance for every shared object
>> + * found in the directories listed in \a path.
>> + *
>> + * \return Number of modules loaded by this call, or a negative error code
>> + * otherwise
> 
> The function never returns a negative error code. I would drop the part
> after the comma, and change the function return type and the ipaCount to
> be unsigned int.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> + */
>> +int IPAManager::addPath(const char *path)
>> +{
>> +	int ipaCount = 0;
>> +
>> +	for (const auto &dir : utils::split(path, ":")) {
>> +		if (dir.empty())
>> +			continue;
>> +
>> +		int ret = addDir(dir.c_str());
>> +		if (ret > 0)
>> +			ipaCount += ret;
>> +	}
>> +
>> +	return ipaCount;
>> +}
>> +
>>  /**
>>   * \brief Create an IPA interface that matches a given pipeline handler
>>   * \param[in] pipe The pipeline handler that wants a matching IPA interface
>

Patch
diff mbox series 

diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
index 126f8babbc8f..94d35d9062e4 100644
--- a/src/libcamera/include/ipa_manager.h
+++ b/src/libcamera/include/ipa_manager.h
@@ -33,6 +33,7 @@  private:
 	~IPAManager();
 
 	int addDir(const char *libDir);
+	int addPath(const char *path);
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index 4ffbdd712ac2..d87f2221b00b 100644
--- a/src/libcamera/ipa_manager.cpp
+++ b/src/libcamera/ipa_manager.cpp
@@ -110,14 +110,7 @@  IPAManager::IPAManager()
 		return;
 	}
 
-	for (const auto &dir : utils::split(modulePaths, ":")) {
-		if (dir.empty())
-			continue;
-
-		int ret = addDir(dir.c_str());
-		if (ret > 0)
-			ipaCount += ret;
-	}
+	ipaCount += addPath(modulePaths);
 
 	if (!ipaCount)
 		LOG(IPAManager, Warning)
@@ -197,6 +190,32 @@  int IPAManager::addDir(const char *libDir)
 	return count;
 }
 
+/**
+ * \brief Load IPA modules from a search path
+ * \param[in] path The colon-separated list of directories to load IPA modules from
+ *
+ * This method tries to create an IPAModule instance for every shared object
+ * found in the directories listed in \a path.
+ *
+ * \return Number of modules loaded by this call, or a negative error code
+ * otherwise
+ */
+int IPAManager::addPath(const char *path)
+{
+	int ipaCount = 0;
+
+	for (const auto &dir : utils::split(path, ":")) {
+		if (dir.empty())
+			continue;
+
+		int ret = addDir(dir.c_str());
+		if (ret > 0)
+			ipaCount += ret;
+	}
+
+	return ipaCount;
+}
+
 /**
  * \brief Create an IPA interface that matches a given pipeline handler
  * \param[in] pipe The pipeline handler that wants a matching IPA interface