On ma, 2016-02-29 at 14:02 +0100, Patrik Jakobsson wrote:
> On Wed, Feb 24, 2016 at 07:57:44PM +0200, Imre Deak wrote:
> > We can simplify the conditions selecting the target DC state during
> > runtime by calculating the allowed DC states in advance during
> > driver
> > loading. This also makes it easier to disable DC states depending
> > on the
> > i915.disable_power_well module option, added in the next patch.
> >
> > CC: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 74
> > +++++++++++++++++++++++----------
> > 2 files changed, 54 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 9e76bfc..b563de5 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -750,6 +750,7 @@ struct intel_csr {
> > i915_reg_t mmioaddr[8];
> > uint32_t mmiodata[8];
> > uint32_t dc_state;
> > + uint32_t allowed_dc_mask;
> > };
> >
> > #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 8276dc2..88df99e 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -538,12 +538,8 @@ static void gen9_set_dc_state(struct
> > drm_i915_private *dev_priv, uint32_t state)
> > else
> > mask |= DC_STATE_EN_UPTO_DC6;
> >
> > - WARN_ON_ONCE(state & ~mask);
> > -
> > - if (i915.enable_dc == 0)
> > - state = DC_STATE_DISABLE;
> > - else if (i915.enable_dc == 1 && state >
> > DC_STATE_EN_UPTO_DC5)
> > - state = DC_STATE_EN_UPTO_DC5;
> > + if (WARN_ON_ONCE(state & ~dev_priv->csr.allowed_dc_mask))
> > + state &= dev_priv->csr.allowed_dc_mask;
> >
> > val = I915_READ(DC_STATE_EN);
> > DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n",
> > @@ -659,8 +655,7 @@ static void gen9_disable_dc5_dc6(struct
> > drm_i915_private *dev_priv)
> > {
> > assert_can_disable_dc5(dev_priv);
> >
> > - if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> > - i915.enable_dc != 0 && i915.enable_dc != 1)
> > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> > assert_can_disable_dc6(dev_priv);
> >
> > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > @@ -839,26 +834,19 @@ static void
> > gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> > static void gen9_dc_off_power_well_disable(struct drm_i915_private
> > *dev_priv,
> > struct i915_power_well
> > *power_well)
> > {
> > - if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> > - i915.enable_dc != 0 && i915.enable_dc != 1)
> > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> > skl_enable_dc6(dev_priv);
> > - else
> > + else if (dev_priv->csr.allowed_dc_mask &
> > DC_STATE_EN_UPTO_DC5)
> > gen9_enable_dc5(dev_priv);
> > }
> >
> > static void gen9_dc_off_power_well_sync_hw(struct drm_i915_private
> > *dev_priv,
> > struct i915_power_well
> > *power_well)
> > {
> > - if (power_well->count > 0) {
> > - gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > - } else {
> > - if ((IS_SKYLAKE(dev_priv) ||
> > IS_KABYLAKE(dev_priv)) &&
> > - i915.enable_dc != 0 &&
> > - i915.enable_dc != 1)
> > - gen9_set_dc_state(dev_priv,
> > DC_STATE_EN_UPTO_DC6);
> > - else
> > - gen9_set_dc_state(dev_priv,
> > DC_STATE_EN_UPTO_DC5);
> > - }
> > + if (power_well->count > 0)
> > + gen9_dc_off_power_well_enable(dev_priv,
> > power_well);
> > + else
> > + gen9_dc_off_power_well_disable(dev_priv,
> > power_well);
> > }
> >
> > static void i9xx_always_on_power_well_noop(struct drm_i915_private
> > *dev_priv,
> > @@ -2023,6 +2011,48 @@ sanitize_disable_power_well_option(const
> > struct drm_i915_private *dev_priv,
> > return 1;
> > }
> >
> > +static uint32_t get_allowed_dc_mask(const struct drm_i915_private
> > *dev_priv,
> > + int enable_dc)
> > +{
> > + uint32_t mask = 0;
> > +
> > + /*
> > + * DC9 has a separate HW flow from the rest of the DC
> > states, not
> > + * depending on the DMC firmware. It's needed by system
> > + * suspend/resume, so allow it unconditionally.
> > + */
> > + if (IS_BROXTON(dev_priv))
> > + mask |= DC_STATE_EN_DC9;
> > +
> > + if (!enable_dc)
> > + return mask;
> > +
> > + if (!HAS_CSR(dev_priv))
> > + return mask;
> > +
> > + mask |= DC_STATE_EN_UPTO_DC5;
> > + if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> > + mask |= DC_STATE_EN_UPTO_DC6;
> > + } else if (!IS_BROXTON(dev_priv)) {
> > + MISSING_CASE(INTEL_DEVID(dev_priv));
> > + mask = 0;
> > + }
> > +
> > + switch (enable_dc) {
> > + case 1:
> > + mask &= ~DC_STATE_EN_UPTO_DC6;
> > + break;
> > + case 2:
>
> Should we make some noise here on BXT? Just a thought, not sure it's
> useful.
Yes, I added this now. I'll resend the whole patchset with this change
since patchwork didn't pick up the first version for testing.
--Imre
>
> > + case -1:
> > + break;
> > + default:
> > + DRM_ERROR("Unexpected value for enable_dc (%d)\n",
> > enable_dc);
> > + break;
> > + }
> > +
> > + return mask;
> > +}
> > +
> > #define set_power_wells(power_domains, __power_wells) ({
> > \
> > (power_domains)->power_wells = (__power_wells);
> > \
> > (power_domains)->power_well_count =
> > ARRAY_SIZE(__power_wells); \
> > @@ -2041,6 +2071,8 @@ int intel_power_domains_init(struct
> > drm_i915_private *dev_priv)
> >
> > i915.disable_power_well =
> > sanitize_disable_power_well_option(dev_priv,
> > i915.disable_
> > power_well);
> > + dev_priv->csr.allowed_dc_mask =
> > get_allowed_dc_mask(dev_priv,
> > + i915.e
> > nable_dc);
> >
> > BUILD_BUG_ON(POWER_DOMAIN_NUM > 31);
> >
> > --
> > 2.5.0
> >
>