On Wed, 24 May 2017, Tvrtko Ursulin <[email protected]> wrote: > On 23/05/2017 12:30, Jani Nikula wrote: >> On Tue, 23 May 2017, Tvrtko Ursulin <[email protected]> wrote: >>> On 23/05/2017 10:56, Jani Nikula wrote: >>>> 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. >>> >>> I would also prefer to make it consistent. >>> >>> There are two other users of GEM_WARN_ON in i915_vma_bind to consider >>> what to do with, but anyway it would be a much better solution. >> >> My suggestion is to make GEM_WARN_ON and friends that are conditional to >> CONFIG_DRM_I915_DEBUG_GEM unusable as expressions. Make them fail to >> build within if (...) for both CONFIG_DRM_I915_DEBUG_GEM=y and =n. Then >> if you need that kind of construct, handle it with something like: >> >> if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) && condition) { >> GEM_WARN(...); >> ... >> } >> >> maybe wrapping that IS_ENABLED bit in a more manageable macro. > > Why not simply make it work like a normal WARN_ON? Eg: > > #ifdef ...DEBUG_GEM > #define GEM_WARN_ON(expr) WARN_ON(expr) > #else > #define GEM_WARN_ON(expr) (expr) > #endif
I thought Chris explicitly wanted it to be lighter and leave expr out completely on non-debug builds. BR, Jani. > > Regards, > > Tvrtko -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
