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

[Tvrtko Ursulin]

On 23/05/2017 10:56, Jani Nikula wrote:
> On Tue, 23 May 2017, Chris Wilson <chris at chris-wilson.co.uk> 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)
>
> 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