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

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

[robert.foss]

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
+
+
+
 $(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
+
 AM_CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/lib
 AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS) $(LIBUNWIND_CFLAGS) \
 	    $(WERROR_CFLAGS)
diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources
index bc4f2b5..a2742f3 100644
--- a/benchmarks/Makefile.sources
+++ b/benchmarks/Makefile.sources
@@ -1,10 +1,7 @@
 benchmarksdir=$(libexecdir)/intel-gpu-tools/benchmarks
 
+
 benchmarks_PROGRAMS =			\
-	intel_upload_blit_large         \
-	intel_upload_blit_large_gtt     \
-	intel_upload_blit_large_map     \
-	intel_upload_blit_small		\
 	gem_blt				\
 	gem_create			\
 	gem_exec_ctx			\
@@ -17,6 +14,13 @@ benchmarks_PROGRAMS =			\
 	gem_prw				\
 	gem_set_domain			\
 	gem_syslatency			\
-	gem_userptr_benchmark		\
 	kms_vblank			\
 	$(NULL)
+
+LIBDRM_INTEL_BENCHMARKS =		\
+	intel_upload_blit_large		\
+	intel_upload_blit_large_gtt	\
+	intel_upload_blit_large_map	\
+	intel_upload_blit_small		\
+	gem_userptr_benchmark		\
+	$(NULL)
-- 
2.7.4

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

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

[Robert Foss]

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

Re: [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@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

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