* [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 @ 2013-04-19 17:22 Jesse Barnes 2013-04-22 11:39 ` Ville Syrjälä 2013-04-22 17:54 ` [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v2 Jesse Barnes 0 siblings, 2 replies; 11+ messages in thread From: Jesse Barnes @ 2013-04-19 17:22 UTC (permalink / raw) To: intel-gfx On VLV, the Punit doesn't automatically drop the GPU to it's minimum voltage level when entering RC6, so we arm a timer to do it for us from the RPS interrupt handler. It'll generally only fire when we go idle (or if for some reason there's a long delay between RPS interrupts), but won't be re-armed again until the next RPS event, so shouldn't affect power consumption after we go idle and it triggers. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_drv.h | 4 ++++ drivers/gpu/drm/i915/i915_irq.c | 11 +++++++++++ drivers/gpu/drm/i915/intel_pm.c | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2557fc7..2900dd4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -660,6 +660,7 @@ struct i915_suspend_saved_registers { struct intel_gen6_power_mgmt { struct work_struct work; + struct work_struct vlv_work; u32 pm_iir; /* lock - irqsave spinlock that protectects the work_struct and * pm_iir. */ @@ -670,6 +671,7 @@ struct intel_gen6_power_mgmt { u8 cur_delay; u8 min_delay; u8 max_delay; + u8 rpe_delay; u8 hw_max; struct delayed_work delayed_resume_work; @@ -945,6 +947,8 @@ typedef struct drm_i915_private { } hpd_stats[HPD_NUM_PINS]; struct timer_list hotplug_reenable_timer; + struct timer_list vlv_rps_timer; + int num_pch_pll; int num_plane; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 932e7f8..99b2e1c 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -488,6 +488,17 @@ static void gen6_pm_rps_work(struct work_struct *work) gen6_set_rps(dev_priv->dev, new_delay); } + if (IS_VALLEYVIEW(dev_priv->dev)) { + /* + * On VLV, when we enter RC6 we may not be at the minimum + * voltage level, so arm a timer to check. It should only + * fire when there's activity or once after we've entered + * RC6, and then won't be re-armed until the next RPS interrupt. + */ + mod_timer(&dev_priv->vlv_rps_timer, jiffies + + msecs_to_jiffies(100)); + } + mutex_unlock(&dev_priv->rps.hw_lock); } diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2557926..c1311c9 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2545,6 +2545,9 @@ static void gen6_disable_rps(struct drm_device *dev) spin_unlock_irq(&dev_priv->rps.lock); I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); + + if (IS_VALLEYVIEW(dev)) + del_timer_sync(&dev_priv->vlv_rps_timer); } int intel_enable_rc6(const struct drm_device *dev) @@ -2822,6 +2825,30 @@ int valleyview_rps_min_freq(struct drm_i915_private *dev_priv) return val & 0xff; } +static void vlv_rps_timer_work(struct work_struct *work) +{ + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, + rps.vlv_work); + + /* + * Timer fired, we must be idle. Drop to min voltage state. + * Note: we use RPe here since it should match the + * Vmin we were shooting for. That should give us better + * perf when we come back out of RC6 than if we used the + * min freq available. + */ + mutex_lock(&dev_priv->rps.hw_lock); + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); + mutex_unlock(&dev_priv->rps.hw_lock); +} + +static void valleyview_rps_timer_func(unsigned long data) +{ + struct drm_i915_private *dev_priv = (struct drm_i915_private *)data; + + queue_work(dev_priv->wq, &dev_priv->rps.vlv_work); +} + static void valleyview_enable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -2886,6 +2913,7 @@ static void valleyview_enable_rps(struct drm_device *dev) rpe = valleyview_rps_rpe_freq(dev_priv); DRM_DEBUG_DRIVER("RPe GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, rpe)); + dev_priv->rps.rpe_delay = rpe; val = valleyview_rps_min_freq(dev_priv); DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, @@ -2895,6 +2923,10 @@ static void valleyview_enable_rps(struct drm_device *dev) DRM_DEBUG_DRIVER("setting GPU freq to %d\n", vlv_gpu_freq(dev_priv->mem_freq, rpe)); + INIT_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work); + setup_timer(&dev_priv->vlv_rps_timer, valleyview_rps_timer_func, + (unsigned long)dev_priv); + valleyview_set_rps(dev_priv->dev, rpe); /* requires MSI enabled */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 2013-04-19 17:22 [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 Jesse Barnes @ 2013-04-22 11:39 ` Ville Syrjälä 2013-04-22 14:46 ` Jesse Barnes 2013-04-22 17:54 ` [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v2 Jesse Barnes 1 sibling, 1 reply; 11+ messages in thread From: Ville Syrjälä @ 2013-04-22 11:39 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Fri, Apr 19, 2013 at 10:22:45AM -0700, Jesse Barnes wrote: > On VLV, the Punit doesn't automatically drop the GPU to it's minimum > voltage level when entering RC6, so we arm a timer to do it for us from > the RPS interrupt handler. It'll generally only fire when we go idle > (or if for some reason there's a long delay between RPS interrupts), but > won't be re-armed again until the next RPS event, so shouldn't affect > power consumption after we go idle and it triggers. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ++++ > drivers/gpu/drm/i915/i915_irq.c | 11 +++++++++++ > drivers/gpu/drm/i915/intel_pm.c | 32 ++++++++++++++++++++++++++++++++ > 3 files changed, 47 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2557fc7..2900dd4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -660,6 +660,7 @@ struct i915_suspend_saved_registers { > > struct intel_gen6_power_mgmt { > struct work_struct work; > + struct work_struct vlv_work; > u32 pm_iir; > /* lock - irqsave spinlock that protectects the work_struct and > * pm_iir. */ > @@ -670,6 +671,7 @@ struct intel_gen6_power_mgmt { > u8 cur_delay; > u8 min_delay; > u8 max_delay; > + u8 rpe_delay; > u8 hw_max; > > struct delayed_work delayed_resume_work; > @@ -945,6 +947,8 @@ typedef struct drm_i915_private { > } hpd_stats[HPD_NUM_PINS]; > struct timer_list hotplug_reenable_timer; > > + struct timer_list vlv_rps_timer; > + > int num_pch_pll; > int num_plane; > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 932e7f8..99b2e1c 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -488,6 +488,17 @@ static void gen6_pm_rps_work(struct work_struct *work) > gen6_set_rps(dev_priv->dev, new_delay); > } > > + if (IS_VALLEYVIEW(dev_priv->dev)) { > + /* > + * On VLV, when we enter RC6 we may not be at the minimum > + * voltage level, so arm a timer to check. It should only > + * fire when there's activity or once after we've entered > + * RC6, and then won't be re-armed until the next RPS interrupt. > + */ > + mod_timer(&dev_priv->vlv_rps_timer, jiffies + > + msecs_to_jiffies(100)); > + } > + > mutex_unlock(&dev_priv->rps.hw_lock); > } > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2557926..c1311c9 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2545,6 +2545,9 @@ static void gen6_disable_rps(struct drm_device *dev) > spin_unlock_irq(&dev_priv->rps.lock); > > I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > + > + if (IS_VALLEYVIEW(dev)) > + del_timer_sync(&dev_priv->vlv_rps_timer); > } > > int intel_enable_rc6(const struct drm_device *dev) > @@ -2822,6 +2825,30 @@ int valleyview_rps_min_freq(struct drm_i915_private *dev_priv) > return val & 0xff; > } > > +static void vlv_rps_timer_work(struct work_struct *work) > +{ > + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, > + rps.vlv_work); > + > + /* > + * Timer fired, we must be idle. Drop to min voltage state. > + * Note: we use RPe here since it should match the > + * Vmin we were shooting for. That should give us better > + * perf when we come back out of RC6 than if we used the > + * min freq available. > + */ > + mutex_lock(&dev_priv->rps.hw_lock); > + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > + mutex_unlock(&dev_priv->rps.hw_lock); > +} > + > +static void valleyview_rps_timer_func(unsigned long data) > +{ > + struct drm_i915_private *dev_priv = (struct drm_i915_private *)data; > + > + queue_work(dev_priv->wq, &dev_priv->rps.vlv_work); > +} This looks somewhat like open coded delayed work to me. Would the standard delayed work be OK for this, or was there some specific reason you did it this way? > + > static void valleyview_enable_rps(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -2886,6 +2913,7 @@ static void valleyview_enable_rps(struct drm_device *dev) > rpe = valleyview_rps_rpe_freq(dev_priv); > DRM_DEBUG_DRIVER("RPe GPU freq: %d\n", > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > + dev_priv->rps.rpe_delay = rpe; > > val = valleyview_rps_min_freq(dev_priv); > DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, > @@ -2895,6 +2923,10 @@ static void valleyview_enable_rps(struct drm_device *dev) > DRM_DEBUG_DRIVER("setting GPU freq to %d\n", > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > > + INIT_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work); > + setup_timer(&dev_priv->vlv_rps_timer, valleyview_rps_timer_func, > + (unsigned long)dev_priv); > + > valleyview_set_rps(dev_priv->dev, rpe); > > /* requires MSI enabled */ > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 2013-04-22 11:39 ` Ville Syrjälä @ 2013-04-22 14:46 ` Jesse Barnes 0 siblings, 0 replies; 11+ messages in thread From: Jesse Barnes @ 2013-04-22 14:46 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Mon, 22 Apr 2013 14:39:17 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Apr 19, 2013 at 10:22:45AM -0700, Jesse Barnes wrote: > > On VLV, the Punit doesn't automatically drop the GPU to it's minimum > > voltage level when entering RC6, so we arm a timer to do it for us from > > the RPS interrupt handler. It'll generally only fire when we go idle > > (or if for some reason there's a long delay between RPS interrupts), but > > won't be re-armed again until the next RPS event, so shouldn't affect > > power consumption after we go idle and it triggers. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 4 ++++ > > drivers/gpu/drm/i915/i915_irq.c | 11 +++++++++++ > > drivers/gpu/drm/i915/intel_pm.c | 32 ++++++++++++++++++++++++++++++++ > > 3 files changed, 47 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 2557fc7..2900dd4 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -660,6 +660,7 @@ struct i915_suspend_saved_registers { > > > > struct intel_gen6_power_mgmt { > > struct work_struct work; > > + struct work_struct vlv_work; > > u32 pm_iir; > > /* lock - irqsave spinlock that protectects the work_struct and > > * pm_iir. */ > > @@ -670,6 +671,7 @@ struct intel_gen6_power_mgmt { > > u8 cur_delay; > > u8 min_delay; > > u8 max_delay; > > + u8 rpe_delay; > > u8 hw_max; > > > > struct delayed_work delayed_resume_work; > > @@ -945,6 +947,8 @@ typedef struct drm_i915_private { > > } hpd_stats[HPD_NUM_PINS]; > > struct timer_list hotplug_reenable_timer; > > > > + struct timer_list vlv_rps_timer; > > + > > int num_pch_pll; > > int num_plane; > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 932e7f8..99b2e1c 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -488,6 +488,17 @@ static void gen6_pm_rps_work(struct work_struct *work) > > gen6_set_rps(dev_priv->dev, new_delay); > > } > > > > + if (IS_VALLEYVIEW(dev_priv->dev)) { > > + /* > > + * On VLV, when we enter RC6 we may not be at the minimum > > + * voltage level, so arm a timer to check. It should only > > + * fire when there's activity or once after we've entered > > + * RC6, and then won't be re-armed until the next RPS interrupt. > > + */ > > + mod_timer(&dev_priv->vlv_rps_timer, jiffies + > > + msecs_to_jiffies(100)); > > + } > > + > > mutex_unlock(&dev_priv->rps.hw_lock); > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 2557926..c1311c9 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -2545,6 +2545,9 @@ static void gen6_disable_rps(struct drm_device *dev) > > spin_unlock_irq(&dev_priv->rps.lock); > > > > I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > > + > > + if (IS_VALLEYVIEW(dev)) > > + del_timer_sync(&dev_priv->vlv_rps_timer); > > } > > > > int intel_enable_rc6(const struct drm_device *dev) > > @@ -2822,6 +2825,30 @@ int valleyview_rps_min_freq(struct drm_i915_private *dev_priv) > > return val & 0xff; > > } > > > > +static void vlv_rps_timer_work(struct work_struct *work) > > +{ > > + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, > > + rps.vlv_work); > > + > > + /* > > + * Timer fired, we must be idle. Drop to min voltage state. > > + * Note: we use RPe here since it should match the > > + * Vmin we were shooting for. That should give us better > > + * perf when we come back out of RC6 than if we used the > > + * min freq available. > > + */ > > + mutex_lock(&dev_priv->rps.hw_lock); > > + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > > + mutex_unlock(&dev_priv->rps.hw_lock); > > +} > > + > > +static void valleyview_rps_timer_func(unsigned long data) > > +{ > > + struct drm_i915_private *dev_priv = (struct drm_i915_private *)data; > > + > > + queue_work(dev_priv->wq, &dev_priv->rps.vlv_work); > > +} > > This looks somewhat like open coded delayed work to me. Would the > standard delayed work be OK for this, or was there some specific reason > you did it this way? No particular reason other than not thinking about delayed work when I wrote it. :) I can fix it up; I guess it'll save us having the rps_timer_func. -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v2 2013-04-19 17:22 [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 Jesse Barnes 2013-04-22 11:39 ` Ville Syrjälä @ 2013-04-22 17:54 ` Jesse Barnes 2013-04-22 18:52 ` Daniel Vetter 1 sibling, 1 reply; 11+ messages in thread From: Jesse Barnes @ 2013-04-22 17:54 UTC (permalink / raw) To: intel-gfx On VLV, the Punit doesn't automatically drop the GPU to it's minimum voltage level when entering RC6, so we arm a timer to do it for us from the RPS interrupt handler. It'll generally only fire when we go idle (or if for some reason there's a long delay between RPS interrupts), but won't be re-armed again until the next RPS event, so shouldn't affect power consumption after we go idle and it triggers. v2: use delayed work instead of timer + work queue combo (Ville) Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_irq.c | 11 +++++++++++ drivers/gpu/drm/i915/intel_pm.c | 23 +++++++++++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2557fc7..f563f25 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -660,6 +660,7 @@ struct i915_suspend_saved_registers { struct intel_gen6_power_mgmt { struct work_struct work; + struct delayed_work vlv_work; u32 pm_iir; /* lock - irqsave spinlock that protectects the work_struct and * pm_iir. */ @@ -670,6 +671,7 @@ struct intel_gen6_power_mgmt { u8 cur_delay; u8 min_delay; u8 max_delay; + u8 rpe_delay; u8 hw_max; struct delayed_work delayed_resume_work; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 932e7f8..af385e2 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -488,6 +488,17 @@ static void gen6_pm_rps_work(struct work_struct *work) gen6_set_rps(dev_priv->dev, new_delay); } + if (IS_VALLEYVIEW(dev_priv->dev)) { + /* + * On VLV, when we enter RC6 we may not be at the minimum + * voltage level, so arm a timer to check. It should only + * fire when there's activity or once after we've entered + * RC6, and then won't be re-armed until the next RPS interrupt. + */ + mod_delayed_work(dev_priv->wq, &dev_priv->rps.vlv_work, + jiffies + msecs_to_jiffies(100)); + } + mutex_unlock(&dev_priv->rps.hw_lock); } diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2557926..4669d8c 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2545,6 +2545,9 @@ static void gen6_disable_rps(struct drm_device *dev) spin_unlock_irq(&dev_priv->rps.lock); I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); + + if (IS_VALLEYVIEW(dev)) + cancel_delayed_work_sync(&dev_priv->rps.vlv_work); } int intel_enable_rc6(const struct drm_device *dev) @@ -2822,6 +2825,23 @@ int valleyview_rps_min_freq(struct drm_i915_private *dev_priv) return val & 0xff; } +static void vlv_rps_timer_work(struct delayed_work *work) +{ + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, + rps.vlv_work); + + /* + * Timer fired, we must be idle. Drop to min voltage state. + * Note: we use RPe here since it should match the + * Vmin we were shooting for. That should give us better + * perf when we come back out of RC6 than if we used the + * min freq available. + */ + mutex_lock(&dev_priv->rps.hw_lock); + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); + mutex_unlock(&dev_priv->rps.hw_lock); +} + static void valleyview_enable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -2886,6 +2906,7 @@ static void valleyview_enable_rps(struct drm_device *dev) rpe = valleyview_rps_rpe_freq(dev_priv); DRM_DEBUG_DRIVER("RPe GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, rpe)); + dev_priv->rps.rpe_delay = rpe; val = valleyview_rps_min_freq(dev_priv); DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, @@ -2895,6 +2916,8 @@ static void valleyview_enable_rps(struct drm_device *dev) DRM_DEBUG_DRIVER("setting GPU freq to %d\n", vlv_gpu_freq(dev_priv->mem_freq, rpe)); + INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work); + valleyview_set_rps(dev_priv->dev, rpe); /* requires MSI enabled */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v2 2013-04-22 17:54 ` [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v2 Jesse Barnes @ 2013-04-22 18:52 ` Daniel Vetter 2013-04-22 23:05 ` [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v3 Jesse Barnes 0 siblings, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2013-04-22 18:52 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx On Mon, Apr 22, 2013 at 7:54 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2557926..4669d8c 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2545,6 +2545,9 @@ static void gen6_disable_rps(struct drm_device *dev) > spin_unlock_irq(&dev_priv->rps.lock); > > I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); > + > + if (IS_VALLEYVIEW(dev)) > + cancel_delayed_work_sync(&dev_priv->rps.vlv_work); We hold the rps mutex here, so this can deadlock. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v3 2013-04-22 18:52 ` Daniel Vetter @ 2013-04-22 23:05 ` Jesse Barnes 2013-04-23 8:45 ` Jani Nikula 0 siblings, 1 reply; 11+ messages in thread From: Jesse Barnes @ 2013-04-22 23:05 UTC (permalink / raw) To: intel-gfx On VLV, the Punit doesn't automatically drop the GPU to it's minimum voltage level when entering RC6, so we arm a timer to do it for us from the RPS interrupt handler. It'll generally only fire when we go idle (or if for some reason there's a long delay between RPS interrupts), but won't be re-armed again until the next RPS event, so shouldn't affect power consumption after we go idle and it triggers. v2: use delayed work instead of timer + work queue combo (Ville) v3: fix up delayed work cancel (must be outside lock) (Daniel) fix up delayed work handling func for delayed work (Jesse) Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_irq.c | 11 +++++++++++ drivers/gpu/drm/i915/intel_pm.c | 22 ++++++++++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2557fc7..f563f25 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -660,6 +660,7 @@ struct i915_suspend_saved_registers { struct intel_gen6_power_mgmt { struct work_struct work; + struct delayed_work vlv_work; u32 pm_iir; /* lock - irqsave spinlock that protectects the work_struct and * pm_iir. */ @@ -670,6 +671,7 @@ struct intel_gen6_power_mgmt { u8 cur_delay; u8 min_delay; u8 max_delay; + u8 rpe_delay; u8 hw_max; struct delayed_work delayed_resume_work; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 932e7f8..af385e2 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -488,6 +488,17 @@ static void gen6_pm_rps_work(struct work_struct *work) gen6_set_rps(dev_priv->dev, new_delay); } + if (IS_VALLEYVIEW(dev_priv->dev)) { + /* + * On VLV, when we enter RC6 we may not be at the minimum + * voltage level, so arm a timer to check. It should only + * fire when there's activity or once after we've entered + * RC6, and then won't be re-armed until the next RPS interrupt. + */ + mod_delayed_work(dev_priv->wq, &dev_priv->rps.vlv_work, + jiffies + msecs_to_jiffies(100)); + } + mutex_unlock(&dev_priv->rps.hw_lock); } diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2557926..c106187 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2822,6 +2822,23 @@ int valleyview_rps_min_freq(struct drm_i915_private *dev_priv) return val & 0xff; } +static void vlv_rps_timer_work(struct work_struct *work) +{ + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, + rps.vlv_work.work); + + /* + * Timer fired, we must be idle. Drop to min voltage state. + * Note: we use RPe here since it should match the + * Vmin we were shooting for. That should give us better + * perf when we come back out of RC6 than if we used the + * min freq available. + */ + mutex_lock(&dev_priv->rps.hw_lock); + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); + mutex_unlock(&dev_priv->rps.hw_lock); +} + static void valleyview_enable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -2886,6 +2903,7 @@ static void valleyview_enable_rps(struct drm_device *dev) rpe = valleyview_rps_rpe_freq(dev_priv); DRM_DEBUG_DRIVER("RPe GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, rpe)); + dev_priv->rps.rpe_delay = rpe; val = valleyview_rps_min_freq(dev_priv); DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, @@ -2895,6 +2913,8 @@ static void valleyview_enable_rps(struct drm_device *dev) DRM_DEBUG_DRIVER("setting GPU freq to %d\n", vlv_gpu_freq(dev_priv->mem_freq, rpe)); + INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work); + valleyview_set_rps(dev_priv->dev, rpe); /* requires MSI enabled */ @@ -3640,6 +3660,8 @@ void intel_disable_gt_powersave(struct drm_device *dev) mutex_lock(&dev_priv->rps.hw_lock); gen6_disable_rps(dev); mutex_unlock(&dev_priv->rps.hw_lock); + if (IS_VALLEYVIEW(dev)) + cancel_delayed_work_sync(&dev_priv->rps.vlv_work); } } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v3 2013-04-22 23:05 ` [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v3 Jesse Barnes @ 2013-04-23 8:45 ` Jani Nikula 2013-04-23 13:28 ` Ville Syrjälä 0 siblings, 1 reply; 11+ messages in thread From: Jani Nikula @ 2013-04-23 8:45 UTC (permalink / raw) To: Jesse Barnes, intel-gfx On Tue, 23 Apr 2013, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On VLV, the Punit doesn't automatically drop the GPU to it's minimum > voltage level when entering RC6, so we arm a timer to do it for us from > the RPS interrupt handler. It'll generally only fire when we go idle > (or if for some reason there's a long delay between RPS interrupts), but > won't be re-armed again until the next RPS event, so shouldn't affect > power consumption after we go idle and it triggers. > > v2: use delayed work instead of timer + work queue combo (Ville) > v3: fix up delayed work cancel (must be outside lock) (Daniel) > fix up delayed work handling func for delayed work (Jesse) > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/i915_irq.c | 11 +++++++++++ > drivers/gpu/drm/i915/intel_pm.c | 22 ++++++++++++++++++++++ > 3 files changed, 35 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2557fc7..f563f25 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -660,6 +660,7 @@ struct i915_suspend_saved_registers { > > struct intel_gen6_power_mgmt { > struct work_struct work; > + struct delayed_work vlv_work; > u32 pm_iir; > /* lock - irqsave spinlock that protectects the work_struct and > * pm_iir. */ > @@ -670,6 +671,7 @@ struct intel_gen6_power_mgmt { > u8 cur_delay; > u8 min_delay; > u8 max_delay; > + u8 rpe_delay; > u8 hw_max; > > struct delayed_work delayed_resume_work; > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 932e7f8..af385e2 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -488,6 +488,17 @@ static void gen6_pm_rps_work(struct work_struct *work) > gen6_set_rps(dev_priv->dev, new_delay); > } > > + if (IS_VALLEYVIEW(dev_priv->dev)) { > + /* > + * On VLV, when we enter RC6 we may not be at the minimum > + * voltage level, so arm a timer to check. It should only > + * fire when there's activity or once after we've entered > + * RC6, and then won't be re-armed until the next RPS interrupt. > + */ > + mod_delayed_work(dev_priv->wq, &dev_priv->rps.vlv_work, > + jiffies + msecs_to_jiffies(100)); Hooray for conflicting semantics. Unlike mod_timer, mod_delayed_work wants a delay, not an absolute time in jiffies. > + } > + > mutex_unlock(&dev_priv->rps.hw_lock); > } > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2557926..c106187 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2822,6 +2822,23 @@ int valleyview_rps_min_freq(struct drm_i915_private *dev_priv) > return val & 0xff; > } > > +static void vlv_rps_timer_work(struct work_struct *work) > +{ > + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, > + rps.vlv_work.work); > + > + /* > + * Timer fired, we must be idle. Drop to min voltage state. > + * Note: we use RPe here since it should match the > + * Vmin we were shooting for. That should give us better > + * perf when we come back out of RC6 than if we used the > + * min freq available. > + */ > + mutex_lock(&dev_priv->rps.hw_lock); > + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > + mutex_unlock(&dev_priv->rps.hw_lock); > +} > + > static void valleyview_enable_rps(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -2886,6 +2903,7 @@ static void valleyview_enable_rps(struct drm_device *dev) > rpe = valleyview_rps_rpe_freq(dev_priv); > DRM_DEBUG_DRIVER("RPe GPU freq: %d\n", > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > + dev_priv->rps.rpe_delay = rpe; > > val = valleyview_rps_min_freq(dev_priv); > DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, > @@ -2895,6 +2913,8 @@ static void valleyview_enable_rps(struct drm_device *dev) > DRM_DEBUG_DRIVER("setting GPU freq to %d\n", > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > > + INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work); > + > valleyview_set_rps(dev_priv->dev, rpe); > > /* requires MSI enabled */ > @@ -3640,6 +3660,8 @@ void intel_disable_gt_powersave(struct drm_device *dev) > mutex_lock(&dev_priv->rps.hw_lock); > gen6_disable_rps(dev); > mutex_unlock(&dev_priv->rps.hw_lock); Theoretically the work could get executed here, between unlock and cancel. Does that matter? > + if (IS_VALLEYVIEW(dev)) > + cancel_delayed_work_sync(&dev_priv->rps.vlv_work); > } > } > > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v3 2013-04-23 8:45 ` Jani Nikula @ 2013-04-23 13:28 ` Ville Syrjälä 2013-04-23 14:46 ` Daniel Vetter ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Ville Syrjälä @ 2013-04-23 13:28 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx On Tue, Apr 23, 2013 at 11:45:03AM +0300, Jani Nikula wrote: > On Tue, 23 Apr 2013, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On VLV, the Punit doesn't automatically drop the GPU to it's minimum > > voltage level when entering RC6, so we arm a timer to do it for us from > > the RPS interrupt handler. It'll generally only fire when we go idle > > (or if for some reason there's a long delay between RPS interrupts), but > > won't be re-armed again until the next RPS event, so shouldn't affect > > power consumption after we go idle and it triggers. > > > > v2: use delayed work instead of timer + work queue combo (Ville) > > v3: fix up delayed work cancel (must be outside lock) (Daniel) > > fix up delayed work handling func for delayed work (Jesse) > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > drivers/gpu/drm/i915/i915_irq.c | 11 +++++++++++ > > drivers/gpu/drm/i915/intel_pm.c | 22 ++++++++++++++++++++++ > > 3 files changed, 35 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 2557fc7..f563f25 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -660,6 +660,7 @@ struct i915_suspend_saved_registers { > > > > struct intel_gen6_power_mgmt { > > struct work_struct work; > > + struct delayed_work vlv_work; > > u32 pm_iir; > > /* lock - irqsave spinlock that protectects the work_struct and > > * pm_iir. */ > > @@ -670,6 +671,7 @@ struct intel_gen6_power_mgmt { > > u8 cur_delay; > > u8 min_delay; > > u8 max_delay; > > + u8 rpe_delay; > > u8 hw_max; > > > > struct delayed_work delayed_resume_work; > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 932e7f8..af385e2 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -488,6 +488,17 @@ static void gen6_pm_rps_work(struct work_struct *work) > > gen6_set_rps(dev_priv->dev, new_delay); > > } > > > > + if (IS_VALLEYVIEW(dev_priv->dev)) { > > + /* > > + * On VLV, when we enter RC6 we may not be at the minimum > > + * voltage level, so arm a timer to check. It should only > > + * fire when there's activity or once after we've entered > > + * RC6, and then won't be re-armed until the next RPS interrupt. > > + */ > > + mod_delayed_work(dev_priv->wq, &dev_priv->rps.vlv_work, > > + jiffies + msecs_to_jiffies(100)); > > Hooray for conflicting semantics. Unlike mod_timer, mod_delayed_work > wants a delay, not an absolute time in jiffies. > > > + } > > + > > mutex_unlock(&dev_priv->rps.hw_lock); > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 2557926..c106187 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -2822,6 +2822,23 @@ int valleyview_rps_min_freq(struct drm_i915_private *dev_priv) > > return val & 0xff; > > } > > > > +static void vlv_rps_timer_work(struct work_struct *work) > > +{ > > + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, > > + rps.vlv_work.work); > > + > > + /* > > + * Timer fired, we must be idle. Drop to min voltage state. > > + * Note: we use RPe here since it should match the > > + * Vmin we were shooting for. That should give us better > > + * perf when we come back out of RC6 than if we used the > > + * min freq available. > > + */ > > + mutex_lock(&dev_priv->rps.hw_lock); > > + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > > + mutex_unlock(&dev_priv->rps.hw_lock); > > +} > > + > > static void valleyview_enable_rps(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -2886,6 +2903,7 @@ static void valleyview_enable_rps(struct drm_device *dev) > > rpe = valleyview_rps_rpe_freq(dev_priv); > > DRM_DEBUG_DRIVER("RPe GPU freq: %d\n", > > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > > + dev_priv->rps.rpe_delay = rpe; > > > > val = valleyview_rps_min_freq(dev_priv); > > DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, > > @@ -2895,6 +2913,8 @@ static void valleyview_enable_rps(struct drm_device *dev) > > DRM_DEBUG_DRIVER("setting GPU freq to %d\n", > > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > > > > + INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work); > > + > > valleyview_set_rps(dev_priv->dev, rpe); > > > > /* requires MSI enabled */ > > @@ -3640,6 +3660,8 @@ void intel_disable_gt_powersave(struct drm_device *dev) > > mutex_lock(&dev_priv->rps.hw_lock); > > gen6_disable_rps(dev); > > mutex_unlock(&dev_priv->rps.hw_lock); > > Theoretically the work could get executed here, between unlock and > cancel. Does that matter? AFAICS even gen6_pm_rps_work() might be executing even after gen6_disable_rps() was called. I'm not quite sure if allowing gen6_pm_rps_work() to do stuff after gen6_disable_rps() was called is already a problem on it's own. At least it can overwrite RPNSWREQ on !VLV. BTW that register doesn't seem to exist on VLV and yet we're poking at it from gen6_disable_rps()... > > > + if (IS_VALLEYVIEW(dev)) > > + cancel_delayed_work_sync(&dev_priv->rps.vlv_work); > > } > > } > > > > -- > > 1.7.10.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v3 2013-04-23 13:28 ` Ville Syrjälä @ 2013-04-23 14:46 ` Daniel Vetter 2013-04-23 16:36 ` Jesse Barnes 2013-04-23 17:10 ` Jesse Barnes 2 siblings, 0 replies; 11+ messages in thread From: Daniel Vetter @ 2013-04-23 14:46 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Tue, Apr 23, 2013 at 04:28:17PM +0300, Ville Syrjälä wrote: > On Tue, Apr 23, 2013 at 11:45:03AM +0300, Jani Nikula wrote: > > On Tue, 23 Apr 2013, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > @@ -2895,6 +2913,8 @@ static void valleyview_enable_rps(struct drm_device *dev) > > > DRM_DEBUG_DRIVER("setting GPU freq to %d\n", > > > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > > > > > > + INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work); > > > + > > > valleyview_set_rps(dev_priv->dev, rpe); > > > > > > /* requires MSI enabled */ > > > @@ -3640,6 +3660,8 @@ void intel_disable_gt_powersave(struct drm_device *dev) > > > mutex_lock(&dev_priv->rps.hw_lock); > > > gen6_disable_rps(dev); > > > mutex_unlock(&dev_priv->rps.hw_lock); > > > > Theoretically the work could get executed here, between unlock and > > cancel. Does that matter? > > AFAICS even gen6_pm_rps_work() might be executing even after > gen6_disable_rps() was called. > > I'm not quite sure if allowing gen6_pm_rps_work() to do stuff after > gen6_disable_rps() was called is already a problem on it's own. At > least it can overwrite RPNSWREQ on !VLV. BTW that register doesn't seem > to exist on VLV and yet we're poking at it from gen6_disable_rps()... Yeah, ordering in our rps teardown code seems screwed-up, since we disable interrupts a bit too late. Now disable_rps does clear PM_IIR and disable all interrupts, but like you've said it doesn't bother to stop the rps_work, so disable_rps can still get rearmed. Two things: - I do not know how much we really should care here for now, since ... - Setup/teardown ordering is already a giant mess and did blow up a lot of times recently. I wonder whether we shouldn't just start to massively sprinkle asserts all over the place to double/triple check ordering sequence constrainst, similarly to how we currently handle the modeset sequence. Another tool to (partially) alleviate these issues (especially failure path handling) is devres.c, which I've just recently stumbled over. Definitely an area to ponder, since imo setup/teardown is the currently worst part (in terms of likeliness to break unnoticed) in our driver. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v3 2013-04-23 13:28 ` Ville Syrjälä 2013-04-23 14:46 ` Daniel Vetter @ 2013-04-23 16:36 ` Jesse Barnes 2013-04-23 17:10 ` Jesse Barnes 2 siblings, 0 replies; 11+ messages in thread From: Jesse Barnes @ 2013-04-23 16:36 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Tue, 23 Apr 2013 16:28:17 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Apr 23, 2013 at 11:45:03AM +0300, Jani Nikula wrote: > > On Tue, 23 Apr 2013, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > On VLV, the Punit doesn't automatically drop the GPU to it's minimum > > > voltage level when entering RC6, so we arm a timer to do it for us from > > > the RPS interrupt handler. It'll generally only fire when we go idle > > > (or if for some reason there's a long delay between RPS interrupts), but > > > won't be re-armed again until the next RPS event, so shouldn't affect > > > power consumption after we go idle and it triggers. > > > > > > v2: use delayed work instead of timer + work queue combo (Ville) > > > v3: fix up delayed work cancel (must be outside lock) (Daniel) > > > fix up delayed work handling func for delayed work (Jesse) > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > > drivers/gpu/drm/i915/i915_irq.c | 11 +++++++++++ > > > drivers/gpu/drm/i915/intel_pm.c | 22 ++++++++++++++++++++++ > > > 3 files changed, 35 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index 2557fc7..f563f25 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -660,6 +660,7 @@ struct i915_suspend_saved_registers { > > > > > > struct intel_gen6_power_mgmt { > > > struct work_struct work; > > > + struct delayed_work vlv_work; > > > u32 pm_iir; > > > /* lock - irqsave spinlock that protectects the work_struct and > > > * pm_iir. */ > > > @@ -670,6 +671,7 @@ struct intel_gen6_power_mgmt { > > > u8 cur_delay; > > > u8 min_delay; > > > u8 max_delay; > > > + u8 rpe_delay; > > > u8 hw_max; > > > > > > struct delayed_work delayed_resume_work; > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > > index 932e7f8..af385e2 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -488,6 +488,17 @@ static void gen6_pm_rps_work(struct work_struct *work) > > > gen6_set_rps(dev_priv->dev, new_delay); > > > } > > > > > > + if (IS_VALLEYVIEW(dev_priv->dev)) { > > > + /* > > > + * On VLV, when we enter RC6 we may not be at the minimum > > > + * voltage level, so arm a timer to check. It should only > > > + * fire when there's activity or once after we've entered > > > + * RC6, and then won't be re-armed until the next RPS interrupt. > > > + */ > > > + mod_delayed_work(dev_priv->wq, &dev_priv->rps.vlv_work, > > > + jiffies + msecs_to_jiffies(100)); > > > > Hooray for conflicting semantics. Unlike mod_timer, mod_delayed_work > > wants a delay, not an absolute time in jiffies. > > > > > + } > > > + > > > mutex_unlock(&dev_priv->rps.hw_lock); > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 2557926..c106187 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -2822,6 +2822,23 @@ int valleyview_rps_min_freq(struct drm_i915_private *dev_priv) > > > return val & 0xff; > > > } > > > > > > +static void vlv_rps_timer_work(struct work_struct *work) > > > +{ > > > + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, > > > + rps.vlv_work.work); > > > + > > > + /* > > > + * Timer fired, we must be idle. Drop to min voltage state. > > > + * Note: we use RPe here since it should match the > > > + * Vmin we were shooting for. That should give us better > > > + * perf when we come back out of RC6 than if we used the > > > + * min freq available. > > > + */ > > > + mutex_lock(&dev_priv->rps.hw_lock); > > > + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > > > + mutex_unlock(&dev_priv->rps.hw_lock); > > > +} > > > + > > > static void valleyview_enable_rps(struct drm_device *dev) > > > { > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > @@ -2886,6 +2903,7 @@ static void valleyview_enable_rps(struct drm_device *dev) > > > rpe = valleyview_rps_rpe_freq(dev_priv); > > > DRM_DEBUG_DRIVER("RPe GPU freq: %d\n", > > > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > > > + dev_priv->rps.rpe_delay = rpe; > > > > > > val = valleyview_rps_min_freq(dev_priv); > > > DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, > > > @@ -2895,6 +2913,8 @@ static void valleyview_enable_rps(struct drm_device *dev) > > > DRM_DEBUG_DRIVER("setting GPU freq to %d\n", > > > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > > > > > > + INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work); > > > + > > > valleyview_set_rps(dev_priv->dev, rpe); > > > > > > /* requires MSI enabled */ > > > @@ -3640,6 +3660,8 @@ void intel_disable_gt_powersave(struct drm_device *dev) > > > mutex_lock(&dev_priv->rps.hw_lock); > > > gen6_disable_rps(dev); > > > mutex_unlock(&dev_priv->rps.hw_lock); > > > > Theoretically the work could get executed here, between unlock and > > cancel. Does that matter? > > AFAICS even gen6_pm_rps_work() might be executing even after > gen6_disable_rps() was called. > > I'm not quite sure if allowing gen6_pm_rps_work() to do stuff after > gen6_disable_rps() was called is already a problem on it's own. At > least it can overwrite RPNSWREQ on !VLV. BTW that register doesn't seem > to exist on VLV and yet we're poking at it from gen6_disable_rps()... Hm that could cause a problem, I know some of the reserved regs in that region have bad side effects. I'll cook up a patch to make a valleyview_disable_rps. Having the work done after rps disable is probably ok, but it's still messy. I'll move the bits up above the rps disable call. Since they're sync that should serialize things. And I'll fix up the mod timer; converting from timers and work queues to delayed work isn't as mindless as it ought to be (I already caught the INIT_DELAYED_WORK bug I had before too, groan). -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v3 2013-04-23 13:28 ` Ville Syrjälä 2013-04-23 14:46 ` Daniel Vetter 2013-04-23 16:36 ` Jesse Barnes @ 2013-04-23 17:10 ` Jesse Barnes 2 siblings, 0 replies; 11+ messages in thread From: Jesse Barnes @ 2013-04-23 17:10 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Tue, 23 Apr 2013 16:28:17 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Apr 23, 2013 at 11:45:03AM +0300, Jani Nikula wrote: > > On Tue, 23 Apr 2013, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > On VLV, the Punit doesn't automatically drop the GPU to it's minimum > > > voltage level when entering RC6, so we arm a timer to do it for us from > > > the RPS interrupt handler. It'll generally only fire when we go idle > > > (or if for some reason there's a long delay between RPS interrupts), but > > > won't be re-armed again until the next RPS event, so shouldn't affect > > > power consumption after we go idle and it triggers. > > > > > > v2: use delayed work instead of timer + work queue combo (Ville) > > > v3: fix up delayed work cancel (must be outside lock) (Daniel) > > > fix up delayed work handling func for delayed work (Jesse) > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > > drivers/gpu/drm/i915/i915_irq.c | 11 +++++++++++ > > > drivers/gpu/drm/i915/intel_pm.c | 22 ++++++++++++++++++++++ > > > 3 files changed, 35 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index 2557fc7..f563f25 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -660,6 +660,7 @@ struct i915_suspend_saved_registers { > > > > > > struct intel_gen6_power_mgmt { > > > struct work_struct work; > > > + struct delayed_work vlv_work; > > > u32 pm_iir; > > > /* lock - irqsave spinlock that protectects the work_struct and > > > * pm_iir. */ > > > @@ -670,6 +671,7 @@ struct intel_gen6_power_mgmt { > > > u8 cur_delay; > > > u8 min_delay; > > > u8 max_delay; > > > + u8 rpe_delay; > > > u8 hw_max; > > > > > > struct delayed_work delayed_resume_work; > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > > index 932e7f8..af385e2 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -488,6 +488,17 @@ static void gen6_pm_rps_work(struct work_struct *work) > > > gen6_set_rps(dev_priv->dev, new_delay); > > > } > > > > > > + if (IS_VALLEYVIEW(dev_priv->dev)) { > > > + /* > > > + * On VLV, when we enter RC6 we may not be at the minimum > > > + * voltage level, so arm a timer to check. It should only > > > + * fire when there's activity or once after we've entered > > > + * RC6, and then won't be re-armed until the next RPS interrupt. > > > + */ > > > + mod_delayed_work(dev_priv->wq, &dev_priv->rps.vlv_work, > > > + jiffies + msecs_to_jiffies(100)); > > > > Hooray for conflicting semantics. Unlike mod_timer, mod_delayed_work > > wants a delay, not an absolute time in jiffies. > > > > > + } > > > + > > > mutex_unlock(&dev_priv->rps.hw_lock); > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 2557926..c106187 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -2822,6 +2822,23 @@ int valleyview_rps_min_freq(struct drm_i915_private *dev_priv) > > > return val & 0xff; > > > } > > > > > > +static void vlv_rps_timer_work(struct work_struct *work) > > > +{ > > > + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, > > > + rps.vlv_work.work); > > > + > > > + /* > > > + * Timer fired, we must be idle. Drop to min voltage state. > > > + * Note: we use RPe here since it should match the > > > + * Vmin we were shooting for. That should give us better > > > + * perf when we come back out of RC6 than if we used the > > > + * min freq available. > > > + */ > > > + mutex_lock(&dev_priv->rps.hw_lock); > > > + valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay); > > > + mutex_unlock(&dev_priv->rps.hw_lock); > > > +} > > > + > > > static void valleyview_enable_rps(struct drm_device *dev) > > > { > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > @@ -2886,6 +2903,7 @@ static void valleyview_enable_rps(struct drm_device *dev) > > > rpe = valleyview_rps_rpe_freq(dev_priv); > > > DRM_DEBUG_DRIVER("RPe GPU freq: %d\n", > > > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > > > + dev_priv->rps.rpe_delay = rpe; > > > > > > val = valleyview_rps_min_freq(dev_priv); > > > DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq, > > > @@ -2895,6 +2913,8 @@ static void valleyview_enable_rps(struct drm_device *dev) > > > DRM_DEBUG_DRIVER("setting GPU freq to %d\n", > > > vlv_gpu_freq(dev_priv->mem_freq, rpe)); > > > > > > + INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work); > > > + > > > valleyview_set_rps(dev_priv->dev, rpe); > > > > > > /* requires MSI enabled */ > > > @@ -3640,6 +3660,8 @@ void intel_disable_gt_powersave(struct drm_device *dev) > > > mutex_lock(&dev_priv->rps.hw_lock); > > > gen6_disable_rps(dev); > > > mutex_unlock(&dev_priv->rps.hw_lock); > > > > Theoretically the work could get executed here, between unlock and > > cancel. Does that matter? > > AFAICS even gen6_pm_rps_work() might be executing even after > gen6_disable_rps() was called. > > I'm not quite sure if allowing gen6_pm_rps_work() to do stuff after > gen6_disable_rps() was called is already a problem on it's own. At > least it can overwrite RPNSWREQ on !VLV. BTW that register doesn't seem > to exist on VLV and yet we're poking at it from gen6_disable_rps()... Ok just posted a new set addressing these issues, please check them out. Thanks, -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-04-23 17:10 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-04-19 17:22 [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 Jesse Barnes 2013-04-22 11:39 ` Ville Syrjälä 2013-04-22 14:46 ` Jesse Barnes 2013-04-22 17:54 ` [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v2 Jesse Barnes 2013-04-22 18:52 ` Daniel Vetter 2013-04-22 23:05 ` [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v3 Jesse Barnes 2013-04-23 8:45 ` Jani Nikula 2013-04-23 13:28 ` Ville Syrjälä 2013-04-23 14:46 ` Daniel Vetter 2013-04-23 16:36 ` Jesse Barnes 2013-04-23 17:10 ` Jesse Barnes
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.