Re: [PATCH v4] drm/i915/chv: Set min freq to efficient frequency on chv

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: deepak.s@linux.intel.com
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v4] drm/i915/chv: Set min freq to efficient frequency on chv
Date: Wed, 29 Apr 2015 22:53:20 +0300	[thread overview]
Message-ID: <20150429195320.GU18908@intel.com> (raw)
In-Reply-To: <20150429153156.GT18908@intel.com>

On Wed, Apr 29, 2015 at 06:31:56PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 29, 2015 at 08:23:21AM +0530, deepak.s@linux.intel.com wrote:
> > From: Deepak S <deepak.s@linux.intel.com>
> > 
> > After feedback from the hardware team, now we set the GPU min/idel freq to RPe.
> > Punit is expecting us to operate GPU between Rpe & Rp0. If we drop the
> > frequency to RPn, punit is failing to change the input voltage to
> > minimum :(
> 
> So far I can't reproduce this problem on my BSW. In fact what I see
> that the voltage at RPn is lower than the voltage at RPe, even while
> we're in rc6.
> 
> without forcewake:
> RPn -> 0x66
> RPe -> 0x67
> RP0 -> 0x69
> 
> with forcewake:
> RPn -> 0x66
> RPe -> 0x76
> RP0 -> 0x9d
> 
> Also asking Punit to change the frequency after the GPU has gone to
> rc6 does absolutely nothing (remind anyone of VLV?). I think I need to
> retest my VLV C0 to see if my earlier observations there were accurate.
> The shared Vnn rail does make it harder to observe this stuff on
> VLV though.

I went back to my VLVs (had a B3 and C0 actually). And I'm seeing the
exact same behaviour on both, ie. requesting a new frequency from Punit
does nothing when the GPU is in rc6, and if I let it enter rc6 with a
high frequency Vnn also remains high. Previously I had thought that C0
fixed this, but now it definitely shows the same problem here. I must
have had some accidental forcewake somewhere when I originally tested
it,

So based on that, your other patch to remove the stepping check from
vlv_set_rps_idle() is in fact correct.

The question remains however what should we do with CHV. According to my
testing to get the minimum voltage we should keep RPn around, and we
should also do the vlv_set_rps_idle() workaround on CHV.

Oh and I also observed something else on VLV. Normally when entering rc6
the GPLL ref clock gets trunk gated at CCK (by Punit I assume). However
when using the vlv_gfx_clock_force() that doesn't happen. So I'm not
entirely sure the GPLL gets turned off properly in that case. Maybe we
should just use forcewake instead? Oh and BTW, CHV doesn't do the trunk
gating in either case. I'm not sure where to check if the GPLL is
actually running or not.

> 
> So based on my tests this patch feels a bit wrong.
> 
> > 
> > Since Punit validates the rps range [RPe, RP0]. This patch
> > removes unused cherryview_rps_min_freq function.
> > 
> > v2: Change commit message
> > 
> > v3: set min_freq before idle_freq (chris)
> > 
> > v4: Squash 'Remove unused rps min function' patch
> > 
> > Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 21 ++-------------------
> >  1 file changed, 2 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index a7516ed..78c89ff 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4715,24 +4715,6 @@ static int cherryview_rps_guar_freq(struct drm_i915_private *dev_priv)
> >  	return rp1;
> >  }
> >  
> > -static int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
> > -{
> > -	struct drm_device *dev = dev_priv->dev;
> > -	u32 val, rpn;
> > -
> > -	if (dev->pdev->revision >= 0x20) {
> > -		val = vlv_punit_read(dev_priv, FB_GFX_FMIN_AT_VMIN_FUSE);
> > -		rpn = ((val >> FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT) &
> > -		       FB_GFX_FREQ_FUSE_MASK);
> > -	} else { /* For pre-production hardware */
> > -		val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
> > -		rpn = ((val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) &
> > -		       PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK);
> > -	}
> > -
> > -	return rpn;
> > -}
> > -
> >  static int valleyview_rps_guar_freq(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 val, rp1;
> > @@ -4984,7 +4966,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
> >  			 intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq),
> >  			 dev_priv->rps.rp1_freq);
> >  
> > -	dev_priv->rps.min_freq = cherryview_rps_min_freq(dev_priv);
> > +	/* PUnit validated range is only [RPe, RP0] */
> > +	dev_priv->rps.min_freq = dev_priv->rps.efficient_freq;
> >  	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
> >  			 intel_gpu_freq(dev_priv, dev_priv->rps.min_freq),
> >  			 dev_priv->rps.min_freq);
> > -- 
> > 1.9.1
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx