[libcamera-devel] [PATCH v3 5/6] libcamera: ipa_manager: Search for IPA libraries in build tree
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Feb 21 14:06:15 CET 2020
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
More information about the libcamera-devel
mailing list