| Message ID | 1389276070-22591-3-git-send-email-deepak.s@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 9 Jan 2014 19:31:09 +0530 deepak.s@intel.com wrote: > From: Deepak S <deepak.s@intel.com> > > When we enter RC6 and GFX Clocks are off, the voltage remains higher > than Vmin. When we try to set the freq to RPe, it might fail since the > Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up > and set the freq to RPe then move GFx down. > > v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel) > > Signed-off-by: Deepak S <deepak.s@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_pm.c | 48 ++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index a699efd..e37831f 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4940,6 +4940,10 @@ > GEN6_PM_RP_DOWN_THRESHOLD | \ > GEN6_PM_RP_DOWN_TIMEOUT) > > +#define VLV_GTLC_SURVIVABILITY_REG 0x130098 > +#define VLV_GFX_CLK_STATUS_BIT (1<<3) > +#define VLV_GFX_CLK_FORCE_ON_BIT (1<<2) > + > #define GEN6_GT_GFX_RC6_LOCKED 0x138104 > #define VLV_COUNTER_CONTROL 0x138104 > #define VLV_COUNT_RANGE_HIGH (1<<15) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 9c950e4..a8e05fe 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3050,6 +3050,51 @@ 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 Rpe if Gfx clocks are down > + * > + * * If Gfx is Idle, then > + * 1. Mask Turbo interrupts > + * 2. Bring up Gfx clock > + * 3. Change the freq to Rpe 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_delay == dev_priv->rps.rpe_delay) > + return; > + > + /* Mask turbo interrupt so that they will not come in between */ > + I915_WRITE(GEN6_PMINTRMSK, 0xffffffff); > + > + /* Bring up the Gfx clock */ > + I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, > + I915_READ(VLV_GTLC_SURVIVABILITY_REG) | > + VLV_GFX_CLK_FORCE_ON_BIT); > + > + if (wait_for_atomic(((VLV_GFX_CLK_STATUS_BIT & > + I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 500)) { > + DRM_ERROR("GFX_CLK_ON request timed out\n"); > + return; > + } > + > + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > + > + /* Release the Gfx clock */ > + I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, > + I915_READ(VLV_GTLC_SURVIVABILITY_REG) & > + ~VLV_GFX_CLK_FORCE_ON_BIT); > + > + /* Unmask Turbo interrupts */ > + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); > +} > + > + > + > void gen6_rps_idle(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; > @@ -3057,7 +3102,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)) > - valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay); > + vlv_set_rps_idle(dev_priv); > else > gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay); > dev_priv->rps.last_adj = 0; > @@ -4288,6 +4333,7 @@ void intel_gpu_ips_teardown(void) > i915_mch_dev = NULL; > spin_unlock_irq(&mchdev_lock); > } > + > static void intel_init_emon(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; Yeah if we need to bring the gfx clocks up (which makes sense) then I guess we need this. I'm not sure about the wait on the punit though; that could end up penalizing us in bursty workloads, since the punit can take quite some time to update the freq, but I don't actually see a wait here? Also, is the 500ms timeout really required for the gfx clock? That's a long time to potentially hold the mutex and delay any clock boosting or other activity...
>Yeah if we need to bring the gfx clocks up (which makes sense) then I guess we need this. I'm not sure about the wait on the punit though; that could end up penalizing us in bursty workloads, since the punit can take quite some time to update the freq, but I don't actually see a wait here? Ville suggested we don't need wait after requesting the freq. " AFAIK the punit will recheck the situation periodically, and it will try to use PUNIT_REG_GPU_FREQ_REQ." >Also, is the 500ms timeout really required for the gfx clock? That's a long time to potentially hold the mutex and delay any clock boosting or other activity... Yes agree, we don't need 500ms. I will change the delay to 5ms and submit a new command. Thanks Deepak -----Original Message----- From: Jesse Barnes [mailto:jbarnes@virtuousgeek.org] Sent: Tuesday, January 14, 2014 4:23 AM To: S, Deepak Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v2 2/3] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated. On Thu, 9 Jan 2014 19:31:09 +0530 deepak.s@intel.com wrote: > From: Deepak S <deepak.s@intel.com> > > When we enter RC6 and GFX Clocks are off, the voltage remains higher > than Vmin. When we try to set the freq to RPe, it might fail since the > Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock > up and set the freq to RPe then move GFx down. > > v2: remove vlv_update_rps_cur_delay function. Update commit message > (Daniel) > > Signed-off-by: Deepak S <deepak.s@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_pm.c | 48 > ++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > b/drivers/gpu/drm/i915/i915_reg.h index a699efd..e37831f 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4940,6 +4940,10 @@ > GEN6_PM_RP_DOWN_THRESHOLD | \ > GEN6_PM_RP_DOWN_TIMEOUT) > > +#define VLV_GTLC_SURVIVABILITY_REG 0x130098 > +#define VLV_GFX_CLK_STATUS_BIT (1<<3) > +#define VLV_GFX_CLK_FORCE_ON_BIT (1<<2) > + > #define GEN6_GT_GFX_RC6_LOCKED 0x138104 > #define VLV_COUNTER_CONTROL 0x138104 > #define VLV_COUNT_RANGE_HIGH (1<<15) > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c index 9c950e4..a8e05fe 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3050,6 +3050,51 @@ 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 Rpe if Gfx clocks are down > + * > + * * If Gfx is Idle, then > + * 1. Mask Turbo interrupts > + * 2. Bring up Gfx clock > + * 3. Change the freq to Rpe 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_delay == dev_priv->rps.rpe_delay) > + return; > + > + /* Mask turbo interrupt so that they will not come in between */ > + I915_WRITE(GEN6_PMINTRMSK, 0xffffffff); > + > + /* Bring up the Gfx clock */ > + I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, > + I915_READ(VLV_GTLC_SURVIVABILITY_REG) | > + VLV_GFX_CLK_FORCE_ON_BIT); > + > + if (wait_for_atomic(((VLV_GFX_CLK_STATUS_BIT & > + I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 500)) { > + DRM_ERROR("GFX_CLK_ON request timed out\n"); > + return; > + } > + > + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > + > + /* Release the Gfx clock */ > + I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, > + I915_READ(VLV_GTLC_SURVIVABILITY_REG) & > + ~VLV_GFX_CLK_FORCE_ON_BIT); > + > + /* Unmask Turbo interrupts */ > + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); } > + > + > + > void gen6_rps_idle(struct drm_i915_private *dev_priv) { > struct drm_device *dev = dev_priv->dev; @@ -3057,7 +3102,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)) > - valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay); > + vlv_set_rps_idle(dev_priv); > else > gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay); > dev_priv->rps.last_adj = 0; > @@ -4288,6 +4333,7 @@ void intel_gpu_ips_teardown(void) > i915_mch_dev = NULL; > spin_unlock_irq(&mchdev_lock); > } > + > static void intel_init_emon(struct drm_device *dev) { > struct drm_i915_private *dev_priv = dev->dev_private; Yeah if we need to bring the gfx clocks up (which makes sense) then I guess we need this. I'm not sure about the wait on the punit though; that could end up penalizing us in bursty workloads, since the punit can take quite some time to update the freq, but I don't actually see a wait here? Also, is the 500ms timeout really required for the gfx clock? That's a long time to potentially hold the mutex and delay any clock boosting or other activity... -- Jesse Barnes, Intel Open Source Technology Center On Sun, Dec 08, 2013 at 02:16:43PM +0530, deepak.s@intel.com wrote: > From: Deepak S <deepak.s@intel.com> > > on VLV, P-Unit doesn't garauntee that last requested freq by driver > is actually the current running frequency. We need to make sure we update > the cur freq. before requesitng new freq. > > Signed-off-by: Deepak S <deepak.s@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++ > drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++++ > 3 files changed, 40 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 780f815..a62ac0c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2416,6 +2416,7 @@ extern bool ironlake_set_drps(struct drm_device *dev, u8 val); > extern void intel_init_pch_refclk(struct drm_device *dev); > extern void gen6_set_rps(struct drm_device *dev, u8 val); > extern void valleyview_set_rps(struct drm_device *dev, u8 val); > +extern bool vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv); > extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv); > extern int valleyview_rps_min_freq(struct drm_i915_private *dev_priv); > extern void intel_detect_pch(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 2715600..4bde03a 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -982,6 +982,14 @@ static void gen6_pm_rps_work(struct work_struct *work) > > mutex_lock(&dev_priv->rps.hw_lock); > > + /* Make sure we have current freq updated properly. Doing this > + * here becuase, on VLV, P-Unit doesnt garauntee that last requested > + * freq by driver is actually the current running frequency > + */ > + > + if (IS_VALLEYVIEW(dev_priv->dev)) > + vlv_update_rps_cur_delay(dev_priv); > + > adj = dev_priv->rps.last_adj; > if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) { > if (adj > 0) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index e6d98fe..7f6c747 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3607,6 +3607,35 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv) > mutex_unlock(&dev_priv->rps.hw_lock); > } > > +/* > + * Wait until the previous freq change has completed, > + * or the timeout elapsed, and then update our notion > + * of the current GPU frequency. > + */ > +bool vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv) > +{ > + u32 pval; > + > + WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > + > + if (wait_for(((pval = vlv_punit_read(dev_priv, > + PUNIT_REG_GPU_FREQ_STS)) & > + GENFREQSTATUS) == 0, 10)) > + DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); > + > + pval >>= 8; > + > + if (pval != dev_priv->rps.cur_delay) > + DRM_DEBUG_DRIVER("Punit overrode GPU freq: %d MHz (%u) requested, but got %d Mhz (%u)\n", > + vlv_gpu_freq(dev_priv, dev_priv->rps.cur_delay), > + dev_priv->rps.cur_delay, > + vlv_gpu_freq(dev_priv, pval), pval); > + > + dev_priv->rps.cur_delay = pval; > + return true; > +} I just killed this guys a while ago. If you think we need to resurrect it, you should do it w/ git revert to make it clear where it came from. But I'd want more justification than what you have provided. My understanding is that PUNIT_REG_GPU_FREQ_STS alwasy reflects the current operating frequency of the GPU, and that can be affected by thermal conditions (and media turbo, which I'll ignore for simplicity) in addition to the frequency requested by the driver. AFAIK the punit will recheck the situation periodically, and it will try to use PUNIT_REG_GPU_FREQ_REQ. It will check the thermal conditions to figure out if it needs to further limit the frequency. Once the thermal conditions permit it, the frequency should return back to the last requested turbo frequency, without the driver having to rewrite PUNIT_REG_GPU_FREQ_REQ. If I'm right updating cur_delay based on PUNIT_REG_GPU_FREQ_STS is clearly the wrong thing to do. So I think we need more details on what the punit does in order to figure out what's the right thing to do here. > + > + > void valleyview_set_rps(struct drm_device *dev, u8 val) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -3615,6 +3644,8 @@ void valleyview_set_rps(struct drm_device *dev, u8 val) > WARN_ON(val > dev_priv->rps.max_delay); > WARN_ON(val < dev_priv->rps.min_delay); > > + vlv_update_rps_cur_delay(dev_priv); > + > DRM_DEBUG_DRIVER("GPU freq request from %d MHz (%u) to %d MHz (%u)\n", > vlv_gpu_freq(dev_priv, dev_priv->rps.cur_delay), > dev_priv->rps.cur_delay, > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index a699efd..e37831f 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4940,6 +4940,10 @@ GEN6_PM_RP_DOWN_THRESHOLD | \ GEN6_PM_RP_DOWN_TIMEOUT) +#define VLV_GTLC_SURVIVABILITY_REG 0x130098 +#define VLV_GFX_CLK_STATUS_BIT (1<<3) +#define VLV_GFX_CLK_FORCE_ON_BIT (1<<2) + #define GEN6_GT_GFX_RC6_LOCKED 0x138104 #define VLV_COUNTER_CONTROL 0x138104 #define VLV_COUNT_RANGE_HIGH (1<<15) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 9c950e4..a8e05fe 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3050,6 +3050,51 @@ 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 Rpe if Gfx clocks are down + * + * * If Gfx is Idle, then + * 1. Mask Turbo interrupts + * 2. Bring up Gfx clock + * 3. Change the freq to Rpe 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_delay == dev_priv->rps.rpe_delay) + return; + + /* Mask turbo interrupt so that they will not come in between */ + I915_WRITE(GEN6_PMINTRMSK, 0xffffffff); + + /* Bring up the Gfx clock */ + I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, + I915_READ(VLV_GTLC_SURVIVABILITY_REG) | + VLV_GFX_CLK_FORCE_ON_BIT); + + if (wait_for_atomic(((VLV_GFX_CLK_STATUS_BIT & + I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 500)) { + DRM_ERROR("GFX_CLK_ON request timed out\n"); + return; + } + + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); + + /* Release the Gfx clock */ + I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, + I915_READ(VLV_GTLC_SURVIVABILITY_REG) & + ~VLV_GFX_CLK_FORCE_ON_BIT); + + /* Unmask Turbo interrupts */ + I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS); +} + + + void gen6_rps_idle(struct drm_i915_private *dev_priv) { struct drm_device *dev = dev_priv->dev; @@ -3057,7 +3102,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)) - valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay); + vlv_set_rps_idle(dev_priv); else gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay); dev_priv->rps.last_adj = 0; @@ -4288,6 +4333,7 @@ void intel_gpu_ips_teardown(void) i915_mch_dev = NULL; spin_unlock_irq(&mchdev_lock); } + static void intel_init_emon(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;