Re: [PATCH v3 10/12] drm/i915/gen9: Turn DC handling into a power well

Imre Deak
2015-11-16T19:28:44Z

On ma, 2015-11-16 at 15:01 +0100, Patrik Jakobsson wrote:
> Handle DC off as a power well where enabling the power well will prevent
> the DMC to enter selected DC states (required around modesets and Aux
> A). Disabling the power well will allow DC states again. For now the
> highest DC state is DC6 for Skylake and DC5 for Broxton but will be
> configurable for Skylake in a later patch.
>
> v2: Check both DC5 and DC6 bits in power well enabled function (Ville)
> v3:
> - Remove unneeded DC_OFF case in skl_set_power_well() (Imre)
> - Add PW2 dependency to DC_OFF (Imre)
>
> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |   6 --
>  drivers/gpu/drm/i915/i915_reg.h         |   1 +
>  drivers/gpu/drm/i915/intel_display.c    |   6 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 111 +++++++++++++++++++++++---------
>  4 files changed, 89 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5a63f9a..0c7f435 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1072,9 +1072,6 @@ static int i915_pm_resume(struct device *dev)
>
>  static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>  {
> - if (dev_priv->csr.dmc_payload)
> - skl_enable_dc6(dev_priv);
> -
>   return 0;
>  }
>
> @@ -1119,9 +1116,6 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
>
>  static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>  {
> - if (dev_priv->csr.dmc_payload)
> - skl_disable_dc6(dev_priv);
> -
>   return 0;
>  }
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0f3849f..b6a3525 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -638,6 +638,7 @@ enum skl_disp_power_wells {
>
>   /* Not actual bit groups. Used as IDs for lookup_power_well() */
>   SKL_DISP_PW_ALWAYS_ON,
> + SKL_DISP_PW_DC_OFF,
>  };
>
>  #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2))
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 978b1b9..21385a0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13323,6 +13323,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>   to_intel_crtc_state(crtc->state)->update_pipe;
>   unsigned long put_domains = 0;
>
> + if (modeset)
> + intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> +
>   if (modeset && crtc->state->active) {
>   update_scanline_offset(to_intel_crtc(crtc));
>   dev_priv->display.crtc_enable(crtc);
> @@ -13346,6 +13349,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>   modeset_put_power_domains(dev_priv, put_domains);
>
>   intel_post_plane_update(intel_crtc);
> +
> + if (modeset)
> + intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
>   }
>
>   /* FIXME: add subpixel order */
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index edf753e..b6ee48e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,9 +49,6 @@
>   * present for a given platform.
>   */
>
> -#define GEN9_ENABLE_DC5(dev) 0
> -#define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev)
> -
>  #define for_each_power_well(i, power_well, domain_mask, power_domains) \
>   for (i = 0; \
>        i < (power_domains)->power_well_count && \
> @@ -309,9 +306,15 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>  #define SKL_DISPLAY_DDI_D_POWER_DOMAINS ( \
>   BIT(POWER_DOMAIN_PORT_DDI_D_LANES) | \
>   BIT(POWER_DOMAIN_INIT))
> +#define SKL_DISPLAY_DC_OFF_POWER_DOMAINS ( \
> + SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \
> + BIT(POWER_DOMAIN_MODESET) | \
> + BIT(POWER_DOMAIN_AUX_A) | \
> + BIT(POWER_DOMAIN_INIT))
>  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS ( \
>   (POWER_DOMAIN_MASK & ~( \
> - SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS)) | \
> + SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \
> + SKL_DISPLAY_DC_OFF_POWER_DOMAINS)) | \
>   BIT(POWER_DOMAIN_INIT))
>
>  #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS ( \
> @@ -339,6 +342,11 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>   BIT(POWER_DOMAIN_AUX_A) | \
>   BIT(POWER_DOMAIN_PLLS) | \
>   BIT(POWER_DOMAIN_INIT))
> +#define BXT_DISPLAY_DC_OFF_POWER_DOMAINS ( \
> + BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS | \
> + BIT(POWER_DOMAIN_MODESET) | \
> + BIT(POWER_DOMAIN_AUX_A) | \
> + BIT(POWER_DOMAIN_INIT))
>  #define BXT_DISPLAY_ALWAYS_ON_POWER_DOMAINS ( \
>   (POWER_DOMAIN_MASK & ~(BXT_DISPLAY_POWERWELL_1_POWER_DOMAINS | \
>   BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS)) | \
> @@ -486,15 +494,6 @@ static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
>   gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
>  }
>
> -static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
> -{
> - assert_can_disable_dc5(dev_priv);
> -
> - DRM_DEBUG_KMS("Disabling DC5\n");
> -
> - gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> -}
> -
>  static void assert_can_enable_dc6(struct drm_i915_private *dev_priv)
>  {
>   struct drm_device *dev = dev_priv->dev;
> @@ -522,6 +521,14 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
>     "DC6 already programmed to be disabled.\n");
>  }
>
> +static void gen9_disable_dc5_dc6(struct drm_i915_private *dev_priv)
> +{
> + assert_can_disable_dc5(dev_priv);
> + assert_can_disable_dc6(dev_priv);
> +
> + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> +}
> +
>  void skl_enable_dc6(struct drm_i915_private *dev_priv)
>  {
>   assert_can_enable_dc6(dev_priv);
> @@ -589,17 +596,13 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   "Invalid for power well status to be enabled, unless done by the BIOS, \
>   when request is to disable!\n");
>   if (power_well->data == SKL_DISP_PW_2) {
> - if (GEN9_ENABLE_DC5(dev))
> - gen9_disable_dc5(dev_priv);
> - if (SKL_ENABLE_DC6(dev)) {
> - /*
> -  * DDI buffer programming unnecessary during driver-load/resume
> -  * as it's already done during modeset initialization then.
> -  * It's also invalid here as encoder list is still uninitialized.
> -  */
> - if (!dev_priv->power_domains.initializing)
> - intel_prepare_ddi(dev);
> - }
> + /*
> +  * DDI buffer programming unnecessary during driver-load/resume
> +  * as it's already done during modeset initialization then.
> +  * It's also invalid here as encoder list is still uninitialized.
> +  */
> + if (!dev_priv->power_domains.initializing)
> + intel_prepare_ddi(dev);
>   }
>   I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
>   }
> @@ -617,10 +620,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   I915_WRITE(HSW_PWR_WELL_DRIVER, tmp & ~req_mask);
>   POSTING_READ(HSW_PWR_WELL_DRIVER);
>   DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> -
> - if (GEN9_ENABLE_DC5(dev) &&
> - power_well->data == SKL_DISP_PW_2)
> - gen9_enable_dc5(dev_priv);
>   }
>   }
>
> @@ -695,6 +694,40 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
>   skl_set_power_well(dev_priv, power_well, false);
>  }
>
> +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;
> +}
> +
> +static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> +  struct i915_power_well *power_well)
> +{
> + gen9_disable_dc5_dc6(dev_priv);
> +}
> +
> +static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
> +   struct i915_power_well *power_well)
> +{
> + if (IS_SKYLAKE(dev_priv))
> + skl_enable_dc6(dev_priv);
> + else
> + gen9_enable_dc5(dev_priv);
> +}
> +
> +static void gen9_dc_off_power_well_sync_hw(struct drm_i915_private *dev_priv,
> +   struct i915_power_well *power_well)
> +{
> + if (power_well->count > 0) {
> + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> + } else {
> + if (IS_SKYLAKE(dev_priv))
> + gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
> + else
> + gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
> + }
> +}
> +
>  static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
>      struct i915_power_well *power_well)
>  {
> @@ -1517,6 +1550,13 @@ static const struct i915_power_well_ops skl_power_well_ops = {
>   .is_enabled = skl_power_well_enabled,
>  };
>
> +static const struct i915_power_well_ops gen9_dc_off_power_well_ops = {
> + .sync_hw = gen9_dc_off_power_well_sync_hw,
> + .enable = gen9_dc_off_power_well_enable,
> + .disable = gen9_dc_off_power_well_disable,
> + .is_enabled = gen9_dc_off_power_well_enabled,
> +};
> +
>  static struct i915_power_well hsw_power_wells[] = {
>   {
>   .name = "always-on",
> @@ -1691,6 +1731,12 @@ static struct i915_power_well skl_power_wells[] = {
>   .data = SKL_DISP_PW_MISC_IO,
>   },
>   {
> + .name = "DC off",
> + .domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
> + .ops = &gen9_dc_off_power_well_ops,
> + .data = SKL_DISP_PW_DC_OFF,
> + },
> + {
>   .name = "power well 2",
>   .domains = SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS,
>   .ops = &skl_power_well_ops,
> @@ -1769,7 +1815,14 @@ static struct i915_power_well bxt_power_wells[] = {
>   .domains = BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS,
>   .ops = &skl_power_well_ops,
>   .data = SKL_DISP_PW_2,
> - }
> + },
> + {
> + .name = "DC off",
> + .domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
> + .ops = &gen9_dc_off_power_well_ops,
> + .data = SKL_DISP_PW_DC_OFF,
> + },
> +

You forgot to move this one. With that fixed the patch looks ok and my
R-b holds.

>  };
>
>  #define set_power_wells(power_domains, __power_wells) ({ \