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

[Laurent Pinchart]

Hi Kieran,

On Fri, Feb 21, 2020 at 12:37:06PM +0000, Kieran Bingham wrote:
> On 20/02/2020 21:24, Laurent Pinchart wrote:
> > 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?

No, it's fine leaving them here for now, we can rework that later when
dealing with the proxy workers path.

> >> +
> >>  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

s/recursive/recurse/ (or "search recursively")

> > 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.

Then updating the comment above should be all we need.

> > 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,

Laurent Pinchart