[Intel-gfx] [PATCH] drm/i915: make GEM_WARN_ON less terrible

[Matthew Auld]

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)
{
        return WARN_ON(v);
}

#define GEM_WARN_ON(expr) __gem_warn_on(expr)

?