[Intel-gfx] [PATCH 5/5] drm/i915/vlv: support save/restore of display state around power well toggle

[Daniel Vetter]

On Tue, Oct 15, 2013 at 01:42:53PM -0700, Jesse Barnes wrote:
> On Tue, 15 Oct 2013 17:09:19 -0300
> Paulo Zanoni <przanoni at gmail.com> wrote:
> 
> > 2013/10/14 Jesse Barnes <jbarnes at virtuousgeek.org>:
> > > If we disable the power well at runtime, we need to save enough display
> > > state so we can restore it when the power well comes back again.  Add
> > > support for that on VLV by reusing some of the _freeze and _thaw code.
> > >
> > > Note we need to drop the power well lock in this path around the restore,
> > > since we'll end up in mode set functions that take more refs on the
> > > power well.
> > >
> > > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/intel_uncore.c | 21 ++++++++++++++++++++-
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index b126f5a..070ff00 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -406,7 +406,26 @@ static void __vlv_set_power_well(struct drm_i915_private *dev_priv,
> > >  static void vlv_set_display_power(struct drm_i915_private *dev_priv,
> > >                                   bool enable)
> > >  {
> > > -       __vlv_set_power_well(dev_priv, DISP2D_PWRGT, enable);
> > > +       struct drm_device *dev = dev_priv->dev;
> > > +       struct i915_power_well *power_well = &dev_priv->power_well;
> > > +
> > > +       if (enable) {
> > > +               /* Lost all the display state, restore it */
> > > +               if (vlv_display_power_enabled(dev_priv))
> > > +                       return; /* already on, skip the fireworks */
> > > +               __vlv_set_power_well(dev_priv, DISP2D_PWRGT, true);
> > > +               spin_unlock_irq(&power_well->lock);
> > > +               i915_restore_state(dev);
> > 
> > At least on Haswell, i915_restore_state restores a lot more than just
> > what was lost on the power well. Do you really need all this? Besides,
> > I kinda fear you can break things by restoring stuff that is already
> > running. While we're doing this restoring, interrputs are happening,
> > everything is running, yet we call stuff like intel_i2c_reset,
> > i915_gem_restore_fences and others. Another example: we have code to
> > restore the backlight registers, but backlight is usually on the
> > output that works even with the power well disabled. So if we disable
> > the power well, then change backlight on the only output that is
> > working, then reenable the power well we'll "restore" a bogus
> > backlight state on a case where we shouldn't be touching backlight at
> > all.
> 
> Yeah this is definitely a WIP.  We'll probably need power well specific
> save/restore functions.  Depending on which well was toggled, the state
> that needs to be saved & restored will be different.  It may be best to
> try to push all this into modeset_setup_hw_state, since it should know
> which power wells are needed for different ops and thus restore the
> right state.  But that means ripping apart save/restore_state and
> putting bits in the right places, potentially with specific hooks like
> the uncore save/restore I added for other stuff.

setup_hw_state only deals with things which should be active (and restores
them when they're not active). But what we want here is to re-run the
->mode_set callback code when enabling a pipe again through dpms. So for
me it feels much saner to just add that callback there instead of
magically restoring register state in the get/put functions.

The reason for that is that at least long-term I expect the ->mode_set
callbacks to die and that we'll move everything which is in there either
into ->enable callbacks or into the compute config stage. With that we'll
have the guarantee that the hw touching code sequence is always the same,
no matter which exact upper level function caused a given state change
(modeset, dpms, resume, ...).

I don't really have an opinion about how we should handle this stuff on
the gem side of things. For that magically restoring the gem hw state in
the get/put functions sounds like the right approach. For gem we probably
also need some delayed actual put to have a bit of hysteresis. That's
kinda why I think we should have separate interfaces for display power
well management and gem/irq/aux power well management.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch