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

[Kieran Bingham]

Hi Laurent,

On 21/02/2020 12:37, Kieran Bingham wrote:
> 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 ...

sorry - I just realised the position of this would /not/ put the
functions between other IPAManager functions which is what I thought on
first read.

Putting just before should be fine.

> 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