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.
Tom
Though you are right that libdrm is technically a requirement and I should add that to the README.
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of StDenis, Tom <Tom.StDenis@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)
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@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)
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@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@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
On 5 February 2017 at 22:24, Tom St Denis <tstdenis82@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@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