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

[Jani Nikula <jani.nikula@linux.intel.com>]

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