* [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
@ 2017-05-30 12:13 Chris Wilson
2017-05-30 12:13 ` [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers Chris Wilson
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: Chris Wilson @ 2017-05-30 12:13 UTC (permalink / raw)
To: intel-gfx
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@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@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;
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers
2017-05-30 12:13 [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Chris Wilson
@ 2017-05-30 12:13 ` Chris Wilson
2017-05-30 12:37 ` Tvrtko Ursulin
2017-05-30 12:38 ` Mika Kuoppala
2017-05-30 12:13 ` [PATCH 3/3] drm/i915: Check the ring is empty when declaring the engines are idle Chris Wilson
` (4 subsequent siblings)
5 siblings, 2 replies; 20+ messages in thread
From: Chris Wilson @ 2017-05-30 12:13 UTC (permalink / raw)
To: intel-gfx; +Cc: Mika Kuoppala
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 <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
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);
+
+ /* 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;
+
+ intel_runtime_pm_put(dev_priv);
+
+ return idle;
+}
+
/**
* intel_engine_is_idle() - Report if the engine has finished process all work
* @engine: the intel_engine_cs
@@ -1237,7 +1253,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
return false;
/* Ring stopped? */
- if (INTEL_GEN(dev_priv) > 2 && !(I915_READ_MODE(engine) & MODE_IDLE))
+ if (!ring_is_idle(engine))
return false;
return true;
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] drm/i915: Check the ring is empty when declaring the engines are idle
2017-05-30 12:13 [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Chris Wilson
2017-05-30 12:13 ` [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers Chris Wilson
@ 2017-05-30 12:13 ` Chris Wilson
2017-05-30 12:33 ` Mika Kuoppala
2017-05-30 12:21 ` [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Tvrtko Ursulin
` (3 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2017-05-30 12:13 UTC (permalink / raw)
To: intel-gfx; +Cc: Mika Kuoppala
As another precaution when testing whether the CS engine is actually
idle, also inspect the ring's HEAD/TAIL registers, which should be equal
when there are no commands left to execute by the GPU.
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 | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 699f2d3861c7..bc38bd128b76 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1212,6 +1212,11 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
intel_runtime_pm_get(dev_priv);
+ /* 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;
+
/* 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;
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
2017-05-30 12:13 [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Chris Wilson
2017-05-30 12:13 ` [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers Chris Wilson
2017-05-30 12:13 ` [PATCH 3/3] drm/i915: Check the ring is empty when declaring the engines are idle Chris Wilson
@ 2017-05-30 12:21 ` Tvrtko Ursulin
2017-05-30 12:36 ` Chris Wilson
2017-05-30 12:37 ` Mika Kuoppala
` (2 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Tvrtko Ursulin @ 2017-05-30 12:21 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] drm/i915: Check the ring is empty when declaring the engines are idle
2017-05-30 12:13 ` [PATCH 3/3] drm/i915: Check the ring is empty when declaring the engines are idle Chris Wilson
@ 2017-05-30 12:33 ` Mika Kuoppala
2017-05-31 13:58 ` Chris Wilson
0 siblings, 1 reply; 20+ messages in thread
From: Mika Kuoppala @ 2017-05-30 12:33 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> As another precaution when testing whether the CS engine is actually
> idle, also inspect the ring's HEAD/TAIL registers, which should be equal
> when there are no commands left to execute by the GPU.
>
> 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 | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 699f2d3861c7..bc38bd128b76 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1212,6 +1212,11 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
>
> intel_runtime_pm_get(dev_priv);
>
> + /* 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;
> +
You are already certain that is not idle so why not goto out?
-Mika
> /* 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;
> --
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
2017-05-30 12:21 ` [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Tvrtko Ursulin
@ 2017-05-30 12:36 ` Chris Wilson
0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2017-05-30 12:36 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Tue, May 30, 2017 at 01:21:29PM +0100, Tvrtko Ursulin wrote:
>
> 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@chris-wilson.co.uk>
> >Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@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.
I have silently asked for lockdep_assert_held_if() before :|
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
2017-05-30 12:13 [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Chris Wilson
` (2 preceding siblings ...)
2017-05-30 12:21 ` [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Tvrtko Ursulin
@ 2017-05-30 12:37 ` Mika Kuoppala
2017-05-30 12:41 ` Joonas Lahtinen
2017-05-30 14:09 ` ✓ Fi.CI.BAT: success for series starting with [1/3] " Patchwork
5 siblings, 0 replies; 20+ messages in thread
From: Mika Kuoppala @ 2017-05-30 12:37 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> 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@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@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;
>
> --
> 2.11.0
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers
2017-05-30 12:13 ` [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers Chris Wilson
@ 2017-05-30 12:37 ` Tvrtko Ursulin
2017-05-30 12:50 ` Chris Wilson
2017-05-30 12:38 ` Mika Kuoppala
1 sibling, 1 reply; 20+ messages in thread
From: Tvrtko Ursulin @ 2017-05-30 12:37 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Mika Kuoppala
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 <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> 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?
> +
> + /* 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.
> +
> + intel_runtime_pm_put(dev_priv);
> +
> + return idle;
> +}
> +
> /**
> * intel_engine_is_idle() - Report if the engine has finished process all work
> * @engine: the intel_engine_cs
> @@ -1237,7 +1253,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
> return false;
>
> /* Ring stopped? */
> - if (INTEL_GEN(dev_priv) > 2 && !(I915_READ_MODE(engine) & MODE_IDLE))
> + if (!ring_is_idle(engine))
> return false;
>
> return true;
>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers
2017-05-30 12:13 ` [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers Chris Wilson
2017-05-30 12:37 ` Tvrtko Ursulin
@ 2017-05-30 12:38 ` Mika Kuoppala
1 sibling, 0 replies; 20+ messages in thread
From: Mika Kuoppala @ 2017-05-30 12:38 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> 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 <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
> 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);
> +
> + /* 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;
> +
> + intel_runtime_pm_put(dev_priv);
> +
> + return idle;
> +}
> +
> /**
> * intel_engine_is_idle() - Report if the engine has finished process all work
> * @engine: the intel_engine_cs
> @@ -1237,7 +1253,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
> return false;
>
> /* Ring stopped? */
> - if (INTEL_GEN(dev_priv) > 2 && !(I915_READ_MODE(engine) & MODE_IDLE))
> + if (!ring_is_idle(engine))
> return false;
>
> return true;
> --
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
2017-05-30 12:13 [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Chris Wilson
` (3 preceding siblings ...)
2017-05-30 12:37 ` Mika Kuoppala
@ 2017-05-30 12:41 ` Joonas Lahtinen
2017-05-30 12:46 ` Chris Wilson
2017-05-30 14:09 ` ✓ Fi.CI.BAT: success for series starting with [1/3] " Patchwork
5 siblings, 1 reply; 20+ messages in thread
From: Joonas Lahtinen @ 2017-05-30 12:41 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On ti, 2017-05-30 at 13:13 +0100, 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@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
<SNIP>
> @@ -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)
This seems to be surrounded by READ_ONCE here and there.
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
2017-05-30 12:41 ` Joonas Lahtinen
@ 2017-05-30 12:46 ` Chris Wilson
0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2017-05-30 12:46 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
On Tue, May 30, 2017 at 03:41:15PM +0300, Joonas Lahtinen wrote:
> On ti, 2017-05-30 at 13:13 +0100, 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@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> <SNIP>
>
> > @@ -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)
>
> This seems to be surrounded by READ_ONCE here and there.
And I was debating using READ_ONCE here as well. Mostly we read it
locked, but since it is a triffling detail adding READ_ONCE to warn the
reader that the unlocked read is perfectly fine is ok.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers
2017-05-30 12:37 ` Tvrtko Ursulin
@ 2017-05-30 12:50 ` Chris Wilson
2017-05-30 13:09 ` Tvrtko Ursulin
0 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2017-05-30 12:50 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx, Mika Kuoppala
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 <chris@chris-wilson.co.uk>
> >Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> > 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers
2017-05-30 12:50 ` Chris Wilson
@ 2017-05-30 13:09 ` Tvrtko Ursulin
0 siblings, 0 replies; 20+ messages in thread
From: Tvrtko Ursulin @ 2017-05-30 13:09 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Mika Kuoppala
On 30/05/2017 13:50, Chris Wilson wrote:
> 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 <chris@chris-wilson.co.uk>
>>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> 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.
Makes sense,
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
2017-05-30 12:13 [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Chris Wilson
` (4 preceding siblings ...)
2017-05-30 12:41 ` Joonas Lahtinen
@ 2017-05-30 14:09 ` Patchwork
2017-05-30 16:20 ` Chris Wilson
5 siblings, 1 reply; 20+ messages in thread
From: Patchwork @ 2017-05-30 14:09 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
URL : https://patchwork.freedesktop.org/series/25039/
State : success
== Summary ==
Series 25039v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/25039/revisions/1/mbox/
Test kms_busy:
Subgroup basic-flip-default-a:
dmesg-warn -> PASS (fi-skl-6700hq) fdo#101144 +2
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:448s
fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:435s
fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:586s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:509s
fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:486s
fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:483s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:434s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:412s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:416s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:484s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:468s
fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:464s
fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:566s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:457s
fi-skl-6700hq total:278 pass:230 dwarn:1 dfail:0 fail:25 skip:22 time:411s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:465s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:501s
fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:436s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:666s
fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:405s
f98c6d553b47ea4ad8eed33a9e768d8e30d8674a drm-tip: 2017y-05m-30d-13h-06m-42s UTC integration manifest
7cfa36f drm/i915: Check the ring is empty when declaring the engines are idle
065a329 drm/i915: Hold a wakeref for probing the ring registers
afb21de drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4830/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
2017-05-30 14:09 ` ✓ Fi.CI.BAT: success for series starting with [1/3] " Patchwork
@ 2017-05-30 16:20 ` Chris Wilson
2017-05-30 19:18 ` Saarinen, Jani
0 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2017-05-30 16:20 UTC (permalink / raw)
To: intel-gfx
On Tue, May 30, 2017 at 02:09:53PM -0000, Patchwork wrote:
> == Series Details ==
>
> Series: series starting with [1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
> URL : https://patchwork.freedesktop.org/series/25039/
> State : success
>
> == Summary ==
>
> Series 25039v1 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/25039/revisions/1/mbox/
>
> Test kms_busy:
> Subgroup basic-flip-default-a:
> dmesg-warn -> PASS (fi-skl-6700hq) fdo#101144 +2
>
> fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
>
> fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:448s
> fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:435s
> fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:586s
> fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:509s
> fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:486s
> fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:483s
> fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:434s
> fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:412s
> fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:416s
> fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:484s
> fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:468s
> fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:464s
> fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:566s
> fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:457s
> fi-skl-6700hq total:278 pass:230 dwarn:1 dfail:0 fail:25 skip:22 time:411s
> fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:465s
> fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:501s
> fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:436s
> fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:666s
> fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:405s
>
> f98c6d553b47ea4ad8eed33a9e768d8e30d8674a drm-tip: 2017y-05m-30d-13h-06m-42s UTC integration manifest
> 7cfa36f drm/i915: Check the ring is empty when declaring the engines are idle
> 065a329 drm/i915: Hold a wakeref for probing the ring registers
> afb21de drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
Pushed the first 2 as that will clear the error reported by CI. I'm
leaving the third until Mika can check as we have discussed something
similar in the past. Plus lockdep_assert_held_if() is still pending.
Thanks for the review,
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
2017-05-30 16:20 ` Chris Wilson
@ 2017-05-30 19:18 ` Saarinen, Jani
2017-05-30 19:39 ` Chris Wilson
0 siblings, 1 reply; 20+ messages in thread
From: Saarinen, Jani @ 2017-05-30 19:18 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Hi,
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Chris Wilson
> Sent: Tuesday, May 30, 2017 7:21 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/3]
> drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
>
> On Tue, May 30, 2017 at 02:09:53PM -0000, Patchwork wrote:
> > == Series Details ==
> >
> > Series: series starting with [1/3] drm/i915: Short-circuit
> i915_gem_wait_for_idle() if already idle
> > URL : https://patchwork.freedesktop.org/series/25039/
> > State : success
> >
> > == Summary ==
> >
> Pushed the first 2 as that will clear the error reported by CI. I'm leaving the third
What was this error. Was there bug about it?
> until Mika can check as we have discussed something similar in the past. Plus
> lockdep_assert_held_if() is still pending.
>
> Thanks for the review,
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
Jani Saarinen
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
2017-05-30 19:18 ` Saarinen, Jani
@ 2017-05-30 19:39 ` Chris Wilson
0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2017-05-30 19:39 UTC (permalink / raw)
To: Saarinen, Jani; +Cc: intel-gfx
On Tue, May 30, 2017 at 07:18:20PM +0000, Saarinen, Jani wrote:
> Hi,
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> > Chris Wilson
> > Sent: Tuesday, May 30, 2017 7:21 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/3]
> > drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle
> >
> > On Tue, May 30, 2017 at 02:09:53PM -0000, Patchwork wrote:
> > > == Series Details ==
> > >
> > > Series: series starting with [1/3] drm/i915: Short-circuit
> > i915_gem_wait_for_idle() if already idle
> > > URL : https://patchwork.freedesktop.org/series/25039/
> > > State : success
> > >
> > > == Summary ==
> > >
> > Pushed the first 2 as that will clear the error reported by CI. I'm leaving the third
> What was this error. Was there bug about it?
The error was the warning reported in the changelog, no bug because Tomi
said look at this, and asked if I wanted him to file a bug. I said no need as
the patch was trivial and would be sent before he could file the bug.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] drm/i915: Check the ring is empty when declaring the engines are idle
2017-05-30 12:33 ` Mika Kuoppala
@ 2017-05-31 13:58 ` Chris Wilson
2017-05-31 14:01 ` Mika Kuoppala
0 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2017-05-31 13:58 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
On Tue, May 30, 2017 at 03:33:41PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > As another precaution when testing whether the CS engine is actually
> > idle, also inspect the ring's HEAD/TAIL registers, which should be equal
> > when there are no commands left to execute by the GPU.
> >
> > 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 | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index 699f2d3861c7..bc38bd128b76 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -1212,6 +1212,11 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
> >
> > intel_runtime_pm_get(dev_priv);
> >
> > + /* 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;
> > +
>
> You are already certain that is not idle so why not goto out?
In this case I could argue that extra path for the jump is not worth it.
It saves a mmio read, yes, but will any one notice?
It boils down to is it easier to read as:
static bool ring_is_idle(struct intel_engine_cs *engine)
{
struct drm_i915_private *dev_priv = engine->i915;
bool idle = false;
intel_runtime_pm_get(dev_priv);
/* First check that no commands are left in the ring */
if ((I915_READ_HEAD(engine) & HEAD_ADDR) !=
(I915_READ_TAIL(engine) & TAIL_ADDR))
goto busy;
/* No bit for gen2, so assume the CS parser is idle */
if (INTEL_GEN(dev_priv) > 2 && !(I915_READ_MODE(engine) & MODE_IDLE))
goto busy;
idle = true;
busy:
intel_runtime_pm_put(dev_priv);
return idle;
}
First instinct would be no because of the goto.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] drm/i915: Check the ring is empty when declaring the engines are idle
2017-05-31 13:58 ` Chris Wilson
@ 2017-05-31 14:01 ` Mika Kuoppala
2017-06-01 13:24 ` Chris Wilson
0 siblings, 1 reply; 20+ messages in thread
From: Mika Kuoppala @ 2017-05-31 14:01 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> On Tue, May 30, 2017 at 03:33:41PM +0300, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>>
>> > As another precaution when testing whether the CS engine is actually
>> > idle, also inspect the ring's HEAD/TAIL registers, which should be equal
>> > when there are no commands left to execute by the GPU.
>> >
>> > 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 | 5 +++++
>> > 1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>> > index 699f2d3861c7..bc38bd128b76 100644
>> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> > @@ -1212,6 +1212,11 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
>> >
>> > intel_runtime_pm_get(dev_priv);
>> >
>> > + /* 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;
>> > +
>>
>> You are already certain that is not idle so why not goto out?
>
> In this case I could argue that extra path for the jump is not worth it.
> It saves a mmio read, yes, but will any one notice?
and one write :P
> It boils down to is it easier to read as:
>
Sold. It is easier to read as is. 3/3 is
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> static bool ring_is_idle(struct intel_engine_cs *engine)
> {
> struct drm_i915_private *dev_priv = engine->i915;
> bool idle = false;
>
> intel_runtime_pm_get(dev_priv);
>
> /* First check that no commands are left in the ring */
> if ((I915_READ_HEAD(engine) & HEAD_ADDR) !=
> (I915_READ_TAIL(engine) & TAIL_ADDR))
> goto busy;
>
> /* No bit for gen2, so assume the CS parser is idle */
> if (INTEL_GEN(dev_priv) > 2 && !(I915_READ_MODE(engine) & MODE_IDLE))
> goto busy;
>
> idle = true;
> busy:
> intel_runtime_pm_put(dev_priv);
>
> return idle;
> }
>
> First instinct would be no because of the goto.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] drm/i915: Check the ring is empty when declaring the engines are idle
2017-05-31 14:01 ` Mika Kuoppala
@ 2017-06-01 13:24 ` Chris Wilson
0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2017-06-01 13:24 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx
On Wed, May 31, 2017 at 05:01:53PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > On Tue, May 30, 2017 at 03:33:41PM +0300, Mika Kuoppala wrote:
> >> Chris Wilson <chris@chris-wilson.co.uk> writes:
> >>
> >> > As another precaution when testing whether the CS engine is actually
> >> > idle, also inspect the ring's HEAD/TAIL registers, which should be equal
> >> > when there are no commands left to execute by the GPU.
> >> >
> >> > 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 | 5 +++++
> >> > 1 file changed, 5 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> >> > index 699f2d3861c7..bc38bd128b76 100644
> >> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >> > @@ -1212,6 +1212,11 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
> >> >
> >> > intel_runtime_pm_get(dev_priv);
> >> >
> >> > + /* 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;
> >> > +
> >>
> >> You are already certain that is not idle so why not goto out?
> >
> > In this case I could argue that extra path for the jump is not worth it.
> > It saves a mmio read, yes, but will any one notice?
> and one write :P
>
> > It boils down to is it easier to read as:
> >
>
> Sold. It is easier to read as is. 3/3 is
>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Thanks, now pushed. Hopefully it never spots an error!
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2017-06-01 13:24 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-30 12:13 [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Chris Wilson
2017-05-30 12:13 ` [PATCH 2/3] drm/i915: Hold a wakeref for probing the ring registers Chris Wilson
2017-05-30 12:37 ` Tvrtko Ursulin
2017-05-30 12:50 ` Chris Wilson
2017-05-30 13:09 ` Tvrtko Ursulin
2017-05-30 12:38 ` Mika Kuoppala
2017-05-30 12:13 ` [PATCH 3/3] drm/i915: Check the ring is empty when declaring the engines are idle Chris Wilson
2017-05-30 12:33 ` Mika Kuoppala
2017-05-31 13:58 ` Chris Wilson
2017-05-31 14:01 ` Mika Kuoppala
2017-06-01 13:24 ` Chris Wilson
2017-05-30 12:21 ` [PATCH 1/3] drm/i915: Short-circuit i915_gem_wait_for_idle() if already idle Tvrtko Ursulin
2017-05-30 12:36 ` Chris Wilson
2017-05-30 12:37 ` Mika Kuoppala
2017-05-30 12:41 ` Joonas Lahtinen
2017-05-30 12:46 ` Chris Wilson
2017-05-30 14:09 ` ✓ Fi.CI.BAT: success for series starting with [1/3] " Patchwork
2017-05-30 16:20 ` Chris Wilson
2017-05-30 19:18 ` Saarinen, Jani
2017-05-30 19:39 ` Chris Wilson
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.