| Message ID | 20180319180854.12771-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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...
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.
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
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) ?
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) > > ?
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)
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(-)