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

Submitted by Clinton Taylor on Feb. 10, 2016, 12:28 a.m.

Details

Message ID 1455064107-32242-1-git-send-email-clinton.a.taylor@intel.com
State New
Headers show
Series "drm/i915/skl: SKL CDCLK change on modeset tracking VCO" ( rev: 2 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Clinton Taylor Feb. 10, 2016, 12:28 a.m.
From: Clint Taylor <clinton.a.taylor@intel.com>

Track VCO frequency of SKL instead of the boot CDCLK and allow modeset
to set cdclk based on the max required pixel clock based on VCO
selected.

The vco should be tracked at the atomic level and all CRTCs updated if
the required vco is changed. At this time the eDP pll is configured
inside the encoder which has no visibility into the atomic state. When
eDP v1.4 panel that require the 8640 vco are available this may need
to be investigated.

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.

Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.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 |   83 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dp.c      |    9 +++-
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 5 files changed, 81 insertions(+), 16 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8216665..f65dd1a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1822,7 +1822,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 hpll_freq;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 6d5b09f..285adab 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2958,7 +2958,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 9e2273b..372a68f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5663,7 +5663,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;
 
@@ -5821,17 +5821,17 @@  void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
 
 void skl_init_cdclk(struct drm_i915_private *dev_priv)
 {
-	unsigned int required_vco;
-
 	/* 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);
 	}
 
 	/* set CDCLK to the frequency the BIOS chose */
-	skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk);
+	skl_set_cdclk(dev_priv, (dev_priv->skl_vco_freq == 8100) ? 337500 : 308570 );
 
 	/* enable DBUF power */
 	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
@@ -5847,7 +5847,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
@@ -5871,11 +5871,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 */
@@ -9845,6 +9841,64 @@  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 drm_i915_private *dev_priv = to_i915(state->dev);
+	int max_pixclk = ilk_max_pixel_rate(state);
+	int cdclk;
+	
+	/*
+	* FIXME should also account for plane ratio
+	* once 64bpp pixel formats are supported.
+	*/
+
+	if (dev_priv->skl_vco_freq == 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;
+	}
+
+	to_intel_atomic_state(state)->cdclk = cdclk;
+
+	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)->cdclk;
+
+	skl_set_cdclk(dev_priv, req_cdclk);
+}
+
 static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
 				      struct intel_crtc_state *crtc_state)
 {
@@ -15002,6 +15056,11 @@  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) {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a073f04..0ed25a8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1242,9 +1242,10 @@  intel_dp_connector_unregister(struct intel_connector *intel_connector)
 }
 
 static void
-skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
+skl_edp_set_pll_config(struct drm_i915_private *dev_priv, struct intel_crtc_state *pipe_config)
 {
 	u32 ctrl1;
+	u32 vco = 8100;
 
 	memset(&pipe_config->dpll_hw_state, 0,
 	       sizeof(pipe_config->dpll_hw_state));
@@ -1277,13 +1278,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;
 
 	}
+
+	dev_priv->skl_vco_freq = vco;
 	pipe_config->dpll_hw_state.ctrl1 = ctrl1;
 }
 
@@ -1664,7 +1669,7 @@  found:
 	}
 
 	if ((IS_SKYLAKE(dev)  || IS_KABYLAKE(dev)) && is_edp(intel_dp))
-		skl_edp_set_pll_config(pipe_config);
+		skl_edp_set_pll_config(dev_priv, pipe_config);
 	else if (IS_BROXTON(dev))
 		/* handled in ddi */;
 	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 878172a..1acfdf9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1191,6 +1191,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,

Comments

couple of questions since i am looking at SKL code for the first time
 > seems we are not reading max cd clock from VBIOS like BDW
       even though SKL has limit register to say max cd clock i dont think
       it is working, so VBIOS saves the value during boot just like in BDW
       and we are supposed to use it. please check VBT spec for the details
 > why should we store vco in a separate variable when it is already
    available as part of "pipe_config->dpll_hw_state.ctrl1"
 > still trying to understand the flow but is "ctrl1"/"VCO" in this patch
    written to   DPLL_CTRL1 before we change the CD Clock ? if not then
    it might be a bug and must be fixed as part of changes
    here.

regards,
Sivakumar

On 2/10/2016 5:58 AM, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
>
> Track VCO frequency of SKL instead of the boot CDCLK and allow modeset
> to set cdclk based on the max required pixel clock based on VCO
> selected.
>
> The vco should be tracked at the atomic level and all CRTCs updated if
> the required vco is changed. At this time the eDP pll is configured
> inside the encoder which has no visibility into the atomic state. When
> eDP v1.4 panel that require the 8640 vco are available this may need
> to be investigated.
>
> 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.
>
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.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 |   83 +++++++++++++++++++++++++++++-----
>   drivers/gpu/drm/i915/intel_dp.c      |    9 +++-
>   drivers/gpu/drm/i915/intel_drv.h     |    1 +
>   5 files changed, 81 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8216665..f65dd1a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1822,7 +1822,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 hpll_freq;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 6d5b09f..285adab 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2958,7 +2958,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 9e2273b..372a68f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5663,7 +5663,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;
>   
> @@ -5821,17 +5821,17 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>   
>   void skl_init_cdclk(struct drm_i915_private *dev_priv)
>   {
> -	unsigned int required_vco;
> -
>   	/* 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);
>   	}
>   
>   	/* set CDCLK to the frequency the BIOS chose */
> -	skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk);
> +	skl_set_cdclk(dev_priv, (dev_priv->skl_vco_freq == 8100) ? 337500 : 308570 );
>   
>   	/* enable DBUF power */
>   	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> @@ -5847,7 +5847,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
> @@ -5871,11 +5871,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 */
> @@ -9845,6 +9841,64 @@ 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 drm_i915_private *dev_priv = to_i915(state->dev);
> +	int max_pixclk = ilk_max_pixel_rate(state);
> +	int cdclk;
> +	
> +	/*
> +	* FIXME should also account for plane ratio
> +	* once 64bpp pixel formats are supported.
> +	*/
> +
> +	if (dev_priv->skl_vco_freq == 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;
> +	}
> +
> +	to_intel_atomic_state(state)->cdclk = cdclk;
> +
> +	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)->cdclk;
> +
> +	skl_set_cdclk(dev_priv, req_cdclk);
> +}
> +
>   static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>   				      struct intel_crtc_state *crtc_state)
>   {
> @@ -15002,6 +15056,11 @@ 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) {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a073f04..0ed25a8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1242,9 +1242,10 @@ intel_dp_connector_unregister(struct intel_connector *intel_connector)
>   }
>   
>   static void
> -skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
> +skl_edp_set_pll_config(struct drm_i915_private *dev_priv, struct intel_crtc_state *pipe_config)
>   {
>   	u32 ctrl1;
> +	u32 vco = 8100;
>   
>   	memset(&pipe_config->dpll_hw_state, 0,
>   	       sizeof(pipe_config->dpll_hw_state));
> @@ -1277,13 +1278,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;
>   
>   	}
> +
> +	dev_priv->skl_vco_freq = vco;
>   	pipe_config->dpll_hw_state.ctrl1 = ctrl1;
>   }
>   
> @@ -1664,7 +1669,7 @@ found:
>   	}
>   
>   	if ((IS_SKYLAKE(dev)  || IS_KABYLAKE(dev)) && is_edp(intel_dp))
> -		skl_edp_set_pll_config(pipe_config);
> +		skl_edp_set_pll_config(dev_priv, pipe_config);
>   	else if (IS_BROXTON(dev))
>   		/* handled in ddi */;
>   	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 878172a..1acfdf9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1191,6 +1191,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,
On Tue, Feb 09, 2016 at 04:28:27PM -0800, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Track VCO frequency of SKL instead of the boot CDCLK and allow modeset
> to set cdclk based on the max required pixel clock based on VCO
> selected.
> 
> The vco should be tracked at the atomic level and all CRTCs updated if
> the required vco is changed. At this time the eDP pll is configured
> inside the encoder which has no visibility into the atomic state.

Yes it does. The passed in pipe_config is the crtc's state. And
if you want to store the thing in the top level atomic state you
just dig up that up from the crtc state:
to_intel_atomic_state(pipe_config->base.state) 
or something along those lines).

> When
> eDP v1.4 panel that require the 8640 vco are available this may need
> to be investigated.
> 
> 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.
> 
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.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 |   83 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_dp.c      |    9 +++-
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  5 files changed, 81 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8216665..f65dd1a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1822,7 +1822,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 hpll_freq;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 6d5b09f..285adab 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2958,7 +2958,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 9e2273b..372a68f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5663,7 +5663,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;
>  
> @@ -5821,17 +5821,17 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>  
>  void skl_init_cdclk(struct drm_i915_private *dev_priv)
>  {
> -	unsigned int required_vco;
> -
>  	/* 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);
>  	}
>  
>  	/* set CDCLK to the frequency the BIOS chose */
> -	skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk);
> +	skl_set_cdclk(dev_priv, (dev_priv->skl_vco_freq == 8100) ? 337500 : 308570 );
>  
>  	/* enable DBUF power */
>  	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> @@ -5847,7 +5847,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
> @@ -5871,11 +5871,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 */
> @@ -9845,6 +9841,64 @@ 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 drm_i915_private *dev_priv = to_i915(state->dev);
> +	int max_pixclk = ilk_max_pixel_rate(state);
> +	int cdclk;
> +	
> +	/*
> +	* FIXME should also account for plane ratio
> +	* once 64bpp pixel formats are supported.
> +	*/
> +
> +	if (dev_priv->skl_vco_freq == 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;
> +	}
> +
> +	to_intel_atomic_state(state)->cdclk = cdclk;
> +
> +	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)->cdclk;
> +
> +	skl_set_cdclk(dev_priv, req_cdclk);
> +}
> +
>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>  				      struct intel_crtc_state *crtc_state)
>  {
> @@ -15002,6 +15056,11 @@ 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) {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a073f04..0ed25a8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1242,9 +1242,10 @@ intel_dp_connector_unregister(struct intel_connector *intel_connector)
>  }
>  
>  static void
> -skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
> +skl_edp_set_pll_config(struct drm_i915_private *dev_priv, struct intel_crtc_state *pipe_config)
>  {
>  	u32 ctrl1;
> +	u32 vco = 8100;
>  
>  	memset(&pipe_config->dpll_hw_state, 0,
>  	       sizeof(pipe_config->dpll_hw_state));
> @@ -1277,13 +1278,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;
>  
>  	}
> +
> +	dev_priv->skl_vco_freq = vco;
>  	pipe_config->dpll_hw_state.ctrl1 = ctrl1;
>  }
>  
> @@ -1664,7 +1669,7 @@ found:
>  	}
>  
>  	if ((IS_SKYLAKE(dev)  || IS_KABYLAKE(dev)) && is_edp(intel_dp))
> -		skl_edp_set_pll_config(pipe_config);
> +		skl_edp_set_pll_config(dev_priv, pipe_config);
>  	else if (IS_BROXTON(dev))
>  		/* handled in ddi */;
>  	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 878172a..1acfdf9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1191,6 +1191,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
On 02/09/2016 07:29 PM, Thulasimani, Sivakumar wrote:
> couple of questions since i am looking at SKL code for the first time
>  > seems we are not reading max cd clock from VBIOS like BDW
>        even though SKL has limit register to say max cd clock i dont think
>        it is working, so VBIOS saves the value during boot just like in BDW
>        and we are supposed to use it. please check VBT spec for the details

Sounds like you might have found a bug. Submit a patch.

>  > why should we store vco in a separate variable when it is already
>     available as part of "pipe_config->dpll_hw_state.ctrl1"

We are going to need to know current VCO and target VCO. Current VCO 
will have the existing VCO in use and target VCO will be compared to 
existing to see in we need a modeset across all CRTCs. ctrl1 will only 
contain the new VCO setting. V4 of the patch is in process.

>  > still trying to understand the flow but is "ctrl1"/"VCO" in this patch
>     written to   DPLL_CTRL1 before we change the CD Clock ? if not then
>     it might be a bug and must be fixed as part of changes
>     here.
>
> regards,
> Sivakumar
>
> On 2/10/2016 5:58 AM, clinton.a.taylor@intel.com wrote:
>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>
>> Track VCO frequency of SKL instead of the boot CDCLK and allow modeset
>> to set cdclk based on the max required pixel clock based on VCO
>> selected.
>>
>> The vco should be tracked at the atomic level and all CRTCs updated if
>> the required vco is changed. At this time the eDP pll is configured
>> inside the encoder which has no visibility into the atomic state. When
>> eDP v1.4 panel that require the 8640 vco are available this may need
>> to be investigated.
>>
>> 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.
>>
>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>> Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.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 |   83
>> +++++++++++++++++++++++++++++-----
>>   drivers/gpu/drm/i915/intel_dp.c      |    9 +++-
>>   drivers/gpu/drm/i915/intel_drv.h     |    1 +
>>   5 files changed, 81 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 8216665..f65dd1a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1822,7 +1822,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 hpll_freq;
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> b/drivers/gpu/drm/i915/intel_ddi.c
>> index 6d5b09f..285adab 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2958,7 +2958,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 9e2273b..372a68f 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5663,7 +5663,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;
>> @@ -5821,17 +5821,17 @@ void skl_uninit_cdclk(struct drm_i915_private
>> *dev_priv)
>>   void skl_init_cdclk(struct drm_i915_private *dev_priv)
>>   {
>> -    unsigned int required_vco;
>> -
>>       /* 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);
>>       }
>>       /* set CDCLK to the frequency the BIOS chose */
>> -    skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk);
>> +    skl_set_cdclk(dev_priv, (dev_priv->skl_vco_freq == 8100) ? 337500
>> : 308570 );
>>       /* enable DBUF power */
>>       I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
>> @@ -5847,7 +5847,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
>> @@ -5871,11 +5871,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 */
>> @@ -9845,6 +9841,64 @@ 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 drm_i915_private *dev_priv = to_i915(state->dev);
>> +    int max_pixclk = ilk_max_pixel_rate(state);
>> +    int cdclk;
>> +
>> +    /*
>> +    * FIXME should also account for plane ratio
>> +    * once 64bpp pixel formats are supported.
>> +    */
>> +
>> +    if (dev_priv->skl_vco_freq == 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;
>> +    }
>> +
>> +    to_intel_atomic_state(state)->cdclk = cdclk;
>> +
>> +    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)->cdclk;
>> +
>> +    skl_set_cdclk(dev_priv, req_cdclk);
>> +}
>> +
>>   static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>>                         struct intel_crtc_state *crtc_state)
>>   {
>> @@ -15002,6 +15056,11 @@ 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) {
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index a073f04..0ed25a8 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1242,9 +1242,10 @@ intel_dp_connector_unregister(struct
>> intel_connector *intel_connector)
>>   }
>>   static void
>> -skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
>> +skl_edp_set_pll_config(struct drm_i915_private *dev_priv, struct
>> intel_crtc_state *pipe_config)
>>   {
>>       u32 ctrl1;
>> +    u32 vco = 8100;
>>       memset(&pipe_config->dpll_hw_state, 0,
>>              sizeof(pipe_config->dpll_hw_state));
>> @@ -1277,13 +1278,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;
>>       }
>> +
>> +    dev_priv->skl_vco_freq = vco;
>>       pipe_config->dpll_hw_state.ctrl1 = ctrl1;
>>   }
>> @@ -1664,7 +1669,7 @@ found:
>>       }
>>       if ((IS_SKYLAKE(dev)  || IS_KABYLAKE(dev)) && is_edp(intel_dp))
>> -        skl_edp_set_pll_config(pipe_config);
>> +        skl_edp_set_pll_config(dev_priv, pipe_config);
>>       else if (IS_BROXTON(dev))
>>           /* handled in ddi */;
>>       else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 878172a..1acfdf9 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1191,6 +1191,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,
>
On 10/02/16 06:27, Ville Syrjälä wrote:
> On Tue, Feb 09, 2016 at 04:28:27PM -0800, clinton.a.taylor@intel.com wrote:
>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>
>> Track VCO frequency of SKL instead of the boot CDCLK and allow modeset
>> to set cdclk based on the max required pixel clock based on VCO
>> selected.
>>
>> The vco should be tracked at the atomic level and all CRTCs updated if
>> the required vco is changed. At this time the eDP pll is configured
>> inside the encoder which has no visibility into the atomic state.
> 
> Yes it does. The passed in pipe_config is the crtc's state. And
> if you want to store the thing in the top level atomic state you
> just dig up that up from the crtc state:
> to_intel_atomic_state(pipe_config->base.state) 
> or something along those lines).

Hi, I'm writing this message with Clint. We understand the following:

- This V3 patch as it is now is fixing many use cases for hardware that has been
  on the shelves for months.

- It does not fix all use cases with future eDP 1.4 panels.
  Example: boot with external monitor and eDP 1.4 lid closed; then lid is
  opened and causes a VCO change. However such cases are not supported yet anyway! 
  If this happens today, the CD clock will move to some uncontrolled value
  and the pipelines will NOT be reprogrammed. So this patch does *not regress* any
  use case - not even future use cases.

- Present and future use cases can be addressed in two consecutive patches: first
  this V3 patch now; then atomic modeset VCO tracking later. This won't increase
  development complexity in any way. In other words, the second patch will almost not
  change the code of the first patch.

- From a validation perspective this first patch can be validated today with today's
  hardware. The second patch cannot be validated yet because eDP 1.4 hardware
  is not readily available yet.

- Getting atomic modeset VCO tracking right will take additional development time.
  And... did I mention validation already?

- This first patch is fixing many use cases for hardware that has been
  on the shelves for months, including failures to reach max resolution.

So - you saw me coming from a distance - can we agree on splitting this work
in two separate patches so we can merge this first V3 patch now? Products have
been needing this for months, thanks!

Cheers,

Marc
On Wed, Feb 10, 2016 at 05:37:20PM -0800, Marc Herbert wrote:
> On 10/02/16 06:27, Ville Syrjälä wrote:
> > On Tue, Feb 09, 2016 at 04:28:27PM -0800, clinton.a.taylor@intel.com wrote:
> >> From: Clint Taylor <clinton.a.taylor@intel.com>
> >>
> >> Track VCO frequency of SKL instead of the boot CDCLK and allow modeset
> >> to set cdclk based on the max required pixel clock based on VCO
> >> selected.
> >>
> >> The vco should be tracked at the atomic level and all CRTCs updated if
> >> the required vco is changed. At this time the eDP pll is configured
> >> inside the encoder which has no visibility into the atomic state.
> > 
> > Yes it does. The passed in pipe_config is the crtc's state. And
> > if you want to store the thing in the top level atomic state you
> > just dig up that up from the crtc state:
> > to_intel_atomic_state(pipe_config->base.state) 
> > or something along those lines).
> 
> Hi, I'm writing this message with Clint. We understand the following:
> 
> - This V3 patch as it is now is fixing many use cases for hardware that has been
>   on the shelves for months.
> 
> - It does not fix all use cases with future eDP 1.4 panels.
>   Example: boot with external monitor and eDP 1.4 lid closed; then lid is
>   opened and causes a VCO change. However such cases are not supported yet anyway! 
>   If this happens today, the CD clock will move to some uncontrolled value
>   and the pipelines will NOT be reprogrammed. So this patch does *not regress* any
>   use case - not even future use cases.
> 
> - Present and future use cases can be addressed in two consecutive patches: first
>   this V3 patch now; then atomic modeset VCO tracking later. This won't increase
>   development complexity in any way. In other words, the second patch will almost not
>   change the code of the first patch.
> 
> - From a validation perspective this first patch can be validated today with today's
>   hardware. The second patch cannot be validated yet because eDP 1.4 hardware
>   is not readily available yet.
> 
> - Getting atomic modeset VCO tracking right will take additional development time.
>   And... did I mention validation already?
> 
> - This first patch is fixing many use cases for hardware that has been
>   on the shelves for months, including failures to reach max resolution.
> 
> So - you saw me coming from a distance - can we agree on splitting this work
> in two separate patches so we can merge this first V3 patch now? Products have
> been needing this for months, thanks!

Atomic is the new world, we need this thing to be atomic. Please work
together with Mika Kahola (who's done all the dynamic cdclk infrastucture
together with Ville).

Wrt validation: That's what we have testcases and CI for, plus in-kernel
consistency checks of atomic state.

Thanks, Daniel
On Tue, Feb 09, 2016 at 04:28:27PM -0800, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Track VCO frequency of SKL instead of the boot CDCLK and allow modeset
> to set cdclk based on the max required pixel clock based on VCO
> selected.
> 
> The vco should be tracked at the atomic level and all CRTCs updated if
> the required vco is changed. At this time the eDP pll is configured
> inside the encoder which has no visibility into the atomic state. When
> eDP v1.4 panel that require the 8640 vco are available this may need
> to be investigated.
> 
> 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.
> 
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>

BTW I think we might want a patch for stable that just removes the
max_cdclk setup for SKL/KBL (ie. just set max_cdclk=current cdclk).
We'd put that in first, then put in the final version of this patch,
and finally revert the first patch.

Without that, backporting any of the DP/HDMI max_dotclock check patches
to stable won't actually help SKL. Should we decide to backport those
that is.