[PATCH 1/2] IPS driver: don't toggle CPU turbo on unsupported CPUs

[PATCH 1/2] IPS driver: don't toggle CPU turbo on unsupported CPUs

[Jesse Barnes]

If the CPU doesn't support turbo, don't try to enable/disable it.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/platform/x86/intel_ips.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c
index bfa9c72..71d04ef 100644
--- a/drivers/platform/x86/intel_ips.c
+++ b/drivers/platform/x86/intel_ips.c
@@ -51,7 +51,6 @@
  * TODO:
  *   - handle CPU hotplug
  *   - provide turbo enable/disable api
- *   - make sure we can write turbo enable/disable reg based on MISC_EN
  *
  * Related documents:
  *   - CDI 403777, 403778 - Auburndale EDS vol 1 & 2
@@ -325,6 +324,7 @@ struct ips_driver {
 	bool gpu_preferred;
 	bool poll_turbo_status;
 	bool second_cpu;
+	bool turbo_toggle_allowed;
 	struct ips_mcp_limits *limits;
 
 	/* Optional MCH interfaces for if i915 is in use */
@@ -461,7 +461,8 @@ static void ips_enable_cpu_turbo(struct ips_driver *ips)
 	if (ips->__cpu_turbo_on)
 		return;
 
-	on_each_cpu(do_enable_cpu_turbo, ips, 1);
+	if (ips->turbo_toggle_allowed)
+		on_each_cpu(do_enable_cpu_turbo, ips, 1);
 
 	ips->__cpu_turbo_on = true;
 }
@@ -498,7 +499,8 @@ static void ips_disable_cpu_turbo(struct ips_driver *ips)
 	if (!ips->__cpu_turbo_on)
 		return;
 
-	on_each_cpu(do_disable_cpu_turbo, ips, 1);
+	if (ips->turbo_toggle_allowed)
+		on_each_cpu(do_disable_cpu_turbo, ips, 1);
 
 	ips->__cpu_turbo_on = false;
 }
@@ -1332,8 +1334,10 @@ static struct ips_mcp_limits *ips_detect_cpu(struct ips_driver *ips)
 	 * turbo manually or we'll get an illegal MSR access, even though
 	 * turbo will still be available.
 	 */
-	if (!(misc_en & IA32_MISC_TURBO_EN))
-		; /* add turbo MSR write allowed flag if necessary */
+	if (misc_en & IA32_MISC_TURBO_EN)
+		ips->turbo_toggle_allowed = true;
+	else
+		ips->turbo_toggle_allowed = false;
 
 	if (strstr(boot_cpu_data.x86_model_id, "CPU       M"))
 		limits = &ips_sv_limits;
-- 
1.7.2.3

[PATCH 2/2] IPS driver: verify BIOS provided limits

[Jesse Barnes]

They're optional.  If not present or sane, we should use the CPU
defaults.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/platform/x86/intel_ips.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c
index 71d04ef..c402cc4 100644
--- a/drivers/platform/x86/intel_ips.c
+++ b/drivers/platform/x86/intel_ips.c
@@ -665,6 +665,27 @@ static bool mch_exceeded(struct ips_driver *ips)
 }
 
 /**
+ * verify_limits - verify BIOS provided limits
+ * @ips: IPS structure
+ *
+ * BIOS can optionally provide non-default limits for power and temp.  Check
+ * them here and use the defaults if the BIOS values are not provided or
+ * are otherwise unusable.
+ */
+static void verify_limits(struct ips_driver *ips)
+{
+	if (ips->mcp_power_limit < ips->limits->mcp_power_limit ||
+	    ips->mcp_power_limit > 35000)
+		ips->mcp_power_limit = ips->limits->mcp_power_limit;
+
+	if (ips->mcp_temp_limit < ips->limits->core_temp_limit ||
+	    ips->mcp_temp_limit < ips->limits->mch_temp_limit ||
+	    ips->mcp_temp_limit > 150)
+		ips->mcp_temp_limit = min(ips->limits->core_temp_limit,
+					  ips->limits->mch_temp_limit);
+}
+
+/**
  * update_turbo_limits - get various limits & settings from regs
  * @ips: IPS driver struct
  *
@@ -688,6 +709,7 @@ static void update_turbo_limits(struct ips_driver *ips)
 	ips->mcp_temp_limit = thm_readw(THM_PTL);
 	ips->mcp_power_limit = thm_readw(THM_MPPC);
 
+	verify_limits(ips);
 	/* Ignore BIOS CPU vs GPU pref */
 }
 
@@ -1156,6 +1178,7 @@ static irqreturn_t ips_irq_handler(int irq, void *arg)
 				STS_PTL_SHIFT;
 			ips->mcp_power_limit = (tc1 & STS_PPL_MASK) >>
 				STS_PPL_SHIFT;
+			verify_limits(ips);
 			spin_unlock(&ips->turbo_status_lock);
 
 			thm_writeb(THM_SEC, SEC_ACK);
-- 
1.7.2.3

Re: [PATCH 2/2] IPS driver: verify BIOS provided limits

[Jesse Barnes]

On Thu, 23 Sep 2010 23:49:29 +0200
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> They're optional.  If not present or sane, we should use the CPU
> defaults.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/platform/x86/intel_ips.c |   23 +++++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)

Note this patch is necessary but not sufficient.  At least on one of my
test platforms, I see really bad looking chipset power values from the
i915 driver.  That will trigger spurious "power exceeded" messages and
unnecessarily limit your GPU performance.  Working on a fix now.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/2] IPS driver: verify BIOS provided limits

[Jesse Barnes]

On Thu, 23 Sep 2010 14:53:01 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Thu, 23 Sep 2010 23:49:29 +0200
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > They're optional.  If not present or sane, we should use the CPU
> > defaults.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/platform/x86/intel_ips.c |   23 +++++++++++++++++++++++
> >  1 files changed, 23 insertions(+), 0 deletions(-)
> 
> Note this patch is necessary but not sufficient.  At least on one of my
> test platforms, I see really bad looking chipset power values from the
> i915 driver.  That will trigger spurious "power exceeded" messages and
> unnecessarily limit your GPU performance.  Working on a fix now.

Already sent this fix to Chris for inclusion, but here it is for
reference and testing in this thread.

-- 
Jesse Barnes, Intel Open Source Technology Center

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 9d67b48..c74e4e8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1787,9 +1787,9 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 		}
 	}
 
-	div_u64(diff, diff1);
+	diff = div_u64(diff, diff1);
 	ret = ((m * diff) + c);
-	div_u64(ret, 10);
+	ret = div_u64(ret, 10);
 
 	dev_priv->last_count1 = total_count;
 	dev_priv->last_time1 = now;
@@ -1858,7 +1858,7 @@ void i915_update_gfx_val(struct drm_i915_private *dev_priv)
 
 	/* More magic constants... */
 	diff = diff * 1181;
-	div_u64(diff, diffms * 10);
+	diff = div_u64(diff, diffms * 10);
 	dev_priv->gfx_power = diff;
 }
 

Re: [PATCH 1/2] IPS driver: don't toggle CPU turbo on unsupported CPUs

[Jesse Barnes]

On Thu, 23 Sep 2010 23:49:28 +0200
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> If the CPU doesn't support turbo, don't try to enable/disable it.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/platform/x86/intel_ips.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c
> index bfa9c72..71d04ef 100644
> --- a/drivers/platform/x86/intel_ips.c
> +++ b/drivers/platform/x86/intel_ips.c
> @@ -51,7 +51,6 @@
>   * TODO:
>   *   - handle CPU hotplug
>   *   - provide turbo enable/disable api
> - *   - make sure we can write turbo enable/disable reg based on MISC_EN
>   *
>   * Related documents:
>   *   - CDI 403777, 403778 - Auburndale EDS vol 1 & 2
> @@ -325,6 +324,7 @@ struct ips_driver {
>  	bool gpu_preferred;
>  	bool poll_turbo_status;
>  	bool second_cpu;
> +	bool turbo_toggle_allowed;
>  	struct ips_mcp_limits *limits;
>  
>  	/* Optional MCH interfaces for if i915 is in use */
> @@ -461,7 +461,8 @@ static void ips_enable_cpu_turbo(struct ips_driver *ips)
>  	if (ips->__cpu_turbo_on)
>  		return;
>  
> -	on_each_cpu(do_enable_cpu_turbo, ips, 1);
> +	if (ips->turbo_toggle_allowed)
> +		on_each_cpu(do_enable_cpu_turbo, ips, 1);
>  
>  	ips->__cpu_turbo_on = true;
>  }
> @@ -498,7 +499,8 @@ static void ips_disable_cpu_turbo(struct ips_driver *ips)
>  	if (!ips->__cpu_turbo_on)
>  		return;
>  
> -	on_each_cpu(do_disable_cpu_turbo, ips, 1);
> +	if (ips->turbo_toggle_allowed)
> +		on_each_cpu(do_disable_cpu_turbo, ips, 1);
>  
>  	ips->__cpu_turbo_on = false;
>  }
> @@ -1332,8 +1334,10 @@ static struct ips_mcp_limits *ips_detect_cpu(struct ips_driver *ips)
>  	 * turbo manually or we'll get an illegal MSR access, even though
>  	 * turbo will still be available.
>  	 */
> -	if (!(misc_en & IA32_MISC_TURBO_EN))
> -		; /* add turbo MSR write allowed flag if necessary */
> +	if (misc_en & IA32_MISC_TURBO_EN)
> +		ips->turbo_toggle_allowed = true;
> +	else
> +		ips->turbo_toggle_allowed = false;
>  
>  	if (strstr(boot_cpu_data.x86_model_id, "CPU       M"))
>  		limits = &ips_sv_limits;

Rafael and infernix, this patch should address the GPF in the IPS
driver (kernel bug 18742).

Matthew, since I forgot to add a link to the kernel bug in the log, can
you add it when you commit?

http://bugzilla.kernel.org/show_bug.cgi?id=18742

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/2] IPS driver: don't toggle CPU turbo on unsupported CPUs

[Rafael J. Wysocki]

On Thursday, September 23, 2010, Jesse Barnes wrote:
> On Thu, 23 Sep 2010 23:49:28 +0200
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > If the CPU doesn't support turbo, don't try to enable/disable it.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/platform/x86/intel_ips.c |   14 +++++++++-----
> >  1 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c
> > index bfa9c72..71d04ef 100644
> > --- a/drivers/platform/x86/intel_ips.c
> > +++ b/drivers/platform/x86/intel_ips.c
> > @@ -51,7 +51,6 @@
> >   * TODO:
> >   *   - handle CPU hotplug
> >   *   - provide turbo enable/disable api
> > - *   - make sure we can write turbo enable/disable reg based on MISC_EN
> >   *
> >   * Related documents:
> >   *   - CDI 403777, 403778 - Auburndale EDS vol 1 & 2
> > @@ -325,6 +324,7 @@ struct ips_driver {
> >  	bool gpu_preferred;
> >  	bool poll_turbo_status;
> >  	bool second_cpu;
> > +	bool turbo_toggle_allowed;
> >  	struct ips_mcp_limits *limits;
> >  
> >  	/* Optional MCH interfaces for if i915 is in use */
> > @@ -461,7 +461,8 @@ static void ips_enable_cpu_turbo(struct ips_driver *ips)
> >  	if (ips->__cpu_turbo_on)
> >  		return;
> >  
> > -	on_each_cpu(do_enable_cpu_turbo, ips, 1);
> > +	if (ips->turbo_toggle_allowed)
> > +		on_each_cpu(do_enable_cpu_turbo, ips, 1);
> >  
> >  	ips->__cpu_turbo_on = true;
> >  }
> > @@ -498,7 +499,8 @@ static void ips_disable_cpu_turbo(struct ips_driver *ips)
> >  	if (!ips->__cpu_turbo_on)
> >  		return;
> >  
> > -	on_each_cpu(do_disable_cpu_turbo, ips, 1);
> > +	if (ips->turbo_toggle_allowed)
> > +		on_each_cpu(do_disable_cpu_turbo, ips, 1);
> >  
> >  	ips->__cpu_turbo_on = false;
> >  }
> > @@ -1332,8 +1334,10 @@ static struct ips_mcp_limits *ips_detect_cpu(struct ips_driver *ips)
> >  	 * turbo manually or we'll get an illegal MSR access, even though
> >  	 * turbo will still be available.
> >  	 */
> > -	if (!(misc_en & IA32_MISC_TURBO_EN))
> > -		; /* add turbo MSR write allowed flag if necessary */
> > +	if (misc_en & IA32_MISC_TURBO_EN)
> > +		ips->turbo_toggle_allowed = true;
> > +	else
> > +		ips->turbo_toggle_allowed = false;
> >  
> >  	if (strstr(boot_cpu_data.x86_model_id, "CPU       M"))
> >  		limits = &ips_sv_limits;
> 
> Rafael and infernix, this patch should address the GPF in the IPS
> driver (kernel bug 18742).
> 
> Matthew, since I forgot to add a link to the kernel bug in the log, can
> you add it when you commit?
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=18742

