From: Imre Deak <imre.deak@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 21/21] drm/i915: power domains: add vlv power wells
Date: Thu, 06 Mar 2014 23:04:04 +0200 [thread overview]
Message-ID: <1394139844.2760.3.camel@ideak-mobl> (raw)
In-Reply-To: <20140306205243.GI17001@phenom.ffwll.local>
On Thu, 2014-03-06 at 21:52 +0100, Daniel Vetter wrote:
> On Thu, Mar 06, 2014 at 12:29:24PM -0800, Jesse Barnes wrote:
> > On Tue, 4 Mar 2014 19:23:10 +0200
> > Imre Deak <imre.deak@intel.com> 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.
> > >
> > > v2:
> > > - undef function local COND macro after its last use (Ville)
> > > - Take dev_priv->irq_lock around the whole sequence of
> > > intel_set_cpu_fifo_underrun_reporting_nolock() and
> > > valleyview_disable_display_irqs(). They are short and releasing
> > > the lock in between only makes proving correctness more difficult.
> > > - sanitize local var names in vlv_power_well_enabled()
> > >
> > > 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_drv.h | 2 +
> > > drivers/gpu/drm/i915/intel_pm.c | 237 +++++++++++++++++++++++++++++++++++
> > > 5 files changed, 241 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 fea0216..3fad6ed 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1054,7 +1054,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 6fb40cb..b2eb07a 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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index afc01a4..f2555ac 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -609,6 +609,8 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
> > > /* i915_irq.c */
> > > bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
> > > enum pipe pipe, bool enable);
> > > +bool intel_set_cpu_fifo_underrun_reporting_nolock(struct drm_device *dev,
> > > + enum pipe pipe, bool enable);
> > > bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
> > > enum transcoder pch_transcoder,
> > > bool enable);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 39acffd..b8b5e3e 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5356,6 +5356,141 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv,
> > > return true;
> > > }
> > >
> > > +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
> > > +
> > > +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 mask;
> > > + u32 state;
> > > + u32 ctrl;
> > > +
> > > + mask = PUNIT_PWRGT_MASK(power_well_id);
> > > + ctrl = PUNIT_PWRGT_PWR_ON(power_well_id);
> > > +
> > > + mutex_lock(&dev_priv->rps.hw_lock);
> > > +
> > > + state = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask;
> > > + /*
> > > + * We only ever set the power-on and power-gate states, anything
> > > + * else is unexpected.
> > > + */
> > > + WARN_ON(state != PUNIT_PWRGT_PWR_ON(power_well_id) &&
> > > + state != PUNIT_PWRGT_PWR_GATE(power_well_id));
> > > + if (state == ctrl)
> > > + enabled = true;
> > > +
> > > + /*
> > > + * A transient state at this point would mean some unexpected party
> > > + * is poking at the power controls too.
> > > + */
> > > + ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL) & mask;
> > > + WARN_ON(ctrl != state);
> > > +
> > > + 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);
> > > +
> > > + spin_lock_irq(&dev_priv->irq_lock);
> > > + valleyview_enable_display_irqs(dev_priv);
> > > + spin_unlock_irq(&dev_priv->irq_lock);
> > > +
> > > + /*
> > > + * 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);
> > > +
> > > + spin_lock_irq(&dev_priv->irq_lock);
> > > + for_each_pipe(pipe)
> > > + intel_set_cpu_fifo_underrun_reporting_nolock(dev, pipe, false);
> > > +
> > > + valleyview_disable_display_irqs(dev_priv);
> > > + spin_unlock_irq(&dev_priv->irq_lock);
> > > +
> > > + spin_lock_irq(&dev->vbl_lock);
> > > + for_each_pipe(pipe)
> > > + reset_vblank_counter(dev, pipe);
> > > + spin_unlock_irq(&dev->vbl_lock);
> > > +
> > > + 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)
> > > {
> > > @@ -5488,6 +5623,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 = {
> > > .sync_hw = i9xx_always_on_power_well_noop,
> > > .enable = i9xx_always_on_power_well_noop,
> > > @@ -5539,6 +5703,77 @@ 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",
> > > + .always_on = 1,
> > > + .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); \
> > > @@ -5560,6 +5795,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);
> > > }
> >
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> I've pulled in the entire series, but due to the different merge order
> please double check that all is fine indeed.
>
> I've also thrown in an additional assert_spin_locked where it seemed to be
> missing.
Seems to match my local version, except for the w/s and
assert_spin_locked fixup. Thanks all for the quick review on this.
--Imre
prev parent reply other threads:[~2014-03-06 21:04 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-04 17:22 [PATCH v2 00/21] drm/i915: vlv power domains support Imre Deak
2014-03-04 17:22 ` [PATCH v2 01/21] drm/i915: use drm_i915_private everywhere in the power domain api Imre Deak
2014-03-06 19:00 ` Jesse Barnes
2014-03-04 17:22 ` [PATCH v2 02/21] drm/i915: fold in __intel_power_well_get/put functions Imre Deak
2014-03-04 17:22 ` [PATCH v2 03/21] drm/i915: move modeset_update_power_wells earlier Imre Deak
2014-03-05 14:20 ` [PATCH v3 " Imre Deak
2014-03-04 17:22 ` [PATCH v2 04/21] drm/i915: move power domain macros to intel_pm.c Imre Deak
2014-03-06 19:00 ` Jesse Barnes
2014-03-04 17:22 ` [PATCH v2 05/21] drm/i915: add init power domain to always-on power wells Imre Deak
2014-03-06 19:01 ` Jesse Barnes
2014-03-04 17:22 ` [PATCH v2 06/21] drm/i915: split power well 'set' handler to separate enable/disable/sync_hw Imre Deak
2014-03-06 20:04 ` Daniel Vetter
2014-03-04 17:22 ` [PATCH v2 07/21] drm/i915: add noop power well handlers instead of NULL checking them Imre Deak
2014-03-06 19:02 ` Jesse Barnes
2014-03-04 17:22 ` [PATCH v2 08/21] drm/i915: add port power domains Imre Deak
2014-03-04 17:22 ` [PATCH v2 09/21] drm/i915: get port power domain in connector detect handlers Imre Deak
2014-03-05 14:20 ` [PATCH v3 " Imre Deak
2014-03-06 19:04 ` Jesse Barnes
2014-03-04 17:22 ` [PATCH v2 10/21] drm/i915: check port power domain when reading the encoder hw state Imre Deak
2014-03-05 14:20 ` [PATCH v3 " Imre Deak
2014-03-06 19:06 ` Jesse Barnes
2014-03-04 17:23 ` [PATCH v2 11/21] drm/i915: check pipe power domain when reading its " Imre Deak
2014-03-05 14:20 ` [PATCH v3 " Imre Deak
2014-03-06 19:06 ` Jesse Barnes
2014-03-04 17:23 ` [PATCH v2 12/21] drm/i915: vlv: keep first level vblank IRQs masked Imre Deak
2014-03-06 19:09 ` Jesse Barnes
2014-03-04 17:23 ` [PATCH v2 13/21] drm/i915: sanitize PUNIT register macro definitions Imre Deak
2014-03-04 17:23 ` [PATCH v2 14/21] drm/i915: factor out reset_vblank_counter Imre Deak
2014-03-06 19:10 ` Jesse Barnes
2014-03-04 17:23 ` [PATCH v2 15/21] drm/i915: switch order of power domain init wrt. irq install Imre Deak
2014-03-04 17:23 ` [PATCH v2 16/21] drm/i915: use power domain api to check vga power state Imre Deak
2014-03-04 17:23 ` [PATCH v2 17/21] drm/i915: sanity check power well sw state against hw state Imre Deak
2014-03-06 19:11 ` Jesse Barnes
2014-03-04 17:23 ` [PATCH v2 18/21] drm/i915: vlv: factor out valleyview_display_irq_install Imre Deak
2014-03-06 19:17 ` Jesse Barnes
2014-03-04 17:23 ` [PATCH v2 19/21] drm/i915: move hsw power domain comment to its right place Imre Deak
2014-03-06 19:17 ` Jesse Barnes
2014-03-04 17:23 ` [PATCH v2 20/21] drm/i915: factor out intel_set_cpu_fifo_underrun_reporting_nolock Imre Deak
2014-03-06 19:18 ` Jesse Barnes
2014-03-06 20:46 ` Daniel Vetter
2014-03-04 17:23 ` [PATCH v2 21/21] drm/i915: power domains: add vlv power wells Imre Deak
2014-03-05 14:20 ` [PATCH v3 " Imre Deak
2014-03-06 20:29 ` [PATCH v2 " Jesse Barnes
2014-03-06 20:52 ` Daniel Vetter
2014-03-06 21:04 ` Imre Deak [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1394139844.2760.3.camel@ideak-mobl \
--to=imre.deak@intel.com \
--cc=daniel@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox