All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Autodetect libdrm path (v2)
@ 2017-02-05 22:24 Tom St Denis
       [not found] ` <20170205222447.19945-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Tom St Denis @ 2017-02-05 22:24 UTC (permalink / raw)
  To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Tom St Denis

(v2):  Use findLibDRM script instead of directly finding path

Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
---
 CMakeLists.txt                 |  3 +++
 cmake_modules/FindLibDRM.cmake | 35 +++++++++++++++++++++++++++++++++++
 src/lib/query_drm.c            |  4 ++--
 3 files changed, 40 insertions(+), 2 deletions(-)
 create mode 100644 cmake_modules/FindLibDRM.cmake

diff --git a/CMakeLists.txt b/CMakeLists.txt
index bef94fdba788..ef78c97ad763 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -25,6 +25,9 @@ include_directories(${CURSES_INCLUDE_DIRS})
 find_package(PCIAccess REQUIRED)
 include_directories(${PCIACCESS_INCLUDE_DIR})
 
+find_package(LibDRM REQUIRED)
+include_directories(${LIBDRM_INCLUDE_DIR})
+
 set(REQUIRED_EXTERNAL_LIBS
   ${CURSES_LIBRARIES}
   ${PCIACCESS_LIBRARIES}
diff --git a/cmake_modules/FindLibDRM.cmake b/cmake_modules/FindLibDRM.cmake
new file mode 100644
index 000000000000..e840c4d1bfd0
--- /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)
+
+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
+)
+
+SET(LIBDRM_LIBRARIES optimized ${LIBDRM_LIBRARY})
+
+include(FindPackageHandleStandardArgs)
+find_package_handle_standard_args(LIBDRM DEFAULT_MSG
+	LIBDRM_LIBRARIES LIBDRM_INCLUDE_DIR
+)
+
+mark_as_advanced(LIBDRM_INCLUDE_DIR LIBDRM_LIBRARIES)
diff --git a/src/lib/query_drm.c b/src/lib/query_drm.c
index b9d80a8fc0c8..755c65fbc662 100644
--- a/src/lib/query_drm.c
+++ b/src/lib/query_drm.c
@@ -25,8 +25,8 @@
 #include "umr.h"
 #include <asm/ioctl.h>
 #include <sys/ioctl.h>
-#include <drm/drm.h>
-#include <drm/amdgpu_drm.h>
+#include <drm.h>
+#include <amdgpu_drm.h>
 
 #define DRM_IOC(dir, group, nr, size) _IOC(dir, group, nr, size)
 #define DRM_IOC_WRITE           _IOC_WRITE
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] Autodetect libdrm path (v2)
       [not found] ` <20170205222447.19945-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>
@ 2017-02-05 22:28   ` Andres Rodriguez
       [not found]     ` <62ec2a45-ea86-a643-f7fd-3e5e5054540b-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  2017-02-06 20:18   ` Emil Velikov
  1 sibling, 1 reply; 10+ messages in thread
From: Andres Rodriguez @ 2017-02-05 22:28 UTC (permalink / raw)
  To: Tom St Denis, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: Tom St Denis

Reviewed-by: Andres Rodriguez<andresx7@gmail.com>

On 2/5/2017 5:24 PM, Tom St Denis wrote:
> (v2):  Use findLibDRM script instead of directly finding path
>
> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
> ---
>  CMakeLists.txt                 |  3 +++
>  cmake_modules/FindLibDRM.cmake | 35 +++++++++++++++++++++++++++++++++++
>  src/lib/query_drm.c            |  4 ++--
>  3 files changed, 40 insertions(+), 2 deletions(-)
>  create mode 100644 cmake_modules/FindLibDRM.cmake
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index bef94fdba788..ef78c97ad763 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -25,6 +25,9 @@ include_directories(${CURSES_INCLUDE_DIRS})
>  find_package(PCIAccess REQUIRED)
>  include_directories(${PCIACCESS_INCLUDE_DIR})
>
> +find_package(LibDRM REQUIRED)
> +include_directories(${LIBDRM_INCLUDE_DIR})
> +
>  set(REQUIRED_EXTERNAL_LIBS
>    ${CURSES_LIBRARIES}
>    ${PCIACCESS_LIBRARIES}
> diff --git a/cmake_modules/FindLibDRM.cmake b/cmake_modules/FindLibDRM.cmake
> new file mode 100644
> index 000000000000..e840c4d1bfd0
> --- /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)
> +
> +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
> +)
> +
> +SET(LIBDRM_LIBRARIES optimized ${LIBDRM_LIBRARY})
> +
> +include(FindPackageHandleStandardArgs)
> +find_package_handle_standard_args(LIBDRM DEFAULT_MSG
> +	LIBDRM_LIBRARIES LIBDRM_INCLUDE_DIR
> +)
> +
> +mark_as_advanced(LIBDRM_INCLUDE_DIR LIBDRM_LIBRARIES)
> diff --git a/src/lib/query_drm.c b/src/lib/query_drm.c
> index b9d80a8fc0c8..755c65fbc662 100644
> --- a/src/lib/query_drm.c
> +++ b/src/lib/query_drm.c
> @@ -25,8 +25,8 @@
>  #include "umr.h"
>  #include <asm/ioctl.h>
>  #include <sys/ioctl.h>
> -#include <drm/drm.h>
> -#include <drm/amdgpu_drm.h>
> +#include <drm.h>
> +#include <amdgpu_drm.h>
>
>  #define DRM_IOC(dir, group, nr, size) _IOC(dir, group, nr, size)
>  #define DRM_IOC_WRITE           _IOC_WRITE
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Autodetect libdrm path (v2)
       [not found]     ` <62ec2a45-ea86-a643-f7fd-3e5e5054540b-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-02-05 22:42       ` StDenis, Tom
  0 siblings, 0 replies; 10+ messages in thread
From: StDenis, Tom @ 2017-02-05 22:42 UTC (permalink / raw)
  To: Andres Rodriguez, Tom St Denis, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW


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

Thanks.  Pushed it.


Tom


________________________________
From: Andres Rodriguez <andresx7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Sent: Sunday, February 5, 2017 17:28
To: Tom St Denis; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Cc: StDenis, Tom
Subject: Re: [PATCH] Autodetect libdrm path (v2)

Reviewed-by: Andres Rodriguez<andresx7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On 2/5/2017 5:24 PM, Tom St Denis wrote:
> (v2):  Use findLibDRM script instead of directly finding path
>
> Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org>
> ---
>  CMakeLists.txt                 |  3 +++
>  cmake_modules/FindLibDRM.cmake | 35 +++++++++++++++++++++++++++++++++++
>  src/lib/query_drm.c            |  4 ++--
>  3 files changed, 40 insertions(+), 2 deletions(-)
>  create mode 100644 cmake_modules/FindLibDRM.cmake
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index bef94fdba788..ef78c97ad763 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -25,6 +25,9 @@ include_directories(${CURSES_INCLUDE_DIRS})
>  find_package(PCIAccess REQUIRED)
>  include_directories(${PCIACCESS_INCLUDE_DIR})
>
> +find_package(LibDRM REQUIRED)
> +include_directories(${LIBDRM_INCLUDE_DIR})
> +
>  set(REQUIRED_EXTERNAL_LIBS
>    ${CURSES_LIBRARIES}
>    ${PCIACCESS_LIBRARIES}
> diff --git a/cmake_modules/FindLibDRM.cmake b/cmake_modules/FindLibDRM.cmake
> new file mode 100644
> index 000000000000..e840c4d1bfd0
> --- /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)
> +
> +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
> +)
> +
> +SET(LIBDRM_LIBRARIES optimized ${LIBDRM_LIBRARY})
> +
> +include(FindPackageHandleStandardArgs)
> +find_package_handle_standard_args(LIBDRM DEFAULT_MSG
> +     LIBDRM_LIBRARIES LIBDRM_INCLUDE_DIR
> +)
> +
> +mark_as_advanced(LIBDRM_INCLUDE_DIR LIBDRM_LIBRARIES)
> diff --git a/src/lib/query_drm.c b/src/lib/query_drm.c
> index b9d80a8fc0c8..755c65fbc662 100644
> --- a/src/lib/query_drm.c
> +++ b/src/lib/query_drm.c
> @@ -25,8 +25,8 @@
>  #include "umr.h"
>  #include <asm/ioctl.h>
>  #include <sys/ioctl.h>
> -#include <drm/drm.h>
> -#include <drm/amdgpu_drm.h>
> +#include <drm.h>
> +#include <amdgpu_drm.h>
>
>  #define DRM_IOC(dir, group, nr, size) _IOC(dir, group, nr, size)
>  #define DRM_IOC_WRITE           _IOC_WRITE
>

[-- Attachment #1.2: Type: text/html, Size: 5117 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Autodetect libdrm path (v2)
       [not found] ` <20170205222447.19945-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>
  2017-02-05 22:28   ` Andres Rodriguez
@ 2017-02-06 20:18   ` Emil Velikov
       [not found]     ` <CACvgo5230Use5sk8dm5jBbLqiJeSkgB6cDikz-N4omx-4Jgy9A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 1 reply; 10+ messages in thread
From: Emil Velikov @ 2017-02-06 20:18 UTC (permalink / raw)
  To: Tom St Denis; +Cc: Tom St Denis, amd-gfx mailing list

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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Autodetect libdrm path (v2)
       [not found]     ` <CACvgo5230Use5sk8dm5jBbLqiJeSkgB6cDikz-N4omx-4Jgy9A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-02-06 21:33       ` StDenis, Tom
       [not found]         ` <CY4PR12MB17689AEC15DEF9662A16DEA3F7400-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: StDenis, Tom @ 2017-02-06 21:33 UTC (permalink / raw)
  To: Emil Velikov, Tom St Denis; +Cc: amd-gfx mailing list


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Autodetect libdrm path (v2)
       [not found]         ` <CY4PR12MB17689AEC15DEF9662A16DEA3F7400-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2017-02-06 22:39           ` StDenis, Tom
       [not found]             ` <CY4PR12MB1768AED537A0253EB70B1E4BF7400-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: StDenis, Tom @ 2017-02-06 22:39 UTC (permalink / raw)
  To: Emil Velikov, Tom St Denis; +Cc: amd-gfx mailing list


[-- Attachment #1.1.1: Type: text/plain, Size: 3315 bytes --]

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@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)


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)

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

[-- Attachment #1.1.2: Type: text/html, Size: 5401 bytes --]

[-- Attachment #1.2: OutlookEmoji-😊.png --]
[-- Type: image/png, Size: 488 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Autodetect libdrm path (v2)
       [not found]             ` <CY4PR12MB1768AED537A0253EB70B1E4BF7400-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
@ 2017-02-07 11:02               ` Emil Velikov
       [not found]                 ` <CACvgo537ciLvg=Q6enktu6ndMSEuSCM+RpmniJdgYNgZDQjUeQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Emil Velikov @ 2017-02-07 11:02 UTC (permalink / raw)
  To: StDenis, Tom; +Cc: Tom St Denis, amd-gfx mailing list

On 6 February 2017 at 22:39, StDenis, Tom <Tom.StDenis@amd.com> wrote:
>
> Apparently I missed the bottom of your reply (all of the clients I have outlook/gmail do top post only ...)
Both Outlook and Gmail can do inline replies and plain text. There
might be some magic required for the former though :-\
I would kindly suggest using inline/text when possible.

> 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.
>
It's a common misconception, influenced by the sheer volume of copy/paste :-)

>
> 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
>
Up-to you really. FWIW using cmake/autoconf/etc. is a huge overkill.
The original makefile was missing a few things* worth 10-20 lines of
code while being noticeably smaller, quicker and easier to read ;-)

* Dependency tracking (pkg-config + foo.pc), incremental builds - ($CC
... -MM -MP ...), tarballs.

> Tom
>
> ________________________________
> 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)
>
>
> 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).
>
Must have missed this part. Thanks for the correction.

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

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Autodetect libdrm path (v2)
       [not found]                 ` <CACvgo537ciLvg=Q6enktu6ndMSEuSCM+RpmniJdgYNgZDQjUeQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-02-07 11:30                   ` Tom St Denis
       [not found]                     ` <13f9479c-1a5c-8887-858e-dadf819a7140-5C7GfCeVMHo@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Tom St Denis @ 2017-02-07 11:30 UTC (permalink / raw)
  To: Emil Velikov; +Cc: Tom St Denis, amd-gfx mailing list

On 07/02/17 06:02 AM, Emil Velikov wrote:
> On 6 February 2017 at 22:39, StDenis, Tom <Tom.StDenis@amd.com> wrote:
>>
>> Apparently I missed the bottom of your reply (all of the clients I have outlook/gmail do top post only ...)
> Both Outlook and Gmail can do inline replies and plain text. There
> might be some magic required for the former though :-\
> I would kindly suggest using inline/text when possible.

You'll be pleased to note I discovered (at some point) that AMD has 
re-enabled imap support and I can now use a more sensible client...

:-)

>> Ironically, I had the pkg_check originally but was told that's a faux-pas.
>>
> It's a common misconception, influenced by the sheer volume of copy/paste :-)
>
>>
>> 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
>>
> Up-to you really. FWIW using cmake/autoconf/etc. is a huge overkill.
> The original makefile was missing a few things* worth 10-20 lines of
> code while being noticeably smaller, quicker and easier to read ;-)

To be honest unless the cmake system gets in the way I'm happy if it 
makes others happy.  It's not easy to say the project is "open" if we 
NAK any and all harmless commits that the users advocate for.

The official stance from AMD (specifically the team I work for) is if 
cmake gets in the way of AMD work we will nuke the cmake system and go 
back to gmake (with pkg-config shell calls to get paths).

Personally I'm not a big fan of overly complicated config/build systems 
but since I don't do package building/etc for a living I'll defer to the 
opinions of others within reason.

>> Also umr can read/write registers via pci access without amdgpu loaded (handy if amdgpu fails to init properly).
>>
> Must have missed this part. Thanks for the correction.

Yup, you can force device + direct access, e.g.

umr -O use_pci -f fiji -r *.uvd5.mmFOO

Which is handy like I said if the module init fails.  Developers also 
use it in NPI bringup to poke things.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Autodetect libdrm path (v2)
       [not found]                     ` <13f9479c-1a5c-8887-858e-dadf819a7140-5C7GfCeVMHo@public.gmane.org>
@ 2017-02-07 12:41                       ` Emil Velikov
  2017-02-08  0:48                       ` Michel Dänzer
  1 sibling, 0 replies; 10+ messages in thread
From: Emil Velikov @ 2017-02-07 12:41 UTC (permalink / raw)
  To: Tom St Denis; +Cc: Tom St Denis, amd-gfx mailing list

On 7 February 2017 at 11:30, Tom St Denis <tom.stdenis@amd.com> wrote:
> On 07/02/17 06:02 AM, Emil Velikov wrote:
>>
>> On 6 February 2017 at 22:39, StDenis, Tom <Tom.StDenis@amd.com> wrote:
>>>
>>>
>>> Apparently I missed the bottom of your reply (all of the clients I have
>>> outlook/gmail do top post only ...)
>>
>> Both Outlook and Gmail can do inline replies and plain text. There
>> might be some magic required for the former though :-\
>> I would kindly suggest using inline/text when possible.
>
>
> You'll be pleased to note I discovered (at some point) that AMD has
> re-enabled imap support and I can now use a more sensible client...
>
> :-)
>
Amazing news, thank you !

>>> Ironically, I had the pkg_check originally but was told that's a
>>> faux-pas.
>>>
>> It's a common misconception, influenced by the sheer volume of copy/paste
>> :-)
>>
>>>
>>> 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
>>>
>> Up-to you really. FWIW using cmake/autoconf/etc. is a huge overkill.
>> The original makefile was missing a few things* worth 10-20 lines of
>> code while being noticeably smaller, quicker and easier to read ;-)
>
>
> To be honest unless the cmake system gets in the way I'm happy if it makes
> others happy.  It's not easy to say the project is "open" if we NAK any and
> all harmless commits that the users advocate for.
>
> The official stance from AMD (specifically the team I work for) is if cmake
> gets in the way of AMD work we will nuke the cmake system and go back to
> gmake (with pkg-config shell calls to get paths).
>
> Personally I'm not a big fan of overly complicated config/build systems but
> since I don't do package building/etc for a living I'll defer to the
> opinions of others within reason.
>
Bth, you're not missing anything much by not tinkering with
autoconf/cmake/etc. It's just a bikeshed fest with overly biased
people on each side ;-)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Autodetect libdrm path (v2)
       [not found]                     ` <13f9479c-1a5c-8887-858e-dadf819a7140-5C7GfCeVMHo@public.gmane.org>
  2017-02-07 12:41                       ` Emil Velikov
@ 2017-02-08  0:48                       ` Michel Dänzer
  1 sibling, 0 replies; 10+ messages in thread
From: Michel Dänzer @ 2017-02-08  0:48 UTC (permalink / raw)
  To: Tom St Denis, Emil Velikov; +Cc: Tom St Denis, amd-gfx mailing list

On 07/02/17 08:30 PM, Tom St Denis wrote:
> On 07/02/17 06:02 AM, Emil Velikov wrote:
>> On 6 February 2017 at 22:39, StDenis, Tom <Tom.StDenis@amd.com> wrote:
>>>
>>> Apparently I missed the bottom of your reply (all of the clients I
>>> have outlook/gmail do top post only ...)
>> Both Outlook and Gmail can do inline replies and plain text. There
>> might be some magic required for the former though :-\
>> I would kindly suggest using inline/text when possible.
> 
> You'll be pleased to note I discovered (at some point) that AMD has
> re-enabled imap support

FWIW, I've always used IMAP for AMD e-mail, it was never disabled per se.


> and I can now use a more sensible client...
> 
> :-)

Anyway, that's great.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-02-08  0:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-05 22:24 [PATCH] Autodetect libdrm path (v2) Tom St Denis
     [not found] ` <20170205222447.19945-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>
2017-02-05 22:28   ` Andres Rodriguez
     [not found]     ` <62ec2a45-ea86-a643-f7fd-3e5e5054540b-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-02-05 22:42       ` StDenis, Tom
2017-02-06 20:18   ` Emil Velikov
     [not found]     ` <CACvgo5230Use5sk8dm5jBbLqiJeSkgB6cDikz-N4omx-4Jgy9A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-02-06 21:33       ` StDenis, Tom
     [not found]         ` <CY4PR12MB17689AEC15DEF9662A16DEA3F7400-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2017-02-06 22:39           ` StDenis, Tom
     [not found]             ` <CY4PR12MB1768AED537A0253EB70B1E4BF7400-rpdhrqHFk06yjjPBNVDk/QdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
2017-02-07 11:02               ` Emil Velikov
     [not found]                 ` <CACvgo537ciLvg=Q6enktu6ndMSEuSCM+RpmniJdgYNgZDQjUeQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-02-07 11:30                   ` Tom St Denis
     [not found]                     ` <13f9479c-1a5c-8887-858e-dadf819a7140-5C7GfCeVMHo@public.gmane.org>
2017-02-07 12:41                       ` Emil Velikov
2017-02-08  0:48                       ` Michel Dänzer

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.