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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
