[Intel-gfx] [RFC] drm/i915: Introduce GEM_MAYBE_BUILD_BUG_ON

[Michal Wajdeczko]

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 at intel.com>
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Chris Wilson <chris at 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)