Bug 34601

Summary: [Pineview bisected]Screen mess when doing rotation
Product: DRI Reporter: meng <mengmeng.meng>
Component: DRM/IntelAssignee: Chris Wilson <chris>
Status: CLOSED FIXED QA Contact:
Severity: major    
Priority: high CC: jbarnes, xunx.fang
Version: unspecified   
Hardware: All   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
The photo of screen mess by a camera
none
The dmesg with the commit ce453
none
Xorg.0.log when testing in the commit 47ae63e
none
The xrandr --verbose.log when testing in the commit 47ae63e
none
The intel_reg_dumper.log
none
Call intel_flush_display_plane() when enabling the plane. none

Description meng 2011-02-23 01:44:35 UTC
System Environment:
--------------------------
Platform:         Pineview
Libdrm:		  (master)2.4.23-14-ga697fb6acad7992c3d23bb6a663663694782eb7b
Mesa:		  (master)94ccc31ba4f64ac480137fd90f1ded44d2072f6e
Xserver:	  (master)xorg-server-1.9.99.902-3-g93a73993708b1345c86ec3ec06b02ed236595673
Xf86_video_intel: (master)2.14.0-34-g287dd56708fa893ee21591d8547e0747fda197af
Cairo:		  (master)90e1ef2d80b252cdc528d8d8d57cace109034e0b
Kernel:	          (drm-intel-next) f4166442e7533ca2668d639d939b1bbdb8e6b423

Bug detailed description:
-------------------------
On gnome-desktop, do rotation using xrandr, which could
make screen mess.It's only on Pineview.Pls see attached photo by a camera. 
Especially,
1.It's kernel regression.3c0eee3fe6a3a1c745379547c7e7c904aa64f6d5 is the good commit.
2.It's OK when in "scrot" as a photo.

Reproduce steps:
-------------------------
1. gnome-session
2. do rotation(e.g.xrandr --output LVDS1 --rotation left)
Comment 1 meng 2011-02-23 02:06:01 UTC
Created attachment 43698 [details]
The photo of screen mess  by a camera
Comment 2 Chris Wilson 2011-02-24 08:14:44 UTC
I suspect an interrupted modesetting. Were there no warnings in dmesg? How reproducibile is this? (And is this fixed by ce453d81cb0397aa7d5148984f51907e14072d74?)
Comment 3 meng 2011-02-25 00:53:55 UTC
(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.
Comment 4 meng 2011-02-25 00:55:46 UTC
Created attachment 43784 [details]
The dmesg with the commit ce453
Comment 5 Chris Wilson 2011-03-14 03:43:24 UTC
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?
Comment 6 meng 2011-03-14 18:50:47 UTC
Created attachment 44456 [details]
Xorg.0.log when testing in the commit 47ae63e
Comment 7 meng 2011-03-14 18:52:25 UTC
Created attachment 44457 [details]
The xrandr --verbose.log when testing in the commit 47ae63e
Comment 8 meng 2011-03-14 18:53:26 UTC
Created attachment 44458 [details]
The intel_reg_dumper.log
Comment 9 meng 2011-03-16 02:38:27 UTC
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>
Comment 10 Chris Wilson 2011-03-16 02:49:07 UTC
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.
Comment 11 Chris Wilson 2011-03-16 12:58:12 UTC
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.
Comment 12 Chris Wilson 2011-03-16 13:21:55 UTC
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.
Comment 13 Jesse Barnes 2011-03-16 14:24:39 UTC
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...
Comment 14 meng 2011-03-16 20:33:43 UTC
(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.
Comment 15 Chris Wilson 2011-03-17 00:37:58 UTC
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!
Comment 16 meng 2011-03-17 02:13:09 UTC
(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.
Comment 17 Jesse Barnes 2011-03-18 10:41:08 UTC
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?
Comment 18 Chris Wilson 2011-03-18 10:54:24 UTC
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.
Comment 19 Chris Wilson 2011-03-21 23:49:22 UTC
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.
Comment 20 meng 2011-03-22 02:20:16 UTC
(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".
Comment 21 Chris Wilson 2011-03-22 23:55:03 UTC
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
Comment 22 meng 2011-03-23 02:35:39 UTC
Verified with the commit 00d70b15125030391d17baab2c2f70f93b3339a6, it works fine.
Comment 23 Jari Tahvanainen 2017-09-04 10:04:48 UTC
Closing old verified+fixed.

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.