Re: [PATCH] drm/i915: Fix waiting for engines to idle

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Tvrtko Ursulin <tursulin@ursulin.net>,
	Intel-gfx@lists.freedesktop.org,
	Tvrtko Ursulin <tvrtko.ursulin@intel.com>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Jani Nikula <jani.nikula@linux.intel.com>,
	Nick Desaulniers <nick.desaulniers@gmail.com>
Subject: Re: [PATCH] drm/i915: Fix waiting for engines to idle
Date: Tue, 23 May 2017 10:45:44 +0100	[thread overview]
Message-ID: <ca433bee-152c-0ec3-2d60-3a2173695518@linux.intel.com> (raw)
In-Reply-To: <20170523092941.GA29386@nuc-i3427.alporthouse.com>

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