[PATCH] drm/i915: make GEM_WARN_ON less terrible

[PATCH] drm/i915: make GEM_WARN_ON less terrible

[Matthew Auld]

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 <matthew.auld@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
index 8922344fc21b..6a4375437b88 100644
--- a/drivers/gpu/drm/i915/i915_gem.h
+++ b/drivers/gpu/drm/i915/i915_gem.h
@@ -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)
-- 
2.14.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: make GEM_WARN_ON less terrible

[Chris Wilson]

Quoting Matthew Auld (2018-03-19 18:08:54)
> 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.

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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Patchwork]

== 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: make GEM_WARN_ON less terrible

[Matthew Auld]

On 19 March 2018 at 18:17, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Matthew Auld (2018-03-19 18:08:54)
>> 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.
>
> 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Patchwork]

== 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: make GEM_WARN_ON less terrible

[Jani Nikula]

On Mon, 19 Mar 2018, Matthew Auld <matthew.william.auld@gmail.com> wrote:
> On 19 March 2018 at 18:17, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> Quoting Matthew Auld (2018-03-19 18:08:54)
>>> 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.
>>
>> 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.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Patchwork]

== 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: make GEM_WARN_ON less terrible

[Joonas Lahtinen]

Quoting Jani Nikula (2018-03-19 22:21:31)
> On Mon, 19 Mar 2018, Matthew Auld <matthew.william.auld@gmail.com> wrote:
> > On 19 March 2018 at 18:17, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> Quoting Matthew Auld (2018-03-19 18:08:54)
> >>> 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.
> >>
> >> 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.

Did somebody write a patch for this?

Regards, Joonas

> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: make GEM_WARN_ON less terrible

[Matthew Auld]

On 4 April 2018 at 10:13, Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
> Quoting Jani Nikula (2018-03-19 22:21:31)
>> On Mon, 19 Mar 2018, Matthew Auld <matthew.william.auld@gmail.com> wrote:
>> > On 19 March 2018 at 18:17, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> >> Quoting Matthew Auld (2018-03-19 18:08:54)
>> >>> 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.
>> >>
>> >> 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.
>
> 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: make GEM_WARN_ON less terrible

[Joonas Lahtinen]

Quoting Matthew Auld (2018-04-04 13:05:23)
> On 4 April 2018 at 10:13, Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com> wrote:
> > Quoting Jani Nikula (2018-03-19 22:21:31)
> >> On Mon, 19 Mar 2018, Matthew Auld <matthew.william.auld@gmail.com> wrote:
> >> > On 19 March 2018 at 18:17, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> >> Quoting Matthew Auld (2018-03-19 18:08:54)
> >> >>> 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.
> >> >>
> >> >> 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.
> >
> > 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;

>         return WARN_ON(v);
> }

Regards, Joonas

> 
> #define GEM_WARN_ON(expr) __gem_warn_on(expr)
> 
> ?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx