| Message ID | 1392674540-10915-20-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 18, 2014 at 12:02:20AM +0200, Imre Deak wrote: > Based on an early draft from Jesse. > > Add support for powering on/off the dynamic power wells on VLV by > registering its display and dpio dynamic power wells with the power > domain framework. > > For now power on all PHY TX lanes regardless of the actual lane > configuration. Later this can be optimized when the PHY side setup > enables only the required lanes. Atm, it enables all lanes in all > cases. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_dma.c | 1 - > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/intel_display.c | 1 + > drivers/gpu/drm/i915/intel_pm.c | 228 +++++++++++++++++++++++++++++++++++ > 4 files changed, 230 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index dca4dc3..f8f7a59 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1668,7 +1668,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > goto out_mtrrfree; > } > > - dev_priv->display_irqs_enabled = true; > intel_irq_init(dev); > intel_uncore_sanitize(dev); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 227c349..804334e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1053,7 +1053,7 @@ struct i915_power_well { > /* power well enable/disable usage count */ > int count; > unsigned long domains; > - void *data; > + unsigned long data; > const struct i915_power_well_ops *ops; > }; > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index ea00878..d6661c4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4224,6 +4224,7 @@ static void valleyview_modeset_global_resources(struct drm_device *dev) > > if (req_cdclk != cur_cdclk) > valleyview_set_cdclk(dev, req_cdclk); > + modeset_update_power_wells(dev); > } > > static void valleyview_crtc_enable(struct drm_crtc *crtc) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 68f58e5..e4416a7 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5344,6 +5344,133 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > hsw_enable_package_c8(dev_priv); > } > > +static void vlv_set_power_well(struct drm_i915_private *dev_priv, > + struct i915_power_well *power_well, bool enable) > +{ > + enum punit_power_well power_well_id = power_well->data; > + u32 mask; > + u32 state; > + u32 ctrl; > + > + mask = PUNIT_PWRGT_MASK(power_well_id); > + state = enable ? PUNIT_PWRGT_PWR_ON(power_well_id) : > + PUNIT_PWRGT_PWR_GATE(power_well_id); > + > + mutex_lock(&dev_priv->rps.hw_lock); > + > +#define COND \ > + ((vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask) == state) > + > + if (COND) > + goto out; > + > + ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL); > + ctrl &= ~mask; > + ctrl |= state; > + vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, ctrl); > + > + if (wait_for(COND, 100)) > + DRM_ERROR("timout setting power well state %08x (%08x)\n", > + state, > + vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL)); #undef COND somewhere to avoid suprises further down in the code? > + > +out: > + mutex_unlock(&dev_priv->rps.hw_lock); > +} > + <snip>
On Wed, 19 Feb 2014 14:29:44 +0200 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Feb 18, 2014 at 12:02:20AM +0200, Imre Deak wrote: > > Based on an early draft from Jesse. > > > > Add support for powering on/off the dynamic power wells on VLV by > > registering its display and dpio dynamic power wells with the power > > domain framework. > > > > For now power on all PHY TX lanes regardless of the actual lane > > configuration. Later this can be optimized when the PHY side setup > > enables only the required lanes. Atm, it enables all lanes in all > > cases. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_dma.c | 1 - > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_display.c | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 228 +++++++++++++++++++++++++++++++++++ > > 4 files changed, 230 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > index dca4dc3..f8f7a59 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -1668,7 +1668,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > > goto out_mtrrfree; > > } > > > > - dev_priv->display_irqs_enabled = true; > > intel_irq_init(dev); > > intel_uncore_sanitize(dev); > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 227c349..804334e 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1053,7 +1053,7 @@ struct i915_power_well { > > /* power well enable/disable usage count */ > > int count; > > unsigned long domains; > > - void *data; > > + unsigned long data; > > const struct i915_power_well_ops *ops; > > }; > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index ea00878..d6661c4 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4224,6 +4224,7 @@ static void valleyview_modeset_global_resources(struct drm_device *dev) > > > > if (req_cdclk != cur_cdclk) > > valleyview_set_cdclk(dev, req_cdclk); > > + modeset_update_power_wells(dev); > > } > > > > static void valleyview_crtc_enable(struct drm_crtc *crtc) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 68f58e5..e4416a7 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5344,6 +5344,133 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > > hsw_enable_package_c8(dev_priv); > > } > > > > +static void vlv_set_power_well(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well, bool enable) > > +{ > > + enum punit_power_well power_well_id = power_well->data; > > + u32 mask; > > + u32 state; > > + u32 ctrl; > > + > > + mask = PUNIT_PWRGT_MASK(power_well_id); > > + state = enable ? PUNIT_PWRGT_PWR_ON(power_well_id) : > > + PUNIT_PWRGT_PWR_GATE(power_well_id); > > + > > + mutex_lock(&dev_priv->rps.hw_lock); > > + > > +#define COND \ > > + ((vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask) == state) > > + > > + if (COND) > > + goto out; > > + > > + ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL); > > + ctrl &= ~mask; > > + ctrl |= state; > > + vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, ctrl); > > + > > + if (wait_for(COND, 100)) > > + DRM_ERROR("timout setting power well state %08x (%08x)\n", > > + state, > > + vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL)); > > #undef COND somewhere to avoid suprises further down in the code? > > > + > > +out: > > + mutex_unlock(&dev_priv->rps.hw_lock); > > +} > > + > <snip> > I'd like to see the code for re-enabling the display state land eventually too, so we can get savings when userspace uses DPMS instead of NULL mode sets for things. But to do that nicely requires some more work in the mode set code to pull out more PLL bits (also needed for atomic mode setting).
On Thu, 2014-02-20 at 11:58 -0800, Jesse Barnes wrote: > On Wed, 19 Feb 2014 14:29:44 +0200 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Tue, Feb 18, 2014 at 12:02:20AM +0200, Imre Deak wrote: > > > Based on an early draft from Jesse. > > > > > > Add support for powering on/off the dynamic power wells on VLV by > > > registering its display and dpio dynamic power wells with the power > > > domain framework. > > > > > > For now power on all PHY TX lanes regardless of the actual lane > > > configuration. Later this can be optimized when the PHY side setup > > > enables only the required lanes. Atm, it enables all lanes in all > > > cases. > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_dma.c | 1 - > > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > > drivers/gpu/drm/i915/intel_display.c | 1 + > > > drivers/gpu/drm/i915/intel_pm.c | 228 +++++++++++++++++++++++++++++++++++ > > > 4 files changed, 230 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > index dca4dc3..f8f7a59 100644 > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > @@ -1668,7 +1668,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > > > goto out_mtrrfree; > > > } > > > > > > - dev_priv->display_irqs_enabled = true; > > > intel_irq_init(dev); > > > intel_uncore_sanitize(dev); > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index 227c349..804334e 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -1053,7 +1053,7 @@ struct i915_power_well { > > > /* power well enable/disable usage count */ > > > int count; > > > unsigned long domains; > > > - void *data; > > > + unsigned long data; > > > const struct i915_power_well_ops *ops; > > > }; > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index ea00878..d6661c4 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -4224,6 +4224,7 @@ static void valleyview_modeset_global_resources(struct drm_device *dev) > > > > > > if (req_cdclk != cur_cdclk) > > > valleyview_set_cdclk(dev, req_cdclk); > > > + modeset_update_power_wells(dev); > > > } > > > > > > static void valleyview_crtc_enable(struct drm_crtc *crtc) > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 68f58e5..e4416a7 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -5344,6 +5344,133 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > > > hsw_enable_package_c8(dev_priv); > > > } > > > > > > +static void vlv_set_power_well(struct drm_i915_private *dev_priv, > > > + struct i915_power_well *power_well, bool enable) > > > +{ > > > + enum punit_power_well power_well_id = power_well->data; > > > + u32 mask; > > > + u32 state; > > > + u32 ctrl; > > > + > > > + mask = PUNIT_PWRGT_MASK(power_well_id); > > > + state = enable ? PUNIT_PWRGT_PWR_ON(power_well_id) : > > > + PUNIT_PWRGT_PWR_GATE(power_well_id); > > > + > > > + mutex_lock(&dev_priv->rps.hw_lock); > > > + > > > +#define COND \ > > > + ((vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask) == state) > > > + > > > + if (COND) > > > + goto out; > > > + > > > + ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL); > > > + ctrl &= ~mask; > > > + ctrl |= state; > > > + vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, ctrl); > > > + > > > + if (wait_for(COND, 100)) > > > + DRM_ERROR("timout setting power well state %08x (%08x)\n", > > > + state, > > > + vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL)); > > > > #undef COND somewhere to avoid suprises further down in the code? > > > > > + > > > +out: > > > + mutex_unlock(&dev_priv->rps.hw_lock); > > > +} > > > + > > <snip> > > > > I'd like to see the code for re-enabling the display state land > eventually too, so we can get savings when userspace uses DPMS instead > of NULL mode sets for things. But to do that nicely requires some more > work in the mode set code to pull out more PLL bits (also needed for > atomic mode setting). I guess you meant here the drm connector->funcs->dpms() callback, that's called when setting the connector's dpms property. But I'm not sure what you meant by re-enabling the display state. I was thinking that we need to get/put the power domains around setting DPMS off/on via the above property, but we actually don't need that. Internally the dpms callback just calls the display.disable_crtc/enable_crtc() which will disable/enable the pipes but won't do anything with the power domains (which is only updated during a normal modeset through display.modeset_global_resources(). This isn't optimal power-wise, but it's a separate issue. I think Daniel had the idea of converting the dpms callback to be a proper NULL modeset. In that case too the power domains will be get/put correctly. --Imre
On Wed, 26 Feb 2014 20:02:19 +0200 Imre Deak <imre.deak@intel.com> wrote: > On Thu, 2014-02-20 at 11:58 -0800, Jesse Barnes wrote: > > On Wed, 19 Feb 2014 14:29:44 +0200 > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > On Tue, Feb 18, 2014 at 12:02:20AM +0200, Imre Deak wrote: > > > > Based on an early draft from Jesse. > > > > > > > > Add support for powering on/off the dynamic power wells on VLV by > > > > registering its display and dpio dynamic power wells with the power > > > > domain framework. > > > > > > > > For now power on all PHY TX lanes regardless of the actual lane > > > > configuration. Later this can be optimized when the PHY side setup > > > > enables only the required lanes. Atm, it enables all lanes in all > > > > cases. > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_dma.c | 1 - > > > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > > > drivers/gpu/drm/i915/intel_display.c | 1 + > > > > drivers/gpu/drm/i915/intel_pm.c | 228 +++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 230 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > > index dca4dc3..f8f7a59 100644 > > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > > @@ -1668,7 +1668,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > > > > goto out_mtrrfree; > > > > } > > > > > > > > - dev_priv->display_irqs_enabled = true; > > > > intel_irq_init(dev); > > > > intel_uncore_sanitize(dev); > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > > index 227c349..804334e 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -1053,7 +1053,7 @@ struct i915_power_well { > > > > /* power well enable/disable usage count */ > > > > int count; > > > > unsigned long domains; > > > > - void *data; > > > > + unsigned long data; > > > > const struct i915_power_well_ops *ops; > > > > }; > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index ea00878..d6661c4 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -4224,6 +4224,7 @@ static void valleyview_modeset_global_resources(struct drm_device *dev) > > > > > > > > if (req_cdclk != cur_cdclk) > > > > valleyview_set_cdclk(dev, req_cdclk); > > > > + modeset_update_power_wells(dev); > > > > } > > > > > > > > static void valleyview_crtc_enable(struct drm_crtc *crtc) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index 68f58e5..e4416a7 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -5344,6 +5344,133 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > > > > hsw_enable_package_c8(dev_priv); > > > > } > > > > > > > > +static void vlv_set_power_well(struct drm_i915_private *dev_priv, > > > > + struct i915_power_well *power_well, bool enable) > > > > +{ > > > > + enum punit_power_well power_well_id = power_well->data; > > > > + u32 mask; > > > > + u32 state; > > > > + u32 ctrl; > > > > + > > > > + mask = PUNIT_PWRGT_MASK(power_well_id); > > > > + state = enable ? PUNIT_PWRGT_PWR_ON(power_well_id) : > > > > + PUNIT_PWRGT_PWR_GATE(power_well_id); > > > > + > > > > + mutex_lock(&dev_priv->rps.hw_lock); > > > > + > > > > +#define COND \ > > > > + ((vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask) == state) > > > > + > > > > + if (COND) > > > > + goto out; > > > > + > > > > + ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL); > > > > + ctrl &= ~mask; > > > > + ctrl |= state; > > > > + vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, ctrl); > > > > + > > > > + if (wait_for(COND, 100)) > > > > + DRM_ERROR("timout setting power well state %08x (%08x)\n", > > > > + state, > > > > + vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL)); > > > > > > #undef COND somewhere to avoid suprises further down in the code? > > > > > > > + > > > > +out: > > > > + mutex_unlock(&dev_priv->rps.hw_lock); > > > > +} > > > > + > > > <snip> > > > > > > > I'd like to see the code for re-enabling the display state land > > eventually too, so we can get savings when userspace uses DPMS instead > > of NULL mode sets for things. But to do that nicely requires some more > > work in the mode set code to pull out more PLL bits (also needed for > > atomic mode setting). > > I guess you meant here the drm connector->funcs->dpms() callback, that's > called when setting the connector's dpms property. But I'm not sure what > you meant by re-enabling the display state. > > I was thinking that we need to get/put the power domains around setting > DPMS off/on via the above property, but we actually don't need that. > Internally the dpms callback just calls the > display.disable_crtc/enable_crtc() which will disable/enable the pipes > but won't do anything with the power domains (which is only updated > during a normal modeset through display.modeset_global_resources(). > > This isn't optimal power-wise, but it's a separate issue. I think Daniel > had the idea of converting the dpms callback to be a proper NULL > modeset. In that case too the power domains will be get/put correctly. Right, that's what I'm getting at. Today when someone uses the DPMS prop, we won't go through the mode set path and thus won't do any power well toggles. I think that's a bug we should fix. Either by making DPMS into a NULL mode set, or pulling out more bits from our mode set sequence into our crtc enable/disable paths. My earlier code did this somewhat differently, but it did cover DPMS.
On Wed, 2014-02-26 at 11:52 -0800, Jesse Barnes wrote: > On Wed, 26 Feb 2014 20:02:19 +0200 > Imre Deak <imre.deak@intel.com> wrote: > > > On Thu, 2014-02-20 at 11:58 -0800, Jesse Barnes wrote: > > > On Wed, 19 Feb 2014 14:29:44 +0200 > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > > > > On Tue, Feb 18, 2014 at 12:02:20AM +0200, Imre Deak wrote: > > > > > Based on an early draft from Jesse. > > > > > > > > > > Add support for powering on/off the dynamic power wells on VLV by > > > > > registering its display and dpio dynamic power wells with the power > > > > > domain framework. > > > > > > > > > > For now power on all PHY TX lanes regardless of the actual lane > > > > > configuration. Later this can be optimized when the PHY side setup > > > > > enables only the required lanes. Atm, it enables all lanes in all > > > > > cases. > > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_dma.c | 1 - > > > > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > > > > drivers/gpu/drm/i915/intel_display.c | 1 + > > > > > drivers/gpu/drm/i915/intel_pm.c | 228 +++++++++++++++++++++++++++++++++++ > > > > > 4 files changed, 230 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > > > index dca4dc3..f8f7a59 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > > > @@ -1668,7 +1668,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > > > > > goto out_mtrrfree; > > > > > } > > > > > > > > > > - dev_priv->display_irqs_enabled = true; > > > > > intel_irq_init(dev); > > > > > intel_uncore_sanitize(dev); > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > > > index 227c349..804334e 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > > @@ -1053,7 +1053,7 @@ struct i915_power_well { > > > > > /* power well enable/disable usage count */ > > > > > int count; > > > > > unsigned long domains; > > > > > - void *data; > > > > > + unsigned long data; > > > > > const struct i915_power_well_ops *ops; > > > > > }; > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > > index ea00878..d6661c4 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -4224,6 +4224,7 @@ static void valleyview_modeset_global_resources(struct drm_device *dev) > > > > > > > > > > if (req_cdclk != cur_cdclk) > > > > > valleyview_set_cdclk(dev, req_cdclk); > > > > > + modeset_update_power_wells(dev); > > > > > } > > > > > > > > > > static void valleyview_crtc_enable(struct drm_crtc *crtc) > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > index 68f58e5..e4416a7 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > @@ -5344,6 +5344,133 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > > > > > hsw_enable_package_c8(dev_priv); > > > > > } > > > > > > > > > > +static void vlv_set_power_well(struct drm_i915_private *dev_priv, > > > > > + struct i915_power_well *power_well, bool enable) > > > > > +{ > > > > > + enum punit_power_well power_well_id = power_well->data; > > > > > + u32 mask; > > > > > + u32 state; > > > > > + u32 ctrl; > > > > > + > > > > > + mask = PUNIT_PWRGT_MASK(power_well_id); > > > > > + state = enable ? PUNIT_PWRGT_PWR_ON(power_well_id) : > > > > > + PUNIT_PWRGT_PWR_GATE(power_well_id); > > > > > + > > > > > + mutex_lock(&dev_priv->rps.hw_lock); > > > > > + > > > > > +#define COND \ > > > > > + ((vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask) == state) > > > > > + > > > > > + if (COND) > > > > > + goto out; > > > > > + > > > > > + ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL); > > > > > + ctrl &= ~mask; > > > > > + ctrl |= state; > > > > > + vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, ctrl); > > > > > + > > > > > + if (wait_for(COND, 100)) > > > > > + DRM_ERROR("timout setting power well state %08x (%08x)\n", > > > > > + state, > > > > > + vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL)); > > > > > > > > #undef COND somewhere to avoid suprises further down in the code? > > > > > > > > > + > > > > > +out: > > > > > + mutex_unlock(&dev_priv->rps.hw_lock); > > > > > +} > > > > > + > > > > <snip> > > > > > > > > > > I'd like to see the code for re-enabling the display state land > > > eventually too, so we can get savings when userspace uses DPMS instead > > > of NULL mode sets for things. But to do that nicely requires some more > > > work in the mode set code to pull out more PLL bits (also needed for > > > atomic mode setting). > > > > I guess you meant here the drm connector->funcs->dpms() callback, that's > > called when setting the connector's dpms property. But I'm not sure what > > you meant by re-enabling the display state. > > > > I was thinking that we need to get/put the power domains around setting > > DPMS off/on via the above property, but we actually don't need that. > > Internally the dpms callback just calls the > > display.disable_crtc/enable_crtc() which will disable/enable the pipes > > but won't do anything with the power domains (which is only updated > > during a normal modeset through display.modeset_global_resources(). > > > > This isn't optimal power-wise, but it's a separate issue. I think Daniel > > had the idea of converting the dpms callback to be a proper NULL > > modeset. In that case too the power domains will be get/put correctly. > > Right, that's what I'm getting at. Today when someone uses the DPMS > prop, we won't go through the mode set path and thus won't do any power > well toggles. I think that's a bug we should fix. Either by making > DPMS into a NULL mode set, or pulling out more bits from our mode set > sequence into our crtc enable/disable paths. Ok, I agree the DPMS prop handling should be fixed. Making it a NULL modeset makes sense to me, X is already using that anyway for blanking and by that we would have a single code path for both modeset and blanking. > My earlier code did this somewhat differently, but it did cover DPMS. Right, checking again [1], you toggled the power around DPMS on/off. I'd suggest that we fix this later by fixing DPMS prop as above, since it: - keeps things simple, no need to save/restore the display HW state during power domain off/on - X is using a NULL modeset for blanking, so the DPMS prop handling is less important IMO I guess this is what you also suggested. [1] http://lists.freedesktop.org/archives/intel-gfx/2013-October/034584.html
On Wed, Feb 26, 2014 at 11:52:25AM -0800, Jesse Barnes wrote: > On Wed, 26 Feb 2014 20:02:19 +0200 > Imre Deak <imre.deak@intel.com> wrote: > > > On Thu, 2014-02-20 at 11:58 -0800, Jesse Barnes wrote: > > > I'd like to see the code for re-enabling the display state land > > > eventually too, so we can get savings when userspace uses DPMS instead > > > of NULL mode sets for things. But to do that nicely requires some more > > > work in the mode set code to pull out more PLL bits (also needed for > > > atomic mode setting). > > > > I guess you meant here the drm connector->funcs->dpms() callback, that's > > called when setting the connector's dpms property. But I'm not sure what > > you meant by re-enabling the display state. > > > > I was thinking that we need to get/put the power domains around setting > > DPMS off/on via the above property, but we actually don't need that. > > Internally the dpms callback just calls the > > display.disable_crtc/enable_crtc() which will disable/enable the pipes > > but won't do anything with the power domains (which is only updated > > during a normal modeset through display.modeset_global_resources(). > > > > This isn't optimal power-wise, but it's a separate issue. I think Daniel > > had the idea of converting the dpms callback to be a proper NULL > > modeset. In that case too the power domains will be get/put correctly. > > Right, that's what I'm getting at. Today when someone uses the DPMS > prop, we won't go through the mode set path and thus won't do any power > well toggles. I think that's a bug we should fix. Either by making > DPMS into a NULL mode set, or pulling out more bits from our mode set > sequence into our crtc enable/disable paths. Yeah, I think in the end we should be able to get rid of the ->mode_set callbacks completely and move all the remaining hw frobbing into ->enable and all the remaining state computation (plls mostly) into ->compute_config. That way ->enable will always restore the full hw state and so won't get screwed over when a power well switched cleared all the registers. With that done we can add the display power well stuff from the modeset path to the dpms path and update so that it follows crtc->active and not crtc->enabled. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index dca4dc3..f8f7a59 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1668,7 +1668,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) goto out_mtrrfree; } - dev_priv->display_irqs_enabled = true; intel_irq_init(dev); intel_uncore_sanitize(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 227c349..804334e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1053,7 +1053,7 @@ struct i915_power_well { /* power well enable/disable usage count */ int count; unsigned long domains; - void *data; + unsigned long data; const struct i915_power_well_ops *ops; }; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ea00878..d6661c4 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4224,6 +4224,7 @@ static void valleyview_modeset_global_resources(struct drm_device *dev) if (req_cdclk != cur_cdclk) valleyview_set_cdclk(dev, req_cdclk); + modeset_update_power_wells(dev); } static void valleyview_crtc_enable(struct drm_crtc *crtc) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 68f58e5..e4416a7 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5344,6 +5344,133 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv, hsw_enable_package_c8(dev_priv); } +static void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable) +{ + enum punit_power_well power_well_id = power_well->data; + u32 mask; + u32 state; + u32 ctrl; + + mask = PUNIT_PWRGT_MASK(power_well_id); + state = enable ? PUNIT_PWRGT_PWR_ON(power_well_id) : + PUNIT_PWRGT_PWR_GATE(power_well_id); + + mutex_lock(&dev_priv->rps.hw_lock); + +#define COND \ + ((vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask) == state) + + if (COND) + goto out; + + ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL); + ctrl &= ~mask; + ctrl |= state; + vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, ctrl); + + if (wait_for(COND, 100)) + DRM_ERROR("timout setting power well state %08x (%08x)\n", + state, + vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL)); + +out: + mutex_unlock(&dev_priv->rps.hw_lock); +} + +static void vlv_power_well_sync_hw(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + vlv_set_power_well(dev_priv, power_well, power_well->count > 0); +} + +static void vlv_power_well_enable(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + vlv_set_power_well(dev_priv, power_well, true); +} + +static void vlv_power_well_disable(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + vlv_set_power_well(dev_priv, power_well, false); +} + +static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + int power_well_id = power_well->data; + bool enabled = false; + u32 gating_mask; + u32 gating_val; + u32 val; + + gating_mask = PUNIT_PWRGT_MASK(power_well_id); + gating_val = PUNIT_PWRGT_PWR_ON(power_well_id); + + mutex_lock(&dev_priv->rps.hw_lock); + + val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & gating_mask; + /* + * We only ever set the power-on and power-gate states, anything + * else is unexpected. + */ + WARN_ON(val != PUNIT_PWRGT_PWR_ON(power_well_id) && + val != PUNIT_PWRGT_PWR_GATE(power_well_id)); + if (val == gating_val) + enabled = true; + + /* + * A transient state at this point would mean some unexpected party + * is poking at the power controls too. + */ + gating_val = val; + val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL) & gating_mask; + WARN_ON(val != gating_val); + + mutex_unlock(&dev_priv->rps.hw_lock); + + return enabled; +} + +static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + + WARN_ON_ONCE(power_well->data != PUNIT_POWER_WELL_DISP2D); + + vlv_set_power_well(dev_priv, power_well, true); + valleyview_enable_display_irqs(dev_priv); + + /* + * During driver initialization we need to defer enabling hotplug + * processing until fbdev is set up. + */ + if (dev_priv->enable_hotplug_processing) + intel_hpd_init(dev_priv->dev); + + i915_redisable_vga_power_on(dev_priv->dev); +} + +static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + struct drm_device *dev = dev_priv->dev; + enum pipe pipe; + + WARN_ON_ONCE(power_well->data != PUNIT_POWER_WELL_DISP2D); + + for_each_pipe(pipe) + intel_set_cpu_fifo_underrun_reporting(dev, pipe, false); + + valleyview_disable_display_irqs(dev_priv); + + for_each_pipe(pipe) + reset_vblank_counter(dev, pipe); + + vlv_set_power_well(dev_priv, power_well, false); +} + static void check_power_well_state(struct drm_i915_private *dev_priv, struct i915_power_well *power_well) { @@ -5472,6 +5599,35 @@ EXPORT_SYMBOL_GPL(i915_release_power_well); (POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS) | \ BIT(POWER_DOMAIN_INIT)) +#define VLV_ALWAYS_ON_POWER_DOMAINS BIT(POWER_DOMAIN_INIT) +#define VLV_DISPLAY_POWER_DOMAINS POWER_DOMAIN_MASK + +#define VLV_DPIO_CMN_BC_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) | \ + BIT(POWER_DOMAIN_PORT_CRT) | \ + BIT(POWER_DOMAIN_INIT)) + +#define VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) | \ + BIT(POWER_DOMAIN_INIT)) + +#define VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) | \ + BIT(POWER_DOMAIN_INIT)) + +#define VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) | \ + BIT(POWER_DOMAIN_INIT)) + +#define VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) | \ + BIT(POWER_DOMAIN_INIT)) + static const struct i915_power_well_ops i9xx_always_on_power_well_ops = { }; static struct i915_power_well i9xx_always_on_power_well[] = { @@ -5515,6 +5671,76 @@ static struct i915_power_well bdw_power_wells[] = { }, }; +static const struct i915_power_well_ops vlv_display_power_well_ops = { + .sync_hw = vlv_power_well_sync_hw, + .enable = vlv_display_power_well_enable, + .disable = vlv_display_power_well_disable, + .is_enabled = vlv_power_well_enabled, +}; + +static const struct i915_power_well_ops vlv_dpio_power_well_ops = { + .sync_hw = vlv_power_well_sync_hw, + .enable = vlv_power_well_enable, + .disable = vlv_power_well_disable, + .is_enabled = vlv_power_well_enabled, +}; + +static struct i915_power_well vlv_power_wells[] = { + { + .name = "always-on", + .domains = VLV_ALWAYS_ON_POWER_DOMAINS, + .ops = &i9xx_always_on_power_well_ops, + }, + { + .name = "display", + .domains = VLV_DISPLAY_POWER_DOMAINS, + .data = PUNIT_POWER_WELL_DISP2D, + .ops = &vlv_display_power_well_ops, + }, + { + .name = "dpio-common", + .domains = VLV_DPIO_CMN_BC_POWER_DOMAINS, + .data = PUNIT_POWER_WELL_DPIO_CMN_BC, + .ops = &vlv_dpio_power_well_ops, + }, + { + .name = "dpio-tx-b-01", + .domains = VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS | + VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS | + VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS | + VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS, + .ops = &vlv_dpio_power_well_ops, + .data = PUNIT_POWER_WELL_DPIO_TX_B_LANES_01, + }, + { + .name = "dpio-tx-b-23", + .domains = VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS | + VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS | + VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS | + VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS, + .ops = &vlv_dpio_power_well_ops, + .data = PUNIT_POWER_WELL_DPIO_TX_B_LANES_23, + }, + { + .name = "dpio-tx-c-01", + .domains = VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS | + VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS | + VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS | + VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS, + .ops = &vlv_dpio_power_well_ops, + .data = PUNIT_POWER_WELL_DPIO_TX_C_LANES_01, + }, + { + .name = "dpio-tx-c-23", + .domains = VLV_DPIO_TX_B_LANES_01_POWER_DOMAINS | + VLV_DPIO_TX_B_LANES_23_POWER_DOMAINS | + VLV_DPIO_TX_C_LANES_01_POWER_DOMAINS | + VLV_DPIO_TX_C_LANES_23_POWER_DOMAINS, + .ops = &vlv_dpio_power_well_ops, + .data = PUNIT_POWER_WELL_DPIO_TX_C_LANES_23, + }, +}; + #define set_power_wells(power_domains, __power_wells) ({ \ (power_domains)->power_wells = (__power_wells); \ (power_domains)->power_well_count = ARRAY_SIZE(__power_wells); \ @@ -5536,6 +5762,8 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv) } else if (IS_BROADWELL(dev_priv->dev)) { set_power_wells(power_domains, bdw_power_wells); hsw_pwr = power_domains; + } else if (IS_VALLEYVIEW(dev_priv->dev)) { + set_power_wells(power_domains, vlv_power_wells); } else { set_power_wells(power_domains, i9xx_always_on_power_well); }
Based on an early draft from Jesse. Add support for powering on/off the dynamic power wells on VLV by registering its display and dpio dynamic power wells with the power domain framework. For now power on all PHY TX lanes regardless of the actual lane configuration. Later this can be optimized when the PHY side setup enables only the required lanes. Atm, it enables all lanes in all cases. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_dma.c | 1 - drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_display.c | 1 + drivers/gpu/drm/i915/intel_pm.c | 228 +++++++++++++++++++++++++++++++++++ 4 files changed, 230 insertions(+), 2 deletions(-)