Skip to contentSkip to conversation

drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check

PATCH In review Bin Yang 3 participants 6 messages Dec 20, 2018

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Bin Yang

author
#0 lore raw

PATCH 1/1 drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check

i915_gem_wait_for_idle() waits for all requests being completed and calls i915_retire_requests() to retire them. It assumes the active_requests should be zero finally.

In i915_retire_requests(), it will retire all requests on the active rings. Unfortunately, active_requests is increased in i915_request_alloc() and reduced in i915_request_retire(), but the request is added into active rings in i915_request_add().

If i915_gem_wait_for_idle() is called between i915_request_alloc() and i915_request_add(), this request will not be retired. Then, the active_requests will not be zero in the end.

Normally, i915_request_alloc() and i915_request_add() will be called in sequence with drm.struct_mutex locked. But in intel_vgpu_create_workload(), it will pre-allocate the request and call i915_request_add() in the workload thread for performance optimization. The above issue will be triggered.

This patch introduced a new counter named reserved_requests for request allocation. The active_requests will be increased when the request is really added into the active rings.

8<----- below is the oops when above issue is hitted.

[2018-11-28 23:17:54] [12278.310417] kernel BUG at drivers/gpu/drm/i915/i915_gem.c:4702!
[2018-11-28 23:17:54] [12278.310802] invalid opcode: 0000 [#1] PREEMPT SMP
[2018-11-28 23:17:54] [12278.311012] CPU: 0 PID: 61 Comm: kswapd0 Tainted: G U WC 4.19.0-26.iot-lts2018-sos #1
[2018-11-28 23:17:54] [12278.311393] RIP: 0010:i915_gem_wait_for_idle.part.78.cold.114+0x45/0x47
[2018-11-28 23:17:54] [12278.311675] Code: 7b 8b ae ff 48 8b 35 e6 92 3c 01 49 c7 c0 af 48 55 a9 b9 5e 12 00 00 48 c7 c2 50 7a 0b a9 48 c7 c7 f4 e6 60 a8 e8 37 38 b6 ff <0f> 0b 48 c7 c1 a8 59 55 a9 ba b8 12 00 00 48 c7 c6 20 7a 0b a9 48 [2018-11-28 23:17:55] [12278.312447] RSP: 0018:ffff8e31acd8bbb8 EFLAGS: 00010246
[2018-11-28 23:17:55] [12278.312673] RAX: 000000000000000e RBX: 000000000000000a RCX: 0000000000000000
[2018-11-28 23:17:55] [12278.312971] RDX: 0000000000000001 RSI: 0000000000000008 RDI: ffff8e31ae841400
[2018-11-28 23:17:55] [12278.313268] RBP: ffff8e31acea8340 R08: 0000000001416578 R09: ffff8e31aea15000
[2018-11-28 23:17:55] [12278.313566] R10: ffff8e31ae807100 R11: ffff8e31ae841400 R12: ffff8e31acea0000
[2018-11-28 23:17:55] [12278.313863] R13: 00000b2ab1d38ed0 R14: 0000000000000000 R15: ffff8e31acd8bd70
[2018-11-28 23:17:55] [12278.314162] FS: 0000000000000000(0000) GS:ffff8e31afa00000(0000) knlGS:0000000000000000
[2018-11-28 23:17:55] [12278.314499] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[2018-11-28 23:17:55] [12278.314741] CR2: 00007ff94948f000 CR3: 0000000226813000 CR4: 00000000003406f0
[2018-11-28 23:17:55] [12278.315039] Call Trace:

[2018-11-28 23:17:55] [12278.315162]  i915_gem_shrink+0x3b7/0x4b0
[2018-11-28 23:17:55] [12278.315340]  i915_gem_shrinker_scan+0x104/0x130
[2018-11-28 23:17:55] [12278.315537]  do_shrink_slab+0x12c/0x2c0
[2018-11-28 23:17:55] [12278.315706]  shrink_slab+0x225/0x2c0
[2018-11-28 23:17:55] [12278.315864]  shrink_node+0xe4/0x430
[2018-11-28 23:17:55] [12278.316018]  kswapd+0x3ce/0x730
[2018-11-28 23:17:55] [12278.316161]  ? mem_cgroup_shrink_node+0x1a0/0x1a0
[2018-11-28 23:17:55] [12278.316365]  kthread+0x11e/0x140
[2018-11-28 23:17:55] [12278.316508]  ? kthread_create_worker_on_cpu+0x70/0x70
[2018-11-28 23:17:55] [12278.316727]  ret_from_fork+0x3a/0x50

[2018-11-28 23:17:55] [12278.316884] Modules linked in: igb_avb(C) xhci_pci xhci_hcd dca ici_isys_mod ipu4_acpi intel_ipu4_isys_csslib intel_ipu4_psys intel_ipu4_psys_csslib intel_ipu4_mmu intel_ipu4 iova crlmodule_lite

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109005

Signed-off-by Bin Yang <[email protected]>
---
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/i915_gem.c     |  2 +-
 drivers/gpu/drm/i915/i915_request.c | 10 +++++++---
 3 files changed, 9 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +1 -0
index 815db160b966..7a757f0f504f 100644
@@ -1948,6 +1948,7 @@ struct drm_i915_private {
 		struct list_head active_rings;
 		struct list_head closed_vma;
 		u32 active_requests;
+		u32 reserved_requests;
 		u32 request_serial;
 
 		/**
drivers/gpu/drm/i915/i915_gem.c +1 -1
index d92147ab4489..1873e21c84c1 100644
@@ -200,7 +200,7 @@ void i915_gem_unpark(struct drm_i915_private *i915)
 	GEM_TRACE("\n");
 
 	lockdep_assert_held(&i915->drm.struct_mutex);
-	GEM_BUG_ON(!i915->gt.active_requests);
+	GEM_BUG_ON(!i915->gt.reserved_requests);
 
 	if (i915->gt.awake)
 		return;
drivers/gpu/drm/i915/i915_request.c +7 -3
index 637909c59f1f..394283799ee9 100644
@@ -200,7 +200,7 @@ static int reserve_gt(struct drm_i915_private *i915)
 		}
 	}
 
-	if (!i915->gt.active_requests++)
+	if (!i915->gt.reserved_requests++)
 		i915_gem_unpark(i915);
 
 	return 0;

@@ -208,8 +208,8 @@ static int reserve_gt(struct drm_i915_private *i915)
 
 static void unreserve_gt(struct drm_i915_private *i915)
 {
-	GEM_BUG_ON(!i915->gt.active_requests);
-	if (!--i915->gt.active_requests)
+	GEM_BUG_ON(!i915->gt.reserved_requests);
+	if (!--i915->gt.reserved_requests)
 		i915_gem_park(i915);
 }
 

@@ -384,6 +384,8 @@ static void i915_request_retire(struct i915_request *request)
 
 	__retire_engine_upto(request->engine, request);
 
+	GEM_BUG_ON(!request->i915->gt.active_requests);
+	request->i915->gt.active_requests--;
 	unreserve_gt(request->i915);
 
 	i915_sched_node_fini(request->i915, &request->sched);

@@ -1006,6 +1008,8 @@ void i915_request_add(struct i915_request *request)
 							 0);
 	}
 
+	++request->i915->gt.active_requests;
+
 	spin_lock_irq(&timeline->lock);
 	list_add_tail(&request->link, &timeline->requests);
 	spin_unlock_irq(&timeline->lock);
Signature · 6 lines
2.19.1

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

Quoting Bin Yang (2018-12-20 08:01:35)

Quoted 5 lines

Normally, i915_request_alloc() and i915_request_add() will be called in sequence with drm.struct_mutex locked. But in intel_vgpu_create_workload(), it will pre-allocate the request and call i915_request_add() in the workload thread for performance optimization. The above issue will be triggered.

That's your bug. It's not normally, it's a strict requirement that the struct_mutex (request generation mutex) be held over the course of generating the request.
-Chris

author Reviewed-by Zhi Wang
#2 lore raw
Quoted 11 lines · Chris Wilson

Quoting Bin Yang (2018-12-20 08:01:35)

Quoted 5 lines

Normally, i915_request_alloc() and i915_request_add() will be called in sequence with drm.struct_mutex locked. But in intel_vgpu_create_workload(), it will pre-allocate the request and call i915_request_add() in the workload thread for performance optimization. The above issue will be triggered.

That's your bug. It's not normally, it's a strict requirement that the struct_mutex (request generation mutex) be held over the course of generating the request.
-Chris

This code is introduced by below patch. Add original patch owners to discuss this issue.

commit d0302e74003bf1f0fc41c06948b745204c4704ea

Author: Ping Gao <[email protected]>
Date:   Thu Jun 29 12:22:43 2017 +0800

drm/i915/gvt: Audit and shadow workload during ELSP writing

Let the workload audit and shadow ahead of vGPU scheduling, that will eliminate GPU idle time and improve performance for multi-VM.

The performance of Heaven running simultaneously in 3VMs has improved 20% after this patch.

v2:Remove condition current->vgpu==vgpu when shadow during ELSP writing.

Signed-off-by Ping Gao <[email protected]>
Reviewed-by Zhi Wang <[email protected]>
Signed-off-by Zhenyu Wang <[email protected]>
CI
#3 lore raw

✗ Fi.CI.CHECKPATCH: warning for drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check

In reply to Bin Yang
== Series Details ==

== Series Details ==

Series: drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check
URL : https://patchwork.freedesktop.org/series/54330/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
bba6cf804980 drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check
-:32: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#32: 
[2018-11-28 23:17:54] [12278.310417] kernel BUG at drivers/gpu/drm/i915/i915_gem.c:4702!

total: 0 errors, 1 warnings, 0 checks, 49 lines checked

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

CI
#4 lore raw

✗ Fi.CI.SPARSE: warning for drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check

In reply to Bin Yang
== Series Details ==

== Series Details ==

Series: drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check
URL : https://patchwork.freedesktop.org/series/54330/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3550:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3551:16: warning: expression using sizeof(void)

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

CI
#5 lore raw

✗ Fi.CI.BAT: failure for drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check

In reply to Bin Yang
== Series Details ==

== Series Details ==

Series: drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check
URL : https://patchwork.freedesktop.org/series/54330/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5335 -> Patchwork_11133
====================================================

Summary
-------

**FAILURE**

Serious unknown changes coming with Patchwork_11133 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in Patchwork_11133, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://patchwork.freedesktop.org/api/1.0/series/54330/revisions/1/mbox/

Possible new issues
-------------------

Here are the unknown changes that may have been introduced in Patchwork_11133:

### IGT changes ###

#### Possible regressions ####

* igt@i915_selftest@live_objects:

    - fi-whl-u:           PASS -> INCOMPLETE
    - fi-kbl-8809g:       PASS -> INCOMPLETE
    - fi-kbl-7500u:       PASS -> INCOMPLETE
    - fi-kbl-x1275:       PASS -> INCOMPLETE
    - fi-hsw-peppy:       PASS -> INCOMPLETE
    - fi-skl-6600u:       PASS -> INCOMPLETE
    - fi-pnv-d510:        PASS -> INCOMPLETE
    - fi-hsw-4770r:       PASS -> INCOMPLETE
    - fi-skl-guc:         PASS -> INCOMPLETE
    - fi-blb-e6850:       NOTRUN -> INCOMPLETE
    - fi-skl-6700k2:      PASS -> INCOMPLETE
    - fi-skl-6260u:       PASS -> INCOMPLETE
    - fi-icl-u3:          PASS -> INCOMPLETE
    - fi-skl-6770hq:      PASS -> INCOMPLETE
    - fi-kbl-r:           PASS -> INCOMPLETE
    - fi-gdg-551:         PASS -> INCOMPLETE
    - fi-bwr-2160:        PASS -> INCOMPLETE
    - fi-bsw-kefka:       PASS -> INCOMPLETE
    - fi-kbl-guc:         PASS -> INCOMPLETE
    - fi-icl-u2:          PASS -> INCOMPLETE
    - fi-snb-2520m:       PASS -> INCOMPLETE
    - fi-kbl-7567u:       PASS -> INCOMPLETE
    - fi-skl-iommu:       PASS -> INCOMPLETE
    - fi-skl-6700hq:      PASS -> INCOMPLETE
    - fi-ivb-3520m:       PASS -> INCOMPLETE
    - fi-hsw-4770:        PASS -> INCOMPLETE
    - fi-ilk-650:         PASS -> INCOMPLETE
    - fi-bsw-n3050:       PASS -> INCOMPLETE

* {igt@runner@aborted}:

    - fi-ilk-650:         NOTRUN -> FAIL
    - fi-pnv-d510:        NOTRUN -> FAIL
    - fi-bdw-gvtdvm:      NOTRUN -> FAIL
    - fi-cfl-8109u:       NOTRUN -> FAIL
    - fi-hsw-peppy:       NOTRUN -> FAIL
    - fi-icl-u2:          NOTRUN -> FAIL
    - fi-gdg-551:         NOTRUN -> FAIL
    - fi-glk-dsi:         NOTRUN -> FAIL
    - fi-snb-2520m:       NOTRUN -> FAIL
    - fi-hsw-4770:        NOTRUN -> FAIL
    - fi-kbl-7500u:       NOTRUN -> FAIL
    - fi-bxt-j4205:       NOTRUN -> FAIL
    - fi-skl-6700hq:      NOTRUN -> FAIL
    - fi-whl-u:           NOTRUN -> FAIL
    - fi-icl-u3:          NOTRUN -> FAIL
    - fi-kbl-7560u:       NOTRUN -> FAIL
    - fi-bxt-dsi:         NOTRUN -> FAIL
    - fi-byt-j1900:       NOTRUN -> FAIL
    - fi-skl-iommu:       NOTRUN -> FAIL
    - fi-cfl-guc:         NOTRUN -> FAIL
    - fi-kbl-7567u:       NOTRUN -> FAIL
    - fi-skl-guc:         NOTRUN -> FAIL
    - fi-skl-6700k2:      NOTRUN -> FAIL
    - fi-bsw-n3050:       NOTRUN -> FAIL
    - fi-blb-e6850:       NOTRUN -> FAIL
    - fi-kbl-x1275:       NOTRUN -> FAIL
    - fi-bsw-kefka:       NOTRUN -> FAIL
    - fi-cfl-8700k:       NOTRUN -> FAIL
    - fi-hsw-4770r:       NOTRUN -> FAIL
    - fi-skl-6600u:       NOTRUN -> FAIL
    - fi-kbl-8809g:       NOTRUN -> FAIL
    - fi-byt-clapper:     NOTRUN -> FAIL
    - fi-apl-guc:         NOTRUN -> FAIL
    - fi-kbl-r:           NOTRUN -> FAIL
    - fi-bdw-5557u:       NOTRUN -> FAIL
    - fi-bwr-2160:        NOTRUN -> FAIL
    - fi-byt-n2820:       NOTRUN -> FAIL
    - fi-skl-6770hq:      NOTRUN -> FAIL
    - fi-kbl-guc:         NOTRUN -> FAIL
    - fi-skl-gvtdvm:      NOTRUN -> FAIL
    - fi-ivb-3520m:       NOTRUN -> FAIL
    - fi-skl-6260u:       NOTRUN -> FAIL
    - fi-elk-e7500:       NOTRUN -> FAIL

Known issues
------------

Here are the changes found in Patchwork_11133 that come from known issues:

### IGT changes ###

#### Issues hit ####

* igt@i915_selftest@live_objects:

    - fi-skl-gvtdvm:      PASS -> INCOMPLETE [fdo#105600]
    - fi-bxt-dsi:         PASS -> INCOMPLETE [fdo#103927]
    - fi-bdw-5557u:       PASS -> INCOMPLETE [fdo#107513]
    - fi-cfl-8700k:       PASS -> INCOMPLETE [fdo#108474]
    - fi-glk-dsi:         PASS -> INCOMPLETE [fdo#103359] / [k.org#198133]
    - fi-apl-guc:         PASS -> INCOMPLETE [fdo#103927]
    - fi-cfl-guc:         PASS -> INCOMPLETE [fdo#108474]
    - fi-elk-e7500:       PASS -> INCOMPLETE [fdo#103989]
    - fi-bxt-j4205:       PASS -> INCOMPLETE [fdo#103927]
    - fi-kbl-7560u:       PASS -> INCOMPLETE [fdo#108808]
    - fi-byt-j1900:       PASS -> INCOMPLETE [fdo#102657]
    - fi-byt-clapper:     PASS -> INCOMPLETE [fdo#102657]
    - fi-cfl-8109u:       PASS -> INCOMPLETE [fdo#108474]
    - fi-byt-n2820:       PASS -> INCOMPLETE [fdo#102657]
    - fi-bdw-gvtdvm:      PASS -> INCOMPLETE [fdo#105600] / [fdo#107513]

#### Possible fixes ####

* igt@gem_ctx_create@basic-files:
- fi-bsw-n3050: FAIL [fdo#108656] -> PASS

* igt@i915_module_load@reload:
- fi-blb-e6850: INCOMPLETE [fdo#107718] -> PASS

* igt@kms_frontbuffer_tracking@basic:
- fi-hsw-peppy: DMESG-WARN [fdo#102614] -> PASS

* igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
- fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS

{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).

[fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
[fdo#102657]: https://bugs.freedesktop.org/show_bug.cgi?id=102657
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#103989]: https://bugs.freedesktop.org/show_bug.cgi?id=103989
[fdo#105600]: https://bugs.freedesktop.org/show_bug.cgi?id=105600
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#107513]: https://bugs.freedesktop.org/show_bug.cgi?id=107513
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108474]: https://bugs.freedesktop.org/show_bug.cgi?id=108474
[fdo#108656]: https://bugs.freedesktop.org/show_bug.cgi?id=108656
[fdo#108808]: https://bugs.freedesktop.org/show_bug.cgi?id=108808
[k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133

Participating hosts (49 -> 43)
------------------------------

Missing (6): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-glk-j4005 fi-icl-y

Build changes
-------------

* Linux: CI_DRM_5335 -> Patchwork_11133

CI_DRM_5335: d1bb3d7e01b06108d5f326706a1cb5d305a847c7 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4751: 47f5a57a81b66c21d06695e263a22b87f5a33009 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_11133: bba6cf8049803b362eb342cf1297b5a39bd54d03 @ git://anongit.freedesktop.org/gfx-ci/linux

== Linux commits ==

bba6cf804980 drm/i915: Fix i915_gem_wait_for_idle oops due to active_requests check

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11133/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx