Re: [PATCH] drm/i915: Avoid waking the engines just to check if they are idle

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx>

> +	/*
> +	 * 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux