Forward to ML.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
