diff mbox

[RFC,i-g-t,2/9] benchmarks/Makefile: Don't build benchmarks that depend on libdrm_intel.

Message ID 1463785173-17557-3-git-send-email-robert.foss@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Robert Foss May 20, 2016, 10:59 p.m. UTC 
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/Makefile.sources | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Emil Velikov May 23, 2016, 2:04 p.m. UTC | #1 
On Friday, May 20, 2016 23:59 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/Makefile.sources | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources
> index 81607a5..26ee3ea 100644
> --- a/benchmarks/Makefile.sources
> +++ b/benchmarks/Makefile.sources
> @@ -1,10 +1,6 @@
>  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			\
> @@ -16,6 +12,15 @@ benchmarks_PROGRAMS =			\
>  	gem_prw				\
>  	gem_set_domain			\
>  	gem_syslatency			\
> -	gem_userptr_benchmark		\
>  	kms_vblank			\
>  	$(NULL)
> +
> +if HAVE_INTEL
> +	benchmarks_PROGRAMS +=			\
> +		intel_upload_blit_large		\
> +		intel_upload_blit_large_gtt	\
> +		intel_upload_blit_large_map	\
> +		intel_upload_blit_small		\
> +		gem_userptr_benchmark		\
> +		$(NULL)
> +endif
Some suggestions:
 - Please don't use conditionals in the Makefile.sources - you'll break the other build (Android)
 - instead of ^^ use separate variable and combine them in the Makefile.am/Android.mk
 - Don't double-indent, don't think any other place does so.

Thanks
Emil
Daniel Vetter May 24, 2016, 8:01 a.m. UTC | #2 
On Mon, May 23, 2016 at 03:04:16PM +0100, Emil Velikov wrote:
> On Friday, May 20, 2016 23:59 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/Makefile.sources | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources
> > index 81607a5..26ee3ea 100644
> > --- a/benchmarks/Makefile.sources
> > +++ b/benchmarks/Makefile.sources
> > @@ -1,10 +1,6 @@
> >  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			\
> > @@ -16,6 +12,15 @@ benchmarks_PROGRAMS =			\
> >  	gem_prw				\
> >  	gem_set_domain			\
> >  	gem_syslatency			\
> > -	gem_userptr_benchmark		\
> >  	kms_vblank			\
> >  	$(NULL)
> > +
> > +if HAVE_INTEL
> > +	benchmarks_PROGRAMS +=			\
> > +		intel_upload_blit_large		\
> > +		intel_upload_blit_large_gtt	\
> > +		intel_upload_blit_large_map	\
> > +		intel_upload_blit_small		\
> > +		gem_userptr_benchmark		\
> > +		$(NULL)
> > +endif
> Some suggestions:
>  - Please don't use conditionals in the Makefile.sources - you'll break the other build (Android)
>  - instead of ^^ use separate variable and combine them in the Makefile.am/Android.mk
>  - Don't double-indent, don't think any other place does so.

Atm the other build is Intel-only, so I don't think it's that bad really -
if we add a #define HAVE_INTEL there before including Makefile.sources (or
whatever the make magic for this is). If someone wants to build igt on
other Android machines that needs some extensions, but hey not my problem
when Android wants to reinvent half of autoconf ;-)
-Daniel
Robert Foss May 25, 2016, 5:47 p.m. UTC | #3 
On 2016-05-24 04:01 AM, Daniel Vetter wrote:
> On Mon, May 23, 2016 at 03:04:16PM +0100, Emil Velikov wrote:
>> On Friday, May 20, 2016 23:59 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/Makefile.sources | 15 ++++++++++-----
>>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources
>>> index 81607a5..26ee3ea 100644
>>> --- a/benchmarks/Makefile.sources
>>> +++ b/benchmarks/Makefile.sources
>>> @@ -1,10 +1,6 @@
>>>   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			\
>>> @@ -16,6 +12,15 @@ benchmarks_PROGRAMS =			\
>>>   	gem_prw				\
>>>   	gem_set_domain			\
>>>   	gem_syslatency			\
>>> -	gem_userptr_benchmark		\
>>>   	kms_vblank			\
>>>   	$(NULL)
>>> +
>>> +if HAVE_INTEL
>>> +	benchmarks_PROGRAMS +=			\
>>> +		intel_upload_blit_large		\
>>> +		intel_upload_blit_large_gtt	\
>>> +		intel_upload_blit_large_map	\
>>> +		intel_upload_blit_small		\
>>> +		gem_userptr_benchmark		\
>>> +		$(NULL)
>>> +endif
>> Some suggestions:
>>   - Please don't use conditionals in the Makefile.sources - you'll break the other build (Android)
>>   - instead of ^^ use separate variable and combine them in the Makefile.am/Android.mk
>>   - Don't double-indent, don't think any other place does so.
>
> Atm the other build is Intel-only, so I don't think it's that bad really -
> if we add a #define HAVE_INTEL there before including Makefile.sources (or
> whatever the make magic for this is). If someone wants to build igt on
> other Android machines that needs some extensions, but hey not my problem
> when Android wants to reinvent half of autoconf ;-)
> -Daniel
>
For v2 I've implemented the changes as suggested by Emil, I've also 
added the (hopefully) equivalent changes to Arduino.mk.
diff mbox

Patch

diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources
index 81607a5..26ee3ea 100644
--- a/benchmarks/Makefile.sources
+++ b/benchmarks/Makefile.sources
@@ -1,10 +1,6 @@ 
 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			\
@@ -16,6 +12,15 @@  benchmarks_PROGRAMS =			\
 	gem_prw				\
 	gem_set_domain			\
 	gem_syslatency			\
-	gem_userptr_benchmark		\
 	kms_vblank			\
 	$(NULL)
+
+if HAVE_INTEL
+	benchmarks_PROGRAMS +=			\
+		intel_upload_blit_large		\
+		intel_upload_blit_large_gtt	\
+		intel_upload_blit_large_map	\
+		intel_upload_blit_small		\
+		gem_userptr_benchmark		\
+		$(NULL)
+endif