[Intel-gfx] [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle

[Tvrtko Ursulin]

On 30/05/2017 13:13, Chris Wilson wrote:
> If the device is asleep (no GT wakeref), we know the GPU is already idle.
> If we add an early return, we can avoid touching registers and checking
> hw state outside of the assumed GT wakelock. This prevents causing such
> errors whilst debugging:
> 
> [ 2613.401647] RPM wakelock ref not held during HW access
> [ 2613.401684] ------------[ cut here ]------------
> [ 2613.401720] WARNING: CPU: 5 PID: 7739 at drivers/gpu/drm/i915/intel_drv.h:1787 gen6_read32+0x21f/0x2b0 [i915]
> [ 2613.401731] Modules linked in: snd_hda_intel i915 vgem snd_hda_codec_hdmi x86_pkg_temp_thermal intel_powerclamp snd_hda_codec_realtek coretemp snd_hda_codec_generic crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_codec snd_hwdep snd_hda_core snd_pcm r8169 mii mei_me lpc_ich mei prime_numbers [last unloaded: i915]
> [ 2613.401823] CPU: 5 PID: 7739 Comm: drv_missed_irq Tainted: G     U          4.12.0-rc2-CI-CI_DRM_421+ #1
> [ 2613.401825] Hardware name: MSI MS-7924/Z97M-G43(MS-7924), BIOS V1.12 02/15/2016
> [ 2613.401840] task: ffff880409e3a740 task.stack: ffffc900084dc000
> [ 2613.401861] RIP: 0010:gen6_read32+0x21f/0x2b0 [i915]
> [ 2613.401863] RSP: 0018:ffffc900084dfce8 EFLAGS: 00010292
> [ 2613.401869] RAX: 000000000000002a RBX: ffff8804016a8000 RCX: 0000000000000006
> [ 2613.401871] RDX: 0000000000000006 RSI: ffffffff81cbf2d9 RDI: ffffffff81c9e3a7
> [ 2613.401874] RBP: ffffc900084dfd18 R08: ffff880409e3afc8 R09: 0000000000000000
> [ 2613.401877] R10: 000000008a1c483f R11: 0000000000000000 R12: 000000000000209c
> [ 2613.401879] R13: 0000000000000001 R14: ffff8804016a8000 R15: ffff8804016ac150
> [ 2613.401882] FS:  00007f39ef3dd8c0(0000) GS:ffff88041fb40000(0000) knlGS:0000000000000000
> [ 2613.401885] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 2613.401887] CR2: 00000000023717c8 CR3: 00000002e7b34000 CR4: 00000000001406e0
> [ 2613.401889] Call Trace:
> [ 2613.401912]  intel_engine_is_idle+0x76/0x90 [i915]
> [ 2613.401931]  i915_gem_wait_for_idle+0xe6/0x1e0 [i915]
> [ 2613.401951]  fault_irq_set+0x40/0x90 [i915]
> [ 2613.401970]  i915_ring_test_irq_set+0x42/0x50 [i915]
> [ 2613.401976]  simple_attr_write+0xc7/0xe0
> [ 2613.401981]  full_proxy_write+0x4f/0x70
> [ 2613.401987]  __vfs_write+0x23/0x120
> [ 2613.401992]  ? rcu_read_lock_sched_held+0x75/0x80
> [ 2613.401996]  ? rcu_sync_lockdep_assert+0x2a/0x50
> [ 2613.401999]  ? __sb_start_write+0xfa/0x1f0
> [ 2613.402004]  vfs_write+0xc5/0x1d0
> [ 2613.402008]  ? trace_hardirqs_on_caller+0xe7/0x1c0
> [ 2613.402013]  SyS_write+0x44/0xb0
> [ 2613.402020]  entry_SYSCALL_64_fastpath+0x1c/0xb1
> [ 2613.402022] RIP: 0033:0x7f39eded6670
> [ 2613.402025] RSP: 002b:00007fffdcdcb1a8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [ 2613.402030] RAX: ffffffffffffffda RBX: ffffffff81470203 RCX: 00007f39eded6670
> [ 2613.402033] RDX: 0000000000000001 RSI: 000000000041bc33 RDI: 0000000000000006
> [ 2613.402036] RBP: ffffc900084dff88 R08: 00007f39ef3dd8c0 R09: 0000000000000001
> [ 2613.402038] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000041bc33
> [ 2613.402041] R13: 0000000000000006 R14: 0000000000000000 R15: 0000000000000000
> [ 2613.402046]  ? __this_cpu_preempt_check+0x13/0x20
> [ 2613.402052] Code: 01 9b fa e0 0f ff e9 28 fe ff ff 80 3d 6a dd 0e 00 00 0f 85 29 fe ff ff 48 c7 c7 48 19 29 a0 c6 05 56 dd 0e 00 01 e8 da 9a fa e0 <0f> ff e9 0f fe ff ff b9 01 00 00 00 ba 01 00 00 00 44 89 e6 48
> [ 2613.402199] ---[ end trace 31f0cfa93ab632bf ]---
> 
> Fixes: Fixes: 25112b64b3d2 ("drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7ab47a84671f..2ce915a1b607 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3348,6 +3348,10 @@ int i915_gem_wait_for_idle(struct drm_i915_private *i915, unsigned int flags)
>   {
>   	int ret;
>   
> +	/* If the device is asleep, we have no requests outstanding */
> +	if (!i915->gt.awake)
> +		return 0;
> +
>   	if (flags & I915_WAIT_LOCKED) {
>   		struct i915_gem_timeline *tl;
>   
> 

Looks like it would be worth pulling up the lockdep_assert_held before 
the fast exit for the I915_WAIT_LOCKED case? Would be better at catching 
bugs which after this patch can become timing sensitive.

Regards,

Tvrtko