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

[Takashi Iwai]

At Thu, 21 Jun 2012 14:33:07 +0200,
Daniel Vetter wrote:
> 
> 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.

Sounds possible.

> 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.

Sure, will be sent soon later.

> 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.

OTOH, these allow shorter initialization than the normal full init.
But I agree with the point of code maintenance mess by such.


thanks,

Takashi


> 
> 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
>