| Message ID | 20181102161232.17742-24-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/25] RFT drm/i915/execlists: Flush memory before signaling ELSQ | expand |
Quoting Chris Wilson (2018-11-02 16:12:31) > If we do a device level reset, we lose vital registers that may be in > concurrent use by userspace (i.e. the GGTT and its fencing). To be > paranoid and prevent that memory access from being corrupted, we want to > pause all other processes/threads, so that the device reset is the only > thing running on the system. If we can live with a glitch in GTT read/writes across a device level reset, we can ignore the requirement to take stop_machine, and relax the atomicity requirements. stop_machine() is quite nasty as it impacts several core systems that allocate structs inside the cpu_hotplug mutex, e.g.: <4> [1802.499647] ====================================================== <4> [1802.499649] WARNING: possible circular locking dependency detected <4> [1802.499652] 4.19.0-CI-Trybot_3176+ #1 Tainted: G U <4> [1802.499653] ------------------------------------------------------ <4> [1802.499655] drv_selftest/12037 is trying to acquire lock: <4> [1802.499657] 00000000f9309a5f (cpu_hotplug_lock.rw_sem){++++}, at: stop_machine+0x12/0x30 <4> [1802.499664] \x0abut task is already holding lock: <4> [1802.499666] 0000000036dadfbb (&dev->struct_mutex){+.+.}, at: igt_reset_queue+0x3f/0x520 [i915] <4> [1802.499698] \x0awhich lock already depends on the new lock.\x0a <4> [1802.499701] \x0athe existing dependency chain (in reverse order) is: <4> [1802.499703] \x0a-> #4 (&dev->struct_mutex){+.+.}: <4> [1802.499707] 0xffffffffa01c9337 <4> [1802.499709] 0xffffffffa01cf89c <4> [1802.499713] __do_fault+0x1b/0x80 <4> [1802.499715] __handle_mm_fault+0x8e0/0xf10 <4> [1802.499717] handle_mm_fault+0x196/0x3a0 <4> [1802.499721] __do_page_fault+0x295/0x590 <4> [1802.499724] page_fault+0x1e/0x30 <4> [1802.499726] \x0a-> #3 (&mm->mmap_sem){++++}: <4> [1802.499731] _copy_to_user+0x1e/0x70 <4> [1802.499734] perf_read+0x232/0x2a0 <4> [1802.499737] __vfs_read+0x31/0x170 <4> [1802.499739] vfs_read+0x9e/0x140 <4> [1802.499742] ksys_read+0x50/0xc0 <4> [1802.499744] do_syscall_64+0x55/0x190 <4> [1802.499758] entry_SYSCALL_64_after_hwframe+0x49/0xbe <4> [1802.499760] \x0a-> #2 (&cpuctx_mutex){+.+.}: <4> [1802.499764] perf_event_init_cpu+0x5a/0x90 <4> [1802.499767] perf_event_init+0x19d/0x1cd <4> [1802.499770] start_kernel+0x32b/0x4c0 <4> [1802.499772] secondary_startup_64+0xa4/0xb0 <4> [1802.499774] \x0a-> #1 (pmus_lock){+.+.}: <4> [1802.499777] perf_event_init_cpu+0x21/0x90 <4> [1802.499780] cpuhp_invoke_callback+0x97/0x9b0 <4> [1802.499782] _cpu_up+0xa2/0x130 <4> [1802.499784] do_cpu_up+0x91/0xc0 <4> [1802.499787] smp_init+0x5d/0xa9 <4> [1802.499790] kernel_init_freeable+0x16f/0x352 <4> [1802.499795] kernel_init+0x5/0x100 <4> [1802.499798] ret_from_fork+0x3a/0x50 <4> [1802.499812] \x0a-> #0 (cpu_hotplug_lock.rw_sem){++++}: <4> [1802.499819] cpus_read_lock+0x34/0xa0 <4> [1802.499822] stop_machine+0x12/0x30 <4> [1802.499856] i915_reset+0x1c0/0x360 [i915] <4> [1802.499898] igt_reset_queue+0x192/0x520 [i915] <4> [1802.499964] __i915_subtests+0x5e/0xf0 [i915] <4> [1802.500007] intel_hangcheck_live_selftests+0x60/0xa0 [i915] <4> [1802.500037] __run_selftests+0x10b/0x190 [i915] <4> [1802.500067] i915_live_selftests+0x2c/0x60 [i915] <4> [1802.500093] i915_pci_probe+0x50/0xa0 [i915] <4> [1802.500097] pci_device_probe+0xa1/0x130 <4> [1802.500100] really_probe+0x25d/0x3c0 <4> [1802.500102] driver_probe_device+0x10a/0x120 <4> [1802.500105] __driver_attach+0xdb/0x100 <4> [1802.500107] bus_for_each_dev+0x74/0xc0 <4> [1802.500109] bus_add_driver+0x15f/0x250 <4> [1802.500111] driver_register+0x56/0xe0 <4> [1802.500114] do_one_initcall+0x58/0x2e0 <4> [1802.500116] do_init_module+0x56/0x1ea <4> [1802.500119] load_module+0x26f5/0x29d0 <4> [1802.500122] __se_sys_finit_module+0xd3/0xf0 <4> [1802.500124] do_syscall_64+0x55/0x190 <4> [1802.500126] entry_SYSCALL_64_after_hwframe+0x49/0xbe <4> [1802.500128] \x0aother info that might help us debug this:\x0a <4> [1802.500131] Chain exists of:\x0a cpu_hotplug_lock.rw_sem --> &mm->mmap_sem --> &dev->struct_mutex\x0a <4> [1802.500137] Possible unsafe locking scenario:\x0a <4> [1802.500139] CPU0 CPU1 <4> [1802.500141] ---- ---- <4> [1802.500142] lock(&dev->struct_mutex); <4> [1802.500145] lock(&mm->mmap_sem); <4> [1802.500147] lock(&dev->struct_mutex); <4> [1802.500149] lock(cpu_hotplug_lock.rw_sem); The recursive lock here is unlikely, but could be hit if we pass a GTT address into a perf read() call. Far fetched, but not impossible. Replacing the struct_mutex to control the PTE insertion inside i915_gem_fault() all end up in the same problem where we may wait on the mutex/semaphore (e.g. via direct reclaim) and need the same mutex for the reset. -Chris
diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c index da1c8db1f93c..77e666a27c69 100644 --- a/drivers/gpu/drm/i915/i915_reset.c +++ b/drivers/gpu/drm/i915/i915_reset.c @@ -5,6 +5,7 @@ */ #include <linux/sched/mm.h> +#include <linux/stop_machine.h> #include "i915_drv.h" #include "i915_gpu_error.h" @@ -428,22 +429,6 @@ int intel_gpu_reset(struct drm_i915_private *i915, unsigned int engine_mask) int retry; int ret; - /* - * We want to perform per-engine reset from atomic context (e.g. - * softirq), which imposes the constraint that we cannot sleep. - * However, experience suggests that spending a bit of time waiting - * for a reset helps in various cases, so for a full-device reset - * we apply the opposite rule and wait if we want to. As we should - * always follow up a failed per-engine reset with a full device reset, - * being a little faster, stricter and more error prone for the - * atomic case seems an acceptable compromise. - * - * Unfortunately this leads to a bimodal routine, when the goal was - * to have a single reset function that worked for resetting any - * number of engines simultaneously. - */ - might_sleep_if(engine_mask == ALL_ENGINES); - /* * If the power well sleeps during the reset, the reset * request may be dropped and never completes (causing -EIO). @@ -475,8 +460,6 @@ int intel_gpu_reset(struct drm_i915_private *i915, unsigned int engine_mask) } if (ret != -ETIMEDOUT || engine_mask != ALL_ENGINES) break; - - cond_resched(); } intel_uncore_forcewake_put(i915, FORCEWAKE_ALL); @@ -645,12 +628,19 @@ static void reset_engine(struct intel_engine_cs *engine, engine->reset.reset(engine, rq); } -static void gt_reset(struct drm_i915_private *i915, unsigned int stalled_mask) +static int gt_reset(struct drm_i915_private *i915, unsigned int stalled_mask) { struct intel_engine_cs *engine; enum intel_engine_id id; + int err; - lockdep_assert_held(&i915->drm.struct_mutex); + /* + * Everything depends on having the GTT running, so we need to start + * there. + */ + err = i915_ggtt_enable_hw(i915); + if (err) + return err; i915_retire_requests(i915); @@ -684,6 +674,7 @@ static void gt_reset(struct drm_i915_private *i915, unsigned int stalled_mask) } i915_gem_restore_fences(i915); + return 0; } static void reset_finish_engine(struct intel_engine_cs *engine) @@ -894,6 +885,36 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915) return ret; } +struct __i915_reset { + struct drm_i915_private *i915; + unsigned int stalled_mask; +}; + +static int __i915_reset__BKL(void *data) +{ + struct __i915_reset *arg = data; + int err; + + err = intel_gpu_reset(arg->i915, ALL_ENGINES); + if (err) + return err; + + return gt_reset(arg->i915, arg->stalled_mask); +} + +static int do_reset(struct drm_i915_private *i915, unsigned int stalled_mask) +{ + struct __i915_reset arg = { i915, stalled_mask }; + int err, i; + + err = stop_machine(__i915_reset__BKL, &arg, NULL); + for (i = 0; err && i < 3; i++) { + msleep(100); + err = stop_machine(__i915_reset__BKL, &arg, NULL); + } + + return err; +} /** * i915_reset - reset chip after a hang * @i915: #drm_i915_private to reset @@ -919,7 +940,6 @@ void i915_reset(struct drm_i915_private *i915, { struct i915_gpu_error *error = &i915->gpu_error; int ret; - int i; GEM_TRACE("flags=%lx\n", error->flags); @@ -952,32 +972,11 @@ void i915_reset(struct drm_i915_private *i915, goto error; } - for (i = 0; i < 3; i++) { - ret = intel_gpu_reset(i915, ALL_ENGINES); - if (ret == 0) - break; - - msleep(100); - } - if (ret) { + if (do_reset(i915, stalled_mask)) { dev_err(i915->drm.dev, "Failed to reset chip\n"); goto taint; } - /* Ok, now get things going again... */ - - /* - * Everything depends on having the GTT running, so we need to start - * there. - */ - ret = i915_ggtt_enable_hw(i915); - if (ret) { - DRM_ERROR("Failed to re-enable GGTT following reset (%d)\n", - ret); - goto error; - } - - gt_reset(i915, stalled_mask); intel_overlay_reset(i915); /* diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c index e30910009fe3..c0b57a679f34 100644 --- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c +++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c @@ -1532,6 +1532,24 @@ static int igt_atomic_reset(void *arg) if (i915_terminally_wedged(&i915->gpu_error)) goto out; + if (intel_has_gpu_reset(i915)) { + const typeof(*phases) *p; + + for (p = phases; p->name; p++) { + GEM_TRACE("intel_gpu_reset under %s\n", p->name); + + p->critical_section_begin(); + err = intel_gpu_reset(i915, ALL_ENGINES); + p->critical_section_end(); + + if (err) { + pr_err("intel_gpu_reset failed under %s\n", + p->name); + goto out; + } + } + } + if (intel_has_reset_engine(i915)) { struct intel_engine_cs *engine; enum intel_engine_id id;
If we do a device level reset, we lose vital registers that may be in concurrent use by userspace (i.e. the GGTT and its fencing). To be paranoid and prevent that memory access from being corrupted, we want to pause all other processes/threads, so that the device reset is the only thing running on the system. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_reset.c | 85 +++++++++---------- .../gpu/drm/i915/selftests/intel_hangcheck.c | 18 ++++ 2 files changed, 60 insertions(+), 43 deletions(-)