On Wed, Mar 19, 2014 at 05:07:53PM +0200, Imre Deak wrote:
> On Fri, 2014-03-07 at 20:08 -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >
> > Currently, when our driver becomes idle for i915.pc8_timeout (default:
> > 5s) we enable PC8, so we save some power, but not everything we can.
> > Then, while PC8 is enabled, if we stay idle for more
> > autosuspend_delay_ms (default: 10s) we'll enter runtime PM and put the
> > graphics device in D3 state, saving even more power. The two features
> > are separate things with increasing levels of power savings, but if we
> > disable PC8 we'll never get into D3.
> >
> > While from the modularity point of view it would be nice to keep these
> > features as separate, we have reasons to merge them:
> > - We are not aware of anybody wanting a "PC8 without D3" environment.
> > - If we keep both features as separate, we'll have to to test both
> > PC8 and PC8+D3 code paths. We're already having a major pain to
> > make QA do automated testing of just one thing, testing both paths
> > will cost even more.
> > - Only Haswell+ supports PC8, so if we want to add runtime PM support
> > to, for example, IVB, we'll have to copy some code from the PC8
> > feature to runtime PM, so merging both features as a single thing
> > will make it easier for enabling runtime PM on other platforms.
> >
> > This patch only does the very basic steps required to have PC8 and
> > runtime PM merged on a single feature: the next patches will take care
> > of cleaning up everything.
> >
> > v2: - Rebase.
> > v3: - Rebase.
> > - Fully remove the deprecated i915 params since Daniel doesn't
> > consider them as part of the ABI.
> > v4: - Rebase.
> > - Fix typo in the commit message.
> > v5: - Rebase, again.
> > - Add a huge comment explaining the different forcewake usage
> > (Chris, Daniel).
> > - Use open-coded forcewake functions (Daniel).
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_dma.c | 2 --
> > drivers/gpu/drm/i915/i915_drv.c | 8 +++++
> > drivers/gpu/drm/i915/i915_drv.h | 7 ++--
> > drivers/gpu/drm/i915/i915_params.c | 10 ------
> > drivers/gpu/drm/i915/intel_display.c | 65 ++++++++++++++++--------------------
> > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > drivers/gpu/drm/i915/intel_pm.c | 1 -
> > 7 files changed, 43 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index e4d2b9f..6aa23af 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1822,8 +1822,6 @@ int i915_driver_unload(struct drm_device *dev)
> > cancel_work_sync(&dev_priv->gpu_error.work);
> > i915_destroy_error_state(dev);
> >
> > - cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
> > -
> > if (dev->pdev->msi_enabled)
> > pci_disable_msi(dev->pdev);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 658fe24..6178c95 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -848,6 +848,9 @@ static int i915_runtime_suspend(struct device *device)
> >
> > DRM_DEBUG_KMS("Suspending device\n");
> >
> > + if (HAS_PC8(dev))
> > + __hsw_do_enable_pc8(dev_priv);
> > +
> > i915_gem_release_all_mmaps(dev_priv);
> >
> > del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> > @@ -862,6 +865,7 @@ static int i915_runtime_suspend(struct device *device)
> > */
> > intel_opregion_notify_adapter(dev, PCI_D1);
> >
> > + DRM_DEBUG_KMS("Device suspended\n");
> > return 0;
> > }
> >
> > @@ -878,6 +882,10 @@ static int i915_runtime_resume(struct device *device)
> > intel_opregion_notify_adapter(dev, PCI_D0);
> > dev_priv->pm.suspended = false;
> >
> > + if (HAS_PC8(dev))
> > + __hsw_do_disable_pc8(dev_priv);
> > +
> > + DRM_DEBUG_KMS("Device resumed\n");
> > return 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2a319ba..a5f1780 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1335,6 +1335,10 @@ struct ilk_wm_values {
> > /*
> > * This struct tracks the state needed for the Package C8+ feature.
> > *
> > + * TODO: we're merging the Package C8+ feature with the runtime PM support. To
> > + * avoid having to update the documentation at each patch of the series, we'll
> > + * do a final update at the end.
> > + *
> > * Package states C8 and deeper are really deep PC states that can only be
> > * reached when all the devices on the system allow it, so even if the graphics
> > * device allows PC8+, it doesn't mean the system will actually get to these
> > @@ -1388,7 +1392,6 @@ struct i915_package_c8 {
> > bool enabled;
> > int disable_count;
> > struct mutex lock;
> > - struct delayed_work enable_work;
> >
> > struct {
> > uint32_t deimr;
> > @@ -2092,8 +2095,6 @@ struct i915_params {
> > unsigned int preliminary_hw_support;
> > int disable_power_well;
> > int enable_ips;
> > - int enable_pc8;
> > - int pc8_timeout;
> > int invert_brightness;
> > int enable_cmd_parser;
> > /* leave bools at the end to not create holes */
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index a66ffb6..d1d7980 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -42,8 +42,6 @@ struct i915_params i915 __read_mostly = {
> > .disable_power_well = 1,
> > .enable_ips = 1,
> > .fastboot = 0,
> > - .enable_pc8 = 1,
> > - .pc8_timeout = 5000,
> > .prefault_disable = 0,
> > .reset = true,
> > .invert_brightness = 0,
> > @@ -135,14 +133,6 @@ module_param_named(fastboot, i915.fastboot, bool, 0600);
> > MODULE_PARM_DESC(fastboot,
> > "Try to skip unnecessary mode sets at boot time (default: false)");
> >
> > -module_param_named(enable_pc8, i915.enable_pc8, int, 0600);
> > -MODULE_PARM_DESC(enable_pc8,
> > - "Enable support for low power package C states (PC8+) (default: true)");
> > -
> > -module_param_named(pc8_timeout, i915.pc8_timeout, int, 0600);
> > -MODULE_PARM_DESC(pc8_timeout,
> > - "Number of msecs of idleness required to enter PC8+ (default: 5000)");
> > -
> > module_param_named(prefault_disable, i915.prefault_disable, bool, 0600);
> > MODULE_PARM_DESC(prefault_disable,
> > "Disable page prefaulting for pread/pwrite/reloc (default:false). "
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9c3e2c5..ab02848 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6726,6 +6726,7 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
> > static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> > {
> > uint32_t val;
> > + unsigned long irqflags;
> >
> > val = I915_READ(LCPLL_CTL);
> >
> > @@ -6733,9 +6734,22 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> > LCPLL_POWER_DOWN_ALLOW)) == LCPLL_PLL_LOCK)
> > return;
> >
> > - /* Make sure we're not on PC8 state before disabling PC8, otherwise
> > - * we'll hang the machine! */
> > - gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> > + /*
> > + * Make sure we're not on PC8 state before disabling PC8, otherwise
>
> While at it the above comment could be clarified. For me 'before
> enabling PLL, which also disables PC8' would be clearer, if that's what
> was meant.
I think we can do this comment polishing later on.
> > + * we'll hang the machine. To prevent PC8 state, just enable force_wake.
> > + *
> > + * The other problem is that hsw_restore_lcpll() is called as part of
> > + * the runtime PM resume sequence, so we can't just call
> > + * gen6_gt_force_wake_get() because that function calls
> > + * intel_runtime_pm_get(), and we can't change the runtime PM refcount
> > + * while we are on the resume sequence. So to solve this problem we have
> > + * to call special forcewake code that doesn't touch runtime PM and
> > + * doesn't enable the forcewake delayed work.
> > + */
> > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > + if (dev_priv->uncore.forcewake_count++ == 0)
> > + dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> > + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >
> > if (val & LCPLL_POWER_DOWN_ALLOW) {
> > val &= ~LCPLL_POWER_DOWN_ALLOW;
> > @@ -6769,14 +6783,20 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> > DRM_ERROR("Switching back to LCPLL failed\n");
> > }
> >
> > - gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> > + /* See the big comment above. */
> > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > + if (--dev_priv->uncore.forcewake_count == 0)
> > + dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> > + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>
> For clarity I would group these in separate functions with the rest of
> force wake helpers in intel_uncore.c.
tbh we currently have a rather big mess with all our forcewake functions
and the setup/init code. Before we encapsulate things like crazy I think
we should work out all the existing little bugs and races we have :(
>
> Otherwise looks good, so with the above fixed:
> Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
Thanks for patches&review, merged it all to dinq.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
