[Intel-gfx] [PATCH] Revert "drm/i915: Fix context/engine cleanup order"
Daniel Vetter
daniel.vetter at ffwll.ch
Wed Jan 27 04:48:08 PST 2016
This reverts commit 1803c035efb88afb9d3e7feb279ac29a83216382.
It seems to blow up on module unload due to a use-after free hitting a
BUG_ON with CONFIG_DEBUG_SG. Quoting from Tvrtko's mail:
"I've decoded the instructions and it pointed to SG_MAGIC checking:
488b8098010000 mov 0x198(%rax),%rax
ba21436587 mov $0x87654321,%edx
488b00 mov (%rax),%rax *** CRASH
"Grep showed 0x87654321 is SG_MAGIC, so likely candidate for this code
pattern is:
static inline struct page *sg_page(struct scatterlist *sg)
{
BUG_ON(sg->sg_magic != SG_MAGIC);
BUG_ON(sg_is_chain(sg));
return (struct page *)((sg)->page_link & ~0x3);
}
"Which would mean the offender is in intel_logical_ring_cleanup is most
likely:
...
if (ring->status_page.obj) {
kunmap(sg_page(ring->status_page.obj->pages->sgl));
ring->status_page.obj = NULL;
}
...
"I think that the i915_gem_context_fini will do a final unref on
dev_priv->kernel_context and then the ring buff has a copy which is
left dangling because:
lrc_setup_hardware_status_page(ring,
dev_priv->kernel_context->engine[ring->id].state);
and:
ring->status_page.obj = default_ctx_obj;
"Where default_ctx_obj == dev_priv->kernel_context->engine[ring->id].state
So indeed looks like the unload ordering is the trigger. In fact it
is almost the same fragility wrt/ kernel_context hidden dependency I
expressed my worry about in an e-mail yesterday or so. It only shows
if CONFIG_DEBUG_SG is set, otherwise it accesses freed memory and
probably just survives."
This causes serious trouble in our CI system since it took out all
gen8+ machines. Not yet clear why this wasn't caught in pre-merge
testing.
Backtrace from CI, for posterity:
[ 163.737836] general protection fault: 0000 [#1] PREEMPT SMP
[ 163.737849] Modules linked in: ax88179_178a usbnet mii snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915(-) x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_codec snd_hwdep snd_hda_core snd_pcm mei_me mei i2c_hid e1000e ptp pps_core [last unloaded: snd_hda_intel]
[ 163.737902] CPU: 0 PID: 5812 Comm: rmmod Tainted: G U W 4.5.0-rc1-gfxbench+ #1
[ 163.737911] Hardware name: System manufacturer System Product Name/Z170M-PLUS, BIOS 0505 11/16/2015
[ 163.737920] task: ffff8800bb99cf80 ti: ffff88022ff2c000 task.ti: ffff88022ff2c000
[ 163.737928] RIP: 0010:[<ffffffffa018f723>] [<ffffffffa018f723>] intel_logical_ring_cleanup+0x83/0x100 [i915]
[ 163.737969] RSP: 0018:ffff88022ff2fd30 EFLAGS: 00010282
[ 163.737975] RAX: 6b6b6b6b6b6b6b6b RBX: ffff8800bb2f31b8 RCX: 0000000000000002
[ 163.737982] RDX: 0000000087654321 RSI: 000000000000000d RDI: ffff8800bb2f31f0
[ 163.737989] RBP: ffff88022ff2fd40 R08: 0000000000000000 R09: 0000000000000001
[ 163.737996] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800bb2f0000
[ 163.738003] R13: ffff8800bb2f8fc8 R14: ffff8800bb285668 R15: 000055af1ae55210
[ 163.738010] FS: 00007f187014b700(0000) GS:ffff88023bc00000(0000) knlGS:0000000000000000
[ 163.738021] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 163.738030] CR2: 0000558f84e4cbc8 CR3: 000000022cd55000 CR4: 00000000003406f0
[ 163.738039] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 163.738048] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 163.738057] Stack:
[ 163.738062] ffff8800bb2f31b8 ffff8800bb2f0000 ffff88022ff2fd70 ffffffffa0180414
[ 163.738079] ffff8800bb2f0000 ffff8800bb285668 ffff8800bb2856c8 ffffffffa0242460
[ 163.738094] ffff88022ff2fd98 ffffffffa0202d30 ffff8800bb285668 ffff8800bb285668
[ 163.738109] Call Trace:
[ 163.738140] [<ffffffffa0180414>] i915_gem_cleanup_engines+0x34/0x60 [i915]
[ 163.738185] [<ffffffffa0202d30>] i915_driver_unload+0x150/0x270 [i915]
[ 163.738198] [<ffffffff815100f4>] drm_dev_unregister+0x24/0xa0
[ 163.738208] [<ffffffff815106ce>] drm_put_dev+0x1e/0x60
[ 163.738225] [<ffffffffa01412a0>] i915_pci_remove+0x10/0x20 [i915]
[ 163.738237] [<ffffffff8143d9b4>] pci_device_remove+0x34/0xb0
[ 163.738249] [<ffffffff81533d15>] __device_release_driver+0x95/0x140
[ 163.738259] [<ffffffff81533eb6>] driver_detach+0xb6/0xc0
[ 163.738268] [<ffffffff81532de3>] bus_remove_driver+0x53/0xd0
[ 163.738278] [<ffffffff815348d7>] driver_unregister+0x27/0x50
[ 163.738289] [<ffffffff8143ca15>] pci_unregister_driver+0x25/0x70
[ 163.738299] [<ffffffff81511de4>] drm_pci_exit+0x74/0x90
[ 163.738337] [<ffffffffa02034a9>] i915_exit+0x20/0x1a5 [i915]
[ 163.738349] [<ffffffff8110400f>] SyS_delete_module+0x18f/0x1f0
[ 163.738361] [<ffffffff817b8a9b>] entry_SYSCALL_64_fastpath+0x16/0x73
[ 163.738370] Code: ff d0 48 89 df e8 de a1 fd ff 48 8d 7b 38 e8 25 ab fd ff 48 8b 83 90 00 00 00 48 85 c0 74 25 48 8b 80 98 01 00 00 ba 21 43 65 87 <48> 8b 00 48 39 10 75 3c f6 40 08 01 75 38 48 c7 83 90 00 00 00
[ 163.738459] RIP [<ffffffffa018f723>] intel_logical_ring_cleanup+0x83/0x100 [i915]
[ 163.738498] RSP <ffff88022ff2fd30>
[ 163.738507] ---[ end trace 68f69ce4740fa44f ]---
Cc: Nick Hoath <nicholas.hoath at intel.com>
Cc: Dave Gordon <david.s.gordon at intel.com>
Cc: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
Tested-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
---
drivers/gpu/drm/i915/i915_dma.c | 4 ++--
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++++------------
3 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 4725e8d61d0f..d70d96fe553b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -451,8 +451,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
cleanup_gem:
mutex_lock(&dev->struct_mutex);
+ i915_gem_cleanup_ringbuffer(dev);
i915_gem_context_fini(dev);
- i915_gem_cleanup_engines(dev);
mutex_unlock(&dev->struct_mutex);
cleanup_irq:
intel_guc_ucode_fini(dev);
@@ -1196,8 +1196,8 @@ int i915_driver_unload(struct drm_device *dev)
intel_guc_ucode_fini(dev);
mutex_lock(&dev->struct_mutex);
+ i915_gem_cleanup_ringbuffer(dev);
i915_gem_context_fini(dev);
- i915_gem_cleanup_engines(dev);
mutex_unlock(&dev->struct_mutex);
intel_fbc_cleanup_cfb(dev_priv);
i915_gem_cleanup_stolen(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 01cc982f5497..211af534e5d0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3019,7 +3019,7 @@ int i915_gem_init_rings(struct drm_device *dev);
int __must_check i915_gem_init_hw(struct drm_device *dev);
int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice);
void i915_gem_init_swizzling(struct drm_device *dev);
-void i915_gem_cleanup_engines(struct drm_device *dev);
+void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
int __must_check i915_gpu_idle(struct drm_device *dev);
int __must_check i915_gem_suspend(struct drm_device *dev);
void __i915_add_request(struct drm_i915_gem_request *req,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 799a53ad04f2..371bbb28c471 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4912,7 +4912,7 @@ i915_gem_init_hw(struct drm_device *dev)
req = i915_gem_request_alloc(ring, NULL);
if (IS_ERR(req)) {
ret = PTR_ERR(req);
- i915_gem_cleanup_engines(dev);
+ i915_gem_cleanup_ringbuffer(dev);
goto out;
}
@@ -4925,7 +4925,7 @@ i915_gem_init_hw(struct drm_device *dev)
if (ret && ret != -EIO) {
DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret);
i915_gem_request_cancel(req);
- i915_gem_cleanup_engines(dev);
+ i915_gem_cleanup_ringbuffer(dev);
goto out;
}
@@ -4933,7 +4933,7 @@ i915_gem_init_hw(struct drm_device *dev)
if (ret && ret != -EIO) {
DRM_ERROR("Context enable ring #%d failed %d\n", i, ret);
i915_gem_request_cancel(req);
- i915_gem_cleanup_engines(dev);
+ i915_gem_cleanup_ringbuffer(dev);
goto out;
}
@@ -5008,7 +5008,7 @@ out_unlock:
}
void
-i915_gem_cleanup_engines(struct drm_device *dev)
+i915_gem_cleanup_ringbuffer(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_engine_cs *ring;
@@ -5017,14 +5017,13 @@ i915_gem_cleanup_engines(struct drm_device *dev)
for_each_ring(ring, dev_priv, i)
dev_priv->gt.cleanup_ring(ring);
- if (i915.enable_execlists) {
- /*
- * Neither the BIOS, ourselves or any other kernel
- * expects the system to be in execlists mode on startup,
- * so we need to reset the GPU back to legacy mode.
- */
- intel_gpu_reset(dev);
- }
+ if (i915.enable_execlists)
+ /*
+ * Neither the BIOS, ourselves or any other kernel
+ * expects the system to be in execlists mode on startup,
+ * so we need to reset the GPU back to legacy mode.
+ */
+ intel_gpu_reset(dev);
}
static void
--
2.7.0.rc3
More information about the Intel-gfx
mailing list