On Tue, 23 May 2017, Chris Wilson <[email protected]> wrote: > On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <[email protected]> >> >> Waiting for engines needs to happen even in the non-debug builds >> so it is incorrect to wrap it in a GEM_WARN_ON. >> >> Call it unconditionally and add GEM_WARN so that the debug >> warning can still be emitted when things go bad. >> >> Signed-off-by: Tvrtko Ursulin <[email protected]> >> Fixes: 25112b64b3d2 ("drm/i915: Wait for all engines to be idle as part of >> i915_gem_wait_for_idle()") >> Cc: Chris Wilson <[email protected]> >> Cc: Joonas Lahtinen <[email protected]> >> Cc: Daniel Vetter <[email protected]> >> Cc: Jani Nikula <[email protected]> >> Cc: [email protected] >> Reported-by: Nick Desaulniers <[email protected]> >> Cc: Nick Desaulniers <[email protected]> >> --- >> drivers/gpu/drm/i915/i915_gem.c | 3 ++- >> drivers/gpu/drm/i915/i915_gem.h | 2 ++ >> 2 files changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c >> b/drivers/gpu/drm/i915/i915_gem.c >> index a637cc05cc4a..ecaa21f106c8 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -3332,7 +3332,8 @@ static int wait_for_engines(struct drm_i915_private >> *i915) >> enum intel_engine_id id; >> >> for_each_engine(engine, i915, id) { >> - if (GEM_WARN_ON(wait_for_engine(engine, 50))) { >> + if (wait_for_engine(engine, 50)) { >> + GEM_WARN(1, "%s wait for idle timeout", engine->name); > > Nice touching adding the engine->name > Reviewed-by: Chris Wilson <[email protected]> > >> i915_gem_set_wedged(i915); >> return -EIO; >> } >> diff --git a/drivers/gpu/drm/i915/i915_gem.h >> b/drivers/gpu/drm/i915/i915_gem.h >> index ee54597465b6..cefc6cf96a60 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.h >> +++ b/drivers/gpu/drm/i915/i915_gem.h >> @@ -30,6 +30,7 @@ >> #ifdef CONFIG_DRM_I915_DEBUG_GEM >> #define GEM_BUG_ON(expr) BUG_ON(expr) >> #define GEM_WARN_ON(expr) WARN_ON(expr) >> +#define GEM_WARN(condition, format, ...) WARN(condition, format, >> __VA_ARGS__) >> >> #define GEM_DEBUG_DECL(var) var >> #define GEM_DEBUG_EXEC(expr) expr >> @@ -38,6 +39,7 @@ >> #else >> #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr) >> #define GEM_WARN_ON(expr) (BUILD_BUG_ON_INVALID(expr), 0) >> +#define GEM_WARN(condition, format, ...) BUILD_BUG_ON_INVALID(condition) > > WARNs can be used as part of an if(), so perhaps > > #define GEM_WARN(condition, format, ...) (BUILD_BUG_ON_INVALID(condition), 0) > #define GEM_WARN_ON(expr) GEM_WARN((expr), 0)
Sorry, I just can't resist the "told you so" here. If you come up with a local pattern that's deceptively similar to a widely used one, with the crucial difference that you can't use anything with required side effects in it, you'll screw it up eventually. if (GEM_WARN_ON(wait_for_engine(engine, 50))) looks completely natural and "obviously correct" in code, but is dead wrong. This won't be the last time. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
