All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON
@ 2017-04-11 13:57 Michal Wajdeczko
  2017-04-11 15:28 ` ✓ Fi.CI.BAT: success for " Patchwork
  2017-04-12 11:13 ` [RFC] " Tvrtko Ursulin
  0 siblings, 2 replies; 5+ messages in thread
From: Michal Wajdeczko @ 2017-04-11 13:57 UTC (permalink / raw)
  To: intel-gfx

This is slightly weaker version of MAYBE_BUILD_BUG_ON that allows
us to avoid adding implicit BUG but still detect as much as possible
during the build. With this new macro we can fix the problem with
GCC 4.4.7 that wrongly triggers build break in wait_for_atomic()
when invoked with non-const parameter.

Fixes: 1d1a9774 ("drm/i915: Extend intel_wait_for_register_fw() with fast timeout")
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.h  | 9 +++++++++
 drivers/gpu/drm/i915/intel_drv.h | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
index 5a49487..ce23cf1 100644
--- a/drivers/gpu/drm/i915/i915_gem.h
+++ b/drivers/gpu/drm/i915/i915_gem.h
@@ -42,6 +42,15 @@
 #define GEM_DEBUG_BUG_ON(expr)
 #endif
 
+#define GEM_MAYBE_BUILD_BUG_ON(expr) \
+	do { \
+		if (__builtin_constant_p((expr))) \
+			BUILD_BUG_ON(expr); \
+		else \
+			GEM_BUG_ON(expr); \
+	} while (0)
+
+
 #define I915_NUM_ENGINES 5
 
 #endif /* __I915_GEM_H__ */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7bc0c25..ce50ec4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -88,7 +88,7 @@
 	int cpu, ret, timeout = (US) * 1000; \
 	u64 base; \
 	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
-	BUILD_BUG_ON((US) > 50000); \
+	GEM_MAYBE_BUILD_BUG_ON((US) > 50000); \
 	if (!(ATOMIC)) { \
 		preempt_disable(); \
 		cpu = smp_processor_id(); \
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON
  2017-04-11 13:57 [RFC] drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON Michal Wajdeczko
@ 2017-04-11 15:28 ` Patchwork
  2017-04-12 11:13 ` [RFC] " Tvrtko Ursulin
  1 sibling, 0 replies; 5+ messages in thread
From: Patchwork @ 2017-04-11 15:28 UTC (permalink / raw)
  To: Michal Wajdeczko; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON
URL   : https://patchwork.freedesktop.org/series/22857/
State : success

== Summary ==

Series 22857v1 drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON
https://patchwork.freedesktop.org/api/1.0/series/22857/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:433s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:429s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:574s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:480s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:481s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:414s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:407s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:422s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:490s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:467s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:452s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:564s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:461s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:576s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:460s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:492s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:430s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:526s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:403s
fi-bxt-j4205 failed to collect. IGT log at Patchwork_4474/fi-bxt-j4205/igt.log

1a8653e657e1154a337956033af17740ef5f9dda drm-tip: 2017y-04m-11d-14h-18m-13s UTC integration manifest
205ac62 drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4474/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [RFC] drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON
  2017-04-11 13:57 [RFC] drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON Michal Wajdeczko
  2017-04-11 15:28 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-04-12 11:13 ` Tvrtko Ursulin
  2017-04-12 11:29   ` Chris Wilson
  2017-04-12 12:58   ` Michal Wajdeczko
  1 sibling, 2 replies; 5+ messages in thread
From: Tvrtko Ursulin @ 2017-04-12 11:13 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx


On 11/04/2017 14:57, Michal Wajdeczko wrote:
> This is slightly weaker version of MAYBE_BUILD_BUG_ON that allows
> us to avoid adding implicit BUG but still detect as much as possible
> during the build. With this new macro we can fix the problem with
> GCC 4.4.7 that wrongly triggers build break in wait_for_atomic()
> when invoked with non-const parameter.
> 
> Fixes: 1d1a9774 ("drm/i915: Extend intel_wait_for_register_fw() with fast timeout")
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.h  | 9 +++++++++
>  drivers/gpu/drm/i915/intel_drv.h | 2 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> index 5a49487..ce23cf1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.h
> +++ b/drivers/gpu/drm/i915/i915_gem.h
> @@ -42,6 +42,15 @@
>  #define GEM_DEBUG_BUG_ON(expr)
>  #endif
>  
> +#define GEM_MAYBE_BUILD_BUG_ON(expr) \
> +	do { \
> +		if (__builtin_constant_p((expr))) \
> +			BUILD_BUG_ON(expr); \
> +		else \
> +			GEM_BUG_ON(expr); \
> +	} while (0)
> +
> +
>  #define I915_NUM_ENGINES 5
>  
>  #endif /* __I915_GEM_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7bc0c25..ce50ec4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -88,7 +88,7 @@
>  	int cpu, ret, timeout = (US) * 1000; \
>  	u64 base; \
>  	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
> -	BUILD_BUG_ON((US) > 50000); \
> +	GEM_MAYBE_BUILD_BUG_ON((US) > 50000); \
>  	if (!(ATOMIC)) { \
>  		preempt_disable(); \
>  		cpu = smp_processor_id(); \
> 

The choice is between this and this:

-#define wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000, 1)
-#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US), 1)
+#define wait_for_atomic(COND, MS) \
+({ \
+	int ret__; \
+	BUILD_BUG_ON((MS) > 50); \
+	ret__ = _wait_for_atomic((COND), (MS) * 1000, 1); \
+	ret__; \
+})
+
+#define wait_for_atomic_us(COND, US) \
+({ \
+	int ret__; \
+	BUILD_BUG_ON((US) > 50000); \
+	ret__ = _wait_for_atomic((COND), (US), 1); \
+	ret__; \
+})

Correct? Both would fix the GCC 4.4 issue?

Regards,

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [RFC] drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON
  2017-04-12 11:13 ` [RFC] " Tvrtko Ursulin
@ 2017-04-12 11:29   ` Chris Wilson
  2017-04-12 12:58   ` Michal Wajdeczko
  1 sibling, 0 replies; 5+ messages in thread
From: Chris Wilson @ 2017-04-12 11:29 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

On Wed, Apr 12, 2017 at 12:13:51PM +0100, Tvrtko Ursulin wrote:
> 
> On 11/04/2017 14:57, Michal Wajdeczko wrote:
> > This is slightly weaker version of MAYBE_BUILD_BUG_ON that allows
> > us to avoid adding implicit BUG but still detect as much as possible
> > during the build. With this new macro we can fix the problem with
> > GCC 4.4.7 that wrongly triggers build break in wait_for_atomic()
> > when invoked with non-const parameter.
> > 
> > Fixes: 1d1a9774 ("drm/i915: Extend intel_wait_for_register_fw() with fast timeout")
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.h  | 9 +++++++++
> >  drivers/gpu/drm/i915/intel_drv.h | 2 +-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> > index 5a49487..ce23cf1 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.h
> > +++ b/drivers/gpu/drm/i915/i915_gem.h
> > @@ -42,6 +42,15 @@
> >  #define GEM_DEBUG_BUG_ON(expr)
> >  #endif
> >  
> > +#define GEM_MAYBE_BUILD_BUG_ON(expr) \
> > +	do { \
> > +		if (__builtin_constant_p((expr))) \
> > +			BUILD_BUG_ON(expr); \
> > +		else \
> > +			GEM_BUG_ON(expr); \
> > +	} while (0)
> > +
> > +
> >  #define I915_NUM_ENGINES 5
> >  
> >  #endif /* __I915_GEM_H__ */
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 7bc0c25..ce50ec4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -88,7 +88,7 @@
> >  	int cpu, ret, timeout = (US) * 1000; \
> >  	u64 base; \
> >  	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
> > -	BUILD_BUG_ON((US) > 50000); \
> > +	GEM_MAYBE_BUILD_BUG_ON((US) > 50000); \
> >  	if (!(ATOMIC)) { \
> >  		preempt_disable(); \
> >  		cpu = smp_processor_id(); \
> > 
> 
> The choice is between this and this:
> 
> -#define wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000, 1)
> -#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US), 1)
> +#define wait_for_atomic(COND, MS) \
> +({ \
> +	int ret__; \
> +	BUILD_BUG_ON((MS) > 50); \
> +	ret__ = _wait_for_atomic((COND), (MS) * 1000, 1); \
> +	ret__; \
> +})
> +
> +#define wait_for_atomic_us(COND, US) \
> +({ \
> +	int ret__; \
> +	BUILD_BUG_ON((US) > 50000); \
> +	ret__ = _wait_for_atomic((COND), (US), 1); \
> +	ret__; \
> +})
> 
> Correct? Both would fix the GCC 4.4 issue?

I was thinking this was the direction we should take as well.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [RFC] drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON
  2017-04-12 11:13 ` [RFC] " Tvrtko Ursulin
  2017-04-12 11:29   ` Chris Wilson
@ 2017-04-12 12:58   ` Michal Wajdeczko
  1 sibling, 0 replies; 5+ messages in thread
From: Michal Wajdeczko @ 2017-04-12 12:58 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

On Wed, Apr 12, 2017 at 12:13:51PM +0100, Tvrtko Ursulin wrote:
> 
> On 11/04/2017 14:57, Michal Wajdeczko wrote:
> > This is slightly weaker version of MAYBE_BUILD_BUG_ON that allows
> > us to avoid adding implicit BUG but still detect as much as possible
> > during the build. With this new macro we can fix the problem with
> > GCC 4.4.7 that wrongly triggers build break in wait_for_atomic()
> > when invoked with non-const parameter.
> > 
> > Fixes: 1d1a9774 ("drm/i915: Extend intel_wait_for_register_fw() with fast timeout")
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.h  | 9 +++++++++
> >  drivers/gpu/drm/i915/intel_drv.h | 2 +-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> > index 5a49487..ce23cf1 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.h
> > +++ b/drivers/gpu/drm/i915/i915_gem.h
> > @@ -42,6 +42,15 @@
> >  #define GEM_DEBUG_BUG_ON(expr)
> >  #endif
> >  
> > +#define GEM_MAYBE_BUILD_BUG_ON(expr) \
> > +	do { \
> > +		if (__builtin_constant_p((expr))) \
> > +			BUILD_BUG_ON(expr); \
> > +		else \
> > +			GEM_BUG_ON(expr); \
> > +	} while (0)
> > +
> > +
> >  #define I915_NUM_ENGINES 5
> >  
> >  #endif /* __I915_GEM_H__ */
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 7bc0c25..ce50ec4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -88,7 +88,7 @@
> >  	int cpu, ret, timeout = (US) * 1000; \
> >  	u64 base; \
> >  	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
> > -	BUILD_BUG_ON((US) > 50000); \
> > +	GEM_MAYBE_BUILD_BUG_ON((US) > 50000); \
> >  	if (!(ATOMIC)) { \
> >  		preempt_disable(); \
> >  		cpu = smp_processor_id(); \
> > 
> 
> The choice is between this and this:
> 
> -#define wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000, 1)
> -#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US), 1)
> +#define wait_for_atomic(COND, MS) \
> +({ \
> +	int ret__; \
> +	BUILD_BUG_ON((MS) > 50); \
> +	ret__ = _wait_for_atomic((COND), (MS) * 1000, 1); \
> +	ret__; \
> +})
> +
> +#define wait_for_atomic_us(COND, US) \
> +({ \
> +	int ret__; \
> +	BUILD_BUG_ON((US) > 50000); \
> +	ret__ = _wait_for_atomic((COND), (US), 1); \
> +	ret__; \
> +})
> 
> Correct? Both would fix the GCC 4.4 issue?

First choice should fix both current issue and shold also survive any new case
when someone decides to pass var to the wait_for_atomic() macro.

The second one, should fix current build break on GCC 4.4 as we are passing
var only to internal variant of _wait_for_atomic macro, but may fail again
on any non-const US/MS param passed to public wait_for_atomic()...
... which maybe is acceptable if we also add:

	#define wait_for_atomic_us(COND, US) \
	({ \
		int ret__; \
+		BUILD_BUG_ON(!__builtin_constant_p(US)); \
		BUILD_BUG_ON((US) > 50000); \
		ret__ = _wait_for_atomic((COND), (US), 1); \
		ret__; \
	})


-Michal

ps. as 50ms == 50000us then we can have only one macro with implementation:


	#define wait_for_atomic(COND, MS) wait_for_atomic_us((COND), (MS) * 1000)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-12 12:58 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-11 13:57 [RFC] drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON Michal Wajdeczko
2017-04-11 15:28 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-04-12 11:13 ` [RFC] " Tvrtko Ursulin
2017-04-12 11:29   ` Chris Wilson
2017-04-12 12:58   ` Michal Wajdeczko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.