diff mbox

[V8] drm/i915/skl: SKL CDCLK change on modeset tracking VCO

Message ID 1457560719-20866-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Taylor, Clinton A March 9, 2016, 9:58 p.m. UTC
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

Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/intel_ddi.c     |    2 +-
 drivers/gpu/drm/i915/intel_display.c |  109 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dp.c      |    5 ++
 drivers/gpu/drm/i915/intel_drv.h     |    5 ++
 5 files changed, 105 insertions(+), 18 deletions(-)

Comments

Maarten Lankhorst March 10, 2016, 8:08 a.m. UTC | #1
Op 09-03-16 om 22:58 schreef clinton.a.taylor@intel.com:
> 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
>
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>
Is the hang in the commit message introduced by this commit, or fixed by this commit?

~Maarten
Ville Syrjälä March 10, 2016, 1:35 p.m. UTC | #2
On Wed, Mar 09, 2016 at 01:58:39PM -0800, clinton.a.taylor@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
> 
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    2 +-
>  drivers/gpu/drm/i915/intel_ddi.c     |    2 +-
>  drivers/gpu/drm/i915/intel_display.c |  109 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_dp.c      |    5 ++
>  drivers/gpu/drm/i915/intel_drv.h     |    5 ++
>  5 files changed, 105 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f37ac12..83bb3fd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1833,7 +1833,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_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 62de9f4..f628647 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3003,7 +3003,7 @@ 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_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 62d36a7..10cdeb7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5811,7 +5811,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq)
>  	return (freq - 1000) / 500;
>  }
>  
> -static unsigned int skl_cdclk_get_vco(unsigned int freq)
> +unsigned int skl_cdclk_get_vco(unsigned int freq)
>  {
>  	unsigned int i;
>  
> @@ -5969,17 +5969,21 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>  
>  void skl_init_cdclk(struct drm_i915_private *dev_priv)
>  {
> -	unsigned int required_vco;
> +	unsigned int cdclk;
>  
>  	/* DPLL0 not enabled (happens on early BIOS versions) */
>  	if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) {
>  		/* enable DPLL0 */
> -		required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk);
> -		skl_dpll0_enable(dev_priv, required_vco);
> +		if (dev_priv->skl_vco_freq != 8640)
> +			dev_priv->skl_vco_freq = 8100;
> +		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);
> @@ -5995,7 +5999,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
> @@ -6019,11 +6023,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 */
> @@ -9963,6 +9963,71 @@ 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 (to_intel_atomic_state(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 = 337500;

That's not correct for the vco==8640 case.

> +
> +	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;

This looks correct now.

> +
> +	/*
> +	 * 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)
>  {
> @@ -13379,9 +13444,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)
> @@ -13753,7 +13824,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);

as does this.

With the "no active pipes with vco==8640" case fixed this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I still need to see if I could reproduce the hang somehow on my end. I
got another SKL now, but unfortunately it's a NUC so I can't actually
test the eDP part on it, and the my eDP SKL machine is too unstable to
do anything with multiple displays attached. I did excercise the eDP
vco stuff on it though, and with my extra patches on top even tested
the vco changing by progamming the wrong vco originally, and then
checked that it got corrected on the first modeset.


>  	}
>  
> @@ -15231,8 +15303,12 @@ static void intel_init_display(struct drm_device *dev)
>  			broxton_modeset_commit_cdclk;
>  		dev_priv->display.modeset_calc_cdclk =
>  			broxton_modeset_calc_cdclk;
> +	} else if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) {
> +		dev_priv->display.modeset_commit_cdclk =
> +			skl_modeset_commit_cdclk;
> +		dev_priv->display.modeset_calc_cdclk =
> +			skl_modeset_calc_cdclk;
>  	}
> -
>  	switch (INTEL_INFO(dev)->gen) {
>  	case 2:
>  		dev_priv->display.queue_flip = intel_gen2_queue_flip;
> @@ -15968,7 +16044,8 @@ 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_BROADWELL(dev_priv)) {
> +			if (IS_BROADWELL(dev_priv) || IS_SKYLAKE(dev_priv) ||
> +			    IS_KABYLAKE(dev_priv)) {
>  				pixclk = ilk_pipe_pixel_rate(crtc_state);
>  
>  				/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 351a8f3..4298f89 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1228,6 +1228,7 @@ static void
>  skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
>  {
>  	u32 ctrl1;
> +	u32 vco = 8100;
>  
>  	memset(&pipe_config->dpll_hw_state, 0,
>  	       sizeof(pipe_config->dpll_hw_state));
> @@ -1260,13 +1261,17 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
>  	case 108000:
>  		ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080,
>  					      SKL_DPLL0);
> +		vco = 8640;
>  		break;
>  	case 216000:
>  		ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2160,
>  					      SKL_DPLL0);
> +		vco = 8640;
>  		break;
>  
>  	}
> +
> +	to_intel_atomic_state(pipe_config->base.state)->cdclk_pll_vco = vco;
>  	pipe_config->dpll_hw_state.ctrl1 = ctrl1;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7b2d66d..17721b2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -301,6 +301,10 @@ struct intel_atomic_state {
>  	 * don't bother calculating intermediate watermarks.
>  	 */
>  	bool skip_intermediate_wm;
> +
> +	/* SKL/KBL Only */
> +	unsigned int cdclk_pll_vco;
> +
>  };
>  
>  struct intel_plane_state {
> @@ -1247,6 +1251,7 @@ void bxt_disable_dc9(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,
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Taylor, Clinton A March 11, 2016, 5:04 p.m. UTC | #3
On 03/10/2016 12:08 AM, Maarten Lankhorst wrote:
> Op 09-03-16 om 22:58 schreef clinton.a.taylor@intel.com:
>> 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
>>
>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>>
> Is the hang in the commit message introduced by this commit, or fixed by this commit?

The hang is introduced by this commit. The hang isn't reproducible using 
Arch Linux this appears to be an existing issue that is exposed for the 
first time now that cdclk can change on SKL. This warning will only 
affect ChromeOS users using drm-intel-nightly as the backport to 
ChromeOS removes many of the atomic specific changes. It's also very 
strange that the issue only affects the encoder attached to DDI1 and its 
computation of dev_cdclk during a cold boot.


>
> ~Maarten
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f37ac12..83bb3fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1833,7 +1833,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_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 62de9f4..f628647 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3003,7 +3003,7 @@  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_display.c b/drivers/gpu/drm/i915/intel_display.c
index 62d36a7..10cdeb7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5811,7 +5811,7 @@  static unsigned int skl_cdclk_decimal(unsigned int freq)
 	return (freq - 1000) / 500;
 }
 
-static unsigned int skl_cdclk_get_vco(unsigned int freq)
+unsigned int skl_cdclk_get_vco(unsigned int freq)
 {
 	unsigned int i;
 
@@ -5969,17 +5969,21 @@  void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
 
 void skl_init_cdclk(struct drm_i915_private *dev_priv)
 {
-	unsigned int required_vco;
+	unsigned int cdclk;
 
 	/* DPLL0 not enabled (happens on early BIOS versions) */
 	if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) {
 		/* enable DPLL0 */
-		required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk);
-		skl_dpll0_enable(dev_priv, required_vco);
+		if (dev_priv->skl_vco_freq != 8640)
+			dev_priv->skl_vco_freq = 8100;
+		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);
@@ -5995,7 +5999,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
@@ -6019,11 +6023,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 */
@@ -9963,6 +9963,71 @@  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 (to_intel_atomic_state(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 = 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)
 {
@@ -13379,9 +13444,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)
@@ -13753,7 +13824,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);
 	}
 
@@ -15231,8 +15303,12 @@  static void intel_init_display(struct drm_device *dev)
 			broxton_modeset_commit_cdclk;
 		dev_priv->display.modeset_calc_cdclk =
 			broxton_modeset_calc_cdclk;
+	} else if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) {
+		dev_priv->display.modeset_commit_cdclk =
+			skl_modeset_commit_cdclk;
+		dev_priv->display.modeset_calc_cdclk =
+			skl_modeset_calc_cdclk;
 	}
-
 	switch (INTEL_INFO(dev)->gen) {
 	case 2:
 		dev_priv->display.queue_flip = intel_gen2_queue_flip;
@@ -15968,7 +16044,8 @@  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_BROADWELL(dev_priv)) {
+			if (IS_BROADWELL(dev_priv) || IS_SKYLAKE(dev_priv) ||
+			    IS_KABYLAKE(dev_priv)) {
 				pixclk = ilk_pipe_pixel_rate(crtc_state);
 
 				/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 351a8f3..4298f89 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1228,6 +1228,7 @@  static void
 skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
 {
 	u32 ctrl1;
+	u32 vco = 8100;
 
 	memset(&pipe_config->dpll_hw_state, 0,
 	       sizeof(pipe_config->dpll_hw_state));
@@ -1260,13 +1261,17 @@  skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
 	case 108000:
 		ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080,
 					      SKL_DPLL0);
+		vco = 8640;
 		break;
 	case 216000:
 		ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2160,
 					      SKL_DPLL0);
+		vco = 8640;
 		break;
 
 	}
+
+	to_intel_atomic_state(pipe_config->base.state)->cdclk_pll_vco = vco;
 	pipe_config->dpll_hw_state.ctrl1 = ctrl1;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7b2d66d..17721b2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -301,6 +301,10 @@  struct intel_atomic_state {
 	 * don't bother calculating intermediate watermarks.
 	 */
 	bool skip_intermediate_wm;
+
+	/* SKL/KBL Only */
+	unsigned int cdclk_pll_vco;
+
 };
 
 struct intel_plane_state {
@@ -1247,6 +1251,7 @@  void bxt_disable_dc9(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,