[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: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