[PATCH] Autodetect libdrm path (v2)

[StDenis, Tom]

Apparently I missed the bottom of your reply (all of the clients I have outlook/gmail do top post only ...) as for the cmake changes. I'm for it if you want to submit a patch.  I can't imagine a lot of cross compiling though since users will typically be using it on the platform they built it for.


Ironically, I had the pkg_check originally but was told that's a faux-pas.


Unless this is breaking for actual users though it's not really a priority to bikeshed the build system of a 30 file project that is meant to work only on developer machines who are likely building for themselves [😊]

Tom

________________________________
From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> on behalf of StDenis, Tom <Tom.StDenis at amd.com>
Sent: Monday, February 6, 2017 16:33
To: Emil Velikov; Tom St Denis
Cc: amd-gfx mailing list
Subject: Re: [PATCH] Autodetect libdrm path (v2)


I have to NAK that idea since we use umr on NPI work which doesn't necessarily have libdrm support.


Also umr can read/write registers via pci access without amdgpu loaded (handy if amdgpu fails to init properly).

Though you are right that libdrm is technically a requirement and I should add that to the README.

Tom
________________________________
From: Emil Velikov <emil.l.velikov at gmail.com>
Sent: Monday, February 6, 2017 15:18
To: Tom St Denis
Cc: amd-gfx mailing list; StDenis, Tom
Subject: Re: [PATCH] Autodetect libdrm path (v2)

Hi Tom,

On 5 February 2017 at 22:24, Tom St Denis <tstdenis82 at gmail.com> wrote:
> (v2):  Use findLibDRM script instead of directly finding path
>
Since the project already depends on libdrm you might want to use the
drmDevice2 API and drop the iteration over all PCI devices
(libpciaccess dependency).
If the former is lacking something feel free to suggest/send patches ;-)

The libdrm requirement was missing last time I've skimmed through the README.

> Signed-off-by: Tom St Denis <tom.stdenis at amd.com>
> ---

> --- /dev/null
> +++ b/cmake_modules/FindLibDRM.cmake
> @@ -0,0 +1,35 @@
> +# Try to find libdrm
> +#
> +# Once done, this will define
> +#
> +# LIBDRM_FOUND
> +# LIBDRM_INCLUDE_DIR
> +# LIBDRM_LIBRARIES
> +
> +find_package(PkgConfig)
> +
> +pkg_check_modules(PC_LIBDRM QUIET libdrm)
You really want a version check.

> +
> +find_path(LIBDRM_INCLUDE_DIR NAMES amdgpu_drm.h
> +    HINTS
> +    ${PC_LIBDRM_INCLUDEDIR}
> +    ${PC_LIBDRM_INCLUDE_DIRS}
> +    /usr/include
> +)
> +
> +find_library(LIBDRM_LIBRARY NAMES libdrm_amdgpu.so.1
> +    HINTS
> +    ${PC_LIBDRM_LIBDIR}
> +    ${PC_LIBDRM_LIBRARY_DIRS}
> +    /usr/lib64
> +    /usr/lib
> +)
> +
Here is one of the things which makes me rage against cmake - no
picking /usr/{include,foo} is _not_ what you want.

During cross-compilation as one is missing the .pc file, the system
headers/libraries will be picked. This is horribly wrong, yet rather
common in cmake world.
What you really want is a lovely error message to warn the user -
s/OPTIONAL/REQUIRED/ will give you just that.

At which point, checking for the header/library in the paths provided
by the .pc file is redundant and thus nearly everything else in the
file ;-)


TL;Dr: All you need is pkg_check_modules(PC_LIBDRM REQUIRED libdrm>=XX).

Thanks
Emil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170206/50fe1908/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OutlookEmoji-😊.png
Type: image/png
Size: 488 bytes
Desc: OutlookEmoji-😊.png
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170206/50fe1908/attachment-0001.png>