On Tue, 23 May 2017, Tvrtko Ursulin <[email protected]> wrote:
> On 23/05/2017 10:56, Jani Nikula wrote:
>> On Tue, 23 May 2017, Chris Wilson <[email protected]> wrote:
>>> On Tue, May 23, 2017 at 10:19:31AM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Fixes: 25112b64b3d2 ("drm/i915: Wait for all engines to be idle as part of
>>>> i915_gem_wait_for_idle()")
>>>> Cc: Chris Wilson <[email protected]>
>>>> Cc: Joonas Lahtinen <[email protected]>
>>>> Cc: Daniel Vetter <[email protected]>
>>>> Cc: Jani Nikula <[email protected]>
>>>> Cc: [email protected]
>>>> Reported-by: Nick Desaulniers <[email protected]>
>>>> Cc: Nick Desaulniers <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
>>>
>>>> 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
