[PATCH] drm/i915: Correct sandybrige overclocking

[PATCH] drm/i915: Correct sandybrige overclocking

[Ben Widawsky]

Change the gen6+ max delay if the pcode read was successful (not the
inverse).

The previous code was all sorts of wrong and has existed since I broke
it:
commit 42c0526c930523425ff6edc95b7235ce7ab9308d
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Wed Sep 26 10:34:00 2012 -0700

    drm/i915: Extract PCU communication

I added some parentheses for clarity, and I also corrected the debug
message message to use the mask (wrong before I came along) and added a
print to show the value we're changing from.

Looking over the code, I'm not actually sure what we're trying to do. I
introduced the bug simply by extracting the function not implementing
anything new. We already set max_delay based on the capabilities
register (which is what we use elsewhere to determine min and max).
This would potentially increase it, I suppose? Jesse, I can't find the
document which explains the definitions of the pcode commands, maybe you
have it around.

Based on Jesse's response, this could potentially be for -fixes, or
stable, or maybe lead to us dropping it entirely. As the current code is
is, things won't completely break because of the aforementioned
capabilities register, and in my experimentation, enabling this has no
effect, it goes from 1100->1100.

I found this while reviewing Jesse's VLV patches.

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c30e89a..86729b1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2631,9 +2631,11 @@ static void gen6_enable_rps(struct drm_device *dev)
 	if (!ret) {
 		pcu_mbox = 0;
 		ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, &pcu_mbox);
-		if (ret && pcu_mbox & (1<<31)) { /* OC supported */
+		if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */
+			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max from %dMHz to %dMHz\n",
+					 ((dev_priv->rps.max_delay & 0xff) * 50),
+					 ((pcu_mbox & 0xff) * 50));
 			dev_priv->rps.max_delay = pcu_mbox & 0xff;
-			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max to %dMHz\n", pcu_mbox * 50);
 		}
 	} else {
 		DRM_DEBUG_DRIVER("Failed to set the min frequency\n");
-- 
1.8.2

Re: [PATCH] drm/i915: Correct sandybrige overclocking

[Chris Wilson]

On Tue, Mar 19, 2013 at 08:19:56PM -0700, Ben Widawsky wrote:
> Change the gen6+ max delay if the pcode read was successful (not the
> inverse).
> 
> The previous code was all sorts of wrong and has existed since I broke
> it:
> commit 42c0526c930523425ff6edc95b7235ce7ab9308d
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Wed Sep 26 10:34:00 2012 -0700
> 
>     drm/i915: Extract PCU communication
> 
> I added some parentheses for clarity,

(Clarity)! ((They) ((were) (not)) (even) (connected) (to) ((the) (mistake))).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Correct sandybrige overclocking

[Jesse Barnes]

On Tue, 19 Mar 2013 20:19:56 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> Change the gen6+ max delay if the pcode read was successful (not the
> inverse).
> 
> The previous code was all sorts of wrong and has existed since I broke
> it:
> commit 42c0526c930523425ff6edc95b7235ce7ab9308d
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Wed Sep 26 10:34:00 2012 -0700
> 
>     drm/i915: Extract PCU communication
> 
> I added some parentheses for clarity, and I also corrected the debug
> message message to use the mask (wrong before I came along) and added a
> print to show the value we're changing from.
> 
> Looking over the code, I'm not actually sure what we're trying to do. I
> introduced the bug simply by extracting the function not implementing
> anything new. We already set max_delay based on the capabilities
> register (which is what we use elsewhere to determine min and max).
> This would potentially increase it, I suppose? Jesse, I can't find the
> document which explains the definitions of the pcode commands, maybe you
> have it around.
> 
> Based on Jesse's response, this could potentially be for -fixes, or
> stable, or maybe lead to us dropping it entirely. As the current code is
> is, things won't completely break because of the aforementioned
> capabilities register, and in my experimentation, enabling this has no
> effect, it goes from 1100->1100.
> 
> I found this while reviewing Jesse's VLV patches.
> 
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c30e89a..86729b1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2631,9 +2631,11 @@ static void gen6_enable_rps(struct drm_device *dev)
>  	if (!ret) {
>  		pcu_mbox = 0;
>  		ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, &pcu_mbox);
> -		if (ret && pcu_mbox & (1<<31)) { /* OC supported */
> +		if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */
> +			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max from %dMHz to %dMHz\n",
> +					 ((dev_priv->rps.max_delay & 0xff) * 50),
> +					 ((pcu_mbox & 0xff) * 50));
>  			dev_priv->rps.max_delay = pcu_mbox & 0xff;
> -			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max to %dMHz\n", pcu_mbox * 50);
>  		}
>  	} else {
>  		DRM_DEBUG_DRIVER("Failed to set the min frequency\n");

Yeah looks ok.  I don't think I've seen a system where this flag gets
set, but according to the docs this is the right thing to do.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Correct sandybrige overclocking

[Ben Widawsky]

On Wed, Mar 20, 2013 at 09:21:47AM -0700, Jesse Barnes wrote:
> On Tue, 19 Mar 2013 20:19:56 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > Change the gen6+ max delay if the pcode read was successful (not the
> > inverse).
> > 
> > The previous code was all sorts of wrong and has existed since I broke
> > it:
> > commit 42c0526c930523425ff6edc95b7235ce7ab9308d
> > Author: Ben Widawsky <ben@bwidawsk.net>
> > Date:   Wed Sep 26 10:34:00 2012 -0700
> > 
> >     drm/i915: Extract PCU communication
> > 
> > I added some parentheses for clarity, and I also corrected the debug
> > message message to use the mask (wrong before I came along) and added a
> > print to show the value we're changing from.
> > 
> > Looking over the code, I'm not actually sure what we're trying to do. I
> > introduced the bug simply by extracting the function not implementing
> > anything new. We already set max_delay based on the capabilities
> > register (which is what we use elsewhere to determine min and max).
> > This would potentially increase it, I suppose? Jesse, I can't find the
> > document which explains the definitions of the pcode commands, maybe you
> > have it around.
> > 
> > Based on Jesse's response, this could potentially be for -fixes, or
> > stable, or maybe lead to us dropping it entirely. As the current code is
> > is, things won't completely break because of the aforementioned
> > capabilities register, and in my experimentation, enabling this has no
> > effect, it goes from 1100->1100.
> > 
> > I found this while reviewing Jesse's VLV patches.
> > 
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index c30e89a..86729b1 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2631,9 +2631,11 @@ static void gen6_enable_rps(struct drm_device *dev)
> >  	if (!ret) {
> >  		pcu_mbox = 0;
> >  		ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, &pcu_mbox);
> > -		if (ret && pcu_mbox & (1<<31)) { /* OC supported */
> > +		if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */
> > +			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max from %dMHz to %dMHz\n",
> > +					 ((dev_priv->rps.max_delay & 0xff) * 50),
> > +					 ((pcu_mbox & 0xff) * 50));
> >  			dev_priv->rps.max_delay = pcu_mbox & 0xff;
> > -			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max to %dMHz\n", pcu_mbox * 50);
> >  		}
> >  	} else {
> >  		DRM_DEBUG_DRIVER("Failed to set the min frequency\n");
> 
> Yeah looks ok.  I don't think I've seen a system where this flag gets
> set, but according to the docs this is the right thing to do.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center

So from what I gather, we shouldn't bother with -fixes, or stable,
correct?

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Correct sandybrige overclocking

[Jesse Barnes]

On Wed, 20 Mar 2013 09:33:28 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Wed, Mar 20, 2013 at 09:21:47AM -0700, Jesse Barnes wrote:
> > On Tue, 19 Mar 2013 20:19:56 -0700
> > Ben Widawsky <ben@bwidawsk.net> wrote:
> > 
> > > Change the gen6+ max delay if the pcode read was successful (not the
> > > inverse).
> > > 
> > > The previous code was all sorts of wrong and has existed since I broke
> > > it:
> > > commit 42c0526c930523425ff6edc95b7235ce7ab9308d
> > > Author: Ben Widawsky <ben@bwidawsk.net>
> > > Date:   Wed Sep 26 10:34:00 2012 -0700
> > > 
> > >     drm/i915: Extract PCU communication
> > > 
> > > I added some parentheses for clarity, and I also corrected the debug
> > > message message to use the mask (wrong before I came along) and added a
> > > print to show the value we're changing from.
> > > 
> > > Looking over the code, I'm not actually sure what we're trying to do. I
> > > introduced the bug simply by extracting the function not implementing
> > > anything new. We already set max_delay based on the capabilities
> > > register (which is what we use elsewhere to determine min and max).
> > > This would potentially increase it, I suppose? Jesse, I can't find the
> > > document which explains the definitions of the pcode commands, maybe you
> > > have it around.
> > > 
> > > Based on Jesse's response, this could potentially be for -fixes, or
> > > stable, or maybe lead to us dropping it entirely. As the current code is
> > > is, things won't completely break because of the aforementioned
> > > capabilities register, and in my experimentation, enabling this has no
> > > effect, it goes from 1100->1100.
> > > 
> > > I found this while reviewing Jesse's VLV patches.
> > > 
> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index c30e89a..86729b1 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -2631,9 +2631,11 @@ static void gen6_enable_rps(struct drm_device *dev)
> > >  	if (!ret) {
> > >  		pcu_mbox = 0;
> > >  		ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, &pcu_mbox);
> > > -		if (ret && pcu_mbox & (1<<31)) { /* OC supported */
> > > +		if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */
> > > +			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max from %dMHz to %dMHz\n",
> > > +					 ((dev_priv->rps.max_delay & 0xff) * 50),
> > > +					 ((pcu_mbox & 0xff) * 50));
> > >  			dev_priv->rps.max_delay = pcu_mbox & 0xff;
> > > -			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max to %dMHz\n", pcu_mbox * 50);
> > >  		}
> > >  	} else {
> > >  		DRM_DEBUG_DRIVER("Failed to set the min frequency\n");
> > 
> > Yeah looks ok.  I don't think I've seen a system where this flag gets
> > set, but according to the docs this is the right thing to do.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > -- 
> > Jesse Barnes, Intel Open Source Technology Center
> 
> So from what I gather, we shouldn't bother with -fixes, or stable,
> correct?

Correct, unless someone complains about their gfx suddenly being a bit
slower, it's not worth the risk.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Correct sandybrige overclocking

[Daniel Vetter]

On Wed, Mar 20, 2013 at 09:57:53AM -0700, Jesse Barnes wrote:
> On Wed, 20 Mar 2013 09:33:28 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > On Wed, Mar 20, 2013 at 09:21:47AM -0700, Jesse Barnes wrote:
> > > On Tue, 19 Mar 2013 20:19:56 -0700
> > > Ben Widawsky <ben@bwidawsk.net> wrote:
> > > 
> > > > Change the gen6+ max delay if the pcode read was successful (not the
> > > > inverse).
> > > > 
> > > > The previous code was all sorts of wrong and has existed since I broke
> > > > it:
> > > > commit 42c0526c930523425ff6edc95b7235ce7ab9308d
> > > > Author: Ben Widawsky <ben@bwidawsk.net>
> > > > Date:   Wed Sep 26 10:34:00 2012 -0700
> > > > 
> > > >     drm/i915: Extract PCU communication
> > > > 
> > > > I added some parentheses for clarity, and I also corrected the debug
> > > > message message to use the mask (wrong before I came along) and added a
> > > > print to show the value we're changing from.
> > > > 
> > > > Looking over the code, I'm not actually sure what we're trying to do. I
> > > > introduced the bug simply by extracting the function not implementing
> > > > anything new. We already set max_delay based on the capabilities
> > > > register (which is what we use elsewhere to determine min and max).
> > > > This would potentially increase it, I suppose? Jesse, I can't find the
> > > > document which explains the definitions of the pcode commands, maybe you
> > > > have it around.
> > > > 
> > > > Based on Jesse's response, this could potentially be for -fixes, or
> > > > stable, or maybe lead to us dropping it entirely. As the current code is
> > > > is, things won't completely break because of the aforementioned
> > > > capabilities register, and in my experimentation, enabling this has no
> > > > effect, it goes from 1100->1100.
> > > > 
> > > > I found this while reviewing Jesse's VLV patches.
> > > > 
> > > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index c30e89a..86729b1 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -2631,9 +2631,11 @@ static void gen6_enable_rps(struct drm_device *dev)
> > > >  	if (!ret) {
> > > >  		pcu_mbox = 0;
> > > >  		ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, &pcu_mbox);
> > > > -		if (ret && pcu_mbox & (1<<31)) { /* OC supported */
> > > > +		if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */
> > > > +			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max from %dMHz to %dMHz\n",
> > > > +					 ((dev_priv->rps.max_delay & 0xff) * 50),
> > > > +					 ((pcu_mbox & 0xff) * 50));
> > > >  			dev_priv->rps.max_delay = pcu_mbox & 0xff;
> > > > -			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max to %dMHz\n", pcu_mbox * 50);
> > > >  		}
> > > >  	} else {
> > > >  		DRM_DEBUG_DRIVER("Failed to set the min frequency\n");
> > > 
> > > Yeah looks ok.  I don't think I've seen a system where this flag gets
> > > set, but according to the docs this is the right thing to do.
> > > 
> > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > 
> > > -- 
> > > Jesse Barnes, Intel Open Source Technology Center
> > 
> > So from what I gather, we shouldn't bother with -fixes, or stable,
> > correct?
> 
> Correct, unless someone complains about their gfx suddenly being a bit
> slower, it's not worth the risk.

Concurred and so merged to dinq, thanks for the patch. I've also applied
Chris' bikeshed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Correct sandybrige overclocking

[Daniel Vetter]

On Tue, Mar 19, 2013 at 08:19:56PM -0700, Ben Widawsky wrote:
> Change the gen6+ max delay if the pcode read was successful (not the
> inverse).
> 
> The previous code was all sorts of wrong and has existed since I broke
> it:
> commit 42c0526c930523425ff6edc95b7235ce7ab9308d
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Wed Sep 26 10:34:00 2012 -0700
> 
>     drm/i915: Extract PCU communication
> 
> I added some parentheses for clarity, and I also corrected the debug
> message message to use the mask (wrong before I came along) and added a
> print to show the value we're changing from.
> 
> Looking over the code, I'm not actually sure what we're trying to do. I
> introduced the bug simply by extracting the function not implementing
> anything new. We already set max_delay based on the capabilities
> register (which is what we use elsewhere to determine min and max).
> This would potentially increase it, I suppose? Jesse, I can't find the
> document which explains the definitions of the pcode commands, maybe you
> have it around.
> 
> Based on Jesse's response, this could potentially be for -fixes, or
> stable, or maybe lead to us dropping it entirely. As the current code is
> is, things won't completely break because of the aforementioned
> capabilities register, and in my experimentation, enabling this has no
> effect, it goes from 1100->1100.
> 
> I found this while reviewing Jesse's VLV patches.
> 
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Random guy on irc just confirmed that this actually fixes gpu overclocking
on his machine: The overclock limit jumped from 1.1GHz to 1.9GHz (which is
what he selected in his bios). Unfortunately he jumped irc before I could
grab his tested-by, so won't (yet) move the patch to -fixes with a cc:
stable.

So: (Other) testers highly welcome ;-)

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c30e89a..86729b1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2631,9 +2631,11 @@ static void gen6_enable_rps(struct drm_device *dev)
>  	if (!ret) {
>  		pcu_mbox = 0;
>  		ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, &pcu_mbox);
> -		if (ret && pcu_mbox & (1<<31)) { /* OC supported */
> +		if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */
> +			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max from %dMHz to %dMHz\n",
> +					 ((dev_priv->rps.max_delay & 0xff) * 50),
> +					 ((pcu_mbox & 0xff) * 50));
>  			dev_priv->rps.max_delay = pcu_mbox & 0xff;
> -			DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max to %dMHz\n", pcu_mbox * 50);
>  		}
>  	} else {
>  		DRM_DEBUG_DRIVER("Failed to set the min frequency\n");
> -- 
> 1.8.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Correct sandybrige overclocking

[Daniel Vetter]

On Sat, Mar 23, 2013 at 08:32:53PM +0100, Daniel Vetter wrote:
> On Tue, Mar 19, 2013 at 08:19:56PM -0700, Ben Widawsky wrote:
> > Change the gen6+ max delay if the pcode read was successful (not the
> > inverse).
> > 
> > The previous code was all sorts of wrong and has existed since I broke
> > it:
> > commit 42c0526c930523425ff6edc95b7235ce7ab9308d
> > Author: Ben Widawsky <ben@bwidawsk.net>
> > Date:   Wed Sep 26 10:34:00 2012 -0700
> > 
> >     drm/i915: Extract PCU communication
> > 
> > I added some parentheses for clarity, and I also corrected the debug
> > message message to use the mask (wrong before I came along) and added a
> > print to show the value we're changing from.
> > 
> > Looking over the code, I'm not actually sure what we're trying to do. I
> > introduced the bug simply by extracting the function not implementing
> > anything new. We already set max_delay based on the capabilities
> > register (which is what we use elsewhere to determine min and max).
> > This would potentially increase it, I suppose? Jesse, I can't find the
> > document which explains the definitions of the pcode commands, maybe you
> > have it around.
> > 
> > Based on Jesse's response, this could potentially be for -fixes, or
> > stable, or maybe lead to us dropping it entirely. As the current code is
> > is, things won't completely break because of the aforementioned
> > capabilities register, and in my experimentation, enabling this has no
> > effect, it goes from 1100->1100.
> > 
> > I found this while reviewing Jesse's VLV patches.
> > 
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Random guy on irc just confirmed that this actually fixes gpu overclocking
> on his machine: The overclock limit jumped from 1.1GHz to 1.9GHz (which is
> what he selected in his bios). Unfortunately he jumped irc before I could
> grab his tested-by, so won't (yet) move the patch to -fixes with a cc:
> stable.
> 
> So: (Other) testers highly welcome ;-)

Apparently massive overclocking like that makes your system die
prematurely in a crash. So 3.10 it is, imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Correct sandybrige overclocking

[Ben Widawsky]

On Sat, Mar 23, 2013 at 08:39:30PM +0100, Daniel Vetter wrote:
> On Sat, Mar 23, 2013 at 08:32:53PM +0100, Daniel Vetter wrote:
> > On Tue, Mar 19, 2013 at 08:19:56PM -0700, Ben Widawsky wrote:
> > > Change the gen6+ max delay if the pcode read was successful (not the
> > > inverse).
> > > 
> > > The previous code was all sorts of wrong and has existed since I broke
> > > it:
> > > commit 42c0526c930523425ff6edc95b7235ce7ab9308d
> > > Author: Ben Widawsky <ben@bwidawsk.net>
> > > Date:   Wed Sep 26 10:34:00 2012 -0700
> > > 
> > >     drm/i915: Extract PCU communication
> > > 
> > > I added some parentheses for clarity, and I also corrected the debug
> > > message message to use the mask (wrong before I came along) and added a
> > > print to show the value we're changing from.
> > > 
> > > Looking over the code, I'm not actually sure what we're trying to do. I
> > > introduced the bug simply by extracting the function not implementing
> > > anything new. We already set max_delay based on the capabilities
> > > register (which is what we use elsewhere to determine min and max).
> > > This would potentially increase it, I suppose? Jesse, I can't find the
> > > document which explains the definitions of the pcode commands, maybe you
> > > have it around.
> > > 
> > > Based on Jesse's response, this could potentially be for -fixes, or
> > > stable, or maybe lead to us dropping it entirely. As the current code is
> > > is, things won't completely break because of the aforementioned
> > > capabilities register, and in my experimentation, enabling this has no
> > > effect, it goes from 1100->1100.
> > > 
> > > I found this while reviewing Jesse's VLV patches.
> > > 
> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > Random guy on irc just confirmed that this actually fixes gpu overclocking
> > on his machine: The overclock limit jumped from 1.1GHz to 1.9GHz (which is
> > what he selected in his bios). Unfortunately he jumped irc before I could
> > grab his tested-by, so won't (yet) move the patch to -fixes with a cc:
> > stable.
> > 
> > So: (Other) testers highly welcome ;-)
> 
> Apparently massive overclocking like that makes your system die
> prematurely in a crash. So 3.10 it is, imo.
> -Daniel

It's like all overclocking I guess, we simply give the user an
opportunity to shoot themselves in the foot. Glad to hear it is actually
doing something though.

I agree that 3.10 sounds right; I'm thinking we'll want a module
parameter or something as well, to prevent spurious bug reports. What do
you think?

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH] drm/i915: Don't overclock on Haswell

[Ben Widawsky]

HSW doesn't overclock the same way as IVB or SNB. I do not know about
VLV, so I've kept that off as well. I'm still working on getting the doc
updates to explain how we overclock on Haswell.

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ce3db2c..9c11dd9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2628,7 +2628,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 		   (IS_HASWELL(dev) ? GEN7_RP_DOWN_IDLE_AVG : GEN6_RP_DOWN_IDLE_CONT));
 
 	ret = sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_MIN_FREQ_TABLE, 0);
-	if (!ret) {
+	if (!ret && IS_GEN6(dev) || IS_IVYBRIDGE(dev)) {
 		pcu_mbox = 0;
 		ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, &pcu_mbox);
 		if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */
-- 
1.8.2

Re: [PATCH] drm/i915: Correct sandybrige overclocking

[Daniel Vetter]

On Sun, Mar 24, 2013 at 12:56 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
>> Apparently massive overclocking like that makes your system die
>> prematurely in a crash. So 3.10 it is, imo.
>> -Daniel
>
> It's like all overclocking I guess, we simply give the user an
> opportunity to shoot themselves in the foot. Glad to hear it is actually
> doing something though.
>
> I agree that 3.10 sounds right; I'm thinking we'll want a module
> parameter or something as well, to prevent spurious bug reports. What do
> you think?

I think not having a module option is ok, I don't expect that the
enthusiast boards required to adjust the limits are that common. So it
shouldn't affect more than just a few users, and those it does affect
should know what their doing. Module options after all don't prevent
them from setting unstable rc6 options, either ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Don't overclock on Haswell

[Jesse Barnes]

On Sat, 23 Mar 2013 17:46:31 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> HSW doesn't overclock the same way as IVB or SNB. I do not know about
> VLV, so I've kept that off as well. I'm still working on getting the doc
> updates to explain how we overclock on Haswell.
> 
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ce3db2c..9c11dd9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2628,7 +2628,7 @@ static void gen6_enable_rps(struct drm_device *dev)
>  		   (IS_HASWELL(dev) ? GEN7_RP_DOWN_IDLE_AVG : GEN6_RP_DOWN_IDLE_CONT));
>  
>  	ret = sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_MIN_FREQ_TABLE, 0);
> -	if (!ret) {
> +	if (!ret && IS_GEN6(dev) || IS_IVYBRIDGE(dev)) {
>  		pcu_mbox = 0;
>  		ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, &pcu_mbox);
>  		if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */

This won't affect VLV, since I have a new rps enable function in my
latest set.  But it looks fine.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Don't overclock on Haswell

[Daniel Vetter]

On Mon, Mar 25, 2013 at 07:36:51AM -0700, Jesse Barnes wrote:
> On Sat, 23 Mar 2013 17:46:31 -0700
> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > HSW doesn't overclock the same way as IVB or SNB. I do not know about
> > VLV, so I've kept that off as well. I'm still working on getting the doc
> > updates to explain how we overclock on Haswell.
> > 
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ce3db2c..9c11dd9 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2628,7 +2628,7 @@ static void gen6_enable_rps(struct drm_device *dev)
> >  		   (IS_HASWELL(dev) ? GEN7_RP_DOWN_IDLE_AVG : GEN6_RP_DOWN_IDLE_CONT));
> >  
> >  	ret = sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_MIN_FREQ_TABLE, 0);
> > -	if (!ret) {
> > +	if (!ret && IS_GEN6(dev) || IS_IVYBRIDGE(dev)) {
> >  		pcu_mbox = 0;
> >  		ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, &pcu_mbox);
> >  		if (!ret && (pcu_mbox & (1<<31))) { /* OC supported */
> 
> This won't affect VLV, since I have a new rps enable function in my
> latest set.  But it looks fine.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch