[Intel-gfx] [PATCH] drm/i915: Acquire RPM wakeref for KMS atomic commit
chris at chris-wilson.co.uk
Tue Dec 22 12:46:56 PST 2015
On Mon, Dec 21, 2015 at 07:21:33PM +0100, Daniel Vetter wrote:
> On Mon, Dec 21, 2015 at 04:37:41PM +0000, Chris Wilson wrote:
> > On Mon, Dec 21, 2015 at 05:28:16PM +0100, Daniel Vetter wrote:
> > > On Mon, Dec 21, 2015 at 04:14:53PM +0000, Chris Wilson wrote:
> > > > On Mon, Dec 21, 2015 at 05:02:08PM +0100, Daniel Vetter wrote:
> > > > > On Sat, Dec 19, 2015 at 09:58:43AM +0000, Chris Wilson wrote:
> > > > > > Once all the preparations are complete, we are ready to write the
> > > > > > modesetting to the hardware. During this phase, we will be making lots
> > > > > > of HW register access, so take a top level wakeref to prevent an
> > > > > > unwarranted rpm suspend cycle mid-commit. Lower level functions should
> > > > > > be waking the individual power wells as required.
> > > > > >
> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93439
> > > > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > > Cc: Imre Deak <imre.deak at intel.com>
> > > > >
> > > > > The original idea here was that doing this will paper over bugs in our rpm
> > > > > refcounting. There's also the problem that for modeset stuff we have all
> > > > > the power wells still to take care of.
> > > > >
> > > > > For the referenced bug we should add a power domain check in the get hw
> > > > > state function instead, which is what we've been doing with all the other
> > > > > similar hw state readout functions too.
> > > >
> > > > Agreed that there is another bug, but in the long term, we do want a
> > > > "prolonged" wakeref here. In the next evolution of the wakeref assertions,
> > > > we should be able to differentiate between the two (i.e. when we have
> > > > fine grained wakerefs around the hw access, we need to assert we hold one
> > > > of that type in the mmio accessor, rather than the prolonged version).
> > >
> > > Why? If we enforce that I fear we lose implicit coverage. Currently if you
> > > touch any piece of modeset hw and don't have the corresponding long-time
> > > rpm/power well ref there's a good chance something will spot this. If we
> > > have a short-term rpm reference for everything we won't noticed these
> > > problems around the long-term rpm references any more.
> > The theory being that when we get autosuspend on the order of say a
> > hundred microseconds, we start to run into the real possibility of an
> > rpm cycle mid update.
> > > Imo the only thing short-term references are useful for is lockdep
> > > annotations to detect deadlocks, since lockdep requires that we drop a
> > > lock in the same process again. Long-term ones would simply do a
> > > might_lock in the get function to annotate the deadlock with rpm resume
> > > functions.
> > I'm thinking of a world where suspend-resume time are on the order of
> > microseconds and the rpm suspend interval not much greater.
> That's why we need a bit of hystersis to avoid that. Statistics rule of
> thumb is to only suspend once you've spent about as much time idle as it
> would take you to suspend/resume. And since we do a few global updates we
> actually acquire new power wells before old ones, so as long as you don't
> do 2 ioctls to change configurations it's impossible to accidentally
> suspend in between. The rest is just appropriately tuning defaults (and
> still setting it to 0 in igt for maximum nastiness).
I still feel we are talking about complementary techiniques for
achieving the same thing - and that there will always be places in the
code where we want to say "do not thing about suspending until the end of
this big function".
Chris Wilson, Intel Open Source Technology Centre
More information about the Intel-gfx