<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <style type="text/css" style="display:none;"></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>