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

[Robert Foss <robert.foss@collabora.com>]

Forward to ML.

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