* [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic
@ 2017-05-10 15:19 Daniel Vetter
2017-05-10 16:06 ` Chris Wilson
2017-05-10 16:26 ` ✓ Fi.CI.BAT: success for drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic (rev2) Patchwork
0 siblings, 2 replies; 10+ messages in thread
From: Daniel Vetter @ 2017-05-10 15:19 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Daniel Vetter
The unconditionally fallback to the blocking wait_for resulted in
impressive fireworks at boot-up on my snb here. Make sure if we set
the slow timeout to 0 that we never ever sleep. The tail of the
callchain was
intel_wait_for_register
-> __intel_wait_for_register_fw
-> usleep_range
-> BOOM
It blew up in intel_crt_detect load detection code on the
ADPA_CRT_HOTPLUG_FORCE_TRIGGER in the ADPA register.
v2: Shut up gcc.
Fixes: 0564654340e2 ("drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/i915/intel_uncore.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index aa9d3065853c..08abb2cb7837 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1598,7 +1598,7 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
unsigned int slow_timeout_ms,
u32 *out_value)
{
- u32 reg_value;
+ u32 reg_value = 0;
#define done (((reg_value = I915_READ_FW(reg)) & mask) == value)
int ret;
@@ -1609,7 +1609,7 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
ret = -ETIMEDOUT;
if (fast_timeout_us && fast_timeout_us <= 20000)
ret = _wait_for_atomic(done, fast_timeout_us, 0);
- if (ret)
+ if (ret && slow_timeout_ms)
ret = wait_for(done, slow_timeout_ms);
if (out_value)
--
2.5.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic 2017-05-10 15:19 [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic Daniel Vetter @ 2017-05-10 16:06 ` Chris Wilson 2017-05-10 18:09 ` Daniel Vetter 2017-05-10 16:26 ` ✓ Fi.CI.BAT: success for drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic (rev2) Patchwork 1 sibling, 1 reply; 10+ messages in thread From: Chris Wilson @ 2017-05-10 16:06 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, Daniel Vetter On Wed, May 10, 2017 at 05:19:32PM +0200, Daniel Vetter wrote: > The unconditionally fallback to the blocking wait_for resulted in > impressive fireworks at boot-up on my snb here. Make sure if we set > the slow timeout to 0 that we never ever sleep. The tail of the > callchain was > > intel_wait_for_register > -> __intel_wait_for_register_fw > -> usleep_range > -> BOOM > > It blew up in intel_crt_detect load detection code on the > ADPA_CRT_HOTPLUG_FORCE_TRIGGER in the ADPA register. > > v2: Shut up gcc. u32 uninitialized_var(reg_value) ? -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] 10+ messages in thread
* Re: [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic 2017-05-10 16:06 ` Chris Wilson @ 2017-05-10 18:09 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2017-05-10 18:09 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, Intel Graphics Development, Michal Wajdeczko, Joonas Lahtinen, Tvrtko Ursulin, Daniel Vetter, Jani Nikula On Wed, May 10, 2017 at 05:06:38PM +0100, Chris Wilson wrote: > On Wed, May 10, 2017 at 05:19:32PM +0200, Daniel Vetter wrote: > > The unconditionally fallback to the blocking wait_for resulted in > > impressive fireworks at boot-up on my snb here. Make sure if we set > > the slow timeout to 0 that we never ever sleep. The tail of the > > callchain was > > > > intel_wait_for_register > > -> __intel_wait_for_register_fw > > -> usleep_range > > -> BOOM > > > > It blew up in intel_crt_detect load detection code on the > > ADPA_CRT_HOTPLUG_FORCE_TRIGGER in the ADPA register. > > > > v2: Shut up gcc. > > u32 uninitialized_var(reg_value) ? Polished to v3 and applied, thanks for your review. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic (rev2) 2017-05-10 15:19 [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic Daniel Vetter 2017-05-10 16:06 ` Chris Wilson @ 2017-05-10 16:26 ` Patchwork 1 sibling, 0 replies; 10+ messages in thread From: Patchwork @ 2017-05-10 16:26 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx == Series Details == Series: drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic (rev2) URL : https://patchwork.freedesktop.org/series/24229/ State : success == Summary == Series 24229v2 drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic https://patchwork.freedesktop.org/api/1.0/series/24229/revisions/2/mbox/ Test gem_exec_suspend: Subgroup basic-s4-devices: pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 Test kms_cursor_legacy: Subgroup basic-busy-flip-before-cursor-atomic: pass -> FAIL (fi-snb-2600) fdo#100215 Test kms_flip: Subgroup basic-flip-vs-modeset: dmesg-warn -> PASS (fi-byt-j1900) fdo#100652 fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215 fdo#100652 https://bugs.freedesktop.org/show_bug.cgi?id=100652 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:438s fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:434s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:583s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:512s fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time:561s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:493s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:493s fi-elk-e7500 total:278 pass:229 dwarn:0 dfail:0 fail:0 skip:49 time:418s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:411s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:411s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:424s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:495s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:471s fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:465s fi-kbl-7560u total:278 pass:267 dwarn:1 dfail:0 fail:0 skip:10 time:569s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:469s fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:575s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:469s fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:499s 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:535s fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time:413s de0799a21722baa8bcc68838d4689cd61647478f drm-tip: 2017y-05m-10d-15h-14m-10s UTC integration manifest e5718382 drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4661/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic
@ 2017-05-10 15:07 Daniel Vetter
2017-05-10 15:31 ` Michal Wajdeczko
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2017-05-10 15:07 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Daniel Vetter
The unconditionally fallback to the blocking wait_for resulted in
impressive fireworks at boot-up on my snb here. Make sure if we set
the slow timeout to 0 that we never ever sleep. The tail of the
callchain was
intel_wait_for_register
-> __intel_wait_for_register_fw
-> usleep_range
-> BOOM
It blew up in intel_crt_detect load detection code on the
ADPA_CRT_HOTPLUG_FORCE_TRIGGER in the ADPA register.
Fixes: 0564654340e2 ("drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/i915/intel_uncore.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index aa9d3065853c..b03ad06bc3b6 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1609,7 +1609,7 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
ret = -ETIMEDOUT;
if (fast_timeout_us && fast_timeout_us <= 20000)
ret = _wait_for_atomic(done, fast_timeout_us, 0);
- if (ret)
+ if (ret && slow_timeout_ms)
ret = wait_for(done, slow_timeout_ms);
if (out_value)
--
2.5.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic 2017-05-10 15:07 [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic Daniel Vetter @ 2017-05-10 15:31 ` Michal Wajdeczko 2017-05-10 15:32 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Michal Wajdeczko @ 2017-05-10 15:31 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, Daniel Vetter On Wed, May 10, 2017 at 05:07:01PM +0200, Daniel Vetter wrote: > The unconditionally fallback to the blocking wait_for resulted in > impressive fireworks at boot-up on my snb here. Make sure if we set > the slow timeout to 0 that we never ever sleep. The tail of the > callchain was > > intel_wait_for_register > -> __intel_wait_for_register_fw > -> usleep_range > -> BOOM > > It blew up in intel_crt_detect load detection code on the > ADPA_CRT_HOTPLUG_FORCE_TRIGGER in the ADPA register. > Hmm, by reading the code, it looks that call stack should be like this: -> intel_wait_for_register(..., timeout_ms=1000) -> __intel_wait_for_register_fw(..., fast_us=2, slow_ms=0, NULL); -> wait_for(..., MS=1000) -> _wait_for(..., US=1000*1000, W=1000) -> usleep_range(W, 2*W) so the slow_timeout_ms will be 0 in __intel_wait_for_register_fw() Are you sure that fix below is in right place? -Michal > Fixes: 0564654340e2 ("drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()") > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index aa9d3065853c..b03ad06bc3b6 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1609,7 +1609,7 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv, > ret = -ETIMEDOUT; > if (fast_timeout_us && fast_timeout_us <= 20000) > ret = _wait_for_atomic(done, fast_timeout_us, 0); > - if (ret) > + if (ret && slow_timeout_ms) > ret = wait_for(done, slow_timeout_ms); > > if (out_value) > -- > 2.5.5 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic 2017-05-10 15:31 ` Michal Wajdeczko @ 2017-05-10 15:32 ` Daniel Vetter 2017-05-10 15:49 ` Michal Wajdeczko 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2017-05-10 15:32 UTC (permalink / raw) To: Michal Wajdeczko; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter On Wed, May 10, 2017 at 05:31:02PM +0200, Michal Wajdeczko wrote: > On Wed, May 10, 2017 at 05:07:01PM +0200, Daniel Vetter wrote: > > The unconditionally fallback to the blocking wait_for resulted in > > impressive fireworks at boot-up on my snb here. Make sure if we set > > the slow timeout to 0 that we never ever sleep. The tail of the > > callchain was > > > > intel_wait_for_register > > -> __intel_wait_for_register_fw > > -> usleep_range > > -> BOOM > > > > It blew up in intel_crt_detect load detection code on the > > ADPA_CRT_HOTPLUG_FORCE_TRIGGER in the ADPA register. > > > > Hmm, by reading the code, it looks that call stack should be like this: > > -> intel_wait_for_register(..., timeout_ms=1000) > -> __intel_wait_for_register_fw(..., fast_us=2, slow_ms=0, NULL); > -> wait_for(..., MS=1000) > -> _wait_for(..., US=1000*1000, W=1000) > -> usleep_range(W, 2*W) > > so the slow_timeout_ms will be 0 in __intel_wait_for_register_fw() > > Are you sure that fix below is in right place? The wait_for is _within the __intel_wait_for_register_fw. I've left out the macros because those don't show up in the bt. We do _not_ blow up on the wait_for after the __intel_wait_for_register_fw call in intel_wait_for_register. -Daniel > > -Michal > > > > > Fixes: 0564654340e2 ("drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()") > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > index aa9d3065853c..b03ad06bc3b6 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -1609,7 +1609,7 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv, > > ret = -ETIMEDOUT; > > if (fast_timeout_us && fast_timeout_us <= 20000) > > ret = _wait_for_atomic(done, fast_timeout_us, 0); > > - if (ret) > > + if (ret && slow_timeout_ms) > > ret = wait_for(done, slow_timeout_ms); > > > > if (out_value) > > -- > > 2.5.5 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic 2017-05-10 15:32 ` Daniel Vetter @ 2017-05-10 15:49 ` Michal Wajdeczko 2017-05-10 16:09 ` Chris Wilson 0 siblings, 1 reply; 10+ messages in thread From: Michal Wajdeczko @ 2017-05-10 15:49 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter On Wed, May 10, 2017 at 05:32:48PM +0200, Daniel Vetter wrote: > On Wed, May 10, 2017 at 05:31:02PM +0200, Michal Wajdeczko wrote: > > On Wed, May 10, 2017 at 05:07:01PM +0200, Daniel Vetter wrote: > > > The unconditionally fallback to the blocking wait_for resulted in > > > impressive fireworks at boot-up on my snb here. Make sure if we set > > > the slow timeout to 0 that we never ever sleep. The tail of the > > > callchain was > > > > > > intel_wait_for_register > > > -> __intel_wait_for_register_fw > > > -> usleep_range > > > -> BOOM > > > > > > It blew up in intel_crt_detect load detection code on the > > > ADPA_CRT_HOTPLUG_FORCE_TRIGGER in the ADPA register. > > > > > > > Hmm, by reading the code, it looks that call stack should be like this: > > > > -> intel_wait_for_register(..., timeout_ms=1000) > > -> __intel_wait_for_register_fw(..., fast_us=2, slow_ms=0, NULL); > > -> wait_for(..., MS=1000) > > -> _wait_for(..., US=1000*1000, W=1000) > > -> usleep_range(W, 2*W) > > > > so the slow_timeout_ms will be 0 in __intel_wait_for_register_fw() > > > > Are you sure that fix below is in right place? > > The wait_for is _within the __intel_wait_for_register_fw. I've left out > the macros because those don't show up in the bt. We do _not_ blow up on > the wait_for after the __intel_wait_for_register_fw call in > intel_wait_for_register. Ok, so the correct call stack is -> intel_wait_for_register(..., timeout_ms=1000) -> __intel_wait_for_register_fw(..., fast_us=2, slow_ms=0, NULL); -> wait_for(..., MS=0) -> _wait_for(..., US=0, W=1000) -> usleep_range(W, 2*W) so maybe we should just fix the wait_for/_wait_for macros and do not attempt to sleep when timeout is zero ? It's rather unexpected that even with with timeout MS=0 we will still call usleep_range(1000us, 2000us) -Michal > -Daniel > > > > > -Michal > > > > > > > > > Fixes: 0564654340e2 ("drm/i915: Acquire uncore.lock over intel_uncore_wait_for_register()") > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > > index aa9d3065853c..b03ad06bc3b6 100644 > > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > > @@ -1609,7 +1609,7 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv, > > > ret = -ETIMEDOUT; > > > if (fast_timeout_us && fast_timeout_us <= 20000) > > > ret = _wait_for_atomic(done, fast_timeout_us, 0); > > > - if (ret) > > > + if (ret && slow_timeout_ms) > > > ret = wait_for(done, slow_timeout_ms); > > > > > > if (out_value) > > > -- > > > 2.5.5 > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic 2017-05-10 15:49 ` Michal Wajdeczko @ 2017-05-10 16:09 ` Chris Wilson 2017-05-10 16:25 ` Michal Wajdeczko 0 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2017-05-10 16:09 UTC (permalink / raw) To: Michal Wajdeczko; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter On Wed, May 10, 2017 at 05:49:26PM +0200, Michal Wajdeczko wrote: > On Wed, May 10, 2017 at 05:32:48PM +0200, Daniel Vetter wrote: > > On Wed, May 10, 2017 at 05:31:02PM +0200, Michal Wajdeczko wrote: > > > On Wed, May 10, 2017 at 05:07:01PM +0200, Daniel Vetter wrote: > > > > The unconditionally fallback to the blocking wait_for resulted in > > > > impressive fireworks at boot-up on my snb here. Make sure if we set > > > > the slow timeout to 0 that we never ever sleep. The tail of the > > > > callchain was > > > > > > > > intel_wait_for_register > > > > -> __intel_wait_for_register_fw > > > > -> usleep_range > > > > -> BOOM > > > > > > > > It blew up in intel_crt_detect load detection code on the > > > > ADPA_CRT_HOTPLUG_FORCE_TRIGGER in the ADPA register. > > > > > > > > > > Hmm, by reading the code, it looks that call stack should be like this: > > > > > > -> intel_wait_for_register(..., timeout_ms=1000) > > > -> __intel_wait_for_register_fw(..., fast_us=2, slow_ms=0, NULL); > > > -> wait_for(..., MS=1000) > > > -> _wait_for(..., US=1000*1000, W=1000) > > > -> usleep_range(W, 2*W) > > > > > > so the slow_timeout_ms will be 0 in __intel_wait_for_register_fw() > > > > > > Are you sure that fix below is in right place? > > > > The wait_for is _within the __intel_wait_for_register_fw. I've left out > > the macros because those don't show up in the bt. We do _not_ blow up on > > the wait_for after the __intel_wait_for_register_fw call in > > intel_wait_for_register. > > Ok, so the correct call stack is > > -> intel_wait_for_register(..., timeout_ms=1000) > -> __intel_wait_for_register_fw(..., fast_us=2, slow_ms=0, NULL); > -> wait_for(..., MS=0) > -> _wait_for(..., US=0, W=1000) > -> usleep_range(W, 2*W) > > so maybe we should just fix the wait_for/_wait_for macros and do not attempt > to sleep when timeout is zero ? It's rather unexpected that even with with > timeout MS=0 we will still call usleep_range(1000us, 2000us) In this case, it was clearly incorrect to do a wait_for_pass at all. In general, those wait_for macros are already complicated enough and the callers of wait_for() must reasonably expect it to sleep and so a seperate dance for timeout==0 seems unjustified. -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] 10+ messages in thread
* Re: [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic 2017-05-10 16:09 ` Chris Wilson @ 2017-05-10 16:25 ` Michal Wajdeczko 0 siblings, 0 replies; 10+ messages in thread From: Michal Wajdeczko @ 2017-05-10 16:25 UTC (permalink / raw) To: Chris Wilson; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter On Wed, May 10, 2017 at 05:09:17PM +0100, Chris Wilson wrote: > On Wed, May 10, 2017 at 05:49:26PM +0200, Michal Wajdeczko wrote: > > On Wed, May 10, 2017 at 05:32:48PM +0200, Daniel Vetter wrote: > > > On Wed, May 10, 2017 at 05:31:02PM +0200, Michal Wajdeczko wrote: > > > > On Wed, May 10, 2017 at 05:07:01PM +0200, Daniel Vetter wrote: > > > > > The unconditionally fallback to the blocking wait_for resulted in > > > > > impressive fireworks at boot-up on my snb here. Make sure if we set > > > > > the slow timeout to 0 that we never ever sleep. The tail of the > > > > > callchain was > > > > > > > > > > intel_wait_for_register > > > > > -> __intel_wait_for_register_fw > > > > > -> usleep_range > > > > > -> BOOM > > > > > > > > > > It blew up in intel_crt_detect load detection code on the > > > > > ADPA_CRT_HOTPLUG_FORCE_TRIGGER in the ADPA register. > > > > > > > > > > > > > Hmm, by reading the code, it looks that call stack should be like this: > > > > > > > > -> intel_wait_for_register(..., timeout_ms=1000) > > > > -> __intel_wait_for_register_fw(..., fast_us=2, slow_ms=0, NULL); > > > > -> wait_for(..., MS=1000) > > > > -> _wait_for(..., US=1000*1000, W=1000) > > > > -> usleep_range(W, 2*W) > > > > > > > > so the slow_timeout_ms will be 0 in __intel_wait_for_register_fw() > > > > > > > > Are you sure that fix below is in right place? > > > > > > The wait_for is _within the __intel_wait_for_register_fw. I've left out > > > the macros because those don't show up in the bt. We do _not_ blow up on > > > the wait_for after the __intel_wait_for_register_fw call in > > > intel_wait_for_register. > > > > Ok, so the correct call stack is > > > > -> intel_wait_for_register(..., timeout_ms=1000) > > -> __intel_wait_for_register_fw(..., fast_us=2, slow_ms=0, NULL); > > -> wait_for(..., MS=0) > > -> _wait_for(..., US=0, W=1000) > > -> usleep_range(W, 2*W) > > > > so maybe we should just fix the wait_for/_wait_for macros and do not attempt > > to sleep when timeout is zero ? It's rather unexpected that even with with > > timeout MS=0 we will still call usleep_range(1000us, 2000us) > > In this case, it was clearly incorrect to do a wait_for_pass at all. In > general, those wait_for macros are already complicated enough and the Maybe no extra dance is needed, just this: #define _wait_for(COND, US, W) ({ \ - unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1; \ + unsigned long timeout__ = jiffies + usecs_to_jiffies(US); \ int ret__; \ for (;;) { \ bool expired__ = time_after(jiffies, timeout__); \ -Michal > callers of wait_for() must reasonably expect it to sleep and so a seperate > dance for timeout==0 seems unjustified. > -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] 10+ messages in thread
end of thread, other threads:[~2017-05-10 18:09 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-10 15:19 [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic Daniel Vetter 2017-05-10 16:06 ` Chris Wilson 2017-05-10 18:09 ` Daniel Vetter 2017-05-10 16:26 ` ✓ Fi.CI.BAT: success for drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic (rev2) Patchwork -- strict thread matches above, loose matches on Subject: below -- 2017-05-10 15:07 [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic Daniel Vetter 2017-05-10 15:31 ` Michal Wajdeczko 2017-05-10 15:32 ` Daniel Vetter 2017-05-10 15:49 ` Michal Wajdeczko 2017-05-10 16:09 ` Chris Wilson 2017-05-10 16:25 ` Michal Wajdeczko
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.