drm/i915: GEM_WARN_ON considered harmful

From: Tvrtko Ursulin <[email protected]>

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.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Matthew Auld <[email protected]>

Cc Mika Kuoppala <[email protected]>

---
Quickly put together and compile tested only!

---
  drivers/gpu/drm/i915/i915_gem.h          | 4 +++-
  drivers/gpu/drm/i915/i915_vma.c          | 8 ++++----
  drivers/gpu/drm/i915/intel_engine_cs.c   | 8 ++++----
  drivers/gpu/drm/i915/intel_lrc.c         | 8 ++++----
  drivers/gpu/drm/i915/intel_workarounds.c | 2 +-
  5 files changed, 16 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_gem.h +3 -1

index 599c4f6eb1ea..b0e4b976880c 100644
@@ -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)

drivers/gpu/drm/i915/i915_vma.c +4 -4

index 31efc971a3a8..82652c3d1bed 100644
@@ -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;

drivers/gpu/drm/i915/intel_engine_cs.c +4 -4

index 10cd051ba29e..8dbdb18b2668 100644
@@ -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]);

@@ -399,7 +399,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);

drivers/gpu/drm/i915/intel_lrc.c +4 -4

index 19c9c46308e5..a274cc695fa2 100644
@@ -1681,7 +1681,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)) {

@@ -1720,8 +1720,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;
  		}

@@ -1730,7 +1730,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
  		wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset);
  	}
 
-	BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE);
+	GEM_BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE);
 
  	kunmap_atomic(batch);
  	if (ret)

drivers/gpu/drm/i915/intel_workarounds.c +0 -0

index 4bcdeaf8d98f..fb746c4b7c38 100644

Ping!

Any comments here?

Main goal was to allow GEM_WARN_ON as a statement, plus also protect uses in if statements, which there are some who I think don't expect the branch to completely disappear.

Regards,

Tvrtko

Quoted 159 lines · Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

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.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Matthew Auld <[email protected]>

Cc Mika Kuoppala <[email protected]>

---
Quickly put together and compile tested only!

---
  drivers/gpu/drm/i915/i915_gem.h          | 4 +++-
  drivers/gpu/drm/i915/i915_vma.c          | 8 ++++----
  drivers/gpu/drm/i915/intel_engine_cs.c   | 8 ++++----
  drivers/gpu/drm/i915/intel_lrc.c         | 8 ++++----
  drivers/gpu/drm/i915/intel_workarounds.c | 2 +-
  5 files changed, 16 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_gem.h +3 -1

index 599c4f6eb1ea..b0e4b976880c 100644
@@ -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)

drivers/gpu/drm/i915/i915_vma.c +4 -4

index 31efc971a3a8..82652c3d1bed 100644
@@ -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;

drivers/gpu/drm/i915/intel_engine_cs.c +4 -4

index 10cd051ba29e..8dbdb18b2668 100644
@@ -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]);

@@ -399,7 +399,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);

drivers/gpu/drm/i915/intel_lrc.c +4 -4

index 19c9c46308e5..a274cc695fa2 100644
@@ -1681,7 +1681,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)) {

@@ -1720,8 +1720,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;
  		}

@@ -1730,7 +1730,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
  		wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset);
  	}
 
-	BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE);
+	GEM_BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE);
 
  	kunmap_atomic(batch);
  	if (ret)

drivers/gpu/drm/i915/intel_workarounds.c +0 -0

index 4bcdeaf8d98f..fb746c4b7c38 100644

drivers/gpu/drm/i915/intel_workarounds.c struct whitelist { · line 947

Quoted 8 lines

drivers/gpu/drm/i915/intel_workarounds.c +1 -1

@@ -941,7 +941,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;

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 7 lines · Tvrtko Ursulin

Ping!

Any comments here?

Main goal was to allow GEM_WARN_ON as a statement, plus also protect uses in if statements, which there are some who I think don't expect the branch to completely disappear.

I've said before I don't like the conditional early returns vanishing depending on config options, but I've been shot down. I think this patch is an improvement.

BR,
Jani.

Quoted 176 lines

Regards,

Tvrtko

Quoted 159 lines · Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

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.

Signed-off-by Tvrtko Ursulin <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Matthew Auld <[email protected]>

Cc Mika Kuoppala <[email protected]>

---
Quickly put together and compile tested only!

---
   drivers/gpu/drm/i915/i915_gem.h          | 4 +++-
   drivers/gpu/drm/i915/i915_vma.c          | 8 ++++----
   drivers/gpu/drm/i915/intel_engine_cs.c   | 8 ++++----
   drivers/gpu/drm/i915/intel_lrc.c         | 8 ++++----
   drivers/gpu/drm/i915/intel_workarounds.c | 2 +-
   5 files changed, 16 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_gem.h +3 -1

index 599c4f6eb1ea..b0e4b976880c 100644
@@ -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)

drivers/gpu/drm/i915/i915_vma.c +4 -4

index 31efc971a3a8..82652c3d1bed 100644
@@ -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;

drivers/gpu/drm/i915/intel_engine_cs.c +4 -4

index 10cd051ba29e..8dbdb18b2668 100644
@@ -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]);

@@ -399,7 +399,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);

drivers/gpu/drm/i915/intel_lrc.c +4 -4

index 19c9c46308e5..a274cc695fa2 100644
@@ -1681,7 +1681,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)) {

@@ -1720,8 +1720,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;
   		}

@@ -1730,7 +1730,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
   		wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset);
   	}
 
-	BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE);
+	GEM_BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE);
 
   	kunmap_atomic(batch);
   	if (ret)

drivers/gpu/drm/i915/intel_workarounds.c +0 -0

index 4bcdeaf8d98f..fb746c4b7c38 100644

drivers/gpu/drm/i915/intel_workarounds.c struct whitelist { · line 947

Quoted 8 lines

drivers/gpu/drm/i915/intel_workarounds.c +1 -1

@@ -941,7 +941,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;

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx