From mboxrd@z Thu Jan 1 00:00:00 1970 From: Deepak S Subject: Re: [PATCH] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated. Date: Mon, 16 Jun 2014 19:54:14 +0530 Message-ID: <539EFE0E.7060300@linux.intel.com> References: <1402654574-5287-1-git-send-email-deepak.s@linux.intel.com> <20140613113344.GX27580@intel.com> <20140613115728.GY27580@intel.com> <539AEE01.1040208@linux.intel.com> <20140613135428.GL5821@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id 888F76E374 for ; Mon, 16 Jun 2014 07:24:33 -0700 (PDT) In-Reply-To: <20140613135428.GL5821@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Friday 13 June 2014 07:24 PM, Daniel Vetter wrote: > On Fri, Jun 13, 2014 at 05:56:41PM +0530, Deepak S wrote: >> On Friday 13 June 2014 05:27 PM, Ville Syrj=E4l=E4 wrote: >>> 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 wro= te: >>>>> From: Deepak S >>>>> >>>>> Workaround fixed in BYT. Forcing Gfx clk up not needed, and Requestin= g 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/i= ntel_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 v= al) >>>>> 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= _priv) >>>>> 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_softlimi= t); >>>> This should take care of https://bugs.freedesktop.org/show_bug.cgi?id= =3D75244 >>>> >>>> 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 th= at >>>> we really need this on VLV to drop the voltage. >> hmm, Yes we might need this for other stepping. I will add a comment > Please don't put the stepping info in the comment or commit message > though, that freaks out people ;-) Usually we go with "pre-production" or > "early revisions" or something non-specific. Ok Sure :) >> Thanks for the review >> >>>> Also it now occurs to me that we might be leaving the GPU frequency (a= nd >>>> 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 befo= re >>>> 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/i91= 5_gem.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; >> Yes, while suspending we need move GPU to min_freq. flush_delayed_work >> should be fine. Let me create a patch for this. > Since this is gt powersave related can you please also check whether we > shouldn't move this to the intel_suspend_gt_powersave function Jesse > recently added to -nightly? > > Thanks, Daniel