Re: i915 regression on 3.6-rc1: lid blanks screen

[Hugh Dickins]

On Fri, 10 Aug 2012, Takashi Iwai wrote:
> At Fri, 10 Aug 2012 14:35:13 +0200,
> Daniel Vetter wrote:
> > 
> > On Fri, Aug 10, 2012 at 1:59 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > > At Mon, 6 Aug 2012 11:25:30 -0700 (PDT),
> > > Hugh Dickins wrote:
> > >>
> > >> On Mon, 6 Aug 2012, Daniel Vetter wrote:
> > >> > On Mon, Aug 6, 2012 at 6:21 AM, Hugh Dickins <hughd@xxxxxxxxxx> wrote:
> > >> > > On Sun, 5 Aug 2012, Takashi Iwai wrote:
> > >> > >> At Sat, 4 Aug 2012 10:01:13 -0700 (PDT),
> > >> > >> Hugh Dickins wrote:
> > >> > >> >
> > >> > >> > Sorry to report that with 3.6-rc1, closing and opening the lid on
> > >> > >> > this ThinkPad T420s leaves the screen blank, and I have to reboot.
> > >> > >> >
> > >> > >> > Bisection led to this commit, and reverting indeed gets my screen back:
> > >> > >> >
> > >> > >> > commit 520c41cf2fa029d1e8b923ac2026f96664f17c4b
> > >> > >> > Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > >> > >> > Date:   Wed Jul 11 16:27:52 2012 +0200
> > >> > >> >
> > >> > >> >     drm/i915/lvds: ditch ->prepare special case
> > >> > > ...
> > >> > >>
> > >> > >> Hm, it's surprising.
> > >> > >>
> > >> > >> Could you check whether the counter-part intel_lvds_enable() is
> > >> > >> called?  If the prepare callback affects, it must be from the mode
> > >> > >> setting (drm_crtc_helper_set_mode()).
> > >> > >
> > >> > > Yes, I put a dump_stack() in both, and intel_lvds_enable() gets called
> > >> > > about 0.28 seconds after the intel_lvds_disable() when I lift the lid;
> > >> > > but with no video display until I revert that commit.
> > >> >
> > >> > Can you please boot with drm.debug=0xe added to your kernel cmdline,
> > >> > reproduce the issue (with the two dump_stack calls added) and then
> > >> > attach the full dmesg?
> > >>
> > >> Collected, I'll send it to you both privately in a moment.
> > >>
> > >> >
> > >> > Also a few other things to try: What happens if you do a modeset on
> > >> > the LVDS while it's still working, e.g.
> > >>
> > >> In the dmesg, I've only gone to runlevel 3, simply working on the
> > >> console without startx.  For these xrandrs to work, I did startx
> > >> and used the graphics screen.
> > >
> > > OK, now I can see the problem here, too.  The key is that it happens
> > > only on Linux console, not on X.  That's why no one else reported.
> > > I guess the problem can be seen on many laptops with LVDS on PCH.

A correction there: for me it was happening both on X and on console;
but once I found that it happened even on the simple console, I mostly
stuck to bisecting and testing on that.

> > >
> > > Looking at intel_reg_dumper output, BLC_PWM_CPU_CTL is 0 while other
> > > registers are set correctly.  This seems coming from the rewrite of
> > > backlight control code by commit
> > >   24ded204: drm/i915: properly enable the blc controller on the right pipe
> > > and
> > >   a4f32fc3: drm/i915: don't forget the PCH backlight registers
> > >
> > > While the latter fixes the regression by the former commit, it still
> > > doesn't cover this regression.
> > >
> > > I don't know the exact hardware behavior, but it looks like that
> > > resetting BLC_PWM_PCH_CTL2 and BLC_PWM_PCH_CTL1 clears the
> > > BLC_PWM_CPU_CTL (oh what confusing reg names).
> > >
> > > FWIW, the commit 520c41cf you mentioned is no direct cause.  This
> > > patch works fine on the top of 3.5 kernel.  But it's like a bad drug,
> > > the combination of this and other two commits break things.
> > >
> > > The patch below is my quick fix.  It worked on an HP laptop.
> > > Hugh, could you give it a try?
> > 
> > Hm, this sounds eerily familiar to the backlight bug you've recently fixed in
> > 
> > commit 6db65cbb941f9d433659bdad02b307f6d94465df
> > Author: Takashi Iwai <tiwai@xxxxxxx>
> > Date:   Thu Jun 21 15:30:41 2012 +0200
> > 
> >     drm/i915: Fix eDP blank screen after S3 resume on HP desktops
> > 
> > Have you checked other code-paths for such issues?
> 
> Not yet, I'd leave such a joy rather to you guys :)
> 
> > The resume code
> > seems to follow this order already ... also, when you submit this
> > patch, can you please add a small comment to explain the ordering
> > constraint, like in the resume register restore function?
> 
> Sure, I'll add a comment and resubmit once when I hear it really fixes
> on Hugh's machine, too.

Indeed, your patch really fixes it on my machine: many thanks!

Hugh

> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > Thanks, Daniel
> > 
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> > >
> > > ===
> > > From: Takashi Iwai <tiwai@xxxxxxx>
> > > Subject: [PATCH] drm/i915: Fix blank panel at reopening lid on Linux console
> > >
> > > When you reopen the lid on Linux console on a laptop with PCH, the
> > > panel suddenly goes blank.  It seems because BLC_PWM_CPU_CTL register
> > > is cleared when BLC_PWM_PCH_CTL1 and BLC_PWM_PCH_CTL2 registers are
> > > played.
> > >
> > > This patch fixes the problem by setting BLC_PWM_CPU_CTL after enabling
> > > BLC_PWM_PCH_CTL_1 and _2 registers.
> > >
> > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/intel_panel.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > > index 10c7d39..d640425 100644
> > > --- a/drivers/gpu/drm/i915/intel_panel.c
> > > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > > @@ -311,9 +311,6 @@ void intel_panel_enable_backlight(struct drm_device *dev,
> > >         if (dev_priv->backlight_level == 0)
> > >                 dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
> > >
> > > -       dev_priv->backlight_enabled = true;
> > > -       intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
> > > -
> > >         if (INTEL_INFO(dev)->gen >= 4) {
> > >                 uint32_t reg, tmp;
> > >
> > > @@ -326,7 +323,7 @@ void intel_panel_enable_backlight(struct drm_device *dev,
> > >                  * we don't track the backlight dpms state, hence check whether
> > >                  * we have to do anything first. */
> > >                 if (tmp & BLM_PWM_ENABLE)
> > > -                       return;
> > > +                       goto set_level;
> > >
> > >                 if (dev_priv->num_pipe == 3)
> > >                         tmp &= ~BLM_PIPE_SELECT_IVB;
> > > @@ -347,6 +344,10 @@ void intel_panel_enable_backlight(struct drm_device *dev,
> > >                         I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
> > >                 }
> > >         }
> > > +
> > > +set_level:
> > > +       dev_priv->backlight_enabled = true;
> > > +       intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
> > >  }
> > >
> > >  static void intel_panel_init_backlight(struct drm_device *dev)
> > > --
> > > 1.7.11.4
> > >
> > 
> > 
> > 
> > -- 
> > Daniel Vetter
> > daniel.vetter@xxxxxxxx - +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/