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

[Daniel Vetter <daniel@ffwll.ch>]

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