[Intel-gfx] [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops

[Takashi Iwai]

At Wed, 20 Jun 2012 11:36:51 +0200,
Daniel Vetter wrote:
> 
> On Wed, Jun 20, 2012 at 11:21:11AM +0200, Takashi Iwai wrote:
> > At Wed, 20 Jun 2012 10:05:12 +0200,
> > Daniel Vetter wrote:
> > > 
> > > On Wed, Jun 20, 2012 at 09:17:41AM +0200, Takashi Iwai wrote:
> > > > This patch fixes the problem on some HP desktop machines with eDP
> > > > which give blank screens after S3 resume.
> > > > 
> > > > The problem looks like a timing issue.  Although BLC_PWM_CPU_CTL
> > > > register is already restored at the beginning of resume, it doesn't
> > > > seem to take effect.  Simply re-issuing the  register write restores
> > > > the backlight gracefully.
> > > > 
> > > > Tested with 3.5-rc3 kernel.
> > > > 
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49233
> > > > 
> > > > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > > 
> > > This patch looks very fishy as-is, simply because I don't like adding
> > > random calls to random functions at pretty much random places without any
> > > hint why it works.
> > 
> > It's difficult to know exactly why it works.  We can only guess, but
> > nothing more than that, just like between husband and wife :)
> 
> Yeah I know, but I've been burnt a bit recently with too much
> cargo-culting. Hence why I'm so dense about this stuff ;-)
> 
> > > Just a few weeks ago we've had tons of fun with writes
> > > that don't stick when the ring isn't yet set up. Can you please check with
> > > a bunch of printks what exactly happens to the value of BLC_PWM_CPU_CTL?
> > > I.e. whether the write doesn't stick or whether it gets reset somewhere
> > > between the register restore functions and the new place.
> > 
> > The register value is already set, and not touched after that.
> > I've already checked.
> 
> So immediately before the backlight_update_status call we have the
> _correct_ value in BLC_PWM_CPU_CTL? That would be indeed puzzling ...

Yes.

> > > I suspect the opregion_init call could allow the bios to frob with these
> > > registers. If that's the case, the patch is missing a comment that to that
> > > effect.
> > 
> > Right.  That's also my suspect.
> > 
> > > Essentially I want to know why this place here works and why not move the
> > > call a few lines up or down.
> > 
> > A few lines down works definitely.  Even just writing to sysfs works.
> > A few lines up, well, I need to check where is the border line.
> 
> Thanks, that would be interesting. Especially if it's opregion that we
> need to have set up before restoring the backlight properly.

This is getting really puzzling.  It's not about the opregion.
Just some order of the register restoration.

Essentially the patch below fixes the problem.  But I have absolutely
no idea why.


Takashi

---
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 0ede02a..0f89dd0 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -740,8 +740,8 @@ static void i915_restore_display(struct drm_device *dev)
 	if (HAS_PCH_SPLIT(dev)) {
 		I915_WRITE(BLC_PWM_PCH_CTL1, dev_priv->saveBLC_PWM_CTL);
 		I915_WRITE(BLC_PWM_PCH_CTL2, dev_priv->saveBLC_PWM_CTL2);
-		I915_WRITE(BLC_PWM_CPU_CTL, dev_priv->saveBLC_CPU_PWM_CTL);
 		I915_WRITE(BLC_PWM_CPU_CTL2, dev_priv->saveBLC_CPU_PWM_CTL2);
+		I915_WRITE(BLC_PWM_CPU_CTL, dev_priv->saveBLC_CPU_PWM_CTL);
 		I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->savePP_ON_DELAYS);
 		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->savePP_OFF_DELAYS);
 		I915_WRITE(PCH_PP_DIVISOR, dev_priv->savePP_DIVISOR);