| Message ID | 1494428821-14893-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 > >
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
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
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
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)
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(-)