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

[Robert Foss]

> On 2016-05-25 03:55 PM, Emil Velikov wrote:
>> On Wednesday, May 25, 2016 19:18 BST, robert.foss at collabora.com wrote:
>>> From: Robert Foss <robert.foss at collabora.com>
>>>
>>> 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 at collabora.com>
>>> ---
>>>   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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx