Bug 90468

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

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

Please also provide dmesg with drm.debug=0x1e log_buf_len=4M in the kernel command line.

Created attachment 115905 [details]
dmesg update

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.

Created attachment 115925 [details]
dmesg run with first bad commit

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.

Could you retest with latest drm-intel-nightly?

Tested with 2015_06_09/drm-intel-nightly/4605bd59a9357d37054e1d9cf0345a73b6743df4, this issue got fixed.

I'm afraid this issue might still exist in drm-intel-next-fixes. Could you please confirm?

Tried with /2015_06_09/drm-intel-next-queued/2da44aae8144b8380f4e45688e570d4c7a4ef2c4.
This issue does not exist.

Tried with 2015_06_09/drm-intel-fixes/8ce7da474f3063f57ac446eb428945a9852e102d
This issue does not exist.

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

Created attachment 116409 [details]
dmesg on drm-intel-next-fixes

Yes, this issue still exist in drm-intel-next-fixes commit bf546f8158e2df2656494a475e6235634121c87c.

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 ?

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

Got it. Thanks for clarification.

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

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

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.

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

(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

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

Both all.

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

OK, so we need a separate bug report for that FIFO underrun, with a dmesg with drm.debug.

There is a bug 89806 tracking it.

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.

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.

Closing resolved+fixed based on verification by ye.tian.