Bug 26422

Summary: [945GM bisected] some OGLC cases performance regression due to memory self refresh
Product: DRI Reporter: Shuang He <shuang.he>
Component: DRM/IntelAssignee: Li Peng <peng.li>
Status: CLOSED FIXED QA Contact:
Severity: major    
Priority: high CC: eric, peng.li
Version: XOrg git   
Hardware: Other   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
test .patch none

Description Shuang He 2010-02-03 19:34:49 UTC
System Environment:
--------------------------
Libdrm 	(master)520c658706aa896d64f374cc74065394111f6122
Mesa 	(master)ad83f3bf0a633d5d7360c849010f01063afc1702
Xserver 	(master)52456c602c3cdd7d5eac677889a18fad37dfb7ae
Xf86_video_intel 	(master)1a76fa5574e8e8f88ac3518a4e4494e1af301dc1
Kernel 	(drm-intel-next)a9c8bea64706f86a9107ab41e7a3e284d1508f9c


Bug detailed description:
-------------------------
This issue happens on two of our netbooks (945GME). 
This regression kind of block our conformance testing on those netbooks, since
it take too much time to run.

Some OGLC cases now take much more time to run, such as: 
colramp.c, logicop.c, pntrast.c, api-coloru.c, pxtrans-cirgbtex.c


Take colramp.c as an example, it used to take less than 2 minutes, now take more than 3 minutes to run.

After some time of investigating, and bisection (we used case colramp.c for
bisection), we found it seems caused by following commit:
commit ac29d4e7dbe5eab2047cb16e02bb1c41aa129a91
Author: Li Peng <peng.li@linux.intel.com>
Date:   Wed Jan 27 19:01:11 2010 +0800

    drm/i915: enable memory self refresh on 9xx

    Enabling memory self refresh (SR) on 9xx needs to set additional
    register bits. On 945, we need bit 31 of FW_BLC_SELF to enable the
    write to self refresh bit and bit 16 to enable the write of self
    refresh watermark. On 915, bit 12 of INSTPM is used to enable SR.

    SR will take effect when CPU enters C3+ state and its entry/exit
    should be automatically controlled by H/W, driver only needs to set
    SR enable bits in wm update. But this isn't safe in my test on 945
    because GPU is hung. So this patch explicitly enables SR when GPU
    is idle, and disables SR when it is busy. In my test on a netbook of
    945GSE chipset, it saves about 0.8W idle power.

    Signed-off-by: Li Peng <peng.li@intel.com>
    Signed-off-by: Eric Anholt <eric@anholt.net>
Comment 1 Li Peng 2010-02-03 19:41:40 UTC
Memory self refresh on 945 will only take effect with GPU or crtc is idle. 

Is there some idle happening in the test case runtime ?
Comment 2 Shuang He 2010-02-03 21:40:34 UTC
(In reply to comment #1)
> Memory self refresh on 945 will only take effect with GPU or crtc is idle. 
> 
> Is there some idle happening in the test case runtime ?
> 

How to check if there's idle happening in the test case runtime?
Comment 3 Li Peng 2010-02-03 21:44:44 UTC
drm.debug=2

if GPU or crtc becoming idle, you would see 

"enable memory self refresh on 945"
Comment 4 Shuang He 2010-02-03 23:32:57 UTC
(In reply to comment #3)
> drm.debug=2
> 
> if GPU or crtc becoming idle, you would see 
> 
> "enable memory self refresh on 945"
> 

There's lots of message like this when running the test case:
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:i915_add_request], 1071233
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:i915_add_request], 1071234
[drm:i915_add_request], 1071235
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:i915_add_request], 1071236
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:i915_add_request], 1071237
[drm:i915_add_request], 1071238
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:i915_add_request], 1071239
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:i915_add_request], 1071240
[drm:i915_add_request], 1071241
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:i915_add_request], 1071242
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
[drm:intel_mark_busy], disable memory self refresh on 945
Comment 5 Li Peng 2010-02-04 00:12:23 UTC
ok, then the test case doesn't keep making GPU or crtc busy, and memory self refresh is enabled when they are idle. So the transition from busy to idle, or idle to busy makes memory self refresh on and off. causing it takes longer time to run the test case.
Comment 6 Shuang He 2010-02-04 00:45:21 UTC
(In reply to comment #5)
> ok, then the test case doesn't keep making GPU or crtc busy, and memory self
> refresh is enabled when they are idle. So the transition from busy to idle, or
> idle to busy makes memory self refresh on and off. causing it takes longer time
> to run the test case.
> 

Is it going into idle state too soon?
Comment 7 Li Peng 2010-02-04 00:46:59 UTC
I will send you a patch tomorrow, see if it can help.
Comment 8 Li Peng 2010-02-04 01:09:08 UTC
Created attachment 33054 [details] [review]
test .patch

Please try this patch, it will only disable memory self refresh when GPU/crtc's busy flag changing from "false" to "true"
Comment 9 Shuang He 2010-02-04 17:48:43 UTC
(In reply to comment #8)
> Created an attachment (id=33054) [details]
> test .patch
> 
> Please try this patch, it will only disable memory self refresh when GPU/crtc's
> busy flag changing from "false" to "true"
> 

This patch works for me
Comment 10 Li Peng 2010-02-04 17:49:38 UTC
Thanks, Shuang. I will submit to upstream.
Comment 11 Jesse Barnes 2010-02-05 14:31:09 UTC
Thanks Peng.
Comment 12 Li Peng 2010-02-10 20:38:23 UTC
Patch is applied in drm-intel-next tree. Mark it as fixed.
Comment 13 Shuang He 2010-02-21 00:04:33 UTC
(In reply to comment #12)
> Patch is applied in drm-intel-next tree. Mark it as fixed.
> 

I'd have to reopen the bug, since, the patch commit into drm-intel-next can't be built due to typo in following line:

drivers/gpu/drm/i915/intel_display.c:2778: error: 'IS_I915GM' undeclared (first use in this function)
Comment 14 Li Peng 2010-02-21 00:07:57 UTC
Eric rebased this patch based on commit 33c5fd121eabbccc9103daf6cda36941eb3c349f

but there is a typo introduced, a fix is post in the mailing list

http://lists.freedesktop.org/archives/intel-gfx/2010-February/005904.html

Eric, would you please fix it ? Thank you.
Comment 15 Li Peng 2010-02-24 18:30:51 UTC
build error is fixed. close this bug
Comment 16 Shuang He 2010-02-28 18:02:26 UTC
verified on 945GM against:
Kernel 	(drm-intel-next)6070a4a928f8c92b9fae7d6717ebbb05f425d6b2
Libdrm 	(master)9a37455b35d746d694760cfe8850a8bf856d73c9
Mesa 	(master)54f9c509a1eddfa7b2600ef4e4c18c2e212f6d51
Xserver 	(master)de86a3a3448f0a55c1cd99aee9ea80070a589877
Xf86_video_intel 	(master)a0ee9c3d9c72962c8d513ec8c43dd4a21e316947
Comment 17 Elizabeth 2017-10-06 14:54:21 UTC
Closing old verified.

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.