Re: [Intel-gfx] [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, 16 Jul 2013 22:43:49 +0200 Daniel Vetter wrote: > On Tue, Jul 16, 2013 at 01:19:25PM -0700, Jesse Barnes wrote: > > On Tue, 16 Jul 2013 10:06:54 -0700 > > Jesse Barnes wrote: > > > > > On Tue, 16 Jul 2013 11:34:25 +0400 > > > Konstantin Khlebnikov wrote: > > > > 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. > > > > Oops, last one triggers a warn about IRQs. This one doesn't and still > > works for me. > > > > Testing welcome. > > > > Thanks, > > -- > > Jesse Barnes, Intel Open Source Technology Center > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > b/drivers/gpu/drm/i915/i915_gem.c > > index c9d9d20..d962ec0 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -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. 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. -- Jesse Barnes, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Intel-gfx] [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, Jul 16, 2013 at 01:19:25PM -0700, Jesse Barnes wrote: > On Tue, 16 Jul 2013 10:06:54 -0700 > Jesse Barnes wrote: > > > On Tue, 16 Jul 2013 11:34:25 +0400 > > Konstantin Khlebnikov wrote: > > > 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. > > Oops, last one triggers a warn about IRQs. This one doesn't and still > works for me. > > Testing welcome. > > Thanks, > -- > Jesse Barnes, Intel Open Source Technology Center > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index c9d9d20..d962ec0 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -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 > + > if (INTEL_INFO(dev)->gen < 6 && !intel_enable_gtt()) > return -EIO; > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Intel-gfx] [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, 16 Jul 2013 10:06:54 -0700 Jesse Barnes wrote: > On Tue, 16 Jul 2013 11:34:25 +0400 > Konstantin Khlebnikov wrote: > > 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. Oops, last one triggers a warn about IRQs. This one doesn't and still works for me. Testing welcome. Thanks, -- Jesse Barnes, Intel Open Source Technology Center diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c9d9d20..d962ec0 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -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; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Intel-gfx] [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, 16 Jul 2013 11:34:25 +0400
Konstantin Khlebnikov wrote:
> 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,
--
Jesse Barnes, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_d
index 12ea1a9..9152cba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, Jul 16, 2013 at 08:32:40AM +0200, Daniel Vetter wrote:
> On Sun, Jul 14, 2013 at 08:30:09PM +0400, Konstantin Khlebnikov wrote:
> > 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
> > Signed-off-by: Konstantin Khlebnikov
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=58971
> Tested-by: Alexander Kaltsas
> Tested-by: rocko
Tested-by: JohnMB
>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c |4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index aa01128..839a43f 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -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)
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, Jul 16, 2013 at 10:31 AM, Chris Wilson wrote: > On Tue, Jul 16, 2013 at 09:44:59AM +0200, Daniel Vetter wrote: >> 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, Jul 16, 2013 at 09:44:59AM +0200, Daniel Vetter wrote: > 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 -- Chris Wilson, Intel Open Source Technology Centre -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, Jul 16, 2013 at 11:34:25AM +0400, Konstantin Khlebnikov wrote:
> On Tue, Jul 16, 2013 at 10:31 AM, Daniel Vetter wrote:
>
> > On Sun, Jul 14, 2013 at 09:56:45PM +0400, Konstantin Khlebnikov wrote:
> > > Daniel Vetter wrote:
> > > >On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
> > > > wrote:
> > > >>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
> > > >>Signed-off-by: Konstantin Khlebnikov
> > > >
> > > >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?
> > >
> > > 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.
> >
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Sun, Jul 14, 2013 at 08:30:09PM +0400, Konstantin Khlebnikov wrote:
> 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
> Signed-off-by: Konstantin Khlebnikov
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=58971
Tested-by: Alexander Kaltsas
Tested-by: rocko
> ---
> drivers/gpu/drm/i915/intel_pm.c |4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index aa01128..839a43f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -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)
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Sun, Jul 14, 2013 at 09:56:45PM +0400, Konstantin Khlebnikov wrote:
> Daniel Vetter wrote:
> >On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
> > wrote:
> >>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
> >>Signed-off-by: Konstantin Khlebnikov
> >
> >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?
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Sun, Jul 14, 2013 at 09:56:45PM +0400, Konstantin Khlebnikov wrote: Daniel Vetter wrote: On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov khlebni...@openvz.org wrote: 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 Signed-off-by: Konstantin Khlebnikovkhlebni...@openvz.org 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? 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Sun, Jul 14, 2013 at 08:30:09PM +0400, Konstantin Khlebnikov wrote: 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 Signed-off-by: Konstantin Khlebnikov khlebni...@openvz.org Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=58971 Tested-by: Alexander Kaltsas alexkalt...@gmail.com Tested-by: rocko rockoreq...@hotmail.com --- drivers/gpu/drm/i915/intel_pm.c |4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index aa01128..839a43f 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -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) -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, Jul 16, 2013 at 11:34:25AM +0400, Konstantin Khlebnikov wrote: On Tue, Jul 16, 2013 at 10:31 AM, Daniel Vetter dan...@ffwll.ch wrote: On Sun, Jul 14, 2013 at 09:56:45PM +0400, Konstantin Khlebnikov wrote: Daniel Vetter wrote: On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov khlebni...@openvz.org wrote: 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 Signed-off-by: Konstantin Khlebnikovkhlebni...@openvz.org 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? 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. 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, Jul 16, 2013 at 09:44:59AM +0200, Daniel Vetter wrote: 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 -- Chris Wilson, Intel Open Source Technology Centre -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, Jul 16, 2013 at 10:31 AM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, Jul 16, 2013 at 09:44:59AM +0200, Daniel Vetter wrote: 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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, Jul 16, 2013 at 08:32:40AM +0200, Daniel Vetter wrote: On Sun, Jul 14, 2013 at 08:30:09PM +0400, Konstantin Khlebnikov wrote: 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 Signed-off-by: Konstantin Khlebnikov khlebni...@openvz.org Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=58971 Tested-by: Alexander Kaltsas alexkalt...@gmail.com Tested-by: rocko rockoreq...@hotmail.com Tested-by: JohnMB johnmbry...@sky.com --- drivers/gpu/drm/i915/intel_pm.c |4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index aa01128..839a43f 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -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) -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Intel-gfx] [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, 16 Jul 2013 11:34:25 +0400
Konstantin Khlebnikov khlebni...@openvz.org wrote:
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,
--
Jesse Barnes, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_d
index 12ea1a9..9152cba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Intel-gfx] [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, 16 Jul 2013 10:06:54 -0700 Jesse Barnes jbar...@virtuousgeek.org wrote: On Tue, 16 Jul 2013 11:34:25 +0400 Konstantin Khlebnikov khlebni...@openvz.org wrote: 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. Oops, last one triggers a warn about IRQs. This one doesn't and still works for me. Testing welcome. Thanks, -- Jesse Barnes, Intel Open Source Technology Center diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c9d9d20..d962ec0 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -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; -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Intel-gfx] [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, Jul 16, 2013 at 01:19:25PM -0700, Jesse Barnes wrote: On Tue, 16 Jul 2013 10:06:54 -0700 Jesse Barnes jbar...@virtuousgeek.org wrote: On Tue, 16 Jul 2013 11:34:25 +0400 Konstantin Khlebnikov khlebni...@openvz.org wrote: 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. Oops, last one triggers a warn about IRQs. This one doesn't and still works for me. Testing welcome. Thanks, -- Jesse Barnes, Intel Open Source Technology Center diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c9d9d20..d962ec0 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -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 + if (INTEL_INFO(dev)-gen 6 !intel_enable_gtt()) return -EIO; ___ dri-devel mailing list dri-de...@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Intel-gfx] [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Tue, 16 Jul 2013 22:43:49 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Tue, Jul 16, 2013 at 01:19:25PM -0700, Jesse Barnes wrote: On Tue, 16 Jul 2013 10:06:54 -0700 Jesse Barnes jbar...@virtuousgeek.org wrote: On Tue, 16 Jul 2013 11:34:25 +0400 Konstantin Khlebnikov khlebni...@openvz.org wrote: 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. Oops, last one triggers a warn about IRQs. This one doesn't and still works for me. Testing welcome. Thanks, -- Jesse Barnes, Intel Open Source Technology Center diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c9d9d20..d962ec0 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -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. 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. -- Jesse Barnes, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Sun, Jul 14, 2013 at 06:52:39PM +0200, Daniel Vetter wrote:
> On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
> wrote:
> > 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
> > Signed-off-by: Konstantin Khlebnikov
>
> 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.
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
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
--
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
Daniel Vetter wrote:
On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
wrote:
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
Signed-off-by: Konstantin Khlebnikov
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?
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.
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
---
drivers/gpu/drm/i915/intel_pm.c |4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aa01128..839a43f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -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)
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
wrote:
> 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
> Signed-off-by: Konstantin Khlebnikov
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
> ---
> drivers/gpu/drm/i915/intel_pm.c |4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index aa01128..839a43f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -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)
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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
Signed-off-by: Konstantin Khlebnikov
---
drivers/gpu/drm/i915/intel_pm.c |4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aa01128..839a43f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -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)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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 Signed-off-by: Konstantin Khlebnikov khlebni...@openvz.org --- drivers/gpu/drm/i915/intel_pm.c |4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index aa01128..839a43f 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -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) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov khlebni...@openvz.org wrote: 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 Signed-off-by: Konstantin Khlebnikov khlebni...@openvz.org 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 --- drivers/gpu/drm/i915/intel_pm.c |4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index aa01128..839a43f 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -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) -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
Daniel Vetter wrote: On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov khlebni...@openvz.org wrote: 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 Signed-off-by: Konstantin Khlebnikovkhlebni...@openvz.org 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? 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. 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 --- drivers/gpu/drm/i915/intel_pm.c |4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index aa01128..839a43f 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -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) -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume
On Sun, Jul 14, 2013 at 06:52:39PM +0200, Daniel Vetter wrote: On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov khlebni...@openvz.org wrote: 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 Signed-off-by: Konstantin Khlebnikov khlebni...@openvz.org 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. 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 ch...@chris-wilson.co.uk 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 -- Chris Wilson, Intel Open Source Technology Centre -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