Can you drop the patch into the Bugzilla for completeness?

Or is it in patchwork?

Re: [PATCH 1/2] IPS driver: don't toggle CPU turbo on unsupported CPUs

[Jesse Barnes]

On Thu, 23 Sep 2010 23:58:10 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Thursday, September 23, 2010, Jesse Barnes wrote:
> > On Thu, 23 Sep 2010 23:49:28 +0200
> > Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > 
> > > If the CPU doesn't support turbo, don't try to enable/disable it.
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/platform/x86/intel_ips.c |   14 +++++++++-----
> > >  1 files changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c
> > > index bfa9c72..71d04ef 100644
> > > --- a/drivers/platform/x86/intel_ips.c
> > > +++ b/drivers/platform/x86/intel_ips.c
> > > @@ -51,7 +51,6 @@
> > >   * TODO:
> > >   *   - handle CPU hotplug
> > >   *   - provide turbo enable/disable api
> > > - *   - make sure we can write turbo enable/disable reg based on MISC_EN
> > >   *
> > >   * Related documents:
> > >   *   - CDI 403777, 403778 - Auburndale EDS vol 1 & 2
> > > @@ -325,6 +324,7 @@ struct ips_driver {
> > >  	bool gpu_preferred;
> > >  	bool poll_turbo_status;
> > >  	bool second_cpu;
> > > +	bool turbo_toggle_allowed;
> > >  	struct ips_mcp_limits *limits;
> > >  
> > >  	/* Optional MCH interfaces for if i915 is in use */
> > > @@ -461,7 +461,8 @@ static void ips_enable_cpu_turbo(struct ips_driver *ips)
> > >  	if (ips->__cpu_turbo_on)
> > >  		return;
> > >  
> > > -	on_each_cpu(do_enable_cpu_turbo, ips, 1);
> > > +	if (ips->turbo_toggle_allowed)
> > > +		on_each_cpu(do_enable_cpu_turbo, ips, 1);
> > >  
> > >  	ips->__cpu_turbo_on = true;
> > >  }
> > > @@ -498,7 +499,8 @@ static void ips_disable_cpu_turbo(struct ips_driver *ips)
> > >  	if (!ips->__cpu_turbo_on)
> > >  		return;
> > >  
> > > -	on_each_cpu(do_disable_cpu_turbo, ips, 1);
> > > +	if (ips->turbo_toggle_allowed)
> > > +		on_each_cpu(do_disable_cpu_turbo, ips, 1);
> > >  
> > >  	ips->__cpu_turbo_on = false;
> > >  }
> > > @@ -1332,8 +1334,10 @@ static struct ips_mcp_limits *ips_detect_cpu(struct ips_driver *ips)
> > >  	 * turbo manually or we'll get an illegal MSR access, even though
> > >  	 * turbo will still be available.
> > >  	 */
> > > -	if (!(misc_en & IA32_MISC_TURBO_EN))
> > > -		; /* add turbo MSR write allowed flag if necessary */
> > > +	if (misc_en & IA32_MISC_TURBO_EN)
> > > +		ips->turbo_toggle_allowed = true;
> > > +	else
> > > +		ips->turbo_toggle_allowed = false;
> > >  
> > >  	if (strstr(boot_cpu_data.x86_model_id, "CPU       M"))
> > >  		limits = &ips_sv_limits;
> > 
> > Rafael and infernix, this patch should address the GPF in the IPS
> > driver (kernel bug 18742).
> > 
> > Matthew, since I forgot to add a link to the kernel bug in the log, can
> > you add it when you commit?
> > 
> > http://bugzilla.kernel.org/show_bug.cgi?id=18742
> 
> Can you drop the patch into the Bugzilla for completeness?
> 
> Or is it in patchwork?

Dunno about patchwork, but I just added it to the bug.

-- 
Jesse Barnes, Intel Open Source Technology Center