From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH] drm/i915: Fix eDP blank screen after S3 resume on HP desktops Date: Thu, 21 Jun 2012 15:17:10 +0200 Message-ID: References: <20120620080512.GA7170@phenom.ffwll.local> <20120620093651.GD7170@phenom.ffwll.local> <20120621123307.GD4704@phenom.ffwll.local> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTP id 2425E9E7F1 for ; Thu, 21 Jun 2012 06:17:12 -0700 (PDT) In-Reply-To: <20120621123307.GD4704@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, John Sundragon Waitz , Rodrigo Vivi List-Id: intel-gfx@lists.freedesktop.org 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 > > > > > > > > > > > > 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@ffwll.ch > Mobile: +41 (0)79 365 57 48 >