[Intel-gfx] [PATCH] drm/i915: fix blank screen booting crashes

[Lucas De Marchi]

On Tue, Sep 21, 2021 at 10:43:32AM -0700, Matthew Brost wrote:
>From: Hugh Dickins <hughd at google.com>
>
>5.15-rc1 crashes with blank screen when booting up on two ThinkPads
>using i915.  Bisections converge convincingly, but arrive at different
>and surprising "culprits", none of them the actual culprit.
>
>netconsole (with init_netconsole() hacked to call i915_init() when
>logging has started, instead of by module_init()) tells the story:
>
>kernel BUG at drivers/gpu/drm/i915/i915_sw_fence.c:245!
>with RSI: ffffffff814d408b pointing to sw_fence_dummy_notify().
>I've been building with CONFIG_CC_OPTIMIZE_FOR_SIZE=y, and that
>function needs to be 4-byte aligned.
>
>v2:
> (Jani Nikula)
>  - Change BUG_ON to WARN_ON
>v3:
> (Jani / Tvrtko)
>  - Short circuit __i915_sw_fence_init on WARN_ON
>
>Fixes: 62eaf0ae217d ("drm/i915/guc: Support request cancellation")
>Signed-off-by: Hugh Dickins <hughd at google.com>
>Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>Reviewed-by: Matthew Brost <matthew.brost at intel.com>
>---
> drivers/gpu/drm/i915/gt/intel_context.c |  4 ++--
> drivers/gpu/drm/i915/i915_sw_fence.c    | 17 ++++++++++-------
> 2 files changed, 12 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
>index ff637147b1a9..e7f78bc7ebfc 100644
>--- a/drivers/gpu/drm/i915/gt/intel_context.c
>+++ b/drivers/gpu/drm/i915/gt/intel_context.c
>@@ -362,8 +362,8 @@ static int __intel_context_active(struct i915_active *active)
> 	return 0;
> }
>

>-static int sw_fence_dummy_notify(struct i915_sw_fence *sf,
>-				 enum i915_sw_fence_notify state)
>+static int __i915_sw_fence_call
>+sw_fence_dummy_notify(struct i915_sw_fence *sf, enum i915_sw_fence_notify state)
> {
> 	return NOTIFY_DONE;
> }
>diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
>index c589a681da77..08cea73264e7 100644
>--- a/drivers/gpu/drm/i915/i915_sw_fence.c
>+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
>@@ -13,9 +13,9 @@
> #include "i915_selftest.h"
>
> #if IS_ENABLED(CONFIG_DRM_I915_DEBUG)
>-#define I915_SW_FENCE_BUG_ON(expr) BUG_ON(expr)
>+#define I915_SW_FENCE_WARN_ON(expr) WARN_ON(expr)
> #else
>-#define I915_SW_FENCE_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
>+#define I915_SW_FENCE_WARN_ON(expr) BUILD_BUG_ON_INVALID(expr)
> #endif
>
> static DEFINE_SPINLOCK(i915_sw_fence_lock);
>@@ -129,7 +129,10 @@ static int __i915_sw_fence_notify(struct i915_sw_fence *fence,
> 	i915_sw_fence_notify_t fn;
>
> 	fn = (i915_sw_fence_notify_t)(fence->flags & I915_SW_FENCE_MASK);
>-	return fn(fence, state);
>+	if (likely(fn))
>+		return fn(fence, state);
>+	else
>+		return 0;

since the knowledge for these being NULL (or with the wrong alignment)
are in the init/reinit functions,  wouldn't it be better to just add a
fence_nop() and assign it there instead this likely() here?

> }
>
> #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
>@@ -242,9 +245,9 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
> 			  const char *name,
> 			  struct lock_class_key *key)
> {
>-	BUG_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK);
>-
> 	__init_waitqueue_head(&fence->wait, name, key);
>+	if (WARN_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK))
>+		return;

like:
	if (WARN_ON(!fn || (unsigned long)fn & ~I915_SW_FENCE_MASK))
		fence->flags = (unsigned long)sw_fence_dummy_notify;
	else
		fence->flags = (unsigned long)fn;


f you return here instead of calling i915_sw_fence_reinit(), aren't you
just going to use uninitialized memory later? At least in the selftests,
which allocate it with kmalloc()... I didn't check others.


For the bug fix we could just add the __aligned(4) and leave the rest to a
separate patch.


Lucas De Marchi

> 	fence->flags = (unsigned long)fn;
>
> 	i915_sw_fence_reinit(fence);
>@@ -257,8 +260,8 @@ void i915_sw_fence_reinit(struct i915_sw_fence *fence)
> 	atomic_set(&fence->pending, 1);
> 	fence->error = 0;
>
>-	I915_SW_FENCE_BUG_ON(!fence->flags);
>-	I915_SW_FENCE_BUG_ON(!list_empty(&fence->wait.head));
>+	I915_SW_FENCE_WARN_ON(!fence->flags);
>+	I915_SW_FENCE_WARN_ON(!list_empty(&fence->wait.head));
> }
>
> void i915_sw_fence_commit(struct i915_sw_fence *fence)
>-- 
>2.32.0
>