[Intel-gfx] [PATCH] drm/i915: Fix waiting for engines to idle

[Chris Wilson]

On Tue, May 23, 2017 at 10:45:44AM +0100, Tvrtko Ursulin wrote:
> 
> On 23/05/2017 10:29, Chris Wilson wrote:
> >On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >>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 <tvrtko.ursulin at intel.com>
> >>Fixes: 25112b64b3d2 ("drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()")
> >>Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >>Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >>Cc: Daniel Vetter <daniel.vetter at intel.com>
> >>Cc: Jani Nikula <jani.nikula at linux.intel.com>
> >>Cc: intel-gfx at lists.freedesktop.org
> >>Reported-by: Nick Desaulniers <nick.desaulniers at gmail.com>
> >>Cc: Nick Desaulniers <nick.desaulniers at gmail.com>
> >>---
> >> 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 <chris at chris-wilson.co.uk>
> >
> >> 			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)
> 
> Doesn't work for statements. :( I don't know, more compiler trickery
> needed...

Unless it turns out to be easy, go with simple and we can play later
when needed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre