[PATCH v6 0/8] All sort of cdclk stuff

[PATCH v6 0/8] All sort of cdclk stuff

[Mika Kahola]

This patch series rebases Ville's original cdclk patch series
excluding the ones that	has already been reviewed.

http://lists.freedesktop.org/archives/intel-gfx/2014-November/055633.html

The patches are rebased to the latest drm-intel-nightly. The major change
to the original series is the patch order change for Broadwell and Haswell.
Now, the Broadwell CD clock patch can be applied before Haswell CD clock
patch. This way, if decided, the CD clock change for Haswell can be discarded
without affecting the Broadwell patch.

Based on the received comments, the style problems are now fixed for this
patch set.

Ville Syrjälä (8):
  drm/i915: Cache current cdclk frequency in dev_priv
  drm/i915: Use cached cdclk value
  drm/i915: Unify ilk and hsw .get_aux_clock_divider
  drm/i915: Store max cdclk value in dev_priv
  drm/i915: Don't enable IPS when pixel rate exceeds 95%
  drm/i915: Add IS_BDW_ULX
  drm/i915: BDW clock change support
  drm/i915: HSW cdclk support

 drivers/gpu/drm/i915/i915_drv.h      |   5 +-
 drivers/gpu/drm/i915/i915_reg.h      |   4 +
 drivers/gpu/drm/i915/intel_display.c | 406 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_dp.c      |  24 +--
 drivers/gpu/drm/i915/intel_drv.h     |   2 +-
 drivers/gpu/drm/i915/intel_pm.c      |  19 +-
 6 files changed, 413 insertions(+), 47 deletions(-)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 1/8] drm/i915: Cache current cdclk frequency in dev_priv

[Mika Kahola]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Rather that extracting the current cdclk freuqncy every time someone
wants to know it, cache the current value and use that. VLV/CHV already
stored a cached value there so just expand that to cover all platforms.

v2: Rebased to the latest
v3: Rebased to the latest
v4: Rebased to the latest
v5: Removed spurious call to 'intel_update_cdclk(dev)' based on
    Damien Lespiau's comment

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 16e159d..9cf1553 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5747,7 +5747,7 @@ static int valleyview_get_vco(struct drm_i915_private *dev_priv)
 	return vco_freq[hpll_freq] * 1000;
 }
 
-static void vlv_update_cdclk(struct drm_device *dev)
+static void intel_update_cdclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -5760,7 +5760,14 @@ static void vlv_update_cdclk(struct drm_device *dev)
 	 * BSpec erroneously claims we should aim for 4MHz, but
 	 * in fact 1MHz is the correct frequency.
 	 */
-	I915_WRITE(GMBUSFREQ_VLV, DIV_ROUND_UP(dev_priv->cdclk_freq, 1000));
+	if (IS_VALLEYVIEW(dev)) {
+		/*
+		 * Program the gmbus_freq based on the cdclk frequency.
+		 * BSpec erroneously claims we should aim for 4MHz, but
+		 * in fact 1MHz is the correct frequency.
+		 */
+		I915_WRITE(GMBUSFREQ_VLV, DIV_ROUND_UP(dev_priv->cdclk_freq, 1000));
+	}
 }
 
 /* Adjust CDclk dividers to allow high res or save power if possible */
@@ -5826,7 +5833,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 
 	mutex_unlock(&dev_priv->sb_lock);
 
-	vlv_update_cdclk(dev);
+	intel_update_cdclk(dev);
 }
 
 static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
@@ -5867,7 +5874,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
 	}
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
-	vlv_update_cdclk(dev);
+	intel_update_cdclk(dev);
 }
 
 static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
@@ -9479,6 +9486,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	}
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+	intel_update_cdclk(dev_priv->dev);
 }
 
 /*
@@ -13273,6 +13281,8 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	intel_update_cdclk(dev);
+
 	if (HAS_DDI(dev))
 		intel_ddi_pll_init(dev);
 	else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
@@ -14848,13 +14858,9 @@ static void i915_disable_vga(struct drm_device *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
+	intel_update_cdclk(dev);
 	intel_prepare_ddi(dev);
-
-	if (IS_VALLEYVIEW(dev))
-		vlv_update_cdclk(dev);
-
 	intel_init_clock_gating(dev);
-
 	intel_enable_gt_powersave(dev);
 }
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 2/8] drm/i915: Use cached cdclk value

[Mika Kahola]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Rather than reading out the current cdclk value use the cached value we
have tucked away in dev_priv.

v2: Rebased to the latest
v3: Rebased to the latest
v4: Fix for patch style problems

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +--
 drivers/gpu/drm/i915/intel_dp.c      | 5 +++--
 drivers/gpu/drm/i915/intel_pm.c      | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9cf1553..d1dd8ab 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6610,8 +6610,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 
 	/* FIXME should check pixel clock limits on all platforms */
 	if (INTEL_INFO(dev)->gen < 4) {
-		int clock_limit =
-			dev_priv->display.get_display_clock_speed(dev);
+		int clock_limit = dev_priv->cdclk_freq;
 
 		/*
 		 * Enable pixel doubling when the dot clock
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6f525093..9a6517d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -708,7 +708,8 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 		return 0;
 
 	if (intel_dig_port->port == PORT_A) {
-		return DIV_ROUND_UP(dev_priv->display.get_display_clock_speed(dev), 2000);
+		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
+
 	} else {
 		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
 	}
@@ -723,7 +724,7 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 	if (intel_dig_port->port == PORT_A) {
 		if (index)
 			return 0;
-		return DIV_ROUND_CLOSEST(dev_priv->display.get_display_clock_speed(dev), 2000);
+		return DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 2000);
 	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
 		/* Workaround for non-ULT HSW */
 		switch (index) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index eadc15c..5db429e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1815,7 +1815,7 @@ hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
 	linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
 				     mode->crtc_clock);
 	ips_linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
-					 dev_priv->display.get_display_clock_speed(dev_priv->dev));
+					 dev_priv->cdclk_freq);
 
 	return PIPE_WM_LINETIME_IPS_LINETIME(ips_linetime) |
 	       PIPE_WM_LINETIME_TIME(linetime);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 3/8] drm/i915: Unify ilk and hsw .get_aux_clock_divider

[Mika Kahola]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

ilk_get_aux_clock_divider() is now a subset of
hsw_get_aux_clock_divider() so unify them.

v2: Rebased to the latest
v3: Rebased to the latest
v4: Fix for patch style problems

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9a6517d..959f115 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -704,23 +704,6 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (index)
-		return 0;
-
-	if (intel_dig_port->port == PORT_A) {
-		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
-
-	} else {
-		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
-	}
-}
-
-static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
-{
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
 	if (intel_dig_port->port == PORT_A) {
 		if (index)
 			return 0;
@@ -733,7 +716,9 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 		default: return 0;
 		}
 	} else  {
-		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
+		if (index)
+			return 0;
+		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
 	}
 }
 
@@ -5746,8 +5731,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
 	else if (IS_VALLEYVIEW(dev))
 		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
-	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
-		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
 	else if (HAS_PCH_SPLIT(dev))
 		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
 	else
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 4/8] drm/i915: Store max cdclk value in dev_priv

[Mika Kahola]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Keep the cdclk maximum supported frequency around in dev_priv so that we
can verify certain things against it before actually changing the cdclk
frequency.

For now only VLV/CHV have support changing cdclk frequency, so other
plarforms get to assume cdclk is fixed.

v2: Rebased to the latest
v3: Rebased to the latest
v4: Fix for patch style problems

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 20 +++++++++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 60aa962..db9e268 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1743,7 +1743,7 @@ struct drm_i915_private {
 
 	unsigned int fsb_freq, mem_freq, is_ddr3;
 	unsigned int skl_boot_cdclk;
-	unsigned int cdclk_freq;
+	unsigned int cdclk_freq, max_cdclk_freq;
 	unsigned int hpll_freq;
 
 	/**
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d1dd8ab..445385d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5747,6 +5747,21 @@ static int valleyview_get_vco(struct drm_i915_private *dev_priv)
 	return vco_freq[hpll_freq] * 1000;
 }
 
+static void intel_update_max_cdclk(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (IS_VALLEYVIEW(dev)) {
+		dev_priv->max_cdclk_freq = 400000;
+	} else {
+		/* otherwise assume cdclk is fixed */
+		dev_priv->max_cdclk_freq = dev_priv->cdclk_freq;
+	}
+
+	DRM_DEBUG_DRIVER("Max CD clock rate: %d kHz\n",
+			 dev_priv->max_cdclk_freq);
+}
+
 static void intel_update_cdclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5768,6 +5783,9 @@ static void intel_update_cdclk(struct drm_device *dev)
 		 */
 		I915_WRITE(GMBUSFREQ_VLV, DIV_ROUND_UP(dev_priv->cdclk_freq, 1000));
 	}
+
+	if (dev_priv->max_cdclk_freq == 0)
+		intel_update_max_cdclk(dev);
 }
 
 /* Adjust CDclk dividers to allow high res or save power if possible */
@@ -6610,7 +6628,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 
 	/* FIXME should check pixel clock limits on all platforms */
 	if (INTEL_INFO(dev)->gen < 4) {
-		int clock_limit = dev_priv->cdclk_freq;
+		int clock_limit = dev_priv->max_cdclk_freq;
 
 		/*
 		 * Enable pixel doubling when the dot clock
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 5/8] drm/i915: Don't enable IPS when pixel rate exceeds 95%

[Mika Kahola]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Bspec says we shouldn't enable IPS on BDW when the pipe pixel rate
exceeds 95% of the core display clock. Apparently this can cause
underruns.

There's no similar restriction listed for HSW, so leave that one alone
for now.

v2: Add pipe_config_supports_ips() (Chris)
v3: Compare against the max cdclk insted of the current cdclk
v4: Rebased to the latest
v5: Rebased to the latest
v6: Fix for patch style problems

Tested-by: Timo Aaltonen <tjaalton@ubuntu.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83497

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 17 ++++++++---------
 3 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 445385d..c3f01aa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6610,12 +6610,38 @@ retry:
 	return ret;
 }
 
+static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
+				     struct intel_crtc_state *pipe_config)
+{
+	if (pipe_config->pipe_bpp > 24)
+		return false;
+
+	/* HSW can handle pixel rate up to cdclk? */
+	if (IS_HASWELL(dev_priv->dev))
+		return true;
+
+	/*
+	 * FIXME if we compare against max we should then
+	 * increase the cdclk frequency when the current
+	 * value is too low. The other option is to compare
+	 * against the cdclk frequency we're going have post
+	 * modeset (ie. one we computed using other constraints).
+	 * Need to measure whether using a lower cdclk w/o IPS
+	 * is better or worse than a higher cdclk w/ IPS.
+	 */
+	return ilk_pipe_pixel_rate(pipe_config) <=
+		dev_priv->max_cdclk_freq * 95 / 100;
+}
+
 static void hsw_compute_ips_config(struct intel_crtc *crtc,
 				   struct intel_crtc_state *pipe_config)
 {
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
 	pipe_config->ips_enabled = i915.enable_ips &&
-				   hsw_crtc_supports_ips(crtc) &&
-				   pipe_config->pipe_bpp <= 24;
+		hsw_crtc_supports_ips(crtc) &&
+		pipe_config_supports_ips(dev_priv, pipe_config);
 }
 
 static int intel_crtc_compute_config(struct intel_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2afb31a..5cb3004 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1375,7 +1375,7 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
 void skl_wm_get_hw_state(struct drm_device *dev);
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
-
+uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 
 /* intel_sdvo.c */
 bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5db429e..d091fec 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1434,23 +1434,22 @@ static void i845_update_wm(struct drm_crtc *unused_crtc)
 	I915_WRITE(FW_BLC, fwater_lo);
 }
 
-static uint32_t ilk_pipe_pixel_rate(struct drm_device *dev,
-				    struct drm_crtc *crtc)
+uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint32_t pixel_rate;
 
-	pixel_rate = intel_crtc->config->base.adjusted_mode.crtc_clock;
+	pixel_rate = pipe_config->base.adjusted_mode.crtc_clock;
 
 	/* We only use IF-ID interlacing. If we ever use PF-ID we'll need to
 	 * adjust the pixel_rate here. */
 
-	if (intel_crtc->config->pch_pfit.enabled) {
+	if (pipe_config->pch_pfit.enabled) {
 		uint64_t pipe_w, pipe_h, pfit_w, pfit_h;
-		uint32_t pfit_size = intel_crtc->config->pch_pfit.size;
+		uint32_t pfit_size = pipe_config->pch_pfit.size;
+
+		pipe_w = pipe_config->pipe_src_w;
+		pipe_h = pipe_config->pipe_src_h;
 
-		pipe_w = intel_crtc->config->pipe_src_w;
-		pipe_h = intel_crtc->config->pipe_src_h;
 		pfit_w = (pfit_size >> 16) & 0xFFFF;
 		pfit_h = pfit_size & 0xFFFF;
 		if (pipe_w < pfit_w)
@@ -2066,7 +2065,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
 
 	p->active = true;
 	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
-	p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
+	p->pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
 
 	if (crtc->primary->state->fb)
 		p->pri.bytes_per_pixel =
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 6/8] drm/i915: Add IS_BDW_ULX

[Mika Kahola]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We need to tell BDW ULT and ULX apart.

v2: Rebased to the latest
v3: Rebased to the latest
v4: Fix for patch style problems

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index db9e268..46722f8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2399,6 +2399,9 @@ struct drm_i915_cmd_table {
 				 ((INTEL_DEVID(dev) & 0xf) == 0x6 ||	\
 				 (INTEL_DEVID(dev) & 0xf) == 0xb ||	\
 				 (INTEL_DEVID(dev) & 0xf) == 0xe))
+/* ULX machines are also considered ULT. */
+#define IS_BDW_ULX(dev)		(IS_BROADWELL(dev) && \
+				 (INTEL_DEVID(dev) & 0xf) == 0xe)
 #define IS_BDW_GT3(dev)		(IS_BROADWELL(dev) && \
 				 (INTEL_DEVID(dev) & 0x00F0) == 0x0020)
 #define IS_HSW_ULT(dev)		(IS_HASWELL(dev) && \
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 7/8] drm/i915: BDW clock change support

[Mika Kahola]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add support for changing cdclk frequency during runtime on BDW. The
procedure is quite a bit different on BDW from the one on HSW, so
add a separate function for it.

Also with IPS enabled the actual pixel rate mustn't exceed 95% of cdclk,
so take that into account when computing the max pixel rate.

v2: Grab rps.hw_lock around sandybridge_pcode_write()
v3: Rebase due to power well vs. .global_resources() reordering
v4: Rebased to the latest
v5: Rebased to the latest
v6: Patch order shuffle so that Broadwell CD clock change is
    applied before the patch for Haswell CD clock change
v7: Fix for patch style problems

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |   2 +
 drivers/gpu/drm/i915/intel_display.c | 216 +++++++++++++++++++++++++++++++++--
 2 files changed, 208 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7213224..0f72c0e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6705,6 +6705,7 @@ enum skl_disp_power_wells {
 #define	  GEN6_PCODE_READ_RC6VIDS		0x5
 #define     GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
 #define     GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
+#define   BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ	0x18
 #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
 #define     GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
 #define     GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
@@ -7170,6 +7171,7 @@ enum skl_disp_power_wells {
 #define  LCPLL_CLK_FREQ_337_5_BDW	(2<<26)
 #define  LCPLL_CLK_FREQ_675_BDW		(3<<26)
 #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
+#define  LCPLL_ROOT_CD_CLOCK_DISABLE	(1<<24)
 #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
 #define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
 #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c3f01aa..b1e2069 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5751,7 +5751,22 @@ static void intel_update_max_cdclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (IS_VALLEYVIEW(dev)) {
+	if (IS_BROADWELL(dev))  {
+		/*
+		 * FIXME with extra cooling we can allow
+		 * 540 MHz for ULX and 675 Mhz for ULT.
+		 * How can we know if extra cooling is
+		 * available? PCI ID, VTB, something else?
+		 */
+		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
+			dev_priv->max_cdclk_freq = 450000;
+		else if (IS_BDW_ULX(dev))
+			dev_priv->max_cdclk_freq = 450000;
+		else if (IS_BDW_ULT(dev))
+			dev_priv->max_cdclk_freq = 540000;
+		else
+			dev_priv->max_cdclk_freq = 675000;
+	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->max_cdclk_freq = 400000;
 	} else {
 		/* otherwise assume cdclk is fixed */
@@ -6621,13 +6636,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
 		return true;
 
 	/*
-	 * FIXME if we compare against max we should then
-	 * increase the cdclk frequency when the current
-	 * value is too low. The other option is to compare
-	 * against the cdclk frequency we're going have post
-	 * modeset (ie. one we computed using other constraints).
-	 * Need to measure whether using a lower cdclk w/o IPS
-	 * is better or worse than a higher cdclk w/ IPS.
+	 * We compare against max which means we must take
+	 * the increased cdclk requirement into account when
+	 * calculating the new cdclk.
+	 *
+	 * Should measure whether using a lower cdclk w/o IPS
 	 */
 	return ilk_pipe_pixel_rate(pipe_config) <=
 		dev_priv->max_cdclk_freq * 95 / 100;
@@ -9608,6 +9621,182 @@ static void broxton_modeset_global_resources(struct drm_atomic_state *old_state)
 		broxton_set_cdclk(dev, req_cdclk);
 }
 
+/* compute the max rate for new configuration */
+static int ilk_max_pixel_rate(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_crtc *intel_crtc;
+	struct drm_crtc *crtc;
+	int max_pixel_rate = 0;
+	int pixel_rate;
+
+	for_each_crtc(dev, crtc) {
+		if (!crtc->state->enable)
+			continue;
+
+		intel_crtc = to_intel_crtc(crtc);
+		pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
+
+		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
+		if (IS_BROADWELL(dev) && intel_crtc->config->ips_enabled)
+			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
+
+		max_pixel_rate = max(max_pixel_rate, pixel_rate);
+	}
+
+	return max_pixel_rate;
+}
+
+static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t val, data;
+	int ret;
+
+	if (WARN((I915_READ(LCPLL_CTL) &
+		  (LCPLL_PLL_DISABLE | LCPLL_PLL_LOCK |
+		   LCPLL_CD_CLOCK_DISABLE | LCPLL_ROOT_CD_CLOCK_DISABLE |
+		   LCPLL_CD2X_CLOCK_DISABLE | LCPLL_POWER_DOWN_ALLOW |
+		   LCPLL_CD_SOURCE_FCLK)) != LCPLL_PLL_LOCK,
+		 "trying to change cdclk frequency with cdclk not enabled\n"))
+		return;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	ret = sandybridge_pcode_write(dev_priv,
+				      BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ, 0x0);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+	if (ret) {
+		DRM_ERROR("failed to inform pcode about cdclk change\n");
+		return;
+	}
+
+	val = I915_READ(LCPLL_CTL);
+	val |= LCPLL_CD_SOURCE_FCLK;
+	I915_WRITE(LCPLL_CTL, val);
+
+	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
+			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
+		DRM_ERROR("Switching to FCLK failed\n");
+
+	val = I915_READ(LCPLL_CTL);
+	val &= ~LCPLL_CLK_FREQ_MASK;
+
+	switch (cdclk) {
+	case 450000:
+		val |= LCPLL_CLK_FREQ_450;
+		data = 0;
+		break;
+	case 540000:
+		val |= LCPLL_CLK_FREQ_54O_BDW;
+		data = 1;
+		break;
+	case 337500:
+		val |= LCPLL_CLK_FREQ_337_5_BDW;
+		data = 2;
+		break;
+	case 675000:
+		val |= LCPLL_CLK_FREQ_675_BDW;
+		data = 3;
+		break;
+	default:
+		WARN(1, "invalid cdclk frequency\n");
+		return;
+	}
+
+	I915_WRITE(LCPLL_CTL, val);
+
+	val = I915_READ(LCPLL_CTL);
+	val &= ~LCPLL_CD_SOURCE_FCLK;
+	I915_WRITE(LCPLL_CTL, val);
+
+	if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
+				LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
+		DRM_ERROR("Switching back to LCPLL failed\n");
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, data);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	intel_update_cdclk(dev);
+
+	WARN(cdclk != dev_priv->cdclk_freq,
+	     "cdclk requested %d kHz but got %d kHz\n",
+	     cdclk, dev_priv->cdclk_freq);
+}
+
+static int broadwell_calc_cdclk(struct drm_i915_private *dev_priv,
+			      int max_pixel_rate)
+{
+	int cdclk;
+
+	/*
+	 * FIXME should also account for plane ratio
+	 * once 64bpp pixel formats are supported.
+	 */
+	if (max_pixel_rate > 540000)
+		cdclk = 675000;
+	else if (max_pixel_rate > 450000)
+		cdclk = 540000;
+	else if (max_pixel_rate > 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;
+	}
+
+	return cdclk;
+}
+
+static int broadwell_modeset_global_pipes(struct drm_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int max_pixclk = ilk_max_pixel_rate(dev_priv);
+	int cdclk, i;
+
+	cdclk = broadwell_calc_cdclk(dev_priv, max_pixclk);
+
+	if (cdclk == dev_priv->cdclk_freq)
+		return 0;
+
+	/* add all active pipes to the state */
+	for_each_crtc(state->dev, crtc) {
+		if (!crtc->state->enable)
+			continue;
+
+		crtc_state = drm_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+	}
+
+	/* disable/enable all currently active pipes while we change cdclk */
+	for_each_crtc_in_state(state, crtc, crtc_state, i)
+		if (crtc_state->enable)
+			crtc_state->mode_changed = true;
+
+	return 0;
+}
+
+static void broadwell_modeset_global_resources(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
+	int req_cdclk = broadwell_calc_cdclk(dev_priv, max_pixel_rate);
+
+	if (req_cdclk != dev_priv->cdclk_freq)
+		broadwell_set_cdclk(dev, req_cdclk);
+}
+
 static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
 				      struct intel_crtc_state *crtc_state)
 {
@@ -12788,8 +12977,12 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	 * mode set on this crtc.  For other crtcs we need to use the
 	 * adjusted_mode bits in the crtc directly.
 	 */
-	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev)) {
-		ret = valleyview_modeset_global_pipes(state);
+	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev)) {
+		if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev))
+			ret = valleyview_modeset_global_pipes(state);
+		else
+			ret = broadwell_modeset_global_pipes(state);
+
 		if (ret)
 			return ret;
 	}
@@ -14677,6 +14870,9 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
+		if (IS_BROADWELL(dev))
+			dev_priv->display.modeset_global_resources =
+				broadwell_modeset_global_resources;
 	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.modeset_global_resources =
 			valleyview_modeset_global_resources;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 8/8] drm/i915: HSW cdclk support

[Mika Kahola]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Implement support for changing the cdclk frequency during runtime on
HSW. VLV/CHV already have support for this, so we can follow their
example for the most part. Only the actual hardware programming differs,
the rest is pretty much the same.

The pipe pixel rate stuff is handled a bit differently for now due to
the difference in pch vs. gmch pfit handling. Eventually we should unify
that part to eliminate what is essentially duplicated code.

v2: Grab rps.hw_lock around sandybridge_pcode_write()
v3: Rebase due to power well vs. .global_resources() reordering
v4: Rebased to the latest
v5: Reformatting 'haswell_modeset_global_pipes' function to
    support atomic state
v6: Shuffling the patch order so the Broadwell CD clock patch
    can be applied before this Haswell CD clock patch
v7: Fix for patch style problems

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |   2 +
 drivers/gpu/drm/i915/intel_display.c | 137 ++++++++++++++++++++++++++++++++++-
 2 files changed, 136 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0f72c0e..418d149 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6705,6 +6705,7 @@ enum skl_disp_power_wells {
 #define	  GEN6_PCODE_READ_RC6VIDS		0x5
 #define     GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
 #define     GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
+#define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
 #define   BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ	0x18
 #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
 #define     GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
@@ -7167,6 +7168,7 @@ enum skl_disp_power_wells {
 #define  LCPLL_PLL_LOCK			(1<<30)
 #define  LCPLL_CLK_FREQ_MASK		(3<<26)
 #define  LCPLL_CLK_FREQ_450		(0<<26)
+#define  LCPLL_CLK_FREQ_ALT_HSW		(1<<26) /* 337.5 (ULX) or 540 */
 #define  LCPLL_CLK_FREQ_54O_BDW		(1<<26)
 #define  LCPLL_CLK_FREQ_337_5_BDW	(2<<26)
 #define  LCPLL_CLK_FREQ_675_BDW		(3<<26)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b1e2069..4b9907d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5751,7 +5751,16 @@ static void intel_update_max_cdclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (IS_BROADWELL(dev))  {
+	if (IS_HASWELL(dev)) {
+		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
+			dev_priv->max_cdclk_freq = 450000;
+		else if (IS_HSW_ULX(dev))
+			dev_priv->max_cdclk_freq = 337500;
+		else if (IS_HSW_ULT(dev))
+			dev_priv->max_cdclk_freq = 450000;
+		else
+			dev_priv->max_cdclk_freq = 540000;
+	} else if (IS_BROADWELL(dev))  {
 		/*
 		 * FIXME with extra cooling we can allow
 		 * 540 MHz for ULX and 675 Mhz for ULT.
@@ -9755,6 +9764,80 @@ static int broadwell_calc_cdclk(struct drm_i915_private *dev_priv,
 	return cdclk;
 }
 
+static void haswell_set_cdclk(struct drm_device *dev, int cdclk)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t val;
+
+	if (WARN((I915_READ(LCPLL_CTL) &
+		  (LCPLL_PLL_DISABLE | LCPLL_PLL_LOCK |
+		   LCPLL_CD_CLOCK_DISABLE | LCPLL_ROOT_CD_CLOCK_DISABLE |
+		   LCPLL_CD2X_CLOCK_DISABLE | LCPLL_POWER_DOWN_ALLOW |
+		   LCPLL_CD_SOURCE_FCLK)) != LCPLL_PLL_LOCK,
+		 "trying to change cdclk frequency with cdclk not enabled\n"))
+		return;
+
+	val = I915_READ(LCPLL_CTL);
+	val &= ~LCPLL_CLK_FREQ_MASK;
+
+	switch (cdclk) {
+	case 450000:
+		val |= LCPLL_CLK_FREQ_450;
+		break;
+	case 337500:
+	case 540000:
+		val |= LCPLL_CLK_FREQ_ALT_HSW;
+		break;
+	default:
+		WARN(1, "invalid cdclk frequency\n");
+		return;
+	}
+
+	I915_WRITE(LCPLL_CTL, val);
+
+	if (IS_HSW_ULX(dev)) {
+		mutex_lock(&dev_priv->rps.hw_lock);
+		sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
+					cdclk == 337500);
+		mutex_unlock(&dev_priv->rps.hw_lock);
+	}
+
+	intel_update_cdclk(dev);
+
+	WARN(cdclk != dev_priv->cdclk_freq,
+	     "cdclk requested %d kHz but got %d kHz\n",
+	     cdclk, dev_priv->cdclk_freq);
+}
+
+static int haswell_calc_cdclk(struct drm_i915_private *dev_priv,
+			      int max_pixel_rate)
+{
+	int cdclk;
+
+	/*
+	 * FIXME should also account for plane ratio
+	 * once 64bpp pixel formats are supported.
+	 */
+	if (max_pixel_rate > 450000)
+		cdclk = 540000;
+	else if (max_pixel_rate > 337500 || !IS_HSW_ULX(dev_priv))
+		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;
+	}
+
+	return cdclk;
+}
+
 static int broadwell_modeset_global_pipes(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
@@ -9797,6 +9880,49 @@ static void broadwell_modeset_global_resources(struct drm_atomic_state *state)
 		broadwell_set_cdclk(dev, req_cdclk);
 }
 
+static int haswell_modeset_global_pipes(struct drm_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int max_pixclk = ilk_max_pixel_rate(dev_priv);
+	int cdclk, i;
+
+	cdclk = haswell_calc_cdclk(dev_priv, max_pixclk);
+
+	if (cdclk == dev_priv->cdclk_freq)
+		return 0;
+
+	/* add all active pipes to the state */
+	for_each_crtc(state->dev, crtc) {
+		if (!crtc->state->enable)
+			continue;
+
+		crtc_state = drm_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+	}
+
+	/* disable/enable all currently active pipes while we change cdclk */
+	for_each_crtc_in_state(state, crtc, crtc_state, i)
+		if (crtc_state->enable)
+			crtc_state->mode_changed = true;
+
+	return 0;
+}
+
+static void haswell_modeset_global_resources(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
+	int req_cdclk = haswell_calc_cdclk(dev_priv, max_pixel_rate);
+
+	if (req_cdclk != dev_priv->cdclk_freq) {
+		haswell_set_cdclk(dev, req_cdclk);
+	}
+}
+
 static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
 				      struct intel_crtc_state *crtc_state)
 {
@@ -12977,11 +13103,13 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	 * mode set on this crtc.  For other crtcs we need to use the
 	 * adjusted_mode bits in the crtc directly.
 	 */
-	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev)) {
+	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev) || IS_HASWELL(dev)) {
 		if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev))
 			ret = valleyview_modeset_global_pipes(state);
-		else
+		else if (IS_BROADWELL(dev))
 			ret = broadwell_modeset_global_pipes(state);
+		else
+			ret = haswell_modeset_global_pipes(state);
 
 		if (ret)
 			return ret;
@@ -14873,6 +15001,9 @@ static void intel_init_display(struct drm_device *dev)
 		if (IS_BROADWELL(dev))
 			dev_priv->display.modeset_global_resources =
 				broadwell_modeset_global_resources;
+		else
+			dev_priv->display.modeset_global_resources =
+				haswell_modeset_global_resources;
 	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.modeset_global_resources =
 			valleyview_modeset_global_resources;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 8/8] drm/i915: HSW cdclk support

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6530
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                 -1              315/315              314/315
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 0/8] All sort of cdclk stuff

[Damien Lespiau]

On Wed, Jun 03, 2015 at 03:45:06PM +0300, Mika Kahola wrote:
> This patch series rebases Ville's original cdclk patch series
> excluding the ones that	has already been reviewed.
> 
> http://lists.freedesktop.org/archives/intel-gfx/2014-November/055633.html
> 
> The patches are rebased to the latest drm-intel-nightly. The major change
> to the original series is the patch order change for Broadwell and Haswell.
> Now, the Broadwell CD clock patch can be applied before Haswell CD clock
> patch. This way, if decided, the CD clock change for Haswell can be discarded
> without affecting the Broadwell patch.
> 
> Based on the received comments, the style problems are now fixed for this
> patch set.

For the whole series:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 3/8] drm/i915: Unify ilk and hsw .get_aux_clock_divider

[Jani Nikula]

On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> ilk_get_aux_clock_divider() is now a subset of
> hsw_get_aux_clock_divider() so unify them.

I do like the clarity of having these two separate, especially with the
early return in the ilk version and the w/a in the hsw/bdw version.

Moreover there's the subtle round up vs. closest difference, and a
history of aux bugs...

I'm dropping this one, doesn't seem to affect later patches.

BR,
Jani.


>
> v2: Rebased to the latest
> v3: Rebased to the latest
> v4: Fix for patch style problems
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>
> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 23 +++--------------------
>  1 file changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9a6517d..959f115 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -704,23 +704,6 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (index)
> -		return 0;
> -
> -	if (intel_dig_port->port == PORT_A) {
> -		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
> -
> -	} else {
> -		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> -	}
> -}
> -
> -static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> -{
> -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -	struct drm_device *dev = intel_dig_port->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
>  	if (intel_dig_port->port == PORT_A) {
>  		if (index)
>  			return 0;
> @@ -733,7 +716,9 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  		default: return 0;
>  		}
>  	} else  {
> -		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> +		if (index)
> +			return 0;
> +		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
>  	}
>  }
>  
> @@ -5746,8 +5731,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
>  	else if (IS_VALLEYVIEW(dev))
>  		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
> -	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> -		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
>  	else if (HAS_PCH_SPLIT(dev))
>  		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
>  	else
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 8/8] drm/i915: HSW cdclk support

[Jani Nikula]

On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Implement support for changing the cdclk frequency during runtime on
> HSW. VLV/CHV already have support for this, so we can follow their
> example for the most part. Only the actual hardware programming differs,
> the rest is pretty much the same.

I'm chickening out of this one. Thanks to the new patch ordering we can
return to it later if needed.

BR,
Jani.

>
> The pipe pixel rate stuff is handled a bit differently for now due to
> the difference in pch vs. gmch pfit handling. Eventually we should unify
> that part to eliminate what is essentially duplicated code.
>
> v2: Grab rps.hw_lock around sandybridge_pcode_write()
> v3: Rebase due to power well vs. .global_resources() reordering
> v4: Rebased to the latest
> v5: Reformatting 'haswell_modeset_global_pipes' function to
>     support atomic state
> v6: Shuffling the patch order so the Broadwell CD clock patch
>     can be applied before this Haswell CD clock patch
> v7: Fix for patch style problems
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>
> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   2 +
>  drivers/gpu/drm/i915/intel_display.c | 137 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 136 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0f72c0e..418d149 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6705,6 +6705,7 @@ enum skl_disp_power_wells {
>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
>  #define     GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
>  #define     GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> +#define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
>  #define   BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ	0x18
>  #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
>  #define     GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
> @@ -7167,6 +7168,7 @@ enum skl_disp_power_wells {
>  #define  LCPLL_PLL_LOCK			(1<<30)
>  #define  LCPLL_CLK_FREQ_MASK		(3<<26)
>  #define  LCPLL_CLK_FREQ_450		(0<<26)
> +#define  LCPLL_CLK_FREQ_ALT_HSW		(1<<26) /* 337.5 (ULX) or 540 */
>  #define  LCPLL_CLK_FREQ_54O_BDW		(1<<26)
>  #define  LCPLL_CLK_FREQ_337_5_BDW	(2<<26)
>  #define  LCPLL_CLK_FREQ_675_BDW		(3<<26)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b1e2069..4b9907d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5751,7 +5751,16 @@ static void intel_update_max_cdclk(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (IS_BROADWELL(dev))  {
> +	if (IS_HASWELL(dev)) {
> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> +			dev_priv->max_cdclk_freq = 450000;
> +		else if (IS_HSW_ULX(dev))
> +			dev_priv->max_cdclk_freq = 337500;
> +		else if (IS_HSW_ULT(dev))
> +			dev_priv->max_cdclk_freq = 450000;
> +		else
> +			dev_priv->max_cdclk_freq = 540000;
> +	} else if (IS_BROADWELL(dev))  {
>  		/*
>  		 * FIXME with extra cooling we can allow
>  		 * 540 MHz for ULX and 675 Mhz for ULT.
> @@ -9755,6 +9764,80 @@ static int broadwell_calc_cdclk(struct drm_i915_private *dev_priv,
>  	return cdclk;
>  }
>  
> +static void haswell_set_cdclk(struct drm_device *dev, int cdclk)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t val;
> +
> +	if (WARN((I915_READ(LCPLL_CTL) &
> +		  (LCPLL_PLL_DISABLE | LCPLL_PLL_LOCK |
> +		   LCPLL_CD_CLOCK_DISABLE | LCPLL_ROOT_CD_CLOCK_DISABLE |
> +		   LCPLL_CD2X_CLOCK_DISABLE | LCPLL_POWER_DOWN_ALLOW |
> +		   LCPLL_CD_SOURCE_FCLK)) != LCPLL_PLL_LOCK,
> +		 "trying to change cdclk frequency with cdclk not enabled\n"))
> +		return;
> +
> +	val = I915_READ(LCPLL_CTL);
> +	val &= ~LCPLL_CLK_FREQ_MASK;
> +
> +	switch (cdclk) {
> +	case 450000:
> +		val |= LCPLL_CLK_FREQ_450;
> +		break;
> +	case 337500:
> +	case 540000:
> +		val |= LCPLL_CLK_FREQ_ALT_HSW;
> +		break;
> +	default:
> +		WARN(1, "invalid cdclk frequency\n");
> +		return;
> +	}
> +
> +	I915_WRITE(LCPLL_CTL, val);
> +
> +	if (IS_HSW_ULX(dev)) {
> +		mutex_lock(&dev_priv->rps.hw_lock);
> +		sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> +					cdclk == 337500);
> +		mutex_unlock(&dev_priv->rps.hw_lock);
> +	}
> +
> +	intel_update_cdclk(dev);
> +
> +	WARN(cdclk != dev_priv->cdclk_freq,
> +	     "cdclk requested %d kHz but got %d kHz\n",
> +	     cdclk, dev_priv->cdclk_freq);
> +}
> +
> +static int haswell_calc_cdclk(struct drm_i915_private *dev_priv,
> +			      int max_pixel_rate)
> +{
> +	int cdclk;
> +
> +	/*
> +	 * FIXME should also account for plane ratio
> +	 * once 64bpp pixel formats are supported.
> +	 */
> +	if (max_pixel_rate > 450000)
> +		cdclk = 540000;
> +	else if (max_pixel_rate > 337500 || !IS_HSW_ULX(dev_priv))
> +		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;
> +	}
> +
> +	return cdclk;
> +}
> +
>  static int broadwell_modeset_global_pipes(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> @@ -9797,6 +9880,49 @@ static void broadwell_modeset_global_resources(struct drm_atomic_state *state)
>  		broadwell_set_cdclk(dev, req_cdclk);
>  }
>  
> +static int haswell_modeset_global_pipes(struct drm_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int max_pixclk = ilk_max_pixel_rate(dev_priv);
> +	int cdclk, i;
> +
> +	cdclk = haswell_calc_cdclk(dev_priv, max_pixclk);
> +
> +	if (cdclk == dev_priv->cdclk_freq)
> +		return 0;
> +
> +	/* add all active pipes to the state */
> +	for_each_crtc(state->dev, crtc) {
> +		if (!crtc->state->enable)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +	}
> +
> +	/* disable/enable all currently active pipes while we change cdclk */
> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
> +		if (crtc_state->enable)
> +			crtc_state->mode_changed = true;
> +
> +	return 0;
> +}
> +
> +static void haswell_modeset_global_resources(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
> +	int req_cdclk = haswell_calc_cdclk(dev_priv, max_pixel_rate);
> +
> +	if (req_cdclk != dev_priv->cdclk_freq) {
> +		haswell_set_cdclk(dev, req_cdclk);
> +	}
> +}
> +
>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>  				      struct intel_crtc_state *crtc_state)
>  {
> @@ -12977,11 +13103,13 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  	 * mode set on this crtc.  For other crtcs we need to use the
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
> -	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev)) {
> +	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev) || IS_HASWELL(dev)) {
>  		if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev))
>  			ret = valleyview_modeset_global_pipes(state);
> -		else
> +		else if (IS_BROADWELL(dev))
>  			ret = broadwell_modeset_global_pipes(state);
> +		else
> +			ret = haswell_modeset_global_pipes(state);
>  
>  		if (ret)
>  			return ret;
> @@ -14873,6 +15001,9 @@ static void intel_init_display(struct drm_device *dev)
>  		if (IS_BROADWELL(dev))
>  			dev_priv->display.modeset_global_resources =
>  				broadwell_modeset_global_resources;
> +		else
> +			dev_priv->display.modeset_global_resources =
> +				haswell_modeset_global_resources;
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->display.modeset_global_resources =
>  			valleyview_modeset_global_resources;
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 0/8] All sort of cdclk stuff

[Jani Nikula]

On Thu, 04 Jun 2015, Damien Lespiau <damien.lespiau@intel.com> wrote:
> On Wed, Jun 03, 2015 at 03:45:06PM +0300, Mika Kahola wrote:
>> This patch series rebases Ville's original cdclk patch series
>> excluding the ones that	has already been reviewed.
>> 
>> http://lists.freedesktop.org/archives/intel-gfx/2014-November/055633.html
>> 
>> The patches are rebased to the latest drm-intel-nightly. The major change
>> to the original series is the patch order change for Broadwell and Haswell.
>> Now, the Broadwell CD clock patch can be applied before Haswell CD clock
>> patch. This way, if decided, the CD clock change for Haswell can be discarded
>> without affecting the Broadwell patch.
>> 
>> Based on the received comments, the style problems are now fixed for this
>> patch set.
>
> For the whole series:
>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Pushed all except 3 and 8 to drm-intel-next-queued, thanks for the
patches and review.

BR,
Jani.


>
> -- 
> Damien
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 3/8] drm/i915: Unify ilk and hsw .get_aux_clock_divider

[Ville Syrjälä]

On Thu, Jun 04, 2015 at 04:24:43PM +0300, Jani Nikula wrote:
> On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > ilk_get_aux_clock_divider() is now a subset of
> > hsw_get_aux_clock_divider() so unify them.
> 
> I do like the clarity of having these two separate,

I see no clarity. Just pointless duplication which risks different
bugs in different subsets of the platforms.

> especially with the
> early return in the ilk version and the w/a in the hsw/bdw version
> Moreover there's the subtle round up vs. closest difference, and a
> history of aux bugs...

The up vs. closest makes no difference for ILK-IVB since the cdclk is
either 450 or 400 MHz. Anyway I suppose we should just change them all
to use DIV_ROUND_CLOSEST().

> 
> I'm dropping this one, doesn't seem to affect later patches.

No biggie. But I'm fairly sure I'll eventually send patches to store
the rawclk in dev_priv, at which point I'll propose killing all of
these (well, maybe settling for skl+ vs. the rest).

> 
> BR,
> Jani.
> 
> 
> >
> > v2: Rebased to the latest
> > v3: Rebased to the latest
> > v4: Fix for patch style problems
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >
> > Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 23 +++--------------------
> >  1 file changed, 3 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 9a6517d..959f115 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -704,23 +704,6 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > -	if (index)
> > -		return 0;
> > -
> > -	if (intel_dig_port->port == PORT_A) {
> > -		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
> > -
> > -	} else {
> > -		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> > -	}
> > -}
> > -
> > -static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> > -{
> > -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > -	struct drm_device *dev = intel_dig_port->base.base.dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> >  	if (intel_dig_port->port == PORT_A) {
> >  		if (index)
> >  			return 0;
> > @@ -733,7 +716,9 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> >  		default: return 0;
> >  		}
> >  	} else  {
> > -		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> > +		if (index)
> > +			return 0;
> > +		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> >  	}
> >  }
> >  
> > @@ -5746,8 +5731,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >  		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
> >  	else if (IS_VALLEYVIEW(dev))
> >  		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
> > -	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > -		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
> >  	else if (HAS_PCH_SPLIT(dev))
> >  		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
> >  	else
> > -- 
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> 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

Re: [PATCH v6 3/8] drm/i915: Unify ilk and hsw .get_aux_clock_divider

[Jani Nikula]

On Thu, 04 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Jun 04, 2015 at 04:24:43PM +0300, Jani Nikula wrote:
>> On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > ilk_get_aux_clock_divider() is now a subset of
>> > hsw_get_aux_clock_divider() so unify them.
>> 
>> I do like the clarity of having these two separate,
>
> I see no clarity. Just pointless duplication which risks different
> bugs in different subsets of the platforms.

I'm only replying because this is a much larger discussion than just
this patch.

The way I see it, it's currently more likely that we'll have subtle
platform specific bugs because of a bunch of if conditions in functions
combined for the sake of deduplicating 10 lines. When we add more
conditions to such combined functions, it's more likely that we
introduce bugs to platforms we didn't even intend to touch.

The ilk+ code returns early for index != 0. Hey, one subsecond glance
and you know there are no retries.

The ilk+ code does not check for a PCH ID which it doesn't ship
with. One fewer brief WTF pauses while reading the code.

When you read the code, it's most often not just these lowest level
functions. It's a bigger context, and each step here adds to a cognitive
burden you have to carry. If you have to consider more stuff, you'll
swap out something else, and it becomes harder and slower.

As a whole, this is hard stuff, and the readability has to be at a level
where you don't have to pause to wonder what's going on. It all adds up.

BR,
Jani.


>> especially with the
>> early return in the ilk version and the w/a in the hsw/bdw version
>> Moreover there's the subtle round up vs. closest difference, and a
>> history of aux bugs...
>
> The up vs. closest makes no difference for ILK-IVB since the cdclk is
> either 450 or 400 MHz. Anyway I suppose we should just change them all
> to use DIV_ROUND_CLOSEST().
>
>> 
>> I'm dropping this one, doesn't seem to affect later patches.
>
> No biggie. But I'm fairly sure I'll eventually send patches to store
> the rawclk in dev_priv, at which point I'll propose killing all of
> these (well, maybe settling for skl+ vs. the rest).
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > v2: Rebased to the latest
>> > v3: Rebased to the latest
>> > v4: Fix for patch style problems
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> >
>> > Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c | 23 +++--------------------
>> >  1 file changed, 3 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index 9a6517d..959f115 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -704,23 +704,6 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
>> >  	struct drm_i915_private *dev_priv = dev->dev_private;
>> >  
>> > -	if (index)
>> > -		return 0;
>> > -
>> > -	if (intel_dig_port->port == PORT_A) {
>> > -		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
>> > -
>> > -	} else {
>> > -		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
>> > -	}
>> > -}
>> > -
>> > -static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>> > -{
>> > -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> > -	struct drm_device *dev = intel_dig_port->base.base.dev;
>> > -	struct drm_i915_private *dev_priv = dev->dev_private;
>> > -
>> >  	if (intel_dig_port->port == PORT_A) {
>> >  		if (index)
>> >  			return 0;
>> > @@ -733,7 +716,9 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>> >  		default: return 0;
>> >  		}
>> >  	} else  {
>> > -		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
>> > +		if (index)
>> > +			return 0;
>> > +		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
>> >  	}
>> >  }
>> >  
>> > @@ -5746,8 +5731,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>> >  		intel_dp->get_aux_clock_divider = skl_get_aux_clock_divider;
>> >  	else if (IS_VALLEYVIEW(dev))
>> >  		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
>> > -	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>> > -		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
>> >  	else if (HAS_PCH_SPLIT(dev))
>> >  		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
>> >  	else
>> > -- 
>> > 1.9.1
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Daniel Vetter]

On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Rather than reading out the current cdclk value use the cached value we
> have tucked away in dev_priv.
> 
> v2: Rebased to the latest
> v3: Rebased to the latest
> v4: Fix for patch style problems
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>

This patch needs to be extended to also cover the recently added
skl_max_scale. Tvrtko has recently written a patch to add some checks to
that code too, would be good to resurrect that too. Chandra can help with
any questions wrt the skl scaler code.

Cheers, Daniel

> 
> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>  drivers/gpu/drm/i915/intel_dp.c      | 5 +++--
>  drivers/gpu/drm/i915/intel_pm.c      | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9cf1553..d1dd8ab 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6610,8 +6610,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  
>  	/* FIXME should check pixel clock limits on all platforms */
>  	if (INTEL_INFO(dev)->gen < 4) {
> -		int clock_limit =
> -			dev_priv->display.get_display_clock_speed(dev);
> +		int clock_limit = dev_priv->cdclk_freq;
>  
>  		/*
>  		 * Enable pixel doubling when the dot clock
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6f525093..9a6517d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -708,7 +708,8 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  		return 0;
>  
>  	if (intel_dig_port->port == PORT_A) {
> -		return DIV_ROUND_UP(dev_priv->display.get_display_clock_speed(dev), 2000);
> +		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
> +
>  	} else {
>  		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
>  	}
> @@ -723,7 +724,7 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  	if (intel_dig_port->port == PORT_A) {
>  		if (index)
>  			return 0;
> -		return DIV_ROUND_CLOSEST(dev_priv->display.get_display_clock_speed(dev), 2000);
> +		return DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 2000);
>  	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
>  		/* Workaround for non-ULT HSW */
>  		switch (index) {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index eadc15c..5db429e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1815,7 +1815,7 @@ hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
>  	linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
>  				     mode->crtc_clock);
>  	ips_linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
> -					 dev_priv->display.get_display_clock_speed(dev_priv->dev));
> +					 dev_priv->cdclk_freq);
>  
>  	return PIPE_WM_LINETIME_IPS_LINETIME(ips_linetime) |
>  	       PIPE_WM_LINETIME_TIME(linetime);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Damien Lespiau]

On Mon, Jun 15, 2015 at 01:54:40PM +0200, Daniel Vetter wrote:
> On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Rather than reading out the current cdclk value use the cached value we
> > have tucked away in dev_priv.
> > 
> > v2: Rebased to the latest
> > v3: Rebased to the latest
> > v4: Fix for patch style problems
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> 
> This patch needs to be extended to also cover the recently added
> skl_max_scale. Tvrtko has recently written a patch to add some checks to
> that code too, would be good to resurrect that too. Chandra can help with
> any questions wrt the skl scaler code.

Not quite I'm afraid. The CDCLK used in skl_max_scale() has to be part
of the atomic state, even bumping CDCLK if possible/needed.

If you use the cached cdclk in skl_max_scale(), it won't do the right
thing when CDCLK is off (ie cached frew is the fallback 24Mhz ref clock)
and we try to do the first modeset before waking up the display.

I filed a bug about it already to track it:

  https://bugs.freedesktop.org/show_bug.cgi?id=90874

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Kahola, Mika]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, June 15, 2015 2:55 PM
> To: Kahola, Mika
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value
> 
> On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Rather than reading out the current cdclk value use the cached value
> > we have tucked away in dev_priv.
> >
> > v2: Rebased to the latest
> > v3: Rebased to the latest
> > v4: Fix for patch style problems
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> 
> This patch needs to be extended to also cover the recently added
> skl_max_scale. Tvrtko has recently written a patch to add some checks to
> that code too, would be good to resurrect that too. Chandra can help with
> any questions wrt the skl scaler code.
> 
> Cheers, Daniel
Jani has pushed these patches already so maybe this is an item for a separate patch?

Cheers,
Mika

> 
> >
> > Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 +--
> >  drivers/gpu/drm/i915/intel_dp.c      | 5 +++--
> >  drivers/gpu/drm/i915/intel_pm.c      | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 9cf1553..d1dd8ab 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6610,8 +6610,7 @@ static int intel_crtc_compute_config(struct
> > intel_crtc *crtc,
> >
> >  	/* FIXME should check pixel clock limits on all platforms */
> >  	if (INTEL_INFO(dev)->gen < 4) {
> > -		int clock_limit =
> > -			dev_priv->display.get_display_clock_speed(dev);
> > +		int clock_limit = dev_priv->cdclk_freq;
> >
> >  		/*
> >  		 * Enable pixel doubling when the dot clock diff --git
> > a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 6f525093..9a6517d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -708,7 +708,8 @@ static uint32_t ilk_get_aux_clock_divider(struct
> intel_dp *intel_dp, int index)
> >  		return 0;
> >
> >  	if (intel_dig_port->port == PORT_A) {
> > -		return DIV_ROUND_UP(dev_priv-
> >display.get_display_clock_speed(dev), 2000);
> > +		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
> > +
> >  	} else {
> >  		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> >  	}
> > @@ -723,7 +724,7 @@ static uint32_t hsw_get_aux_clock_divider(struct
> intel_dp *intel_dp, int index)
> >  	if (intel_dig_port->port == PORT_A) {
> >  		if (index)
> >  			return 0;
> > -		return DIV_ROUND_CLOSEST(dev_priv-
> >display.get_display_clock_speed(dev), 2000);
> > +		return DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 2000);
> >  	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
> >  		/* Workaround for non-ULT HSW */
> >  		switch (index) {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c index eadc15c..5db429e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1815,7 +1815,7 @@ hsw_compute_linetime_wm(struct drm_device
> *dev, struct drm_crtc *crtc)
> >  	linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
> >  				     mode->crtc_clock);
> >  	ips_linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
> > -					 dev_priv-
> >display.get_display_clock_speed(dev_priv->dev));
> > +					 dev_priv->cdclk_freq);
> >
> >  	return PIPE_WM_LINETIME_IPS_LINETIME(ips_linetime) |
> >  	       PIPE_WM_LINETIME_TIME(linetime);
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Tvrtko Ursulin]

On 06/15/2015 01:14 PM, Damien Lespiau wrote:
> On Mon, Jun 15, 2015 at 01:54:40PM +0200, Daniel Vetter wrote:
>> On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Rather than reading out the current cdclk value use the cached value we
>>> have tucked away in dev_priv.
>>>
>>> v2: Rebased to the latest
>>> v3: Rebased to the latest
>>> v4: Fix for patch style problems
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>>
>> This patch needs to be extended to also cover the recently added
>> skl_max_scale. Tvrtko has recently written a patch to add some checks to
>> that code too, would be good to resurrect that too. Chandra can help with
>> any questions wrt the skl scaler code.
>
> Not quite I'm afraid. The CDCLK used in skl_max_scale() has to be part
> of the atomic state, even bumping CDCLK if possible/needed.
>
> If you use the cached cdclk in skl_max_scale(), it won't do the right
> thing when CDCLK is off (ie cached frew is the fallback 24Mhz ref clock)
> and we try to do the first modeset before waking up the display.
>
> I filed a bug about it already to track it:
>
>    https://bugs.freedesktop.org/show_bug.cgi?id=90874

I know nothing about these specific clocks, but FWIW, my patch was only 
about enabling new platforms - making skl_max_scale more robust in cases 
where clock querying does not yet work correctly.

My reasoning was based on a comment from Ville that one of those two 
clocks must never be lower than the other.

So it sounded reasonable to ignore such cases ie. assume no scaling is 
possible and allow a normal (unscaled) modeset to succeed rather than 
fail it and display nothing.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Daniel Vetter]

On Mon, Jun 15, 2015 at 12:21:17PM +0000, Kahola, Mika wrote:
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Monday, June 15, 2015 2:55 PM
> > To: Kahola, Mika
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value
> > 
> > On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Rather than reading out the current cdclk value use the cached value
> > > we have tucked away in dev_priv.
> > >
> > > v2: Rebased to the latest
> > > v3: Rebased to the latest
> > > v4: Fix for patch style problems
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > 
> > This patch needs to be extended to also cover the recently added
> > skl_max_scale. Tvrtko has recently written a patch to add some checks to
> > that code too, would be good to resurrect that too. Chandra can help with
> > any questions wrt the skl scaler code.
> > 
> > Cheers, Daniel
> Jani has pushed these patches already so maybe this is an item for a separate patch?

Yeah that's what I've meant with extending. Please work together with
Damien and Chandra and Maarten in figuring out what exactly is needed
here.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Damien Lespiau]

On Mon, Jun 15, 2015 at 01:40:24PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/15/2015 01:14 PM, Damien Lespiau wrote:
> >On Mon, Jun 15, 2015 at 01:54:40PM +0200, Daniel Vetter wrote:
> >>On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> >>>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>>Rather than reading out the current cdclk value use the cached value we
> >>>have tucked away in dev_priv.
> >>>
> >>>v2: Rebased to the latest
> >>>v3: Rebased to the latest
> >>>v4: Fix for patch style problems
> >>>
> >>>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >>
> >>This patch needs to be extended to also cover the recently added
> >>skl_max_scale. Tvrtko has recently written a patch to add some checks to
> >>that code too, would be good to resurrect that too. Chandra can help with
> >>any questions wrt the skl scaler code.
> >
> >Not quite I'm afraid. The CDCLK used in skl_max_scale() has to be part
> >of the atomic state, even bumping CDCLK if possible/needed.
> >
> >If you use the cached cdclk in skl_max_scale(), it won't do the right
> >thing when CDCLK is off (ie cached frew is the fallback 24Mhz ref clock)
> >and we try to do the first modeset before waking up the display.
> >
> >I filed a bug about it already to track it:
> >
> >   https://bugs.freedesktop.org/show_bug.cgi?id=90874
> 
> I know nothing about these specific clocks, but FWIW, my patch was only
> about enabling new platforms - making skl_max_scale more robust in cases
> where clock querying does not yet work correctly.
> 
> My reasoning was based on a comment from Ville that one of those two clocks
> must never be lower than the other.
> 
> So it sounded reasonable to ignore such cases ie. assume no scaling is
> possible and allow a normal (unscaled) modeset to succeed rather than fail
> it and display nothing.

So to be more specific, I believe this is because we detect CDCLK as
being "disabled" or on the ref clock in simulation?

Generally speaking, it's questionable if we want to work around such
limitations in the code like that, I'd rather go for defaulting to max
CDCLK in simulation.

In this particular case, we really shouldn't get cdclk < crtc_clock at
this point, I'd expect the cdclk we use (probably part of the atomic
state) to be bumped to cover crtc_clock prior to plane checks (See
Marteen's [PATCH v3 19/19] drm/i915: Make cdclk part of the atomic
state.), I guess we could add a WARN_ON(cdclk < crtc_clock) in
skl_max_scale() to ensure that's indeed the case?

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Damien Lespiau]

On Mon, Jun 15, 2015 at 03:05:27PM +0200, Daniel Vetter wrote:
> On Mon, Jun 15, 2015 at 12:21:17PM +0000, Kahola, Mika wrote:
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > > Vetter
> > > Sent: Monday, June 15, 2015 2:55 PM
> > > To: Kahola, Mika
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value
> > > 
> > > On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Rather than reading out the current cdclk value use the cached value
> > > > we have tucked away in dev_priv.
> > > >
> > > > v2: Rebased to the latest
> > > > v3: Rebased to the latest
> > > > v4: Fix for patch style problems
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > 
> > > This patch needs to be extended to also cover the recently added
> > > skl_max_scale. Tvrtko has recently written a patch to add some checks to
> > > that code too, would be good to resurrect that too. Chandra can help with
> > > any questions wrt the skl scaler code.
> > > 
> > > Cheers, Daniel
> > Jani has pushed these patches already so maybe this is an item for a separate patch?
> 
> Yeah that's what I've meant with extending. Please work together with
> Damien and Chandra and Maarten in figuring out what exactly is needed
> here.

I think Maarten has it already covered in:

  [PATCH v3 19/19] drm/i915: Make cdclk part of the atomic state.

but I probably should test it :)

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Tvrtko Ursulin]

On 06/15/2015 02:09 PM, Damien Lespiau wrote:
> On Mon, Jun 15, 2015 at 01:40:24PM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/15/2015 01:14 PM, Damien Lespiau wrote:
>>> On Mon, Jun 15, 2015 at 01:54:40PM +0200, Daniel Vetter wrote:
>>>> On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>
>>>>> Rather than reading out the current cdclk value use the cached value we
>>>>> have tucked away in dev_priv.
>>>>>
>>>>> v2: Rebased to the latest
>>>>> v3: Rebased to the latest
>>>>> v4: Fix for patch style problems
>>>>>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>>>>
>>>> This patch needs to be extended to also cover the recently added
>>>> skl_max_scale. Tvrtko has recently written a patch to add some checks to
>>>> that code too, would be good to resurrect that too. Chandra can help with
>>>> any questions wrt the skl scaler code.
>>>
>>> Not quite I'm afraid. The CDCLK used in skl_max_scale() has to be part
>>> of the atomic state, even bumping CDCLK if possible/needed.
>>>
>>> If you use the cached cdclk in skl_max_scale(), it won't do the right
>>> thing when CDCLK is off (ie cached frew is the fallback 24Mhz ref clock)
>>> and we try to do the first modeset before waking up the display.
>>>
>>> I filed a bug about it already to track it:
>>>
>>>    https://bugs.freedesktop.org/show_bug.cgi?id=90874
>>
>> I know nothing about these specific clocks, but FWIW, my patch was only
>> about enabling new platforms - making skl_max_scale more robust in cases
>> where clock querying does not yet work correctly.
>>
>> My reasoning was based on a comment from Ville that one of those two clocks
>> must never be lower than the other.
>>
>> So it sounded reasonable to ignore such cases ie. assume no scaling is
>> possible and allow a normal (unscaled) modeset to succeed rather than fail
>> it and display nothing.
>
> So to be more specific, I believe this is because we detect CDCLK as
> being "disabled" or on the ref clock in simulation?

Probably a reference clock. It definitely wasn't zero since 
skl_max_scale already handles that. But I forgot the exact details.

> Generally speaking, it's questionable if we want to work around such
> limitations in the code like that, I'd rather go for defaulting to max
> CDCLK in simulation.
>
> In this particular case, we really shouldn't get cdclk < crtc_clock at
> this point, I'd expect the cdclk we use (probably part of the atomic
> state) to be bumped to cover crtc_clock prior to plane checks (See
> Marteen's [PATCH v3 19/19] drm/i915: Make cdclk part of the atomic
> state.), I guess we could add a WARN_ON(cdclk < crtc_clock) in
> skl_max_scale() to ensure that's indeed the case?

WARN_ON sounds fine to me. For the other considerations - you're the 
expert. :)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Konduru, Chandra]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Damien Lespiau
> Sent: Monday, June 15, 2015 6:15 AM
> To: Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value
> 
> On Mon, Jun 15, 2015 at 03:05:27PM +0200, Daniel Vetter wrote:
> > On Mon, Jun 15, 2015 at 12:21:17PM +0000, Kahola, Mika wrote:
> > > > -----Original Message-----
> > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > > > Vetter
> > > > Sent: Monday, June 15, 2015 2:55 PM
> > > > To: Kahola, Mika
> > > > Cc: intel-gfx@lists.freedesktop.org
> > > > Subject: Re: [Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value
> > > >
> > > > On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Rather than reading out the current cdclk value use the cached value
> > > > > we have tucked away in dev_priv.
> > > > >
> > > > > v2: Rebased to the latest
> > > > > v3: Rebased to the latest
> > > > > v4: Fix for patch style problems
> > > > >
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > >
> > > > This patch needs to be extended to also cover the recently added
> > > > skl_max_scale. Tvrtko has recently written a patch to add some checks to
> > > > that code too, would be good to resurrect that too. Chandra can help with
> > > > any questions wrt the skl scaler code.
> > > >
> > > > Cheers, Daniel
> > > Jani has pushed these patches already so maybe this is an item for a separate
> patch?
> >
> > Yeah that's what I've meant with extending. Please work together with
> > Damien and Chandra and Maarten in figuring out what exactly is needed
> > here.
> 
> I think Maarten has it already covered in:
> 
>   [PATCH v3 19/19] drm/i915: Make cdclk part of the atomic state.
> 
> but I probably should test it :)

Yes, cdclk < crtc_clock shouldn't happen this late unless one of the value
is stale (cdclk?). 

Checked Maarten's other patch, and it should address this issue.


> 
> --
> Damien
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 7/8] drm/i915: BDW clock change support

[Jani Nikula]

On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add support for changing cdclk frequency during runtime on BDW. The
> procedure is quite a bit different on BDW from the one on HSW, so
> add a separate function for it.
>
> Also with IPS enabled the actual pixel rate mustn't exceed 95% of cdclk,
> so take that into account when computing the max pixel rate.
>
> v2: Grab rps.hw_lock around sandybridge_pcode_write()
> v3: Rebase due to power well vs. .global_resources() reordering
> v4: Rebased to the latest
> v5: Rebased to the latest
> v6: Patch order shuffle so that Broadwell CD clock change is
>     applied before the patch for Haswell CD clock change
> v7: Fix for patch style problems
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>

This patch hard hangs my BDW NUC at boot when both DP and HDMI are
connected. Either DP or HDMI alone are good, same with hotplugging the
other afterwards. Booting to grub with both connected, and unplugging
HDMI before loading the kernel also reproduces the issue.

It looks like the problem boils down to the BIOS setting up a smaller
resolution on the DP display when both are connected, and this patch
fails to cope with that on i915 load.

BR,
Jani.



>
> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   2 +
>  drivers/gpu/drm/i915/intel_display.c | 216 +++++++++++++++++++++++++++++++++--
>  2 files changed, 208 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7213224..0f72c0e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6705,6 +6705,7 @@ enum skl_disp_power_wells {
>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
>  #define     GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
>  #define     GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> +#define   BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ	0x18
>  #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
>  #define     GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
>  #define     GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
> @@ -7170,6 +7171,7 @@ enum skl_disp_power_wells {
>  #define  LCPLL_CLK_FREQ_337_5_BDW	(2<<26)
>  #define  LCPLL_CLK_FREQ_675_BDW		(3<<26)
>  #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
> +#define  LCPLL_ROOT_CD_CLOCK_DISABLE	(1<<24)
>  #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
>  #define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
>  #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c3f01aa..b1e2069 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5751,7 +5751,22 @@ static void intel_update_max_cdclk(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (IS_VALLEYVIEW(dev)) {
> +	if (IS_BROADWELL(dev))  {
> +		/*
> +		 * FIXME with extra cooling we can allow
> +		 * 540 MHz for ULX and 675 Mhz for ULT.
> +		 * How can we know if extra cooling is
> +		 * available? PCI ID, VTB, something else?
> +		 */
> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> +			dev_priv->max_cdclk_freq = 450000;
> +		else if (IS_BDW_ULX(dev))
> +			dev_priv->max_cdclk_freq = 450000;
> +		else if (IS_BDW_ULT(dev))
> +			dev_priv->max_cdclk_freq = 540000;
> +		else
> +			dev_priv->max_cdclk_freq = 675000;
> +	} else if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->max_cdclk_freq = 400000;
>  	} else {
>  		/* otherwise assume cdclk is fixed */
> @@ -6621,13 +6636,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>  		return true;
>  
>  	/*
> -	 * FIXME if we compare against max we should then
> -	 * increase the cdclk frequency when the current
> -	 * value is too low. The other option is to compare
> -	 * against the cdclk frequency we're going have post
> -	 * modeset (ie. one we computed using other constraints).
> -	 * Need to measure whether using a lower cdclk w/o IPS
> -	 * is better or worse than a higher cdclk w/ IPS.
> +	 * We compare against max which means we must take
> +	 * the increased cdclk requirement into account when
> +	 * calculating the new cdclk.
> +	 *
> +	 * Should measure whether using a lower cdclk w/o IPS
>  	 */
>  	return ilk_pipe_pixel_rate(pipe_config) <=
>  		dev_priv->max_cdclk_freq * 95 / 100;
> @@ -9608,6 +9621,182 @@ static void broxton_modeset_global_resources(struct drm_atomic_state *old_state)
>  		broxton_set_cdclk(dev, req_cdclk);
>  }
>  
> +/* compute the max rate for new configuration */
> +static int ilk_max_pixel_rate(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_crtc *intel_crtc;
> +	struct drm_crtc *crtc;
> +	int max_pixel_rate = 0;
> +	int pixel_rate;
> +
> +	for_each_crtc(dev, crtc) {
> +		if (!crtc->state->enable)
> +			continue;
> +
> +		intel_crtc = to_intel_crtc(crtc);
> +		pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
> +
> +		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> +		if (IS_BROADWELL(dev) && intel_crtc->config->ips_enabled)
> +			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> +
> +		max_pixel_rate = max(max_pixel_rate, pixel_rate);
> +	}
> +
> +	return max_pixel_rate;
> +}
> +
> +static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t val, data;
> +	int ret;
> +
> +	if (WARN((I915_READ(LCPLL_CTL) &
> +		  (LCPLL_PLL_DISABLE | LCPLL_PLL_LOCK |
> +		   LCPLL_CD_CLOCK_DISABLE | LCPLL_ROOT_CD_CLOCK_DISABLE |
> +		   LCPLL_CD2X_CLOCK_DISABLE | LCPLL_POWER_DOWN_ALLOW |
> +		   LCPLL_CD_SOURCE_FCLK)) != LCPLL_PLL_LOCK,
> +		 "trying to change cdclk frequency with cdclk not enabled\n"))
> +		return;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	ret = sandybridge_pcode_write(dev_priv,
> +				      BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ, 0x0);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +	if (ret) {
> +		DRM_ERROR("failed to inform pcode about cdclk change\n");
> +		return;
> +	}
> +
> +	val = I915_READ(LCPLL_CTL);
> +	val |= LCPLL_CD_SOURCE_FCLK;
> +	I915_WRITE(LCPLL_CTL, val);
> +
> +	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
> +			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
> +		DRM_ERROR("Switching to FCLK failed\n");
> +
> +	val = I915_READ(LCPLL_CTL);
> +	val &= ~LCPLL_CLK_FREQ_MASK;
> +
> +	switch (cdclk) {
> +	case 450000:
> +		val |= LCPLL_CLK_FREQ_450;
> +		data = 0;
> +		break;
> +	case 540000:
> +		val |= LCPLL_CLK_FREQ_54O_BDW;
> +		data = 1;
> +		break;
> +	case 337500:
> +		val |= LCPLL_CLK_FREQ_337_5_BDW;
> +		data = 2;
> +		break;
> +	case 675000:
> +		val |= LCPLL_CLK_FREQ_675_BDW;
> +		data = 3;
> +		break;
> +	default:
> +		WARN(1, "invalid cdclk frequency\n");
> +		return;
> +	}
> +
> +	I915_WRITE(LCPLL_CTL, val);
> +
> +	val = I915_READ(LCPLL_CTL);
> +	val &= ~LCPLL_CD_SOURCE_FCLK;
> +	I915_WRITE(LCPLL_CTL, val);
> +
> +	if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
> +				LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
> +		DRM_ERROR("Switching back to LCPLL failed\n");
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, data);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	intel_update_cdclk(dev);
> +
> +	WARN(cdclk != dev_priv->cdclk_freq,
> +	     "cdclk requested %d kHz but got %d kHz\n",
> +	     cdclk, dev_priv->cdclk_freq);
> +}
> +
> +static int broadwell_calc_cdclk(struct drm_i915_private *dev_priv,
> +			      int max_pixel_rate)
> +{
> +	int cdclk;
> +
> +	/*
> +	 * FIXME should also account for plane ratio
> +	 * once 64bpp pixel formats are supported.
> +	 */
> +	if (max_pixel_rate > 540000)
> +		cdclk = 675000;
> +	else if (max_pixel_rate > 450000)
> +		cdclk = 540000;
> +	else if (max_pixel_rate > 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;
> +	}
> +
> +	return cdclk;
> +}
> +
> +static int broadwell_modeset_global_pipes(struct drm_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int max_pixclk = ilk_max_pixel_rate(dev_priv);
> +	int cdclk, i;
> +
> +	cdclk = broadwell_calc_cdclk(dev_priv, max_pixclk);
> +
> +	if (cdclk == dev_priv->cdclk_freq)
> +		return 0;
> +
> +	/* add all active pipes to the state */
> +	for_each_crtc(state->dev, crtc) {
> +		if (!crtc->state->enable)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +	}
> +
> +	/* disable/enable all currently active pipes while we change cdclk */
> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
> +		if (crtc_state->enable)
> +			crtc_state->mode_changed = true;
> +
> +	return 0;
> +}
> +
> +static void broadwell_modeset_global_resources(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
> +	int req_cdclk = broadwell_calc_cdclk(dev_priv, max_pixel_rate);
> +
> +	if (req_cdclk != dev_priv->cdclk_freq)
> +		broadwell_set_cdclk(dev, req_cdclk);
> +}
> +
>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>  				      struct intel_crtc_state *crtc_state)
>  {
> @@ -12788,8 +12977,12 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  	 * mode set on this crtc.  For other crtcs we need to use the
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
> -	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev)) {
> -		ret = valleyview_modeset_global_pipes(state);
> +	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev)) {
> +		if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev))
> +			ret = valleyview_modeset_global_pipes(state);
> +		else
> +			ret = broadwell_modeset_global_pipes(state);
> +
>  		if (ret)
>  			return ret;
>  	}
> @@ -14677,6 +14870,9 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
> +		if (IS_BROADWELL(dev))
> +			dev_priv->display.modeset_global_resources =
> +				broadwell_modeset_global_resources;
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->display.modeset_global_resources =
>  			valleyview_modeset_global_resources;
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 7/8] drm/i915: BDW clock change support

[Jani Nikula]

On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Add support for changing cdclk frequency during runtime on BDW. The
>> procedure is quite a bit different on BDW from the one on HSW, so
>> add a separate function for it.
>>
>> Also with IPS enabled the actual pixel rate mustn't exceed 95% of cdclk,
>> so take that into account when computing the max pixel rate.
>>
>> v2: Grab rps.hw_lock around sandybridge_pcode_write()
>> v3: Rebase due to power well vs. .global_resources() reordering
>> v4: Rebased to the latest
>> v5: Rebased to the latest
>> v6: Patch order shuffle so that Broadwell CD clock change is
>>     applied before the patch for Haswell CD clock change
>> v7: Fix for patch style problems
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>
> This patch hard hangs my BDW NUC at boot when both DP and HDMI are
> connected. Either DP or HDMI alone are good, same with hotplugging the
> other afterwards. Booting to grub with both connected, and unplugging
> HDMI before loading the kernel also reproduces the issue.
>
> It looks like the problem boils down to the BIOS setting up a smaller
> resolution on the DP display when both are connected, and this patch
> fails to cope with that on i915 load.

By "this patch" I obviously refer to

commit b432e5cfd5e92127ad2dd83bfc3083f1dbce43fb
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Wed Jun 3 15:45:13 2015 +0300

    drm/i915: BDW clock change support

and everything works for the commit before that.

BR,
Jani.





>
> BR,
> Jani.
>
>
>
>>
>> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h      |   2 +
>>  drivers/gpu/drm/i915/intel_display.c | 216 +++++++++++++++++++++++++++++++++--
>>  2 files changed, 208 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 7213224..0f72c0e 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6705,6 +6705,7 @@ enum skl_disp_power_wells {
>>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
>>  #define     GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
>>  #define     GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
>> +#define   BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ	0x18
>>  #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
>>  #define     GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
>>  #define     GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
>> @@ -7170,6 +7171,7 @@ enum skl_disp_power_wells {
>>  #define  LCPLL_CLK_FREQ_337_5_BDW	(2<<26)
>>  #define  LCPLL_CLK_FREQ_675_BDW		(3<<26)
>>  #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
>> +#define  LCPLL_ROOT_CD_CLOCK_DISABLE	(1<<24)
>>  #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
>>  #define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
>>  #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index c3f01aa..b1e2069 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5751,7 +5751,22 @@ static void intel_update_max_cdclk(struct drm_device *dev)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  
>> -	if (IS_VALLEYVIEW(dev)) {
>> +	if (IS_BROADWELL(dev))  {
>> +		/*
>> +		 * FIXME with extra cooling we can allow
>> +		 * 540 MHz for ULX and 675 Mhz for ULT.
>> +		 * How can we know if extra cooling is
>> +		 * available? PCI ID, VTB, something else?
>> +		 */
>> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
>> +			dev_priv->max_cdclk_freq = 450000;
>> +		else if (IS_BDW_ULX(dev))
>> +			dev_priv->max_cdclk_freq = 450000;
>> +		else if (IS_BDW_ULT(dev))
>> +			dev_priv->max_cdclk_freq = 540000;
>> +		else
>> +			dev_priv->max_cdclk_freq = 675000;
>> +	} else if (IS_VALLEYVIEW(dev)) {
>>  		dev_priv->max_cdclk_freq = 400000;
>>  	} else {
>>  		/* otherwise assume cdclk is fixed */
>> @@ -6621,13 +6636,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>>  		return true;
>>  
>>  	/*
>> -	 * FIXME if we compare against max we should then
>> -	 * increase the cdclk frequency when the current
>> -	 * value is too low. The other option is to compare
>> -	 * against the cdclk frequency we're going have post
>> -	 * modeset (ie. one we computed using other constraints).
>> -	 * Need to measure whether using a lower cdclk w/o IPS
>> -	 * is better or worse than a higher cdclk w/ IPS.
>> +	 * We compare against max which means we must take
>> +	 * the increased cdclk requirement into account when
>> +	 * calculating the new cdclk.
>> +	 *
>> +	 * Should measure whether using a lower cdclk w/o IPS
>>  	 */
>>  	return ilk_pipe_pixel_rate(pipe_config) <=
>>  		dev_priv->max_cdclk_freq * 95 / 100;
>> @@ -9608,6 +9621,182 @@ static void broxton_modeset_global_resources(struct drm_atomic_state *old_state)
>>  		broxton_set_cdclk(dev, req_cdclk);
>>  }
>>  
>> +/* compute the max rate for new configuration */
>> +static int ilk_max_pixel_rate(struct drm_i915_private *dev_priv)
>> +{
>> +	struct drm_device *dev = dev_priv->dev;
>> +	struct intel_crtc *intel_crtc;
>> +	struct drm_crtc *crtc;
>> +	int max_pixel_rate = 0;
>> +	int pixel_rate;
>> +
>> +	for_each_crtc(dev, crtc) {
>> +		if (!crtc->state->enable)
>> +			continue;
>> +
>> +		intel_crtc = to_intel_crtc(crtc);
>> +		pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
>> +
>> +		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>> +		if (IS_BROADWELL(dev) && intel_crtc->config->ips_enabled)
>> +			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
>> +
>> +		max_pixel_rate = max(max_pixel_rate, pixel_rate);
>> +	}
>> +
>> +	return max_pixel_rate;
>> +}
>> +
>> +static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	uint32_t val, data;
>> +	int ret;
>> +
>> +	if (WARN((I915_READ(LCPLL_CTL) &
>> +		  (LCPLL_PLL_DISABLE | LCPLL_PLL_LOCK |
>> +		   LCPLL_CD_CLOCK_DISABLE | LCPLL_ROOT_CD_CLOCK_DISABLE |
>> +		   LCPLL_CD2X_CLOCK_DISABLE | LCPLL_POWER_DOWN_ALLOW |
>> +		   LCPLL_CD_SOURCE_FCLK)) != LCPLL_PLL_LOCK,
>> +		 "trying to change cdclk frequency with cdclk not enabled\n"))
>> +		return;
>> +
>> +	mutex_lock(&dev_priv->rps.hw_lock);
>> +	ret = sandybridge_pcode_write(dev_priv,
>> +				      BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ, 0x0);
>> +	mutex_unlock(&dev_priv->rps.hw_lock);
>> +	if (ret) {
>> +		DRM_ERROR("failed to inform pcode about cdclk change\n");
>> +		return;
>> +	}
>> +
>> +	val = I915_READ(LCPLL_CTL);
>> +	val |= LCPLL_CD_SOURCE_FCLK;
>> +	I915_WRITE(LCPLL_CTL, val);
>> +
>> +	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
>> +			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
>> +		DRM_ERROR("Switching to FCLK failed\n");
>> +
>> +	val = I915_READ(LCPLL_CTL);
>> +	val &= ~LCPLL_CLK_FREQ_MASK;
>> +
>> +	switch (cdclk) {
>> +	case 450000:
>> +		val |= LCPLL_CLK_FREQ_450;
>> +		data = 0;
>> +		break;
>> +	case 540000:
>> +		val |= LCPLL_CLK_FREQ_54O_BDW;
>> +		data = 1;
>> +		break;
>> +	case 337500:
>> +		val |= LCPLL_CLK_FREQ_337_5_BDW;
>> +		data = 2;
>> +		break;
>> +	case 675000:
>> +		val |= LCPLL_CLK_FREQ_675_BDW;
>> +		data = 3;
>> +		break;
>> +	default:
>> +		WARN(1, "invalid cdclk frequency\n");
>> +		return;
>> +	}
>> +
>> +	I915_WRITE(LCPLL_CTL, val);
>> +
>> +	val = I915_READ(LCPLL_CTL);
>> +	val &= ~LCPLL_CD_SOURCE_FCLK;
>> +	I915_WRITE(LCPLL_CTL, val);
>> +
>> +	if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
>> +				LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
>> +		DRM_ERROR("Switching back to LCPLL failed\n");
>> +
>> +	mutex_lock(&dev_priv->rps.hw_lock);
>> +	sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, data);
>> +	mutex_unlock(&dev_priv->rps.hw_lock);
>> +
>> +	intel_update_cdclk(dev);
>> +
>> +	WARN(cdclk != dev_priv->cdclk_freq,
>> +	     "cdclk requested %d kHz but got %d kHz\n",
>> +	     cdclk, dev_priv->cdclk_freq);
>> +}
>> +
>> +static int broadwell_calc_cdclk(struct drm_i915_private *dev_priv,
>> +			      int max_pixel_rate)
>> +{
>> +	int cdclk;
>> +
>> +	/*
>> +	 * FIXME should also account for plane ratio
>> +	 * once 64bpp pixel formats are supported.
>> +	 */
>> +	if (max_pixel_rate > 540000)
>> +		cdclk = 675000;
>> +	else if (max_pixel_rate > 450000)
>> +		cdclk = 540000;
>> +	else if (max_pixel_rate > 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;
>> +	}
>> +
>> +	return cdclk;
>> +}
>> +
>> +static int broadwell_modeset_global_pipes(struct drm_atomic_state *state)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *crtc_state;
>> +	int max_pixclk = ilk_max_pixel_rate(dev_priv);
>> +	int cdclk, i;
>> +
>> +	cdclk = broadwell_calc_cdclk(dev_priv, max_pixclk);
>> +
>> +	if (cdclk == dev_priv->cdclk_freq)
>> +		return 0;
>> +
>> +	/* add all active pipes to the state */
>> +	for_each_crtc(state->dev, crtc) {
>> +		if (!crtc->state->enable)
>> +			continue;
>> +
>> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>> +		if (IS_ERR(crtc_state))
>> +			return PTR_ERR(crtc_state);
>> +	}
>> +
>> +	/* disable/enable all currently active pipes while we change cdclk */
>> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
>> +		if (crtc_state->enable)
>> +			crtc_state->mode_changed = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static void broadwell_modeset_global_resources(struct drm_atomic_state *state)
>> +{
>> +	struct drm_device *dev = state->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
>> +	int req_cdclk = broadwell_calc_cdclk(dev_priv, max_pixel_rate);
>> +
>> +	if (req_cdclk != dev_priv->cdclk_freq)
>> +		broadwell_set_cdclk(dev, req_cdclk);
>> +}
>> +
>>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>>  				      struct intel_crtc_state *crtc_state)
>>  {
>> @@ -12788,8 +12977,12 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>>  	 * mode set on this crtc.  For other crtcs we need to use the
>>  	 * adjusted_mode bits in the crtc directly.
>>  	 */
>> -	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev)) {
>> -		ret = valleyview_modeset_global_pipes(state);
>> +	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev)) {
>> +		if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev))
>> +			ret = valleyview_modeset_global_pipes(state);
>> +		else
>> +			ret = broadwell_modeset_global_pipes(state);
>> +
>>  		if (ret)
>>  			return ret;
>>  	}
>> @@ -14677,6 +14870,9 @@ static void intel_init_display(struct drm_device *dev)
>>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
>>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>> +		if (IS_BROADWELL(dev))
>> +			dev_priv->display.modeset_global_resources =
>> +				broadwell_modeset_global_resources;
>>  	} else if (IS_VALLEYVIEW(dev)) {
>>  		dev_priv->display.modeset_global_resources =
>>  			valleyview_modeset_global_resources;
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 7/8] drm/i915: BDW clock change support

[Jani Nikula]

On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Add support for changing cdclk frequency during runtime on BDW. The
>>> procedure is quite a bit different on BDW from the one on HSW, so
>>> add a separate function for it.
>>>
>>> Also with IPS enabled the actual pixel rate mustn't exceed 95% of cdclk,
>>> so take that into account when computing the max pixel rate.
>>>
>>> v2: Grab rps.hw_lock around sandybridge_pcode_write()
>>> v3: Rebase due to power well vs. .global_resources() reordering
>>> v4: Rebased to the latest
>>> v5: Rebased to the latest
>>> v6: Patch order shuffle so that Broadwell CD clock change is
>>>     applied before the patch for Haswell CD clock change
>>> v7: Fix for patch style problems
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>>
>> This patch hard hangs my BDW NUC at boot when both DP and HDMI are
>> connected. Either DP or HDMI alone are good, same with hotplugging the
>> other afterwards. Booting to grub with both connected, and unplugging
>> HDMI before loading the kernel also reproduces the issue.
>>
>> It looks like the problem boils down to the BIOS setting up a smaller
>> resolution on the DP display when both are connected, and this patch
>> fails to cope with that on i915 load.
>
> By "this patch" I obviously refer to
>
> commit b432e5cfd5e92127ad2dd83bfc3083f1dbce43fb
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Wed Jun 3 15:45:13 2015 +0300
>
>     drm/i915: BDW clock change support
>
> and everything works for the commit before that.

Anyone working on this, or should we revert?

BR,
Jani.


>
> BR,
> Jani.
>
>
>
>
>
>>
>> BR,
>> Jani.
>>
>>
>>
>>>
>>> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_reg.h      |   2 +
>>>  drivers/gpu/drm/i915/intel_display.c | 216 +++++++++++++++++++++++++++++++++--
>>>  2 files changed, 208 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 7213224..0f72c0e 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -6705,6 +6705,7 @@ enum skl_disp_power_wells {
>>>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
>>>  #define     GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
>>>  #define     GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
>>> +#define   BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ	0x18
>>>  #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
>>>  #define     GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
>>>  #define     GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
>>> @@ -7170,6 +7171,7 @@ enum skl_disp_power_wells {
>>>  #define  LCPLL_CLK_FREQ_337_5_BDW	(2<<26)
>>>  #define  LCPLL_CLK_FREQ_675_BDW		(3<<26)
>>>  #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
>>> +#define  LCPLL_ROOT_CD_CLOCK_DISABLE	(1<<24)
>>>  #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
>>>  #define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
>>>  #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index c3f01aa..b1e2069 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -5751,7 +5751,22 @@ static void intel_update_max_cdclk(struct drm_device *dev)
>>>  {
>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>>  
>>> -	if (IS_VALLEYVIEW(dev)) {
>>> +	if (IS_BROADWELL(dev))  {
>>> +		/*
>>> +		 * FIXME with extra cooling we can allow
>>> +		 * 540 MHz for ULX and 675 Mhz for ULT.
>>> +		 * How can we know if extra cooling is
>>> +		 * available? PCI ID, VTB, something else?
>>> +		 */
>>> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
>>> +			dev_priv->max_cdclk_freq = 450000;
>>> +		else if (IS_BDW_ULX(dev))
>>> +			dev_priv->max_cdclk_freq = 450000;
>>> +		else if (IS_BDW_ULT(dev))
>>> +			dev_priv->max_cdclk_freq = 540000;
>>> +		else
>>> +			dev_priv->max_cdclk_freq = 675000;
>>> +	} else if (IS_VALLEYVIEW(dev)) {
>>>  		dev_priv->max_cdclk_freq = 400000;
>>>  	} else {
>>>  		/* otherwise assume cdclk is fixed */
>>> @@ -6621,13 +6636,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>>>  		return true;
>>>  
>>>  	/*
>>> -	 * FIXME if we compare against max we should then
>>> -	 * increase the cdclk frequency when the current
>>> -	 * value is too low. The other option is to compare
>>> -	 * against the cdclk frequency we're going have post
>>> -	 * modeset (ie. one we computed using other constraints).
>>> -	 * Need to measure whether using a lower cdclk w/o IPS
>>> -	 * is better or worse than a higher cdclk w/ IPS.
>>> +	 * We compare against max which means we must take
>>> +	 * the increased cdclk requirement into account when
>>> +	 * calculating the new cdclk.
>>> +	 *
>>> +	 * Should measure whether using a lower cdclk w/o IPS
>>>  	 */
>>>  	return ilk_pipe_pixel_rate(pipe_config) <=
>>>  		dev_priv->max_cdclk_freq * 95 / 100;
>>> @@ -9608,6 +9621,182 @@ static void broxton_modeset_global_resources(struct drm_atomic_state *old_state)
>>>  		broxton_set_cdclk(dev, req_cdclk);
>>>  }
>>>  
>>> +/* compute the max rate for new configuration */
>>> +static int ilk_max_pixel_rate(struct drm_i915_private *dev_priv)
>>> +{
>>> +	struct drm_device *dev = dev_priv->dev;
>>> +	struct intel_crtc *intel_crtc;
>>> +	struct drm_crtc *crtc;
>>> +	int max_pixel_rate = 0;
>>> +	int pixel_rate;
>>> +
>>> +	for_each_crtc(dev, crtc) {
>>> +		if (!crtc->state->enable)
>>> +			continue;
>>> +
>>> +		intel_crtc = to_intel_crtc(crtc);
>>> +		pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
>>> +
>>> +		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>>> +		if (IS_BROADWELL(dev) && intel_crtc->config->ips_enabled)
>>> +			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
>>> +
>>> +		max_pixel_rate = max(max_pixel_rate, pixel_rate);
>>> +	}
>>> +
>>> +	return max_pixel_rate;
>>> +}
>>> +
>>> +static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
>>> +{
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	uint32_t val, data;
>>> +	int ret;
>>> +
>>> +	if (WARN((I915_READ(LCPLL_CTL) &
>>> +		  (LCPLL_PLL_DISABLE | LCPLL_PLL_LOCK |
>>> +		   LCPLL_CD_CLOCK_DISABLE | LCPLL_ROOT_CD_CLOCK_DISABLE |
>>> +		   LCPLL_CD2X_CLOCK_DISABLE | LCPLL_POWER_DOWN_ALLOW |
>>> +		   LCPLL_CD_SOURCE_FCLK)) != LCPLL_PLL_LOCK,
>>> +		 "trying to change cdclk frequency with cdclk not enabled\n"))
>>> +		return;
>>> +
>>> +	mutex_lock(&dev_priv->rps.hw_lock);
>>> +	ret = sandybridge_pcode_write(dev_priv,
>>> +				      BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ, 0x0);
>>> +	mutex_unlock(&dev_priv->rps.hw_lock);
>>> +	if (ret) {
>>> +		DRM_ERROR("failed to inform pcode about cdclk change\n");
>>> +		return;
>>> +	}
>>> +
>>> +	val = I915_READ(LCPLL_CTL);
>>> +	val |= LCPLL_CD_SOURCE_FCLK;
>>> +	I915_WRITE(LCPLL_CTL, val);
>>> +
>>> +	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
>>> +			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
>>> +		DRM_ERROR("Switching to FCLK failed\n");
>>> +
>>> +	val = I915_READ(LCPLL_CTL);
>>> +	val &= ~LCPLL_CLK_FREQ_MASK;
>>> +
>>> +	switch (cdclk) {
>>> +	case 450000:
>>> +		val |= LCPLL_CLK_FREQ_450;
>>> +		data = 0;
>>> +		break;
>>> +	case 540000:
>>> +		val |= LCPLL_CLK_FREQ_54O_BDW;
>>> +		data = 1;
>>> +		break;
>>> +	case 337500:
>>> +		val |= LCPLL_CLK_FREQ_337_5_BDW;
>>> +		data = 2;
>>> +		break;
>>> +	case 675000:
>>> +		val |= LCPLL_CLK_FREQ_675_BDW;
>>> +		data = 3;
>>> +		break;
>>> +	default:
>>> +		WARN(1, "invalid cdclk frequency\n");
>>> +		return;
>>> +	}
>>> +
>>> +	I915_WRITE(LCPLL_CTL, val);
>>> +
>>> +	val = I915_READ(LCPLL_CTL);
>>> +	val &= ~LCPLL_CD_SOURCE_FCLK;
>>> +	I915_WRITE(LCPLL_CTL, val);
>>> +
>>> +	if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
>>> +				LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
>>> +		DRM_ERROR("Switching back to LCPLL failed\n");
>>> +
>>> +	mutex_lock(&dev_priv->rps.hw_lock);
>>> +	sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, data);
>>> +	mutex_unlock(&dev_priv->rps.hw_lock);
>>> +
>>> +	intel_update_cdclk(dev);
>>> +
>>> +	WARN(cdclk != dev_priv->cdclk_freq,
>>> +	     "cdclk requested %d kHz but got %d kHz\n",
>>> +	     cdclk, dev_priv->cdclk_freq);
>>> +}
>>> +
>>> +static int broadwell_calc_cdclk(struct drm_i915_private *dev_priv,
>>> +			      int max_pixel_rate)
>>> +{
>>> +	int cdclk;
>>> +
>>> +	/*
>>> +	 * FIXME should also account for plane ratio
>>> +	 * once 64bpp pixel formats are supported.
>>> +	 */
>>> +	if (max_pixel_rate > 540000)
>>> +		cdclk = 675000;
>>> +	else if (max_pixel_rate > 450000)
>>> +		cdclk = 540000;
>>> +	else if (max_pixel_rate > 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;
>>> +	}
>>> +
>>> +	return cdclk;
>>> +}
>>> +
>>> +static int broadwell_modeset_global_pipes(struct drm_atomic_state *state)
>>> +{
>>> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
>>> +	struct drm_crtc *crtc;
>>> +	struct drm_crtc_state *crtc_state;
>>> +	int max_pixclk = ilk_max_pixel_rate(dev_priv);
>>> +	int cdclk, i;
>>> +
>>> +	cdclk = broadwell_calc_cdclk(dev_priv, max_pixclk);
>>> +
>>> +	if (cdclk == dev_priv->cdclk_freq)
>>> +		return 0;
>>> +
>>> +	/* add all active pipes to the state */
>>> +	for_each_crtc(state->dev, crtc) {
>>> +		if (!crtc->state->enable)
>>> +			continue;
>>> +
>>> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>> +		if (IS_ERR(crtc_state))
>>> +			return PTR_ERR(crtc_state);
>>> +	}
>>> +
>>> +	/* disable/enable all currently active pipes while we change cdclk */
>>> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
>>> +		if (crtc_state->enable)
>>> +			crtc_state->mode_changed = true;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void broadwell_modeset_global_resources(struct drm_atomic_state *state)
>>> +{
>>> +	struct drm_device *dev = state->dev;
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
>>> +	int req_cdclk = broadwell_calc_cdclk(dev_priv, max_pixel_rate);
>>> +
>>> +	if (req_cdclk != dev_priv->cdclk_freq)
>>> +		broadwell_set_cdclk(dev, req_cdclk);
>>> +}
>>> +
>>>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>>>  				      struct intel_crtc_state *crtc_state)
>>>  {
>>> @@ -12788,8 +12977,12 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>>>  	 * mode set on this crtc.  For other crtcs we need to use the
>>>  	 * adjusted_mode bits in the crtc directly.
>>>  	 */
>>> -	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev)) {
>>> -		ret = valleyview_modeset_global_pipes(state);
>>> +	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev)) {
>>> +		if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev))
>>> +			ret = valleyview_modeset_global_pipes(state);
>>> +		else
>>> +			ret = broadwell_modeset_global_pipes(state);
>>> +
>>>  		if (ret)
>>>  			return ret;
>>>  	}
>>> @@ -14677,6 +14870,9 @@ static void intel_init_display(struct drm_device *dev)
>>>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
>>>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>>>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>>> +		if (IS_BROADWELL(dev))
>>> +			dev_priv->display.modeset_global_resources =
>>> +				broadwell_modeset_global_resources;
>>>  	} else if (IS_VALLEYVIEW(dev)) {
>>>  		dev_priv->display.modeset_global_resources =
>>>  			valleyview_modeset_global_resources;
>>> -- 
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 7/8] drm/i915: BDW clock change support

[Mika Kahola]

On Mon, 2015-06-29 at 14:24 +0300, Jani Nikula wrote:
> On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >> On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Add support for changing cdclk frequency during runtime on BDW. The
> >>> procedure is quite a bit different on BDW from the one on HSW, so
> >>> add a separate function for it.
> >>>
> >>> Also with IPS enabled the actual pixel rate mustn't exceed 95% of cdclk,
> >>> so take that into account when computing the max pixel rate.
> >>>
> >>> v2: Grab rps.hw_lock around sandybridge_pcode_write()
> >>> v3: Rebase due to power well vs. .global_resources() reordering
> >>> v4: Rebased to the latest
> >>> v5: Rebased to the latest
> >>> v6: Patch order shuffle so that Broadwell CD clock change is
> >>>     applied before the patch for Haswell CD clock change
> >>> v7: Fix for patch style problems
> >>>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >>
> >> This patch hard hangs my BDW NUC at boot when both DP and HDMI are
> >> connected. Either DP or HDMI alone are good, same with hotplugging the
> >> other afterwards. Booting to grub with both connected, and unplugging
> >> HDMI before loading the kernel also reproduces the issue.
> >>
> >> It looks like the problem boils down to the BIOS setting up a smaller
> >> resolution on the DP display when both are connected, and this patch
> >> fails to cope with that on i915 load.
> >
> > By "this patch" I obviously refer to
> >
> > commit b432e5cfd5e92127ad2dd83bfc3083f1dbce43fb
> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Date:   Wed Jun 3 15:45:13 2015 +0300
> >
> >     drm/i915: BDW clock change support
> >
> > and everything works for the commit before that.
> 
> Anyone working on this, or should we revert?
> 
> BR,
> Jani.
> 
My understanding is that Ville has an idea or how to fix this.

Cheers,
Mika

> 
> >
> > BR,
> > Jani.
> >
> >
> >
> >
> >
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >>
> >>>
> >>> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/i915_reg.h      |   2 +
> >>>  drivers/gpu/drm/i915/intel_display.c | 216 +++++++++++++++++++++++++++++++++--
> >>>  2 files changed, 208 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>> index 7213224..0f72c0e 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -6705,6 +6705,7 @@ enum skl_disp_power_wells {
> >>>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
> >>>  #define     GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
> >>>  #define     GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> >>> +#define   BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ	0x18
> >>>  #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
> >>>  #define     GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
> >>>  #define     GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
> >>> @@ -7170,6 +7171,7 @@ enum skl_disp_power_wells {
> >>>  #define  LCPLL_CLK_FREQ_337_5_BDW	(2<<26)
> >>>  #define  LCPLL_CLK_FREQ_675_BDW		(3<<26)
> >>>  #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
> >>> +#define  LCPLL_ROOT_CD_CLOCK_DISABLE	(1<<24)
> >>>  #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
> >>>  #define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
> >>>  #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index c3f01aa..b1e2069 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -5751,7 +5751,22 @@ static void intel_update_max_cdclk(struct drm_device *dev)
> >>>  {
> >>>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>  
> >>> -	if (IS_VALLEYVIEW(dev)) {
> >>> +	if (IS_BROADWELL(dev))  {
> >>> +		/*
> >>> +		 * FIXME with extra cooling we can allow
> >>> +		 * 540 MHz for ULX and 675 Mhz for ULT.
> >>> +		 * How can we know if extra cooling is
> >>> +		 * available? PCI ID, VTB, something else?
> >>> +		 */
> >>> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> >>> +			dev_priv->max_cdclk_freq = 450000;
> >>> +		else if (IS_BDW_ULX(dev))
> >>> +			dev_priv->max_cdclk_freq = 450000;
> >>> +		else if (IS_BDW_ULT(dev))
> >>> +			dev_priv->max_cdclk_freq = 540000;
> >>> +		else
> >>> +			dev_priv->max_cdclk_freq = 675000;
> >>> +	} else if (IS_VALLEYVIEW(dev)) {
> >>>  		dev_priv->max_cdclk_freq = 400000;
> >>>  	} else {
> >>>  		/* otherwise assume cdclk is fixed */
> >>> @@ -6621,13 +6636,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> >>>  		return true;
> >>>  
> >>>  	/*
> >>> -	 * FIXME if we compare against max we should then
> >>> -	 * increase the cdclk frequency when the current
> >>> -	 * value is too low. The other option is to compare
> >>> -	 * against the cdclk frequency we're going have post
> >>> -	 * modeset (ie. one we computed using other constraints).
> >>> -	 * Need to measure whether using a lower cdclk w/o IPS
> >>> -	 * is better or worse than a higher cdclk w/ IPS.
> >>> +	 * We compare against max which means we must take
> >>> +	 * the increased cdclk requirement into account when
> >>> +	 * calculating the new cdclk.
> >>> +	 *
> >>> +	 * Should measure whether using a lower cdclk w/o IPS
> >>>  	 */
> >>>  	return ilk_pipe_pixel_rate(pipe_config) <=
> >>>  		dev_priv->max_cdclk_freq * 95 / 100;
> >>> @@ -9608,6 +9621,182 @@ static void broxton_modeset_global_resources(struct drm_atomic_state *old_state)
> >>>  		broxton_set_cdclk(dev, req_cdclk);
> >>>  }
> >>>  
> >>> +/* compute the max rate for new configuration */
> >>> +static int ilk_max_pixel_rate(struct drm_i915_private *dev_priv)
> >>> +{
> >>> +	struct drm_device *dev = dev_priv->dev;
> >>> +	struct intel_crtc *intel_crtc;
> >>> +	struct drm_crtc *crtc;
> >>> +	int max_pixel_rate = 0;
> >>> +	int pixel_rate;
> >>> +
> >>> +	for_each_crtc(dev, crtc) {
> >>> +		if (!crtc->state->enable)
> >>> +			continue;
> >>> +
> >>> +		intel_crtc = to_intel_crtc(crtc);
> >>> +		pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
> >>> +
> >>> +		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> >>> +		if (IS_BROADWELL(dev) && intel_crtc->config->ips_enabled)
> >>> +			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> >>> +
> >>> +		max_pixel_rate = max(max_pixel_rate, pixel_rate);
> >>> +	}
> >>> +
> >>> +	return max_pixel_rate;
> >>> +}
> >>> +
> >>> +static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
> >>> +{
> >>> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >>> +	uint32_t val, data;
> >>> +	int ret;
> >>> +
> >>> +	if (WARN((I915_READ(LCPLL_CTL) &
> >>> +		  (LCPLL_PLL_DISABLE | LCPLL_PLL_LOCK |
> >>> +		   LCPLL_CD_CLOCK_DISABLE | LCPLL_ROOT_CD_CLOCK_DISABLE |
> >>> +		   LCPLL_CD2X_CLOCK_DISABLE | LCPLL_POWER_DOWN_ALLOW |
> >>> +		   LCPLL_CD_SOURCE_FCLK)) != LCPLL_PLL_LOCK,
> >>> +		 "trying to change cdclk frequency with cdclk not enabled\n"))
> >>> +		return;
> >>> +
> >>> +	mutex_lock(&dev_priv->rps.hw_lock);
> >>> +	ret = sandybridge_pcode_write(dev_priv,
> >>> +				      BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ, 0x0);
> >>> +	mutex_unlock(&dev_priv->rps.hw_lock);
> >>> +	if (ret) {
> >>> +		DRM_ERROR("failed to inform pcode about cdclk change\n");
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	val = I915_READ(LCPLL_CTL);
> >>> +	val |= LCPLL_CD_SOURCE_FCLK;
> >>> +	I915_WRITE(LCPLL_CTL, val);
> >>> +
> >>> +	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
> >>> +			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
> >>> +		DRM_ERROR("Switching to FCLK failed\n");
> >>> +
> >>> +	val = I915_READ(LCPLL_CTL);
> >>> +	val &= ~LCPLL_CLK_FREQ_MASK;
> >>> +
> >>> +	switch (cdclk) {
> >>> +	case 450000:
> >>> +		val |= LCPLL_CLK_FREQ_450;
> >>> +		data = 0;
> >>> +		break;
> >>> +	case 540000:
> >>> +		val |= LCPLL_CLK_FREQ_54O_BDW;
> >>> +		data = 1;
> >>> +		break;
> >>> +	case 337500:
> >>> +		val |= LCPLL_CLK_FREQ_337_5_BDW;
> >>> +		data = 2;
> >>> +		break;
> >>> +	case 675000:
> >>> +		val |= LCPLL_CLK_FREQ_675_BDW;
> >>> +		data = 3;
> >>> +		break;
> >>> +	default:
> >>> +		WARN(1, "invalid cdclk frequency\n");
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	I915_WRITE(LCPLL_CTL, val);
> >>> +
> >>> +	val = I915_READ(LCPLL_CTL);
> >>> +	val &= ~LCPLL_CD_SOURCE_FCLK;
> >>> +	I915_WRITE(LCPLL_CTL, val);
> >>> +
> >>> +	if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
> >>> +				LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
> >>> +		DRM_ERROR("Switching back to LCPLL failed\n");
> >>> +
> >>> +	mutex_lock(&dev_priv->rps.hw_lock);
> >>> +	sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, data);
> >>> +	mutex_unlock(&dev_priv->rps.hw_lock);
> >>> +
> >>> +	intel_update_cdclk(dev);
> >>> +
> >>> +	WARN(cdclk != dev_priv->cdclk_freq,
> >>> +	     "cdclk requested %d kHz but got %d kHz\n",
> >>> +	     cdclk, dev_priv->cdclk_freq);
> >>> +}
> >>> +
> >>> +static int broadwell_calc_cdclk(struct drm_i915_private *dev_priv,
> >>> +			      int max_pixel_rate)
> >>> +{
> >>> +	int cdclk;
> >>> +
> >>> +	/*
> >>> +	 * FIXME should also account for plane ratio
> >>> +	 * once 64bpp pixel formats are supported.
> >>> +	 */
> >>> +	if (max_pixel_rate > 540000)
> >>> +		cdclk = 675000;
> >>> +	else if (max_pixel_rate > 450000)
> >>> +		cdclk = 540000;
> >>> +	else if (max_pixel_rate > 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;
> >>> +	}
> >>> +
> >>> +	return cdclk;
> >>> +}
> >>> +
> >>> +static int broadwell_modeset_global_pipes(struct drm_atomic_state *state)
> >>> +{
> >>> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >>> +	struct drm_crtc *crtc;
> >>> +	struct drm_crtc_state *crtc_state;
> >>> +	int max_pixclk = ilk_max_pixel_rate(dev_priv);
> >>> +	int cdclk, i;
> >>> +
> >>> +	cdclk = broadwell_calc_cdclk(dev_priv, max_pixclk);
> >>> +
> >>> +	if (cdclk == dev_priv->cdclk_freq)
> >>> +		return 0;
> >>> +
> >>> +	/* add all active pipes to the state */
> >>> +	for_each_crtc(state->dev, crtc) {
> >>> +		if (!crtc->state->enable)
> >>> +			continue;
> >>> +
> >>> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >>> +		if (IS_ERR(crtc_state))
> >>> +			return PTR_ERR(crtc_state);
> >>> +	}
> >>> +
> >>> +	/* disable/enable all currently active pipes while we change cdclk */
> >>> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
> >>> +		if (crtc_state->enable)
> >>> +			crtc_state->mode_changed = true;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static void broadwell_modeset_global_resources(struct drm_atomic_state *state)
> >>> +{
> >>> +	struct drm_device *dev = state->dev;
> >>> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >>> +	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
> >>> +	int req_cdclk = broadwell_calc_cdclk(dev_priv, max_pixel_rate);
> >>> +
> >>> +	if (req_cdclk != dev_priv->cdclk_freq)
> >>> +		broadwell_set_cdclk(dev, req_cdclk);
> >>> +}
> >>> +
> >>>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> >>>  				      struct intel_crtc_state *crtc_state)
> >>>  {
> >>> @@ -12788,8 +12977,12 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
> >>>  	 * mode set on this crtc.  For other crtcs we need to use the
> >>>  	 * adjusted_mode bits in the crtc directly.
> >>>  	 */
> >>> -	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev)) {
> >>> -		ret = valleyview_modeset_global_pipes(state);
> >>> +	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev)) {
> >>> +		if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev))
> >>> +			ret = valleyview_modeset_global_pipes(state);
> >>> +		else
> >>> +			ret = broadwell_modeset_global_pipes(state);
> >>> +
> >>>  		if (ret)
> >>>  			return ret;
> >>>  	}
> >>> @@ -14677,6 +14870,9 @@ static void intel_init_display(struct drm_device *dev)
> >>>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> >>>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> >>>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
> >>> +		if (IS_BROADWELL(dev))
> >>> +			dev_priv->display.modeset_global_resources =
> >>> +				broadwell_modeset_global_resources;
> >>>  	} else if (IS_VALLEYVIEW(dev)) {
> >>>  		dev_priv->display.modeset_global_resources =
> >>>  			valleyview_modeset_global_resources;
> >>> -- 
> >>> 1.9.1
> >>>
> >>> _______________________________________________
> >>> Intel-gfx mailing list
> >>> Intel-gfx@lists.freedesktop.org
> >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> >> -- 
> >> Jani Nikula, Intel Open Source Technology Center
> >
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 7/8] drm/i915: BDW clock change support

[Jani Nikula]

On Mon, 29 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> On Mon, 2015-06-29 at 14:24 +0300, Jani Nikula wrote:
>> On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> > On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> >> On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
>> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >>>
>> >>> Add support for changing cdclk frequency during runtime on BDW. The
>> >>> procedure is quite a bit different on BDW from the one on HSW, so
>> >>> add a separate function for it.
>> >>>
>> >>> Also with IPS enabled the actual pixel rate mustn't exceed 95% of cdclk,
>> >>> so take that into account when computing the max pixel rate.
>> >>>
>> >>> v2: Grab rps.hw_lock around sandybridge_pcode_write()
>> >>> v3: Rebase due to power well vs. .global_resources() reordering
>> >>> v4: Rebased to the latest
>> >>> v5: Rebased to the latest
>> >>> v6: Patch order shuffle so that Broadwell CD clock change is
>> >>>     applied before the patch for Haswell CD clock change
>> >>> v7: Fix for patch style problems
>> >>>
>> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> >>
>> >> This patch hard hangs my BDW NUC at boot when both DP and HDMI are
>> >> connected. Either DP or HDMI alone are good, same with hotplugging the
>> >> other afterwards. Booting to grub with both connected, and unplugging
>> >> HDMI before loading the kernel also reproduces the issue.
>> >>
>> >> It looks like the problem boils down to the BIOS setting up a smaller
>> >> resolution on the DP display when both are connected, and this patch
>> >> fails to cope with that on i915 load.
>> >
>> > By "this patch" I obviously refer to
>> >
>> > commit b432e5cfd5e92127ad2dd83bfc3083f1dbce43fb
>> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Date:   Wed Jun 3 15:45:13 2015 +0300
>> >
>> >     drm/i915: BDW clock change support
>> >
>> > and everything works for the commit before that.
>> 
>> Anyone working on this, or should we revert?
>> 
>> BR,
>> Jani.
>> 
> My understanding is that Ville has an idea or how to fix this.

Ville, when do you think we can convert the idea into a patch? Should we
revert in the mean time?

BR,
Jani.



>
> Cheers,
> Mika
>
>> 
>> >
>> > BR,
>> > Jani.
>> >
>> >
>> >
>> >
>> >
>> >>
>> >> BR,
>> >> Jani.
>> >>
>> >>
>> >>
>> >>>
>> >>> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >>> ---
>> >>>  drivers/gpu/drm/i915/i915_reg.h      |   2 +
>> >>>  drivers/gpu/drm/i915/intel_display.c | 216 +++++++++++++++++++++++++++++++++--
>> >>>  2 files changed, 208 insertions(+), 10 deletions(-)
>> >>>
>> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> >>> index 7213224..0f72c0e 100644
>> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >>> @@ -6705,6 +6705,7 @@ enum skl_disp_power_wells {
>> >>>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
>> >>>  #define     GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
>> >>>  #define     GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
>> >>> +#define   BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ	0x18
>> >>>  #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
>> >>>  #define     GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
>> >>>  #define     GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
>> >>> @@ -7170,6 +7171,7 @@ enum skl_disp_power_wells {
>> >>>  #define  LCPLL_CLK_FREQ_337_5_BDW	(2<<26)
>> >>>  #define  LCPLL_CLK_FREQ_675_BDW		(3<<26)
>> >>>  #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
>> >>> +#define  LCPLL_ROOT_CD_CLOCK_DISABLE	(1<<24)
>> >>>  #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
>> >>>  #define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
>> >>>  #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> >>> index c3f01aa..b1e2069 100644
>> >>> --- a/drivers/gpu/drm/i915/intel_display.c
>> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> >>> @@ -5751,7 +5751,22 @@ static void intel_update_max_cdclk(struct drm_device *dev)
>> >>>  {
>> >>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> >>>  
>> >>> -	if (IS_VALLEYVIEW(dev)) {
>> >>> +	if (IS_BROADWELL(dev))  {
>> >>> +		/*
>> >>> +		 * FIXME with extra cooling we can allow
>> >>> +		 * 540 MHz for ULX and 675 Mhz for ULT.
>> >>> +		 * How can we know if extra cooling is
>> >>> +		 * available? PCI ID, VTB, something else?
>> >>> +		 */
>> >>> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
>> >>> +			dev_priv->max_cdclk_freq = 450000;
>> >>> +		else if (IS_BDW_ULX(dev))
>> >>> +			dev_priv->max_cdclk_freq = 450000;
>> >>> +		else if (IS_BDW_ULT(dev))
>> >>> +			dev_priv->max_cdclk_freq = 540000;
>> >>> +		else
>> >>> +			dev_priv->max_cdclk_freq = 675000;
>> >>> +	} else if (IS_VALLEYVIEW(dev)) {
>> >>>  		dev_priv->max_cdclk_freq = 400000;
>> >>>  	} else {
>> >>>  		/* otherwise assume cdclk is fixed */
>> >>> @@ -6621,13 +6636,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>> >>>  		return true;
>> >>>  
>> >>>  	/*
>> >>> -	 * FIXME if we compare against max we should then
>> >>> -	 * increase the cdclk frequency when the current
>> >>> -	 * value is too low. The other option is to compare
>> >>> -	 * against the cdclk frequency we're going have post
>> >>> -	 * modeset (ie. one we computed using other constraints).
>> >>> -	 * Need to measure whether using a lower cdclk w/o IPS
>> >>> -	 * is better or worse than a higher cdclk w/ IPS.
>> >>> +	 * We compare against max which means we must take
>> >>> +	 * the increased cdclk requirement into account when
>> >>> +	 * calculating the new cdclk.
>> >>> +	 *
>> >>> +	 * Should measure whether using a lower cdclk w/o IPS
>> >>>  	 */
>> >>>  	return ilk_pipe_pixel_rate(pipe_config) <=
>> >>>  		dev_priv->max_cdclk_freq * 95 / 100;
>> >>> @@ -9608,6 +9621,182 @@ static void broxton_modeset_global_resources(struct drm_atomic_state *old_state)
>> >>>  		broxton_set_cdclk(dev, req_cdclk);
>> >>>  }
>> >>>  
>> >>> +/* compute the max rate for new configuration */
>> >>> +static int ilk_max_pixel_rate(struct drm_i915_private *dev_priv)
>> >>> +{
>> >>> +	struct drm_device *dev = dev_priv->dev;
>> >>> +	struct intel_crtc *intel_crtc;
>> >>> +	struct drm_crtc *crtc;
>> >>> +	int max_pixel_rate = 0;
>> >>> +	int pixel_rate;
>> >>> +
>> >>> +	for_each_crtc(dev, crtc) {
>> >>> +		if (!crtc->state->enable)
>> >>> +			continue;
>> >>> +
>> >>> +		intel_crtc = to_intel_crtc(crtc);
>> >>> +		pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
>> >>> +
>> >>> +		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>> >>> +		if (IS_BROADWELL(dev) && intel_crtc->config->ips_enabled)
>> >>> +			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
>> >>> +
>> >>> +		max_pixel_rate = max(max_pixel_rate, pixel_rate);
>> >>> +	}
>> >>> +
>> >>> +	return max_pixel_rate;
>> >>> +}
>> >>> +
>> >>> +static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
>> >>> +{
>> >>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> >>> +	uint32_t val, data;
>> >>> +	int ret;
>> >>> +
>> >>> +	if (WARN((I915_READ(LCPLL_CTL) &
>> >>> +		  (LCPLL_PLL_DISABLE | LCPLL_PLL_LOCK |
>> >>> +		   LCPLL_CD_CLOCK_DISABLE | LCPLL_ROOT_CD_CLOCK_DISABLE |
>> >>> +		   LCPLL_CD2X_CLOCK_DISABLE | LCPLL_POWER_DOWN_ALLOW |
>> >>> +		   LCPLL_CD_SOURCE_FCLK)) != LCPLL_PLL_LOCK,
>> >>> +		 "trying to change cdclk frequency with cdclk not enabled\n"))
>> >>> +		return;
>> >>> +
>> >>> +	mutex_lock(&dev_priv->rps.hw_lock);
>> >>> +	ret = sandybridge_pcode_write(dev_priv,
>> >>> +				      BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ, 0x0);
>> >>> +	mutex_unlock(&dev_priv->rps.hw_lock);
>> >>> +	if (ret) {
>> >>> +		DRM_ERROR("failed to inform pcode about cdclk change\n");
>> >>> +		return;
>> >>> +	}
>> >>> +
>> >>> +	val = I915_READ(LCPLL_CTL);
>> >>> +	val |= LCPLL_CD_SOURCE_FCLK;
>> >>> +	I915_WRITE(LCPLL_CTL, val);
>> >>> +
>> >>> +	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
>> >>> +			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
>> >>> +		DRM_ERROR("Switching to FCLK failed\n");
>> >>> +
>> >>> +	val = I915_READ(LCPLL_CTL);
>> >>> +	val &= ~LCPLL_CLK_FREQ_MASK;
>> >>> +
>> >>> +	switch (cdclk) {
>> >>> +	case 450000:
>> >>> +		val |= LCPLL_CLK_FREQ_450;
>> >>> +		data = 0;
>> >>> +		break;
>> >>> +	case 540000:
>> >>> +		val |= LCPLL_CLK_FREQ_54O_BDW;
>> >>> +		data = 1;
>> >>> +		break;
>> >>> +	case 337500:
>> >>> +		val |= LCPLL_CLK_FREQ_337_5_BDW;
>> >>> +		data = 2;
>> >>> +		break;
>> >>> +	case 675000:
>> >>> +		val |= LCPLL_CLK_FREQ_675_BDW;
>> >>> +		data = 3;
>> >>> +		break;
>> >>> +	default:
>> >>> +		WARN(1, "invalid cdclk frequency\n");
>> >>> +		return;
>> >>> +	}
>> >>> +
>> >>> +	I915_WRITE(LCPLL_CTL, val);
>> >>> +
>> >>> +	val = I915_READ(LCPLL_CTL);
>> >>> +	val &= ~LCPLL_CD_SOURCE_FCLK;
>> >>> +	I915_WRITE(LCPLL_CTL, val);
>> >>> +
>> >>> +	if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
>> >>> +				LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
>> >>> +		DRM_ERROR("Switching back to LCPLL failed\n");
>> >>> +
>> >>> +	mutex_lock(&dev_priv->rps.hw_lock);
>> >>> +	sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, data);
>> >>> +	mutex_unlock(&dev_priv->rps.hw_lock);
>> >>> +
>> >>> +	intel_update_cdclk(dev);
>> >>> +
>> >>> +	WARN(cdclk != dev_priv->cdclk_freq,
>> >>> +	     "cdclk requested %d kHz but got %d kHz\n",
>> >>> +	     cdclk, dev_priv->cdclk_freq);
>> >>> +}
>> >>> +
>> >>> +static int broadwell_calc_cdclk(struct drm_i915_private *dev_priv,
>> >>> +			      int max_pixel_rate)
>> >>> +{
>> >>> +	int cdclk;
>> >>> +
>> >>> +	/*
>> >>> +	 * FIXME should also account for plane ratio
>> >>> +	 * once 64bpp pixel formats are supported.
>> >>> +	 */
>> >>> +	if (max_pixel_rate > 540000)
>> >>> +		cdclk = 675000;
>> >>> +	else if (max_pixel_rate > 450000)
>> >>> +		cdclk = 540000;
>> >>> +	else if (max_pixel_rate > 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;
>> >>> +	}
>> >>> +
>> >>> +	return cdclk;
>> >>> +}
>> >>> +
>> >>> +static int broadwell_modeset_global_pipes(struct drm_atomic_state *state)
>> >>> +{
>> >>> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
>> >>> +	struct drm_crtc *crtc;
>> >>> +	struct drm_crtc_state *crtc_state;
>> >>> +	int max_pixclk = ilk_max_pixel_rate(dev_priv);
>> >>> +	int cdclk, i;
>> >>> +
>> >>> +	cdclk = broadwell_calc_cdclk(dev_priv, max_pixclk);
>> >>> +
>> >>> +	if (cdclk == dev_priv->cdclk_freq)
>> >>> +		return 0;
>> >>> +
>> >>> +	/* add all active pipes to the state */
>> >>> +	for_each_crtc(state->dev, crtc) {
>> >>> +		if (!crtc->state->enable)
>> >>> +			continue;
>> >>> +
>> >>> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>> >>> +		if (IS_ERR(crtc_state))
>> >>> +			return PTR_ERR(crtc_state);
>> >>> +	}
>> >>> +
>> >>> +	/* disable/enable all currently active pipes while we change cdclk */
>> >>> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
>> >>> +		if (crtc_state->enable)
>> >>> +			crtc_state->mode_changed = true;
>> >>> +
>> >>> +	return 0;
>> >>> +}
>> >>> +
>> >>> +static void broadwell_modeset_global_resources(struct drm_atomic_state *state)
>> >>> +{
>> >>> +	struct drm_device *dev = state->dev;
>> >>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> >>> +	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
>> >>> +	int req_cdclk = broadwell_calc_cdclk(dev_priv, max_pixel_rate);
>> >>> +
>> >>> +	if (req_cdclk != dev_priv->cdclk_freq)
>> >>> +		broadwell_set_cdclk(dev, req_cdclk);
>> >>> +}
>> >>> +
>> >>>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>> >>>  				      struct intel_crtc_state *crtc_state)
>> >>>  {
>> >>> @@ -12788,8 +12977,12 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>> >>>  	 * mode set on this crtc.  For other crtcs we need to use the
>> >>>  	 * adjusted_mode bits in the crtc directly.
>> >>>  	 */
>> >>> -	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev)) {
>> >>> -		ret = valleyview_modeset_global_pipes(state);
>> >>> +	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev)) {
>> >>> +		if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev))
>> >>> +			ret = valleyview_modeset_global_pipes(state);
>> >>> +		else
>> >>> +			ret = broadwell_modeset_global_pipes(state);
>> >>> +
>> >>>  		if (ret)
>> >>>  			return ret;
>> >>>  	}
>> >>> @@ -14677,6 +14870,9 @@ static void intel_init_display(struct drm_device *dev)
>> >>>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
>> >>>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>> >>>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
>> >>> +		if (IS_BROADWELL(dev))
>> >>> +			dev_priv->display.modeset_global_resources =
>> >>> +				broadwell_modeset_global_resources;
>> >>>  	} else if (IS_VALLEYVIEW(dev)) {
>> >>>  		dev_priv->display.modeset_global_resources =
>> >>>  			valleyview_modeset_global_resources;
>> >>> -- 
>> >>> 1.9.1
>> >>>
>> >>> _______________________________________________
>> >>> Intel-gfx mailing list
>> >>> Intel-gfx@lists.freedesktop.org
>> >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >>
>> >> -- 
>> >> Jani Nikula, Intel Open Source Technology Center
>> >
>> > -- 
>> > Jani Nikula, Intel Open Source Technology Center
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 7/8] drm/i915: BDW clock change support

[Ville Syrjälä]

On Mon, Jun 29, 2015 at 02:42:25PM +0300, Jani Nikula wrote:
> On Mon, 29 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> > On Mon, 2015-06-29 at 14:24 +0300, Jani Nikula wrote:
> >> On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >> > On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >> >> On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> >> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >>>
> >> >>> Add support for changing cdclk frequency during runtime on BDW. The
> >> >>> procedure is quite a bit different on BDW from the one on HSW, so
> >> >>> add a separate function for it.
> >> >>>
> >> >>> Also with IPS enabled the actual pixel rate mustn't exceed 95% of cdclk,
> >> >>> so take that into account when computing the max pixel rate.
> >> >>>
> >> >>> v2: Grab rps.hw_lock around sandybridge_pcode_write()
> >> >>> v3: Rebase due to power well vs. .global_resources() reordering
> >> >>> v4: Rebased to the latest
> >> >>> v5: Rebased to the latest
> >> >>> v6: Patch order shuffle so that Broadwell CD clock change is
> >> >>>     applied before the patch for Haswell CD clock change
> >> >>> v7: Fix for patch style problems
> >> >>>
> >> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >> >>
> >> >> This patch hard hangs my BDW NUC at boot when both DP and HDMI are
> >> >> connected. Either DP or HDMI alone are good, same with hotplugging the
> >> >> other afterwards. Booting to grub with both connected, and unplugging
> >> >> HDMI before loading the kernel also reproduces the issue.
> >> >>
> >> >> It looks like the problem boils down to the BIOS setting up a smaller
> >> >> resolution on the DP display when both are connected, and this patch
> >> >> fails to cope with that on i915 load.
> >> >
> >> > By "this patch" I obviously refer to
> >> >
> >> > commit b432e5cfd5e92127ad2dd83bfc3083f1dbce43fb
> >> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > Date:   Wed Jun 3 15:45:13 2015 +0300
> >> >
> >> >     drm/i915: BDW clock change support
> >> >
> >> > and everything works for the commit before that.
> >> 
> >> Anyone working on this, or should we revert?
> >> 
> >> BR,
> >> Jani.
> >> 
> > My understanding is that Ville has an idea or how to fix this.
> 
> Ville, when do you think we can convert the idea into a patch? Should we
> revert in the mean time?

I was thinking it may have been atomic making a mess of things and
enabling planes while the pipe was off. But I may be totally off here
since the current atomic code doesn't agree with my brain at all.

So no, I have no patch to fix it. Someone needs to first check where
exactly the hang happens.

> 
> BR,
> Jani.
> 
> 
> 
> >
> > Cheers,
> > Mika
> >
> >> 
> >> >
> >> > BR,
> >> > Jani.
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >>
> >> >> BR,
> >> >> Jani.
> >> >>
> >> >>
> >> >>
> >> >>>
> >> >>> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >>> ---
> >> >>>  drivers/gpu/drm/i915/i915_reg.h      |   2 +
> >> >>>  drivers/gpu/drm/i915/intel_display.c | 216 +++++++++++++++++++++++++++++++++--
> >> >>>  2 files changed, 208 insertions(+), 10 deletions(-)
> >> >>>
> >> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> >>> index 7213224..0f72c0e 100644
> >> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> >>> @@ -6705,6 +6705,7 @@ enum skl_disp_power_wells {
> >> >>>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
> >> >>>  #define     GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
> >> >>>  #define     GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> >> >>> +#define   BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ	0x18
> >> >>>  #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
> >> >>>  #define     GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
> >> >>>  #define     GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
> >> >>> @@ -7170,6 +7171,7 @@ enum skl_disp_power_wells {
> >> >>>  #define  LCPLL_CLK_FREQ_337_5_BDW	(2<<26)
> >> >>>  #define  LCPLL_CLK_FREQ_675_BDW		(3<<26)
> >> >>>  #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
> >> >>> +#define  LCPLL_ROOT_CD_CLOCK_DISABLE	(1<<24)
> >> >>>  #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
> >> >>>  #define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
> >> >>>  #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
> >> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> >>> index c3f01aa..b1e2069 100644
> >> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> >>> @@ -5751,7 +5751,22 @@ static void intel_update_max_cdclk(struct drm_device *dev)
> >> >>>  {
> >> >>>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> >>>  
> >> >>> -	if (IS_VALLEYVIEW(dev)) {
> >> >>> +	if (IS_BROADWELL(dev))  {
> >> >>> +		/*
> >> >>> +		 * FIXME with extra cooling we can allow
> >> >>> +		 * 540 MHz for ULX and 675 Mhz for ULT.
> >> >>> +		 * How can we know if extra cooling is
> >> >>> +		 * available? PCI ID, VTB, something else?
> >> >>> +		 */
> >> >>> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> >> >>> +			dev_priv->max_cdclk_freq = 450000;
> >> >>> +		else if (IS_BDW_ULX(dev))
> >> >>> +			dev_priv->max_cdclk_freq = 450000;
> >> >>> +		else if (IS_BDW_ULT(dev))
> >> >>> +			dev_priv->max_cdclk_freq = 540000;
> >> >>> +		else
> >> >>> +			dev_priv->max_cdclk_freq = 675000;
> >> >>> +	} else if (IS_VALLEYVIEW(dev)) {
> >> >>>  		dev_priv->max_cdclk_freq = 400000;
> >> >>>  	} else {
> >> >>>  		/* otherwise assume cdclk is fixed */
> >> >>> @@ -6621,13 +6636,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> >> >>>  		return true;
> >> >>>  
> >> >>>  	/*
> >> >>> -	 * FIXME if we compare against max we should then
> >> >>> -	 * increase the cdclk frequency when the current
> >> >>> -	 * value is too low. The other option is to compare
> >> >>> -	 * against the cdclk frequency we're going have post
> >> >>> -	 * modeset (ie. one we computed using other constraints).
> >> >>> -	 * Need to measure whether using a lower cdclk w/o IPS
> >> >>> -	 * is better or worse than a higher cdclk w/ IPS.
> >> >>> +	 * We compare against max which means we must take
> >> >>> +	 * the increased cdclk requirement into account when
> >> >>> +	 * calculating the new cdclk.
> >> >>> +	 *
> >> >>> +	 * Should measure whether using a lower cdclk w/o IPS
> >> >>>  	 */
> >> >>>  	return ilk_pipe_pixel_rate(pipe_config) <=
> >> >>>  		dev_priv->max_cdclk_freq * 95 / 100;
> >> >>> @@ -9608,6 +9621,182 @@ static void broxton_modeset_global_resources(struct drm_atomic_state *old_state)
> >> >>>  		broxton_set_cdclk(dev, req_cdclk);
> >> >>>  }
> >> >>>  
> >> >>> +/* compute the max rate for new configuration */
> >> >>> +static int ilk_max_pixel_rate(struct drm_i915_private *dev_priv)
> >> >>> +{
> >> >>> +	struct drm_device *dev = dev_priv->dev;
> >> >>> +	struct intel_crtc *intel_crtc;
> >> >>> +	struct drm_crtc *crtc;
> >> >>> +	int max_pixel_rate = 0;
> >> >>> +	int pixel_rate;
> >> >>> +
> >> >>> +	for_each_crtc(dev, crtc) {
> >> >>> +		if (!crtc->state->enable)
> >> >>> +			continue;
> >> >>> +
> >> >>> +		intel_crtc = to_intel_crtc(crtc);
> >> >>> +		pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
> >> >>> +
> >> >>> +		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> >> >>> +		if (IS_BROADWELL(dev) && intel_crtc->config->ips_enabled)
> >> >>> +			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> >> >>> +
> >> >>> +		max_pixel_rate = max(max_pixel_rate, pixel_rate);
> >> >>> +	}
> >> >>> +
> >> >>> +	return max_pixel_rate;
> >> >>> +}
> >> >>> +
> >> >>> +static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
> >> >>> +{
> >> >>> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >> >>> +	uint32_t val, data;
> >> >>> +	int ret;
> >> >>> +
> >> >>> +	if (WARN((I915_READ(LCPLL_CTL) &
> >> >>> +		  (LCPLL_PLL_DISABLE | LCPLL_PLL_LOCK |
> >> >>> +		   LCPLL_CD_CLOCK_DISABLE | LCPLL_ROOT_CD_CLOCK_DISABLE |
> >> >>> +		   LCPLL_CD2X_CLOCK_DISABLE | LCPLL_POWER_DOWN_ALLOW |
> >> >>> +		   LCPLL_CD_SOURCE_FCLK)) != LCPLL_PLL_LOCK,
> >> >>> +		 "trying to change cdclk frequency with cdclk not enabled\n"))
> >> >>> +		return;
> >> >>> +
> >> >>> +	mutex_lock(&dev_priv->rps.hw_lock);
> >> >>> +	ret = sandybridge_pcode_write(dev_priv,
> >> >>> +				      BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ, 0x0);
> >> >>> +	mutex_unlock(&dev_priv->rps.hw_lock);
> >> >>> +	if (ret) {
> >> >>> +		DRM_ERROR("failed to inform pcode about cdclk change\n");
> >> >>> +		return;
> >> >>> +	}
> >> >>> +
> >> >>> +	val = I915_READ(LCPLL_CTL);
> >> >>> +	val |= LCPLL_CD_SOURCE_FCLK;
> >> >>> +	I915_WRITE(LCPLL_CTL, val);
> >> >>> +
> >> >>> +	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
> >> >>> +			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
> >> >>> +		DRM_ERROR("Switching to FCLK failed\n");
> >> >>> +
> >> >>> +	val = I915_READ(LCPLL_CTL);
> >> >>> +	val &= ~LCPLL_CLK_FREQ_MASK;
> >> >>> +
> >> >>> +	switch (cdclk) {
> >> >>> +	case 450000:
> >> >>> +		val |= LCPLL_CLK_FREQ_450;
> >> >>> +		data = 0;
> >> >>> +		break;
> >> >>> +	case 540000:
> >> >>> +		val |= LCPLL_CLK_FREQ_54O_BDW;
> >> >>> +		data = 1;
> >> >>> +		break;
> >> >>> +	case 337500:
> >> >>> +		val |= LCPLL_CLK_FREQ_337_5_BDW;
> >> >>> +		data = 2;
> >> >>> +		break;
> >> >>> +	case 675000:
> >> >>> +		val |= LCPLL_CLK_FREQ_675_BDW;
> >> >>> +		data = 3;
> >> >>> +		break;
> >> >>> +	default:
> >> >>> +		WARN(1, "invalid cdclk frequency\n");
> >> >>> +		return;
> >> >>> +	}
> >> >>> +
> >> >>> +	I915_WRITE(LCPLL_CTL, val);
> >> >>> +
> >> >>> +	val = I915_READ(LCPLL_CTL);
> >> >>> +	val &= ~LCPLL_CD_SOURCE_FCLK;
> >> >>> +	I915_WRITE(LCPLL_CTL, val);
> >> >>> +
> >> >>> +	if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
> >> >>> +				LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
> >> >>> +		DRM_ERROR("Switching back to LCPLL failed\n");
> >> >>> +
> >> >>> +	mutex_lock(&dev_priv->rps.hw_lock);
> >> >>> +	sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, data);
> >> >>> +	mutex_unlock(&dev_priv->rps.hw_lock);
> >> >>> +
> >> >>> +	intel_update_cdclk(dev);
> >> >>> +
> >> >>> +	WARN(cdclk != dev_priv->cdclk_freq,
> >> >>> +	     "cdclk requested %d kHz but got %d kHz\n",
> >> >>> +	     cdclk, dev_priv->cdclk_freq);
> >> >>> +}
> >> >>> +
> >> >>> +static int broadwell_calc_cdclk(struct drm_i915_private *dev_priv,
> >> >>> +			      int max_pixel_rate)
> >> >>> +{
> >> >>> +	int cdclk;
> >> >>> +
> >> >>> +	/*
> >> >>> +	 * FIXME should also account for plane ratio
> >> >>> +	 * once 64bpp pixel formats are supported.
> >> >>> +	 */
> >> >>> +	if (max_pixel_rate > 540000)
> >> >>> +		cdclk = 675000;
> >> >>> +	else if (max_pixel_rate > 450000)
> >> >>> +		cdclk = 540000;
> >> >>> +	else if (max_pixel_rate > 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;
> >> >>> +	}
> >> >>> +
> >> >>> +	return cdclk;
> >> >>> +}
> >> >>> +
> >> >>> +static int broadwell_modeset_global_pipes(struct drm_atomic_state *state)
> >> >>> +{
> >> >>> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >> >>> +	struct drm_crtc *crtc;
> >> >>> +	struct drm_crtc_state *crtc_state;
> >> >>> +	int max_pixclk = ilk_max_pixel_rate(dev_priv);
> >> >>> +	int cdclk, i;
> >> >>> +
> >> >>> +	cdclk = broadwell_calc_cdclk(dev_priv, max_pixclk);
> >> >>> +
> >> >>> +	if (cdclk == dev_priv->cdclk_freq)
> >> >>> +		return 0;
> >> >>> +
> >> >>> +	/* add all active pipes to the state */
> >> >>> +	for_each_crtc(state->dev, crtc) {
> >> >>> +		if (!crtc->state->enable)
> >> >>> +			continue;
> >> >>> +
> >> >>> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >> >>> +		if (IS_ERR(crtc_state))
> >> >>> +			return PTR_ERR(crtc_state);
> >> >>> +	}
> >> >>> +
> >> >>> +	/* disable/enable all currently active pipes while we change cdclk */
> >> >>> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
> >> >>> +		if (crtc_state->enable)
> >> >>> +			crtc_state->mode_changed = true;
> >> >>> +
> >> >>> +	return 0;
> >> >>> +}
> >> >>> +
> >> >>> +static void broadwell_modeset_global_resources(struct drm_atomic_state *state)
> >> >>> +{
> >> >>> +	struct drm_device *dev = state->dev;
> >> >>> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >> >>> +	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
> >> >>> +	int req_cdclk = broadwell_calc_cdclk(dev_priv, max_pixel_rate);
> >> >>> +
> >> >>> +	if (req_cdclk != dev_priv->cdclk_freq)
> >> >>> +		broadwell_set_cdclk(dev, req_cdclk);
> >> >>> +}
> >> >>> +
> >> >>>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> >> >>>  				      struct intel_crtc_state *crtc_state)
> >> >>>  {
> >> >>> @@ -12788,8 +12977,12 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
> >> >>>  	 * mode set on this crtc.  For other crtcs we need to use the
> >> >>>  	 * adjusted_mode bits in the crtc directly.
> >> >>>  	 */
> >> >>> -	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev)) {
> >> >>> -		ret = valleyview_modeset_global_pipes(state);
> >> >>> +	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev)) {
> >> >>> +		if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev))
> >> >>> +			ret = valleyview_modeset_global_pipes(state);
> >> >>> +		else
> >> >>> +			ret = broadwell_modeset_global_pipes(state);
> >> >>> +
> >> >>>  		if (ret)
> >> >>>  			return ret;
> >> >>>  	}
> >> >>> @@ -14677,6 +14870,9 @@ static void intel_init_display(struct drm_device *dev)
> >> >>>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> >> >>>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> >> >>>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
> >> >>> +		if (IS_BROADWELL(dev))
> >> >>> +			dev_priv->display.modeset_global_resources =
> >> >>> +				broadwell_modeset_global_resources;
> >> >>>  	} else if (IS_VALLEYVIEW(dev)) {
> >> >>>  		dev_priv->display.modeset_global_resources =
> >> >>>  			valleyview_modeset_global_resources;
> >> >>> -- 
> >> >>> 1.9.1
> >> >>>
> >> >>> _______________________________________________
> >> >>> Intel-gfx mailing list
> >> >>> Intel-gfx@lists.freedesktop.org
> >> >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >>
> >> >> -- 
> >> >> Jani Nikula, Intel Open Source Technology Center
> >> >
> >> > -- 
> >> > Jani Nikula, Intel Open Source Technology Center
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Technology Center
> >
> >
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

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

Re: [PATCH v6 7/8] drm/i915: BDW clock change support

[Daniel Vetter]

On Mon, Jun 29, 2015 at 02:46:41PM +0300, Ville Syrjälä wrote:
> On Mon, Jun 29, 2015 at 02:42:25PM +0300, Jani Nikula wrote:
> > On Mon, 29 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> > > On Mon, 2015-06-29 at 14:24 +0300, Jani Nikula wrote:
> > >> On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > >> > On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > >> >> On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> > >> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> >>>
> > >> >>> Add support for changing cdclk frequency during runtime on BDW. The
> > >> >>> procedure is quite a bit different on BDW from the one on HSW, so
> > >> >>> add a separate function for it.
> > >> >>>
> > >> >>> Also with IPS enabled the actual pixel rate mustn't exceed 95% of cdclk,
> > >> >>> so take that into account when computing the max pixel rate.
> > >> >>>
> > >> >>> v2: Grab rps.hw_lock around sandybridge_pcode_write()
> > >> >>> v3: Rebase due to power well vs. .global_resources() reordering
> > >> >>> v4: Rebased to the latest
> > >> >>> v5: Rebased to the latest
> > >> >>> v6: Patch order shuffle so that Broadwell CD clock change is
> > >> >>>     applied before the patch for Haswell CD clock change
> > >> >>> v7: Fix for patch style problems
> > >> >>>
> > >> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> >>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > >> >>
> > >> >> This patch hard hangs my BDW NUC at boot when both DP and HDMI are
> > >> >> connected. Either DP or HDMI alone are good, same with hotplugging the
> > >> >> other afterwards. Booting to grub with both connected, and unplugging
> > >> >> HDMI before loading the kernel also reproduces the issue.
> > >> >>
> > >> >> It looks like the problem boils down to the BIOS setting up a smaller
> > >> >> resolution on the DP display when both are connected, and this patch
> > >> >> fails to cope with that on i915 load.
> > >> >
> > >> > By "this patch" I obviously refer to
> > >> >
> > >> > commit b432e5cfd5e92127ad2dd83bfc3083f1dbce43fb
> > >> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> > Date:   Wed Jun 3 15:45:13 2015 +0300
> > >> >
> > >> >     drm/i915: BDW clock change support
> > >> >
> > >> > and everything works for the commit before that.
> > >> 
> > >> Anyone working on this, or should we revert?
> > >> 
> > >> BR,
> > >> Jani.
> > >> 
> > > My understanding is that Ville has an idea or how to fix this.
> > 
> > Ville, when do you think we can convert the idea into a patch? Should we
> > revert in the mean time?
> 
> I was thinking it may have been atomic making a mess of things and
> enabling planes while the pipe was off. But I may be totally off here
> since the current atomic code doesn't agree with my brain at all.
> 
> So no, I have no patch to fix it. Someone needs to first check where
> exactly the hang happens.

We do have evidence that at least the code in 4.2 does touch planes when
the pipe is off. Linus has backtraces at least.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 7/8] drm/i915: BDW clock change support [regression]

[Daniel Vetter]

On Tue, Jun 16, 2015 at 04:07:40PM +0300, Jani Nikula wrote:
> On Tue, 16 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Wed, 03 Jun 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Add support for changing cdclk frequency during runtime on BDW. The
> >> procedure is quite a bit different on BDW from the one on HSW, so
> >> add a separate function for it.
> >>
> >> Also with IPS enabled the actual pixel rate mustn't exceed 95% of cdclk,
> >> so take that into account when computing the max pixel rate.
> >>
> >> v2: Grab rps.hw_lock around sandybridge_pcode_write()
> >> v3: Rebase due to power well vs. .global_resources() reordering
> >> v4: Rebased to the latest
> >> v5: Rebased to the latest
> >> v6: Patch order shuffle so that Broadwell CD clock change is
> >>     applied before the patch for Haswell CD clock change
> >> v7: Fix for patch style problems
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >
> > This patch hard hangs my BDW NUC at boot when both DP and HDMI are
> > connected. Either DP or HDMI alone are good, same with hotplugging the
> > other afterwards. Booting to grub with both connected, and unplugging
> > HDMI before loading the kernel also reproduces the issue.
> >
> > It looks like the problem boils down to the BIOS setting up a smaller
> > resolution on the DP display when both are connected, and this patch
> > fails to cope with that on i915 load.
> 
> By "this patch" I obviously refer to
> 
> commit b432e5cfd5e92127ad2dd83bfc3083f1dbce43fb
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Wed Jun 3 15:45:13 2015 +0300
> 
>     drm/i915: BDW clock change support
> 
> and everything works for the commit before that.

Another regression for Jairo to track. Jairo please add the bisect result
to the bugzilla and mark it as bisected too.

Thanks, Daniel

> 
> BR,
> Jani.
> 
> 
> 
> 
> 
> >
> > BR,
> > Jani.
> >
> >
> >
> >>
> >> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_reg.h      |   2 +
> >>  drivers/gpu/drm/i915/intel_display.c | 216 +++++++++++++++++++++++++++++++++--
> >>  2 files changed, 208 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index 7213224..0f72c0e 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -6705,6 +6705,7 @@ enum skl_disp_power_wells {
> >>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
> >>  #define     GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
> >>  #define     GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> >> +#define   BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ	0x18
> >>  #define   GEN9_PCODE_READ_MEM_LATENCY		0x6
> >>  #define     GEN9_MEM_LATENCY_LEVEL_MASK		0xFF
> >>  #define     GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT	8
> >> @@ -7170,6 +7171,7 @@ enum skl_disp_power_wells {
> >>  #define  LCPLL_CLK_FREQ_337_5_BDW	(2<<26)
> >>  #define  LCPLL_CLK_FREQ_675_BDW		(3<<26)
> >>  #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
> >> +#define  LCPLL_ROOT_CD_CLOCK_DISABLE	(1<<24)
> >>  #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
> >>  #define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
> >>  #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index c3f01aa..b1e2069 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5751,7 +5751,22 @@ static void intel_update_max_cdclk(struct drm_device *dev)
> >>  {
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  
> >> -	if (IS_VALLEYVIEW(dev)) {
> >> +	if (IS_BROADWELL(dev))  {
> >> +		/*
> >> +		 * FIXME with extra cooling we can allow
> >> +		 * 540 MHz for ULX and 675 Mhz for ULT.
> >> +		 * How can we know if extra cooling is
> >> +		 * available? PCI ID, VTB, something else?
> >> +		 */
> >> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> >> +			dev_priv->max_cdclk_freq = 450000;
> >> +		else if (IS_BDW_ULX(dev))
> >> +			dev_priv->max_cdclk_freq = 450000;
> >> +		else if (IS_BDW_ULT(dev))
> >> +			dev_priv->max_cdclk_freq = 540000;
> >> +		else
> >> +			dev_priv->max_cdclk_freq = 675000;
> >> +	} else if (IS_VALLEYVIEW(dev)) {
> >>  		dev_priv->max_cdclk_freq = 400000;
> >>  	} else {
> >>  		/* otherwise assume cdclk is fixed */
> >> @@ -6621,13 +6636,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> >>  		return true;
> >>  
> >>  	/*
> >> -	 * FIXME if we compare against max we should then
> >> -	 * increase the cdclk frequency when the current
> >> -	 * value is too low. The other option is to compare
> >> -	 * against the cdclk frequency we're going have post
> >> -	 * modeset (ie. one we computed using other constraints).
> >> -	 * Need to measure whether using a lower cdclk w/o IPS
> >> -	 * is better or worse than a higher cdclk w/ IPS.
> >> +	 * We compare against max which means we must take
> >> +	 * the increased cdclk requirement into account when
> >> +	 * calculating the new cdclk.
> >> +	 *
> >> +	 * Should measure whether using a lower cdclk w/o IPS
> >>  	 */
> >>  	return ilk_pipe_pixel_rate(pipe_config) <=
> >>  		dev_priv->max_cdclk_freq * 95 / 100;
> >> @@ -9608,6 +9621,182 @@ static void broxton_modeset_global_resources(struct drm_atomic_state *old_state)
> >>  		broxton_set_cdclk(dev, req_cdclk);
> >>  }
> >>  
> >> +/* compute the max rate for new configuration */
> >> +static int ilk_max_pixel_rate(struct drm_i915_private *dev_priv)
> >> +{
> >> +	struct drm_device *dev = dev_priv->dev;
> >> +	struct intel_crtc *intel_crtc;
> >> +	struct drm_crtc *crtc;
> >> +	int max_pixel_rate = 0;
> >> +	int pixel_rate;
> >> +
> >> +	for_each_crtc(dev, crtc) {
> >> +		if (!crtc->state->enable)
> >> +			continue;
> >> +
> >> +		intel_crtc = to_intel_crtc(crtc);
> >> +		pixel_rate = ilk_pipe_pixel_rate(intel_crtc->config);
> >> +
> >> +		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> >> +		if (IS_BROADWELL(dev) && intel_crtc->config->ips_enabled)
> >> +			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> >> +
> >> +		max_pixel_rate = max(max_pixel_rate, pixel_rate);
> >> +	}
> >> +
> >> +	return max_pixel_rate;
> >> +}
> >> +
> >> +static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
> >> +{
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	uint32_t val, data;
> >> +	int ret;
> >> +
> >> +	if (WARN((I915_READ(LCPLL_CTL) &
> >> +		  (LCPLL_PLL_DISABLE | LCPLL_PLL_LOCK |
> >> +		   LCPLL_CD_CLOCK_DISABLE | LCPLL_ROOT_CD_CLOCK_DISABLE |
> >> +		   LCPLL_CD2X_CLOCK_DISABLE | LCPLL_POWER_DOWN_ALLOW |
> >> +		   LCPLL_CD_SOURCE_FCLK)) != LCPLL_PLL_LOCK,
> >> +		 "trying to change cdclk frequency with cdclk not enabled\n"))
> >> +		return;
> >> +
> >> +	mutex_lock(&dev_priv->rps.hw_lock);
> >> +	ret = sandybridge_pcode_write(dev_priv,
> >> +				      BDW_PCODE_DISPLAY_FREQ_CHANGE_REQ, 0x0);
> >> +	mutex_unlock(&dev_priv->rps.hw_lock);
> >> +	if (ret) {
> >> +		DRM_ERROR("failed to inform pcode about cdclk change\n");
> >> +		return;
> >> +	}
> >> +
> >> +	val = I915_READ(LCPLL_CTL);
> >> +	val |= LCPLL_CD_SOURCE_FCLK;
> >> +	I915_WRITE(LCPLL_CTL, val);
> >> +
> >> +	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
> >> +			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
> >> +		DRM_ERROR("Switching to FCLK failed\n");
> >> +
> >> +	val = I915_READ(LCPLL_CTL);
> >> +	val &= ~LCPLL_CLK_FREQ_MASK;
> >> +
> >> +	switch (cdclk) {
> >> +	case 450000:
> >> +		val |= LCPLL_CLK_FREQ_450;
> >> +		data = 0;
> >> +		break;
> >> +	case 540000:
> >> +		val |= LCPLL_CLK_FREQ_54O_BDW;
> >> +		data = 1;
> >> +		break;
> >> +	case 337500:
> >> +		val |= LCPLL_CLK_FREQ_337_5_BDW;
> >> +		data = 2;
> >> +		break;
> >> +	case 675000:
> >> +		val |= LCPLL_CLK_FREQ_675_BDW;
> >> +		data = 3;
> >> +		break;
> >> +	default:
> >> +		WARN(1, "invalid cdclk frequency\n");
> >> +		return;
> >> +	}
> >> +
> >> +	I915_WRITE(LCPLL_CTL, val);
> >> +
> >> +	val = I915_READ(LCPLL_CTL);
> >> +	val &= ~LCPLL_CD_SOURCE_FCLK;
> >> +	I915_WRITE(LCPLL_CTL, val);
> >> +
> >> +	if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
> >> +				LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
> >> +		DRM_ERROR("Switching back to LCPLL failed\n");
> >> +
> >> +	mutex_lock(&dev_priv->rps.hw_lock);
> >> +	sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, data);
> >> +	mutex_unlock(&dev_priv->rps.hw_lock);
> >> +
> >> +	intel_update_cdclk(dev);
> >> +
> >> +	WARN(cdclk != dev_priv->cdclk_freq,
> >> +	     "cdclk requested %d kHz but got %d kHz\n",
> >> +	     cdclk, dev_priv->cdclk_freq);
> >> +}
> >> +
> >> +static int broadwell_calc_cdclk(struct drm_i915_private *dev_priv,
> >> +			      int max_pixel_rate)
> >> +{
> >> +	int cdclk;
> >> +
> >> +	/*
> >> +	 * FIXME should also account for plane ratio
> >> +	 * once 64bpp pixel formats are supported.
> >> +	 */
> >> +	if (max_pixel_rate > 540000)
> >> +		cdclk = 675000;
> >> +	else if (max_pixel_rate > 450000)
> >> +		cdclk = 540000;
> >> +	else if (max_pixel_rate > 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;
> >> +	}
> >> +
> >> +	return cdclk;
> >> +}
> >> +
> >> +static int broadwell_modeset_global_pipes(struct drm_atomic_state *state)
> >> +{
> >> +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >> +	struct drm_crtc *crtc;
> >> +	struct drm_crtc_state *crtc_state;
> >> +	int max_pixclk = ilk_max_pixel_rate(dev_priv);
> >> +	int cdclk, i;
> >> +
> >> +	cdclk = broadwell_calc_cdclk(dev_priv, max_pixclk);
> >> +
> >> +	if (cdclk == dev_priv->cdclk_freq)
> >> +		return 0;
> >> +
> >> +	/* add all active pipes to the state */
> >> +	for_each_crtc(state->dev, crtc) {
> >> +		if (!crtc->state->enable)
> >> +			continue;
> >> +
> >> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >> +		if (IS_ERR(crtc_state))
> >> +			return PTR_ERR(crtc_state);
> >> +	}
> >> +
> >> +	/* disable/enable all currently active pipes while we change cdclk */
> >> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
> >> +		if (crtc_state->enable)
> >> +			crtc_state->mode_changed = true;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void broadwell_modeset_global_resources(struct drm_atomic_state *state)
> >> +{
> >> +	struct drm_device *dev = state->dev;
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
> >> +	int req_cdclk = broadwell_calc_cdclk(dev_priv, max_pixel_rate);
> >> +
> >> +	if (req_cdclk != dev_priv->cdclk_freq)
> >> +		broadwell_set_cdclk(dev, req_cdclk);
> >> +}
> >> +
> >>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> >>  				      struct intel_crtc_state *crtc_state)
> >>  {
> >> @@ -12788,8 +12977,12 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
> >>  	 * mode set on this crtc.  For other crtcs we need to use the
> >>  	 * adjusted_mode bits in the crtc directly.
> >>  	 */
> >> -	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev)) {
> >> -		ret = valleyview_modeset_global_pipes(state);
> >> +	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev) || IS_BROADWELL(dev)) {
> >> +		if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev))
> >> +			ret = valleyview_modeset_global_pipes(state);
> >> +		else
> >> +			ret = broadwell_modeset_global_pipes(state);
> >> +
> >>  		if (ret)
> >>  			return ret;
> >>  	}
> >> @@ -14677,6 +14870,9 @@ static void intel_init_display(struct drm_device *dev)
> >>  		dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
> >>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> >>  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
> >> +		if (IS_BROADWELL(dev))
> >> +			dev_priv->display.modeset_global_resources =
> >> +				broadwell_modeset_global_resources;
> >>  	} else if (IS_VALLEYVIEW(dev)) {
> >>  		dev_priv->display.modeset_global_resources =
> >>  			valleyview_modeset_global_resources;
> >> -- 
> >> 1.9.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx