[libcamera-devel] [PATCH v3 5/6] libcamera: ipa_manager: Search for IPA libraries in build tree

[Kieran Bingham]

Hi Laurent,

On 20/02/2020 21:24, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Feb 20, 2020 at 04:57:03PM +0000, Kieran Bingham wrote:
>> When libcamera is built and tested (or used at all) before installing to
>> the configured prefix path, it will be unable to locate the IPA
>> binaries, or IPA binaries previously installed in the system paths may
>> be incorrect to load.
>>
>> Utilise the build_rpath dynamic tag which is stripped out by meson at
>> install time to determine at runtime if the library currently executing
>> has been installed or not.
>>
>> When not installed and running from a build tree, identify the location
>> of that tree by finding the path of the active libcamera.so itself, and
>> from that point add a relative path to be able to load the most recently
>> built IPA modules.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>> ---
>> v2:
>>  - Move get_runpath to ipa_manager.c : libcamera::elfRunPath()
>>  - Minor fixes
>>  - Squash elfRunPath() into this patch
>>
>> v3:
>>  - Full rework. It's just all different :-)
>>  - Maybe this one is going to cut it ...
>> ---
>>  src/libcamera/ipa_manager.cpp | 46 ++++++++++++++++++++++++++++++++++-
>>  src/libcamera/meson.build     |  6 +++++
>>  2 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index 3b1d4c0b295e..e60bf3dabebe 100644
>> --- a/src/libcamera/ipa_manager.cpp
>> +++ b/src/libcamera/ipa_manager.cpp
>> @@ -9,6 +9,9 @@
>>  
>>  #include <algorithm>
>>  #include <dirent.h>
>> +#include <dlfcn.h>
>> +#include <elf.h>
>> +#include <link.h>
>>  #include <string.h>
>>  #include <sys/types.h>
>>  
>> @@ -24,6 +27,30 @@
>>   * \brief Image Processing Algorithm module manager
>>   */
>>  
>> +static bool isLibcameraInstalled()
>> +{
> 
> I'd add a comment here to state
> 
> 	/* musl doesn't declare _DYNAMIC in link.h, declare it manually. */
> 
> to remember this is here for a reason.

Added.

> 
>> +	extern ElfW(Dyn) _DYNAMIC[];
> 
> Maybe add a blank line here ?

Hrm... I used to have one. Maybe I lost it during a rebase :(
Added

> 
>> +	/* DT_RUNPATH (DT_RPATH on musl) is removed on install. */
> 
> Do we really need to check for DT_RPATH ? As far as I know the tag that
> is generated depends solely on the linker version, not on the C library.
> Does musl really generate DT_RPATH ?
> 
>> +	for (const ElfW(Dyn) *dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn)
>> +		if (dyn->d_tag == DT_RUNPATH || dyn->d_tag == DT_RPATH)
>> +			return false;
> 
> Braces for the 'for' ?

(Re-)added

> 
>> +
>> +	return true;
>> +}
>> +
>> +static const char *libcameraPath()
>> +{
>> +	Dl_info info;
>> +	int ret;
>> +
>> +	/* look up our own symbol. */
> 
> s/look/Look/
> 
> That's a neat way to do it, we could have used an unrelated static
> function (such as IPAManager::instance) and inlined this code in
> IPAManager::IPAManager, but create a separate static function makes the
> code self-contained.

Yes, and I envision this function being re-used (As you've noticed
later, we also have the proxy worker variable to deal with).

> 
>> +	ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info);
>> +	if (ret == 0)
>> +		return nullptr;
>> +
>> +	return info.dli_fname;
>> +}
> 
> I would move these two functions just above IPAManager::IPAManager to
> keep them close to the location where they're used.

Without them being member functions? Euewww ...

I don't think they should be member functions, and in fact will have to
be moved to utils:: when we handle LIBCAMERA_IPA_PROXY_PATH

I had been leaving that until I got this working.

Would you prefer I put these functions in utils:: now?


>> +
>>  namespace libcamera {
>>  
>>  LOG_DEFINE_CATEGORY(IPAManager)
>> @@ -108,7 +135,24 @@ IPAManager::IPAManager()
>>  				<< "No IPA found in '" << modulePaths << "'";
>>  	}
>>  
>> -	/* Load IPAs from the installed system path. */
>> +	/*
>> +	 * When libcamera is used before it is installed, load IPAs from the
>> +	 * same build directory as the libcamera library itself. This requires
>> +	 * identifying the path of the libcamera.so, and referencing a relative
>> +	 * path for the IPA from that point.
> 
> Maybe add "We need to recursive into one level of sub-directories to
> match the build tree." ?

Added

> 
>> +	 */
>> +	if (!isLibcameraInstalled()) {
>> +		std::string ipaBuildPath = utils::dirname(libcameraPath()) + "/../ipa";
>> +		constexpr int maxDepth = 1;
>> +
>> +		LOG(IPAManager, Info)
>> +			<< "libcamera is not installed. Adding '"
>> +			<< ipaBuildPath << "' to the IPA search path";
>> +
>> +		ipaCount += addDir(ipaBuildPath.c_str(), maxDepth);
>> +	}
>> +
>> +	/* Finally try to load IPAs from the installed system path. */
>>  	ret = addDir(IPA_MODULE_DIR);
>>  	if (ret > 0)
>>  		ipaCount += ret;
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 1e5b54b34078..88658ac563f7 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -107,11 +107,17 @@ if get_option('android')
>>      libcamera_link_with += android_camera_metadata
>>  endif
>>  
>> +# We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
>> +# The build_rpath is stripped at install time by meson, so we determine at
>> +# runtime if the library is running from an installed location by checking
>> +# for the presence or abscence of the dynamic tag.
> 
> s/abscence/absence/
> 
> Don't tell me this is a UK spelling :-)

Just a typo ;-)

> 
>> +
>>  libcamera = shared_library('camera',
>>                             libcamera_sources,
>>                             install : true,
>>                             link_with : libcamera_link_with,
>>                             include_directories : includes,
>> +                           build_rpath : '/',
>>                             dependencies : libcamera_deps)
> 
> I double-checked based on what criteria build_rpath would generate
> DT_RPATH or DT_RUNPATH. It turns out this is controlled by the
> --enable-new-dtags and --disable-new-dtags options to ld, with the
> format generating DT_RUNPATH and the latter DT_RPATH. None of these
> are set by meson by default. GNU ld defaulted to --enable-new-dtags in
> binutils v2.24, released on December 2013, before gcc 5.1 (April 2015)
> which is the earliest compiler version we support (I'm actually not even
> sure we support gcc 5.1, I have gcc 5.4 installed in my test setup,
> released on June 2016). Debian Jessie ships binutils v2.25 and Ubuntu
> 16.04 ships binutils v2.26. We should thus be safe.
> 
> Assuming the DT_RPATH comment for musl isn't right and the corresponding
> condition can be removed,

The condition can't be removed. But having investigated it's possibly a
linker topic when using the Alpine distribution rather than an effect of
the musl c-library.


> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Otherwise let's discuss it.

Going to have to send a v4 of this series anyway now :-S

Check again then :-)


>>  libcamera_dep = declare_dependency(sources : [libcamera_api, libcamera_ipa_api, libcamera_h],
> 

-- 
Regards
--
Kieran