| Message ID | 20190926145621.9090-4-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DC3CO Support for TGL | expand |
On Thu, Sep 26, 2019 at 08:26:17PM +0530, Anshuman Gupta wrote: > Add target_dc_state and tgl_set_target_dc_state() API > in order to enable DC3CO state with existing DC states. > target_dc_state will enable/disable the desired DC state in > DC_STATE_EN reg when "DC Off" power well gets disable/enable. > > v2: commit log improvement. > v3: Used intel_wait_for_register to wait for DC3CO exit. [Imre] > Used gen9_set_dc_state() to allow/disallow DC3CO. [Imre] > Moved transcoder psr2 exit line enablement from tgl_allow_dc3co() > to a appropriate place haswell_crtc_enable(). [Imre] > Changed the DC3CO power well enabled call back logic as > recommended in review comments. [Imre] > v4: Used wait_for_us() instead of intel_wait_for_reg(). [Imre (IRC)] > v5: using udelay() instead of waiting for DC3CO exit status. > v6: Fixed minor unwanted change. > v7: Removed DC3CO powerwell and POWER_DOMAIN_VIDEO. > v8: Uniform checks by using only target_dc_state instead of allowed_dc_mask > in "DC off" power well callback. [Imre] > Adding "DC off" power well id to older platforms. [Imre] > Removed psr2_deep_sleep flag from tgl_set_target_dc_state. [Imre] > v9: Used switch case for target DC state in > gen9_dc_off_power_well_disable(), checking DC3CO state against > allowed DC mask, using WARN_ON() in > tgl_set_target_dc_state(). [Imre] > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Animesh Manna <animesh.manna@intel.com> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > .../drm/i915/display/intel_display_power.c | 110 ++++++++++++++++-- > .../drm/i915/display/intel_display_power.h | 2 + > drivers/gpu/drm/i915/i915_drv.h | 1 + > 3 files changed, 104 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > index 0b685c517bcb..9f787556f80d 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > @@ -785,6 +785,38 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state) > dev_priv->csr.dc_state = val & mask; > } > > +static void > +allowed_dc_mask_to_target_dc_state(struct drm_i915_private *dev_priv) > +{ > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) > + dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC6; > + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > + dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC5; > + else > + dev_priv->csr.target_dc_state = DC_STATE_DISABLE; > +} The following can be used in tgl_set_target_dc_state() as well: static u32 sanitize_target_dc_state(struct drm_i915_private *dev_priv, u32 target_dc_state) { u32 states[] = { DC_STATE_EN_UPTO_DC6, DC_STATE_EN_UPTO_DC5, DC_STATE_EN_DC3CO, }; int i; for (i = 0; i < ARRAY_SIZE(states); i++) if (target_dc_state == states[i] && (dev_priv->csr.allowed_dc_mask & states[i])) return states[i]; return DC_STATE_DISABLE; } > + > +static void tgl_enable_dc3co(struct drm_i915_private *dev_priv) > +{ > + DRM_DEBUG_KMS("Enabling DC3CO\n"); > + gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO); > +} > + > +static void tgl_disable_dc3co(struct drm_i915_private *dev_priv) > +{ > + u32 val; > + > + DRM_DEBUG_KMS("Disabling DC3CO\n"); > + val = I915_READ(DC_STATE_EN); > + val &= ~DC_STATE_DC3CO_STATUS; > + I915_WRITE(DC_STATE_EN, val); > + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > + /* > + * Delay of 200us DC3CO Exit time B.Spec 49196 > + */ > + usleep_range(200, 210); > +} > + > static void bxt_enable_dc9(struct drm_i915_private *dev_priv) > { > assert_can_enable_dc9(dev_priv); > @@ -952,7 +984,8 @@ static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv) > static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv, > struct i915_power_well *power_well) > { > - return (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0; > + return ((I915_READ(DC_STATE_EN) & DC_STATE_EN_DC3CO) == 0 && > + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0); > } > > static void gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv) > @@ -968,6 +1001,11 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv) > { > struct intel_cdclk_state cdclk_state = {}; > > + if (dev_priv->csr.target_dc_state == DC_STATE_EN_DC3CO) { > + tgl_disable_dc3co(dev_priv); > + return; > + } > + > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > dev_priv->display.get_cdclk(dev_priv, &cdclk_state); > @@ -1000,10 +1038,63 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv, > if (!dev_priv->csr.dmc_payload) > return; > > - if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) > + switch (dev_priv->csr.target_dc_state) { > + case DC_STATE_EN_DC3CO: > + tgl_enable_dc3co(dev_priv); > + break; > + case DC_STATE_EN_UPTO_DC6: > skl_enable_dc6(dev_priv); > - else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > + break; > + case DC_STATE_EN_UPTO_DC5: > gen9_enable_dc5(dev_priv); > + break; > + } > +} > + > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state) > +{ > + struct i915_power_well *power_well; > + bool dc_off_enabled; > + struct i915_power_domains *power_domains = &dev_priv->power_domains; > + > + mutex_lock(&power_domains->lock); > + power_well = lookup_power_well(dev_priv, SKL_DISP_DC_OFF); > + > + if (WARN_ON(!power_well)) > + goto unlock; > + > + /* > + * Compute the adjusted state wrt to the permisisble allowed dc mask. > + */ > + if (state != DC_STATE_EN_DC3CO || > + !(dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)) { > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) > + state = DC_STATE_EN_UPTO_DC6; > + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > + state = DC_STATE_EN_UPTO_DC5; > + else > + state = DC_STATE_DISABLE; > + } Instead of the above: state = sanitize_dc_target_state(dev_priv, state); > + > + if (state == dev_priv->csr.target_dc_state) > + goto unlock; > + > + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv, > + power_well); > + /* > + * If DC off power well is disabled, need to enable and disable the > + * DC off power well to effect target DC state. > + */ > + if (!dc_off_enabled) > + power_well->desc->ops->enable(dev_priv, power_well); > + > + dev_priv->csr.target_dc_state = state; > + > + if (!dc_off_enabled) > + power_well->desc->ops->disable(dev_priv, power_well); > + > +unlock: > + mutex_unlock(&power_domains->lock); > } > > static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv, > @@ -2951,7 +3042,7 @@ static const struct i915_power_well_desc skl_power_wells[] = { > .name = "DC off", > .domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS, > .ops = &gen9_dc_off_power_well_ops, > - .id = DISP_PW_ID_NONE, > + .id = SKL_DISP_DC_OFF, > }, > { > .name = "power well 2", > @@ -3033,7 +3124,7 @@ static const struct i915_power_well_desc bxt_power_wells[] = { > .name = "DC off", > .domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS, > .ops = &gen9_dc_off_power_well_ops, > - .id = DISP_PW_ID_NONE, > + .id = SKL_DISP_DC_OFF, > }, > { > .name = "power well 2", > @@ -3093,7 +3184,7 @@ static const struct i915_power_well_desc glk_power_wells[] = { > .name = "DC off", > .domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS, > .ops = &gen9_dc_off_power_well_ops, > - .id = DISP_PW_ID_NONE, > + .id = SKL_DISP_DC_OFF, > }, > { > .name = "power well 2", > @@ -3262,7 +3353,7 @@ static const struct i915_power_well_desc cnl_power_wells[] = { > .name = "DC off", > .domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS, > .ops = &gen9_dc_off_power_well_ops, > - .id = DISP_PW_ID_NONE, > + .id = SKL_DISP_DC_OFF, > }, > { > .name = "power well 2", > @@ -3390,7 +3481,7 @@ static const struct i915_power_well_desc icl_power_wells[] = { > .name = "DC off", > .domains = ICL_DISPLAY_DC_OFF_POWER_DOMAINS, > .ops = &gen9_dc_off_power_well_ops, > - .id = DISP_PW_ID_NONE, > + .id = SKL_DISP_DC_OFF, > }, > { > .name = "power well 2", > @@ -3623,7 +3714,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = { > .name = "DC off", > .domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS, > .ops = &gen9_dc_off_power_well_ops, > - .id = DISP_PW_ID_NONE, > + .id = SKL_DISP_DC_OFF, > }, > { > .name = "power well 2", > @@ -4056,6 +4147,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv) > dev_priv->csr.allowed_dc_mask = > get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc); > > + allowed_dc_mask_to_target_dc_state(dev_priv); and this would become: dev_priv->csr.target_dc_state = sanitize_dc_target_state(dev_priv, DC_STATE_EN_UPTO_DC6); > BUILD_BUG_ON(POWER_DOMAIN_NUM > 64); > > mutex_init(&power_domains->lock); > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h > index 737b5def7fc6..13fc705799fd 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power.h > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h > @@ -100,6 +100,7 @@ enum i915_power_well_id { > SKL_DISP_PW_MISC_IO, > SKL_DISP_PW_1, > SKL_DISP_PW_2, > + SKL_DISP_DC_OFF, > }; > > #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) > @@ -256,6 +257,7 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915); > void intel_display_power_resume_early(struct drm_i915_private *i915); > void intel_display_power_suspend(struct drm_i915_private *i915); > void intel_display_power_resume(struct drm_i915_private *i915); > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state); > > const char * > intel_display_power_domain_str(enum intel_display_power_domain domain); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index fcf7423075ef..cddc98ea9965 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -338,6 +338,7 @@ struct intel_csr { > i915_reg_t mmioaddr[20]; > u32 mmiodata[20]; > u32 dc_state; > + u32 target_dc_state; > u32 allowed_dc_mask; > intel_wakeref_t wakeref; > }; > -- > 2.21.0 >
On Fri, Sep 27, 2019 at 02:07:43PM +0300, Imre Deak wrote: > On Thu, Sep 26, 2019 at 08:26:17PM +0530, Anshuman Gupta wrote: > > Add target_dc_state and tgl_set_target_dc_state() API > > in order to enable DC3CO state with existing DC states. > > target_dc_state will enable/disable the desired DC state in > > DC_STATE_EN reg when "DC Off" power well gets disable/enable. > > > > v2: commit log improvement. > > v3: Used intel_wait_for_register to wait for DC3CO exit. [Imre] > > Used gen9_set_dc_state() to allow/disallow DC3CO. [Imre] > > Moved transcoder psr2 exit line enablement from tgl_allow_dc3co() > > to a appropriate place haswell_crtc_enable(). [Imre] > > Changed the DC3CO power well enabled call back logic as > > recommended in review comments. [Imre] > > v4: Used wait_for_us() instead of intel_wait_for_reg(). [Imre (IRC)] > > v5: using udelay() instead of waiting for DC3CO exit status. > > v6: Fixed minor unwanted change. > > v7: Removed DC3CO powerwell and POWER_DOMAIN_VIDEO. > > v8: Uniform checks by using only target_dc_state instead of allowed_dc_mask > > in "DC off" power well callback. [Imre] > > Adding "DC off" power well id to older platforms. [Imre] > > Removed psr2_deep_sleep flag from tgl_set_target_dc_state. [Imre] > > v9: Used switch case for target DC state in > > gen9_dc_off_power_well_disable(), checking DC3CO state against > > allowed DC mask, using WARN_ON() in > > tgl_set_target_dc_state(). [Imre] > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > > Cc: Animesh Manna <animesh.manna@intel.com> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > --- > > .../drm/i915/display/intel_display_power.c | 110 ++++++++++++++++-- > > .../drm/i915/display/intel_display_power.h | 2 + > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > 3 files changed, 104 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > index 0b685c517bcb..9f787556f80d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -785,6 +785,38 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state) > > dev_priv->csr.dc_state = val & mask; > > } > > > > +static void > > +allowed_dc_mask_to_target_dc_state(struct drm_i915_private *dev_priv) > > +{ > > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) > > + dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC6; > > + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > > + dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC5; > > + else > > + dev_priv->csr.target_dc_state = DC_STATE_DISABLE; > > +} > > The following can be used in tgl_set_target_dc_state() as well: > > static u32 > sanitize_target_dc_state(struct drm_i915_private *dev_priv, > u32 target_dc_state) > { > u32 states[] = { > DC_STATE_EN_UPTO_DC6, > DC_STATE_EN_UPTO_DC5, > DC_STATE_EN_DC3CO, > }; > int i; > > for (i = 0; i < ARRAY_SIZE(states); i++) > if (target_dc_state == states[i] && > (dev_priv->csr.allowed_dc_mask & states[i])) > return states[i]; > > return DC_STATE_DISABLE; > } > > > + > > +static void tgl_enable_dc3co(struct drm_i915_private *dev_priv) > > +{ > > + DRM_DEBUG_KMS("Enabling DC3CO\n"); > > + gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO); > > +} > > + > > +static void tgl_disable_dc3co(struct drm_i915_private *dev_priv) > > +{ > > + u32 val; > > + > > + DRM_DEBUG_KMS("Disabling DC3CO\n"); > > + val = I915_READ(DC_STATE_EN); > > + val &= ~DC_STATE_DC3CO_STATUS; > > + I915_WRITE(DC_STATE_EN, val); > > + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > + /* > > + * Delay of 200us DC3CO Exit time B.Spec 49196 > > + */ > > + usleep_range(200, 210); > > +} > > + > > static void bxt_enable_dc9(struct drm_i915_private *dev_priv) > > { > > assert_can_enable_dc9(dev_priv); > > @@ -952,7 +984,8 @@ static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv) > > static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv, > > struct i915_power_well *power_well) > > { > > - return (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0; > > + return ((I915_READ(DC_STATE_EN) & DC_STATE_EN_DC3CO) == 0 && > > + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0); > > } > > > > static void gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv) > > @@ -968,6 +1001,11 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv) > > { > > struct intel_cdclk_state cdclk_state = {}; > > > > + if (dev_priv->csr.target_dc_state == DC_STATE_EN_DC3CO) { > > + tgl_disable_dc3co(dev_priv); > > + return; > > + } > > + > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > dev_priv->display.get_cdclk(dev_priv, &cdclk_state); > > @@ -1000,10 +1038,63 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv, > > if (!dev_priv->csr.dmc_payload) > > return; > > > > - if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) > > + switch (dev_priv->csr.target_dc_state) { > > + case DC_STATE_EN_DC3CO: > > + tgl_enable_dc3co(dev_priv); > > + break; > > + case DC_STATE_EN_UPTO_DC6: > > skl_enable_dc6(dev_priv); > > - else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > > + break; > > + case DC_STATE_EN_UPTO_DC5: > > gen9_enable_dc5(dev_priv); > > + break; > > + } > > +} > > + > > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state) We need a documentation for exported functions. > > +{ > > + struct i915_power_well *power_well; > > + bool dc_off_enabled; > > + struct i915_power_domains *power_domains = &dev_priv->power_domains; > > + > > + mutex_lock(&power_domains->lock); > > + power_well = lookup_power_well(dev_priv, SKL_DISP_DC_OFF); > > + > > + if (WARN_ON(!power_well)) > > + goto unlock; > > + > > + /* > > + * Compute the adjusted state wrt to the permisisble allowed dc mask. > > + */ > > + if (state != DC_STATE_EN_DC3CO || > > + !(dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)) { > > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) > > + state = DC_STATE_EN_UPTO_DC6; > > + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > > + state = DC_STATE_EN_UPTO_DC5; > > + else > > + state = DC_STATE_DISABLE; > > + } > > Instead of the above: > state = sanitize_dc_target_state(dev_priv, state); > > > + > > + if (state == dev_priv->csr.target_dc_state) > > + goto unlock; > > + > > + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv, > > + power_well); > > + /* > > + * If DC off power well is disabled, need to enable and disable the > > + * DC off power well to effect target DC state. > > + */ > > + if (!dc_off_enabled) > > + power_well->desc->ops->enable(dev_priv, power_well); > > + > > + dev_priv->csr.target_dc_state = state; > > + > > + if (!dc_off_enabled) > > + power_well->desc->ops->disable(dev_priv, power_well); > > + > > +unlock: > > + mutex_unlock(&power_domains->lock); > > } > > > > static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv, > > @@ -2951,7 +3042,7 @@ static const struct i915_power_well_desc skl_power_wells[] = { > > .name = "DC off", > > .domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS, > > .ops = &gen9_dc_off_power_well_ops, > > - .id = DISP_PW_ID_NONE, > > + .id = SKL_DISP_DC_OFF, > > }, > > { > > .name = "power well 2", > > @@ -3033,7 +3124,7 @@ static const struct i915_power_well_desc bxt_power_wells[] = { > > .name = "DC off", > > .domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS, > > .ops = &gen9_dc_off_power_well_ops, > > - .id = DISP_PW_ID_NONE, > > + .id = SKL_DISP_DC_OFF, > > }, > > { > > .name = "power well 2", > > @@ -3093,7 +3184,7 @@ static const struct i915_power_well_desc glk_power_wells[] = { > > .name = "DC off", > > .domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS, > > .ops = &gen9_dc_off_power_well_ops, > > - .id = DISP_PW_ID_NONE, > > + .id = SKL_DISP_DC_OFF, > > }, > > { > > .name = "power well 2", > > @@ -3262,7 +3353,7 @@ static const struct i915_power_well_desc cnl_power_wells[] = { > > .name = "DC off", > > .domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS, > > .ops = &gen9_dc_off_power_well_ops, > > - .id = DISP_PW_ID_NONE, > > + .id = SKL_DISP_DC_OFF, > > }, > > { > > .name = "power well 2", > > @@ -3390,7 +3481,7 @@ static const struct i915_power_well_desc icl_power_wells[] = { > > .name = "DC off", > > .domains = ICL_DISPLAY_DC_OFF_POWER_DOMAINS, > > .ops = &gen9_dc_off_power_well_ops, > > - .id = DISP_PW_ID_NONE, > > + .id = SKL_DISP_DC_OFF, > > }, > > { > > .name = "power well 2", > > @@ -3623,7 +3714,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = { > > .name = "DC off", > > .domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS, > > .ops = &gen9_dc_off_power_well_ops, > > - .id = DISP_PW_ID_NONE, > > + .id = SKL_DISP_DC_OFF, > > }, > > { > > .name = "power well 2", > > @@ -4056,6 +4147,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv) > > dev_priv->csr.allowed_dc_mask = > > get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc); > > > > + allowed_dc_mask_to_target_dc_state(dev_priv); > > and this would become: > > dev_priv->csr.target_dc_state = > sanitize_dc_target_state(dev_priv, DC_STATE_EN_UPTO_DC6); > > > BUILD_BUG_ON(POWER_DOMAIN_NUM > 64); > > > > mutex_init(&power_domains->lock); > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h > > index 737b5def7fc6..13fc705799fd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h > > @@ -100,6 +100,7 @@ enum i915_power_well_id { > > SKL_DISP_PW_MISC_IO, > > SKL_DISP_PW_1, > > SKL_DISP_PW_2, > > + SKL_DISP_DC_OFF, > > }; > > > > #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) > > @@ -256,6 +257,7 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915); > > void intel_display_power_resume_early(struct drm_i915_private *i915); > > void intel_display_power_suspend(struct drm_i915_private *i915); > > void intel_display_power_resume(struct drm_i915_private *i915); > > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state); > > > > const char * > > intel_display_power_domain_str(enum intel_display_power_domain domain); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index fcf7423075ef..cddc98ea9965 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -338,6 +338,7 @@ struct intel_csr { > > i915_reg_t mmioaddr[20]; > > u32 mmiodata[20]; > > u32 dc_state; > > + u32 target_dc_state; > > u32 allowed_dc_mask; > > intel_wakeref_t wakeref; > > }; > > -- > > 2.21.0 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 27 Sep 2019, Imre Deak <imre.deak@intel.com> wrote: > On Fri, Sep 27, 2019 at 02:07:43PM +0300, Imre Deak wrote: >> On Thu, Sep 26, 2019 at 08:26:17PM +0530, Anshuman Gupta wrote: >> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state) > > We need a documentation for exported functions. And really you should make an effort to *NOT* expose platform specific functions from your C modules. Yes, we have some, but the direction should be the opposite of adding more. I'll be more strict about this going forward. We need to improve the interfaces we have. >> > @@ -256,6 +257,7 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915); >> > void intel_display_power_resume_early(struct drm_i915_private *i915); >> > void intel_display_power_suspend(struct drm_i915_private *i915); >> > void intel_display_power_resume(struct drm_i915_private *i915); >> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state); This sticks out like a sore thumb. And you're not even using the function outside of intel_display_power.h! BR, Jani. >> > >> > const char * >> > intel_display_power_domain_str(enum intel_display_power_domain domain);
On Fri, Sep 27, 2019 at 04:40:59PM +0300, Jani Nikula wrote: > On Fri, 27 Sep 2019, Imre Deak <imre.deak@intel.com> wrote: > > On Fri, Sep 27, 2019 at 02:07:43PM +0300, Imre Deak wrote: > >> On Thu, Sep 26, 2019 at 08:26:17PM +0530, Anshuman Gupta wrote: > >> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state) > > > > We need a documentation for exported functions. > > And really you should make an effort to *NOT* expose platform specific > functions from your C modules. Yes, we have some, but the direction > should be the opposite of adding more. > > I'll be more strict about this going forward. We need to improve the > interfaces we have. Yes, the original idea was to add a new power well for the DC3co power state, which would allow us to use the existing get/put interface. That doesn't work too well though, because we can have only one of the states enabled, so we went with having only one DC power well and a new API to set its target DC state. > > >> > @@ -256,6 +257,7 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915); > >> > void intel_display_power_resume_early(struct drm_i915_private *i915); > >> > void intel_display_power_suspend(struct drm_i915_private *i915); > >> > void intel_display_power_resume(struct drm_i915_private *i915); > >> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state); > > This sticks out like a sore thumb. Yep, so the rule is to have a uniform prefix for all exported functions, so we could rename this to intel_display_power_set_dc_target(), > And you're not even using the function outside of intel_display_power.h! It's used, but the use is added only later. I agree that, as commented elsewhere, new functions should be added in the same patch where they are used. > BR, > Jani. > > > >> > > >> > const char * > >> > intel_display_power_domain_str(enum intel_display_power_domain domain); > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 0b685c517bcb..9f787556f80d 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -785,6 +785,38 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state) dev_priv->csr.dc_state = val & mask; } +static void +allowed_dc_mask_to_target_dc_state(struct drm_i915_private *dev_priv) +{ + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) + dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC6; + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) + dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC5; + else + dev_priv->csr.target_dc_state = DC_STATE_DISABLE; +} + +static void tgl_enable_dc3co(struct drm_i915_private *dev_priv) +{ + DRM_DEBUG_KMS("Enabling DC3CO\n"); + gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO); +} + +static void tgl_disable_dc3co(struct drm_i915_private *dev_priv) +{ + u32 val; + + DRM_DEBUG_KMS("Disabling DC3CO\n"); + val = I915_READ(DC_STATE_EN); + val &= ~DC_STATE_DC3CO_STATUS; + I915_WRITE(DC_STATE_EN, val); + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); + /* + * Delay of 200us DC3CO Exit time B.Spec 49196 + */ + usleep_range(200, 210); +} + static void bxt_enable_dc9(struct drm_i915_private *dev_priv) { assert_can_enable_dc9(dev_priv); @@ -952,7 +984,8 @@ static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv) static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv, struct i915_power_well *power_well) { - return (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0; + return ((I915_READ(DC_STATE_EN) & DC_STATE_EN_DC3CO) == 0 && + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0); } static void gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv) @@ -968,6 +1001,11 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv) { struct intel_cdclk_state cdclk_state = {}; + if (dev_priv->csr.target_dc_state == DC_STATE_EN_DC3CO) { + tgl_disable_dc3co(dev_priv); + return; + } + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); dev_priv->display.get_cdclk(dev_priv, &cdclk_state); @@ -1000,10 +1038,63 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv, if (!dev_priv->csr.dmc_payload) return; - if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) + switch (dev_priv->csr.target_dc_state) { + case DC_STATE_EN_DC3CO: + tgl_enable_dc3co(dev_priv); + break; + case DC_STATE_EN_UPTO_DC6: skl_enable_dc6(dev_priv); - else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) + break; + case DC_STATE_EN_UPTO_DC5: gen9_enable_dc5(dev_priv); + break; + } +} + +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state) +{ + struct i915_power_well *power_well; + bool dc_off_enabled; + struct i915_power_domains *power_domains = &dev_priv->power_domains; + + mutex_lock(&power_domains->lock); + power_well = lookup_power_well(dev_priv, SKL_DISP_DC_OFF); + + if (WARN_ON(!power_well)) + goto unlock; + + /* + * Compute the adjusted state wrt to the permisisble allowed dc mask. + */ + if (state != DC_STATE_EN_DC3CO || + !(dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)) { + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) + state = DC_STATE_EN_UPTO_DC6; + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) + state = DC_STATE_EN_UPTO_DC5; + else + state = DC_STATE_DISABLE; + } + + if (state == dev_priv->csr.target_dc_state) + goto unlock; + + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv, + power_well); + /* + * If DC off power well is disabled, need to enable and disable the + * DC off power well to effect target DC state. + */ + if (!dc_off_enabled) + power_well->desc->ops->enable(dev_priv, power_well); + + dev_priv->csr.target_dc_state = state; + + if (!dc_off_enabled) + power_well->desc->ops->disable(dev_priv, power_well); + +unlock: + mutex_unlock(&power_domains->lock); } static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv, @@ -2951,7 +3042,7 @@ static const struct i915_power_well_desc skl_power_wells[] = { .name = "DC off", .domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS, .ops = &gen9_dc_off_power_well_ops, - .id = DISP_PW_ID_NONE, + .id = SKL_DISP_DC_OFF, }, { .name = "power well 2", @@ -3033,7 +3124,7 @@ static const struct i915_power_well_desc bxt_power_wells[] = { .name = "DC off", .domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS, .ops = &gen9_dc_off_power_well_ops, - .id = DISP_PW_ID_NONE, + .id = SKL_DISP_DC_OFF, }, { .name = "power well 2", @@ -3093,7 +3184,7 @@ static const struct i915_power_well_desc glk_power_wells[] = { .name = "DC off", .domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS, .ops = &gen9_dc_off_power_well_ops, - .id = DISP_PW_ID_NONE, + .id = SKL_DISP_DC_OFF, }, { .name = "power well 2", @@ -3262,7 +3353,7 @@ static const struct i915_power_well_desc cnl_power_wells[] = { .name = "DC off", .domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS, .ops = &gen9_dc_off_power_well_ops, - .id = DISP_PW_ID_NONE, + .id = SKL_DISP_DC_OFF, }, { .name = "power well 2", @@ -3390,7 +3481,7 @@ static const struct i915_power_well_desc icl_power_wells[] = { .name = "DC off", .domains = ICL_DISPLAY_DC_OFF_POWER_DOMAINS, .ops = &gen9_dc_off_power_well_ops, - .id = DISP_PW_ID_NONE, + .id = SKL_DISP_DC_OFF, }, { .name = "power well 2", @@ -3623,7 +3714,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = { .name = "DC off", .domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS, .ops = &gen9_dc_off_power_well_ops, - .id = DISP_PW_ID_NONE, + .id = SKL_DISP_DC_OFF, }, { .name = "power well 2", @@ -4056,6 +4147,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv) dev_priv->csr.allowed_dc_mask = get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc); + allowed_dc_mask_to_target_dc_state(dev_priv); BUILD_BUG_ON(POWER_DOMAIN_NUM > 64); mutex_init(&power_domains->lock); diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h index 737b5def7fc6..13fc705799fd 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.h +++ b/drivers/gpu/drm/i915/display/intel_display_power.h @@ -100,6 +100,7 @@ enum i915_power_well_id { SKL_DISP_PW_MISC_IO, SKL_DISP_PW_1, SKL_DISP_PW_2, + SKL_DISP_DC_OFF, }; #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) @@ -256,6 +257,7 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915); void intel_display_power_resume_early(struct drm_i915_private *i915); void intel_display_power_suspend(struct drm_i915_private *i915); void intel_display_power_resume(struct drm_i915_private *i915); +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state); const char * intel_display_power_domain_str(enum intel_display_power_domain domain); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index fcf7423075ef..cddc98ea9965 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -338,6 +338,7 @@ struct intel_csr { i915_reg_t mmioaddr[20]; u32 mmiodata[20]; u32 dc_state; + u32 target_dc_state; u32 allowed_dc_mask; intel_wakeref_t wakeref; };
Add target_dc_state and tgl_set_target_dc_state() API in order to enable DC3CO state with existing DC states. target_dc_state will enable/disable the desired DC state in DC_STATE_EN reg when "DC Off" power well gets disable/enable. v2: commit log improvement. v3: Used intel_wait_for_register to wait for DC3CO exit. [Imre] Used gen9_set_dc_state() to allow/disallow DC3CO. [Imre] Moved transcoder psr2 exit line enablement from tgl_allow_dc3co() to a appropriate place haswell_crtc_enable(). [Imre] Changed the DC3CO power well enabled call back logic as recommended in review comments. [Imre] v4: Used wait_for_us() instead of intel_wait_for_reg(). [Imre (IRC)] v5: using udelay() instead of waiting for DC3CO exit status. v6: Fixed minor unwanted change. v7: Removed DC3CO powerwell and POWER_DOMAIN_VIDEO. v8: Uniform checks by using only target_dc_state instead of allowed_dc_mask in "DC off" power well callback. [Imre] Adding "DC off" power well id to older platforms. [Imre] Removed psr2_deep_sleep flag from tgl_set_target_dc_state. [Imre] v9: Used switch case for target DC state in gen9_dc_off_power_well_disable(), checking DC3CO state against allowed DC mask, using WARN_ON() in tgl_set_target_dc_state(). [Imre] Cc: Jani Nikula <jani.nikula@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Animesh Manna <animesh.manna@intel.com> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- .../drm/i915/display/intel_display_power.c | 110 ++++++++++++++++-- .../drm/i915/display/intel_display_power.h | 2 + drivers/gpu/drm/i915/i915_drv.h | 1 + 3 files changed, 104 insertions(+), 9 deletions(-)