Re: [PATCH 02/21] drm/i915/skl: SKL CDCLK change on modeset tracking VCO

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

On pe, 2016-05-13 at 23:41 +0300, ville.syrjala@linux.intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> WARNING: Using ChromeOS with an eDP panel and a 4K@60 DP monitor connected
> to DDI1 the system will hard hang during a cold boot. Occurs when DDI1
> is enabled when the cdclk is less then required. DP connected to DDI2
> and HPD on either port works correctly.
> 
> Set cdclk based on the max required pixel clock based on VCO
> selected. Track boot vco instead of boot cdclk.
> 
> The vco is now tracked at the atomic level and all CRTCs updated if
> the required vco is changed. Not tested with eDP v1.4 panels that
> require 8640 vco due to availability.
> 
> V1: initial version
> V2: add vco tracking in intel_dp_compute_config(), rename
> skl_boot_cdclk.
> V3: rebase, V2 feedback not possible as encoders are not aware of
> atomic.
> V4: track target vco is atomic state. modeset all CRTCs if vco changes
> V5: rename atomic variable, cleaner if/else logic, use existing vco if
>       encoder does not return a new vco value. check_patch.pl cleanup
> V6: simplify logic in intel_modeset_checks.
> V7: reorder an IF for readability and whitespace fix.
> V8: use dev_cdclk for tracking new cdclk during atomic
> V9: correctly handle vco 8640 when crtcs==0
> V10: Clean up if else in crtcs==0
> V11: Rebase for new intel_dpll_mgr.c
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> [vsyrjala: rebased due to churn]
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

It has R-b already, but since I went through it in any case:
Reviewed-by: Imre Deak <imre.deak@intel.com>

A few notes below, none of them are about actual problems.

> ---
>  drivers/gpu/drm/i915/i915_drv.h       |   2 +-
>  drivers/gpu/drm/i915/intel_display.c  | 109 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_dpll_mgr.c |   9 +--
>  drivers/gpu/drm/i915/intel_drv.h      |   4 ++
>  4 files changed, 104 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1ba614193cc9..b319da970c8a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1820,7 +1820,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 skl_vco_freq;
>  	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
>  	unsigned int max_dotclk_freq;
>  	unsigned int rawclk_freq;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cc9a8b42fbc6..41fe18c4b761 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5540,7 +5540,7 @@ static const struct skl_cdclk_entry {
>  	{ .freq = 675000, .vco = 8100 },
>  };
>  
> -static unsigned int skl_cdclk_get_vco(unsigned int freq)
> +unsigned int skl_cdclk_get_vco(unsigned int freq)
>  {
>  	unsigned int i;
>  
> @@ -5698,17 +5698,21 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>  
>  void skl_init_cdclk(struct drm_i915_private *dev_priv)
>  {
> -	unsigned int vco;
> +	unsigned int cdclk;
>  
>  	/* DPLL0 not enabled (happens on early BIOS versions) */
>  	if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) {
>  		/* enable DPLL0 */
> -		vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk);
> -		skl_dpll0_enable(dev_priv, vco);
> +		if (dev_priv->skl_vco_freq != 8640)
> +			dev_priv->skl_vco_freq = 8100;

This seems redundant, VCO can only be either of the above two, but you
change/fix this later in the series, so it's ok.

> +		skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq);
> +		cdclk = ((dev_priv->skl_vco_freq == 8100) ? 337500 : 308570);
> +	} else {
> +		cdclk = dev_priv->cdclk_freq;
>  	}
>  
> -	/* set CDCLK to the frequency the BIOS chose */
> -	skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk);
> +	/* set CDCLK to the lowest frequency, Modeset follows */
> +	skl_set_cdclk(dev_priv, cdclk);
>  
>  	/* enable DBUF power */
>  	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> @@ -5724,7 +5728,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  {
>  	uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
>  	uint32_t cdctl = I915_READ(CDCLK_CTL);
> -	int freq = dev_priv->skl_boot_cdclk;
> +	int freq = dev_priv->cdclk_freq;
>  
>  	/*
>  	 * check if the pre-os intialized the display
> @@ -5748,11 +5752,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  		/* All well; nothing to sanitize */
>  		return false;
>  sanitize:
> -	/*
> -	 * As of now initialize with max cdclk till
> -	 * we get dynamic cdclk support
> -	 * */
> -	dev_priv->skl_boot_cdclk = dev_priv->max_cdclk_freq;
> +
>  	skl_init_cdclk(dev_priv);
>  
>  	/* we did have to sanitize */
> @@ -9719,6 +9719,73 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>  	broadwell_set_cdclk(dev, req_cdclk);
>  }
>  
> +static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> +{
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> +	const int max_pixclk = ilk_max_pixel_rate(state);
> +	int cdclk;
> +
> +	/*
> +	 * FIXME should also account for plane ratio
> +	 * once 64bpp pixel formats are supported.
> +	 */
> +
> +	if (intel_state->cdclk_pll_vco == 8640) {
> +		/* vco 8640 */
> +		if (max_pixclk > 540000)
> +			cdclk = 617140;
> +		else if (max_pixclk > 432000)
> +			cdclk = 540000;
> +		else if (max_pixclk > 308570)
> +			cdclk = 432000;
> +		else
> +			cdclk = 308570;
> +	} else {
> +		/* VCO 8100 */
> +		if (max_pixclk > 540000)
> +			cdclk = 675000;
> +		else if (max_pixclk > 450000)
> +			cdclk = 540000;
> +		else if (max_pixclk > 337500)
> +			cdclk = 450000;
> +		else
> +			cdclk = 337500;
> +	}
> +
> +	/*
> +	 * FIXME move the cdclk caclulation to
> +	 * compute_config() so we can fail gracegully.
> +	 */
> +	if (cdclk > dev_priv->max_cdclk_freq) {
> +		DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> +			  cdclk, dev_priv->max_cdclk_freq);
> +		cdclk = dev_priv->max_cdclk_freq;
> +	}
> +
> +	intel_state->cdclk = intel_state->dev_cdclk = cdclk;
> +	if (!intel_state->active_crtcs)
> +		intel_state->dev_cdclk = ((intel_state->cdclk_pll_vco == 8640) ?
> +					   308570 : 337500);
> +
> +
> +	return 0;
> +}
> +
> +static void skl_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> +{
> +	struct drm_device *dev = old_state->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned int req_cdclk = to_intel_atomic_state(old_state)->dev_cdclk;
> +
> +	/*
> +	 * FIXME disable/enable PLL should wrap set_cdclk()
> +	 */
> +	skl_set_cdclk(dev_priv, req_cdclk);
> +
> +	dev_priv->skl_vco_freq = to_intel_atomic_state(old_state)->cdclk_pll_vco;
> +}
> +
>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>  				      struct intel_crtc_state *crtc_state)
>  {
> @@ -13283,9 +13350,15 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
>  	if (dev_priv->display.modeset_calc_cdclk) {
> +		if (!intel_state->cdclk_pll_vco)
> +			intel_state->cdclk_pll_vco = dev_priv->skl_vco_freq;
> +
>  		ret = dev_priv->display.modeset_calc_cdclk(state);
> +		if (ret < 0)
> +			return ret;
>  
> -		if (!ret && intel_state->dev_cdclk != dev_priv->cdclk_freq)
> +		if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
> +		    intel_state->cdclk_pll_vco != dev_priv->skl_vco_freq)
>  			ret = intel_modeset_all_pipes(state);
>  
>  		if (ret < 0)
> @@ -13626,7 +13699,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>  
>  		if (dev_priv->display.modeset_commit_cdclk &&
> -		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
> +		    (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
> +		     intel_state->cdclk_pll_vco != dev_priv->skl_vco_freq))
>  			dev_priv->display.modeset_commit_cdclk(state);
>  
>  		intel_modeset_verify_disabled(dev);
> @@ -15041,6 +15115,11 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  			broxton_modeset_commit_cdclk;
>  		dev_priv->display.modeset_calc_cdclk =
>  			broxton_modeset_calc_cdclk;
> +	} else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> +		dev_priv->display.modeset_commit_cdclk =
> +			skl_modeset_commit_cdclk;
> +		dev_priv->display.modeset_calc_cdclk =
> +			skl_modeset_calc_cdclk;
>  	}
>  
>  	switch (INTEL_INFO(dev_priv)->gen) {
> @@ -15748,7 +15827,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		if (crtc_state->base.active) {
>  			dev_priv->active_crtcs |= 1 << crtc->pipe;
>  
> -			if (IS_BROXTON(dev_priv) || IS_BROADWELL(dev_priv))
> +			if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
>  				pixclk = ilk_pipe_pixel_rate(crtc_state);
>  			else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  				pixclk = crtc_state->base.adjusted_mode.crtc_clock;
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index c283ba4babe8..e99e306e8743 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1194,6 +1194,7 @@ skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  	struct intel_shared_dpll *pll;
>  	uint32_t ctrl1, cfgcr1, cfgcr2;
>  	int clock = crtc_state->port_clock;
> +	uint32_t vco = 8100;
>  
>  	/*
>  	 * See comment in intel_dpll_hw_state to understand why we always use 0
> @@ -1236,17 +1237,17 @@ skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  		case 162000:
>  			ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1620, 0);
>  			break;
> -		/* TBD: For DP link rates 2.16 GHz and 4.32 GHz, VCO is 8640 which
> -		results in CDCLK change. Need to handle the change of CDCLK by
> -		disabling pipes and re-enabling them */
>  		case 108000:
>  			ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080, 0);
> +			vco = 8640;
>  			break;
>  		case 216000:
>  			ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2160, 0);
> +			vco = 8640;
>  			break;
>  		}
>  
> +		to_intel_atomic_state(crtc_state->base.state)->cdclk_pll_vco = vco;

If VCO was previously set to 8640 (for DPLL0) and later we'd set here
the rate for another DPLL with VCO 8100 this would incorrectly change
the CDCLK VCO to 8100. VCO isn't actually changed at this point in the
code though and you later fix this, so it's ok.

>  		cfgcr1 = cfgcr2 = 0;
>  	} else {
>  		return NULL;
> @@ -1639,7 +1640,7 @@ static void intel_ddi_pll_init(struct drm_device *dev)
>  		int cdclk_freq;
>  
>  		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> -		dev_priv->skl_boot_cdclk = cdclk_freq;
> +		dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq);
>  		if (skl_sanitize_cdclk(dev_priv))
>  			DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
>  		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 0dc2bc9c65cf..c7cb9829547e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -304,6 +304,9 @@ struct intel_atomic_state {
>  	unsigned int active_crtcs;
>  	unsigned int min_pixclk[I915_MAX_PIPES];
>  
> +	/* SKL/KBL Only */
> +	unsigned int cdclk_pll_vco;
> +
>  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
>  
>  	/*
> @@ -1277,6 +1280,7 @@ void gen9_enable_dc5(struct drm_i915_private *dev_priv);
>  void skl_init_cdclk(struct drm_i915_private *dev_priv);
>  int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
>  void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> +unsigned int skl_cdclk_get_vco(unsigned int freq);
>  void skl_enable_dc6(struct drm_i915_private *dev_priv);
>  void skl_disable_dc6(struct drm_i915_private *dev_priv);
>  void intel_dp_get_m_n(struct intel_crtc *crtc,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx