[Intel-gfx] [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v3)

[Jani Nikula]

On Thu, 07 Jan 2016, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Thu, Jan 07, 2016 at 10:15:00AM +0200, Jani Nikula wrote:
>> On Wed, 06 Jan 2016, Matt Roper <matthew.d.roper at intel.com> wrote:
>> > Our attempts save/restore panel power state in i915_suspend.c are
>> > causing unclaimed register warnings on BXT since the registers for this
>> > platform differ from older platforms.
>> >
>> > The big hammer suspend/resume shouldn't be necessary for PP since the
>> > connector/encoder hooks should already handle this.  In theory we could
>> > remove this for all platforms, but in practice it's likely that would
>> > cause some regressions since older platforms with LVDS may have
>> > incomplete PP handling.  For now we'll leave the PCH save/restore alone
>> > and change the non-PCH branch to only operate on gen <= 4 so that BXT
>> > and future platforms aren't included.
>> >
>> > v2: Typo fix: s/||/&&/
>> >
>> > v3: Change non-PCH condition to a gen <= 4 test rather than listing
>> >     VLV/CHV/BXT as specific platforms to exclude; should be more
>> >     future-proof as we add new platforms.  (Daniel)
>> >
>> > Cc: Vandana Kannan <vandana.kannan at intel.com>
>> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> > Cc: Daniel Vetter <daniel at ffwll.ch>
>> > Cc: drm-intel-fixes at lists.freedesktop.org
>> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>> > ---
>> > Although it's the direction we want to move, I'm not brave enough to blow away
>> > the entire PP save/restore for all platforms since I don't have the HW
>> > necessary to deal with the regressions that might pop up.  The consensus on IRC
>> > is that there probably will be a few regressions when we do that, so I'd rather
>> > have people with appropriate platform access make those changes.
>> 
>> This is the first step we want to take anyway.
>> 
>> Reviewed-by: Jani Nikula <jani.nikula at intel.com>
>
> Just looked at this some more, and the code here is the only code that
> restores PP registers. Except for vlv/chv, where we have a call to
> intel_dp_init_panel_power_sequencer_registers in vlv_power_sequencer_pipe.
>
> I think we first need to sprinkle more calls to restore PP registers
> around before we can disable this here for bxt. This patch here just
> papers over a legit bug, without fixing it really.

No, this patch doesn't paper over anything. The code that gets run for
Broxton before this patch doesn't save/restore any meaningful registers
that could possibly help with PP. This fixes unclaimed register
read/writes, and it's a valid fix as such.

The commit message should probably be fixed to say that we're not there
yet for Broxton with the hooks. But IMO this patch is a valid fix and
orthogonal to the issue of fixing the hooks.


BR,
Jani.


> -Daniel
>
>> 
>> 
>> 
>> >
>> >  drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
>> > index a2aa09c..a8af594 100644
>> > --- a/drivers/gpu/drm/i915/i915_suspend.c
>> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
>> > @@ -49,7 +49,7 @@ static void i915_save_display(struct drm_device *dev)
>> >  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
>> >  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
>> >  		dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
>> > -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
>> > +	} else if (INTEL_INFO(dev)->gen <= 4) {
>> >  		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
>> >  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
>> >  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);
>> > @@ -84,7 +84,7 @@ static void i915_restore_display(struct drm_device *dev)
>> >  		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>> >  		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
>> >  		I915_WRITE(PCH_PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
>> > -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
>> > +	} else if (INTEL_INFO(dev)->gen <= 4) {
>> >  		I915_WRITE(PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
>> >  		I915_WRITE(PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>> >  		I915_WRITE(PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center