drm/i915: GEM_WARN_ON considered harmful

GEM_WARN_ON currently has dangerous semantics where it is completely
compiled out on !GEM_DEBUG builds. This can leave users who expect it to
be more like a WARN_ON, just without a warning in non-debug builds, in
complete ignorance.

Another gotcha with it is that it cannot be used as a statement. Which is
again different from a standard kernel WARN_ON.

This patch fixes both problems by making it behave as one would expect.

It can now be used both as an expression and as statement, and also the
condition evaluates properly in all builds - code under the conditional
will therefore not unexpectedly disappear.

To satisfy call sites which really want the code under the conditional to
completely disappear, we add GEM_DEBUG_WARN_ON and convert some of the
callers to it. This one can also be used as both expression and statement.

>From the above it follows GEM_DEBUG_WARN_ON should be used in situations
where we are certain the condition will be hit during development, but at
a place in code where error can be handled to the benefit of not crashing
the machine.

GEM_WARN_ON on the other hand should be used where condition may happen in
production and we just want to distinguish the level of debugging output
emitted between the production and debug build.

v2:
 * Dropped BUG_ON hunk.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tomasz Lis <tomasz.lis@intel.com>
Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20181012063142.16080-1-tvrtko.ursulin@linux.intel.com

diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
index 599c4f6eb1eab017f20ef7e06c4ce43aa817c42f..b0e4b976880c0bf6699197f4f0215e15289c23f4 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem.h
+++ b/drivers/gpu/drm/i915/i915_gem.h
@@ -47,17 +47,19 @@ struct drm_i915_private;
 #define GEM_DEBUG_DECL(var) var
 #define GEM_DEBUG_EXEC(expr) expr
 #define GEM_DEBUG_BUG_ON(expr) GEM_BUG_ON(expr)
+#define GEM_DEBUG_WARN_ON(expr) GEM_WARN_ON(expr)
 
 #else
 
 #define GEM_SHOW_DEBUG() (0)
 
 #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) ({ unlikely(!!(expr)); })
 
 #define GEM_DEBUG_DECL(var)
 #define GEM_DEBUG_EXEC(expr) do { } while (0)
 #define GEM_DEBUG_BUG_ON(expr)
+#define GEM_DEBUG_WARN_ON(expr) ({ BUILD_BUG_ON_INVALID(expr); 0; })
 #endif
 
 #if IS_ENABLED(CONFIG_DRM_I915_TRACE_GEM)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 31efc971a3a83897b545c49142fc0e1366641521..82652c3d1bed65145b465a9a6e5e244500dcc803 100644 (file)
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -305,12 +305,12 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
        GEM_BUG_ON(!drm_mm_node_allocated(&vma->node));
        GEM_BUG_ON(vma->size > vma->node.size);
 
-       if (GEM_WARN_ON(range_overflows(vma->node.start,
-                                       vma->node.size,
-                                       vma->vm->total)))
+       if (GEM_DEBUG_WARN_ON(range_overflows(vma->node.start,
+                                             vma->node.size,
+                                             vma->vm->total)))
                return -ENODEV;
 
-       if (GEM_WARN_ON(!flags))
+       if (GEM_DEBUG_WARN_ON(!flags))
                return -EINVAL;
 
        bind_flags = 0;

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index bc793b0c8806b3bd90d907215faceaeb3d21464b..8bfab22068a3fd5306176d37115f32ca5655f9b8 100644 (file)
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -273,13 +273,13 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
        BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH));
        BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH));
 
-       if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS))
+       if (GEM_DEBUG_WARN_ON(info->class > MAX_ENGINE_CLASS))
                return -EINVAL;
 
-       if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE))
+       if (GEM_DEBUG_WARN_ON(info->instance > MAX_ENGINE_INSTANCE))
                return -EINVAL;
 
-       if (GEM_WARN_ON(dev_priv->engine_class[info->class][info->instance]))
+       if (GEM_DEBUG_WARN_ON(dev_priv->engine_class[info->class][info->instance]))
                return -EINVAL;
 
        GEM_BUG_ON(dev_priv->engine[id]);
@@ -402,7 +402,7 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
                err = -EINVAL;
                err_id = id;
 
-               if (GEM_WARN_ON(!init))
+               if (GEM_DEBUG_WARN_ON(!init))
                        goto cleanup;
 
                err = init(engine);

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ff0e2b36cb8ba41e14aebab88b1437a67c5589aa..22b57b8926fc032271c41f551271a4e1f7f513d7 100644 (file)
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1515,7 +1515,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
        unsigned int i;
        int ret;
 
-       if (GEM_WARN_ON(engine->id != RCS))
+       if (GEM_DEBUG_WARN_ON(engine->id != RCS))
                return -EINVAL;
 
        switch (INTEL_GEN(engine->i915)) {
@@ -1554,8 +1554,8 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
         */
        for (i = 0; i < ARRAY_SIZE(wa_bb_fn); i++) {
                wa_bb[i]->offset = batch_ptr - batch;
-               if (GEM_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset,
-                                           CACHELINE_BYTES))) {
+               if (GEM_DEBUG_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset,
+                                                 CACHELINE_BYTES))) {
                        ret = -EINVAL;
                        break;
                }

diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
index e4136590fed9b2975bed7742eb1611c5a1521d5d..01b9b7591c5d688eba14108c0c24817be5dc6bae 100644 (file)
--- a/drivers/gpu/drm/i915/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/intel_workarounds.c
@@ -948,7 +948,7 @@ struct whitelist {
 
 static void whitelist_reg(struct whitelist *w, i915_reg_t reg)
 {
-       if (GEM_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS))
+       if (GEM_DEBUG_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS))
                return;
 
        w->reg[w->count++] = reg;