| Message ID | 1390838706-10934-3-git-send-email-deepak.s@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 27, 2014 at 09:35:06PM +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 RPn, 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 RPn then move GFx down. > > v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel) > > v3: Fix the timeout during wait for gfx clock (Jesse) > > v4: addressed comments on set freq and punit wait (Ville) > > Signed-off-by: Deepak S <deepak.s@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 242f540..feaa83b 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4944,6 +4944,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 c6a07c9..84e20d0 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3035,6 +3035,56 @@ 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_delay <= dev_priv->rps.min_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 & Do we really need an atomic register busy loop here? Afaics this is only called from process context, so the normal wait_for macro should be good enough ... -Daniel > + I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) { > + DRM_ERROR("GFX_CLK_ON request timed out\n"); > + return; > + } > + > + vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay); > + > + if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) > + & GENFREQSTATUS) == 0, 5)) > + DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); > + > + /* 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 | > + GEN6_PM_RP_UP_EI_EXPIRED)); > +} > + > + > + > void gen6_rps_idle(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; > @@ -3042,7 +3092,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; > @@ -4273,6 +4323,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; > -- > 1.8.5.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jan 27, 2014 at 09:35:06PM +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 RPn, 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 RPn then move GFx down. > > v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel) > > v3: Fix the timeout during wait for gfx clock (Jesse) > > v4: addressed comments on set freq and punit wait (Ville) > > Signed-off-by: Deepak S <deepak.s@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 242f540..feaa83b 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4944,6 +4944,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 c6a07c9..84e20d0 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3035,6 +3035,56 @@ 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_delay <= dev_priv->rps.min_delay) > + return; If we're already at min freq I guess there's a good chance we're at the min voltage too. But I'm not sure that's really guaranteed by anything. Maybe it's enough. If not then I guess we should track whether we've already called this function w/o going to higher voltage in between. > + > + /* 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), 5)) { > + DRM_ERROR("GFX_CLK_ON request timed out\n"); > + return; > + } > + > + vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay); We should update cur_delay to reflect this. > + > + if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) > + & GENFREQSTATUS) == 0, 5)) > + DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); > + > + /* 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 | > + GEN6_PM_RP_UP_EI_EXPIRED)); Wouldn't that confuse the interrupt masking logic you just introduced in the previous patch? So looks to me like pretending we got a down threshold interrupt here is all that's needed to keep things in sync. So somehting like: gen6_set_pm_mask(dev_priv, GEN6_PM_RP_DOWN_THRESHOLD, min_delay); > +} > + > + > + > void gen6_rps_idle(struct drm_i915_private *dev_priv) > { > struct drm_device *dev = dev_priv->dev; > @@ -3042,7 +3092,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; > @@ -4273,6 +4323,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; > -- > 1.8.5.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 1/27/2014 10:37 PM, Ville Syrjälä wrote: > On Mon, Jan 27, 2014 at 09:35:06PM +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 RPn, 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 RPn then move GFx down. >> >> v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel) >> >> v3: Fix the timeout during wait for gfx clock (Jesse) >> >> v4: addressed comments on set freq and punit wait (Ville) >> >> Signed-off-by: Deepak S <deepak.s@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ >> drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 56 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 242f540..feaa83b 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -4944,6 +4944,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 c6a07c9..84e20d0 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -3035,6 +3035,56 @@ 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_delay <= dev_priv->rps.min_delay) >> + return; > > If we're already at min freq I guess there's a good chance we're at the > min voltage too. But I'm not sure that's really guaranteed by anything. > Maybe it's enough. If not then I guess we should track whether we've > already called this function w/o going to higher voltage in between. If we are already in min_freq we will just return right? Only if we have crossed the min_delay and if the gpu is idle we are setting is freq back to min. >> + >> + /* 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), 5)) { >> + DRM_ERROR("GFX_CLK_ON request timed out\n"); >> + return; >> + } >> + >> + vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay); > > We should update cur_delay to reflect this. I will fix this issue. >> + >> + if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) >> + & GENFREQSTATUS) == 0, 5)) >> + DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); >> + >> + /* 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 | >> + GEN6_PM_RP_UP_EI_EXPIRED)); > > Wouldn't that confuse the interrupt masking logic you just introduced > in the previous patch? > > So looks to me like pretending we got a down threshold interrupt here > is all that's needed to keep things in sync. So somehting like: > gen6_set_pm_mask(dev_priv, GEN6_PM_RP_DOWN_THRESHOLD, min_delay); > >> +} >> + >> + >> + >> void gen6_rps_idle(struct drm_i915_private *dev_priv) >> { >> struct drm_device *dev = dev_priv->dev; >> @@ -3042,7 +3092,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; >> @@ -4273,6 +4323,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; >> -- >> 1.8.5.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On 1/27/2014 10:22 PM, Daniel Vetter wrote: > On Mon, Jan 27, 2014 at 09:35:06PM +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 RPn, 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 RPn then move GFx down. >> >> v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel) >> >> v3: Fix the timeout during wait for gfx clock (Jesse) >> >> v4: addressed comments on set freq and punit wait (Ville) >> >> Signed-off-by: Deepak S <deepak.s@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ >> drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 56 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 242f540..feaa83b 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -4944,6 +4944,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 c6a07c9..84e20d0 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -3035,6 +3035,56 @@ 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_delay <= dev_priv->rps.min_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 & > > Do we really need an atomic register busy loop here? Afaics this is only > called from process context, so the normal wait_for macro should be good > enough ... > -Daniel I agree, the reason why i did the _atomic as we observed delay in scheduling the wait_for and we ended up spending lot of time here. I will wait for other comments, If i dont get any, I will address this comment and submit the patch. >> + I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) { >> + DRM_ERROR("GFX_CLK_ON request timed out\n"); >> + return; >> + } >> + >> + vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay); >> + >> + if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) >> + & GENFREQSTATUS) == 0, 5)) >> + DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); >> + >> + /* 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 | >> + GEN6_PM_RP_UP_EI_EXPIRED)); >> +} >> + >> + >> + >> void gen6_rps_idle(struct drm_i915_private *dev_priv) >> { >> struct drm_device *dev = dev_priv->dev; >> @@ -3042,7 +3092,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; >> @@ -4273,6 +4323,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; >> -- >> 1.8.5.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Tue, Jan 28, 2014 at 08:02:56PM +0530, S, Deepak wrote: > > > On 1/27/2014 10:22 PM, Daniel Vetter wrote: > >On Mon, Jan 27, 2014 at 09:35:06PM +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 RPn, 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 RPn then move GFx down. > >> > >>v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel) > >> > >>v3: Fix the timeout during wait for gfx clock (Jesse) > >> > >>v4: addressed comments on set freq and punit wait (Ville) > >> > >>Signed-off-by: Deepak S <deepak.s@intel.com> > >>--- > >> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > >> drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++- > >> 2 files changed, 56 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >>index 242f540..feaa83b 100644 > >>--- a/drivers/gpu/drm/i915/i915_reg.h > >>+++ b/drivers/gpu/drm/i915/i915_reg.h > >>@@ -4944,6 +4944,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 c6a07c9..84e20d0 100644 > >>--- a/drivers/gpu/drm/i915/intel_pm.c > >>+++ b/drivers/gpu/drm/i915/intel_pm.c > >>@@ -3035,6 +3035,56 @@ 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_delay <= dev_priv->rps.min_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 & > > > >Do we really need an atomic register busy loop here? Afaics this is only > >called from process context, so the normal wait_for macro should be good > >enough ... > >-Daniel > I agree, the reason why i did the _atomic as we observed delay in > scheduling the wait_for and we ended up spending lot of time here. > I will wait for other comments, If i dont get any, I will address > this comment and submit the patch. Now that we're using Chris' idle-clamping rps logic this would be run from a work queue. So I hope that the potential scheduling delays here won't affect things badly. If it does you can stick with the _atomic, but then it needs a comment. But I really hope that we only call this from worker threads, so shouldn't matter too badly if there's a bit a scheduler delay. -Daniel > > >>+ I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) { > >>+ DRM_ERROR("GFX_CLK_ON request timed out\n"); > >>+ return; > >>+ } > >>+ > >>+ vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay); > >>+ > >>+ if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) > >>+ & GENFREQSTATUS) == 0, 5)) > >>+ DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); > >>+ > >>+ /* 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 | > >>+ GEN6_PM_RP_UP_EI_EXPIRED)); > >>+} > >>+ > >>+ > >>+ > >> void gen6_rps_idle(struct drm_i915_private *dev_priv) > >> { > >> struct drm_device *dev = dev_priv->dev; > >>@@ -3042,7 +3092,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; > >>@@ -4273,6 +4323,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; > >>-- > >>1.8.5.2 > >> > >>_______________________________________________ > >>Intel-gfx mailing list > >>Intel-gfx@lists.freedesktop.org > >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On 1/29/2014 1:03 AM, Daniel Vetter wrote: > On Tue, Jan 28, 2014 at 08:02:56PM +0530, S, Deepak wrote: >> >> >> On 1/27/2014 10:22 PM, Daniel Vetter wrote: >>> On Mon, Jan 27, 2014 at 09:35:06PM +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 RPn, 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 RPn then move GFx down. >>>> >>>> v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel) >>>> >>>> v3: Fix the timeout during wait for gfx clock (Jesse) >>>> >>>> v4: addressed comments on set freq and punit wait (Ville) >>>> >>>> Signed-off-by: Deepak S <deepak.s@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ >>>> drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++- >>>> 2 files changed, 56 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>> index 242f540..feaa83b 100644 >>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>> @@ -4944,6 +4944,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 c6a07c9..84e20d0 100644 >>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>> @@ -3035,6 +3035,56 @@ 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_delay <= dev_priv->rps.min_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 & >>> >>> Do we really need an atomic register busy loop here? Afaics this is only >>> called from process context, so the normal wait_for macro should be good >>> enough ... >>> -Daniel >> I agree, the reason why i did the _atomic as we observed delay in >> scheduling the wait_for and we ended up spending lot of time here. >> I will wait for other comments, If i dont get any, I will address >> this comment and submit the patch. > > Now that we're using Chris' idle-clamping rps logic this would be run from > a work queue. So I hope that the potential scheduling delays here won't > affect things badly. If it does you can stick with the _atomic, but then > it needs a comment. > > But I really hope that we only call this from worker threads, so shouldn't > matter too badly if there's a bit a scheduler delay. > -Daniel I verified and we don't have a scheduling delays, I will change this to _atomic to wait_for -Deepak >> >>>> + I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) { >>>> + DRM_ERROR("GFX_CLK_ON request timed out\n"); >>>> + return; >>>> + } >>>> + >>>> + vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay); >>>> + >>>> + if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) >>>> + & GENFREQSTATUS) == 0, 5)) >>>> + DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); >>>> + >>>> + /* 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 | >>>> + GEN6_PM_RP_UP_EI_EXPIRED)); >>>> +} >>>> + >>>> + >>>> + >>>> void gen6_rps_idle(struct drm_i915_private *dev_priv) >>>> { >>>> struct drm_device *dev = dev_priv->dev; >>>> @@ -3042,7 +3092,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; >>>> @@ -4273,6 +4323,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; >>>> -- >>>> 1.8.5.2 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 242f540..feaa83b 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4944,6 +4944,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 c6a07c9..84e20d0 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3035,6 +3035,56 @@ 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_delay <= dev_priv->rps.min_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), 5)) { + DRM_ERROR("GFX_CLK_ON request timed out\n"); + return; + } + + vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay); + + if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) + & GENFREQSTATUS) == 0, 5)) + DRM_DEBUG_DRIVER("timed out waiting for Punit\n"); + + /* 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 | + GEN6_PM_RP_UP_EI_EXPIRED)); +} + + + void gen6_rps_idle(struct drm_i915_private *dev_priv) { struct drm_device *dev = dev_priv->dev; @@ -3042,7 +3092,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; @@ -4273,6 +4323,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;