<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
</head>
<body dir="ltr">
<div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Arial,Helvetica,sans-serif;" dir="ltr">
<p>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.  </p>
<p><br>
</p>
<p>Ironically, I had the pkg_check originally but was told that's a faux-pas.</p>
<p><br>
</p>
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
<img class="EmojiInsert" id="OWAEmoji937119" alt="😊" style="vertical-align: bottom;" src="cid:1987625f-34c4-4229-b0c1-6af6053ae6be">
<div><br>
</div>
<div>Tom<br>
<br>
<div style="color: rgb(0, 0, 0);">
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of StDenis, Tom <Tom.StDenis@amd.com><br>
<b>Sent:</b> Monday, February 6, 2017 16:33<br>
<b>To:</b> Emil Velikov; Tom St Denis<br>
<b>Cc:</b> amd-gfx mailing list<br>
<b>Subject:</b> Re: [PATCH] Autodetect libdrm path (v2)</font>
<div> </div>
</div>
<div>
<div id="divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:#000000; font-family:Calibri,Arial,Helvetica,sans-serif">
<p>I have to NAK that idea since we use umr on NPI work which doesn't necessarily have libdrm support.</p>
<p><br>
</p>
<p>Also umr can read/write registers via pci access without amdgpu loaded (handy if amdgpu fails to init properly). </p>
<br>
Though you are right that libdrm is technically a requirement and I should add that to the README.
<div><br>
</div>
<div>Tom<br>
<div style="color:rgb(0,0,0)">
<div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Emil Velikov <emil.l.velikov@gmail.com><br>
<b>Sent:</b> Monday, February 6, 2017 15:18<br>
<b>To:</b> Tom St Denis<br>
<b>Cc:</b> amd-gfx mailing list; StDenis, Tom<br>
<b>Subject:</b> Re: [PATCH] Autodetect libdrm path (v2)</font>
<div> </div>
</div>
</div>
<font size="2"><span style="font-size:10pt">
<div class="PlainText">Hi Tom,<br>
<br>
On 5 February 2017 at 22:24, Tom St Denis <tstdenis82@gmail.com> wrote:<br>
> (v2):  Use findLibDRM script instead of directly finding path<br>
><br>
Since the project already depends on libdrm you might want to use the<br>
drmDevice2 API and drop the iteration over all PCI devices<br>
(libpciaccess dependency).<br>
If the former is lacking something feel free to suggest/send patches ;-)<br>
<br>
The libdrm requirement was missing last time I've skimmed through the README.<br>
<br>
> Signed-off-by: Tom St Denis <tom.stdenis@amd.com><br>
> ---<br>
<br>
> --- /dev/null<br>
> +++ b/cmake_modules/FindLibDRM.cmake<br>
> @@ -0,0 +1,35 @@<br>
> +# Try to find libdrm<br>
> +#<br>
> +# Once done, this will define<br>
> +#<br>
> +# LIBDRM_FOUND<br>
> +# LIBDRM_INCLUDE_DIR<br>
> +# LIBDRM_LIBRARIES<br>
> +<br>
> +find_package(PkgConfig)<br>
> +<br>
> +pkg_check_modules(PC_LIBDRM QUIET libdrm)<br>
You really want a version check.<br>
<br>
> +<br>
> +find_path(LIBDRM_INCLUDE_DIR NAMES amdgpu_drm.h<br>
> +    HINTS<br>
> +    ${PC_LIBDRM_INCLUDEDIR}<br>
> +    ${PC_LIBDRM_INCLUDE_DIRS}<br>
> +    /usr/include<br>
> +)<br>
> +<br>
> +find_library(LIBDRM_LIBRARY NAMES libdrm_amdgpu.so.1<br>
> +    HINTS<br>
> +    ${PC_LIBDRM_LIBDIR}<br>
> +    ${PC_LIBDRM_LIBRARY_DIRS}<br>
> +    /usr/lib64<br>
> +    /usr/lib<br>
> +)<br>
> +<br>
Here is one of the things which makes me rage against cmake - no<br>
picking /usr/{include,foo} is _not_ what you want.<br>
<br>
During cross-compilation as one is missing the .pc file, the system<br>
headers/libraries will be picked. This is horribly wrong, yet rather<br>
common in cmake world.<br>
What you really want is a lovely error message to warn the user -<br>
s/OPTIONAL/REQUIRED/ will give you just that.<br>
<br>
At which point, checking for the header/library in the paths provided<br>
by the .pc file is redundant and thus nearly everything else in the<br>
file ;-)<br>
<br>
<br>
TL;Dr: All you need is pkg_check_modules(PC_LIBDRM REQUIRED libdrm>=XX).<br>
<br>
Thanks<br>
Emil<br>
</div>
</span></font></div>
</div>
</div>
</div>
</div>
</div>
</div>
</body>
</html>