Skip to contentSkip to conversation

drm/i915: make GEM_WARN_ON less terrible

PATCH In review Matthew Auld 6 participants 10 messages Mar 19, 2018 – Apr 4, 2018

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Matthew Auld

author
#0 lore raw

PATCH 1/1 drm/i915: make GEM_WARN_ON less terrible

GEM_WARN_ON() was originally intended to be used only as:

if (GEM_WARN_ON(expr))

	...

but it just so happens to also work as simply:

   GEM_WARN_ON(expr);

since it just wraps WARN_ON, which is a little misleading since for !DRM_I915_DEBUG_GEM builds the second case will actually break the build. Given that there are some patches floating around which seem to miss this, it probably makes sense to just make it work for both cases.

Signed-off-by Matthew Auld <[email protected]>
Cc Chris Wilson <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/i915_gem.h +1 -1
index 8922344fc21b..6a4375437b88 100644
@@ -44,7 +44,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_ON(expr) ({BUILD_BUG_ON_INVALID(expr), 0;})
 
 #define GEM_DEBUG_DECL(var)
 #define GEM_DEBUG_EXEC(expr) do { } while (0)
Signature · 6 lines
2.14.3

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#1 lore raw

Quoting Matthew Auld (2018-03-19 18:08:54)

Quoted 5 lines

GEM_WARN_ON() was originally intended to be used only as:

   if (GEM_WARN_ON(expr))
        ...
Quoted 8 lines

but it just so happens to also work as simply:

   GEM_WARN_ON(expr);

since it just wraps WARN_ON, which is a little misleading since for !DRM_I915_DEBUG_GEM builds the second case will actually break the build. Given that there are some patches floating around which seem to miss this, it probably makes sense to just make it work for both cases.

That really was quite intentional. The only time to use GEM_WARN_ON() is inside an if, otherwise what's the point?
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#2 lore raw
Quoted 10 lines · Chris Wilson

Quoting Matthew Auld (2018-03-19 18:08:54)

Quoted 9 lines

GEM_WARN_ON() was originally intended to be used only as:

   if (GEM_WARN_ON(expr))
        ...

but it just so happens to also work as simply:

   GEM_WARN_ON(expr);
Quoted 7 lines
Quoted 4 lines

since it just wraps WARN_ON, which is a little misleading since for !DRM_I915_DEBUG_GEM builds the second case will actually break the build. Given that there are some patches floating around which seem to miss this, it probably makes sense to just make it work for both cases.

That really was quite intentional. The only time to use GEM_WARN_ON() is inside an if, otherwise what's the point?

Why wouldn't we want it to behave like WARN_ON? That seems to be what people expect, since it does wrap WARN_ON, and we don't always use WARN_ON in an if...
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

maintainer
#3 lore raw
Quoted 16 lines · Matthew Auld
Quoted 15 lines · Chris Wilson

Quoting Matthew Auld (2018-03-19 18:08:54)

Quoted 13 lines

GEM_WARN_ON() was originally intended to be used only as:

   if (GEM_WARN_ON(expr))
        ...

but it just so happens to also work as simply:

   GEM_WARN_ON(expr);

since it just wraps WARN_ON, which is a little misleading since for !DRM_I915_DEBUG_GEM builds the second case will actually break the build. Given that there are some patches floating around which seem to miss this, it probably makes sense to just make it work for both cases.

Quoted 6 lines
Quoted 2 lines

That really was quite intentional. The only time to use GEM_WARN_ON() is inside an if, otherwise what's the point?

Why wouldn't we want it to behave like WARN_ON? That seems to be what people expect, since it does wrap WARN_ON, and we don't always use WARN_ON in an if...

Looking at this, I'm more baffled by GEM_WARN_ON() evaluating to expr on CONFIG_DRM_I915_DEBUG_GEM=y and 0 otherwise. That's the seriously misleading part here.

Are you sure all those if (GEM_WARN_ON(expr)) are to be ignored? I'm no expert on gem code, but I could be easily persuaded to believe not.

BR,
Jani.

PS. On the original question, if you design GEM_WARN_ON() to be used within if conditions only, I think you better squeeze in an inline function with __must_check.

Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
maintainer
#4 lore raw

Quoting Jani Nikula (2018-03-19 22:21:31)

Quoted 32 lines · Matthew Auld
Quoted 16 lines · Chris Wilson
Quoted 15 lines

Quoting Matthew Auld (2018-03-19 18:08:54)

Quoted 13 lines

GEM_WARN_ON() was originally intended to be used only as:

   if (GEM_WARN_ON(expr))
        ...

but it just so happens to also work as simply:

   GEM_WARN_ON(expr);

since it just wraps WARN_ON, which is a little misleading since for !DRM_I915_DEBUG_GEM builds the second case will actually break the build. Given that there are some patches floating around which seem to miss this, it probably makes sense to just make it work for both cases.

Quoted 6 lines
Quoted 2 lines

That really was quite intentional. The only time to use GEM_WARN_ON() is inside an if, otherwise what's the point?

Why wouldn't we want it to behave like WARN_ON? That seems to be what people expect, since it does wrap WARN_ON, and we don't always use WARN_ON in an if...

Looking at this, I'm more baffled by GEM_WARN_ON() evaluating to expr on CONFIG_DRM_I915_DEBUG_GEM=y and 0 otherwise. That's the seriously misleading part here.

Are you sure all those if (GEM_WARN_ON(expr)) are to be ignored? I'm no expert on gem code, but I could be easily persuaded to believe not.

Quoted 6 lines

BR,
Jani.

PS. On the original question, if you design GEM_WARN_ON() to be used within if conditions only, I think you better squeeze in an inline function with __must_check.

Did somebody write a patch for this?

Regards, Joonas

Quoted 8 lines
Signature · 6 lines
-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#5 lore raw
Quoted 33 lines · Joonas Lahtinen

Quoting Jani Nikula (2018-03-19 22:21:31)

Quoted 32 lines · Matthew Auld
Quoted 16 lines · Chris Wilson
Quoted 15 lines

Quoting Matthew Auld (2018-03-19 18:08:54)

Quoted 13 lines

GEM_WARN_ON() was originally intended to be used only as:

   if (GEM_WARN_ON(expr))
        ...

but it just so happens to also work as simply:

   GEM_WARN_ON(expr);

since it just wraps WARN_ON, which is a little misleading since for !DRM_I915_DEBUG_GEM builds the second case will actually break the build. Given that there are some patches floating around which seem to miss this, it probably makes sense to just make it work for both cases.

Quoted 6 lines
Quoted 2 lines

That really was quite intentional. The only time to use GEM_WARN_ON() is inside an if, otherwise what's the point?

Why wouldn't we want it to behave like WARN_ON? That seems to be what people expect, since it does wrap WARN_ON, and we don't always use WARN_ON in an if...

Looking at this, I'm more baffled by GEM_WARN_ON() evaluating to expr on CONFIG_DRM_I915_DEBUG_GEM=y and 0 otherwise. That's the seriously misleading part here.

Are you sure all those if (GEM_WARN_ON(expr)) are to be ignored? I'm no expert on gem code, but I could be easily persuaded to believe not.

Quoted 8 lines
Quoted 6 lines

BR,
Jani.

PS. On the original question, if you design GEM_WARN_ON() to be used within if conditions only, I think you better squeeze in an inline function with __must_check.

Did somebody write a patch for this?

So just something like:

inline static bool __must_check __gem_warn_on(bool v)

{
        return WARN_ON(v);
}

#define GEM_WARN_ON(expr) __gem_warn_on(expr)

?
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

maintainer
#6 lore raw

Quoting Matthew Auld (2018-04-04 13:05:23)

Quoted 42 lines
Quoted 40 lines · Joonas Lahtinen

Quoting Jani Nikula (2018-03-19 22:21:31)

Quoted 38 lines · Matthew Auld
Quoted 16 lines · Chris Wilson
Quoted 15 lines

Quoting Matthew Auld (2018-03-19 18:08:54)

Quoted 13 lines

GEM_WARN_ON() was originally intended to be used only as:

   if (GEM_WARN_ON(expr))
        ...

but it just so happens to also work as simply:

   GEM_WARN_ON(expr);

since it just wraps WARN_ON, which is a little misleading since for !DRM_I915_DEBUG_GEM builds the second case will actually break the build. Given that there are some patches floating around which seem to miss this, it probably makes sense to just make it work for both cases.

Quoted 6 lines
Quoted 2 lines

That really was quite intentional. The only time to use GEM_WARN_ON() is inside an if, otherwise what's the point?

Why wouldn't we want it to behave like WARN_ON? That seems to be what people expect, since it does wrap WARN_ON, and we don't always use WARN_ON in an if...

Looking at this, I'm more baffled by GEM_WARN_ON() evaluating to expr on CONFIG_DRM_I915_DEBUG_GEM=y and 0 otherwise. That's the seriously misleading part here.

Are you sure all those if (GEM_WARN_ON(expr)) are to be ignored? I'm no expert on gem code, but I could be easily persuaded to believe not.

BR,
Jani.

PS. On the original question, if you design GEM_WARN_ON() to be used within if conditions only, I think you better squeeze in an inline function with __must_check.

Quoted 6 lines
Quoted 1 line

Did somebody write a patch for this?

So just something like:

inline static bool __must_check __gem_warn_on(bool v) {

I wonder if all GCC versions are smart enough to eliminate code (if we make this force_inline):

	if (!CONFIG_I915_DEBUG_GEM)
		return false;
Quoted 2 lines
        return WARN_ON(v);
}

Regards, Joonas

Quoted 4 lines

#define GEM_WARN_ON(expr) __gem_warn_on(expr)

?

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

CI
#7 lore raw

✗ Fi.CI.CHECKPATCH: warning for drm/i915: make GEM_WARN_ON less terrible

In reply to Matthew Auld
== Series Details ==

== Series Details ==

Series: drm/i915: make GEM_WARN_ON less terrible
URL : https://patchwork.freedesktop.org/series/40215/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
09aaab021f8c drm/i915: make GEM_WARN_ON less terrible
-:32: ERROR:SPACING: space required after that ';' (ctx:VxV)
#32: FILE: drivers/gpu/drm/i915/i915_gem.h:47:
+#define GEM_WARN_ON(expr) ({BUILD_BUG_ON_INVALID(expr), 0;})
                                                          ^

total: 1 errors, 0 warnings, 0 checks, 8 lines checked

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

CI
#8 lore raw

✓ Fi.CI.BAT: success for drm/i915: make GEM_WARN_ON less terrible

In reply to Matthew Auld
== Series Details ==

== Series Details ==

Series: drm/i915: make GEM_WARN_ON less terrible
URL : https://patchwork.freedesktop.org/series/40215/
State : success

== Summary ==

Series 40215v1 drm/i915: make GEM_WARN_ON less terrible
https://patchwork.freedesktop.org/api/1.0/series/40215/revisions/1/mbox/

---- Known issues:

Test gem_mmap_gtt:

        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:433s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:441s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:385s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:537s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:297s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:513s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:520s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:510s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:410s
fi-cfl-s2        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:579s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:512s
fi-cnl-drrs      total:285  pass:254  dwarn:3   dfail:0   fail:0   skip:28  time:520s
fi-elk-e7500     total:285  pass:225  dwarn:1   dfail:0   fail:0   skip:59  time:429s
fi-gdg-551       total:285  pass:177  dwarn:0   dfail:0   fail:0   skip:108 time:321s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:538s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:411s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:420s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:475s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:428s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:472s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:465s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:518s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:652s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:444s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:533s
fi-skl-6700hq    total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:542s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:508s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:500s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:430s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:445s
fi-snb-2520m     total:242  pass:208  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:403s

260af42eeff094e4768265a6ec8bbcb29b87e9a0 drm-tip: 2018y-03m-19d-17h-15m-08s UTC integration manifest 09aaab021f8c drm/i915: make GEM_WARN_ON less terrible

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8401/issues.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

CI
#9 lore raw

✓ Fi.CI.IGT: success for drm/i915: make GEM_WARN_ON less terrible

In reply to Matthew Auld
== Series Details ==

== Series Details ==

Series: drm/i915: make GEM_WARN_ON less terrible
URL : https://patchwork.freedesktop.org/series/40215/
State : success

== Summary ==

---- Known issues:

Test kms_flip:

        Subgroup flip-vs-absolute-wf_vblank-interruptible:
                fail       -> PASS       (shard-hsw) fdo#100368
Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912 +1
Test kms_sysfs_edid_timing:
                warn       -> PASS       (shard-apl) fdo#100047

fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

shard-apl        total:3478 pass:1816 dwarn:1   dfail:0   fail:6   skip:1655 time:13013s
shard-hsw        total:3478 pass:1768 dwarn:1   dfail:0   fail:1   skip:1707 time:11719s
shard-snb        total:3478 pass:1358 dwarn:1   dfail:0   fail:2   skip:2117 time:7253s

Blacklisted hosts:
shard-kbl total:3478 pass:1911 dwarn:29 dfail:0 fail:8 skip:1530 time:9843s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8401/shards.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx