[libcamera-devel] [PATCH 4/5] libcamera: ipa_manager: Search the runpath for IPA libraries

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wed, Feb 05, 2020 at 01:04:19PM +0000, Kieran Bingham wrote:
> Expose the IPA build path through the libcamera library runpath entry in

s/library runpath entry/library search path entry (DT_RUNPATH)/

> the elf string table. Utilise this path to search for compiled IPA

s/elf string table/ELF dynamic tags/ ?

> libraries when loading the IPAManager.
> 
> The build time path is removed during the library install phase.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/ipa_manager.cpp | 9 +++++++++
>  src/libcamera/meson.build     | 1 +
>  src/meson.build               | 7 +++++++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 24fe709108fe..3dfe84fcf95e 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -102,6 +102,15 @@ IPAManager::IPAManager()
>  	if (ret > 0)
>  		ipaCount += ret;
>  
> +	/*
> +	 * Utilise the Elf runpath to supply the IPA build directory, allowing

s/Elf/ELF/

> +	 * us to load IPA's before they are installed for testing and
> +	 * development.
> +	 */
> +	const char *runpath = utils::get_runpath();
> +	if (runpath)
> +		ipaCount += addPath(runpath, true);

Shouldn't this go after LIBCAMERA_IPA_MODULE_PATH ? If a user sets
LIBCAMERA_IPA_MODULE_PATH, it should be considered before DT_RUNPATH.

I think we also need to move IPA_MODULE_DIR after DT_RUNPATH, as the
build directory should have precedence over the system directory.

Maybe the above comment should be expanded to explain the load order ?


	/*
	 * Load IPA modules. The IPA manager uses the following search paths to
	 * locate the modules.
	 *
	 * - The directories in DT_RUNPATH. The build system sets DT_RUNPATH to
	 *   point to the src/ipa/ subdirectory of the build directory, allowing
	 *   finding modules in the build directory. DT_RUNPATH is stripped when
	 *   libcamera.so is installed.
	 *
	 * - The directories specified in the LIBCAMERA_IPA_MODULE_PATH
	 *   environment variable, if set.
	 *
	 * - The $prefix/ipa/ directory.
	 */

> +
>  	const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
>  	if (!modulePaths) {
>  		if (!ipaCount)
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 68d89559b290..87c5bbbb17a4 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -107,6 +107,7 @@ libcamera = shared_library('camera',
>                             install : true,
>                             link_with : libcamera_link_with,
>                             include_directories : includes,
> +                           build_rpath : ipa_build_path,
>                             dependencies : libcamera_deps)
>  
>  libcamera_dep = declare_dependency(sources : [libcamera_api, libcamera_ipa_api, libcamera_h],
> diff --git a/src/meson.build b/src/meson.build
> index 5adcd61fd913..42e1eb984ccd 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -2,6 +2,13 @@ if get_option('android')
>      subdir('android')
>  endif
>  
> +# Provide libcamera with a reference to the current build for IPA

s/current build/build directory path/ ?

> +# loading. This allows IPAs to be successfully loaded when tests and
> +# applications are run from the build tree, but is stripped during the
> +# install phase, ensuring we maintain reproducible builds.
> +
> +ipa_build_path = join_paths(meson.current_build_dir(), 'ipa')
> +
>  subdir('libcamera')
>  subdir('ipa')
>  subdir('cam')

-- 
Regards,

Laurent Pinchart