| Message ID | 20170411135756.148032-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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)
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(); \
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(-)