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

[Chris Wilson]

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