Re: [RFC i-g-t 3/7] benchmarks/Makefile: Don't build benchmarks that depend on libdrm_intel.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2016-05-25 03:55 PM, Emil Velikov wrote:
On Wednesday, May 25, 2016 19:18 BST, robert.foss@xxxxxxxxxxxxx wrote:
From: Robert Foss <robert.foss@xxxxxxxxxxxxx>

Use the HAS_INTEL automake flag to avoid building benchmarks that won't
compile unless libdrm_intel is available in the build system.

Signed-off-by: Robert Foss <robert.foss@xxxxxxxxxxxxx>
---
  benchmarks/Android.mk       |  6 ++++++
  benchmarks/Makefile.am      |  5 ++++-
  benchmarks/Makefile.sources | 14 +++++++++-----
  3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/benchmarks/Android.mk b/benchmarks/Android.mk
index 207a177..08b6816 100644
--- a/benchmarks/Android.mk
+++ b/benchmarks/Android.mk
@@ -34,4 +34,10 @@ endef

  benchmark_list := $(benchmarks_PROGRAMS)

+ifeq ($(HAVE_LIBDRM_INTEL),true)
+    benchmark_list += $(LIBDRM_INTEL_BENCHMARKS)
+endif
+
+
+
Add just one blank line. Yet again... one should set HAVE_LIBDRM_INTEL
so that things don't regress/change. Something like the following
should do it

--- a/Android.mk
+++ b/Android.mk
@@ -1,2 +1,4 @@
+HAVE_LIBDRM_INTEL := true
+
  include $(call all-named-subdir-makefiles, lib tests tools benchmarks)

I'd keep ^^ either as separate patch (1.1/7) or just fold it into 1/7
and update the commit message.

  $(foreach item,$(benchmark_list),$(eval $(call
add_benchmark,$(item))))
diff --git a/benchmarks/Makefile.am b/benchmarks/Makefile.am
index 49d2f64..7400dd0 100644
--- a/benchmarks/Makefile.am
+++ b/benchmarks/Makefile.am
@@ -1,6 +1,9 @@
-
  include Makefile.sources

+if HAVE_LIBDRM_INTEL
+    benchmarks_PROGRAMS += $(LIBDRM_INTEL_BENCHMARKS)
+endif
+
This is absolutely correct, sadly the existing Makefile.sources
(ab)use makes things hard to grasp.
The 'hard to grasp' part being - this (and other) Makefile.am files
are missing the definition of the *PROGRAMS, *LTLIBRARIES, etc.
variables thus the whole file (and this hunk and particular makes
things harder to read.

Ideally one will give clearer (non-autoconf specific) names of the
variables in the Makefile.sources and move the PROGRAMS... bits into
Makefile.am. That is rather evasive so I'm not sure if you're up for it.
Example (note it has some ~unrelated comments/nitpicks)

diff --git a/benchmarks/Android.mk b/benchmarks/Android.mk
index 207a177..dfe34bc 100644
--- a/benchmarks/Android.mk
+++ b/benchmarks/Android.mk
@@ -32,6 +32,6 @@ endef

  #================#

-benchmark_list := $(benchmarks_PROGRAMS)
+benchmark_list := $(benchmarks_prog_list)

  $(foreach item,$(benchmark_list),$(eval $(call add_benchmark,$(item))))
diff --git a/benchmarks/Makefile.am b/benchmarks/Makefile.am
index 2c2d100..5396db1 100644
--- a/benchmarks/Makefile.am
+++ b/benchmarks/Makefile.am
@@ -3,15 +3,23 @@ include Makefile.sources

  AM_CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/lib
  AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS)
$(LIBUNWIND_CFLAGS)
+
+benchmarks_PROGRAMS = $(benchmarks_prog_list)
  LDADD = $(top_builddir)/lib/libintel_tools.la

+# Sees to be unused by IGT. Add a comment and move it after gem_foo

+benchmarksdir=$(libexecdir)/intel-gpu-tools/benchmarks
  benchmarks_LTLIBRARIES = gem_exec_tracer.la
  gem_exec_tracer_la_LDFLAGS = -module -avoid-version -no-undefined
+# there should be a detection in configure.ac as some platforms don't
use/have libdl.so
  gem_exec_tracer_la_LIBADD = -ldl

+# one could/should use AX_PTHREADS in configure and
PTHREAD_CFLAGS/PTHREAD_LIBS through the project.
+# unless they use pthread and don't strictly require the locking.
then they can use the pthread-stubs package.
  gem_latency_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
  gem_latency_LDADD = $(LDADD) -lpthread
  gem_syslatency_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
  gem_syslatency_LDADD = $(LDADD) -lpthread -lrt

+# spaces around = ?
  EXTRA_DIST=README
diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources

index 81607a5..51f59e5 100644
--- a/benchmarks/Makefile.sources
+++ b/benchmarks/Makefile.sources
@@ -1,6 +1,4 @@
-benchmarksdir=$(libexecdir)/intel-gpu-tools/benchmarks
-
-benchmarks_PROGRAMS =            \
+benchmarks_prog_list =            \
      intel_upload_blit_large         \
      intel_upload_blit_large_gtt     \
      intel_upload_blit_large_map     \


--- a/benchmarks/Makefile.sources
+++ b/benchmarks/Makefile.sources
@@ -1,10 +1,7 @@
  benchmarksdir=$(libexecdir)/intel-gpu-tools/benchmarks

+
Please don't add unneeded whitespace changes.

+LIBDRM_INTEL_BENCHMARKS =        \
With the above suggestions in place this could become
intel_benchmarks_prog_list. Don't think there's a difference between
upper/lower case and/or short/long names. Go with what IGT people are
happy.

-Emil

About the LIBDRM_INTEL_BENCHMARKS -> changes (and the corresponding changes to tools/lib/etc.) this would change the Android.mk behavior too. The Android.mk changes would make it deviate from the the standard expected behaviour with having XXX_PROGRAMS being assumed to contain the program listing.

Is that a desired consequence?


_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux