Re: [PATCH] drm/i915: Remove wait for for punit to updates freq.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 29, 2015 at 08:20:20AM +0530, Deepak S wrote:
> 
> 
> On Wednesday 29 April 2015 12:02 AM, Ville Syrjälä wrote:
> > On Tue, Apr 28, 2015 at 11:16:29AM -0700, Jesse Barnes wrote:
> >> On 03/04/2015 08:08 PM, deepak.s@linux.intel.com wrote:
> >>> From: Deepak S <deepak.s@linux.intel.com>
> >>>
> >>> When GPU is idle on VLV, Request freq to punit should be good enough to
> >>> get the voltage back to VNN. Also, make sure gfx clock force applies
> >>> before requesting the freq fot vlv.
> >>>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75244
> >>> suggested-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/intel_pm.c | 20 ++++----------------
> >>>   1 file changed, 4 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>> index e710b43..2e1ed07 100644
> >>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>> @@ -3894,7 +3894,7 @@ static void valleyview_set_rps(struct drm_device *dev, u8 val)
> >>>    * * If Gfx is Idle, then
> >>>    * 1. Mask Turbo interrupts
> >>>    * 2. Bring up Gfx clock
> >>> - * 3. Change the freq to Rpn and wait till P-Unit updates freq
> >>> + * 3. Request the freq to Rpn.
> >>>    * 4. Clear the Force GFX CLK ON bit so that Gfx can down
> >>>    * 5. Unmask Turbo interrupts
> >>>   */
> >>> @@ -3902,8 +3902,8 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> >>>   {
> >>>   	struct drm_device *dev = dev_priv->dev;
> >>>   
> >>> -	/* CHV and latest VLV don't need to force the gfx clock */
> >>> -	if (IS_CHERRYVIEW(dev) || dev->pdev->revision >= 0xd) {
> >>> +	/* CHV don't need to force the gfx clock */
> >>> +	if (IS_CHERRYVIEW(dev)) {
> >>>   		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> >>>   		return;
> >>>   	}
> >>> @@ -3920,20 +3920,8 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> >>>   		   gen6_sanitize_rps_pm_mask(dev_priv, ~0));
> >>>   
> >>>   	vlv_force_gfx_clock(dev_priv, true);
> >>> -
> >>> -	dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
> >>> -
> >>> -	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> >>> -					dev_priv->rps.min_freq_softlimit);
> >>> -
> >>> -	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> >>> -				& GENFREQSTATUS) == 0, 100))
> >>> -		DRM_ERROR("timed out waiting for Punit\n");
> >>> -
> >>> +	valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> >>>   	vlv_force_gfx_clock(dev_priv, false);
> >>> -
> >>> -	I915_WRITE(GEN6_PMINTRMSK,
> >>> -		   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
> >>>   }
> >>>   
> >>>   void gen6_rps_idle(struct drm_i915_private *dev_priv)
> >>>
> >> Yeah I think this is fine (may need a rebase though, you can keep my r-b
> >> if you do that in case Jani doesn't want to deal with the merge conflicts).
> >>
> >> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > The removal of the stepping check is still confusing me even if the
> > rest would be OK.
> >
> Stepping check was added latest BYT release. On older BYT stepping, We used to wait for punit to grant the freq in GT Idle case, (most of the cases punit is timing out :( )
> We now make the gfx clock force apply to all VLV and then request the freq to RPn this should be good enough to get voltage to Vnn.

But we shouldn't need the gfx clock force for the latest VLV
stepping(s), and we certainly didn't do it before. So why do
it now?

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