Re: [PATCH 6/8] drm/i915/skl: Deinit/init the display at suspend/resume

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 30, 2015 at 04:39:21PM +0100, Damien Lespiau wrote:
> We need to re-init the display hardware when going out of suspend. This
> includes:
> 
>   - Hooking the PCH to the reset logic
>   - Restoring CDCDLK
>   - Enabling the DDB power
> 
> Among those, only the CDCDLK one is a bit tricky. There's some
> complexity in that:
> 
>   - DPLL0 (which is the source for CDCLK) has two VCOs, each with a set
>     of supported frequencies. As eDP also uses DPLL0 for its link rate,
>     once DPLL0 is on, we restrict the possible eDP link rates the chosen
>     VCO.
>   - CDCLK also limits the bandwidth available to push pixels.
>   - My current PCU firmware never ack the frequency change request
> 
> So, as a first step, this commit restore what the BIOS set, until I can
> do more testing.
> 
> ----
> In case that's of interest for the reviewer, I've unit tested the
> function that derives the decimal frequency field:
> 
>   #include <stdio.h>
>   #include <stdint.h>
>   #include <assert.h>
> 
>   #define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
> 
>   static const struct dpll_freq {
>           unsigned int freq;
>           unsigned int decimal;
>   } freqs[] = {
>           { .freq = 308570, .decimal = 0b01001100111},
>           { .freq = 337500, .decimal = 0b01010100001},
>           { .freq = 432000, .decimal = 0b01101011110},
>           { .freq = 450000, .decimal = 0b01110000010},
>           { .freq = 540000, .decimal = 0b10000110110},
>           { .freq = 617140, .decimal = 0b10011010000},
>           { .freq = 675000, .decimal = 0b10101000100},
>   };
> 
>   static void intbits(unsigned int v)
>   {
>           int i;
> 
>           for(i = 10; i >= 0; i--)
>                   putchar('0' + ((v >> i) & 1));
>   }
> 
>   static unsigned int freq_decimal(unsigned int freq /* in kHz */)
>   {
>           unsigned int mhz, dot5;
> 
>           mhz = freq / 1000;
>           dot5 = (freq - mhz * 1000) >= 500;
> 
>           return (mhz - 1) << 1 | dot5;
>   }
> 
>   static void test_freq(const struct dpll_freq *entry)
>   {
>           unsigned int decimal = freq_decimal(entry->freq);
> 
>           printf("freq: %d, expected: ", entry->freq);
>           intbits(entry->decimal);
>           printf(", got: ");
>           intbits(decimal);
>           putchar('\n');
> 
>           assert(decimal == entry->decimal);
>   }
> 
>   int main(int argc, char **argv)
>   {
>           int i;
> 
>           for (i = 0; i < ARRAY_SIZE(freqs); i++)
>                   test_freq(&freqs[i]);
> 
>           return 0;
>   }
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  26 ++++-
>  drivers/gpu/drm/i915/i915_drv.h      |   1 +
>  drivers/gpu/drm/i915/i915_reg.h      |   3 +
>  drivers/gpu/drm/i915/intel_ddi.c     |   2 +
>  drivers/gpu/drm/i915/intel_display.c | 208 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  6 files changed, 240 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e70adfd..58db0c6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -574,6 +574,7 @@ static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
>  static int intel_suspend_complete(struct drm_i915_private *dev_priv);
>  static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
>  			      bool rpm_resume);
> +static int skl_resume_prepare(struct drm_i915_private *dev_priv);
>  
>  static int i915_drm_suspend(struct drm_device *dev)
>  {
> @@ -781,6 +782,9 @@ static int i915_drm_resume_early(struct drm_device *dev)
>  
>  	if (IS_VALLEYVIEW(dev_priv))
>  		ret = vlv_resume_prepare(dev_priv, false);
> +	else if (IS_SKYLAKE(dev_priv))
> +		ret = skl_resume_prepare(dev_priv);
> +
>  	if (ret)
>  		DRM_ERROR("Resume prepare failed: %d,Continuing resume\n", ret);
>  
> @@ -1009,6 +1013,20 @@ static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> +static int skl_suspend_complete(struct drm_i915_private *dev_priv)
> +{
> +	skl_display_suspend(dev_priv);
> +
> +	return 0;
> +}
> +
> +static int skl_resume_prepare(struct drm_i915_private *dev_priv)
> +{
> +	skl_display_resume(dev_priv);
> +
> +	return 0;
> +}
> +
>  static int bxt_suspend_complete(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> @@ -1500,7 +1518,9 @@ static int intel_runtime_resume(struct device *device)
>  	if (IS_GEN6(dev_priv))
>  		intel_init_pch_refclk(dev);
>  
> -	if (IS_BROXTON(dev))
> +	if (IS_SKYLAKE(dev))
> +		ret = skl_resume_prepare(dev_priv);
> +	else if (IS_BROXTON(dev))
>  		ret = bxt_resume_prepare(dev_priv);
>  	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		hsw_disable_pc8(dev_priv);
> @@ -1534,7 +1554,9 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	int ret;
>  
> -	if (IS_BROXTON(dev))
> +	if (IS_SKYLAKE(dev))
> +		ret = skl_suspend_complete(dev_priv);
> +	else if (IS_BROXTON(dev))
>  		ret = bxt_suspend_complete(dev_priv);
>  	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		ret = hsw_suspend_complete(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 908c124..f637667 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1659,6 +1659,7 @@ struct drm_i915_private {
>  	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
>  
>  	unsigned int fsb_freq, mem_freq, is_ddr3;
> +	unsigned int skl_boot_cdclk;
>  	unsigned int cdclk_freq;
>  	unsigned int hpll_freq;
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6d428a5..4b063a8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6647,6 +6647,9 @@ enum skl_disp_power_wells {
>  #define     GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
>  #define     GEN9_MEM_LATENCY_LEVEL_2_6_SHIFT	16
>  #define     GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT	24
> +#define   SKL_PCODE_CDCLK_CONTROL		0x7
> +#define     SKL_CDCLK_PREPARE_FOR_CHANGE	0x3
> +#define     SKL_CDCLK_READY_FOR_CHANGE		0x1
>  #define   GEN6_PCODE_WRITE_MIN_FREQ_TABLE	0x8
>  #define   GEN6_PCODE_READ_MIN_FREQ_TABLE	0x9
>  #define   GEN6_READ_OC_PARAMS			0xc
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index d5bee8b..f76bcd3 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2481,6 +2481,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>  	if (IS_SKYLAKE(dev)) {
>  		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>  			DRM_ERROR("LCPLL1 is disabled\n");
> +		else
> +			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>  	} 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 f2f4ad5..9c8338f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5420,6 +5420,213 @@ void broxton_uninit_cdclk(struct drm_device *dev)
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>  }
>  
> +/*
> + * For now, we store the CDCLK frequency set by the BIOS and restore it at
> + * resume.
> + */
> +static void skl_init_cdclk(struct drm_i915_private *dev_priv)
> +{
> +	if (!IS_SKYLAKE(dev_priv))
> +		return;
> +
> +	dev_priv->skl_boot_cdclk =
> +		dev_priv->display.get_display_clock_speed(dev_priv->dev);
> +
> +	DRM_DEBUG_DRIVER("CDCLK: %d kHz\n", dev_priv->skl_boot_cdclk);
> +}

The naming is a bit confusing wrt. bxt now.

So it looks like broxton_init_cdclk() == skl_display_resume(),
and skl_init_cdclk() just reads out the current setting and stashes it
somewhere, and there's no counterpart for that on bxt. Would be nice to
unify a bit to avoid loads of confusion.

> +
> +static const struct skl_cdclk_entry {
> +	unsigned int freq;
> +	unsigned int vco;
> +} skl_cdclk_frequencies[] = {
> +	{ .freq = 308570, .vco = 8640 },
> +	{ .freq = 337500, .vco = 8100 },
> +	{ .freq = 432000, .vco = 8640 },
> +	{ .freq = 450000, .vco = 8100 },
> +	{ .freq = 540000, .vco = 8100 },
> +	{ .freq = 617140, .vco = 8640 },
> +	{ .freq = 675000, .vco = 8100 },
> +};
> +
> +static unsigned int skl_cdclk_get_vco(unsigned int freq)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(skl_cdclk_frequencies); i++) {
> +		const struct skl_cdclk_entry *e = &skl_cdclk_frequencies[i];
> +
> +		if (e->freq == freq)
> +			return e->vco;
> +	}
> +
> +	return 8100;
> +}
> +
> +static unsigned int skl_cdlck_decimal(unsigned int freq)
> +{
> +        unsigned int mhz, dot5;
> +
> +        mhz = freq / 1000;
> +        dot5 = (freq - mhz * 1000) >= 500;
> +
> +        return (mhz - 1) << 1 | dot5;
> +}

Just '(freq - 1000) / 500' like we do for bxt?

> +
> +static void
> +skl_dpll0_enable(struct drm_i915_private *dev_priv, unsigned int required_vco)
> +{
> +	unsigned int min_freq;
> +	u32 val;
> +
> +	/* select the minimum CDCLK before enabling DPLL 0 */
> +	val = I915_READ(CDCLK_CTL);
> +	val &= ~CDCLK_FREQ_SEL_MASK | ~CDCLK_FREQ_DECIMAL_MASK;
> +	val |= CDCLK_FREQ_337_308;
> +
> +	if (required_vco == 8640)
> +		min_freq = 308570;
> +	else
> +		min_freq = 337500;
> +
> +	val = CDCLK_FREQ_337_308 | skl_cdlck_decimal(min_freq);
> +
> +	I915_WRITE(CDCLK_CTL, val);
> +	POSTING_READ(CDCLK_CTL);
> +
> +	/*
> +	 * We always enable DPLL0 with the lowest link rate possible, but still
> +	 * taking into account the VCO required to operate the eDP panel at the
> +	 * desired frequency. The usual DP link rates operate with a VCO of
> +	 * 8100 while the eDP 1.4 alternate link rates need a VCO of 8640.
> +	 * The modeset code is responsible for the selection of the exact link
> +	 * rate later on, with the constraint of choosing a frequency that
> +	 * works with required_vco.
> +	 */
> +	val = I915_READ(DPLL_CTRL1);
> +
> +	val &= ~(DPLL_CTRL1_HDMI_MODE(0) | DPLL_CTRL1_SSC(0) |
> +		 DPLL_CTRL1_LINK_RATE_MASK(0));
> +	val |= DPLL_CTRL1_OVERRIDE(0);
> +	if (required_vco == 8640)
> +		val |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080, 0);
> +	else
> +		val |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_810, 0);

Hmm. These new pll registers are very confusing. But looks correct
based on my understanding.

> +
> +	I915_WRITE(DPLL_CTRL1, val);
> +	POSTING_READ(DPLL_CTRL1);
> +
> +	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) | LCPLL_PLL_ENABLE);
> +
> +	if (wait_for(I915_READ(LCPLL1_CTL) & (1 << 30), 5))

LCPLL_PLL_LOCK

> +		DRM_ERROR("DPLL0 not locked\n");
> +}
> +
> +static void skl_set_cdclk(struct drm_i915_private *dev_priv, unsigned int freq)
> +{
> +	int ret;
> +	u32 val, freq_select, freq_decimal, pcu_ack;
> +
> +	DRM_DEBUG_DRIVER("Changing CDCLK to %dKHz\n", freq);
> +
> +	/* inform PCU we want to change CDCLK */
> +	val = SKL_CDCLK_PREPARE_FOR_CHANGE;
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	ret = sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL, &val);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +	if (ret || !(val & SKL_CDCLK_READY_FOR_CHANGE)) {
> +		DRM_ERROR("failed to inform PCU about cdclk change\n");
> +		return;
> +	}

Spec says we should keep repeating this until we get the
SKL_CDCLK_READY_FOR_CHANGE bit or 150us timeout has passed. The code
however will only do it once.

> +
> +	/* set CDCLK_CTL */
> +	switch(freq) {
> +	case 450000:
> +	case 432000:
> +		freq_select = CDCLK_FREQ_450_432;
> +		pcu_ack = 1;
> +		break;
> +	case 540000:
> +		freq_select = CDCLK_FREQ_540;
> +		pcu_ack = 2;
> +		break;
> +	case 308570:
> +	case 337500:
> +	default:
> +		freq_select = CDCLK_FREQ_337_308;
> +		pcu_ack = 0;
> +		break;
> +	case 617140:
> +	case 675000:
> +		freq_select = CDCLK_FREQ_675_617;
> +		pcu_ack = 3;
> +		break;
> +	}
> +
> +	freq_decimal = skl_cdlck_decimal(freq);
> +
> +	I915_WRITE(CDCLK_CTL, freq_select | freq_decimal);
> +	POSTING_READ(CDCLK_CTL);
> +
> +	/* inform PCU of the change */
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, pcu_ack);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +}
> +
> +void skl_display_suspend(struct drm_i915_private *dev_priv)
> +{
> +	/* disable DBUF power */
> +	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) & ~DBUF_POWER_REQUEST);
> +	POSTING_READ(DBUF_CTL);
> +
> +	udelay(10);
> +
> +	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> +		DRM_ERROR("DBuf power disable timeout\n");
> +
> +	/* disable DPLL0 */
> +	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> +	if (wait_for(!(I915_READ(LCPLL1_CTL) & (1 << 30)), 1))

LCPLL_PLL_LOCK

> +		DRM_ERROR("Couldn't disable DPLL0\n");
> +
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> +}
> +
> +void skl_display_resume(struct drm_i915_private *dev_priv)
> +{
> +	u32 val;
> +	unsigned int required_vco;
> +
> +	/* enable PCH reset handshake */
> +	val = I915_READ(HSW_NDE_RSTWRN_OPT);
> +	I915_WRITE(HSW_NDE_RSTWRN_OPT, val & RESET_PCH_HANDSHAKE_ENABLE);

s/&/|/

> +
> +	/* enable PG1 and Misc I/O */
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +
> +	/* DPLL0 already enabed !? */
> +	if (I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE) {
> +		DRM_DEBUG_DRIVER("DPLL0 already running\n");
> +		return;
> +	}
> +
> +	/* enable DPLL0 */
> +	required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk);
> +	skl_dpll0_enable(dev_priv, required_vco);
> +
> +	/* set CDCLK to the frequency the BIOS chose */
> +	skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk);
> +
> +	/* enable DBUF power */
> +	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> +	POSTING_READ(DBUF_CTL);
> +
> +	udelay(10);
> +
> +	if (!(I915_READ(DBUF_CTL) & DBUF_POWER_STATE))
> +		DRM_ERROR("DBuf power enable timeout\n");
> +}
> +
>  /* returns HPLL frequency in kHz */
>  static int valleyview_get_vco(struct drm_i915_private *dev_priv)
>  {
> @@ -14573,6 +14780,7 @@ void intel_modeset_init(struct drm_device *dev)
>  
>  	intel_init_dpio(dev);
>  
> +	skl_init_cdclk(dev_priv);

Maybe just stick that into the ddi pll init code?

>  	intel_shared_dpll_init(dev);
>  
>  	/* Just disable it once at startup */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 43fe003..67275a1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1123,6 +1123,8 @@ void broxton_ddi_phy_init(struct drm_device *dev);
>  void broxton_ddi_phy_uninit(struct drm_device *dev);
>  void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>  void bxt_disable_dc9(struct drm_i915_private *dev_priv);
> +void skl_display_suspend(struct drm_i915_private *dev_priv);
> +void skl_display_resume(struct drm_i915_private *dev_priv);
>  void intel_dp_get_m_n(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *pipe_config);
>  void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx