[Intel-gfx] [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 at linux.intel.com> wrote:
> > Quoting Jani Nikula (2018-03-19 22:21:31)
> >> On Mon, 19 Mar 2018, Matthew Auld <matthew.william.auld at gmail.com> wrote:
> >> > On 19 March 2018 at 18:17, Chris Wilson <chris at 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)
> 
> ?