Bug 90468

Summary: [IVB Bisected] Both DVI/DP monitors are black after X rotation + S3
Product: DRI Reporter: Jeff Zheng <jeff.zheng>
Component: DRM/IntelAssignee: Ander Conselvan de Oliveira <conselvan2>
Status: CLOSED FIXED QA Contact: Intel GFX Bugs mailing list <intel-gfx-bugs>
Severity: normal    
Priority: medium CC: bin.a.xu, christophe.prigent, intel-gfx-bugs
Version: unspecified   
Hardware: Other   
OS: All   
See Also: https://bugs.freedesktop.org/show_bug.cgi?id=90396
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
dmesg log
none
dmesg update
none
dmesg run with first bad commit
none
dmesg before first bad commit
none
dmesg on drm-intel-next-fixes none

Description Jeff Zheng 2015-05-15 10:13:54 UTC
Created attachment 115797 [details]
dmesg log

==System Environment==
--------------------------
DVI + DP monitors
Regression: Yes, don't see this issue on drm-intel-testing-2015-03-13. 
Non-working platforms: IVB

==kernel==
--------------------------
-testing: drm-intel-testing-2015-05-08 (fails)

==Bug detailed description==
-----------------------------
Both DVI/DP monitors are black after X rotation + S3


==Reproduce steps==
---------------------------- 
1. Reboot with DVI/DP monitors connected
2. xinit &
3. xrandr --output DP2 --rotation left
4. echo mem > /sys/power/state
5. Resume by clicking the keyboard
6. Both DVI/DP monitors are black
Comment 1 Ander Conselvan de Oliveira 2015-05-15 13:08:38 UTC
(In reply to Jeff Zheng from comment #0)
> Regression: Yes, don't see this issue on drm-intel-testing-2015-03-13. 

Please bisect. Also note that your dmesg doesn't include debug information.
Comment 2 xubin 2015-05-18 13:37:10 UTC
==Bisect results==
----------------------------
Bisect shows: 0a9ab303b87a94115e361a7f3a15d9f481bc453b is the first bad commit.
commit 0a9ab303b87a94115e361a7f3a15d9f481bc453b
Author:     Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
AuthorDate: Tue Apr 21 17:13:04 2015 +0300
Commit:     Daniel Vetter <daniel.vetter@ffwll.ch>
CommitDate: Fri May 8 13:04:01 2015 +0200

    drm/i915: Remove all *_pipes flags from modeset

    Set the mode_changed field on the crtc_states and use that instead.

    Note that even though this patch doesn't completely replace the logic in
    intel_modeset_affected_pipes(), that logic was never fully used to its
    full extent. Since the commit mentioned below, modeset_pipes and
    prepare_pipes would only contain at most the pipe for which the set_crtc
    ioctl was called. We can grow back that logic when the time comes.

    commit b6c5164d7bf624f3e1b750787ddb983150c5117c
    Author: Daniel Vetter <daniel.vetter@ffwll.ch>
    Date:   Fri Apr 12 18:48:43 2013 +0200

        drm/i915: Fixup Oops in the pipe config computation

    v2: Don't set mode_changed unconditionally for modeset_crtc. (Ander)
        Check for needs_modeset() before trying to allocate a PLL. (Ander)
        Only call .crtc_enable() for pipes that were disabled. (Maarten)

    Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
    Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Comment 3 Ander Conselvan de Oliveira 2015-05-19 07:49:19 UTC
Please also provide dmesg with drm.debug=0x1e log_buf_len=4M in the kernel command line.
Comment 4 xubin 2015-05-20 03:20:56 UTC
Created attachment 115905 [details]
dmesg update
Comment 5 Ander Conselvan de Oliveira 2015-05-20 06:34:25 UTC
With all the changes introduced since the first bad commit it gets a bit hard to follow the dmesg. Could you provide another dmesg running with that commit? A dmesg for the commit before would also be helpful, for comparison.
Comment 6 xubin 2015-05-20 13:37:51 UTC
Created attachment 115925 [details]
dmesg run with first bad commit
Comment 7 xubin 2015-05-20 13:41:20 UTC
Created attachment 115926 [details]
dmesg before first bad commit

Here,I have give two dmesgs,one is dmesg running with first bad commit,another is dmesg before the first bad commit.Then ,you can have a comparison.
Comment 8 Maarten Lankhorst 2015-06-08 06:45:13 UTC
Could you retest with latest drm-intel-nightly?
Comment 9 Jeff Zheng 2015-06-09 07:34:30 UTC
Tested with 2015_06_09/drm-intel-nightly/4605bd59a9357d37054e1d9cf0345a73b6743df4, this issue got fixed.
Comment 10 Ander Conselvan de Oliveira 2015-06-09 08:25:12 UTC
I'm afraid this issue might still exist in drm-intel-next-fixes. Could you please confirm?
Comment 11 Jeff Zheng 2015-06-09 08:39:30 UTC
Tried with /2015_06_09/drm-intel-next-queued/2da44aae8144b8380f4e45688e570d4c7a4ef2c4.
This issue does not exist.
Comment 12 Jeff Zheng 2015-06-09 08:43:25 UTC
Tried with 2015_06_09/drm-intel-fixes/8ce7da474f3063f57ac446eb428945a9852e102d
This issue does not exist.
Comment 13 Jani Nikula 2015-06-09 09:07:59 UTC
(In reply to Ander Conselvan de Oliveira from comment #10)
> I'm afraid this issue might still exist in drm-intel-next-fixes. Could you
> please confirm?

You did not try drm-intel-next-fixes.
Comment 14 Jeff Zheng 2015-06-10 01:47:02 UTC
Created attachment 116409 [details]
dmesg on drm-intel-next-fixes

Yes, this issue still exist in drm-intel-next-fixes commit bf546f8158e2df2656494a475e6235634121c87c.
Comment 15 Jeff Zheng 2015-06-10 01:59:56 UTC
Change status to NEW, please change to status that you think proper.

One more question is: QA basically tests two branches:
  remotes/origin/drm-intel-nightly
  remotes/origin/drm-intel-testing

And will check those two branches when there is bug:
  remotes/origin/drm-intel-fixes
  remotes/origin/drm-intel-next-queued

According to your comments, does it mean that QA should check remotes/origin/drm-intel-next-fixes instead of remotes/origin/drm-intel-fixes ?
Comment 16 Ander Conselvan de Oliveira 2015-06-10 06:56:44 UTC
(In reply to Jeff Zheng from comment #15)
> Change status to NEW, please change to status that you think proper.
> 
> One more question is: QA basically tests two branches:
>   remotes/origin/drm-intel-nightly
>   remotes/origin/drm-intel-testing
> 
> And will check those two branches when there is bug:
>   remotes/origin/drm-intel-fixes
>   remotes/origin/drm-intel-next-queued
> 
> According to your comments, does it mean that QA should check
> remotes/origin/drm-intel-next-fixes instead of
> remotes/origin/drm-intel-fixes ?

During this development cycle we have a situation that doesn't always happens, we have branches for three different kernel versions:

patches from drm-intel-fixes will go to kernel v4.1;
patches from drm-intel-next-fixes will go to kernel v4.2;
patches from drm-intel-next-queued will go to kernel v4.3.

The patch series that fixed this issue in -nightly is not going to kernel v4.2, so we'll need a different fix for that version.

If I understand correcly, once v4.1 is released, we'll start tracking fixes for v4.2 in drm-intel-fixes.

To sum it up, right now we have an additional branch that needs testing, but things should go back to normal soon, once v4.1 is released.

Jani, anything you might want to add/correct?
Comment 17 Jeff Zheng 2015-06-10 07:01:37 UTC
Got it. Thanks for clarification.
Comment 18 Jani Nikula 2015-06-10 08:26:06 UTC
(In reply to Ander Conselvan de Oliveira from comment #16)
> If I understand correcly, once v4.1 is released, we'll start tracking fixes
> for v4.2 in drm-intel-fixes.

Yes. To be precise, the switch happens at v4.2-rc1, i.e. after the merge window closes.

> To sum it up, right now we have an additional branch that needs testing, but
> things should go back to normal soon, once v4.1 is released.
> 
> Jani, anything you might want to add/correct?

Just that we'll need to either a) identify and backport the fix from drm-intel-next-queued (currently aimed at v4.3) to drm-intel-next-fixes (currently aimed at v4.2), *or* if that's not feasible, b) write a different fix for drm-intel-next-fixes.

The sooner we do this, the more liberties we have in fixing it.
Comment 19 Ander Conselvan de Oliveira 2015-06-10 15:12:50 UTC
(In reply to Jani Nikula from comment #18)
> Just that we'll need to either a) identify and backport the fix from
> drm-intel-next-queued (currently aimed at v4.3) to drm-intel-next-fixes
> (currently aimed at v4.2), *or* if that's not feasible,

Unfortunately the fix in -nightly was one of the patches that was reverted because of causing other issues.

> b) write a different fix for drm-intel-next-fixes.

The root cause here is the same as bug 90396. The way the atomic state is set up causes the call to intel_crtc_restore_mode() to be aborted, since it tries to enable more than once CRTC but only sets up the encoders for one of them.

I tested a simple fix for the issue above today, but it uncovered another issue. The modeset path now copies the atomic state to the staged config. But the force restore path does up to 3 modesets, and each of those need to have the staged config from before the suspend. Since that's overwritten, we only restore the mode correctly for pipe A.
Comment 20 Ander Conselvan de Oliveira 2015-06-11 08:56:50 UTC
Could you test the following branch:

https://github.com/anderco/linux/tree/fdo/90468

It has three patches on top of drm-intel-next-fixes that should fix this issue.
Comment 21 Jeff Zheng 2015-06-12 05:22:18 UTC
I failed to clone, could you please attach your patch so that I can apply to drm-intel-next-fixes?

# tsocks git clone https://github.com/anderco/linux/tree/fdo/90468
Cloning into '90468'...
13:03:25 libtsocks(14346): Local network specification (localhost) is not validly constructed on line 20 in configuration file
error: The requested URL returned error: 403 while accessing https://github.com/anderco/linux/tree/fdo/90468/info/refs
fatal: HTTP request failed
Comment 22 ye.tian 2015-06-12 06:09:39 UTC
(In reply to Ander Conselvan de Oliveira from comment #20)
> Could you test the following branch:
> 
> https://github.com/anderco/linux/tree/fdo/90468
> 
> It has three patches on top of drm-intel-next-fixes that should fix this
> issue.

Test it on fdo/90468 branch, this problem does not exists.

BTW, Boot up causes dmesg WARNING.

output:
-------------------------

[    2.477493] [drm:intel_set_pch_fifo_underrun_reporting [i915]] *ERROR* uncleared pch fifo underrun on pch transcoder A
[    2.477515] [drm:cpt_irq_handler [i915]] *ERROR* PCH transcoder A FIFO underrun
[    2.477921] [drm:cpt_set_fdi_bc_bifurcation] enabling fdi C rx
Comment 23 Ander Conselvan de Oliveira 2015-06-12 06:37:55 UTC
(In reply to ye.tian from comment #22)
> BTW, Boot up causes dmesg WARNING.
> 
> output:
> -------------------------
> 
> [    2.477493] [drm:intel_set_pch_fifo_underrun_reporting [i915]] *ERROR*
> uncleared pch fifo underrun on pch transcoder A
> [    2.477515] [drm:cpt_irq_handler [i915]] *ERROR* PCH transcoder A FIFO
> underrun
> [    2.477921] [drm:cpt_set_fdi_bc_bifurcation] enabling fdi C rx

Does this happen also with drm-intel-next-fixes, or only with the fixes in my fdo/90468 branch?
Comment 24 ye.tian 2015-06-12 07:00:04 UTC
Both all.
Comment 25 ye.tian 2015-06-12 07:01:33 UTC
output info with drm-intel-next-fixes.
-------------------------------
[root@x-ivb6 ~]# dmesg  | grep ERROR
[    2.468569] [drm:intel_set_cpu_fifo_underrun_reporting [i915]] *ERROR* uncleared fifo underrun on pipe A
[    2.468590] [drm:ironlake_irq_handler [i915]] *ERROR* CPU pipe A FIFO underrun
[    2.470690] [drm:intel_set_pch_fifo_underrun_reporting [i915]] *ERROR* uncleared pch fifo underrun on pch transcoder A
[    2.470709] [drm:cpt_irq_handler [i915]] *ERROR* PCH transcoder A FIFO underrun
Comment 26 Ander Conselvan de Oliveira 2015-06-12 07:06:34 UTC
OK, so we need a separate bug report for that FIFO underrun, with a dmesg with drm.debug.
Comment 27 ye.tian 2015-06-12 08:12:08 UTC
There is a bug 89806 tracking it.
Comment 28 Jani Nikula 2015-06-17 12:14:03 UTC
Fixed by

commit 0d26fb891a119d78d8e4b345cfd0be0f2112cb7a
Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Date:   Tue Jun 16 11:49:44 2015 +0300

    drm/i915: Don't update staged config during force restore modesets

in drm-intel-next-fixes for v4.2 merge window.

Ye Tian, please verify we have indeed fixed this on drm-intel-next-fixes, and please file a new bug if you can reproduce this on drm-intel-nightly.
Comment 29 Jani Nikula 2015-06-17 12:15:32 UTC
Additionally

commit 4ed9fb371ccdfe465bd3bbb69e4cad5243e6c4e2
Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Date:   Tue Jun 16 11:49:45 2015 +0300

    drm/i915: Don't set enabled value of all CRTCs when restoring the mode

in drm-intel-next-fixes.
Comment 30 Jari Tahvanainen 2016-10-10 11:50:07 UTC
Closing resolved+fixed based on verification by ye.tian.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.