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.jakobs...@linux.intel.com> > Signed-off-by: Imre Deak <imre.d...@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. > + 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.enable_dc); > > BUILD_BUG_ON(POWER_DOMAIN_NUM > 31); > > -- > 2.5.0 > -- --- Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx