Bug 34601

Created attachment 43698 [details]
The photo of screen mess  by a camera

I suspect an interrupted modesetting. Were there no warnings in dmesg? How reproducibile is this? (And is this fixed by ce453d81cb0397aa7d5148984f51907e14072d74?)

(In reply to comment #2)
> I suspect an interrupted modesetting. Were there no warnings in dmesg? How
> reproducibile is this? (And is this fixed by
> ce453d81cb0397aa7d5148984f51907e14072d74?)

Test in commit ce453,the problem still exists.Reproduce,do rotation (left-right-inverted-normal as a time),it comes out about 3-5 times.Pls see the attached dmesg.

Created attachment 43784 [details]
The dmesg with the commit ce453

I tried reproducing on current ddx+drm-intel-next using a Ubuntu Maverick netbook with awesome, gnome+openbox, gnome+compiz and KDE/kwin - worked as expected. And no errors in your dmesg, so not the failure mode I was expecting.

Can you attach the Xorg.log, xrandr --verbose and intel_reg_dumper after the screen is messed up?

Created attachment 44456 [details]
Xorg.0.log when testing in the commit 47ae63e

Created attachment 44457 [details]
The xrandr --verbose.log when testing in the commit 47ae63e

Created attachment 44458 [details]
The intel_reg_dumper.log

By bisected,show b24e71798871089da1a4ab049db2800afc1aac0c is the first bad commit.

commit b24e71798871089da1a4ab049db2800afc1aac0c
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Tue Jan 4 15:09:30 2011 -0800

    drm/i915: add pipe/plane enable/disable functions

    Add plane enable/disable functions to prevent duplicated code and allow
    us to easily check for plane enable/disable requirements (such as pipe
    enable, plane status, pll status etc).

    Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok, something should show up in the intel_reg_dumper then. Let's be thorough and get reg dumps before and after rotations with b24e71798871089^ and compare them against dumps before and after rotations with HEAD.

Sorry, not the commit I was thinking of. The register dumps won't help, since it appears to be more of sequence/timing issue - would also may help explain just why it may only affect certain machines and configurations.

Created attachment 44521 [details] [review]
Call intel_flush_display_plane() when enabling the plane.

Spotted one intel_flush_display_plane() that became a wait-for-vblank.

The other possibility is that changing bits of PIPECONF (or DSPCNTR) is invalid unless PIPECONF_ENABLE is also set. That appears to be the other subtle change in the patch.

I thought I preserved the pipeconf enable stuff by keeping a pipe & plane enable in crtc_mode_set where they would have been set before... Is there another place where that changed?  I can imagine that having an effect too...

(In reply to comment #12)
> Created an attachment (id=44521) [details]
> Call intel_flush_display_plane() when enabling the plane.

In your patch(id=44521),
+   intel_flush_display_plane(plane);
Maybe there is a mistype in your patch which caused it failed to compile. I
guess maybe you meant as following:
+   intel_flush_display_plane(dev_priv, plane);
After we applied the new patch, it works fine in commit 47ae63e0c2e5.

Ok, since that patch was really a conglomeration of a few different "fixes", can you try the broken down patches on drm-intel-staging:


commit fc944efc615bbe86bfd43678d4495c62e1dd5f04
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Mar 17 07:20:20 2011 +0000

    drm/i915: Remove surplus POSTING_READs before wait_for_vblank
    
    ... as wait_for_vblank (and friends) will do a flush of the MMIO writes
    anyway.

commit 60f1aa6bd4b575c8a8247e9bf1e175e5d69774c6
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Mar 17 07:18:29 2011 +0000

    drm/i915: skip redundant operations whilst enabling pipes and planes
    
    If the pipe or plane is already enabled, then we do not need to enable
    it again and can skip the delay.

commit d0da72d743843bc01583450f94872b6a156bc893
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Mar 17 07:19:52 2011 +0000

    drm/i915: Flush the plane enable using the DSPCNTR latch

commit 0c9c82e5431db8ab8c94bc243047d5d6a5341878
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Mar 17 07:27:17 2011 +0000

    drm/i915: Only wait for vblank after pipe enabling on gen2
    
    Later gen can proceed immediately with the modesetting as it is
    automatically double-buffered.

If you can test each in order and report when the failure stops, that would be perfect. Thanks!

(In reply to comment #15)
Test in above four commits you given, only one commit fc944ef is bad and the other three are all good.

Meng, can you clarify which patch actually fixed the issue?  I'd expect it to be the display plane flush patch, while the other two had no effect.  Did you test them all independently?

After reviewing the proposed set of fixes, we dropped the last two patches ("only wait for vblank after pipe disable on gen2" and "flush the plane enable using DSPCNTR"). I've pushed the patch-set to drm-intel-staging, can you please confirm that branch behaves correctly on your machine? Thanks.

I am waiting on confirmation that

commit 2fac49a13526909b704ba75110d3f17fae3bf3af
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Mar 17 07:18:29 2011 +0000

    drm/i915: skip redundant operations whilst enabling pipes and planes
    
    If the pipe or plane is already enabled, then we do not need to enable
    it again and can skip the delay. Similarly if it is already disabled
    when we want to disable it, we can also skip it.
    
    This fixes a regression from b24e717988, which caused the LVDS
    output on one PineView machine to become corrupt after changing
    orientation several times.
    
    References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
    Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Reviewed-by: Keith Packard <keithp@keithp.com>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

drm-intel-staging is sufficient to fix this bug.

(In reply to comment #19)
Test in the commit 2fac4,it works fine on Pineview.
we can compile it successfully unless deleted the sentence "c:702 drivers/usb/serial/usb_wwan.c".

commit 00d70b15125030391d17baab2c2f70f93b3339a6
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Mar 17 07:18:29 2011 +0000

    drm/i915: skip redundant operations whilst enabling pipes and planes
    
    If the pipe or plane is already enabled, then we do not need to enable
    it again and can skip the delay. Similarly if it is already disabled
    when we want to disable it, we can also skip it.
    
    This fixes a regression from b24e717988, which caused the LVDS
    output on one PineView machine to become corrupt after changing
    orientation several times.
    
    References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
    Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Reviewed-by: Keith Packard <keithp@keithp.com>
    Tested-by: mengmeng.meng@intel.com

Verified with the commit 00d70b15125030391d17baab2c2f70f93b3339a6, it works fine.

Closing old verified+fixed.