drm/i915: fix long-standing SNB regression in power consumption after resume
Subsystems (2)
PATCH 1/1 drm/i915: fix long-standing SNB regression in power consumption after resume
This patch fixes regression in power consumtion of sandy bridge gpu, which exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that it's extremely busy. After that it never reaches rc6 state.
Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0 ("drm/i915: load boot context at driver init time"). Without documentation it's not clear what is happening here, probably this breaks internal state of hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake during whole initialization sequence in gen6_init_clock_gating() fixes this bug.
References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
--- drivers/gpu/drm/i915/intel_pm.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/gpu/drm/i915/intel_pm.c +4 -0
@@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)int pipe;uint32_t dspclk_gate =ILK_VRHUNIT_CLOCK_GATE_DISABLE ; +gen6_gt_force_wake_get (dev_priv); +I915_WRITE (ILK_DSPCLK_GATE_D , dspclk_gate); I915_WRITE(ILK_DISPLAY_CHICKEN2,
@@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)cpt_init_clock_gating (dev);gen6_check_mch_setup (dev); + +gen6_gt_force_wake_put (dev_priv); }static void gen7_setup_fixed_func_scheduler (struct drm_i915_private *dev_priv)
Quoted 4 lines · Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu, which exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that it's extremely busy. After that it never reaches rc6 state.
Quoted 8 lines
Bug was introduce by commit
b4ae3f22d238617ca11610b29fde16cf8c0bc6e0("drm/i915: load boot context at driver init time"). Without documentation it's not clear what is happening here, probably this breaks internal state of hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake during whole initialization sequence ingen6_init_clock_gating()fixes this bug.References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by Konstantin Khlebnikov <[email protected]>
We already hold an forcewake reference while setting up the rps stuff, should we maybe hold the forcewake for the entire duration, i.e. grab it here in clock_gating and release it only in gen6/vlv_enable_rps? Can you please test that version, too?
In any case the forcewake grabbing here in the clock gating function needs a big comment that otherwise setting the MCTL register might break rc6 entry.
-Daniel
Quoted 19 lines
--- drivers/gpu/drm/i915/intel_pm.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/gpu/drm/i915/intel_pm.c +2
-0@@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)int pipe;uint32_t dspclk_gate =ILK_VRHUNIT_CLOCK_GATE_DISABLE ; +gen6_gt_force_wake_get (dev_priv); +I915_WRITE (ILK_DSPCLK_GATE_D , dspclk_gate); I915_WRITE(ILK_DISPLAY_CHICKEN2,
drivers/gpu/drm/i915/intel_pm.c static void gen6_init_clock_gating(struct drm_device *dev) · line 3737
Quoted 8 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0@@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)cpt_init_clock_gating (dev);gen6_check_mch_setup (dev); + +gen6_gt_force_wake_put (dev_priv); }static void gen7_setup_fixed_func_scheduler (struct drm_i915_private *dev_priv)
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 12 lines · Daniel Vetter
Quoted 10 lines · Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu, which exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that it's extremely busy. After that it never reaches rc6 state.
Bug was introduce by commit
b4ae3f22d238617ca11610b29fde16cf8c0bc6e0("drm/i915: load boot context at driver init time"). Without documentation it's not clear what is happening here, probably this breaks internal state of hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake during whole initialization sequence ingen6_init_clock_gating()fixes this bug.
Quoted 7 lines
Quoted 2 lines
References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by Konstantin Khlebnikov<[email protected]>We already hold an forcewake reference while setting up the rps stuff, should we maybe hold the forcewake for the entire duration, i.e. grab it here in
clock_gatingand release it only in gen6/vlv_enable_rps? Can you please test that version, too?
This will be racy because rps stuff is done in separate work which might be canceled if intel_disable_gt_powersave() happens before its completion.
Quoted 40 lines
In any case the forcewake grabbing here in the clock gating function needs a big comment that otherwise setting the MCTL register might break rc6 entry.
-DanielQuoted 19 lines
--- drivers/gpu/drm/i915/intel_pm.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/gpu/drm/i915/intel_pm.c +2
-0@@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)int pipe;uint32_t dspclk_gate =ILK_VRHUNIT_CLOCK_GATE_DISABLE ; +gen6_gt_force_wake_get (dev_priv); +I915_WRITE (ILK_DSPCLK_GATE_D , dspclk_gate); I915_WRITE(ILK_DISPLAY_CHICKEN2,drivers/gpu/drm/i915/intel_pm.c static void gen6_init_clock_gating(struct drm_device *dev) · line 3737
Quoted 8 lines
drivers/gpu/drm/i915/intel_pm.c +2
-0@@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)cpt_init_clock_gating (dev);gen6_check_mch_setup (dev); + +gen6_gt_force_wake_put (dev_priv); }static void gen7_setup_fixed_func_scheduler (struct drm_i915_private *dev_priv)--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 16 lines · Konstantin Khlebnikov
Quoted 15 lines · Daniel Vetter
Quoted 12 lines · Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu, which exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that it's extremely busy. After that it never reaches rc6 state.
Bug was introduce by commit
b4ae3f22d238617ca11610b29fde16cf8c0bc6e0("drm/i915: load boot context at driver init time"). Without documentation it's not clear what is happening here, probably this breaks internal state of hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake during whole initialization sequence ingen6_init_clock_gating()fixes this bug.References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by Konstantin Khlebnikov<[email protected]>
Quoted 7 lines
Quoted 4 lines
We already hold an forcewake reference while setting up the rps stuff, should we maybe hold the forcewake for the entire duration, i.e. grab it here in
clock_gatingand release it only in gen6/vlv_enable_rps? Can you please test that version, too?This will be racy because rps stuff is done in separate work which might be canceled if
intel_disable_gt_powersave()happens before its completion.
Can be fixed with a flush_delayed_work. And since that has the same requirements wrt locking to prevent deadlocks as cancel_work_sync it would be a drop-in replacement. Can I volunteer you to look into testing that out a bit? Otherwise I could volunteer someone from our team.
In any case I think we should apply this trick to all platforms where we've added the MBCTL write (i.e. snb, ivb, hsw & vlv) since rps/rc6 works _very_ similar on all of those.
Thanks, Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 32 lines · Daniel Vetter
Quoted 4 lines · Konstantin Khlebnikov
Quoted 3 lines · Daniel Vetter
Quoted 1 line · Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu,
which
Quoted 1 line
Quoted 1 line
Quoted 1 line
exists since v3.6 Sometimes after resuming from s2ram gpu starts
thinking that
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
it's extremely busy. After that it never reaches rc6 state.
Bug was introduce by commit
b4ae3f22d238617ca11610b29fde16cf8c0bc6e0("drm/i915: load boot context at driver init time"). Withoutdocumentation
Quoted 1 line
Quoted 1 line
Quoted 1 line
it's not clear what is happening here, probably this breaks internal
state of
Quoted 1 line
Quoted 1 line
Quoted 1 line
hardware ring buffers and confuses RPS engine. Fortunately keeping
forcewake
Quoted 1 line
Quoted 1 line
Quoted 1 line
during whole initialization sequence in
gen6_init_clock_gating()fixesthis bug.
Quoted 10 lines
Quoted 8 lines
Quoted 3 lines
References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by Konstantin Khlebnikov<[email protected]>We already hold an forcewake reference while setting up the rps stuff, should we maybe hold the forcewake for the entire duration, i.e. grab it here in
clock_gatingand release it only in gen6/vlv_enable_rps? Can you please test that version, too?This will be racy because rps stuff is done in separate work which might
be canceled
Quoted 1 line
if
intel_disable_gt_powersave()happens before its completion.
Quoted 9 lines
Can be fixed with a
flush_delayed_work. And since that has the same requirements wrt locking to prevent deadlocks ascancel_work_syncit would be a drop-in replacement. Can I volunteer you to look into testing that out a bit? Otherwise I could volunteer someone from our team.In any case I think we should apply this trick to all platforms where we've added the MBCTL write (i.e. snb, ivb, hsw & vlv) since rps/rc6 works _very_ similar on all of those.
I've tested that patch and it really works for me. If you want change something for other hardware or
extend range where forcewake is held prease do it in a separate patch. This will be good for bisecting new bugs in the future.
Quoted 7 lines
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 44 lines · Konstantin Khlebnikov
Quoted 32 lines · Daniel Vetter
Quoted 4 lines · Konstantin Khlebnikov
Quoted 3 lines · Daniel Vetter
Quoted 1 line · Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu,
which
Quoted 1 line
Quoted 1 line
Quoted 1 line
exists since v3.6 Sometimes after resuming from s2ram gpu starts
thinking that
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
it's extremely busy. After that it never reaches rc6 state.
Bug was introduce by commit
b4ae3f22d238617ca11610b29fde16cf8c0bc6e0("drm/i915: load boot context at driver init time"). Withoutdocumentation
Quoted 1 line
Quoted 1 line
Quoted 1 line
it's not clear what is happening here, probably this breaks internal
state of
Quoted 1 line
Quoted 1 line
Quoted 1 line
hardware ring buffers and confuses RPS engine. Fortunately keeping
forcewake
Quoted 1 line
Quoted 1 line
Quoted 1 line
during whole initialization sequence in
gen6_init_clock_gating()fixesthis bug.
Quoted 10 lines
Quoted 8 lines
Quoted 3 lines
References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by Konstantin Khlebnikov<[email protected]>We already hold an forcewake reference while setting up the rps stuff, should we maybe hold the forcewake for the entire duration, i.e. grab it here in
clock_gatingand release it only in gen6/vlv_enable_rps? Can you please test that version, too?This will be racy because rps stuff is done in separate work which might
be canceled
Quoted 1 line
if
intel_disable_gt_powersave()happens before its completion.Quoted 9 lines
Can be fixed with a
flush_delayed_work. And since that has the same requirements wrt locking to prevent deadlocks ascancel_work_syncit would be a drop-in replacement. Can I volunteer you to look into testing that out a bit? Otherwise I could volunteer someone from our team.In any case I think we should apply this trick to all platforms where we've added the MBCTL write (i.e. snb, ivb, hsw & vlv) since rps/rc6 works _very_ similar on all of those.
Quoted 4 lines
I've tested that patch and it really works for me. If you want change something for other hardware or
extend range where forcewake is held prease do it in a separate patch. This will be good for bisecting new bugs in the future.
The issue I have with the current patch is that it looks a bit like duct-tape since the point where we drop the forcewake references seems to lack justification. The write to MBCTL itself will temporarily wake up the chip, so just wrapping that up in with forcewake is very likely not good enough. So I fear that we'll only hold forcewake long enough on most systems and still have a bunch of oddball broken systems out there.
Holding forcewake otoh until we've fully set up rps/rc6 makes imo tons of sense, hence why I've brought up the idea. Same reasoning applies to extending the w/a to all systems supporting rc6.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 10 lines · Daniel Vetter
The issue I have with the current patch is that it looks a bit like duct-tape since the point where we drop the forcewake references seems to lack justification. The write to MBCTL itself will temporarily wake up the chip, so just wrapping that up in with forcewake is very likely not good enough. So I fear that we'll only hold forcewake long enough on most systems and still have a bunch of oddball broken systems out there.
Holding forcewake otoh until we've fully set up rps/rc6 makes imo tons of sense, hence why I've brought up the idea. Same reasoning applies to extending the w/a to all systems supporting rc6.
In which case disable rc6 at the start of init gating and only enable it at the end of the deferred task. That I think will better test your hypothesis and make the transistion steps clearer.
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Quoted 8 lines · Chris Wilson
Quoted 7 lines · Daniel Vetter
The issue I have with the current patch is that it looks a bit like duct-tape since the point where we drop the forcewake references seems to lack justification. The write to MBCTL itself will temporarily wake up the chip, so just wrapping that up in with forcewake is very likely not good enough. So I fear that we'll only hold forcewake long enough on most systems and still have a bunch of oddball broken systems out there.
Quoted 7 lines
Quoted 3 lines
Holding forcewake otoh until we've fully set up rps/rc6 makes imo tons of sense, hence why I've brought up the idea. Same reasoning applies to extending the w/a to all systems supporting rc6.
In which case disable rc6 at the start of init gating and only enable it at the end of the deferred task. That I think will better test your hypothesis and make the transistion steps clearer.
Hm yeah, that would be much clearer instead of risky tricks with a refcount which is only dropped someplace completely else. -Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 4 lines · On Tue, 16 Jul 2013 11:34:25 +0400 Konstantin Khlebnikov…
I've tested that patch and it really works for me. If you want change something for other hardware or
extend range where forcewake is held prease do it in a separate patch. This will be good for bisecting new bugs in the future.
Thanks a ton for finding this Konstantin, it puts us on the right track.
Can I ask you to test this patch? The theory is that having RC6 enabled messes with the initial programming sequence, so it's probably best to just shut it off at init until we're done, rather than trying to forcewake around everywhere we need it.
Thanks,
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
drivers/gpu/drm/i915/intel_display.c +3 -0
@@ -9648,6 +9648,9 @@ static void i915_disable_vga(struct drm_device *dev)void intel_modeset_init_hw (struct drm_device *dev) { +/* BIOS often leaves RC6 enabled, but disable it for hw init */ +intel_disable_gt_powersave (dev); +intel_init_power_well (dev);intel_prepare_ddi (dev);
Quoted 7 lines · On Tue, 16 Jul 2013 10:06:54 -0700 Jesse Barnes <[email protected]>…
Quoted 4 lines · On Tue, 16 Jul 2013 11:34:25 +0400 Konstantin Khlebnikov…
I've tested that patch and it really works for me. If you want change something for other hardware or
extend range where forcewake is held prease do it in a separate patch. This will be good for bisecting new bugs in the future.
Quoted 7 lines
Thanks a ton for finding this Konstantin, it puts us on the right track.
Can I ask you to test this patch? The theory is that having RC6 enabled messes with the initial programming sequence, so it's probably best to just shut it off at init until we're done, rather than trying to forcewake around everywhere we need it.
Oops, last one triggers a warn about IRQs. This one doesn't and still works for me.
Testing welcome.
Thanks,
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
drivers/gpu/drm/i915/i915_gem.c +3 -0
@@ -4112,6 +4112,9 @@ i915_gem_init_hw(struct drm_device *dev)drm_i915_private_t *dev_priv = dev->dev_private;int ret; +/* BIOS often leaves RC6 enabled, but disable it for hw init */ +intel_disable_gt_powersave (dev); +if (INTEL_INFO (dev)->gen <6 && !intel_enable_gtt ())return -EIO ;
Quoted 32 lines · Jesse Barnes
Quoted 7 lines · On Tue, 16 Jul 2013 10:06:54 -0700 Jesse Barnes <[email protected]>…
Quoted 4 lines · On Tue, 16 Jul 2013 11:34:25 +0400 Konstantin Khlebnikov…
I've tested that patch and it really works for me. If you want change something for other hardware or
extend range where forcewake is held prease do it in a separate patch. This will be good for bisecting new bugs in the future.Quoted 7 lines
Thanks a ton for finding this Konstantin, it puts us on the right track.
Can I ask you to test this patch? The theory is that having RC6 enabled messes with the initial programming sequence, so it's probably best to just shut it off at init until we're done, rather than trying to forcewake around everywhere we need it.
Oops, last one triggers a warn about IRQs. This one doesn't and still works for me.
Testing welcome.
Thanks,
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Centerdrivers/gpu/drm/i915/i915_gem.c +0
-0
drivers/gpu/drm/i915/i915_gem.c i915_gem_init_hw(struct drm_device *dev) · line 4116
Quoted 5 lines
drivers/gpu/drm/i915/i915_gem.c +2
-0@@ -4112,6 +4112,9 @@ i915_gem_init_hw(struct drm_device *dev)drm_i915_private_t *dev_priv = dev->dev_private;int ret; +/* BIOS often leaves RC6 enabled, but disable it for hw init */ +intel_disable_gt_powersave (dev);
I think it'd be better to have an explicit gen >= 6 check here and a disable_rps call. disable_gt_powersave also calls the ironlake version, which restores probably bogus values (since we haven't read them out yet in the enable code) into the hw.
-Daniel
Quoted 8 lines
drivers/gpu/drm/i915/i915_gem.c +1
-0+if (INTEL_INFO (dev)->gen <6 && !intel_enable_gtt ())return -EIO ;_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 36 lines · On Tue, 16 Jul 2013 22:43:49 +0200 Daniel Vetter <[email protected]> wrote:
Quoted 35 lines · Jesse Barnes
Quoted 7 lines · On Tue, 16 Jul 2013 10:06:54 -0700 Jesse Barnes <[email protected]>…
Quoted 4 lines · On Tue, 16 Jul 2013 11:34:25 +0400 Konstantin Khlebnikov…
I've tested that patch and it really works for me. If you want change something for other hardware or
extend range where forcewake is held prease do it in a separate patch. This will be good for bisecting new bugs in the future.Quoted 7 lines
Thanks a ton for finding this Konstantin, it puts us on the right track.
Can I ask you to test this patch? The theory is that having RC6 enabled messes with the initial programming sequence, so it's probably best to just shut it off at init until we're done, rather than trying to forcewake around everywhere we need it.
Oops, last one triggers a warn about IRQs. This one doesn't and still works for me.
Testing welcome.
Thanks,
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Centerdrivers/gpu/drm/i915/i915_gem.c +0
-0@@ -4112,6 +4112,9 @@ i915_gem_init_hw(struct drm_device *dev)drm_i915_private_t *dev_priv = dev->dev_private;int ret;
Quoted 7 lines
drivers/gpu/drm/i915/i915_gem.c i915_gem_init_hw(struct drm_device *dev) · line 4116
Quoted 2 lines
drivers/gpu/drm/i915/i915_gem.c +2
-0+/* BIOS often leaves RC6 enabled, but disable it for hw init */ +intel_disable_gt_powersave (dev);I think it'd be better to have an explicit gen >= 6 check here and a
disable_rpscall.disable_gt_powersavealso calls the ironlake version, which restores probably bogus values (since we haven't read them out yet in the enable code) into the hw.
Yeah the ilk "restore to initial freq" needs fixing, but I thought we wanted this on all gens? It shouldn't hurt anything, and may help with other issues as well.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Quoted 15 lines · Daniel Vetter
Quoted 4 lines · Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu, which exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that it's extremely busy. After that it never reaches rc6 state.
Quoted 8 lines
Bug was introduce by commit
b4ae3f22d238617ca11610b29fde16cf8c0bc6e0("drm/i915: load boot context at driver init time"). Without documentation it's not clear what is happening here, probably this breaks internal state of hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake during whole initialization sequence ingen6_init_clock_gating()fixes this bug.References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by Konstantin Khlebnikov <[email protected]>
Quoted 8 lines
We already hold an forcewake reference while setting up the rps stuff, should we maybe hold the forcewake for the entire duration, i.e. grab it here in
clock_gatingand release it only in gen6/vlv_enable_rps? Can you please test that version, too?In any case the forcewake grabbing here in the clock gating function needs a big comment that otherwise setting the MCTL register might break rc6 entry.
It is not clear why the forcewake works, but is easy to imagine one of the operations in that sequence requires the GPU to be awake at the time of programming for it to succeed. MBCTL:EnableBootFetch does seem the most suspicious from its wording in the bspec. I guess all instances of poking this bit should be protected similary (snb, ivb, vlv, hsw).
Based on that reasoning and that waking the GPU up here has no negative consequences, and so long as all paths are fixed, I am happy to give this an Acked-by: Chris Wilson <[email protected]>
Also, we need to reapply the w/a after a Function Level Reset, in other words we do need to repeat the init_clock_gating after intel_gpu_reset().
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Quoted 4 lines · Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu, which exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that it's extremely busy. After that it never reaches rc6 state.
Quoted 8 lines
Bug was introduce by commit
b4ae3f22d238617ca11610b29fde16cf8c0bc6e0("drm/i915: load boot context at driver init time"). Without documentation it's not clear what is happening here, probably this breaks internal state of hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake during whole initialization sequence ingen6_init_clock_gating()fixes this bug.References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by Konstantin Khlebnikov <[email protected]>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=58971
Quoted 27 lines
--- drivers/gpu/drm/i915/intel_pm.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/gpu/drm/i915/intel_pm.c +4
-0@@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)int pipe;uint32_t dspclk_gate =ILK_VRHUNIT_CLOCK_GATE_DISABLE ; +gen6_gt_force_wake_get (dev_priv); +I915_WRITE (ILK_DSPCLK_GATE_D , dspclk_gate); I915_WRITE(ILK_DISPLAY_CHICKEN2,@@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)cpt_init_clock_gating (dev);gen6_check_mch_setup (dev); + +gen6_gt_force_wake_put (dev_priv); }static void gen7_setup_fixed_func_scheduler (struct drm_i915_private *dev_priv)
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Daniel Vetter
Tested-by JohnMBQuoted 11 lines · Daniel Vetter
Quoted 10 lines · Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu, which exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that it's extremely busy. After that it never reaches rc6 state.
Bug was introduce by commit
b4ae3f22d238617ca11610b29fde16cf8c0bc6e0("drm/i915: load boot context at driver init time"). Without documentation it's not clear what is happening here, probably this breaks internal state of hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake during whole initialization sequence ingen6_init_clock_gating()fixes this bug.
Quoted 6 lines
Quoted 2 lines
References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by Konstantin Khlebnikov <[email protected]>Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=58971
Tested-by Alexander Kaltsas <[email protected]>Tested-by rocko <[email protected]>
Quoted 33 lines
Quoted 28 lines
--- drivers/gpu/drm/i915/intel_pm.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/gpu/drm/i915/intel_pm.c +4
-0@@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)int pipe;uint32_t dspclk_gate =ILK_VRHUNIT_CLOCK_GATE_DISABLE ; +gen6_gt_force_wake_get (dev_priv); +I915_WRITE (ILK_DSPCLK_GATE_D , dspclk_gate); I915_WRITE(ILK_DISPLAY_CHICKEN2,@@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)cpt_init_clock_gating (dev);gen6_check_mch_setup (dev); + +gen6_gt_force_wake_put (dev_priv); }static void gen7_setup_fixed_func_scheduler (struct drm_i915_private *dev_priv)Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH v2 1/1 drm/i915: fix long-standing SNB regression in power consumption after resume
In reply to Konstantin KhlebnikovThis patch fixes regression in power consumtion of sandy bridge gpu, which exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that it's extremely busy. After that it never reaches rc6 state.
Bug exists since kernel v3.6, commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0 ("drm/i915: load boot context at driver init time").
For some reason RC6 is already enabled at the beginning of resuming process. Following initliaztion breaks some internal state and confuses RPS engine. This patch disables RC6 at the beginnig of resume and initialization.
I've rearranged initialization sequence, because intel_disable_gt_powersave() needs initialized force_wake_get/put and some locks from the dev_priv.
References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
References: https://bugzilla.kernel.org/show_bug.cgi?id=58971
--- drivers/gpu/drm/i915/i915_dma.c | 16 ++++++++-------- drivers/gpu/drm/i915/intel_pm.c | 5 +++-- 2 files changed, 11 insertions(+), 10 deletions(-)
drivers/gpu/drm/i915/i915_dma.c +8 -8
@@ -1511,6 +1511,13 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) dev_priv->dev = dev; dev_priv->info = info; +spin_lock_init (&dev_priv->irq_lock); +spin_lock_init (&dev_priv->gpu_error.lock); +spin_lock_init (&dev_priv->rps.lock); +mutex_init (&dev_priv->dpio_lock); +mutex_init (&dev_priv->rps.hw_lock); +mutex_init (&dev_priv->modeset_restore_lock); +i915_dump_device_info (dev_priv);if (i915_get_bridge_dev (dev)) {
@@ -1602,6 +1609,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)intel_irq_init (dev);intel_gt_init (dev); +intel_gt_reset (dev);/* Try to make sure MCHBAR is enabled before poking at it */ intel_setup_mchbar (dev);
@@ -1626,14 +1634,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)if (!IS_I945G (dev) && !IS_I945GM (dev))pci_enable_msi (dev->pdev); -spin_lock_init (&dev_priv->irq_lock); -spin_lock_init (&dev_priv->gpu_error.lock); -spin_lock_init (&dev_priv->rps.lock); -mutex_init (&dev_priv->dpio_lock); - -mutex_init (&dev_priv->rps.hw_lock); -mutex_init (&dev_priv->modeset_restore_lock); - dev_priv->num_plane =1 ;if (IS_VALLEYVIEW (dev)) dev_priv->num_plane =2 ;
drivers/gpu/drm/i915/intel_pm.c +3 -2
@@ -4497,6 +4497,9 @@ void intel_gt_reset(struct drm_device *dev)if (IS_IVYBRIDGE (dev) ||IS_HASWELL (dev))__gen6_gt_force_wake_mt_reset (dev_priv); } + +/* BIOS often leaves RC6 enabled, but disable it for hw init */ +intel_disable_gt_powersave (dev); }void intel_gt_init (struct drm_device *dev)
@@ -4505,8 +4508,6 @@ void intel_gt_init(struct drm_device *dev)spin_lock_init (&dev_priv->gt_lock); -intel_gt_reset (dev); -if (IS_VALLEYVIEW (dev)) { dev_priv->gt.force_wake_get = vlv_force_wake_get; dev_priv->gt.force_wake_put = vlv_force_wake_put;
Quoted 14 lines · Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu, which exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that it's extremely busy. After that it never reaches rc6 state.
Bug exists since kernel v3.6, commit
b4ae3f22d238617ca11610b29fde16cf8c0bc6e0("drm/i915: load boot context at driver init time").For some reason RC6 is already enabled at the beginning of resuming process. Following initliaztion breaks some internal state and confuses RPS engine. This patch disables RC6 at the beginnig of resume and initialization.
I've rearranged initialization sequence, because
intel_disable_gt_powersave()needs initialized force_wake_get/put and some locks from thedev_priv.
Quoted 6 lines
References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
References: https://bugzilla.kernel.org/show_bug.cgi?id=58971Signed-off-by Konstantin Khlebnikov <[email protected]>Cc Daniel Vetter <[email protected]>Cc Chris Wilson <[email protected]>Cc Jesse Barnes <[email protected]>
lgtm, thanks a lot for digging through this giant mess and figuring out what's actually broken here. Patch is merged to my -fixes queue with a slightly pimped commit message and cc: stable added.
Note that there's a small issue with the drps code on ilk, we now write a bogus fstart value as the requested frequency. I'll follow-up with a patch to fix this, but I've figured that merging this one here first for wider testing is more important.
Cheers, Daniel
Quoted 70 lines
--- drivers/gpu/drm/i915/i915_dma.c | 16 ++++++++-------- drivers/gpu/drm/i915/intel_pm.c | 5 +++-- 2 files changed, 11 insertions(+), 10 deletions(-)drivers/gpu/drm/i915/i915_dma.c +8
-8@@ -1511,6 +1511,13 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) dev_priv->dev = dev; dev_priv->info = info; +spin_lock_init (&dev_priv->irq_lock); +spin_lock_init (&dev_priv->gpu_error.lock); +spin_lock_init (&dev_priv->rps.lock); +mutex_init (&dev_priv->dpio_lock); +mutex_init (&dev_priv->rps.hw_lock); +mutex_init (&dev_priv->modeset_restore_lock); +i915_dump_device_info (dev_priv);if (i915_get_bridge_dev (dev)) {@@ -1602,6 +1609,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)intel_irq_init (dev);intel_gt_init (dev); +intel_gt_reset (dev);/* Try to make sure MCHBAR is enabled before poking at it */ intel_setup_mchbar (dev);@@ -1626,14 +1634,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)if (!IS_I945G (dev) && !IS_I945GM (dev))pci_enable_msi (dev->pdev); -spin_lock_init (&dev_priv->irq_lock); -spin_lock_init (&dev_priv->gpu_error.lock); -spin_lock_init (&dev_priv->rps.lock); -mutex_init (&dev_priv->dpio_lock); - -mutex_init (&dev_priv->rps.hw_lock); -mutex_init (&dev_priv->modeset_restore_lock); - dev_priv->num_plane =1 ;if (IS_VALLEYVIEW (dev)) dev_priv->num_plane =2 ;drivers/gpu/drm/i915/intel_pm.c +3
-2@@ -4497,6 +4497,9 @@ void intel_gt_reset(struct drm_device *dev)if (IS_IVYBRIDGE (dev) ||IS_HASWELL (dev))__gen6_gt_force_wake_mt_reset (dev_priv); } + +/* BIOS often leaves RC6 enabled, but disable it for hw init */ +intel_disable_gt_powersave (dev); }void intel_gt_init (struct drm_device *dev)@@ -4505,8 +4508,6 @@ void intel_gt_init(struct drm_device *dev)spin_lock_init (&dev_priv->gt_lock); -intel_gt_reset (dev); -if (IS_VALLEYVIEW (dev)) { dev_priv->gt.force_wake_get = vlv_force_wake_get; dev_priv->gt.force_wake_put = vlv_force_wake_put;
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 14 lines · On Wed, 17 Jul 2013 10:22:58 +0400 Konstantin Khlebnikov…
This patch fixes regression in power consumtion of sandy bridge gpu, which exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that it's extremely busy. After that it never reaches rc6 state.
Bug exists since kernel v3.6, commit
b4ae3f22d238617ca11610b29fde16cf8c0bc6e0("drm/i915: load boot context at driver init time").For some reason RC6 is already enabled at the beginning of resuming process. Following initliaztion breaks some internal state and confuses RPS engine. This patch disables RC6 at the beginnig of resume and initialization.
I've rearranged initialization sequence, because
intel_disable_gt_powersave()needs initialized force_wake_get/put and some locks from thedev_priv.
Quoted 7 lines
References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
References: https://bugzilla.kernel.org/show_bug.cgi?id=58971Signed-off-by Konstantin Khlebnikov <[email protected]>Cc Daniel Vetter <[email protected]>Cc Chris Wilson <[email protected]>Cc Jesse Barnes <[email protected]>---
My hero!
So the later init change didn't work?
Either way, great to have this fix in the tree... thanks again.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center