On Thu, Jun 21, 2012 at 02:11:50PM +0200, Takashi Iwai wrote: > At Wed, 20 Jun 2012 11:39:51 +0200, > Takashi Iwai wrote: > > > > 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. > > Daniel, do you have any clue about this? > > Is it a strict order of these registers? Could very well be that the write does not get forwarded internally when the BLC stuff isn't enabled yet. Or the other way round. E.g. the new pipe select stuff for the blc clock I've implemented just recently only works while the backlight is off. I'm much happier with this diff than the older one to just hit the regs harder, so can you please bake this into a proper patch? And please add a 1-line comment to the code that for some unknown reason we need to write the _CTL reg after the _CTL2. Also, I loathe this save/restore register dance. It duplicates code we already have, but with the twist that we might accidentally get the ordering (or some timing constraint) slightly wrong. Resulting in broken s/r, at least on some machines. Imo we should try to wean off these in the long term. Thanks, Daniel > > > > Takashi > > > > > 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); -- Daniel Vetter Mail: daniel at ffwll.ch Mobile: +41 (0)79 365 57 48
