On Tue, Sep 21, 2021 at 10:43:32AM -0700, Matthew Brost wrote:
From: Hugh Dickins hughd@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@google.com Signed-off-by: Matthew Brost matthew.brost@intel.com Reviewed-by: Matthew Brost matthew.brost@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