[Intel-gfx] [PATCH] drm/i915: Avoid waking the engines just to check if they are idle
Mika Kuoppala
mika.kuoppala at linux.intel.com
Wed Feb 27 13:36:56 UTC 2019
Chris Wilson <chris at chris-wilson.co.uk> writes:
> Exploit that reads of the ring registers return 0 from the engine when
> it is idle and we do not apply forcewake to know that if the engine is
> idle then both reads will be identical (and so we interpret the ring as
> idle).
>
> The ulterior motive is to try and reduce the number of spurious wakeups
> to avoid untimely death, such as:
>
> <3> [85.046836] [drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
> <4> [85.051916] ------------[ cut here ]------------
> <4> [85.051917] GT thread status wait timed out
> <4> [85.051963] WARNING: CPU: 2 PID: 2195 at drivers/gpu/drm/i915/intel_uncore.c:303 __gen6_gt_wait_for_thread_c0+0x6e/0xa0 [i915]
> <4> [85.051964] Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 x86_pkg_temp_thermal coretemp mei_hdcp crct10dif_pclmul crc32_pclmul snd_hda_intel ghash_clmulni_intel snd_hda_codec broadcom bcm_phy_lib i2c_i801 snd_hwdep snd_hda_core tg3 snd_pcm ptp pps_core mei_me mei prime_numbers lpc_ich
> <4> [85.051980] CPU: 2 PID: 2195 Comm: drm_read Tainted: G U 5.0.0-rc8-CI-CI_DRM_5662+ #1
> <4> [85.051981] Hardware name: Dell Inc. XPS 8300 /0Y2MRG, BIOS A06 10/17/2011
> <4> [85.052012] RIP: 0010:__gen6_gt_wait_for_thread_c0+0x6e/0xa0 [i915]
> <4> [85.052015] Code: 8b 92 5c 80 13 00 83 e2 07 75 d5 5b 5d c3 80 3d 5b 6a 1a 00 00 75 f4 48 c7 c7 38 21 31 a0 c6 05 4b 6a 1a 00 01 e8 e2 84 ea e0 <0f> 0b eb dd 80 3d 3a 6a 1a 00 00 75 98 48 c7 c6 08 21 31 a0 48 c7
> <4> [85.052016] RSP: 0018:ffffc9000043bd00 EFLAGS: 00010086
> <4> [85.052019] RAX: 0000000000000000 RBX: ffff888217c50000 RCX: 0000000000000000
> <4> [85.052020] RDX: 0000000000000007 RSI: ffffffff820cb141 RDI: 00000000ffffffff
> <4> [85.052022] RBP: 00000013cd30f2fb R08: 0000000000000000 R09: 0000000000000001
> <4> [85.052024] R10: ffffc9000043bce0 R11: 0000000000000000 R12: ffff888217c50ee0
> <4> [85.052025] R13: 0000000000000001 R14: 00000000ffffffff R15: ffff888218076530
> <4> [85.052028] FS: 00007fc79d049980(0000) GS:ffff888227a80000(0000) knlGS:0000000000000000
> <4> [85.052029] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4> [85.052031] CR2: 00007f782e2940f8 CR3: 000000022458e006 CR4: 00000000000606e0
> <4> [85.052033] Call Trace:
> <4> [85.052064] gen6_read32+0x14e/0x250 [i915]
> <4> [85.052096] intel_engine_is_idle+0x7d/0x180 [i915]
> <4> [85.052126] intel_engines_are_idle+0x29/0x50 [i915]
> <4> [85.052153] i915_drop_caches_set+0x21c/0x290 [i915]
> <4> [85.052160] simple_attr_write+0xb0/0xd0
> <4> [85.052165] full_proxy_write+0x51/0x80
> <4> [85.052170] __vfs_write+0x31/0x190
> <4> [85.052176] ? rcu_read_lock_sched_held+0x6f/0x80
> <4> [85.052178] ? rcu_sync_lockdep_assert+0x29/0x50
> <4> [85.052181] ? __sb_start_write+0x152/0x1f0
> <4> [85.052183] ? __sb_start_write+0x163/0x1f0
> <4> [85.052187] vfs_write+0xbd/0x1b0
> <4> [85.052191] ksys_write+0x50/0xc0
> <4> [85.052196] do_syscall_64+0x55/0x190
> <4> [85.052200] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> <4> [85.052202] RIP: 0033:0x7fc79c9d3281
> <4> [85.052204] Code: c3 0f 1f 84 00 00 00 00 00 48 8b 05 59 8d 20 00 c3 0f 1f 84 00 00 00 00 00 8b 05 8a d1 20 00 85 c0 75 16 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 57 f3 c3 0f 1f 44 00 00 41 54 55 49 89 d4 53
> <4> [85.052206] RSP: 002b:00007fffa4a0a7f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> <4> [85.052208] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fc79c9d3281
> <4> [85.052210] RDX: 0000000000000005 RSI: 00007fffa4a0a880 RDI: 0000000000000008
> <4> [85.052212] RBP: 00007fffa4a0a820 R08: 0000000000000000 R09: 0000000000000000
> <4> [85.052213] R10: 0000000000000000 R11: 0000000000000246 R12: 00007fc79c9bc718
> <4> [85.052215] R13: 0000000000000003 R14: 00007fc79c9c1628 R15: 00007fc79c9bdd80
> <4> [85.052223] irq event stamp: 71630
> <4> [85.052226] hardirqs last enabled at (71629): [<ffffffff8197b64c>] _raw_spin_unlock_irqrestore+0x4c/0x60
> <4> [85.052228] hardirqs last disabled at (71630): [<ffffffff8197b4bd>] _raw_spin_lock_irqsave+0xd/0x50
> <4> [85.052231] softirqs last enabled at (70444): [<ffffffff81c0033a>] __do_softirq+0x33a/0x4b9
> <4> [85.052234] softirqs last disabled at (70433): [<ffffffff810b51b1>] irq_exit+0xd1/0xe0
> <4> [85.052264] WARNING: CPU: 2 PID: 2195 at drivers/gpu/drm/i915/intel_uncore.c:303 __gen6_gt_wait_for_thread_c0+0x6e/0xa0 [i915]
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
> drivers/gpu/drm/i915/intel_engine_cs.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index b7b626195eda..4f244019560d 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -968,6 +968,7 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
> {
> struct drm_i915_private *dev_priv = engine->i915;
> intel_wakeref_t wakeref;
> + unsigned long flags;
> bool idle = true;
>
> if (I915_SELFTEST_ONLY(!engine->mmio_base))
> @@ -978,15 +979,19 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
> if (!wakeref)
> return true;
>
> - /* First check that no commands are left in the ring */
> - if ((I915_READ_HEAD(engine) & HEAD_ADDR) !=
> - (I915_READ_TAIL(engine) & TAIL_ADDR))
> - idle = false;
> + spin_lock_irqsave(&dev_priv->uncore.lock, flags);
>
> - /* No bit for gen2, so assume the CS parser is idle */
> - if (INTEL_GEN(dev_priv) > 2 && !(I915_READ_MODE(engine) & MODE_IDLE))
I agree on the sentiment that in here we should have a one marker only.
And that is idle == (head == tail). We have MODE_IDLE checks
on other parts but they dont affect the flow.
Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> + /*
> + * Check that no commands are left in the ring.
> + *
> + * If the engine is not awake, both reads return 0 as we do so without
> + * forcewake.
> + */
> + if ((I915_READ_FW(RING_HEAD(engine->mmio_base)) & HEAD_ADDR) !=
> + (I915_READ_FW(RING_TAIL(engine->mmio_base)) & TAIL_ADDR))
> idle = false;
>
> + spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> intel_runtime_pm_put(dev_priv, wakeref);
>
> return idle;
> --
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list