| Message ID | 20190907171443.16181-4-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DC3CO Support for TGL | expand |
On Sat, Sep 07, 2019 at 10:44:39PM +0530, Anshuman Gupta wrote: > Add max_dc_state and tgl_set_target_dc_state() API > in order to enable DC3CO state with existing DC states. > max_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. > > 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 | 111 ++++++++++++++---- > .../drm/i915/display/intel_display_power.h | 3 + > drivers/gpu/drm/i915/i915_drv.h | 1 + > 3 files changed, 95 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > index 496fa1b53ffb..83b10f61ee42 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > @@ -772,6 +772,29 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state) > dev_priv->csr.dc_state = val & mask; > } > > +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv) Should be tgl_enable_dc3co(), to match the rest of DC state helpers. > +{ > + if (!dev_priv->psr.sink_psr2_support) > + return; PSR knows when to enable DC3co, so no need to double-check that here. > + > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO) This check is out-of-place wrt. the same checks for other DC states. > + gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO); > +} > + > +static void tgl_disallow_dc3co(struct drm_i915_private *dev_priv) > +{ > + u32 val; > + > + 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 > + */ > + udelay(200); > +} > + > static void bxt_enable_dc9(struct drm_i915_private *dev_priv) > { > assert_can_enable_dc9(dev_priv); > @@ -939,7 +962,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) > @@ -955,24 +979,32 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv) > { > struct intel_cdclk_state cdclk_state = {}; > > - gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) { > + tgl_disallow_dc3co(dev_priv); > + } else { With an early return you can avoid the extra diff and make reviewing easier. > + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > - dev_priv->display.get_cdclk(dev_priv, &cdclk_state); > - /* Can't read out voltage_level so can't use intel_cdclk_changed() */ > - WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state)); > + dev_priv->display.get_cdclk(dev_priv, &cdclk_state); > + /* > + * Can't read out voltage_level so can't use > + * intel_cdclk_changed() > + */ > + WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, > + &cdclk_state)); > > - gen9_assert_dbuf_enabled(dev_priv); > + gen9_assert_dbuf_enabled(dev_priv); > > - if (IS_GEN9_LP(dev_priv)) > - bxt_verify_ddi_phy_power_wells(dev_priv); > + if (IS_GEN9_LP(dev_priv)) > + bxt_verify_ddi_phy_power_wells(dev_priv); > > - if (INTEL_GEN(dev_priv) >= 11) > - /* > - * DMC retains HW context only for port A, the other combo > - * PHY's HW context for port B is lost after DC transitions, > - * so we need to restore it manually. > - */ > - intel_combo_phy_init(dev_priv); > + if (INTEL_GEN(dev_priv) >= 11) > + /* > + * DMC retains HW context only for port A, the other > + * combo PHY's HW context for port B is lost after > + * DC transitions, so we need to restore it manually. > + */ > + intel_combo_phy_init(dev_priv); > + } > } > > static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv, > @@ -987,10 +1019,48 @@ 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) > - skl_enable_dc6(dev_priv); > - else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > - gen9_enable_dc5(dev_priv); > + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) { target_dc_state would be a better name and it shold be an exact state instead of a mask. > + tgl_allow_dc3co(dev_priv); > + } else if (dev_priv->csr.max_dc_state & DC_STATE_EN_UPTO_DC5_DC6_MASK) { > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) We should make these checks more uniform, by checking here only target_dc_state. > + skl_enable_dc6(dev_priv); > + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > + gen9_enable_dc5(dev_priv); > + } > +} > + > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state, > + bool psr2_deep_sleep) psr2_deep_sleep is a PSR internal stuff, while PSR is using the power domains framework, so let's keep the deep sleep programming in the PSR code. > +{ > + 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, TGL_DISP_DC_OFF); > + > + if (!power_well) > + goto unlock; > + > + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv, > + power_well); > + if (state == dev_priv->csr.max_dc_state) > + goto unlock; > + > + if (!dc_off_enabled) { > + /* > + * Need to disable the DC off power well to > + * effect target DC state. > + */ > + power_well->desc->ops->enable(dev_priv, power_well); > + dev_priv->csr.max_dc_state = state; > + power_well->desc->ops->disable(dev_priv, power_well); > + goto unlock; > + } > + dev_priv->csr.max_dc_state = state; > + > +unlock: > + mutex_unlock(&power_domains->lock); > } > > static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv, > @@ -3610,7 +3680,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 = TGL_DISP_DC_OFF, Let's assign this ID on all platforms for consistency. > }, > { > .name = "power well 2", > @@ -4039,6 +4109,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); > > + dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK; > 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..69ebde992342 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, > + TGL_DISP_DC_OFF, > }; > > #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) > @@ -256,6 +257,8 @@ 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, > + bool psr2_deep_sleep); > > 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 db7480831e52..999da5d2da0b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -336,6 +336,7 @@ struct intel_csr { > i915_reg_t mmioaddr[20]; > u32 mmiodata[20]; > u32 dc_state; > + u32 max_dc_state; > u32 allowed_dc_mask; > intel_wakeref_t wakeref; > }; > -- > 2.21.0 >
On 2019-09-08 at 19:44:35 +0300, Imre Deak wrote: > On Sat, Sep 07, 2019 at 10:44:39PM +0530, Anshuman Gupta wrote: Hi Imre, Thanks for reviewing the pacthes i will rework the patches. There are few comments from my side which will help to rework. > > Add max_dc_state and tgl_set_target_dc_state() API > > in order to enable DC3CO state with existing DC states. > > max_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. > > > > 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 | 111 ++++++++++++++---- > > .../drm/i915/display/intel_display_power.h | 3 + > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > 3 files changed, 95 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > index 496fa1b53ffb..83b10f61ee42 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -772,6 +772,29 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state) > > dev_priv->csr.dc_state = val & mask; > > } > > > > +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv) > > Should be tgl_enable_dc3co(), to match the rest of DC state helpers. > > > +{ > > + if (!dev_priv->psr.sink_psr2_support) > > + return; > > PSR knows when to enable DC3co, so no need to double-check that here. > > > + > > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO) > > This check is out-of-place wrt. the same checks for other DC states. > > > + gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO); > > +} > > + > > +static void tgl_disallow_dc3co(struct drm_i915_private *dev_priv) > > +{ > > + u32 val; > > + > > + 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 > > + */ > > + udelay(200); > > +} > > + > > static void bxt_enable_dc9(struct drm_i915_private *dev_priv) > > { > > assert_can_enable_dc9(dev_priv); > > @@ -939,7 +962,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) > > @@ -955,24 +979,32 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv) > > { > > struct intel_cdclk_state cdclk_state = {}; > > > > - gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) { > > + tgl_disallow_dc3co(dev_priv); > > + } else { > > With an early return you can avoid the extra diff and make reviewing > easier. > > > + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > - dev_priv->display.get_cdclk(dev_priv, &cdclk_state); > > - /* Can't read out voltage_level so can't use intel_cdclk_changed() */ > > - WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state)); > > + dev_priv->display.get_cdclk(dev_priv, &cdclk_state); > > + /* > > + * Can't read out voltage_level so can't use > > + * intel_cdclk_changed() > > + */ > > + WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, > > + &cdclk_state)); > > > > - gen9_assert_dbuf_enabled(dev_priv); > > + gen9_assert_dbuf_enabled(dev_priv); > > > > - if (IS_GEN9_LP(dev_priv)) > > - bxt_verify_ddi_phy_power_wells(dev_priv); > > + if (IS_GEN9_LP(dev_priv)) > > + bxt_verify_ddi_phy_power_wells(dev_priv); > > > > - if (INTEL_GEN(dev_priv) >= 11) > > - /* > > - * DMC retains HW context only for port A, the other combo > > - * PHY's HW context for port B is lost after DC transitions, > > - * so we need to restore it manually. > > - */ > > - intel_combo_phy_init(dev_priv); > > + if (INTEL_GEN(dev_priv) >= 11) > > + /* > > + * DMC retains HW context only for port A, the other > > + * combo PHY's HW context for port B is lost after > > + * DC transitions, so we need to restore it manually. > > + */ > > + intel_combo_phy_init(dev_priv); > > + } > > } > > > > static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv, > > @@ -987,10 +1019,48 @@ 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) > > - skl_enable_dc6(dev_priv); > > - else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > > - gen9_enable_dc5(dev_priv); > > + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) { > > target_dc_state would be a better name and it shold be an exact state > instead of a mask. > > > + tgl_allow_dc3co(dev_priv); > > + } else if (dev_priv->csr.max_dc_state & DC_STATE_EN_UPTO_DC5_DC6_MASK) { > > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) > > We should make these checks more uniform, by checking here only > target_dc_state. If i understand correctly this comment is about to have a uniform condition target_dc_state in order to decide which DC state we interested. Then we may require to check allowed_dc_mask accordingly in tgl_set_target_dc_state(), but that would not be fitting for older platforms. What is your input on this? please correct me if i am wrong here. > > > + skl_enable_dc6(dev_priv); > > + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > > + gen9_enable_dc5(dev_priv); > > + } > > +} > > + > > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state, > > + bool psr2_deep_sleep) > > psr2_deep_sleep is a PSR internal stuff, while PSR is using the power > domains framework, so let's keep the deep sleep programming in the PSR > code. I wanted not to enable/disable psr2 deep sleep for each flip flush request. Though PSR2 deep sleep is PSR internal stuff but it is H/W has tied with DC3CO. That was intention to keep psr2_deep sleep enable/diable here (Doing in Patch 6). In order to switch between DC3CO and DC5 we need to call tgl_psr2_deep_sleep_disable/enable() from outside of PSR code. please correct me if i am wrong here. Thanks , Anshuman Gupta. > > > +{ > > + 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, TGL_DISP_DC_OFF); > > + > > + if (!power_well) > > + goto unlock; > > + > > + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv, > > + power_well); > > + if (state == dev_priv->csr.max_dc_state) > > + goto unlock; > > + > > + if (!dc_off_enabled) { > > + /* > > + * Need to disable the DC off power well to > > + * effect target DC state. > > + */ > > + power_well->desc->ops->enable(dev_priv, power_well); > > + dev_priv->csr.max_dc_state = state; > > + power_well->desc->ops->disable(dev_priv, power_well); > > + goto unlock; > > + } > > + dev_priv->csr.max_dc_state = state; > > + > > +unlock: > > + mutex_unlock(&power_domains->lock); > > } > > > > static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv, > > @@ -3610,7 +3680,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 = TGL_DISP_DC_OFF, > > Let's assign this ID on all platforms for consistency. IMO that should be done in different patch, as tgl_set_target_dc_state() is only consumer of this id ? What is your opinion on this ? > > > }, > > { > > .name = "power well 2", > > @@ -4039,6 +4109,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); > > > > + dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK; > > 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..69ebde992342 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, > > + TGL_DISP_DC_OFF, > > }; > > > > #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) > > @@ -256,6 +257,8 @@ 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, > > + bool psr2_deep_sleep); > > > > 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 db7480831e52..999da5d2da0b 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -336,6 +336,7 @@ struct intel_csr { > > i915_reg_t mmioaddr[20]; > > u32 mmiodata[20]; > > u32 dc_state; > > + u32 max_dc_state; > > u32 allowed_dc_mask; > > intel_wakeref_t wakeref; > > }; > > -- > > 2.21.0 > >
On Mon, Sep 09, 2019 at 09:49:17PM +0530, Anshuman Gupta wrote: > On 2019-09-08 at 19:44:35 +0300, Imre Deak wrote: > > On Sat, Sep 07, 2019 at 10:44:39PM +0530, Anshuman Gupta wrote: > Hi Imre, > Thanks for reviewing the pacthes i will rework the patches. > There are few comments from my side which will help to rework. > > > Add max_dc_state and tgl_set_target_dc_state() API > > > in order to enable DC3CO state with existing DC states. > > > max_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. > > > > > > 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 | 111 ++++++++++++++---- > > > .../drm/i915/display/intel_display_power.h | 3 + > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > 3 files changed, 95 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > > index 496fa1b53ffb..83b10f61ee42 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > @@ -772,6 +772,29 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state) > > > dev_priv->csr.dc_state = val & mask; > > > } > > > > > > +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv) > > > > Should be tgl_enable_dc3co(), to match the rest of DC state helpers. > > > > > +{ > > > + if (!dev_priv->psr.sink_psr2_support) > > > + return; > > > > PSR knows when to enable DC3co, so no need to double-check that here. > > > > > + > > > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO) > > > > This check is out-of-place wrt. the same checks for other DC states. > > > > > + gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO); > > > +} > > > + > > > +static void tgl_disallow_dc3co(struct drm_i915_private *dev_priv) > > > +{ > > > + u32 val; > > > + > > > + 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 > > > + */ > > > + udelay(200); > > > +} > > > + > > > static void bxt_enable_dc9(struct drm_i915_private *dev_priv) > > > { > > > assert_can_enable_dc9(dev_priv); > > > @@ -939,7 +962,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) > > > @@ -955,24 +979,32 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv) > > > { > > > struct intel_cdclk_state cdclk_state = {}; > > > > > > - gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) { > > > + tgl_disallow_dc3co(dev_priv); > > > + } else { > > > > With an early return you can avoid the extra diff and make reviewing > > easier. > > > > > + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > > > - dev_priv->display.get_cdclk(dev_priv, &cdclk_state); > > > - /* Can't read out voltage_level so can't use intel_cdclk_changed() */ > > > - WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state)); > > > + dev_priv->display.get_cdclk(dev_priv, &cdclk_state); > > > + /* > > > + * Can't read out voltage_level so can't use > > > + * intel_cdclk_changed() > > > + */ > > > + WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, > > > + &cdclk_state)); > > > > > > - gen9_assert_dbuf_enabled(dev_priv); > > > + gen9_assert_dbuf_enabled(dev_priv); > > > > > > - if (IS_GEN9_LP(dev_priv)) > > > - bxt_verify_ddi_phy_power_wells(dev_priv); > > > + if (IS_GEN9_LP(dev_priv)) > > > + bxt_verify_ddi_phy_power_wells(dev_priv); > > > > > > - if (INTEL_GEN(dev_priv) >= 11) > > > - /* > > > - * DMC retains HW context only for port A, the other combo > > > - * PHY's HW context for port B is lost after DC transitions, > > > - * so we need to restore it manually. > > > - */ > > > - intel_combo_phy_init(dev_priv); > > > + if (INTEL_GEN(dev_priv) >= 11) > > > + /* > > > + * DMC retains HW context only for port A, the other > > > + * combo PHY's HW context for port B is lost after > > > + * DC transitions, so we need to restore it manually. > > > + */ > > > + intel_combo_phy_init(dev_priv); > > > + } > > > } > > > > > > static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv, > > > @@ -987,10 +1019,48 @@ 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) > > > - skl_enable_dc6(dev_priv); > > > - else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > > > - gen9_enable_dc5(dev_priv); > > > + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) { > > > > target_dc_state would be a better name and it shold be an exact state > > instead of a mask. > > > > > + tgl_allow_dc3co(dev_priv); > > > + } else if (dev_priv->csr.max_dc_state & DC_STATE_EN_UPTO_DC5_DC6_MASK) { > > > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) > > > > We should make these checks more uniform, by checking here only > > target_dc_state. > If i understand correctly this comment is about to have a uniform > condition target_dc_state in order to decide which DC state we > interested. Then we may require to check allowed_dc_mask accordingly in > tgl_set_target_dc_state(), Yes, we could check here only target_dc_state, setting that one only to values permitted by allowed_dc_mask. > but that would not be fitting for > older platforms. > What is your input on this? please correct me if i am wrong here. > > > > > + skl_enable_dc6(dev_priv); > > > + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > > > + gen9_enable_dc5(dev_priv); > > > + } > > > +} > > > + > > > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state, > > > + bool psr2_deep_sleep) > > > > psr2_deep_sleep is a PSR internal stuff, while PSR is using the power > > domains framework, so let's keep the deep sleep programming in the PSR > > code. > I wanted not to enable/disable psr2 deep sleep for each flip flush request. > Though PSR2 deep sleep is PSR internal stuff but it is H/W has tied with DC3CO. > That was intention to keep psr2_deep sleep enable/diable here (Doing in Patch 6). > In order to switch between DC3CO and DC5 we need to call > tgl_psr2_deep_sleep_disable/enable() from outside of PSR code. > please correct me if i am wrong here. I think it still belongs to the PSR code with other steps needed for DC3co setup, like the idle frame programming. PSR deep sleep could also be enabled/disabled independently of the DC state we enable (for instance enable/disable deep sleep when both DC3co and DC5 is disabled). It also needs the PSR lock, which again is internal to the PSR code. > Thanks , > Anshuman Gupta. > > > > > +{ > > > + 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, TGL_DISP_DC_OFF); > > > + > > > + if (!power_well) > > > + goto unlock; > > > + > > > + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv, > > > + power_well); > > > + if (state == dev_priv->csr.max_dc_state) > > > + goto unlock; > > > + > > > + if (!dc_off_enabled) { > > > + /* > > > + * Need to disable the DC off power well to > > > + * effect target DC state. > > > + */ > > > + power_well->desc->ops->enable(dev_priv, power_well); > > > + dev_priv->csr.max_dc_state = state; > > > + power_well->desc->ops->disable(dev_priv, power_well); > > > + goto unlock; > > > + } > > > + dev_priv->csr.max_dc_state = state; > > > + > > > +unlock: > > > + mutex_unlock(&power_domains->lock); > > > } > > > > > > static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv, > > > @@ -3610,7 +3680,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 = TGL_DISP_DC_OFF, > > > > Let's assign this ID on all platforms for consistency. > IMO that should be done in different patch, as tgl_set_target_dc_state() is > only consumer of this id ? > What is your opinion on this ? > > > > > }, > > > { > > > .name = "power well 2", > > > @@ -4039,6 +4109,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); > > > > > > + dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK; > > > 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..69ebde992342 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, > > > + TGL_DISP_DC_OFF, > > > }; > > > > > > #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) > > > @@ -256,6 +257,8 @@ 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, > > > + bool psr2_deep_sleep); > > > > > > 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 db7480831e52..999da5d2da0b 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -336,6 +336,7 @@ struct intel_csr { > > > i915_reg_t mmioaddr[20]; > > > u32 mmiodata[20]; > > > u32 dc_state; > > > + u32 max_dc_state; > > > u32 allowed_dc_mask; > > > intel_wakeref_t wakeref; > > > }; > > > -- > > > 2.21.0 > > >
On 2019-09-11 at 11:21:42 +0300, Imre Deak wrote: > On Mon, Sep 09, 2019 at 09:49:17PM +0530, Anshuman Gupta wrote: > > On 2019-09-08 at 19:44:35 +0300, Imre Deak wrote: > > > On Sat, Sep 07, 2019 at 10:44:39PM +0530, Anshuman Gupta wrote: > > Hi Imre, > > Thanks for reviewing the pacthes i will rework the patches. > > There are few comments from my side which will help to rework. > > > > Add max_dc_state and tgl_set_target_dc_state() API > > > > in order to enable DC3CO state with existing DC states. > > > > max_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. > > > > > > > > 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 | 111 ++++++++++++++---- > > > > .../drm/i915/display/intel_display_power.h | 3 + > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > 3 files changed, 95 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > > > index 496fa1b53ffb..83b10f61ee42 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > > @@ -772,6 +772,29 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state) > > > > dev_priv->csr.dc_state = val & mask; > > > > } > > > > > > > > +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv) > > > > > > Should be tgl_enable_dc3co(), to match the rest of DC state helpers. > > > > > > > +{ > > > > + if (!dev_priv->psr.sink_psr2_support) > > > > + return; > > > > > > PSR knows when to enable DC3co, so no need to double-check that here. > > > > > > > + > > > > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO) > > > > > > This check is out-of-place wrt. the same checks for other DC states. > > > > > > > + gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO); > > > > +} > > > > + > > > > +static void tgl_disallow_dc3co(struct drm_i915_private *dev_priv) > > > > +{ > > > > + u32 val; > > > > + > > > > + 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 > > > > + */ > > > > + udelay(200); > > > > +} > > > > + > > > > static void bxt_enable_dc9(struct drm_i915_private *dev_priv) > > > > { > > > > assert_can_enable_dc9(dev_priv); > > > > @@ -939,7 +962,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) > > > > @@ -955,24 +979,32 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv) > > > > { > > > > struct intel_cdclk_state cdclk_state = {}; > > > > > > > > - gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) { > > > > + tgl_disallow_dc3co(dev_priv); > > > > + } else { > > > > > > With an early return you can avoid the extra diff and make reviewing > > > easier. > > > > > > > + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); > > > > > > > > - dev_priv->display.get_cdclk(dev_priv, &cdclk_state); > > > > - /* Can't read out voltage_level so can't use intel_cdclk_changed() */ > > > > - WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state)); > > > > + dev_priv->display.get_cdclk(dev_priv, &cdclk_state); > > > > + /* > > > > + * Can't read out voltage_level so can't use > > > > + * intel_cdclk_changed() > > > > + */ > > > > + WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, > > > > + &cdclk_state)); > > > > > > > > - gen9_assert_dbuf_enabled(dev_priv); > > > > + gen9_assert_dbuf_enabled(dev_priv); > > > > > > > > - if (IS_GEN9_LP(dev_priv)) > > > > - bxt_verify_ddi_phy_power_wells(dev_priv); > > > > + if (IS_GEN9_LP(dev_priv)) > > > > + bxt_verify_ddi_phy_power_wells(dev_priv); > > > > > > > > - if (INTEL_GEN(dev_priv) >= 11) > > > > - /* > > > > - * DMC retains HW context only for port A, the other combo > > > > - * PHY's HW context for port B is lost after DC transitions, > > > > - * so we need to restore it manually. > > > > - */ > > > > - intel_combo_phy_init(dev_priv); > > > > + if (INTEL_GEN(dev_priv) >= 11) > > > > + /* > > > > + * DMC retains HW context only for port A, the other > > > > + * combo PHY's HW context for port B is lost after > > > > + * DC transitions, so we need to restore it manually. > > > > + */ > > > > + intel_combo_phy_init(dev_priv); > > > > + } > > > > } > > > > > > > > static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv, > > > > @@ -987,10 +1019,48 @@ 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) > > > > - skl_enable_dc6(dev_priv); > > > > - else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > > > > - gen9_enable_dc5(dev_priv); > > > > + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) { > > > > > > target_dc_state would be a better name and it shold be an exact state > > > instead of a mask. > > > > > > > + tgl_allow_dc3co(dev_priv); > > > > + } else if (dev_priv->csr.max_dc_state & DC_STATE_EN_UPTO_DC5_DC6_MASK) { > > > > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) > > > > > > We should make these checks more uniform, by checking here only > > > target_dc_state. > > If i understand correctly this comment is about to have a uniform > > condition target_dc_state in order to decide which DC state we > > interested. Then we may require to check allowed_dc_mask accordingly in > > tgl_set_target_dc_state(), > > Yes, we could check here only target_dc_state, setting that one only to > values permitted by allowed_dc_mask. For older platform setting target_dc_state value permitted by allowed_dc_mask i am planning to have a allowed_dc_mask_to_target_dc_state() func, which will be called from intel_power_domains_init, and initilizes target_dc_state. for tgl onwards tgl_set_target_dc_state() will do it. Hope it will be ok ? > > > but that would not be fitting for > > older platforms. > > What is your input on this? please correct me if i am wrong here. > > > > > > > + skl_enable_dc6(dev_priv); > > > > + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) > > > > + gen9_enable_dc5(dev_priv); > > > > + } > > > > +} > > > > + > > > > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state, > > > > + bool psr2_deep_sleep) > > > > > > psr2_deep_sleep is a PSR internal stuff, while PSR is using the power > > > domains framework, so let's keep the deep sleep programming in the PSR > > > code. > > I wanted not to enable/disable psr2 deep sleep for each flip flush request. > > Though PSR2 deep sleep is PSR internal stuff but it is H/W has tied with DC3CO. > > That was intention to keep psr2_deep sleep enable/diable here (Doing in Patch 6). > > In order to switch between DC3CO and DC5 we need to call > > tgl_psr2_deep_sleep_disable/enable() from outside of PSR code. > > please correct me if i am wrong here. > > I think it still belongs to the PSR code with other steps needed for > DC3co setup, like the idle frame programming. PSR deep sleep could also > be enabled/disabled independently of the DC state we enable (for > instance enable/disable deep sleep when both DC3co and DC5 is > disabled). It also needs the PSR lock, which again is internal to the > PSR code. > > > Thanks , > > Anshuman Gupta. > > > > > > > +{ > > > > + 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, TGL_DISP_DC_OFF); > > > > + > > > > + if (!power_well) > > > > + goto unlock; > > > > + > > > > + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv, > > > > + power_well); > > > > + if (state == dev_priv->csr.max_dc_state) > > > > + goto unlock; > > > > + > > > > + if (!dc_off_enabled) { > > > > + /* > > > > + * Need to disable the DC off power well to > > > > + * effect target DC state. > > > > + */ > > > > + power_well->desc->ops->enable(dev_priv, power_well); > > > > + dev_priv->csr.max_dc_state = state; > > > > + power_well->desc->ops->disable(dev_priv, power_well); > > > > + goto unlock; > > > > + } > > > > + dev_priv->csr.max_dc_state = state; > > > > + > > > > +unlock: > > > > + mutex_unlock(&power_domains->lock); > > > > } > > > > > > > > static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv, > > > > @@ -3610,7 +3680,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 = TGL_DISP_DC_OFF, > > > > > > Let's assign this ID on all platforms for consistency. > > IMO that should be done in different patch, as tgl_set_target_dc_state() is > > only consumer of this id ? > > What is your opinion on this ? > > > > > > > }, > > > > { > > > > .name = "power well 2", > > > > @@ -4039,6 +4109,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); > > > > > > > > + dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK; > > > > 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..69ebde992342 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, > > > > + TGL_DISP_DC_OFF, > > > > }; > > > > > > > > #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) > > > > @@ -256,6 +257,8 @@ 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, > > > > + bool psr2_deep_sleep); > > > > > > > > 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 db7480831e52..999da5d2da0b 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -336,6 +336,7 @@ struct intel_csr { > > > > i915_reg_t mmioaddr[20]; > > > > u32 mmiodata[20]; > > > > u32 dc_state; > > > > + u32 max_dc_state; > > > > u32 allowed_dc_mask; > > > > intel_wakeref_t wakeref; > > > > }; > > > > -- > > > > 2.21.0 > > > >
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 496fa1b53ffb..83b10f61ee42 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -772,6 +772,29 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state) dev_priv->csr.dc_state = val & mask; } +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv) +{ + if (!dev_priv->psr.sink_psr2_support) + return; + + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO) + gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO); +} + +static void tgl_disallow_dc3co(struct drm_i915_private *dev_priv) +{ + u32 val; + + 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 + */ + udelay(200); +} + static void bxt_enable_dc9(struct drm_i915_private *dev_priv) { assert_can_enable_dc9(dev_priv); @@ -939,7 +962,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) @@ -955,24 +979,32 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv) { struct intel_cdclk_state cdclk_state = {}; - gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) { + tgl_disallow_dc3co(dev_priv); + } else { + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE); - dev_priv->display.get_cdclk(dev_priv, &cdclk_state); - /* Can't read out voltage_level so can't use intel_cdclk_changed() */ - WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state)); + dev_priv->display.get_cdclk(dev_priv, &cdclk_state); + /* + * Can't read out voltage_level so can't use + * intel_cdclk_changed() + */ + WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, + &cdclk_state)); - gen9_assert_dbuf_enabled(dev_priv); + gen9_assert_dbuf_enabled(dev_priv); - if (IS_GEN9_LP(dev_priv)) - bxt_verify_ddi_phy_power_wells(dev_priv); + if (IS_GEN9_LP(dev_priv)) + bxt_verify_ddi_phy_power_wells(dev_priv); - if (INTEL_GEN(dev_priv) >= 11) - /* - * DMC retains HW context only for port A, the other combo - * PHY's HW context for port B is lost after DC transitions, - * so we need to restore it manually. - */ - intel_combo_phy_init(dev_priv); + if (INTEL_GEN(dev_priv) >= 11) + /* + * DMC retains HW context only for port A, the other + * combo PHY's HW context for port B is lost after + * DC transitions, so we need to restore it manually. + */ + intel_combo_phy_init(dev_priv); + } } static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv, @@ -987,10 +1019,48 @@ 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) - skl_enable_dc6(dev_priv); - else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) - gen9_enable_dc5(dev_priv); + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) { + tgl_allow_dc3co(dev_priv); + } else if (dev_priv->csr.max_dc_state & DC_STATE_EN_UPTO_DC5_DC6_MASK) { + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6) + skl_enable_dc6(dev_priv); + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5) + gen9_enable_dc5(dev_priv); + } +} + +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state, + bool psr2_deep_sleep) +{ + 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, TGL_DISP_DC_OFF); + + if (!power_well) + goto unlock; + + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv, + power_well); + if (state == dev_priv->csr.max_dc_state) + goto unlock; + + if (!dc_off_enabled) { + /* + * Need to disable the DC off power well to + * effect target DC state. + */ + power_well->desc->ops->enable(dev_priv, power_well); + dev_priv->csr.max_dc_state = state; + power_well->desc->ops->disable(dev_priv, power_well); + goto unlock; + } + dev_priv->csr.max_dc_state = state; + +unlock: + mutex_unlock(&power_domains->lock); } static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv, @@ -3610,7 +3680,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 = TGL_DISP_DC_OFF, }, { .name = "power well 2", @@ -4039,6 +4109,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); + dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK; 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..69ebde992342 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, + TGL_DISP_DC_OFF, }; #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A) @@ -256,6 +257,8 @@ 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, + bool psr2_deep_sleep); 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 db7480831e52..999da5d2da0b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -336,6 +336,7 @@ struct intel_csr { i915_reg_t mmioaddr[20]; u32 mmiodata[20]; u32 dc_state; + u32 max_dc_state; u32 allowed_dc_mask; intel_wakeref_t wakeref; };
Add max_dc_state and tgl_set_target_dc_state() API in order to enable DC3CO state with existing DC states. max_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. 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 | 111 ++++++++++++++---- .../drm/i915/display/intel_display_power.h | 3 + drivers/gpu/drm/i915/i915_drv.h | 1 + 3 files changed, 95 insertions(+), 20 deletions(-)