Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

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

+15 -13
+3 -1
drivers/gpu/drm/i915/i915_gem.h
··· 47 47 #define GEM_DEBUG_DECL(var) var 48 48 #define GEM_DEBUG_EXEC(expr) expr 49 49 #define GEM_DEBUG_BUG_ON(expr) GEM_BUG_ON(expr) 50 + #define GEM_DEBUG_WARN_ON(expr) GEM_WARN_ON(expr) 50 51 51 52 #else 52 53 53 54 #define GEM_SHOW_DEBUG() (0) 54 55 55 56 #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr) 56 - #define GEM_WARN_ON(expr) (BUILD_BUG_ON_INVALID(expr), 0) 57 + #define GEM_WARN_ON(expr) ({ unlikely(!!(expr)); }) 57 58 58 59 #define GEM_DEBUG_DECL(var) 59 60 #define GEM_DEBUG_EXEC(expr) do { } while (0) 60 61 #define GEM_DEBUG_BUG_ON(expr) 62 + #define GEM_DEBUG_WARN_ON(expr) ({ BUILD_BUG_ON_INVALID(expr); 0; }) 61 63 #endif 62 64 63 65 #if IS_ENABLED(CONFIG_DRM_I915_TRACE_GEM)
+4 -4
drivers/gpu/drm/i915/i915_vma.c
··· 305 305 GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); 306 306 GEM_BUG_ON(vma->size > vma->node.size); 307 307 308 - if (GEM_WARN_ON(range_overflows(vma->node.start, 309 - vma->node.size, 310 - vma->vm->total))) 308 + if (GEM_DEBUG_WARN_ON(range_overflows(vma->node.start, 309 + vma->node.size, 310 + vma->vm->total))) 311 311 return -ENODEV; 312 312 313 - if (GEM_WARN_ON(!flags)) 313 + if (GEM_DEBUG_WARN_ON(!flags)) 314 314 return -EINVAL; 315 315 316 316 bind_flags = 0;
+4 -4
drivers/gpu/drm/i915/intel_engine_cs.c
··· 273 273 BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH)); 274 274 BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH)); 275 275 276 - if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) 276 + if (GEM_DEBUG_WARN_ON(info->class > MAX_ENGINE_CLASS)) 277 277 return -EINVAL; 278 278 279 - if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) 279 + if (GEM_DEBUG_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) 280 280 return -EINVAL; 281 281 282 - if (GEM_WARN_ON(dev_priv->engine_class[info->class][info->instance])) 282 + if (GEM_DEBUG_WARN_ON(dev_priv->engine_class[info->class][info->instance])) 283 283 return -EINVAL; 284 284 285 285 GEM_BUG_ON(dev_priv->engine[id]); ··· 402 402 err = -EINVAL; 403 403 err_id = id; 404 404 405 - if (GEM_WARN_ON(!init)) 405 + if (GEM_DEBUG_WARN_ON(!init)) 406 406 goto cleanup; 407 407 408 408 err = init(engine);
+3 -3
drivers/gpu/drm/i915/intel_lrc.c
··· 1515 1515 unsigned int i; 1516 1516 int ret; 1517 1517 1518 - if (GEM_WARN_ON(engine->id != RCS)) 1518 + if (GEM_DEBUG_WARN_ON(engine->id != RCS)) 1519 1519 return -EINVAL; 1520 1520 1521 1521 switch (INTEL_GEN(engine->i915)) { ··· 1554 1554 */ 1555 1555 for (i = 0; i < ARRAY_SIZE(wa_bb_fn); i++) { 1556 1556 wa_bb[i]->offset = batch_ptr - batch; 1557 - if (GEM_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, 1558 - CACHELINE_BYTES))) { 1557 + if (GEM_DEBUG_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, 1558 + CACHELINE_BYTES))) { 1559 1559 ret = -EINVAL; 1560 1560 break; 1561 1561 }
+1 -1
drivers/gpu/drm/i915/intel_workarounds.c
··· 948 948 949 949 static void whitelist_reg(struct whitelist *w, i915_reg_t reg) 950 950 { 951 - if (GEM_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS)) 951 + if (GEM_DEBUG_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS)) 952 952 return; 953 953 954 954 w->reg[w->count++] = reg;