* [PATCH] drm/i915: Fix waiting for engines to idle
@ 2017-05-23 9:19 Tvrtko Ursulin
2017-05-23 9:29 ` Chris Wilson
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-05-23 9:19 UTC (permalink / raw)
To: Intel-gfx; +Cc: intel-gfx, Nick Desaulniers, Daniel Vetter
From: Tvrtko Ursulin <tvrtko.ursulin@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@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@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Reported-by: Nick Desaulniers <nick.desaulniers@gmail.com>
Cc: Nick Desaulniers <nick.desaulniers@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);
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)
#define GEM_DEBUG_DECL(var)
#define GEM_DEBUG_EXEC(expr) do { } while (0)
--
2.9.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 9:19 [PATCH] drm/i915: Fix waiting for engines to idle Tvrtko Ursulin @ 2017-05-23 9:29 ` Chris Wilson 2017-05-23 9:45 ` Tvrtko Ursulin 2017-05-23 9:56 ` Jani Nikula 2017-05-23 9:36 ` ✓ Fi.CI.BAT: success for " Patchwork ` (2 subsequent siblings) 3 siblings, 2 replies; 17+ messages in thread From: Chris Wilson @ 2017-05-23 9:29 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: Intel-gfx, Nick Desaulniers, Daniel Vetter On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@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@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@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: intel-gfx@lists.freedesktop.org > Reported-by: Nick Desaulniers <nick.desaulniers@gmail.com> > Cc: Nick Desaulniers <nick.desaulniers@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@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) ? -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 9:29 ` Chris Wilson @ 2017-05-23 9:45 ` Tvrtko Ursulin 2017-05-23 9:51 ` Chris Wilson 2017-05-23 9:56 ` Jani Nikula 1 sibling, 1 reply; 17+ messages in thread From: Tvrtko Ursulin @ 2017-05-23 9:45 UTC (permalink / raw) To: Chris Wilson, Tvrtko Ursulin, Intel-gfx, Tvrtko Ursulin, Joonas Lahtinen, Daniel Vetter, Jani Nikula, Nick Desaulniers 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@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@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@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: intel-gfx@lists.freedesktop.org >> Reported-by: Nick Desaulniers <nick.desaulniers@gmail.com> >> Cc: Nick Desaulniers <nick.desaulniers@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@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... Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 9:45 ` Tvrtko Ursulin @ 2017-05-23 9:51 ` Chris Wilson 0 siblings, 0 replies; 17+ messages in thread From: Chris Wilson @ 2017-05-23 9:51 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: Intel-gfx, Nick Desaulniers, Daniel Vetter 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@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@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@chris-wilson.co.uk> > >>Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >>Cc: Daniel Vetter <daniel.vetter@intel.com> > >>Cc: Jani Nikula <jani.nikula@linux.intel.com> > >>Cc: intel-gfx@lists.freedesktop.org > >>Reported-by: Nick Desaulniers <nick.desaulniers@gmail.com> > >>Cc: Nick Desaulniers <nick.desaulniers@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@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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 9:29 ` Chris Wilson 2017-05-23 9:45 ` Tvrtko Ursulin @ 2017-05-23 9:56 ` Jani Nikula 2017-05-23 10:05 ` Tvrtko Ursulin 1 sibling, 1 reply; 17+ messages in thread From: Jani Nikula @ 2017-05-23 9:56 UTC (permalink / raw) To: Chris Wilson, Tvrtko Ursulin; +Cc: Daniel Vetter, Intel-gfx, Nick Desaulniers On Tue, 23 May 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@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@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@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: intel-gfx@lists.freedesktop.org >> Reported-by: Nick Desaulniers <nick.desaulniers@gmail.com> >> Cc: Nick Desaulniers <nick.desaulniers@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@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) 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 Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 9:56 ` Jani Nikula @ 2017-05-23 10:05 ` Tvrtko Ursulin 2017-05-23 11:30 ` Jani Nikula 0 siblings, 1 reply; 17+ messages in thread From: Tvrtko Ursulin @ 2017-05-23 10:05 UTC (permalink / raw) To: Jani Nikula, Chris Wilson, Tvrtko Ursulin Cc: Daniel Vetter, Intel-gfx, Nick Desaulniers On 23/05/2017 10:56, Jani Nikula wrote: > On Tue, 23 May 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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@chris-wilson.co.uk> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> Cc: Daniel Vetter <daniel.vetter@intel.com> >>> Cc: Jani Nikula <jani.nikula@linux.intel.com> >>> Cc: intel-gfx@lists.freedesktop.org >>> Reported-by: Nick Desaulniers <nick.desaulniers@gmail.com> >>> Cc: Nick Desaulniers <nick.desaulniers@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@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) > > 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. Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 10:05 ` Tvrtko Ursulin @ 2017-05-23 11:30 ` Jani Nikula 2017-05-24 9:11 ` Tvrtko Ursulin 0 siblings, 1 reply; 17+ messages in thread From: Jani Nikula @ 2017-05-23 11:30 UTC (permalink / raw) To: Tvrtko Ursulin, Chris Wilson, Tvrtko Ursulin Cc: Daniel Vetter, Intel-gfx, Nick Desaulniers On Tue, 23 May 2017, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 23/05/2017 10:56, Jani Nikula wrote: >> On Tue, 23 May 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>> On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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@chris-wilson.co.uk> >>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>>> Cc: Daniel Vetter <daniel.vetter@intel.com> >>>> Cc: Jani Nikula <jani.nikula@linux.intel.com> >>>> Cc: intel-gfx@lists.freedesktop.org >>>> Reported-by: Nick Desaulniers <nick.desaulniers@gmail.com> >>>> Cc: Nick Desaulniers <nick.desaulniers@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@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) >> >> 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. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 11:30 ` Jani Nikula @ 2017-05-24 9:11 ` Tvrtko Ursulin 2017-05-24 11:39 ` Jani Nikula 0 siblings, 1 reply; 17+ messages in thread From: Tvrtko Ursulin @ 2017-05-24 9:11 UTC (permalink / raw) To: Jani Nikula, Chris Wilson, Tvrtko Ursulin Cc: Daniel Vetter, Intel-gfx, Nick Desaulniers On 23/05/2017 12:30, Jani Nikula wrote: > On Tue, 23 May 2017, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> On 23/05/2017 10:56, Jani Nikula wrote: >>> On Tue, 23 May 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>>> On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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@chris-wilson.co.uk> >>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>>>> Cc: Daniel Vetter <daniel.vetter@intel.com> >>>>> Cc: Jani Nikula <jani.nikula@linux.intel.com> >>>>> Cc: intel-gfx@lists.freedesktop.org >>>>> Reported-by: Nick Desaulniers <nick.desaulniers@gmail.com> >>>>> Cc: Nick Desaulniers <nick.desaulniers@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@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) >>> >>> 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 Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-24 9:11 ` Tvrtko Ursulin @ 2017-05-24 11:39 ` Jani Nikula 0 siblings, 0 replies; 17+ messages in thread From: Jani Nikula @ 2017-05-24 11:39 UTC (permalink / raw) To: Tvrtko Ursulin, Chris Wilson, Tvrtko Ursulin Cc: Daniel Vetter, Intel-gfx, Nick Desaulniers On Wed, 24 May 2017, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 23/05/2017 12:30, Jani Nikula wrote: >> On Tue, 23 May 2017, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >>> On 23/05/2017 10:56, Jani Nikula wrote: >>>> On Tue, 23 May 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>>>> On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: >>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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@chris-wilson.co.uk> >>>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>>>>> Cc: Daniel Vetter <daniel.vetter@intel.com> >>>>>> Cc: Jani Nikula <jani.nikula@linux.intel.com> >>>>>> Cc: intel-gfx@lists.freedesktop.org >>>>>> Reported-by: Nick Desaulniers <nick.desaulniers@gmail.com> >>>>>> Cc: Nick Desaulniers <nick.desaulniers@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@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) >>>> >>>> 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 Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Fix waiting for engines to idle 2017-05-23 9:19 [PATCH] drm/i915: Fix waiting for engines to idle Tvrtko Ursulin 2017-05-23 9:29 ` Chris Wilson @ 2017-05-23 9:36 ` Patchwork 2017-05-23 9:36 ` [PATCH] " Chris Wilson 2017-05-24 0:45 ` Nick Desaulniers 3 siblings, 0 replies; 17+ messages in thread From: Patchwork @ 2017-05-23 9:36 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: intel-gfx == Series Details == Series: drm/i915: Fix waiting for engines to idle URL : https://patchwork.freedesktop.org/series/24821/ State : success == Summary == Series 24821v1 drm/i915: Fix waiting for engines to idle https://patchwork.freedesktop.org/api/1.0/series/24821/revisions/1/mbox/ Test kms_cursor_legacy: Subgroup basic-busy-flip-before-cursor-legacy: pass -> FAIL (fi-snb-2600) fdo#100215 fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:451s fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:433s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:586s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:512s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:491s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:495s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:413s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:413s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:419s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:491s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:472s fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:468s fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:564s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:460s fi-skl-6700hq total:278 pass:260 dwarn:1 dfail:0 fail:0 skip:17 time:576s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:468s fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:508s fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:439s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:530s fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time:415s f0db4d0afaead343e29f6c4609d4b912ad3304c1 drm-tip: 2017y-05m-23d-07h-43m-17s UTC integration manifest f7002d6 drm/i915: Fix waiting for engines to idle == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4782/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 9:19 [PATCH] drm/i915: Fix waiting for engines to idle Tvrtko Ursulin 2017-05-23 9:29 ` Chris Wilson 2017-05-23 9:36 ` ✓ Fi.CI.BAT: success for " Patchwork @ 2017-05-23 9:36 ` Chris Wilson 2017-05-23 10:09 ` Chris Wilson 2017-05-24 0:45 ` Nick Desaulniers 3 siblings, 1 reply; 17+ messages in thread From: Chris Wilson @ 2017-05-23 9:36 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: Intel-gfx, Nick Desaulniers, Daniel Vetter On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> > Fixes: 25112b64b3d2 ("drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()") I would also say Fixes: 7a453fb82f86 ("drm/i915: Remove redudant wait for each engine to idle from seqno wrap") as that's the patch that assumes wait_for_idle included the wait on engines. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 9:36 ` [PATCH] " Chris Wilson @ 2017-05-23 10:09 ` Chris Wilson 2017-05-23 10:30 ` Chris Wilson 0 siblings, 1 reply; 17+ messages in thread From: Chris Wilson @ 2017-05-23 10:09 UTC (permalink / raw) To: Tvrtko Ursulin, Intel-gfx, Tvrtko Ursulin, Joonas Lahtinen, Daniel Vetter, Jani Nikula, Nick Desaulniers On Tue, May 23, 2017 at 10:36:34AM +0100, Chris Wilson wrote: > On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> > > Fixes: 25112b64b3d2 ("drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()") > > I would also say > Fixes: 7a453fb82f86 ("drm/i915: Remove redudant wait for each engine to idle from seqno wrap") > as that's the patch that assumes wait_for_idle included the wait on > engines. Hmm, actually that was only to prevent a DEBUG_GEM failure so not a relevant citation for fixes? -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 10:09 ` Chris Wilson @ 2017-05-23 10:30 ` Chris Wilson 2017-05-23 10:51 ` Tvrtko Ursulin 0 siblings, 1 reply; 17+ messages in thread From: Chris Wilson @ 2017-05-23 10:30 UTC (permalink / raw) To: Tvrtko Ursulin, Intel-gfx, Tvrtko Ursulin, Joonas Lahtinen, Daniel Vetter, Jani Nikula, Nick Desaulniers On Tue, May 23, 2017 at 11:09:17AM +0100, Chris Wilson wrote: > On Tue, May 23, 2017 at 10:36:34AM +0100, Chris Wilson wrote: > > On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: > > > From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> > > > Fixes: 25112b64b3d2 ("drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()") > > > > I would also say > > Fixes: 7a453fb82f86 ("drm/i915: Remove redudant wait for each engine to idle from seqno wrap") > > as that's the patch that assumes wait_for_idle included the wait on > > engines. > > Hmm, actually that was only to prevent a DEBUG_GEM failure so not a > relevant citation for fixes? And actually, this if for debug only code so the Fixes 25112b64b3d2 is wrong as well as we as introducing a change in behaviour (making the debug only code run all the time) with no urgent need for backporting. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 10:30 ` Chris Wilson @ 2017-05-23 10:51 ` Tvrtko Ursulin 2017-05-23 11:07 ` Chris Wilson 0 siblings, 1 reply; 17+ messages in thread From: Tvrtko Ursulin @ 2017-05-23 10:51 UTC (permalink / raw) To: Chris Wilson, Tvrtko Ursulin, Intel-gfx, Tvrtko Ursulin, Joonas Lahtinen, Daniel Vetter, Jani Nikula, Nick Desaulniers On 23/05/2017 11:30, Chris Wilson wrote: > On Tue, May 23, 2017 at 11:09:17AM +0100, Chris Wilson wrote: >> On Tue, May 23, 2017 at 10:36:34AM +0100, Chris Wilson wrote: >>> On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> >>>> Fixes: 25112b64b3d2 ("drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()") >>> >>> I would also say >>> Fixes: 7a453fb82f86 ("drm/i915: Remove redudant wait for each engine to idle from seqno wrap") >>> as that's the patch that assumes wait_for_idle included the wait on >>> engines. >> >> Hmm, actually that was only to prevent a DEBUG_GEM failure so not a >> relevant citation for fixes? > > And actually, this if for debug only code so the Fixes 25112b64b3d2 is > wrong as well as we as introducing a change in behaviour (making the > debug only code run all the time) with no urgent need for backporting. Which part is for debug code only? Without it i915_gem_wait_for_idle is left with no checking for irq idleness, but it only seqno based. If that is what you say is debug only I think we need to mark it better in the code. Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 10:51 ` Tvrtko Ursulin @ 2017-05-23 11:07 ` Chris Wilson 2017-05-23 11:14 ` Tvrtko Ursulin 0 siblings, 1 reply; 17+ messages in thread From: Chris Wilson @ 2017-05-23 11:07 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: Intel-gfx, Nick Desaulniers, Daniel Vetter On Tue, May 23, 2017 at 11:51:46AM +0100, Tvrtko Ursulin wrote: > > On 23/05/2017 11:30, Chris Wilson wrote: > >On Tue, May 23, 2017 at 11:09:17AM +0100, Chris Wilson wrote: > >>On Tue, May 23, 2017 at 10:36:34AM +0100, Chris Wilson wrote: > >>>On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: > >>>>From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> > >>>>Fixes: 25112b64b3d2 ("drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()") > >>> > >>>I would also say > >>>Fixes: 7a453fb82f86 ("drm/i915: Remove redudant wait for each engine to idle from seqno wrap") > >>>as that's the patch that assumes wait_for_idle included the wait on > >>>engines. > >> > >>Hmm, actually that was only to prevent a DEBUG_GEM failure so not a > >>relevant citation for fixes? > > > >And actually, this if for debug only code so the Fixes 25112b64b3d2 is > >wrong as well as we as introducing a change in behaviour (making the > >debug only code run all the time) with no urgent need for backporting. > > Which part is for debug code only? Without it i915_gem_wait_for_idle > is left with no checking for irq idleness, but it only seqno based. > If that is what you say is debug only I think we need to mark it > better in the code. The code for waiting on engines was added purely to solve an issue created by GEM_BUG_ONs presuming a strict order between context-switch, retirement and seqno update on wrap. It is not observable, i.e. has no effect, outside of DRM_I915_DEBUG_GEM. This patch is introducing an observable effect for production systems by declaring the machine wedged, which is far more severe than the normal course of events to first try a reset. I'm suggesting that we should be in no hurry to call set-wedged here without first allowing it to percolate through linux-next. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 11:07 ` Chris Wilson @ 2017-05-23 11:14 ` Tvrtko Ursulin 0 siblings, 0 replies; 17+ messages in thread From: Tvrtko Ursulin @ 2017-05-23 11:14 UTC (permalink / raw) To: Chris Wilson, Tvrtko Ursulin, Intel-gfx, Tvrtko Ursulin, Joonas Lahtinen, Daniel Vetter, Jani Nikula, Nick Desaulniers On 23/05/2017 12:07, Chris Wilson wrote: > On Tue, May 23, 2017 at 11:51:46AM +0100, Tvrtko Ursulin wrote: >> >> On 23/05/2017 11:30, Chris Wilson wrote: >>> On Tue, May 23, 2017 at 11:09:17AM +0100, Chris Wilson wrote: >>>> On Tue, May 23, 2017 at 10:36:34AM +0100, Chris Wilson wrote: >>>>> On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote: >>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> >>>>>> Fixes: 25112b64b3d2 ("drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()") >>>>> >>>>> I would also say >>>>> Fixes: 7a453fb82f86 ("drm/i915: Remove redudant wait for each engine to idle from seqno wrap") >>>>> as that's the patch that assumes wait_for_idle included the wait on >>>>> engines. >>>> >>>> Hmm, actually that was only to prevent a DEBUG_GEM failure so not a >>>> relevant citation for fixes? >>> >>> And actually, this if for debug only code so the Fixes 25112b64b3d2 is >>> wrong as well as we as introducing a change in behaviour (making the >>> debug only code run all the time) with no urgent need for backporting. >> >> Which part is for debug code only? Without it i915_gem_wait_for_idle >> is left with no checking for irq idleness, but it only seqno based. >> If that is what you say is debug only I think we need to mark it >> better in the code. > > The code for waiting on engines was added purely to solve an issue > created by GEM_BUG_ONs presuming a strict order between context-switch, > retirement and seqno update on wrap. It is not observable, i.e. has no > effect, outside of DRM_I915_DEBUG_GEM. This patch is introducing an > observable effect for production systems by declaring the machine wedged, > which is far more severe than the normal course of events to first try a > reset. I'm suggesting that we should be in no hurry to call set-wedged > here without first allowing it to percolate through linux-next. But that means broken GEM_WARN_ON semantics were actually correct in this case, even if non-obviously so. Which is why I said which should mark it better in the code. And by that I mean the call site of wait_engines in i915_gem_wait_for_idle should be #ifdef GEM_DEBUG (I can't suggest GEM_WARN_ON!). Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix waiting for engines to idle 2017-05-23 9:19 [PATCH] drm/i915: Fix waiting for engines to idle Tvrtko Ursulin ` (2 preceding siblings ...) 2017-05-23 9:36 ` [PATCH] " Chris Wilson @ 2017-05-24 0:45 ` Nick Desaulniers 3 siblings, 0 replies; 17+ messages in thread From: Nick Desaulniers @ 2017-05-24 0:45 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: Intel-gfx, Daniel Vetter Your patch also fixes the original warning reported in: https://lkml.org/lkml/2017/5/21/1 Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-05-24 11:39 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-23 9:19 [PATCH] drm/i915: Fix waiting for engines to idle Tvrtko Ursulin 2017-05-23 9:29 ` Chris Wilson 2017-05-23 9:45 ` Tvrtko Ursulin 2017-05-23 9:51 ` Chris Wilson 2017-05-23 9:56 ` Jani Nikula 2017-05-23 10:05 ` Tvrtko Ursulin 2017-05-23 11:30 ` Jani Nikula 2017-05-24 9:11 ` Tvrtko Ursulin 2017-05-24 11:39 ` Jani Nikula 2017-05-23 9:36 ` ✓ Fi.CI.BAT: success for " Patchwork 2017-05-23 9:36 ` [PATCH] " Chris Wilson 2017-05-23 10:09 ` Chris Wilson 2017-05-23 10:30 ` Chris Wilson 2017-05-23 10:51 ` Tvrtko Ursulin 2017-05-23 11:07 ` Chris Wilson 2017-05-23 11:14 ` Tvrtko Ursulin 2017-05-24 0:45 ` Nick Desaulniers
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.