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@xxxxxxxxx>
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Animesh Manna <animesh.manna@xxxxxxxxx>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> > ---
> > .../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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
