On Tue, May 24, 2016 at 01:14:53PM +0300, Ville Syrjälä wrote:
> On Tue, May 24, 2016 at 10:27:19AM +0200, Daniel Vetter wrote:
> > On Mon, May 23, 2016 at 05:09:43PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > On SNB (at least) it's dangeruopus to hang the GPU with an infinite
> > > batch buffer loop while RPS is disabled. The only thing that keeps
> > > the system going in such circumstances are the internal RPS timers,
> > > so we should never feed the GPU with RPS disabled unless we want to
> > > risk a total system hang.
> > >
> > > Previously we didn't fully disable RPS, but that changes in
> > > commit 2030d684f7b3 ("drm/i915/bxt: Explicitly clear the Turbo control register")
> > > so we probably didn't see the problem so often previously. But
> > > even before that we were at the mercy of the BIOS for the initial
> > > RPS state, so if the BIOS didn't enable RPS a GPU hang immediately
> > > upon boot could have been fatal.
> > >
> > > To avoid the problems let's just make the RPS enable immediate.
> > > This renders the delayed_resume_work useless actually. We could perhaps
> > > just move the ring freq table initialization to the work and do the
> > > other stull synchronously?
> > >
> > > Fixes: 2030d684f7b3 ("drm/i915/bxt: Explicitly clear the Turbo control register")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > This is more and RFC at this point. Perhaps we might want to keep the delayed
> > > work but just for the ring freq table update (which is the main reson this whole
> > > thing exists in the first place). Another factor is that wait_for() is not
> > > exactly optiomal currently, so it makes the operation slower than it needs to
> > > be. Would need some hard numbers to see if it's worth keeping the delayed work
> > > or not I suppose.
> >
> > Loading the ring freq tables takes forever, so definitely want to keep
> > that.
>
> It only takes forever becasue wait_for() sucks.
>
> with current sleep duration of 1000-2000 us:
> [ 308.231364] rps init took 5533 us
> [ 308.266322] ring freq init took 34952 us
>
> sleep duration reduced to 100-200 us:
> [ 155.367588] rps init took 679 us
> [ 155.371100] ring freq init took 3509 us
>
> So looks like someone just failed to root cause the slowness, and then
> went on to optimize the wrong thing.
It's still 4ms that can be done in parallel to userspace starting :)
(And looks can be reduced further with an smarter wait_for).
So we encourage Mika to send his updated patches...
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
