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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 15 Oct 2013 17:09:19 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2013/10/14 Jesse Barnes <jbarnes@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@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.

-- 
Jesse Barnes, Intel Open Source Technology Center