Re: [PATCH] Autodetect libdrm path (v2)

From: "StDenis, Tom" <Tom.StDenis-5C7GfCeVMHo@public.gmane.org>
To: Emil Velikov
	<emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Tom St Denis <tstdenis82-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: amd-gfx mailing list
	<amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Subject: Re: [PATCH] Autodetect libdrm path (v2)
Date: Mon, 6 Feb 2017 21:33:10 +0000	[thread overview]
Message-ID: <CY4PR12MB17689AEC15DEF9662A16DEA3F7400@CY4PR12MB1768.namprd12.prod.outlook.com> (raw)
In-Reply-To: <CACvgo5230Use5sk8dm5jBbLqiJeSkgB6cDikz-N4omx-4Jgy9A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

[-- Attachment #1.1: Type: text/plain, Size: 2495 bytes --]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-5C7GfCeVMHo@public.gmane.org>
> ---

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

[-- Attachment #1.2: Type: text/html, Size: 3947 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx