Re: [PATCH rdma-core 3/3] configure: Add check for the presence of DRM headers

From: Jason Gunthorpe <jgg@nvidia.com>
To: Jianxin Xiong <jianxin.xiong@intel.com>
Cc: <linux-rdma@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
	"Doug Ledford" <dledford@redhat.com>,
	Leon Romanovsky <leon@kernel.org>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	Christian Koenig <christian.koenig@amd.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	"Edward Srouji" <edwards@nvidia.com>,
	Yishai Hadas <yishaih@nvidia.com>,
	John Hubbard <jhubbard@nvidia.com>,
	Ali Alnubani <alialnu@nvidia.com>,
	Gal Pressman <galpress@amazon.com>,
	Emil Velikov <emil.l.velikov@gmail.com>
Subject: Re: [PATCH rdma-core 3/3] configure: Add check for the presence of DRM headers
Date: Thu, 4 Feb 2021 17:12:10 -0400	[thread overview]
Message-ID: <20210204211210.GQ4247@nvidia.com> (raw)
In-Reply-To: <1612464651-54073-4-git-send-email-jianxin.xiong@intel.com>

On Thu, Feb 04, 2021 at 10:50:51AM -0800, Jianxin Xiong wrote:
> Compilation of pyverbs/dmabuf_alloc.c depends on a few DRM headers
> that are installed by either the kernel-header or the libdrm package.
> The installation is optional and the location is not unique.
> 
> The standard locations (such as /usr/include/drm, /usr/include/libdrm)
> are checked first. If failed, pkg-config is tried to find the include
> path of custom libdrm installation. The dmabuf allocation routines now
> return suitable error when the headers are not available. The related
> tests will recognize this error code and skip.
> 
> Signed-off-by: Jianxin Xiong <jianxin.xiong@intel.com>
>  CMakeLists.txt         |  7 +++++++
>  buildlib/Finddrm.cmake | 19 +++++++++++++++++++
>  buildlib/config.h.in   |  2 ++
>  pyverbs/dmabuf_alloc.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----
>  4 files changed, 70 insertions(+), 5 deletions(-)
>  create mode 100644 buildlib/Finddrm.cmake
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 4113423..feaba3a 100644
> +++ b/CMakeLists.txt
> @@ -515,6 +515,13 @@ find_package(Systemd)
>  include_directories(${SYSTEMD_INCLUDE_DIRS})
>  RDMA_DoFixup("${SYSTEMD_FOUND}" "systemd/sd-daemon.h")
>  
> +# drm headers
> +find_package(drm)
> +if (DRM_INCLUDE_DIRS)
> +  include_directories(${DRM_INCLUDE_DIRS})
> +  set(HAVE_DRM_H 1)
> +endif()
> +
>  #-------------------------
>  # Apply fixups
>  
> diff --git a/buildlib/Finddrm.cmake b/buildlib/Finddrm.cmake
> new file mode 100644
> index 0000000..6f8e5f2
> +++ b/buildlib/Finddrm.cmake
> @@ -0,0 +1,19 @@
> +# COPYRIGHT (c) 2021 Intel Corporation.
> +# Licensed under BSD (MIT variant) or GPLv2. See COPYING.
> +
> +# Check standard locations first
> +find_path(DRM_INCLUDE_DIRS "drm.h" PATH_SUFFIXES "drm" "libdrm")
> +
> +# Check custom libdrm installation, if any
> +if (NOT DRM_INCLUDE_DIRS)
> +  execute_process(COMMAND pkg-config --cflags-only-I libdrm
> +    OUTPUT_VARIABLE _LIBDRM
> +    RESULT_VARIABLE _LIBDRM_RESULT
> +    ERROR_QUIET)
> +
> +  if (NOT _LIBDRM_RESULT)
> +    string(REGEX REPLACE "^-I" "" DRM_INCLUDE_DIRS "${_LIBDRM}")
> +  endif()
> +  unset(_LIBDRM)
> +  unset(_LIBDRM_RESULT)
> +endif()

I think this should be using pkg_check_modules() ??

Look at the NL stuff:

  pkg_check_modules(NL libnl-3.0 libnl-route-3.0 REQUIRED)
  include_directories(${NL_INCLUDE_DIRS})
  link_directories(${NL_LIBRARY_DIRS})

> +#if HAVE_DRM_H
> +

Would rather you use cmake to conditionally compile a dmabuf_alloc.c
or a dmabuf_alloc_stub.c than ifdef the entire file

Jaason