On Tue, May 30, 2017 at 01:37:23PM +0100, Tvrtko Ursulin wrote:
>
> On 30/05/2017 13:13, Chris Wilson wrote:
> >Allow intel_engine_is_idle() to be called outside of the GT wakeref by
> >acquiring the device runtime pm for ourselves. This allows the function
> >to act as check after we assume the engine is idle and we release the GT
> >wakeref held whilst we have requests.
> >
> >[ 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: 5400367a864d ("drm/i915: Ensure the engine is idle before manually
> >changing HWS")
> >Signed-off-by: Chris Wilson <[email protected]>
> >Cc: Mika Kuoppala <[email protected]>
> >Cc: Joonas Lahtinen <[email protected]>
> >Cc: Tvrtko Ursulin <[email protected]>
> >---
> > drivers/gpu/drm/i915/intel_engine_cs.c | 18 +++++++++++++++++-
> > 1 file changed, 17 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> >b/drivers/gpu/drm/i915/intel_engine_cs.c
> >index 413bfd8d4bf4..699f2d3861c7 100644
> >--- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >@@ -1205,6 +1205,22 @@ int intel_ring_workarounds_emit(struct
> >drm_i915_gem_request *req)
> > return 0;
> > }
> >+static bool ring_is_idle(struct intel_engine_cs *engine)
> >+{
> >+ struct drm_i915_private *dev_priv = engine->i915;
> >+ bool idle = true;
> >+
> >+ intel_runtime_pm_get(dev_priv);
>
> I didn't find a path where this would be needed on top of the
> previous patch. I am not objecting to making it more robust, just
> checking if I have missed something?
I don't think there is currently. I thought this was suitable
"belt-and-braces" approach so that we could just write an assertion
later on (ignorant of the GPU state) without causing more pain.
> >+ /* No bit for gen2, so assume the CS parser is idle */
> >+ if (INTEL_GEN(dev_priv) > 2 && !(I915_READ_MODE(engine) & MODE_IDLE))
> >+ idle = false;
>
> Could assign a logic evaluation to idle directly here but doesn't matter.
Yes, but patch 3 encouraged this more convoluted approach.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
