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

[Takashi Iwai <tiwai@suse.de>]

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.
> 
> I understand there's also an nVidia graphics device in here,
> but I have that configured out, preferring to use the i915:
> 
> 00:02.0 VGA compatible controller: Intel Corporation 2nd Generation Core Processor Family Integrated Graphics Controller (rev 09) (prog-if 00 [VGA controller])
> 	Subsystem: Lenovo Device 21d3
> 	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
> 	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
> 	Latency: 0
> 	Interrupt: pin A routed to IRQ 41
> 	Region 0: Memory at f1400000 (64-bit, non-prefetchable) [size=4M]
> 	Region 2: Memory at e0000000 (64-bit, prefetchable) [size=256M]
> 	Region 4: I/O ports at 5000 [size=64]
> 	Expansion ROM at <unassigned> [disabled]
> 	Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit-
> 		Address: fee0100c  Data: 41c2
> 	Capabilities: [d0] Power Management version 2
> 		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
> 		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
> 	Capabilities: [a4] PCI Advanced Features
> 		AFCap: TP+ FLR+
> 		AFCtrl: FLR-
> 		AFStatus: TP-
> 	Kernel driver in use: i915
> 
> Bisection led to this commit, and reverting indeed gets my screen back:
> 
> commit 520c41cf2fa029d1e8b923ac2026f96664f17c4b
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Jul 11 16:27:52 2012 +0200
> 
>     drm/i915/lvds: ditch ->prepare special case
>     
>     LVDS is the first output where dpms on/off and prepare/commit don't
>     perfectly match. Now the idea behind this special case seems to be
>     that for simple resolution changes on the LVDS we don't need to stop
>     the pipe, because (at least on newer chips) we can adjust the panel
>     fitter on the fly.
>     
>     There are a few problems with the current code though:
>     - We still stop and restart the pipe unconditionally, because the crtc
>       helper code isn't flexible enough.
>     - We show some ugly flickering, especially when changing crtcs (this
>       the crtc helper would actually take into account, but we don't
>       implement the encoder->get_crtc callback required to make this work
>       properly).
>     
>     So it doesn't even work as advertised. I agree that it would be nice
>     to do resolution changes on LVDS (and also eDP) whithout blacking the
>     screen where the panel fitter allows to do that. But imo we should
>     implement this as a special case a few layers up in the mode set code,
>     akin to how we already detect simple framebuffer changes (and only
>     update the required registers with ->mode_set_base).
>     
>     Until this is all in place, make our lives easier and just rip it out.
>     
>     Also note that this seems to fix actual bugs with enabling the lvds
>     output, see:
>     
>     http://lists.freedesktop.org/archives/intel-gfx/2012-July/018614.html
>     
>     Cc: Takashi Iwai <tiwai@suse.de>
>     Cc: Giacomo Comes <comes@naic.edu>
>     Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
>     Tested-by: Takashi Iwai <tiwai@suse.de>
>     Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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


Takashi

> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 49f09a8..e05c0d3 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -409,13 +409,7 @@ static void intel_lvds_prepare(struct drm_encoder *encoder)
>  {
>  	struct intel_lvds *intel_lvds = to_intel_lvds(encoder);
>  
> -	/*
> -	 * Prior to Ironlake, we must disable the pipe if we want to adjust
> -	 * the panel fitter. However at all other times we can just reset
> -	 * the registers regardless.
> -	 */
> -	if (!HAS_PCH_SPLIT(encoder->dev) && intel_lvds->pfit_dirty)
> -		intel_lvds_disable(intel_lvds);
> +	intel_lvds_disable(intel_lvds);
>  }
>  
>  static void intel_lvds_commit(struct drm_encoder *encoder)
>