From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated. Date: Fri, 13 Jun 2014 14:57:28 +0300 Message-ID: <20140613115728.GY27580@intel.com> References: <1402654574-5287-1-git-send-email-deepak.s@linux.intel.com> <20140613113344.GX27580@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 990258A389 for ; Fri, 13 Jun 2014 04:57:31 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140613113344.GX27580@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: deepak.s@linux.intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Jun 13, 2014 at 02:33:44PM +0300, Ville Syrj=E4l=E4 wrote: > On Fri, Jun 13, 2014 at 03:46:14PM +0530, deepak.s@linux.intel.com wrote: > > From: Deepak S > > = > > Workaround fixed in BYT. Forcing Gfx clk up not needed, and Requesting = the > > min freq should bring bring the voltage Vnn. > > = > > Signed-off-by: Deepak S > > --- > > drivers/gpu/drm/i915/intel_pm.c | 40 +--------------------------------= ------- > > 1 file changed, 1 insertion(+), 39 deletions(-) > > = > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/int= el_pm.c > > index 0b088fe..9aee28b 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3198,44 +3198,6 @@ void gen6_set_rps(struct drm_device *dev, u8 val) > > trace_intel_gpu_freq_change(val * 50); > > } > > = > > -/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down > > - * > > - * * 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 > > - * 4. Clear the Force GFX CLK ON bit so that Gfx can down > > - * 5. Unmask Turbo interrupts > > -*/ > > -static void vlv_set_rps_idle(struct drm_i915_private *dev_priv) > > -{ > > - /* > > - * When we are idle. Drop to min voltage state. > > - */ > > - > > - if (dev_priv->rps.cur_freq <=3D dev_priv->rps.min_freq_softlimit) > > - return; > > - > > - /* Mask turbo interrupt so that they will not come in between */ > > - I915_WRITE(GEN6_PMINTRMSK, 0xffffffff); > > - > > - vlv_force_gfx_clock(dev_priv, true); > > - > > - dev_priv->rps.cur_freq =3D 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) =3D=3D 0, 5)) > > - DRM_ERROR("timed out waiting for Punit\n"); > > - > > - 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) > > { > > struct drm_device *dev =3D dev_priv->dev; > > @@ -3243,7 +3205,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_p= riv) > > mutex_lock(&dev_priv->rps.hw_lock); > > if (dev_priv->rps.enabled) { > > if (IS_VALLEYVIEW(dev)) > > - vlv_set_rps_idle(dev_priv); > > + valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit); > = > This should take care of https://bugs.freedesktop.org/show_bug.cgi?id=3D7= 5244 > = > I don't know when the hardware got fixed so I'm hesitant to r-b it, but > at least my C0 works fine without this stuff, so: > Acked-by: Ville Syrj=E4l=E4 > = > However to avoid future mishaps I think we should have some kind of a > comment before the valleyview_set_rps() call to let the reader know that > we really need this on VLV to drop the voltage. > = > Also it now occurs to me that we might be leaving the GPU frequency (and > thus Vnn) high during a system suspend. I think we need an explicit > rps_idle() call in the suspend path somewhere. Runtime suspend should be > fine already since it depends on intel_mark_idle() getting called before > the last rpm reference is dropped. Maybe this is all we need? diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_ge= m.c index 3768199..fabd852 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4530,7 +4530,7 @@ i915_gem_suspend(struct drm_device *dev) = del_timer_sync(&dev_priv->gpu_error.hangcheck_timer); cancel_delayed_work_sync(&dev_priv->mm.retire_work); - cancel_delayed_work_sync(&dev_priv->mm.idle_work); + flush_delayed_work(&dev_priv->mm.idle_work); = return 0; -- = Ville Syrj=E4l=E4 Intel OTC