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@xxxxxxxxx>
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
