Bug 96645

Summary: [regression 4.7] [BISECT]Low package c-states used only after forcing DPMS to off
Product: DRI Reporter: Gabriele Mazzotta <gabriele.mzt>
Component: DRM/IntelAssignee: Matt Roper <matthew.d.roper>
Status: CLOSED FIXED QA Contact: Intel GFX Bugs mailing list <intel-gfx-bugs>
Severity: blocker    
Priority: highest CC: clayton, dcpurton, dp-fd, edouard.thuleau, gabriele.mzt, hugo, intel-gfx-bugs
Version: unspecifiedKeywords: bisected, regression
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
Whiteboard:
i915 platform: HSW i915 features: display/watermark
Attachments:
Description Flags
dmesg with drm.debug=14
none
[PATCH] drm/i915: Do .init_clock_gating() earlier to avoid it clobbering watermarks none

Description Gabriele Mazzotta 2016-06-22 21:17:50 UTC
I noticed that with v4.7-rc4 the CPU of my laptop can only enter package c-state 2, while it can normally enter pc7 with v4.6 or older kernels.

I also noticed that after I turned the screen off once running 'xset dpms force off', the CPU can enter pc7, no matter what's the current state of the display.

I ran a bisection and found that the regression was introduced with commit ed4a6a7ca853253f9b86f3005d76345482a71283 ("drm/i915: Add two-stage ILK-style watermark programming (v11)").

My laptop has an i7-4500U CPU and its integrated HD Graphics 4400.

I'm not sure what kind of information are needed to debug the problem, so let me know what other information I should provide.

Thanks,
Gabriele
Comment 1 yann 2016-06-30 08:31:07 UTC
Gabriele, please add drm.debug=14 on your boot command line and then attached your kernel log.
Thanks for the bisect.
Comment 2 Gabriele Mazzotta 2016-06-30 09:36:57 UTC
Created attachment 124794 [details]
dmesg with drm.debug=14

Here the dmesg as requested.

I toggled the DPMS status at 63s from boot.
Comment 3 yann 2016-06-30 10:33:56 UTC
thanks Gabriele
beside forcing dpms to reach pc7, can you also confirm the steps you were using previously when you are noticing that you cannot enter further than c2 ?
Comment 4 Gabriele Mazzotta 2016-06-30 11:14:00 UTC
With v4.6 or older I just need to change the SATA link power management to go further than PC2.

With v4.7 this is not enough, I have to change the SATA link power management and toggle DPMS once.

I forgot to mention that I can also suspend/resume my laptop to "unlock" the lower package states, instead of toggling DPMS.
Comment 5 Gabriele Mazzotta 2016-08-26 13:59:35 UTC
There is something else that I forgot to mention that might be relevant.

The fist thing I see when I turn the screen on after 'xset dpms force off' is the first frame that was shown on the screen when X was loaded, in my case the display manager. This frame stays there until something changes on the screen (cursor excluded, for some reason).

This happens only the first time I run 'xset dpms force off' and it didn't happen before ed4a6a7ca853253f9b86f3005d76345482a71283.
Comment 6 Jari Tahvanainen 2016-12-19 11:01:16 UTC
Highest+Blocker as being regression w/o workaround
Comment 7 Matt Roper 2017-01-17 23:35:07 UTC
Gabriele, do you still see this behavior on recent kernels?  We've landed several bugfixes for ILK-style watermarks since the patch that you bisected the problem to, so we want to make sure we haven't already fixed this without noticing.
Comment 8 Gabriele Mazzotta 2017-01-17 23:39:51 UTC
(In reply to Matt Roper from comment #7)
> Gabriele, do you still see this behavior on recent kernels?  We've landed
> several bugfixes for ILK-style watermarks since the patch that you bisected
> the problem to, so we want to make sure we haven't already fixed this
> without noticing.

4.10-rc3 is still affected. This is the newest version I tried.
Comment 9 Matt Roper 2017-01-18 18:54:02 UTC
(In reply to Gabriele Mazzotta from comment #8)
> 4.10-rc3 is still affected. This is the newest version I tried.

Thanks for confirming.  I'll need to refresh my memory on this area of the code, but my suspicion is that we're probably doing something wrong when we inherit/sanitize the watermark values setup by the BIOS.  Your DPMS off/on workaround forces us to truly recalculate proper watermarks from scartch, and presumably enter the deeper sleep states.
Comment 10 Dorota Czaplejewicz 2017-01-30 17:43:54 UTC
I have tried to reproduce this on a HSW-4770R machine, but I'm not sure whether I got matching reasults.

Cputop shows available package states: C3, C6. System stays in C3 after SATA link management enabled, never enters C6. Toggling DPMS has no effect. The machine doesn't have a display connected.

Does it seem similar to the symptoms of this bug?
Comment 11 Gabriele Mazzotta 2017-01-30 19:05:52 UTC
(In reply to Dorota Czaplejewicz from comment #10)
> I have tried to reproduce this on a HSW-4770R machine, but I'm not sure
> whether I got matching reasults.
> 
> Cputop shows available package states: C3, C6. System stays in C3 after SATA
> link management enabled, never enters C6. Toggling DPMS has no effect. The
> machine doesn't have a display connected.

I don't know what cputop is, I use powertop to have an overview of the current state, so I don't know if you are referring to the core c-state or package c-states. I'm having issues with the package c-states.

> Does it seem similar to the symptoms of this bug?

What I reported is a regression. With v4.6 or older the CPU could enter pc7, now I have to toggle DPMS. If your CPU could enter some package c-states with v4.6 that can no longer enter with newer kernel, then yes, your system is affected by the same bug.
Comment 12 Dorota Czaplejewicz 2017-01-31 13:19:44 UTC
Sorry, I meant "powertop". I was referring to package power states, same as you reported. I will report back after checking if 4.6 allows for more power states.
Comment 13 Dorota Czaplejewicz 2017-01-31 16:33:27 UTC
I tried kernel 4.6 from drm-tip (2dcd0af568b0cf583645c8a317dd12e344b1c72a) and the package power state still only goes as low as C3. This is the same as newest drm-tip.
No regression on my hardware.
Comment 14 Ville Syrjala 2017-02-07 17:39:49 UTC
There does seem to be some bug in the wm code. My IVB apparently manages to come up with LP1 disabled and LP2+ enabled, which is pretty much nonsense. DPMS toggling fixes that misconfiguration.

Please run intel_watermark from intel-gpu-tools before and after the DPMS trick so that we can confirm whether it's a similar story with your HSW.
Comment 15 Gabriele Mazzotta 2017-02-07 17:49:35 UTC
Before toggling DPMS:

    WM_PIPE_A = 0x00140006
    WM_PIPE_B = 0x00000000
    WM_PIPE_C = 0x00000000
WM_LINETIME_A = 0x00250078
WM_LINETIME_B = 0x00000000
WM_LINETIME_C = 0x00000000
       WM_LP1 = 0x02301406
       WM_LP2 = 0x06618912
       WM_LP3 = 0x0882b81a
   WM_LP1_SPR = 0x00000000
   WM_LP2_SPR = 0x00000000
   WM_LP3_SPR = 0x00000000
      ARB_CTL = 0x16663056
     ARB_CTL2 = 0x80000e80
      WM_MISC = 0x00000000
WM_PIPE_A: primary=20, cursor=6, sprite=0
WM_PIPE_B: primary=0, cursor=0, sprite=0
WM_PIPE_C: primary=0, cursor=0, sprite=0
WM_LINETIME_A: line time=120, ips line time=37
WM_LINETIME_B: line time=0, ips line time=0
WM_LINETIME_C: line time=0, ips line time=0
WM_LP1: disabled, latency=2, fbc=3, primary=20, cursor=6, sprite=0
WM_LP2: disabled, latency=6, fbc=6, primary=393, cursor=18, sprite=0
WM_LP3: disabled, latency=8, fbc=8, primary=696, cursor=26, sprite=0
Primary A trickle feed = enabled
Sprite A trickle feed = enabled
Primary B trickle feed = enabled
Sprite B trickle feed = enabled
Primary C trickle feed = enabled
Sprite C trickle feed = enabled
DDB partitioning = 1/2
FBC watermark = enabled

--------------------------

After toggling DPMS:

    WM_PIPE_A = 0x00140006
    WM_PIPE_B = 0x00000000
    WM_PIPE_C = 0x00000000
WM_LINETIME_A = 0x00250078
WM_LINETIME_B = 0x00000000
WM_LINETIME_C = 0x00000000
       WM_LP1 = 0x82301406
       WM_LP2 = 0x86618912
       WM_LP3 = 0x8882b81a
   WM_LP1_SPR = 0x00000000
   WM_LP2_SPR = 0x00000000
   WM_LP3_SPR = 0x00000000
      ARB_CTL = 0x16663056
     ARB_CTL2 = 0x80000e80
      WM_MISC = 0x00000000
WM_PIPE_A: primary=20, cursor=6, sprite=0
WM_PIPE_B: primary=0, cursor=0, sprite=0
WM_PIPE_C: primary=0, cursor=0, sprite=0
WM_LINETIME_A: line time=120, ips line time=37
WM_LINETIME_B: line time=0, ips line time=0
WM_LINETIME_C: line time=0, ips line time=0
WM_LP1: enabled, latency=2, fbc=3, primary=20, cursor=6, sprite=0
WM_LP2: enabled, latency=6, fbc=6, primary=393, cursor=18, sprite=0
WM_LP3: enabled, latency=8, fbc=8, primary=696, cursor=26, sprite=0
Primary A trickle feed = enabled
Sprite A trickle feed = enabled
Primary B trickle feed = enabled
Sprite B trickle feed = enabled
Primary C trickle feed = enabled
Sprite C trickle feed = enabled
DDB partitioning = 1/2
FBC watermark = enabled
Comment 16 Ville Syrjala 2017-02-17 20:00:28 UTC
*** Bug 99340 has been marked as a duplicate of this bug. ***
Comment 17 Ville Syrjala 2017-02-17 20:20:35 UTC
Created attachment 129720 [details] [review]
[PATCH] drm/i915: Do .init_clock_gating() earlier to avoid it  clobbering watermarks

This should fix the problem I think. Or at least it did clear out the nonsense watermark state on my IVB.
Comment 18 Gabriele Mazzotta 2017-02-17 21:04:25 UTC
I applied the attached patch to 4.10-rc8 as follows and it fixed my problem: right after a reboot the CPU can enter pc7 and intel_watermak reports the same values it did with the unpatched kernel after toggling DPSM.

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 891c86aef99d..0b15b694baa0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -16497,12 +16497,11 @@ int intel_modeset_init(struct drm_device *dev)
 		}
 	}
 
-	intel_update_czclk(dev_priv);
-	intel_update_cdclk(dev_priv);
-	dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
-
 	intel_shared_dpll_init(dev);
 
+	intel_update_czclk(dev_priv);
+	intel_modeset_init_hw(dev);
+
 	if (dev_priv->max_cdclk_freq == 0)
 		intel_update_max_cdclk(dev_priv);
 
@@ -17057,8 +17056,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
 
 	intel_init_gt_powersave(dev_priv);
 
-	intel_modeset_init_hw(dev);
-
 	intel_setup_overlay(dev_priv);
 }
Comment 19 yann 2017-02-20 14:09:30 UTC
reference to Ville's patch: https://patchwork.freedesktop.org/series/19953/
Comment 20 Ville Syrjala 2017-03-02 20:24:31 UTC
commit 5be6e33400992d3450e1c8234a5af353e1560580
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Mon Feb 20 16:04:43 2017 +0200

    drm/i915: Do .init_clock_gating() earlier to avoid it clobbering watermarks

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.