From: Mika Kuoppala <mika.kuoppala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Avoid waking the engines just to check if they are idle
Date: Wed, 27 Feb 2019 15:36:56 +0200 [thread overview]
Message-ID: <87sgw9uzd3.fsf@gaia.fi.intel.com> (raw)
In-Reply-To: <20190227114958.32438-1-chris@chris-wilson.co.uk>
Chris Wilson <chris@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@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-02-27 13:37 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-27 11:49 [PATCH] drm/i915: Avoid waking the engines just to check if they are idle Chris Wilson
2019-02-27 12:06 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2019-02-27 12:26 ` ✓ Fi.CI.BAT: success " Patchwork
2019-02-27 13:36 ` Mika Kuoppala [this message]
2019-02-27 15:07 ` ✗ Fi.CI.IGT: failure " Patchwork
2019-02-27 15:25 ` Chris Wilson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87sgw9uzd3.fsf@gaia.fi.intel.com \
--to=mika.kuoppala@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.