[Intel-gfx] [PATCH] drm/i915: Fix __intel_wait_for_register_fw to not sleep in atomic

[Michal Wajdeczko]

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 at chris-wilson.co.uk>
> > > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > > Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at 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