Re: [PATCH 03/10] drm/i915: Make turning on/off PW1 and Misc I/O part of the init/fini sequences

[Imre Deak <imre.deak@intel.com>]

On ke, 2015-11-04 at 19:24 +0200, Imre Deak wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> Before this patch, we used the intel_display_power_{get,put}
> functions
> to make sure the PW1 and Misc I/O power wells were enabled all the
> time while LCPLL was enabled. We called a get() at
> intel_ddi_pll_init() when we discovered that LCPLL was enabled, then
> we would call put/get at skl_{un,}init_cdclk().
> 
> The problem is that skl_uninit_cdclk() is indirectly called by
> intel_runtime_suspend(). So it will only release its power well
> _after_ we already decided to runtime suspend. But since we only
> decide to runtime suspend after all power wells and refcounts are
> released, that basically means we will never decide to runtime
> suspend.
> 
> So what this patch does to fix that problem is move the PW1 + Misc
> I/O
> power well handling out of the runtime PM mechanism: instead of
> calling intel_display_power_{get_put} - functions that touch the
> refcount -, we'll call the low level intel_power_well_{en,dis}able,
> which don't change the refcount. This way, it is now possible for the
> refcount to actually reach zero, and we'll now start runtime
> suspending/resuming.
> 
> v2 (from Paulo):
>   - Write a commit message since the original patch left it empty.
>   - Rebase after the intel_power_well_{en,dis}able rename.
>   - Use lookup_power_well() instead of hardcoded indexes.
> 
> Testcase: igt/pm_rpm/rte (and every other rpm test)
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92211
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92605

> ---
>  drivers/gpu/drm/i915/intel_ddi.c        |  4 ++--
>  drivers/gpu/drm/i915/intel_display.c    |  5 +++--
>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 29
> +++++++++++++++++++++++++++++
>  4 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index b164122..cacc406 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2951,8 +2951,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>  		dev_priv->skl_boot_cdclk = cdclk_freq;
>  		if (skl_sanitize_cdclk(dev_priv))
>  			DRM_DEBUG_KMS("Sanitized cdclk programmed by
> pre-os\n");
> -		else
> -			intel_display_power_get(dev_priv,
> POWER_DOMAIN_PLLS);
> +		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> +			DRM_ERROR("LCPLL1 is disabled\n");
>  	} else if (IS_BROXTON(dev)) {
>  		broxton_init_cdclk(dev);
>  		broxton_ddi_phy_init(dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 7b3cfb6..ef2ebcd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5702,7 +5702,8 @@ void skl_uninit_cdclk(struct drm_i915_private
> *dev_priv)
>  			DRM_ERROR("Couldn't disable DPLL0\n");
>  	}
>  
> -	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> +	/* disable PG1 and Misc I/O */
> +	skl_pw1_misc_io_fini(dev_priv);
>  }
>  
>  void skl_init_cdclk(struct drm_i915_private *dev_priv)
> @@ -5715,7 +5716,7 @@ void skl_init_cdclk(struct drm_i915_private
> *dev_priv)
>  	I915_WRITE(HSW_NDE_RSTWRN_OPT, val |
> RESET_PCH_HANDSHAKE_ENABLE);
>  
>  	/* enable PG1 and Misc I/O */
> -	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +	skl_pw1_misc_io_init(dev_priv);
>  
>  	/* DPLL0 not enabled (happens on early BIOS versions) */
>  	if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 5bc744a..3d9d1d3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1381,6 +1381,8 @@ void intel_psr_single_frame_update(struct
> drm_device *dev,
>  int intel_power_domains_init(struct drm_i915_private *);
>  void intel_power_domains_fini(struct drm_i915_private *);
>  void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
> +void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv);
> +void skl_pw1_misc_io_fini(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_enable(struct drm_i915_private *dev_priv);
>  
>  bool intel_display_power_is_enabled(struct drm_i915_private
> *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 868c0f2..16691a3 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1783,6 +1783,35 @@ static struct i915_power_well
> skl_power_wells[] = {
>  	},
>  };
>  
> +void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_power_well *well;
> +
> +	if (!IS_SKYLAKE(dev_priv))
> +		return;
> +
> +	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
> +	intel_power_well_enable(dev_priv, well);
> +
> +	well = lookup_power_well(dev_priv, SKL_DISP_PW_MISC_IO);
> +	intel_power_well_enable(dev_priv, well);
> +}
> +
> +void skl_pw1_misc_io_fini(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_power_well *well;
> +
> +	if (!IS_SKYLAKE(dev_priv))
> +		return;
> +
> +	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
> +	intel_power_well_disable(dev_priv, well);
> +
> +	well = lookup_power_well(dev_priv, SKL_DISP_PW_MISC_IO);
> +	intel_power_well_disable(dev_priv, well);
> +
> +}
> +
>  static struct i915_power_well bxt_power_wells[] = {
>  	{
>  		.name = "always-on",
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx