diff mbox

[Intel-gfx] drm/i915: fix long-standing SNB regression in power consumption after resume

Message ID 20130716131925.7997a7ed@jbarnes-desktop (mailing list archive)
State New, archived
Headers show

Commit Message

Jesse Barnes July 16, 2013, 8:19 p.m. UTC
On Tue, 16 Jul 2013 10:06:54 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Tue, 16 Jul 2013 11:34:25 +0400
> Konstantin Khlebnikov <khlebnikov@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,

Comments

Daniel Vetter July 16, 2013, 8:43 p.m. UTC | #1
On Tue, Jul 16, 2013 at 01:19:25PM -0700, Jesse Barnes wrote:
> On Tue, 16 Jul 2013 10:06:54 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > On Tue, 16 Jul 2013 11:34:25 +0400
> > Konstantin Khlebnikov <khlebnikov@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-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Jesse Barnes July 16, 2013, 8:52 p.m. UTC | #2
On Tue, 16 Jul 2013 22:43:49 +0200
Daniel Vetter <daniel@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 <jbarnes@virtuousgeek.org> wrote:
> > 
> > > On Tue, 16 Jul 2013 11:34:25 +0400
> > > Konstantin Khlebnikov <khlebnikov@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.
diff mbox

Patch

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;