[PATCH 00/13] DVFS v2

[PATCH 00/13] DVFS v2

[Rodrigo Vivi]

These v2 fixes bugs found by CI, also shuffle things
around to get a bit more organized and avoid temporary
duplications.

Also by the last patch I try to make functions more generic
and include documentation.

So the only missing bit would be expanding that back to SKL.
But I suffered a lot with rebase around today and I'm sure
that skl patch would just increase the pain for now with no
benefit. So this is to be done after this start getting merged.

Thanks,
Rodrigo.

Kahola, Mika (3):
  drm/i915/cnl: Expose DVFS change functions
  drm/i915/cnl: DVFS for PLL enabling
  drm/i915/cnl: DVFS for PLL disabling

Paulo Zanoni (1):
  drm/i915/cnl: extract cnl_dvfs_{pre,post}_change

Rodrigo Vivi (9):
  drm/i915: Let's use more enum intel_dpll_id pll_id.
  drm/i915/cnl: Extract cnl_calc_pll_link following bxt style.
  drm/i915/skl: Extract skl_calc_pll_link following bxt,cnl style.
  drm/i915: Unify and export gen9+ port_clock calculation.
  drm/i915/cnl: Invert dvfs default level.
  drm/i915/cnl: Unify dvfs level selection.
  drm/i915/cnl: Only request voltage frequency switching when needed.
  drm/i915/cnl: When disabling pll put dvfs back to cdclk requirement.
  drm/i915: Make DVFS more generic and document them.

 drivers/gpu/drm/i915/intel_cdclk.c    | 108 ++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_ddi.c      |  85 +++++++++++++++++---------
 drivers/gpu/drm/i915/intel_dpll_mgr.c |  42 ++++++++-----
 drivers/gpu/drm/i915/intel_drv.h      |   7 ++-
 4 files changed, 175 insertions(+), 67 deletions(-)

-- 
2.13.5

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

[PATCH 01/13] drm/i915: Let's use more enum intel_dpll_id pll_id.

[Rodrigo Vivi]

No functional change expected. Just let's use this enum
when possible and also same standard pll_id name
so we can rework gen9+ port clock later.

Cc: Mika Kahola <mika.kahola@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 93cbbcbbc193..b5dd82a0e357 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1102,14 +1102,14 @@ static int hsw_ddi_calc_wrpll_link(struct drm_i915_private *dev_priv,
 }
 
 static int skl_calc_wrpll_link(struct drm_i915_private *dev_priv,
-			       uint32_t dpll)
+			       enum intel_dpll_id pll_id)
 {
 	i915_reg_t cfgcr1_reg, cfgcr2_reg;
 	uint32_t cfgcr1_val, cfgcr2_val;
 	uint32_t p0, p1, p2, dco_freq;
 
-	cfgcr1_reg = DPLL_CFGCR1(dpll);
-	cfgcr2_reg = DPLL_CFGCR2(dpll);
+	cfgcr1_reg = DPLL_CFGCR1(pll_id);
+	cfgcr2_reg = DPLL_CFGCR2(pll_id);
 
 	cfgcr1_val = I915_READ(cfgcr1_reg);
 	cfgcr2_val = I915_READ(cfgcr2_reg);
@@ -1162,7 +1162,7 @@ static int skl_calc_wrpll_link(struct drm_i915_private *dev_priv,
 }
 
 static int cnl_calc_wrpll_link(struct drm_i915_private *dev_priv,
-			       uint32_t pll_id)
+			       enum intel_dpll_id pll_id)
 {
 	uint32_t cfgcr0, cfgcr1;
 	uint32_t p0, p1, p2, dco_freq, ref_clock;
@@ -1246,7 +1246,8 @@ static void cnl_ddi_clock_get(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	int link_clock = 0;
-	uint32_t cfgcr0, pll_id;
+	uint32_t cfgcr0;
+	enum intel_dpll_id pll_id;
 
 	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
 
@@ -1299,17 +1300,18 @@ static void skl_ddi_clock_get(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	int link_clock = 0;
-	uint32_t dpll_ctl1, dpll;
+	uint32_t dpll_ctl1;
+	enum intel_dpll_id pll_id;
 
-	dpll = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
+	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
 
 	dpll_ctl1 = I915_READ(DPLL_CTRL1);
 
-	if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(dpll)) {
-		link_clock = skl_calc_wrpll_link(dev_priv, dpll);
+	if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(pll_id)) {
+		link_clock = skl_calc_wrpll_link(dev_priv, pll_id);
 	} else {
-		link_clock = dpll_ctl1 & DPLL_CTRL1_LINK_RATE_MASK(dpll);
-		link_clock >>= DPLL_CTRL1_LINK_RATE_SHIFT(dpll);
+		link_clock = dpll_ctl1 & DPLL_CTRL1_LINK_RATE_MASK(pll_id);
+		link_clock >>= DPLL_CTRL1_LINK_RATE_SHIFT(pll_id);
 
 		switch (link_clock) {
 		case DPLL_CTRL1_LINK_RATE_810:
@@ -1390,17 +1392,17 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
 }
 
 static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
-				enum intel_dpll_id dpll)
+			     enum intel_dpll_id pll_id)
 {
 	struct intel_shared_dpll *pll;
 	struct intel_dpll_hw_state *state;
 	struct dpll clock;
 
 	/* For DDI ports we always use a shared PLL. */
-	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
+	if (WARN_ON(pll_id == DPLL_ID_PRIVATE))
 		return 0;
 
-	pll = &dev_priv->shared_dplls[dpll];
+	pll = &dev_priv->shared_dplls[pll_id];
 	state = &pll->state.hw_state;
 
 	clock.m1 = 2;
@@ -1419,9 +1421,9 @@ static void bxt_ddi_clock_get(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	enum port port = intel_ddi_get_encoder_port(encoder);
-	uint32_t dpll = port;
+	enum intel_dpll_id pll_id = port;
 
-	pipe_config->port_clock = bxt_calc_pll_link(dev_priv, dpll);
+	pipe_config->port_clock = bxt_calc_pll_link(dev_priv, pll_id);
 
 	ddi_dotclock_get(pipe_config);
 }
-- 
2.13.5

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

[PATCH 02/13] drm/i915/cnl: Extract cnl_calc_pll_link following bxt style.

[Rodrigo Vivi]

No functional change. Just spliting the function for
better port clock handling later.

v2: Put link_clock *=2 inside the function only for DP,
    otherwise we mess up clocks on HDMI. (Caught by CI).

Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b5dd82a0e357..71040c3dd6fc 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1241,15 +1241,11 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
 	pipe_config->base.adjusted_mode.crtc_clock = dotclock;
 }
 
-static void cnl_ddi_clock_get(struct intel_encoder *encoder,
-			      struct intel_crtc_state *pipe_config)
+static int cnl_calc_pll_link(struct drm_i915_private *dev_priv,
+			     enum intel_dpll_id pll_id)
 {
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	int link_clock = 0;
 	uint32_t cfgcr0;
-	enum intel_dpll_id pll_id;
-
-	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
 
 	cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));
 
@@ -1290,7 +1286,18 @@ static void cnl_ddi_clock_get(struct intel_encoder *encoder,
 		link_clock *= 2;
 	}
 
-	pipe_config->port_clock = link_clock;
+	return link_clock;
+}
+
+static void cnl_ddi_clock_get(struct intel_encoder *encoder,
+			      struct intel_crtc_state *pipe_config)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum intel_dpll_id pll_id;
+
+	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
+
+	pipe_config->port_clock = cnl_calc_pll_link(dev_priv, pll_id);
 
 	ddi_dotclock_get(pipe_config);
 }
-- 
2.13.5

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

[PATCH 03/13] drm/i915/skl: Extract skl_calc_pll_link following bxt, cnl style.

[Rodrigo Vivi]

No functional change. Just spliting the function for
better port clock handling later.

v2: Fix subject and also put link_clock *= 2 back inside
    the new function so we only multiply for DP and avoid
    messing up the HDMI clocks. (Caught by CI).

Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 71040c3dd6fc..92eabb6cc1ab 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1302,15 +1302,11 @@ static void cnl_ddi_clock_get(struct intel_encoder *encoder,
 	ddi_dotclock_get(pipe_config);
 }
 
-static void skl_ddi_clock_get(struct intel_encoder *encoder,
-				struct intel_crtc_state *pipe_config)
+static int skl_calc_pll_link(struct drm_i915_private *dev_priv,
+			     enum intel_dpll_id pll_id)
 {
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	int link_clock = 0;
 	uint32_t dpll_ctl1;
-	enum intel_dpll_id pll_id;
-
-	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
 
 	dpll_ctl1 = I915_READ(DPLL_CTRL1);
 
@@ -1346,7 +1342,18 @@ static void skl_ddi_clock_get(struct intel_encoder *encoder,
 		link_clock *= 2;
 	}
 
-	pipe_config->port_clock = link_clock;
+	return link_clock;
+}
+
+static void skl_ddi_clock_get(struct intel_encoder *encoder,
+			      struct intel_crtc_state *pipe_config)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum intel_dpll_id pll_id;
+
+	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
+
+	pipe_config->port_clock = skl_calc_pll_link(dev_priv, pll_id);
 
 	ddi_dotclock_get(pipe_config);
 }
-- 
2.13.5

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

[PATCH 04/13] drm/i915: Unify and export gen9+ port_clock calculation.

[Rodrigo Vivi]

On Cannonlake the DVFS level selection depends on the
port clock.

So let's re-org in a way that we can easily export without
duplicating any code.

v2: Rebased on changes on previous patches

Cc: Mika Kahola <mika.kahola@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 21 ++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h |  3 ++-
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 92eabb6cc1ab..ee64b1a50453 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1297,7 +1297,7 @@ static void cnl_ddi_clock_get(struct intel_encoder *encoder,
 
 	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
 
-	pipe_config->port_clock = cnl_calc_pll_link(dev_priv, pll_id);
+	pipe_config->port_clock = intel_ddi_port_clock(dev_priv, pll_id);
 
 	ddi_dotclock_get(pipe_config);
 }
@@ -1353,7 +1353,7 @@ static void skl_ddi_clock_get(struct intel_encoder *encoder,
 
 	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
 
-	pipe_config->port_clock = skl_calc_pll_link(dev_priv, pll_id);
+	pipe_config->port_clock = intel_ddi_port_clock(dev_priv, pll_id);
 
 	ddi_dotclock_get(pipe_config);
 }
@@ -1437,11 +1437,26 @@ static void bxt_ddi_clock_get(struct intel_encoder *encoder,
 	enum port port = intel_ddi_get_encoder_port(encoder);
 	enum intel_dpll_id pll_id = port;
 
-	pipe_config->port_clock = bxt_calc_pll_link(dev_priv, pll_id);
+	pipe_config->port_clock = intel_ddi_port_clock(dev_priv, pll_id);
 
 	ddi_dotclock_get(pipe_config);
 }
 
+int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
+			 enum intel_dpll_id pll_id)
+{
+	if (IS_GEN9_BC(dev_priv)) {
+		return skl_calc_pll_link(dev_priv, pll_id);
+	} else if (IS_GEN9_LP(dev_priv)) {
+		return bxt_calc_pll_link(dev_priv, pll_id);
+	} else if (IS_CANNONLAKE(dev_priv)) {
+		return cnl_calc_pll_link(dev_priv, pll_id);
+	} else {
+		MISSING_CASE(INTEL_GEN(dev_priv));
+		return 0;
+	}
+}
+
 void intel_ddi_clock_get(struct intel_encoder *encoder,
 			 struct intel_crtc_state *pipe_config)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0cab667fff57..fe4650d6db03 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1293,7 +1293,8 @@ bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
 				 struct intel_crtc *intel_crtc);
 void intel_ddi_get_config(struct intel_encoder *encoder,
 			  struct intel_crtc_state *pipe_config);
-
+int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
+			 enum intel_dpll_id pll_id);
 void intel_ddi_clock_get(struct intel_encoder *encoder,
 			 struct intel_crtc_state *pipe_config);
 void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state *crtc_state,
-- 
2.13.5

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

[PATCH 05/13] drm/i915/cnl: extract cnl_dvfs_{pre, post}_change

[Rodrigo Vivi]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

These functions even have their own page in our spec,
so extract them from cnl_set_cdclk().

v2: (By Rodrigo) Fixed inverted logic on error return of
    cnl_dvfs_pre_change.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 87fc42b19336..b35eb145d66e 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1510,12 +1510,8 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
 	dev_priv->cdclk.hw.vco = vco;
 }
 
-static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
-			  const struct intel_cdclk_state *cdclk_state)
+static int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
 {
-	int cdclk = cdclk_state->cdclk;
-	int vco = cdclk_state->vco;
-	u32 val, divider, pcu_ack;
 	int ret;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
@@ -1524,11 +1520,30 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
 				SKL_CDCLK_READY_FOR_CHANGE,
 				SKL_CDCLK_READY_FOR_CHANGE, 3);
 	mutex_unlock(&dev_priv->rps.hw_lock);
-	if (ret) {
+
+	if (ret)
 		DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
 			  ret);
+
+	return ret;
+}
+
+static void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level)
+{
+	mutex_lock(&dev_priv->rps.hw_lock);
+	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, level);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+}
+
+static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
+			  const struct intel_cdclk_state *cdclk_state)
+{
+	int cdclk = cdclk_state->cdclk;
+	int vco = cdclk_state->vco;
+	u32 val, divider, pcu_ack;
+
+	if (cnl_dvfs_pre_change(dev_priv))
 		return;
-	}
 
 	/* cdclk = vco / 2 / div{1,2} */
 	switch (DIV_ROUND_CLOSEST(vco, cdclk)) {
@@ -1575,9 +1590,7 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
 	I915_WRITE(CDCLK_CTL, val);
 
 	/* inform PCU of the change */
-	mutex_lock(&dev_priv->rps.hw_lock);
-	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, pcu_ack);
-	mutex_unlock(&dev_priv->rps.hw_lock);
+	cnl_dvfs_post_change(dev_priv, pcu_ack);
 
 	intel_update_cdclk(dev_priv);
 }
-- 
2.13.5

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

[PATCH 06/13] drm/i915/cnl: Expose DVFS change functions

[Rodrigo Vivi]

From: "Kahola, Mika" <mika.kahola@intel.com>

DVFS computation needs cnl_dvfs_{pre,post}_change() functions to be exposed.
These functions will be used when computing DVFS levels in intel_dpll_mgr.c

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 4 ++--
 drivers/gpu/drm/i915/intel_drv.h   | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index b35eb145d66e..af8411c2a6b9 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1510,7 +1510,7 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
 	dev_priv->cdclk.hw.vco = vco;
 }
 
-static int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
+int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
@@ -1528,7 +1528,7 @@ static int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level)
+void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level)
 {
 	mutex_lock(&dev_priv->rps.hw_lock);
 	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, level);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fe4650d6db03..934ccf17f8ab 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1323,6 +1323,8 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv);
 void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
 void cnl_init_cdclk(struct drm_i915_private *dev_priv);
 void cnl_uninit_cdclk(struct drm_i915_private *dev_priv);
+int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
+void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level);
 void bxt_init_cdclk(struct drm_i915_private *dev_priv);
 void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
 void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
-- 
2.13.5

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

[PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

[Rodrigo Vivi]

From: "Kahola, Mika" <mika.kahola@intel.com>

Display Voltage and Frequency Switching (DVFS) is used to adjust the
display voltage to match the display clock frequencies. If voltage is
set too low, it will break functionality. If voltage is set too high,
it will waste power. Voltage level is selected based on CD clock and
DDI clock.

The sequence before frequency change is the following and it requests
the power controller to raise voltage to maximum

- Ensure any previous GT Driver Mailbox transaction is complete.
- Write GT Driver Mailbox Data Low = 0x3.
- Write GT Driver Mailbox Data High = 0x0.
- Write GT Driver Mailbox Interface = 0x80000007.
- Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
- Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
  Timeout after 3ms

The sequence after frequency change is the following and it requests
the port controller to raise voltage to the requested level.

- Write GT Driver Mailbox Data Low
 * For level 0, write 0x0
 * For level 1, write 0x1
 * For level 2, write 0x2
 * For level 3, write 0x3
   - Write GT Driver Mailbox Data High = 0x0.
   - Write GT Driver Mailbox Interface = 0x80000007.

For Cannonlake, the level 3 is not used and it aliases to level 2.

v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
    [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
    The port clock calculation here was only addressing DP, so let's reuse
    the current port calculation that is already in place without any duplication.
    Alos fix portclk <= 594000 instead of portclk < 594000.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index a2a3d93d67bd..6030fbafa580 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
 	.dump_hw_state = bxt_dump_hw_state,
 };
 
+static int cnl_get_dvfs_level(int cdclk, int portclk)
+{
+	if (cdclk == 168000 && portclk <= 594000)
+		return 0;
+	else if (cdclk == 336000 && portclk <= 594000)
+		return 1;
+	else
+		return 2;
+}
+
 static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 			       struct intel_shared_dpll *pll)
 {
 	uint32_t val;
+	int ret;
+	int level;
+	int cdclk, portclk;
 
 	/* 1. Enable DPLL power in DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
@@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	/*
 	 * 5. If the frequency will result in a change to the voltage
 	 * requirement, follow the Display Voltage Frequency Switching
-	 * Sequence Before Frequency Change
-	 *
-	 * FIXME: (DVFS) is used to adjust the display voltage to match the
-	 * display clock frequencies
+	 * (DVFS) Sequence Before Frequency Change
 	 */
+	ret = cnl_dvfs_pre_change(dev_priv);
 
 	/* 6. Enable DPLL in DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
@@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	/*
 	 * 8. If the frequency will result in a change to the voltage
 	 * requirement, follow the Display Voltage Frequency Switching
-	 * Sequence After Frequency Change
-	 *
-	 * FIXME: (DVFS) is used to adjust the display voltage to match the
-	 * display clock frequencies
+	 * (DVFS) Sequence After Frequency Change
 	 */
+	if (ret == 0) {
+		cdclk = dev_priv->cdclk.hw.cdclk;
+		portclk = intel_ddi_port_clock(dev_priv, pll->id);
+		level = cnl_get_dvfs_level(cdclk, portclk);
+		cnl_dvfs_post_change(dev_priv, level);
+	}
 
 	/*
 	 * 9. turn on the clock for the DDI and map the DPLL to the DDI
-- 
2.13.5

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

[PATCH 08/13] drm/i915/cnl: DVFS for PLL disabling

[Rodrigo Vivi]

From: "Kahola, Mika" <mika.kahola@intel.com>

Display Voltage and Frequency Switching (DVFS) is used to adjust the
display voltage to match the display clock frequencies. To save power the
voltage is set to minimum when disabling PLL.

The sequence before frequency change is the following and it requests
the power controller to raise voltage to maximum

- Ensure any previous GT Driver Mailbox transaction is complete.
- Write GT Driver Mailbox Data Low = 0x3.
- Write GT Driver Mailbox Data High = 0x0.
- Write GT Driver Mailbox Interface = 0x80000007.
- Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
- Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
  Timeout after 3ms

The sequence after frequency change is the following and it requests
the port controller to lower voltage to the minimum.

- Write GT Driver Mailbox Data Low = 0x0
- Write GT Driver Mailbox Data High = 0x0.
- Write GT Driver Mailbox Interface = 0x80000007.

v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
    [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
v3: (By Rodrigo): Fix typo on Paulo's name.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 6030fbafa580..a71a6c396bbd 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -2058,6 +2058,7 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
 				struct intel_shared_dpll *pll)
 {
 	uint32_t val;
+	int ret;
 
 	/*
 	 * 1. Configure DPCLKA_CFGCR0 to turn off the clock for the DDI.
@@ -2067,11 +2068,9 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
 	/*
 	 * 2. If the frequency will result in a change to the voltage
 	 * requirement, follow the Display Voltage Frequency Switching
-	 * Sequence Before Frequency Change
-	 *
-	 * FIXME: (DVFS) is used to adjust the display voltage to match the
-	 * display clock frequencies
+	 * (DVFS) Sequence Before Frequency Change
 	 */
+	ret = cnl_dvfs_pre_change(dev_priv);
 
 	/* 3. Disable DPLL through DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
@@ -2089,11 +2088,10 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
 	/*
 	 * 5. If the frequency will result in a change to the voltage
 	 * requirement, follow the Display Voltage Frequency Switching
-	 * Sequence After Frequency Change
-	 *
-	 * FIXME: (DVFS) is used to adjust the display voltage to match the
-	 * display clock frequencies
+	 * (DVFS) Sequence After Frequency Change
 	 */
+	if (ret == 0)
+		cnl_dvfs_post_change(dev_priv, 0);
 
 	/* 6. Disable DPLL power in DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
-- 
2.13.5

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

[PATCH 09/13] drm/i915/cnl: Invert dvfs default level.

[Rodrigo Vivi]

According to spec "If voltage is set too low,
it will break functionality. If voltage is set too high,
 it will waste power."

So, let's prefer the waste of power instead of breaking
functionality.

But also the logic of deciding the level on spec
tells "Else, use level 2."
So, default is actually "2", not "0".

v2: Rebase moving it up to avoid some temporary code
    duplication.

Cc: Mika Kahola <mika.kahola@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index af8411c2a6b9..7e9c4444c844 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1562,15 +1562,15 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
 	}
 
 	switch (cdclk) {
-	case 528000:
-		pcu_ack = 2;
+	case 168000:
+		pcu_ack = 0;
 		break;
 	case 336000:
 		pcu_ack = 1;
 		break;
-	case 168000:
+	case 528000:
 	default:
-		pcu_ack = 0;
+		pcu_ack = 2;
 		break;
 	}
 
-- 
2.13.5

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

[PATCH 10/13] drm/i915/cnl: Unify dvfs level selection.

[Rodrigo Vivi]

When port clock is zero or undefined we base our
calculation on cdclk. So, same function can be used
for port clock == 0 now that we have the same default "2".

v2: s/get/new: When documenting "get" sounded ambiguous
    because we could be getting the current level at pcode.

Cc: Mika Kahola <mika.kahola@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c    | 28 +++++++++++++---------------
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 12 +-----------
 drivers/gpu/drm/i915/intel_drv.h      |  1 +
 3 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 7e9c4444c844..c62d6e752fb7 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1535,12 +1535,22 @@ void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level)
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
+int cnl_dvfs_new_level(int cdclk, int portclk)
+{
+	if (cdclk == 168000 && portclk <= 594000)
+		return 0;
+	else if (cdclk == 336000 && portclk <= 594000)
+		return 1;
+	else
+		return 2;
+}
+
 static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
 			  const struct intel_cdclk_state *cdclk_state)
 {
 	int cdclk = cdclk_state->cdclk;
 	int vco = cdclk_state->vco;
-	u32 val, divider, pcu_ack;
+	u32 val, divider, level;
 
 	if (cnl_dvfs_pre_change(dev_priv))
 		return;
@@ -1561,19 +1571,6 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
 		break;
 	}
 
-	switch (cdclk) {
-	case 168000:
-		pcu_ack = 0;
-		break;
-	case 336000:
-		pcu_ack = 1;
-		break;
-	case 528000:
-	default:
-		pcu_ack = 2;
-		break;
-	}
-
 	if (dev_priv->cdclk.hw.vco != 0 &&
 	    dev_priv->cdclk.hw.vco != vco)
 		cnl_cdclk_pll_disable(dev_priv);
@@ -1590,7 +1587,8 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
 	I915_WRITE(CDCLK_CTL, val);
 
 	/* inform PCU of the change */
-	cnl_dvfs_post_change(dev_priv, pcu_ack);
+	level = cnl_dvfs_new_level(cdclk, 0);
+	cnl_dvfs_post_change(dev_priv, level);
 
 	intel_update_cdclk(dev_priv);
 }
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index a71a6c396bbd..0dddbd3a7a97 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -1966,16 +1966,6 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
 	.dump_hw_state = bxt_dump_hw_state,
 };
 
-static int cnl_get_dvfs_level(int cdclk, int portclk)
-{
-	if (cdclk == 168000 && portclk <= 594000)
-		return 0;
-	else if (cdclk == 336000 && portclk <= 594000)
-		return 1;
-	else
-		return 2;
-}
-
 static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 			       struct intel_shared_dpll *pll)
 {
@@ -2044,7 +2034,7 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	if (ret == 0) {
 		cdclk = dev_priv->cdclk.hw.cdclk;
 		portclk = intel_ddi_port_clock(dev_priv, pll->id);
-		level = cnl_get_dvfs_level(cdclk, portclk);
+		level = cnl_dvfs_new_level(cdclk, portclk);
 		cnl_dvfs_post_change(dev_priv, level);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 934ccf17f8ab..dec11d7b15ab 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1325,6 +1325,7 @@ void cnl_init_cdclk(struct drm_i915_private *dev_priv);
 void cnl_uninit_cdclk(struct drm_i915_private *dev_priv);
 int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
 void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level);
+int cnl_dvfs_new_level(int cdclk, int portclk);
 void bxt_init_cdclk(struct drm_i915_private *dev_priv);
 void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
 void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
-- 
2.13.5

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

[PATCH 11/13] drm/i915/cnl: Only request voltage frequency switching when needed.

[Rodrigo Vivi]

At cdclk initialization at spec they tell us to always use dvfs
or avoid change cdclk if it fails.

Here on pll sequence, spec tells us to only use dvfs "if the
frequency will result in a change to the voltage requirement."

So in order to respect that and avoid necessary interactions with
PCODE we compare our current needed level with the current set level
and only perform dvfs sequences when we need to change that level.

v2: - Add missing blank line after declaration block
    - s/need/needs: When adding doc needs sounded better.

Cc: Mika Kahola <mika.kahola@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c    | 10 ++++++++++
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 16 +++++++++-------
 drivers/gpu/drm/i915/intel_drv.h      |  1 +
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index c62d6e752fb7..8111a13079e1 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1545,6 +1545,16 @@ int cnl_dvfs_new_level(int cdclk, int portclk)
 		return 2;
 }
 
+bool cnl_dvfs_needs_change(struct drm_i915_private *dev_priv, int level)
+{
+	int old_level;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL, &old_level);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+	return level != old_level;
+}
+
 static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
 			  const struct intel_cdclk_state *cdclk_state)
 {
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 0dddbd3a7a97..85c000891439 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -1970,9 +1970,10 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 			       struct intel_shared_dpll *pll)
 {
 	uint32_t val;
-	int ret;
+	int ret = 0;
 	int level;
 	int cdclk, portclk;
+	bool change_level;
 
 	/* 1. Enable DPLL power in DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
@@ -2011,7 +2012,12 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	 * requirement, follow the Display Voltage Frequency Switching
 	 * (DVFS) Sequence Before Frequency Change
 	 */
-	ret = cnl_dvfs_pre_change(dev_priv);
+	cdclk = dev_priv->cdclk.hw.cdclk;
+	portclk = intel_ddi_port_clock(dev_priv, pll->id);
+	level = cnl_dvfs_new_level(cdclk, portclk);
+	change_level = cnl_dvfs_needs_change(dev_priv, level);
+	if (change_level)
+		ret = cnl_dvfs_pre_change(dev_priv);
 
 	/* 6. Enable DPLL in DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
@@ -2031,12 +2037,8 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	 * requirement, follow the Display Voltage Frequency Switching
 	 * (DVFS) Sequence After Frequency Change
 	 */
-	if (ret == 0) {
-		cdclk = dev_priv->cdclk.hw.cdclk;
-		portclk = intel_ddi_port_clock(dev_priv, pll->id);
-		level = cnl_dvfs_new_level(cdclk, portclk);
+	if (change_level && ret == 0)
 		cnl_dvfs_post_change(dev_priv, level);
-	}
 
 	/*
 	 * 9. turn on the clock for the DDI and map the DPLL to the DDI
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dec11d7b15ab..8621110401e3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1326,6 +1326,7 @@ void cnl_uninit_cdclk(struct drm_i915_private *dev_priv);
 int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
 void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level);
 int cnl_dvfs_new_level(int cdclk, int portclk);
+bool cnl_dvfs_needs_change(struct drm_i915_private *dev_priv, int level);
 void bxt_init_cdclk(struct drm_i915_private *dev_priv);
 void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
 void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
-- 
2.13.5

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

[PATCH 12/13] drm/i915/cnl: When disabling pll put dvfs back to cdclk requirement.

[Rodrigo Vivi]

Spec tells us to change the level "if the frequency will result
in a change to the voltage requirement."

When we don't have pll enabled yet we only base our level
calculation on cdclk. So let's do the same when disabling the
pll instead of forcing randomly to zero.

v2: Rebase.

Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 85c000891439..4eb1be91a669 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -2050,7 +2050,9 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
 				struct intel_shared_dpll *pll)
 {
 	uint32_t val;
-	int ret;
+	int ret = 0;
+	int cdclk, level;
+	bool change_level;
 
 	/*
 	 * 1. Configure DPCLKA_CFGCR0 to turn off the clock for the DDI.
@@ -2062,7 +2064,11 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
 	 * requirement, follow the Display Voltage Frequency Switching
 	 * (DVFS) Sequence Before Frequency Change
 	 */
-	ret = cnl_dvfs_pre_change(dev_priv);
+	cdclk = dev_priv->cdclk.hw.cdclk;
+	level = cnl_dvfs_new_level(cdclk, 0);
+	change_level = cnl_dvfs_needs_change(dev_priv, level);
+	if (change_level)
+		ret = cnl_dvfs_pre_change(dev_priv);
 
 	/* 3. Disable DPLL through DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
@@ -2082,8 +2088,8 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
 	 * requirement, follow the Display Voltage Frequency Switching
 	 * (DVFS) Sequence After Frequency Change
 	 */
-	if (ret == 0)
-		cnl_dvfs_post_change(dev_priv, 0);
+	if (change_level && ret == 0)
+		cnl_dvfs_post_change(dev_priv, level);
 
 	/* 6. Disable DPLL power in DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
-- 
2.13.5

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

[PATCH 13/13] drm/i915: Make DVFS more generic and document them.

[Rodrigo Vivi]

Before extending this to SKL let's document DVFS a bit.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c    | 55 ++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 16 +++++-----
 drivers/gpu/drm/i915/intel_drv.h      |  8 ++---
 3 files changed, 60 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 8111a13079e1..58ee4dd07cf6 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1510,7 +1510,22 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
 	dev_priv->cdclk.hw.vco = vco;
 }
 
-int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
+/**
+ * DOC: Display Voltage and Frequency Switching (DVFS)
+ *
+ * DVFS is used to adjust the display voltage to match the display clock
+ * frequencies. To save power the voltage is set to minimum when disabling PLL.
+ *
+ */
+
+/**
+ * intel_dvfs_pre_change - (DVFS) Sequence Before Frequency Change
+ * @dev_priv: i915 device
+ *
+ * The sequence before frequency change is the following and it requests
+ * the power controller to raise voltage to maximum
+ */
+int intel_dvfs_pre_change(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
@@ -1528,14 +1543,31 @@ int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level)
+/**
+ * intel_dvfs_post_change - (DVFS) Sequence After Frequency Change
+ * @dev_priv: i915 device
+ * @level: New desired voltage-frequency level.
+ *
+ * The sequence after frequency change is the following and it requests
+ * the port controller to lower voltage to the minimum.
+ */
+void intel_dvfs_post_change(struct drm_i915_private *dev_priv, int level)
 {
 	mutex_lock(&dev_priv->rps.hw_lock);
 	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, level);
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
-int cnl_dvfs_new_level(int cdclk, int portclk)
+/**
+ * intel_dvfs_new_level - DVFS get new level.
+ * @cdclk: CD Clock.
+ * @portclk: Link rate. Optional argument. When 0, only cdclk is enough.
+ *
+ * Returns:
+ * New required level that should be informed to pcode
+ * based on @cdclk and on @portclk, if available.
+ */
+int intel_dvfs_new_level(int cdclk, int portclk)
 {
 	if (cdclk == 168000 && portclk <= 594000)
 		return 0;
@@ -1545,7 +1577,16 @@ int cnl_dvfs_new_level(int cdclk, int portclk)
 		return 2;
 }
 
-bool cnl_dvfs_needs_change(struct drm_i915_private *dev_priv, int level)
+/**
+ * intel_dvfs_needs_change - DVFS needs change.
+ * @dev_priv: i915 device
+ * @level: New desired voltage-frequency level.
+ *
+ * Returns:
+ * True if current level known by pcode is different of the
+ * new desired one.
+ */
+bool intel_dvfs_needs_change(struct drm_i915_private *dev_priv, int level)
 {
 	int old_level;
 
@@ -1562,7 +1603,7 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
 	int vco = cdclk_state->vco;
 	u32 val, divider, level;
 
-	if (cnl_dvfs_pre_change(dev_priv))
+	if (intel_dvfs_pre_change(dev_priv))
 		return;
 
 	/* cdclk = vco / 2 / div{1,2} */
@@ -1597,8 +1638,8 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
 	I915_WRITE(CDCLK_CTL, val);
 
 	/* inform PCU of the change */
-	level = cnl_dvfs_new_level(cdclk, 0);
-	cnl_dvfs_post_change(dev_priv, level);
+	level = intel_dvfs_new_level(cdclk, 0);
+	intel_dvfs_post_change(dev_priv, level);
 
 	intel_update_cdclk(dev_priv);
 }
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 4eb1be91a669..55997389a29f 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -2014,10 +2014,10 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	 */
 	cdclk = dev_priv->cdclk.hw.cdclk;
 	portclk = intel_ddi_port_clock(dev_priv, pll->id);
-	level = cnl_dvfs_new_level(cdclk, portclk);
-	change_level = cnl_dvfs_needs_change(dev_priv, level);
+	level = intel_dvfs_new_level(cdclk, portclk);
+	change_level = intel_dvfs_needs_change(dev_priv, level);
 	if (change_level)
-		ret = cnl_dvfs_pre_change(dev_priv);
+		ret = intel_dvfs_pre_change(dev_priv);
 
 	/* 6. Enable DPLL in DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
@@ -2038,7 +2038,7 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	 * (DVFS) Sequence After Frequency Change
 	 */
 	if (change_level && ret == 0)
-		cnl_dvfs_post_change(dev_priv, level);
+		intel_dvfs_post_change(dev_priv, level);
 
 	/*
 	 * 9. turn on the clock for the DDI and map the DPLL to the DDI
@@ -2065,10 +2065,10 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
 	 * (DVFS) Sequence Before Frequency Change
 	 */
 	cdclk = dev_priv->cdclk.hw.cdclk;
-	level = cnl_dvfs_new_level(cdclk, 0);
-	change_level = cnl_dvfs_needs_change(dev_priv, level);
+	level = intel_dvfs_new_level(cdclk, 0);
+	change_level = intel_dvfs_needs_change(dev_priv, level);
 	if (change_level)
-		ret = cnl_dvfs_pre_change(dev_priv);
+		ret = intel_dvfs_pre_change(dev_priv);
 
 	/* 3. Disable DPLL through DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
@@ -2089,7 +2089,7 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
 	 * (DVFS) Sequence After Frequency Change
 	 */
 	if (change_level && ret == 0)
-		cnl_dvfs_post_change(dev_priv, level);
+		intel_dvfs_post_change(dev_priv, level);
 
 	/* 6. Disable DPLL power in DPLL_ENABLE. */
 	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8621110401e3..567b6e846a69 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1323,10 +1323,10 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv);
 void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
 void cnl_init_cdclk(struct drm_i915_private *dev_priv);
 void cnl_uninit_cdclk(struct drm_i915_private *dev_priv);
-int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
-void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level);
-int cnl_dvfs_new_level(int cdclk, int portclk);
-bool cnl_dvfs_needs_change(struct drm_i915_private *dev_priv, int level);
+int intel_dvfs_pre_change(struct drm_i915_private *dev_priv);
+void intel_dvfs_post_change(struct drm_i915_private *dev_priv, int level);
+int intel_dvfs_new_level(int cdclk, int portclk);
+bool intel_dvfs_needs_change(struct drm_i915_private *dev_priv, int level);
 void bxt_init_cdclk(struct drm_i915_private *dev_priv);
 void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
 void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
-- 
2.13.5

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

✓ Fi.CI.BAT: success for DVFS v2

[Patchwork]

== Series Details ==

Series: DVFS v2
URL   : https://patchwork.freedesktop.org/series/31305/
State : success

== Summary ==

Series 31305v1 DVFS v2
https://patchwork.freedesktop.org/api/1.0/series/31305/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-j1900) fdo#101705

fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:463s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:476s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:393s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:575s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:288s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:527s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:542s
fi-byt-j1900     total:289  pass:255  dwarn:0   dfail:0   fail:0   skip:34  time:546s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:522s
fi-cfl-s         total:289  pass:256  dwarn:1   dfail:0   fail:0   skip:32  time:564s
fi-cnl-y         total:289  pass:261  dwarn:1   dfail:0   fail:0   skip:27  time:608s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:437s
fi-glk-1         total:289  pass:258  dwarn:3   dfail:0   fail:0   skip:28  time:594s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:438s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:417s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:461s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:509s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:481s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:497s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:576s
fi-kbl-7567u     total:289  pass:265  dwarn:4   dfail:0   fail:0   skip:20  time:491s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:588s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:662s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:478s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:536s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:513s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:480s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:583s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:430s

14f3207683e165f826d0e5861944fc6f39e8e20f drm-tip: 2017y-10m-02d-20h-23m-38s UTC integration manifest
81970172d010 drm/i915: Make DVFS more generic and document them.
528de806792f drm/i915/cnl: When disabling pll put dvfs back to cdclk requirement.
48724aeb5fe0 drm/i915/cnl: Only request voltage frequency switching when needed.
3fe6bdf64a86 drm/i915/cnl: Unify dvfs level selection.
7dfe8070c792 drm/i915/cnl: Invert dvfs default level.
fc81405bc60d drm/i915/cnl: DVFS for PLL disabling
9125f204b5fd drm/i915/cnl: DVFS for PLL enabling
acffc9e24174 drm/i915/cnl: Expose DVFS change functions
749f88976985 drm/i915/cnl: extract cnl_dvfs_{pre, post}_change
5e8a2d60c7bb drm/i915: Unify and export gen9+ port_clock calculation.
d891284af1b8 drm/i915/skl: Extract skl_calc_pll_link following bxt, cnl style.
e7d0e3961889 drm/i915/cnl: Extract cnl_calc_pll_link following bxt style.
72371e0a619f drm/i915: Let's use more enum intel_dpll_id pll_id.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5875/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for DVFS v2

[Patchwork]

== Series Details ==

Series: DVFS v2
URL   : https://patchwork.freedesktop.org/series/31305/
State : warning

== Summary ==

Test kms_flip:
        Subgroup wf_vblank-vs-dpms-interruptible:
                dmesg-warn -> PASS       (shard-hsw) fdo#102614
        Subgroup rcs-wf_vblank-vs-modeset-interruptible:
                pass       -> DMESG-WARN (shard-hsw)
Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252

fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-hsw        total:2429 pass:1329 dwarn:8   dfail:0   fail:9   skip:1083 time:10061s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5875/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/13] drm/i915: Let's use more enum intel_dpll_id pll_id.

[Mika Kahola]

On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> No functional change expected. Just let's use this enum
> when possible and also same standard pll_id name
> so we can rework gen9+ port clock later.
> 
> Cc: Mika Kahola <mika.kahola@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 34 ++++++++++++++++++----------
> ------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 93cbbcbbc193..b5dd82a0e357 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1102,14 +1102,14 @@ static int hsw_ddi_calc_wrpll_link(struct
> drm_i915_private *dev_priv,
>  }
>  
>  static int skl_calc_wrpll_link(struct drm_i915_private *dev_priv,
> -			       uint32_t dpll)
> +			       enum intel_dpll_id pll_id)
>  {
>  	i915_reg_t cfgcr1_reg, cfgcr2_reg;
>  	uint32_t cfgcr1_val, cfgcr2_val;
>  	uint32_t p0, p1, p2, dco_freq;
>  
> -	cfgcr1_reg = DPLL_CFGCR1(dpll);
> -	cfgcr2_reg = DPLL_CFGCR2(dpll);
> +	cfgcr1_reg = DPLL_CFGCR1(pll_id);
> +	cfgcr2_reg = DPLL_CFGCR2(pll_id);
>  
>  	cfgcr1_val = I915_READ(cfgcr1_reg);
>  	cfgcr2_val = I915_READ(cfgcr2_reg);
> @@ -1162,7 +1162,7 @@ static int skl_calc_wrpll_link(struct
> drm_i915_private *dev_priv,
>  }
>  
>  static int cnl_calc_wrpll_link(struct drm_i915_private *dev_priv,
> -			       uint32_t pll_id)
> +			       enum intel_dpll_id pll_id)
>  {
>  	uint32_t cfgcr0, cfgcr1;
>  	uint32_t p0, p1, p2, dco_freq, ref_clock;
> @@ -1246,7 +1246,8 @@ static void cnl_ddi_clock_get(struct
> intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
>  	int link_clock = 0;
> -	uint32_t cfgcr0, pll_id;
> +	uint32_t cfgcr0;
> +	enum intel_dpll_id pll_id;
>  
>  	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> >shared_dpll);
>  
> @@ -1299,17 +1300,18 @@ static void skl_ddi_clock_get(struct
> intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
>  	int link_clock = 0;
> -	uint32_t dpll_ctl1, dpll;
> +	uint32_t dpll_ctl1;
> +	enum intel_dpll_id pll_id;
>  
> -	dpll = intel_get_shared_dpll_id(dev_priv, pipe_config-
> >shared_dpll);
> +	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> >shared_dpll);
>  
>  	dpll_ctl1 = I915_READ(DPLL_CTRL1);
>  
> -	if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(dpll)) {
> -		link_clock = skl_calc_wrpll_link(dev_priv, dpll);
> +	if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(pll_id)) {
> +		link_clock = skl_calc_wrpll_link(dev_priv, pll_id);
>  	} else {
> -		link_clock = dpll_ctl1 &
> DPLL_CTRL1_LINK_RATE_MASK(dpll);
> -		link_clock >>= DPLL_CTRL1_LINK_RATE_SHIFT(dpll);
> +		link_clock = dpll_ctl1 &
> DPLL_CTRL1_LINK_RATE_MASK(pll_id);
> +		link_clock >>= DPLL_CTRL1_LINK_RATE_SHIFT(pll_id);
>  
>  		switch (link_clock) {
>  		case DPLL_CTRL1_LINK_RATE_810:
> @@ -1390,17 +1392,17 @@ static void hsw_ddi_clock_get(struct
> intel_encoder *encoder,
>  }
>  
>  static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
> -				enum intel_dpll_id dpll)
> +			     enum intel_dpll_id pll_id)
>  {
>  	struct intel_shared_dpll *pll;
>  	struct intel_dpll_hw_state *state;
>  	struct dpll clock;
>  
>  	/* For DDI ports we always use a shared PLL. */
> -	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
> +	if (WARN_ON(pll_id == DPLL_ID_PRIVATE))
>  		return 0;
>  
> -	pll = &dev_priv->shared_dplls[dpll];
> +	pll = &dev_priv->shared_dplls[pll_id];
>  	state = &pll->state.hw_state;
>  
>  	clock.m1 = 2;
> @@ -1419,9 +1421,9 @@ static void bxt_ddi_clock_get(struct
> intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
>  	enum port port = intel_ddi_get_encoder_port(encoder);
> -	uint32_t dpll = port;
> +	enum intel_dpll_id pll_id = port;
>  
> -	pipe_config->port_clock = bxt_calc_pll_link(dev_priv, dpll);
> +	pipe_config->port_clock = bxt_calc_pll_link(dev_priv,
> pll_id);
>  
>  	ddi_dotclock_get(pipe_config);
>  }
-- 
Mika Kahola - Intel OTC

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

Re: [PATCH 02/13] drm/i915/cnl: Extract cnl_calc_pll_link following bxt style.

[Mika Kahola]

On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> No functional change. Just spliting the function for
> better port clock handling later.
> 
> v2: Put link_clock *=2 inside the function only for DP,
>     otherwise we mess up clocks on HDMI. (Caught by CI).
> 
> Cc: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index b5dd82a0e357..71040c3dd6fc 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1241,15 +1241,11 @@ static void ddi_dotclock_get(struct
> intel_crtc_state *pipe_config)
>  	pipe_config->base.adjusted_mode.crtc_clock = dotclock;
>  }
>  
> -static void cnl_ddi_clock_get(struct intel_encoder *encoder,
> -			      struct intel_crtc_state *pipe_config)
> +static int cnl_calc_pll_link(struct drm_i915_private *dev_priv,
> +			     enum intel_dpll_id pll_id)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
>  	int link_clock = 0;
>  	uint32_t cfgcr0;
> -	enum intel_dpll_id pll_id;
> -
> -	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> >shared_dpll);
>  
>  	cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));
>  
> @@ -1290,7 +1286,18 @@ static void cnl_ddi_clock_get(struct
> intel_encoder *encoder,
>  		link_clock *= 2;
>  	}
>  
> -	pipe_config->port_clock = link_clock;
> +	return link_clock;
> +}
> +
> +static void cnl_ddi_clock_get(struct intel_encoder *encoder,
> +			      struct intel_crtc_state *pipe_config)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
> +	enum intel_dpll_id pll_id;
> +
> +	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> >shared_dpll);
> +
> +	pipe_config->port_clock = cnl_calc_pll_link(dev_priv,
> pll_id);
>  
>  	ddi_dotclock_get(pipe_config);
>  }
-- 
Mika Kahola - Intel OTC

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

Re: [PATCH 03/13] drm/i915/skl: Extract skl_calc_pll_link following bxt, cnl style.

[Mika Kahola]

On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> No functional change. Just spliting the function for
> better port clock handling later.
> 
> v2: Fix subject and also put link_clock *= 2 back inside
>     the new function so we only multiply for DP and avoid
>     messing up the HDMI clocks. (Caught by CI).
> 
> Cc: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 71040c3dd6fc..92eabb6cc1ab 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1302,15 +1302,11 @@ static void cnl_ddi_clock_get(struct
> intel_encoder *encoder,
>  	ddi_dotclock_get(pipe_config);
>  }
>  
> -static void skl_ddi_clock_get(struct intel_encoder *encoder,
> -				struct intel_crtc_state
> *pipe_config)
> +static int skl_calc_pll_link(struct drm_i915_private *dev_priv,
> +			     enum intel_dpll_id pll_id)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
>  	int link_clock = 0;
>  	uint32_t dpll_ctl1;
> -	enum intel_dpll_id pll_id;
> -
> -	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> >shared_dpll);
>  
>  	dpll_ctl1 = I915_READ(DPLL_CTRL1);
>  
> @@ -1346,7 +1342,18 @@ static void skl_ddi_clock_get(struct
> intel_encoder *encoder,
>  		link_clock *= 2;
>  	}
>  
> -	pipe_config->port_clock = link_clock;
> +	return link_clock;
> +}
> +
> +static void skl_ddi_clock_get(struct intel_encoder *encoder,
> +			      struct intel_crtc_state *pipe_config)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
> +	enum intel_dpll_id pll_id;
> +
> +	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> >shared_dpll);
> +
> +	pipe_config->port_clock = skl_calc_pll_link(dev_priv,
> pll_id);
>  
>  	ddi_dotclock_get(pipe_config);
>  }
-- 
Mika Kahola - Intel OTC

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

✓ Fi.CI.BAT: success for DVFS v2

[Patchwork]

== Series Details ==

Series: DVFS v2
URL   : https://patchwork.freedesktop.org/series/31305/
State : success

== Summary ==

Series 31305v1 DVFS v2
https://patchwork.freedesktop.org/api/1.0/series/31305/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                pass       -> FAIL       (fi-kbl-7500u) fdo#102514

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:459s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:474s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:391s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:580s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:287s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:525s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:534s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:548s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:542s
fi-cfl-s         total:289  pass:256  dwarn:1   dfail:0   fail:0   skip:32  time:554s
fi-cnl-y         total:289  pass:261  dwarn:1   dfail:0   fail:0   skip:27  time:635s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:440s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:602s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:443s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:422s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:478s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:501s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:475s
fi-kbl-7500u     total:289  pass:263  dwarn:1   dfail:0   fail:1   skip:24  time:497s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:580s
fi-kbl-7567u     total:289  pass:265  dwarn:4   dfail:0   fail:0   skip:20  time:495s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:593s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:655s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:473s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:539s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:515s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:469s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:591s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:435s

654360cf73feea27f7ed5bfa2e5b2fa5ede2e8ec drm-tip: 2017y-10m-03d-17h-55m-08s UTC integration manifest
7591d8ba19b2 drm/i915: Make DVFS more generic and document them.
a14661ddc36c drm/i915/cnl: When disabling pll put dvfs back to cdclk requirement.
d1e746f1a012 drm/i915/cnl: Only request voltage frequency switching when needed.
4cdec21e68d4 drm/i915/cnl: Unify dvfs level selection.
a0889ddfbbd0 drm/i915/cnl: Invert dvfs default level.
28a003f8593c drm/i915/cnl: DVFS for PLL disabling
d2afef1e0afb drm/i915/cnl: DVFS for PLL enabling
606856932ecc drm/i915/cnl: Expose DVFS change functions
59e5cd287048 drm/i915/cnl: extract cnl_dvfs_{pre, post}_change
6c807b506a37 drm/i915: Unify and export gen9+ port_clock calculation.
3764f058b99f drm/i915/skl: Extract skl_calc_pll_link following bxt, cnl style.
6b3e7fc3549a drm/i915/cnl: Extract cnl_calc_pll_link following bxt style.
03f2869b4f14 drm/i915: Let's use more enum intel_dpll_id pll_id.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5886/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/13] drm/i915: Unify and export gen9+ port_clock calculation.

[Mika Kahola]

On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> On Cannonlake the DVFS level selection depends on the
> port clock.
> 
> So let's re-org in a way that we can easily export without
> duplicating any code.
> 
> v2: Rebased on changes on previous patches
> 
> Cc: Mika Kahola <mika.kahola@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 21 ++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 92eabb6cc1ab..ee64b1a50453 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1297,7 +1297,7 @@ static void cnl_ddi_clock_get(struct
> intel_encoder *encoder,
>  
>  	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> >shared_dpll);
>  
> -	pipe_config->port_clock = cnl_calc_pll_link(dev_priv,
> pll_id);
> +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> pll_id);
>  
>  	ddi_dotclock_get(pipe_config);
>  }
> @@ -1353,7 +1353,7 @@ static void skl_ddi_clock_get(struct
> intel_encoder *encoder,
>  
>  	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> >shared_dpll);
>  
> -	pipe_config->port_clock = skl_calc_pll_link(dev_priv,
> pll_id);
> +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> pll_id);
>  
>  	ddi_dotclock_get(pipe_config);
>  }
> @@ -1437,11 +1437,26 @@ static void bxt_ddi_clock_get(struct
> intel_encoder *encoder,
>  	enum port port = intel_ddi_get_encoder_port(encoder);
>  	enum intel_dpll_id pll_id = port;
>  
> -	pipe_config->port_clock = bxt_calc_pll_link(dev_priv,
> pll_id);
> +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> pll_id);
>  
>  	ddi_dotclock_get(pipe_config);
>  }
>  
> +int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
> +			 enum intel_dpll_id pll_id)
> +{
> +	if (IS_GEN9_BC(dev_priv)) {
> +		return skl_calc_pll_link(dev_priv, pll_id);
> +	} else if (IS_GEN9_LP(dev_priv)) {
> +		return bxt_calc_pll_link(dev_priv, pll_id);
> +	} else if (IS_CANNONLAKE(dev_priv)) {
> +		return cnl_calc_pll_link(dev_priv, pll_id);
> +	} else {
> +		MISSING_CASE(INTEL_GEN(dev_priv));
> +		return 0;
> +	}
> +}
Personally, I feel that we wouldn't need to unify this. We call
platform specific functions from this intel_ddi_port_clock() and this
is called from platform specific *_ddi_clock_get() routine. Why not
just keep all platform specific functions in one place?

Paolo, any opinions on this one?

> +
>  void intel_ddi_clock_get(struct intel_encoder *encoder,
>  			 struct intel_crtc_state *pipe_config)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 0cab667fff57..fe4650d6db03 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1293,7 +1293,8 @@ bool intel_ddi_is_audio_enabled(struct
> drm_i915_private *dev_priv,
>  				 struct intel_crtc *intel_crtc);
>  void intel_ddi_get_config(struct intel_encoder *encoder,
>  			  struct intel_crtc_state *pipe_config);
> -
> +int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
> +			 enum intel_dpll_id pll_id);
>  void intel_ddi_clock_get(struct intel_encoder *encoder,
>  			 struct intel_crtc_state *pipe_config);
>  void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state
> *crtc_state,
-- 
Mika Kahola - Intel OTC

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

Re: [PATCH 09/13] drm/i915/cnl: Invert dvfs default level.

[Mika Kahola]

On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> According to spec "If voltage is set too low,
> it will break functionality. If voltage is set too high,
>  it will waste power."
> 
> So, let's prefer the waste of power instead of breaking
> functionality.
> 
> But also the logic of deciding the level on spec
> tells "Else, use level 2."
> So, default is actually "2", not "0".
The spec also says

"If CD clock = 168 MHz AND DDI clock ≤ 594 MHz, use level 0.
Else If CD clock = 336 MHz AND DDI clock ≤ 594 MHz, use level 1.
Else, use level 2."

Should we add check for DDI clock rate as well here?

> 
> v2: Rebase moving it up to avoid some temporary code
>     duplication.
> 
> Cc: Mika Kahola <mika.kahola@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index af8411c2a6b9..7e9c4444c844 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1562,15 +1562,15 @@ static void cnl_set_cdclk(struct
> drm_i915_private *dev_priv,
>  	}
>  
>  	switch (cdclk) {
> -	case 528000:
> -		pcu_ack = 2;
> +	case 168000:
> +		pcu_ack = 0;
>  		break;
>  	case 336000:
>  		pcu_ack = 1;
>  		break;
> -	case 168000:
> +	case 528000:
>  	default:
> -		pcu_ack = 0;
> +		pcu_ack = 2;
>  		break;
>  	}
>  
-- 
Mika Kahola - Intel OTC

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

Re: [PATCH 10/13] drm/i915/cnl: Unify dvfs level selection.

[Mika Kahola]

On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> When port clock is zero or undefined we base our
> calculation on cdclk. So, same function can be used
> for port clock == 0 now that we have the same default "2".
> 
> v2: s/get/new: When documenting "get" sounded ambiguous
>     because we could be getting the current level at pcode.
> 
> Cc: Mika Kahola <mika.kahola@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c    | 28 +++++++++++++----------
> -----
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 12 +-----------
>  drivers/gpu/drm/i915/intel_drv.h      |  1 +
>  3 files changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index 7e9c4444c844..c62d6e752fb7 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1535,12 +1535,22 @@ void cnl_dvfs_post_change(struct
> drm_i915_private *dev_priv, int level)
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> +int cnl_dvfs_new_level(int cdclk, int portclk)
> +{
> +	if (cdclk == 168000 && portclk <= 594000)
> +		return 0;
> +	else if (cdclk == 336000 && portclk <= 594000)
> +		return 1;
> +	else
> +		return 2;
> +}
> +
Maybe we could leave the function name as cnl_get_dvfs_level()?
The original one is stripped down later in this patch.
 
>  static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
>  			  const struct intel_cdclk_state
> *cdclk_state)
>  {
>  	int cdclk = cdclk_state->cdclk;
>  	int vco = cdclk_state->vco;
> -	u32 val, divider, pcu_ack;
> +	u32 val, divider, level;
>  
>  	if (cnl_dvfs_pre_change(dev_priv))
>  		return;
> @@ -1561,19 +1571,6 @@ static void cnl_set_cdclk(struct
> drm_i915_private *dev_priv,
>  		break;
>  	}
>  
> -	switch (cdclk) {
> -	case 168000:
> -		pcu_ack = 0;
> -		break;
> -	case 336000:
> -		pcu_ack = 1;
> -		break;
> -	case 528000:
> -	default:
> -		pcu_ack = 2;
> -		break;
> -	}
> -
>  	if (dev_priv->cdclk.hw.vco != 0 &&
>  	    dev_priv->cdclk.hw.vco != vco)
>  		cnl_cdclk_pll_disable(dev_priv);
> @@ -1590,7 +1587,8 @@ static void cnl_set_cdclk(struct
> drm_i915_private *dev_priv,
>  	I915_WRITE(CDCLK_CTL, val);
>  
>  	/* inform PCU of the change */
> -	cnl_dvfs_post_change(dev_priv, pcu_ack);
> +	level = cnl_dvfs_new_level(cdclk, 0);
> +	cnl_dvfs_post_change(dev_priv, level);
>  
>  	intel_update_cdclk(dev_priv);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index a71a6c396bbd..0dddbd3a7a97 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1966,16 +1966,6 @@ static const struct intel_dpll_mgr bxt_pll_mgr
> = {
>  	.dump_hw_state = bxt_dump_hw_state,
>  };
>  
> -static int cnl_get_dvfs_level(int cdclk, int portclk)
> -{
> -	if (cdclk == 168000 && portclk <= 594000)
> -		return 0;
> -	else if (cdclk == 336000 && portclk <= 594000)
> -		return 1;
> -	else
> -		return 2;
> -}
> -
>  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  			       struct intel_shared_dpll *pll)
>  {
> @@ -2044,7 +2034,7 @@ static void cnl_ddi_pll_enable(struct
> drm_i915_private *dev_priv,
>  	if (ret == 0) {
>  		cdclk = dev_priv->cdclk.hw.cdclk;
>  		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> -		level = cnl_get_dvfs_level(cdclk, portclk);
> +		level = cnl_dvfs_new_level(cdclk, portclk);
>  		cnl_dvfs_post_change(dev_priv, level);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 934ccf17f8ab..dec11d7b15ab 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1325,6 +1325,7 @@ void cnl_init_cdclk(struct drm_i915_private
> *dev_priv);
>  void cnl_uninit_cdclk(struct drm_i915_private *dev_priv);
>  int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
>  void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int
> level);
> +int cnl_dvfs_new_level(int cdclk, int portclk);
>  void bxt_init_cdclk(struct drm_i915_private *dev_priv);
>  void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
>  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
-- 
Mika Kahola - Intel OTC

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

Re: [PATCH 09/13] drm/i915/cnl: Invert dvfs default level.

[Rodrigo Vivi]

On Wed, Oct 04, 2017 at 09:46:41AM +0000, Mika Kahola wrote:
> On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > According to spec "If voltage is set too low,
> > it will break functionality. If voltage is set too high,
> >  it will waste power."
> > 
> > So, let's prefer the waste of power instead of breaking
> > functionality.
> > 
> > But also the logic of deciding the level on spec
> > tells "Else, use level 2."
> > So, default is actually "2", not "0".
> The spec also says
> 
> "If CD clock = 168 MHz AND DDI clock ≤ 594 MHz, use level 0.
> Else If CD clock = 336 MHz AND DDI clock ≤ 594 MHz, use level 1.
> Else, use level 2."
> 
> Should we add check for DDI clock rate as well here?

By this time dpll are disabled and not associated to any
port yet. So portclock is 0. So if we invert the default
we do respect the same algorightm you pasted.
Also if you notice I'm just inverting this in a separeted
patch to make it clear and if needed to bisect we end up
on this single point of change. But on a later patch on
this series I change to use your function with this algoright
there, but giving portclock = 0.

So by the end of this series the flow is:

- enable cdclk
- request dvfs level enough for cdclk in question.
- enable pll
- adjust dvfs level only if needed taking the port clock into account.
- disable pll
- put back level to cdclock level if needed.

> 
> > 
> > v2: Rebase moving it up to avoid some temporary code
> >     duplication.
> > 
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index af8411c2a6b9..7e9c4444c844 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1562,15 +1562,15 @@ static void cnl_set_cdclk(struct
> > drm_i915_private *dev_priv,
> >  	}
> >  
> >  	switch (cdclk) {
> > -	case 528000:
> > -		pcu_ack = 2;
> > +	case 168000:
> > +		pcu_ack = 0;
> >  		break;
> >  	case 336000:
> >  		pcu_ack = 1;
> >  		break;
> > -	case 168000:
> > +	case 528000:
> >  	default:
> > -		pcu_ack = 0;
> > +		pcu_ack = 2;
> >  		break;
> >  	}
> >  
> -- 
> Mika Kahola - Intel OTC
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/13] drm/i915: Unify and export gen9+ port_clock calculation.

[Rodrigo Vivi]

On Wed, Oct 04, 2017 at 06:39:19AM +0000, Mika Kahola wrote:
> On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > On Cannonlake the DVFS level selection depends on the
> > port clock.
> > 
> > So let's re-org in a way that we can easily export without
> > duplicating any code.
> > 
> > v2: Rebased on changes on previous patches
> > 
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 21 ++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
> >  2 files changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 92eabb6cc1ab..ee64b1a50453 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1297,7 +1297,7 @@ static void cnl_ddi_clock_get(struct
> > intel_encoder *encoder,
> >  
> >  	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> > >shared_dpll);
> >  
> > -	pipe_config->port_clock = cnl_calc_pll_link(dev_priv,
> > pll_id);
> > +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> > pll_id);
> >  
> >  	ddi_dotclock_get(pipe_config);
> >  }
> > @@ -1353,7 +1353,7 @@ static void skl_ddi_clock_get(struct
> > intel_encoder *encoder,
> >  
> >  	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> > >shared_dpll);
> >  
> > -	pipe_config->port_clock = skl_calc_pll_link(dev_priv,
> > pll_id);
> > +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> > pll_id);
> >  
> >  	ddi_dotclock_get(pipe_config);
> >  }
> > @@ -1437,11 +1437,26 @@ static void bxt_ddi_clock_get(struct
> > intel_encoder *encoder,
> >  	enum port port = intel_ddi_get_encoder_port(encoder);
> >  	enum intel_dpll_id pll_id = port;
> >  
> > -	pipe_config->port_clock = bxt_calc_pll_link(dev_priv,
> > pll_id);
> > +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> > pll_id);
> >  
> >  	ddi_dotclock_get(pipe_config);
> >  }
> >  
> > +int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
> > +			 enum intel_dpll_id pll_id)
> > +{
> > +	if (IS_GEN9_BC(dev_priv)) {
> > +		return skl_calc_pll_link(dev_priv, pll_id);
> > +	} else if (IS_GEN9_LP(dev_priv)) {
> > +		return bxt_calc_pll_link(dev_priv, pll_id);
> > +	} else if (IS_CANNONLAKE(dev_priv)) {
> > +		return cnl_calc_pll_link(dev_priv, pll_id);
> > +	} else {
> > +		MISSING_CASE(INTEL_GEN(dev_priv));
> > +		return 0;
> > +	}
> > +}
> Personally, I feel that we wouldn't need to unify this. We call
> platform specific functions from this intel_ddi_port_clock() and this
> is called from platform specific *_ddi_clock_get() routine. Why not
> just keep all platform specific functions in one place?

Yeap, it is indeed silly to unify since we are just calling it
from the specific functions.

Let's drop this patch ;)

> 
> Paolo, any opinions on this one?
> 
> > +
> >  void intel_ddi_clock_get(struct intel_encoder *encoder,
> >  			 struct intel_crtc_state *pipe_config)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 0cab667fff57..fe4650d6db03 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1293,7 +1293,8 @@ bool intel_ddi_is_audio_enabled(struct
> > drm_i915_private *dev_priv,
> >  				 struct intel_crtc *intel_crtc);
> >  void intel_ddi_get_config(struct intel_encoder *encoder,
> >  			  struct intel_crtc_state *pipe_config);
> > -
> > +int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
> > +			 enum intel_dpll_id pll_id);
> >  void intel_ddi_clock_get(struct intel_encoder *encoder,
> >  			 struct intel_crtc_state *pipe_config);
> >  void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state
> > *crtc_state,
> -- 
> Mika Kahola - Intel OTC
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/13] drm/i915: Unify and export gen9+ port_clock calculation.

[Rodrigo Vivi]

On Wed, Oct 04, 2017 at 07:38:16PM +0000, Rodrigo Vivi wrote:
> On Wed, Oct 04, 2017 at 06:39:19AM +0000, Mika Kahola wrote:
> > On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > > On Cannonlake the DVFS level selection depends on the
> > > port clock.
> > > 
> > > So let's re-org in a way that we can easily export without
> > > duplicating any code.
> > > 
> > > v2: Rebased on changes on previous patches
> > > 
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c | 21 ++++++++++++++++++---
> > >  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
> > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 92eabb6cc1ab..ee64b1a50453 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1297,7 +1297,7 @@ static void cnl_ddi_clock_get(struct
> > > intel_encoder *encoder,
> > >  
> > >  	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> > > >shared_dpll);
> > >  
> > > -	pipe_config->port_clock = cnl_calc_pll_link(dev_priv,
> > > pll_id);
> > > +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> > > pll_id);
> > >  
> > >  	ddi_dotclock_get(pipe_config);
> > >  }
> > > @@ -1353,7 +1353,7 @@ static void skl_ddi_clock_get(struct
> > > intel_encoder *encoder,
> > >  
> > >  	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> > > >shared_dpll);
> > >  
> > > -	pipe_config->port_clock = skl_calc_pll_link(dev_priv,
> > > pll_id);
> > > +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> > > pll_id);
> > >  
> > >  	ddi_dotclock_get(pipe_config);
> > >  }
> > > @@ -1437,11 +1437,26 @@ static void bxt_ddi_clock_get(struct
> > > intel_encoder *encoder,
> > >  	enum port port = intel_ddi_get_encoder_port(encoder);
> > >  	enum intel_dpll_id pll_id = port;
> > >  
> > > -	pipe_config->port_clock = bxt_calc_pll_link(dev_priv,
> > > pll_id);
> > > +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> > > pll_id);
> > >  
> > >  	ddi_dotclock_get(pipe_config);
> > >  }
> > >  
> > > +int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
> > > +			 enum intel_dpll_id pll_id)
> > > +{
> > > +	if (IS_GEN9_BC(dev_priv)) {
> > > +		return skl_calc_pll_link(dev_priv, pll_id);
> > > +	} else if (IS_GEN9_LP(dev_priv)) {
> > > +		return bxt_calc_pll_link(dev_priv, pll_id);
> > > +	} else if (IS_CANNONLAKE(dev_priv)) {
> > > +		return cnl_calc_pll_link(dev_priv, pll_id);
> > > +	} else {
> > > +		MISSING_CASE(INTEL_GEN(dev_priv));
> > > +		return 0;
> > > +	}
> > > +}
> > Personally, I feel that we wouldn't need to unify this. We call
> > platform specific functions from this intel_ddi_port_clock() and this
> > is called from platform specific *_ddi_clock_get() routine. Why not
> > just keep all platform specific functions in one place?
> 
> Yeap, it is indeed silly to unify since we are just calling it
> from the specific functions.
> 
> Let's drop this patch ;)

oh no! wait! I was rebasing here and I'm not sure I want to drop this patch.

my OCD prefers to export general functions and leave platform specific
functions as static instead of exporting all specific platforms one.
Apparently in the future we will always need this port clock for dvfs
functions called from both places cdclk and dpll so every platform
exporting a specific function won't be ideal.

so imo it would be good if we could organize this one starting here
already.

> 
> > 
> > Paolo, any opinions on this one?
> > 
> > > +
> > >  void intel_ddi_clock_get(struct intel_encoder *encoder,
> > >  			 struct intel_crtc_state *pipe_config)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 0cab667fff57..fe4650d6db03 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1293,7 +1293,8 @@ bool intel_ddi_is_audio_enabled(struct
> > > drm_i915_private *dev_priv,
> > >  				 struct intel_crtc *intel_crtc);
> > >  void intel_ddi_get_config(struct intel_encoder *encoder,
> > >  			  struct intel_crtc_state *pipe_config);
> > > -
> > > +int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
> > > +			 enum intel_dpll_id pll_id);
> > >  void intel_ddi_clock_get(struct intel_encoder *encoder,
> > >  			 struct intel_crtc_state *pipe_config);
> > >  void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state
> > > *crtc_state,
> > -- 
> > Mika Kahola - Intel OTC
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/13] drm/i915/cnl: extract cnl_dvfs_{pre, post}_change

[Ausmus, James]

On Tue, Oct 3, 2017 at 12:06 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> These functions even have their own page in our spec,
> so extract them from cnl_set_cdclk().
>
> v2: (By Rodrigo) Fixed inverted logic on error return of
>     cnl_dvfs_pre_change.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Reviewed-by: James Ausmus <james.ausmus@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 87fc42b19336..b35eb145d66e 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1510,12 +1510,8 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
>         dev_priv->cdclk.hw.vco = vco;
>  }
>
> -static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> -                         const struct intel_cdclk_state *cdclk_state)
> +static int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
>  {
> -       int cdclk = cdclk_state->cdclk;
> -       int vco = cdclk_state->vco;
> -       u32 val, divider, pcu_ack;
>         int ret;
>
>         mutex_lock(&dev_priv->rps.hw_lock);
> @@ -1524,11 +1520,30 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
>                                 SKL_CDCLK_READY_FOR_CHANGE,
>                                 SKL_CDCLK_READY_FOR_CHANGE, 3);
>         mutex_unlock(&dev_priv->rps.hw_lock);
> -       if (ret) {
> +
> +       if (ret)
>                 DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
>                           ret);
> +
> +       return ret;
> +}
> +
> +static void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level)
> +{
> +       mutex_lock(&dev_priv->rps.hw_lock);
> +       sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, level);
> +       mutex_unlock(&dev_priv->rps.hw_lock);
> +}
> +
> +static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> +                         const struct intel_cdclk_state *cdclk_state)
> +{
> +       int cdclk = cdclk_state->cdclk;
> +       int vco = cdclk_state->vco;
> +       u32 val, divider, pcu_ack;
> +
> +       if (cnl_dvfs_pre_change(dev_priv))
>                 return;
> -       }
>
>         /* cdclk = vco / 2 / div{1,2} */
>         switch (DIV_ROUND_CLOSEST(vco, cdclk)) {
> @@ -1575,9 +1590,7 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
>         I915_WRITE(CDCLK_CTL, val);
>
>         /* inform PCU of the change */
> -       mutex_lock(&dev_priv->rps.hw_lock);
> -       sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, pcu_ack);
> -       mutex_unlock(&dev_priv->rps.hw_lock);
> +       cnl_dvfs_post_change(dev_priv, pcu_ack);
>
>         intel_update_cdclk(dev_priv);
>  }
> --
> 2.13.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 


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

Re: [PATCH 05/13] drm/i915/cnl: extract cnl_dvfs_{pre, post}_change

[Manasi Navare]

Looks good and this refactoring makes since.

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

On Tue, Oct 03, 2017 at 12:06:06AM -0700, Rodrigo Vivi wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> These functions even have their own page in our spec,
> so extract them from cnl_set_cdclk().
> 
> v2: (By Rodrigo) Fixed inverted logic on error return of
>     cnl_dvfs_pre_change.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 87fc42b19336..b35eb145d66e 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1510,12 +1510,8 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
>  	dev_priv->cdclk.hw.vco = vco;
>  }
>  
> -static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state)
> +static int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
>  {
> -	int cdclk = cdclk_state->cdclk;
> -	int vco = cdclk_state->vco;
> -	u32 val, divider, pcu_ack;
>  	int ret;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> @@ -1524,11 +1520,30 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
>  				SKL_CDCLK_READY_FOR_CHANGE,
>  				SKL_CDCLK_READY_FOR_CHANGE, 3);
>  	mutex_unlock(&dev_priv->rps.hw_lock);
> -	if (ret) {
> +
> +	if (ret)
>  		DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
>  			  ret);
> +
> +	return ret;
> +}
> +
> +static void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level)
> +{
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, level);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +}
> +
> +static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> +			  const struct intel_cdclk_state *cdclk_state)
> +{
> +	int cdclk = cdclk_state->cdclk;
> +	int vco = cdclk_state->vco;
> +	u32 val, divider, pcu_ack;
> +
> +	if (cnl_dvfs_pre_change(dev_priv))
>  		return;
> -	}
>  
>  	/* cdclk = vco / 2 / div{1,2} */
>  	switch (DIV_ROUND_CLOSEST(vco, cdclk)) {
> @@ -1575,9 +1590,7 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
>  	I915_WRITE(CDCLK_CTL, val);
>  
>  	/* inform PCU of the change */
> -	mutex_lock(&dev_priv->rps.hw_lock);
> -	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, pcu_ack);
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> +	cnl_dvfs_post_change(dev_priv, pcu_ack);
>  
>  	intel_update_cdclk(dev_priv);
>  }
> -- 
> 2.13.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/13] drm/i915/cnl: Expose DVFS change functions

[Manasi Navare]

On Tue, Oct 03, 2017 at 12:06:07AM -0700, Rodrigo Vivi wrote:
> From: "Kahola, Mika" <mika.kahola@intel.com>
> 
> DVFS computation needs cnl_dvfs_{pre,post}_change() functions to be exposed.
> These functions will be used when computing DVFS levels in intel_dpll_mgr.c
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 4 ++--
>  drivers/gpu/drm/i915/intel_drv.h   | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index b35eb145d66e..af8411c2a6b9 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1510,7 +1510,7 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
>  	dev_priv->cdclk.hw.vco = vco;
>  }
>  
> -static int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
> +int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
>  {
>  	int ret;
>  
> @@ -1528,7 +1528,7 @@ static int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -static void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level)
> +void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level)
>  {
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, level);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fe4650d6db03..934ccf17f8ab 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1323,6 +1323,8 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv);
>  void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
>  void cnl_init_cdclk(struct drm_i915_private *dev_priv);
>  void cnl_uninit_cdclk(struct drm_i915_private *dev_priv);
> +int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
> +void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level);
>  void bxt_init_cdclk(struct drm_i915_private *dev_priv);
>  void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
>  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> -- 
> 2.13.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

[Manasi Navare]

On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> From: "Kahola, Mika" <mika.kahola@intel.com>
> 
> Display Voltage and Frequency Switching (DVFS) is used to adjust the
> display voltage to match the display clock frequencies. If voltage is
> set too low, it will break functionality. If voltage is set too high,
> it will waste power. Voltage level is selected based on CD clock and
> DDI clock.
> 
> The sequence before frequency change is the following and it requests
> the power controller to raise voltage to maximum
> 
> - Ensure any previous GT Driver Mailbox transaction is complete.
> - Write GT Driver Mailbox Data Low = 0x3.
> - Write GT Driver Mailbox Data High = 0x0.
> - Write GT Driver Mailbox Interface = 0x80000007.
> - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
>   Timeout after 3ms
> 
> The sequence after frequency change is the following and it requests
> the port controller to raise voltage to the requested level.
> 
> - Write GT Driver Mailbox Data Low
>  * For level 0, write 0x0
>  * For level 1, write 0x1
>  * For level 2, write 0x2
>  * For level 3, write 0x3
>    - Write GT Driver Mailbox Data High = 0x0.
>    - Write GT Driver Mailbox Interface = 0x80000007.
> 
> For Cannonlake, the level 3 is not used and it aliases to level 2.
> 
> v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
>     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
> v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
>     The port clock calculation here was only addressing DP, so let's reuse
>     the current port calculation that is already in place without any duplication.
>     Alos fix portclk <= 594000 instead of portclk < 594000.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Looks good, dowble checked with the voltage levels from the spec.

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index a2a3d93d67bd..6030fbafa580 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
>  	.dump_hw_state = bxt_dump_hw_state,
>  };
>  
> +static int cnl_get_dvfs_level(int cdclk, int portclk)
> +{
> +	if (cdclk == 168000 && portclk <= 594000)
> +		return 0;
> +	else if (cdclk == 336000 && portclk <= 594000)
> +		return 1;
> +	else
> +		return 2;
> +}
> +
>  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  			       struct intel_shared_dpll *pll)
>  {
>  	uint32_t val;
> +	int ret;
> +	int level;
> +	int cdclk, portclk;
>  
>  	/* 1. Enable DPLL power in DPLL_ENABLE. */
>  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  	/*
>  	 * 5. If the frequency will result in a change to the voltage
>  	 * requirement, follow the Display Voltage Frequency Switching
> -	 * Sequence Before Frequency Change
> -	 *
> -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> -	 * display clock frequencies
> +	 * (DVFS) Sequence Before Frequency Change
>  	 */
> +	ret = cnl_dvfs_pre_change(dev_priv);
>  
>  	/* 6. Enable DPLL in DPLL_ENABLE. */
>  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  	/*
>  	 * 8. If the frequency will result in a change to the voltage
>  	 * requirement, follow the Display Voltage Frequency Switching
> -	 * Sequence After Frequency Change
> -	 *
> -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> -	 * display clock frequencies
> +	 * (DVFS) Sequence After Frequency Change
>  	 */
> +	if (ret == 0) {
> +		cdclk = dev_priv->cdclk.hw.cdclk;
> +		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> +		level = cnl_get_dvfs_level(cdclk, portclk);
> +		cnl_dvfs_post_change(dev_priv, level);
> +	}
>  
>  	/*
>  	 * 9. turn on the clock for the DDI and map the DPLL to the DDI
> -- 
> 2.13.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/13] drm/i915/cnl: DVFS for PLL disabling

[Manasi Navare]

On Tue, Oct 03, 2017 at 12:06:09AM -0700, Rodrigo Vivi wrote:
> From: "Kahola, Mika" <mika.kahola@intel.com>
> 
> Display Voltage and Frequency Switching (DVFS) is used to adjust the
> display voltage to match the display clock frequencies. To save power the
> voltage is set to minimum when disabling PLL.
> 
> The sequence before frequency change is the following and it requests
> the power controller to raise voltage to maximum
> 
> - Ensure any previous GT Driver Mailbox transaction is complete.
> - Write GT Driver Mailbox Data Low = 0x3.
> - Write GT Driver Mailbox Data High = 0x0.
> - Write GT Driver Mailbox Interface = 0x80000007.
> - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
>   Timeout after 3ms
> 
> The sequence after frequency change is the following and it requests
> the port controller to lower voltage to the minimum.
> 
> - Write GT Driver Mailbox Data Low = 0x0
> - Write GT Driver Mailbox Data High = 0x0.
> - Write GT Driver Mailbox Interface = 0x80000007.
> 
> v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
>     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> v3: (By Rodrigo): Fix typo on Paulo's name.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 6030fbafa580..a71a6c396bbd 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2058,6 +2058,7 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
>  				struct intel_shared_dpll *pll)
>  {
>  	uint32_t val;
> +	int ret;
>  
>  	/*
>  	 * 1. Configure DPCLKA_CFGCR0 to turn off the clock for the DDI.
> @@ -2067,11 +2068,9 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
>  	/*
>  	 * 2. If the frequency will result in a change to the voltage
>  	 * requirement, follow the Display Voltage Frequency Switching
> -	 * Sequence Before Frequency Change
> -	 *
> -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> -	 * display clock frequencies
> +	 * (DVFS) Sequence Before Frequency Change
>  	 */
> +	ret = cnl_dvfs_pre_change(dev_priv);
>  
>  	/* 3. Disable DPLL through DPLL_ENABLE. */
>  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> @@ -2089,11 +2088,10 @@ static void cnl_ddi_pll_disable(struct drm_i915_private *dev_priv,
>  	/*
>  	 * 5. If the frequency will result in a change to the voltage
>  	 * requirement, follow the Display Voltage Frequency Switching
> -	 * Sequence After Frequency Change
> -	 *
> -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> -	 * display clock frequencies
> +	 * (DVFS) Sequence After Frequency Change
>  	 */
> +	if (ret == 0)
> +		cnl_dvfs_post_change(dev_priv, 0);
>  
>  	/* 6. Disable DPLL power in DPLL_ENABLE. */
>  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> -- 
> 2.13.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/13] drm/i915/cnl: Invert dvfs default level.

[Manasi Navare]

On Wed, Oct 04, 2017 at 12:36:42PM -0700, Rodrigo Vivi wrote:
> On Wed, Oct 04, 2017 at 09:46:41AM +0000, Mika Kahola wrote:
> > On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > > According to spec "If voltage is set too low,
> > > it will break functionality. If voltage is set too high,
> > >  it will waste power."
> > > 
> > > So, let's prefer the waste of power instead of breaking
> > > functionality.
> > > 
> > > But also the logic of deciding the level on spec
> > > tells "Else, use level 2."
> > > So, default is actually "2", not "0".
> > The spec also says
> > 
> > "If CD clock = 168 MHz AND DDI clock ≤ 594 MHz, use level 0.
> > Else If CD clock = 336 MHz AND DDI clock ≤ 594 MHz, use level 1.
> > Else, use level 2."
> > 
> > Should we add check for DDI clock rate as well here?
> 
> By this time dpll are disabled and not associated to any
> port yet. So portclock is 0. So if we invert the default
> we do respect the same algorightm you pasted.
> Also if you notice I'm just inverting this in a separeted
> patch to make it clear and if needed to bisect we end up
> on this single point of change. But on a later patch on
> this series I change to use your function with this algoright
> there, but giving portclock = 0.
> 
> So by the end of this series the flow is:
> 
> - enable cdclk
> - request dvfs level enough for cdclk in question.
> - enable pll
> - adjust dvfs level only if needed taking the port clock into account.
> - disable pll
> - put back level to cdclock level if needed.
> 
> > 
> > > 
> > > v2: Rebase moving it up to avoid some temporary code
> > >     duplication.
> > > 
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index af8411c2a6b9..7e9c4444c844 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -1562,15 +1562,15 @@ static void cnl_set_cdclk(struct
> > > drm_i915_private *dev_priv,
> > >  	}
> > >  
> > >  	switch (cdclk) {
> > > -	case 528000:
> > > -		pcu_ack = 2;
> > > +	case 168000:
> > > +		pcu_ack = 0;
> > >  		break;
> > >  	case 336000:
> > >  		pcu_ack = 1;
> > >  		break;
> > > -	case 168000:
> > > +	case 528000:
> > >  	default:
> > > -		pcu_ack = 0;
> > > +		pcu_ack = 2;
> > >  		break;

The spec says "Else If CD clock ≤ 556.8 AND DDI clock > 594 MHz, use level 1"
So in case of cdclock = 528000, it is still < 556.8Mhz and the level should be 1

Manasi

> > >  	}
> > >  
> > -- 
> > Mika Kahola - Intel OTC
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/13] drm/i915/cnl: Invert dvfs default level.

[Manasi Navare]

On Wed, Oct 04, 2017 at 03:40:11PM -0700, Manasi Navare wrote:
> On Wed, Oct 04, 2017 at 12:36:42PM -0700, Rodrigo Vivi wrote:
> > On Wed, Oct 04, 2017 at 09:46:41AM +0000, Mika Kahola wrote:
> > > On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > > > According to spec "If voltage is set too low,
> > > > it will break functionality. If voltage is set too high,
> > > >  it will waste power."
> > > > 
> > > > So, let's prefer the waste of power instead of breaking
> > > > functionality.
> > > > 
> > > > But also the logic of deciding the level on spec
> > > > tells "Else, use level 2."
> > > > So, default is actually "2", not "0".
> > > The spec also says
> > > 
> > > "If CD clock = 168 MHz AND DDI clock ≤ 594 MHz, use level 0.
> > > Else If CD clock = 336 MHz AND DDI clock ≤ 594 MHz, use level 1.
> > > Else, use level 2."
> > > 
> > > Should we add check for DDI clock rate as well here?
> > 
> > By this time dpll are disabled and not associated to any
> > port yet. So portclock is 0. So if we invert the default
> > we do respect the same algorightm you pasted.
> > Also if you notice I'm just inverting this in a separeted
> > patch to make it clear and if needed to bisect we end up
> > on this single point of change. But on a later patch on
> > this series I change to use your function with this algoright
> > there, but giving portclock = 0.
> > 
> > So by the end of this series the flow is:
> > 
> > - enable cdclk
> > - request dvfs level enough for cdclk in question.
> > - enable pll
> > - adjust dvfs level only if needed taking the port clock into account.
> > - disable pll
> > - put back level to cdclock level if needed.
> > 
> > > 
> > > > 
> > > > v2: Rebase moving it up to avoid some temporary code
> > > >     duplication.
> > > > 
> > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > > b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > index af8411c2a6b9..7e9c4444c844 100644
> > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > @@ -1562,15 +1562,15 @@ static void cnl_set_cdclk(struct
> > > > drm_i915_private *dev_priv,
> > > >  	}
> > > >  
> > > >  	switch (cdclk) {
> > > > -	case 528000:
> > > > -		pcu_ack = 2;
> > > > +	case 168000:
> > > > +		pcu_ack = 0;
> > > >  		break;
> > > >  	case 336000:
> > > >  		pcu_ack = 1;
> > > >  		break;
> > > > -	case 168000:
> > > > +	case 528000:
> > > >  	default:
> > > > -		pcu_ack = 0;
> > > > +		pcu_ack = 2;
> > > >  		break;
> 
> The spec says "Else If CD clock ≤ 556.8 AND DDI clock > 594 MHz, use level 1"
> So in case of cdclock = 528000, it is still < 556.8Mhz and the level should be 1
> 
> Manasi
>

Never mind, I was looking at a different section which had this logic.

Manasi 
> > > >  	}
> > > >  
> > > -- 
> > > Mika Kahola - Intel OTC
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/13] drm/i915: Unify and export gen9+ port_clock calculation.

[Mika Kahola]

On Wed, 2017-10-04 at 14:26 -0700, Rodrigo Vivi wrote:
> On Wed, Oct 04, 2017 at 07:38:16PM +0000, Rodrigo Vivi wrote:
> > 
> > On Wed, Oct 04, 2017 at 06:39:19AM +0000, Mika Kahola wrote:
> > > 
> > > On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > > > 
> > > > On Cannonlake the DVFS level selection depends on the
> > > > port clock.
> > > > 
> > > > So let's re-org in a way that we can easily export without
> > > > duplicating any code.
> > > > 
> > > > v2: Rebased on changes on previous patches
> > > > 
> > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c | 21 ++++++++++++++++++---
> > > >  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
> > > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 92eabb6cc1ab..ee64b1a50453 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -1297,7 +1297,7 @@ static void cnl_ddi_clock_get(struct
> > > > intel_encoder *encoder,
> > > >  
> > > >  	pll_id = intel_get_shared_dpll_id(dev_priv,
> > > > pipe_config-
> > > > > 
> > > > > shared_dpll);
> > > >  
> > > > -	pipe_config->port_clock = cnl_calc_pll_link(dev_priv,
> > > > pll_id);
> > > > +	pipe_config->port_clock =
> > > > intel_ddi_port_clock(dev_priv,
> > > > pll_id);
> > > >  
> > > >  	ddi_dotclock_get(pipe_config);
> > > >  }
> > > > @@ -1353,7 +1353,7 @@ static void skl_ddi_clock_get(struct
> > > > intel_encoder *encoder,
> > > >  
> > > >  	pll_id = intel_get_shared_dpll_id(dev_priv,
> > > > pipe_config-
> > > > > 
> > > > > shared_dpll);
> > > >  
> > > > -	pipe_config->port_clock = skl_calc_pll_link(dev_priv,
> > > > pll_id);
> > > > +	pipe_config->port_clock =
> > > > intel_ddi_port_clock(dev_priv,
> > > > pll_id);
> > > >  
> > > >  	ddi_dotclock_get(pipe_config);
> > > >  }
> > > > @@ -1437,11 +1437,26 @@ static void bxt_ddi_clock_get(struct
> > > > intel_encoder *encoder,
> > > >  	enum port port = intel_ddi_get_encoder_port(encoder);
> > > >  	enum intel_dpll_id pll_id = port;
> > > >  
> > > > -	pipe_config->port_clock = bxt_calc_pll_link(dev_priv,
> > > > pll_id);
> > > > +	pipe_config->port_clock =
> > > > intel_ddi_port_clock(dev_priv,
> > > > pll_id);
> > > >  
> > > >  	ddi_dotclock_get(pipe_config);
> > > >  }
> > > >  
> > > > +int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
> > > > +			 enum intel_dpll_id pll_id)
> > > > +{
> > > > +	if (IS_GEN9_BC(dev_priv)) {
> > > > +		return skl_calc_pll_link(dev_priv, pll_id);
> > > > +	} else if (IS_GEN9_LP(dev_priv)) {
> > > > +		return bxt_calc_pll_link(dev_priv, pll_id);
> > > > +	} else if (IS_CANNONLAKE(dev_priv)) {
> > > > +		return cnl_calc_pll_link(dev_priv, pll_id);
> > > > +	} else {
> > > > +		MISSING_CASE(INTEL_GEN(dev_priv));
> > > > +		return 0;
> > > > +	}
> > > > +}
> > > Personally, I feel that we wouldn't need to unify this. We call
> > > platform specific functions from this intel_ddi_port_clock() and
> > > this
> > > is called from platform specific *_ddi_clock_get() routine. Why
> > > not
> > > just keep all platform specific functions in one place?
> > Yeap, it is indeed silly to unify since we are just calling it
> > from the specific functions.
> > 
> > Let's drop this patch ;)
> oh no! wait! I was rebasing here and I'm not sure I want to drop this
> patch.
> 
> my OCD prefers to export general functions and leave platform
> specific
> functions as static instead of exporting all specific platforms one.
> Apparently in the future we will always need this port clock for dvfs
> functions called from both places cdclk and dpll so every platform
> exporting a specific function won't be ideal.
> 
> so imo it would be good if we could organize this one starting here
> already.
Ok, fair enough :) to be future proof we can export only general
functions and leave platform specific routines as static ones.

Therefore, you can bash my r-b

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> 
> > 
> > 
> > > 
> > > 
> > > Paolo, any opinions on this one?
> > > 
> > > > 
> > > > +
> > > >  void intel_ddi_clock_get(struct intel_encoder *encoder,
> > > >  			 struct intel_crtc_state *pipe_config)
> > > >  {
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 0cab667fff57..fe4650d6db03 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1293,7 +1293,8 @@ bool intel_ddi_is_audio_enabled(struct
> > > > drm_i915_private *dev_priv,
> > > >  				 struct intel_crtc
> > > > *intel_crtc);
> > > >  void intel_ddi_get_config(struct intel_encoder *encoder,
> > > >  			  struct intel_crtc_state
> > > > *pipe_config);
> > > > -
> > > > +int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
> > > > +			 enum intel_dpll_id pll_id);
> > > >  void intel_ddi_clock_get(struct intel_encoder *encoder,
> > > >  			 struct intel_crtc_state
> > > > *pipe_config);
> > > >  void intel_ddi_set_vc_payload_alloc(const struct
> > > > intel_crtc_state
> > > > *crtc_state,
-- 
Mika Kahola - Intel OTC

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

Re: [PATCH 11/13] drm/i915/cnl: Only request voltage frequency switching when needed.

[Mika Kahola]

On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> At cdclk initialization at spec they tell us to always use dvfs
> or avoid change cdclk if it fails.
> 
> Here on pll sequence, spec tells us to only use dvfs "if the
> frequency will result in a change to the voltage requirement."
> 
> So in order to respect that and avoid necessary interactions with
> PCODE we compare our current needed level with the current set level
> and only perform dvfs sequences when we need to change that level.
> 
> v2: - Add missing blank line after declaration block
>     - s/need/needs: When adding doc needs sounded better.
> 
> Cc: Mika Kahola <mika.kahola@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c    | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 16 +++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h      |  1 +
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index c62d6e752fb7..8111a13079e1 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1545,6 +1545,16 @@ int cnl_dvfs_new_level(int cdclk, int portclk)
>  		return 2;
>  }
>  
> +bool cnl_dvfs_needs_change(struct drm_i915_private *dev_priv, int
> level)
> +{
> +	int old_level;
I was thinking of naming of this variable. We are fetching the current
level so why not rename this variable as cur_level?

Either way,

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> &old_level);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +	return level != old_level;
> +}
> +
>  static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
>  			  const struct intel_cdclk_state
> *cdclk_state)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 0dddbd3a7a97..85c000891439 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1970,9 +1970,10 @@ static void cnl_ddi_pll_enable(struct
> drm_i915_private *dev_priv,
>  			       struct intel_shared_dpll *pll)
>  {
>  	uint32_t val;
> -	int ret;
> +	int ret = 0;
>  	int level;
>  	int cdclk, portclk;
> +	bool change_level;
>  
>  	/* 1. Enable DPLL power in DPLL_ENABLE. */
>  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> @@ -2011,7 +2012,12 @@ static void cnl_ddi_pll_enable(struct
> drm_i915_private *dev_priv,
>  	 * requirement, follow the Display Voltage Frequency
> Switching
>  	 * (DVFS) Sequence Before Frequency Change
>  	 */
> -	ret = cnl_dvfs_pre_change(dev_priv);
> +	cdclk = dev_priv->cdclk.hw.cdclk;
> +	portclk = intel_ddi_port_clock(dev_priv, pll->id);
> +	level = cnl_dvfs_new_level(cdclk, portclk);
> +	change_level = cnl_dvfs_needs_change(dev_priv, level);
> +	if (change_level)
> +		ret = cnl_dvfs_pre_change(dev_priv);
>  
>  	/* 6. Enable DPLL in DPLL_ENABLE. */
>  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> @@ -2031,12 +2037,8 @@ static void cnl_ddi_pll_enable(struct
> drm_i915_private *dev_priv,
>  	 * requirement, follow the Display Voltage Frequency
> Switching
>  	 * (DVFS) Sequence After Frequency Change
>  	 */
> -	if (ret == 0) {
> -		cdclk = dev_priv->cdclk.hw.cdclk;
> -		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> -		level = cnl_dvfs_new_level(cdclk, portclk);
> +	if (change_level && ret == 0)
>  		cnl_dvfs_post_change(dev_priv, level);
> -	}
>  
>  	/*
>  	 * 9. turn on the clock for the DDI and map the DPLL to the
> DDI
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index dec11d7b15ab..8621110401e3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1326,6 +1326,7 @@ void cnl_uninit_cdclk(struct drm_i915_private
> *dev_priv);
>  int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
>  void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int
> level);
>  int cnl_dvfs_new_level(int cdclk, int portclk);
> +bool cnl_dvfs_needs_change(struct drm_i915_private *dev_priv, int
> level);
>  void bxt_init_cdclk(struct drm_i915_private *dev_priv);
>  void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
>  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
-- 
Mika Kahola - Intel OTC

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

Re: [PATCH 10/13] drm/i915/cnl: Unify dvfs level selection.

[Rodrigo Vivi]

On Wed, Oct 04, 2017 at 01:20:57PM +0000, Mika Kahola wrote:
> On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > When port clock is zero or undefined we base our
> > calculation on cdclk. So, same function can be used
> > for port clock == 0 now that we have the same default "2".
> > 
> > v2: s/get/new: When documenting "get" sounded ambiguous
> >     because we could be getting the current level at pcode.
> > 
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c    | 28 +++++++++++++----------
> > -----
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 12 +-----------
> >  drivers/gpu/drm/i915/intel_drv.h      |  1 +
> >  3 files changed, 15 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 7e9c4444c844..c62d6e752fb7 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1535,12 +1535,22 @@ void cnl_dvfs_post_change(struct
> > drm_i915_private *dev_priv, int level)
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> >  }
> >  
> > +int cnl_dvfs_new_level(int cdclk, int portclk)
> > +{
> > +	if (cdclk == 168000 && portclk <= 594000)
> > +		return 0;
> > +	else if (cdclk == 336000 && portclk <= 594000)
> > +		return 1;
> > +	else
> > +		return 2;
> > +}
> > +
> Maybe we could leave the function name as cnl_get_dvfs_level()?
> The original one is stripped down later in this patch.

Since I was exporting I decided to use the same standard as others
and move dvfs back a bit
so cnl_dvfs_get_level...
But also when I was documenting on the last patch I noticed
that "get level" is kind of ambiguous... which level? the current
or the new? the more natural is current, but this function also
get the new desired/needed.

But I'm open to other suggestions that fix the ambiguity.

>  
> >  static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> >  			  const struct intel_cdclk_state
> > *cdclk_state)
> >  {
> >  	int cdclk = cdclk_state->cdclk;
> >  	int vco = cdclk_state->vco;
> > -	u32 val, divider, pcu_ack;
> > +	u32 val, divider, level;
> >  
> >  	if (cnl_dvfs_pre_change(dev_priv))
> >  		return;
> > @@ -1561,19 +1571,6 @@ static void cnl_set_cdclk(struct
> > drm_i915_private *dev_priv,
> >  		break;
> >  	}
> >  
> > -	switch (cdclk) {
> > -	case 168000:
> > -		pcu_ack = 0;
> > -		break;
> > -	case 336000:
> > -		pcu_ack = 1;
> > -		break;
> > -	case 528000:
> > -	default:
> > -		pcu_ack = 2;
> > -		break;
> > -	}
> > -
> >  	if (dev_priv->cdclk.hw.vco != 0 &&
> >  	    dev_priv->cdclk.hw.vco != vco)
> >  		cnl_cdclk_pll_disable(dev_priv);
> > @@ -1590,7 +1587,8 @@ static void cnl_set_cdclk(struct
> > drm_i915_private *dev_priv,
> >  	I915_WRITE(CDCLK_CTL, val);
> >  
> >  	/* inform PCU of the change */
> > -	cnl_dvfs_post_change(dev_priv, pcu_ack);
> > +	level = cnl_dvfs_new_level(cdclk, 0);
> > +	cnl_dvfs_post_change(dev_priv, level);
> >  
> >  	intel_update_cdclk(dev_priv);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index a71a6c396bbd..0dddbd3a7a97 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -1966,16 +1966,6 @@ static const struct intel_dpll_mgr bxt_pll_mgr
> > = {
> >  	.dump_hw_state = bxt_dump_hw_state,
> >  };
> >  
> > -static int cnl_get_dvfs_level(int cdclk, int portclk)
> > -{
> > -	if (cdclk == 168000 && portclk <= 594000)
> > -		return 0;
> > -	else if (cdclk == 336000 && portclk <= 594000)
> > -		return 1;
> > -	else
> > -		return 2;
> > -}
> > -
> >  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >  			       struct intel_shared_dpll *pll)
> >  {
> > @@ -2044,7 +2034,7 @@ static void cnl_ddi_pll_enable(struct
> > drm_i915_private *dev_priv,
> >  	if (ret == 0) {
> >  		cdclk = dev_priv->cdclk.hw.cdclk;
> >  		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > -		level = cnl_get_dvfs_level(cdclk, portclk);
> > +		level = cnl_dvfs_new_level(cdclk, portclk);
> >  		cnl_dvfs_post_change(dev_priv, level);
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 934ccf17f8ab..dec11d7b15ab 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1325,6 +1325,7 @@ void cnl_init_cdclk(struct drm_i915_private
> > *dev_priv);
> >  void cnl_uninit_cdclk(struct drm_i915_private *dev_priv);
> >  int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
> >  void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int
> > level);
> > +int cnl_dvfs_new_level(int cdclk, int portclk);
> >  void bxt_init_cdclk(struct drm_i915_private *dev_priv);
> >  void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
> >  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> -- 
> Mika Kahola - Intel OTC
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/13] drm/i915/cnl: Only request voltage frequency switching when needed.

[Rodrigo Vivi]

On Thu, Oct 05, 2017 at 12:07:08PM +0000, Mika Kahola wrote:
> On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > At cdclk initialization at spec they tell us to always use dvfs
> > or avoid change cdclk if it fails.
> > 
> > Here on pll sequence, spec tells us to only use dvfs "if the
> > frequency will result in a change to the voltage requirement."
> > 
> > So in order to respect that and avoid necessary interactions with
> > PCODE we compare our current needed level with the current set level
> > and only perform dvfs sequences when we need to change that level.
> > 
> > v2: - Add missing blank line after declaration block
> >     - s/need/needs: When adding doc needs sounded better.
> > 
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c    | 10 ++++++++++
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 16 +++++++++-------
> >  drivers/gpu/drm/i915/intel_drv.h      |  1 +
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index c62d6e752fb7..8111a13079e1 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1545,6 +1545,16 @@ int cnl_dvfs_new_level(int cdclk, int portclk)
> >  		return 2;
> >  }
> >  
> > +bool cnl_dvfs_needs_change(struct drm_i915_private *dev_priv, int
> > level)
> > +{
> > +	int old_level;
> I was thinking of naming of this variable. We are fetching the current
> level so why not rename this variable as cur_level?

good idea. cur_level it will be on the next version! ;)
thanks

> 
> Either way,
> 
> Reviewed-by: Mika Kahola <mika.kahola@intel.com>
> 
> > +
> > +	mutex_lock(&dev_priv->rps.hw_lock);
> > +	sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> > &old_level);
> > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > +	return level != old_level;
> > +}
> > +
> >  static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> >  			  const struct intel_cdclk_state
> > *cdclk_state)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index 0dddbd3a7a97..85c000891439 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -1970,9 +1970,10 @@ static void cnl_ddi_pll_enable(struct
> > drm_i915_private *dev_priv,
> >  			       struct intel_shared_dpll *pll)
> >  {
> >  	uint32_t val;
> > -	int ret;
> > +	int ret = 0;
> >  	int level;
> >  	int cdclk, portclk;
> > +	bool change_level;
> >  
> >  	/* 1. Enable DPLL power in DPLL_ENABLE. */
> >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > @@ -2011,7 +2012,12 @@ static void cnl_ddi_pll_enable(struct
> > drm_i915_private *dev_priv,
> >  	 * requirement, follow the Display Voltage Frequency
> > Switching
> >  	 * (DVFS) Sequence Before Frequency Change
> >  	 */
> > -	ret = cnl_dvfs_pre_change(dev_priv);
> > +	cdclk = dev_priv->cdclk.hw.cdclk;
> > +	portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > +	level = cnl_dvfs_new_level(cdclk, portclk);
> > +	change_level = cnl_dvfs_needs_change(dev_priv, level);
> > +	if (change_level)
> > +		ret = cnl_dvfs_pre_change(dev_priv);
> >  
> >  	/* 6. Enable DPLL in DPLL_ENABLE. */
> >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > @@ -2031,12 +2037,8 @@ static void cnl_ddi_pll_enable(struct
> > drm_i915_private *dev_priv,
> >  	 * requirement, follow the Display Voltage Frequency
> > Switching
> >  	 * (DVFS) Sequence After Frequency Change
> >  	 */
> > -	if (ret == 0) {
> > -		cdclk = dev_priv->cdclk.hw.cdclk;
> > -		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > -		level = cnl_dvfs_new_level(cdclk, portclk);
> > +	if (change_level && ret == 0)
> >  		cnl_dvfs_post_change(dev_priv, level);
> > -	}
> >  
> >  	/*
> >  	 * 9. turn on the clock for the DDI and map the DPLL to the
> > DDI
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index dec11d7b15ab..8621110401e3 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1326,6 +1326,7 @@ void cnl_uninit_cdclk(struct drm_i915_private
> > *dev_priv);
> >  int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
> >  void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int
> > level);
> >  int cnl_dvfs_new_level(int cdclk, int portclk);
> > +bool cnl_dvfs_needs_change(struct drm_i915_private *dev_priv, int
> > level);
> >  void bxt_init_cdclk(struct drm_i915_private *dev_priv);
> >  void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
> >  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> -- 
> Mika Kahola - Intel OTC
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

[Ville Syrjälä]

On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> From: "Kahola, Mika" <mika.kahola@intel.com>
> 
> Display Voltage and Frequency Switching (DVFS) is used to adjust the
> display voltage to match the display clock frequencies. If voltage is
> set too low, it will break functionality. If voltage is set too high,
> it will waste power. Voltage level is selected based on CD clock and
> DDI clock.
> 
> The sequence before frequency change is the following and it requests
> the power controller to raise voltage to maximum
> 
> - Ensure any previous GT Driver Mailbox transaction is complete.
> - Write GT Driver Mailbox Data Low = 0x3.
> - Write GT Driver Mailbox Data High = 0x0.
> - Write GT Driver Mailbox Interface = 0x80000007.
> - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
>   Timeout after 3ms
> 
> The sequence after frequency change is the following and it requests
> the port controller to raise voltage to the requested level.
> 
> - Write GT Driver Mailbox Data Low
>  * For level 0, write 0x0
>  * For level 1, write 0x1
>  * For level 2, write 0x2
>  * For level 3, write 0x3
>    - Write GT Driver Mailbox Data High = 0x0.
>    - Write GT Driver Mailbox Interface = 0x80000007.
> 
> For Cannonlake, the level 3 is not used and it aliases to level 2.
> 
> v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
>     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
> v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
>     The port clock calculation here was only addressing DP, so let's reuse
>     the current port calculation that is already in place without any duplication.
>     Alos fix portclk <= 594000 instead of portclk < 594000.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index a2a3d93d67bd..6030fbafa580 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
>  	.dump_hw_state = bxt_dump_hw_state,
>  };
>  
> +static int cnl_get_dvfs_level(int cdclk, int portclk)
> +{
> +	if (cdclk == 168000 && portclk <= 594000)
> +		return 0;
> +	else if (cdclk == 336000 && portclk <= 594000)
> +		return 1;
> +	else
> +		return 2;
> +}
> +
>  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  			       struct intel_shared_dpll *pll)
>  {
>  	uint32_t val;
> +	int ret;
> +	int level;
> +	int cdclk, portclk;
>  
>  	/* 1. Enable DPLL power in DPLL_ENABLE. */
>  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  	/*
>  	 * 5. If the frequency will result in a change to the voltage
>  	 * requirement, follow the Display Voltage Frequency Switching
> -	 * Sequence Before Frequency Change
> -	 *
> -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> -	 * display clock frequencies
> +	 * (DVFS) Sequence Before Frequency Change
>  	 */
> +	ret = cnl_dvfs_pre_change(dev_priv);
>  
>  	/* 6. Enable DPLL in DPLL_ENABLE. */
>  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  	/*
>  	 * 8. If the frequency will result in a change to the voltage
>  	 * requirement, follow the Display Voltage Frequency Switching
> -	 * Sequence After Frequency Change
> -	 *
> -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> -	 * display clock frequencies
> +	 * (DVFS) Sequence After Frequency Change
>  	 */
> +	if (ret == 0) {
> +		cdclk = dev_priv->cdclk.hw.cdclk;
> +		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> +		level = cnl_get_dvfs_level(cdclk, portclk);
> +		cnl_dvfs_post_change(dev_priv, level);

This isn't how I imagined we'd handle this. What I was after is
pre-computing the correct "dfvfs level" (or what I'd probably just call
"voltage" or something like that), and then we'd just let the normal
cdclk programming sequence do its thing.

Is it even safe to do this with other pipes/ports enabled? I would
have assumed no, but maybe I'm mistaken?


> +	}
>  
>  	/*
>  	 * 9. turn on the clock for the DDI and map the DPLL to the DDI
> -- 
> 2.13.5

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

Re: [PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

[Rodrigo Vivi]

On Tue, Oct 17, 2017 at 03:44:21PM +0000, Ville Syrjälä wrote:
> On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> > From: "Kahola, Mika" <mika.kahola@intel.com>
> > 
> > Display Voltage and Frequency Switching (DVFS) is used to adjust the
> > display voltage to match the display clock frequencies. If voltage is
> > set too low, it will break functionality. If voltage is set too high,
> > it will waste power. Voltage level is selected based on CD clock and
> > DDI clock.
> > 
> > The sequence before frequency change is the following and it requests
> > the power controller to raise voltage to maximum
> > 
> > - Ensure any previous GT Driver Mailbox transaction is complete.
> > - Write GT Driver Mailbox Data Low = 0x3.
> > - Write GT Driver Mailbox Data High = 0x0.
> > - Write GT Driver Mailbox Interface = 0x80000007.
> > - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> > - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
> >   Timeout after 3ms
> > 
> > The sequence after frequency change is the following and it requests
> > the port controller to raise voltage to the requested level.
> > 
> > - Write GT Driver Mailbox Data Low
> >  * For level 0, write 0x0
> >  * For level 1, write 0x1
> >  * For level 2, write 0x2
> >  * For level 3, write 0x3
> >    - Write GT Driver Mailbox Data High = 0x0.
> >    - Write GT Driver Mailbox Interface = 0x80000007.
> > 
> > For Cannonlake, the level 3 is not used and it aliases to level 2.
> > 
> > v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
> >     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> > v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
> > v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
> >     The port clock calculation here was only addressing DP, so let's reuse
> >     the current port calculation that is already in place without any duplication.
> >     Alos fix portclk <= 594000 instead of portclk < 594000.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
> >  1 file changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index a2a3d93d67bd..6030fbafa580 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
> >  	.dump_hw_state = bxt_dump_hw_state,
> >  };
> >  
> > +static int cnl_get_dvfs_level(int cdclk, int portclk)
> > +{
> > +	if (cdclk == 168000 && portclk <= 594000)
> > +		return 0;
> > +	else if (cdclk == 336000 && portclk <= 594000)
> > +		return 1;
> > +	else
> > +		return 2;
> > +}
> > +
> >  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >  			       struct intel_shared_dpll *pll)
> >  {
> >  	uint32_t val;
> > +	int ret;
> > +	int level;
> > +	int cdclk, portclk;
> >  
> >  	/* 1. Enable DPLL power in DPLL_ENABLE. */
> >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >  	/*
> >  	 * 5. If the frequency will result in a change to the voltage
> >  	 * requirement, follow the Display Voltage Frequency Switching
> > -	 * Sequence Before Frequency Change
> > -	 *
> > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > -	 * display clock frequencies
> > +	 * (DVFS) Sequence Before Frequency Change
> >  	 */
> > +	ret = cnl_dvfs_pre_change(dev_priv);
> >  
> >  	/* 6. Enable DPLL in DPLL_ENABLE. */
> >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >  	/*
> >  	 * 8. If the frequency will result in a change to the voltage
> >  	 * requirement, follow the Display Voltage Frequency Switching
> > -	 * Sequence After Frequency Change
> > -	 *
> > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > -	 * display clock frequencies
> > +	 * (DVFS) Sequence After Frequency Change
> >  	 */
> > +	if (ret == 0) {
> > +		cdclk = dev_priv->cdclk.hw.cdclk;
> > +		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > +		level = cnl_get_dvfs_level(cdclk, portclk);
> > +		cnl_dvfs_post_change(dev_priv, level);
> 
> This isn't how I imagined we'd handle this. What I was after is
> pre-computing the correct "dfvfs level" (or what I'd probably just call
> "voltage" or something like that), and then we'd just let the normal
> cdclk programming sequence do its thing.
> 
> Is it even safe to do this with other pipes/ports enabled? I would
> have assumed no, but maybe I'm mistaken?

Well, this is how the spec is written. So I assume it is safe to change
that at that point.

Also we enable CDCLK during boot while we have no way to compute the
link rate. So I don't see a better way of hadling that.

So, Art is it ok in the way that I'm proposing here:

on core display init:

- dvfs-pre;
- enable cdclk;
- dvfs-post considering cdclk only and link rate 0; 

on pll init:

- consider cdclk and link rate and if desired level is different from what is currently set: dvfs-pre
- enable pll
- consider cdclk and link rate and if desired level is different from what is currently set: dvfs-post with new level.

This is how I read the spec, but please let me know what I'm missing.

Maybe I'm missing the link rate for eDP during core display init like we are currently kind of missing for SKL?

> 
> 
> > +	}
> >  
> >  	/*
> >  	 * 9. turn on the clock for the DDI and map the DPLL to the DDI
> > -- 
> > 2.13.5
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

[Ville Syrjälä]

On Tue, Oct 17, 2017 at 09:47:05AM -0700, Rodrigo Vivi wrote:
> On Tue, Oct 17, 2017 at 03:44:21PM +0000, Ville Syrjälä wrote:
> > On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> > > From: "Kahola, Mika" <mika.kahola@intel.com>
> > > 
> > > Display Voltage and Frequency Switching (DVFS) is used to adjust the
> > > display voltage to match the display clock frequencies. If voltage is
> > > set too low, it will break functionality. If voltage is set too high,
> > > it will waste power. Voltage level is selected based on CD clock and
> > > DDI clock.
> > > 
> > > The sequence before frequency change is the following and it requests
> > > the power controller to raise voltage to maximum
> > > 
> > > - Ensure any previous GT Driver Mailbox transaction is complete.
> > > - Write GT Driver Mailbox Data Low = 0x3.
> > > - Write GT Driver Mailbox Data High = 0x0.
> > > - Write GT Driver Mailbox Interface = 0x80000007.
> > > - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> > > - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
> > >   Timeout after 3ms
> > > 
> > > The sequence after frequency change is the following and it requests
> > > the port controller to raise voltage to the requested level.
> > > 
> > > - Write GT Driver Mailbox Data Low
> > >  * For level 0, write 0x0
> > >  * For level 1, write 0x1
> > >  * For level 2, write 0x2
> > >  * For level 3, write 0x3
> > >    - Write GT Driver Mailbox Data High = 0x0.
> > >    - Write GT Driver Mailbox Interface = 0x80000007.
> > > 
> > > For Cannonlake, the level 3 is not used and it aliases to level 2.
> > > 
> > > v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
> > >     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> > > v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
> > > v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
> > >     The port clock calculation here was only addressing DP, so let's reuse
> > >     the current port calculation that is already in place without any duplication.
> > >     Alos fix portclk <= 594000 instead of portclk < 594000.
> > > 
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
> > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index a2a3d93d67bd..6030fbafa580 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
> > >  	.dump_hw_state = bxt_dump_hw_state,
> > >  };
> > >  
> > > +static int cnl_get_dvfs_level(int cdclk, int portclk)
> > > +{
> > > +	if (cdclk == 168000 && portclk <= 594000)
> > > +		return 0;
> > > +	else if (cdclk == 336000 && portclk <= 594000)
> > > +		return 1;
> > > +	else
> > > +		return 2;
> > > +}
> > > +
> > >  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > >  			       struct intel_shared_dpll *pll)
> > >  {
> > >  	uint32_t val;
> > > +	int ret;
> > > +	int level;
> > > +	int cdclk, portclk;
> > >  
> > >  	/* 1. Enable DPLL power in DPLL_ENABLE. */
> > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > >  	/*
> > >  	 * 5. If the frequency will result in a change to the voltage
> > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > -	 * Sequence Before Frequency Change
> > > -	 *
> > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > -	 * display clock frequencies
> > > +	 * (DVFS) Sequence Before Frequency Change
> > >  	 */
> > > +	ret = cnl_dvfs_pre_change(dev_priv);
> > >  
> > >  	/* 6. Enable DPLL in DPLL_ENABLE. */
> > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > >  	/*
> > >  	 * 8. If the frequency will result in a change to the voltage
> > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > -	 * Sequence After Frequency Change
> > > -	 *
> > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > -	 * display clock frequencies
> > > +	 * (DVFS) Sequence After Frequency Change
> > >  	 */
> > > +	if (ret == 0) {
> > > +		cdclk = dev_priv->cdclk.hw.cdclk;
> > > +		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > > +		level = cnl_get_dvfs_level(cdclk, portclk);
> > > +		cnl_dvfs_post_change(dev_priv, level);
> > 
> > This isn't how I imagined we'd handle this. What I was after is
> > pre-computing the correct "dfvfs level" (or what I'd probably just call
> > "voltage" or something like that), and then we'd just let the normal
> > cdclk programming sequence do its thing.
> > 
> > Is it even safe to do this with other pipes/ports enabled? I would
> > have assumed no, but maybe I'm mistaken?
> 
> Well, this is how the spec is written. So I assume it is safe to change
> that at that point.
> 
> Also we enable CDCLK during boot while we have no way to compute the
> link rate. So I don't see a better way of hadling that.

We do a full state readout, which should tell us what voltage we need.
And if the cdclk isn't even enabled, then obviously we can't have any
pipes running either, so we can safely enable it with any frequency
even before the full state readout.

> So, Art is it ok in the way that I'm proposing here:
> 
> on core display init:
> 
> - dvfs-pre;
> - enable cdclk;
> - dvfs-post considering cdclk only and link rate 0; 
> 
> on pll init:
> 
> - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-pre
> - enable pll
> - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-post with new level.
> 
> This is how I read the spec, but please let me know what I'm missing.

Hmm. OK, so maybe we can change the voltage while things are up and
running.

That said, the voltage level is a global thing, so whenever we change it
we must consider the state of all ports, and the cdclk. This looks like
it only considers the current port. Thus if we first enable a port that
needs high voltage, and later a port that can make due with a lower
voltage we'll end up with something not working.

We migth also end up changing the voltage multiple times if we enable
multiple ports with one atomic operation. If things went via the normal
cdclk path we'd just do the voltage change the once.

Additonally I don't like that the code is getting the current clock by
means of readout in the middle the modeset. With atomic the design is
pretty much to pass all state in explicitly, and readout is only used
to take over the from the BIOS, and to verify that we programmed the
hardware correctly.
 
> 
> Maybe I'm missing the link rate for eDP during core display init like we are currently kind of missing for SKL?

Hmm. What exactly are we missing?

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

Re: [PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

[Rodrigo Vivi]

On Tue, Oct 17, 2017 at 05:23:20PM +0000, Ville Syrjälä wrote:
> On Tue, Oct 17, 2017 at 09:47:05AM -0700, Rodrigo Vivi wrote:
> > On Tue, Oct 17, 2017 at 03:44:21PM +0000, Ville Syrjälä wrote:
> > > On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> > > > From: "Kahola, Mika" <mika.kahola@intel.com>
> > > > 
> > > > Display Voltage and Frequency Switching (DVFS) is used to adjust the
> > > > display voltage to match the display clock frequencies. If voltage is
> > > > set too low, it will break functionality. If voltage is set too high,
> > > > it will waste power. Voltage level is selected based on CD clock and
> > > > DDI clock.
> > > > 
> > > > The sequence before frequency change is the following and it requests
> > > > the power controller to raise voltage to maximum
> > > > 
> > > > - Ensure any previous GT Driver Mailbox transaction is complete.
> > > > - Write GT Driver Mailbox Data Low = 0x3.
> > > > - Write GT Driver Mailbox Data High = 0x0.
> > > > - Write GT Driver Mailbox Interface = 0x80000007.
> > > > - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> > > > - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
> > > >   Timeout after 3ms
> > > > 
> > > > The sequence after frequency change is the following and it requests
> > > > the port controller to raise voltage to the requested level.
> > > > 
> > > > - Write GT Driver Mailbox Data Low
> > > >  * For level 0, write 0x0
> > > >  * For level 1, write 0x1
> > > >  * For level 2, write 0x2
> > > >  * For level 3, write 0x3
> > > >    - Write GT Driver Mailbox Data High = 0x0.
> > > >    - Write GT Driver Mailbox Interface = 0x80000007.
> > > > 
> > > > For Cannonlake, the level 3 is not used and it aliases to level 2.
> > > > 
> > > > v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
> > > >     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> > > > v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
> > > > v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
> > > >     The port clock calculation here was only addressing DP, so let's reuse
> > > >     the current port calculation that is already in place without any duplication.
> > > >     Alos fix portclk <= 594000 instead of portclk < 594000.
> > > > 
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
> > > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > index a2a3d93d67bd..6030fbafa580 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
> > > >  	.dump_hw_state = bxt_dump_hw_state,
> > > >  };
> > > >  
> > > > +static int cnl_get_dvfs_level(int cdclk, int portclk)
> > > > +{
> > > > +	if (cdclk == 168000 && portclk <= 594000)
> > > > +		return 0;
> > > > +	else if (cdclk == 336000 && portclk <= 594000)
> > > > +		return 1;
> > > > +	else
> > > > +		return 2;
> > > > +}
> > > > +
> > > >  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > >  			       struct intel_shared_dpll *pll)
> > > >  {
> > > >  	uint32_t val;
> > > > +	int ret;
> > > > +	int level;
> > > > +	int cdclk, portclk;
> > > >  
> > > >  	/* 1. Enable DPLL power in DPLL_ENABLE. */
> > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > >  	/*
> > > >  	 * 5. If the frequency will result in a change to the voltage
> > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > -	 * Sequence Before Frequency Change
> > > > -	 *
> > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > -	 * display clock frequencies
> > > > +	 * (DVFS) Sequence Before Frequency Change
> > > >  	 */
> > > > +	ret = cnl_dvfs_pre_change(dev_priv);
> > > >  
> > > >  	/* 6. Enable DPLL in DPLL_ENABLE. */
> > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > >  	/*
> > > >  	 * 8. If the frequency will result in a change to the voltage
> > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > -	 * Sequence After Frequency Change
> > > > -	 *
> > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > -	 * display clock frequencies
> > > > +	 * (DVFS) Sequence After Frequency Change
> > > >  	 */
> > > > +	if (ret == 0) {
> > > > +		cdclk = dev_priv->cdclk.hw.cdclk;
> > > > +		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > > > +		level = cnl_get_dvfs_level(cdclk, portclk);
> > > > +		cnl_dvfs_post_change(dev_priv, level);
> > > 
> > > This isn't how I imagined we'd handle this. What I was after is
> > > pre-computing the correct "dfvfs level" (or what I'd probably just call
> > > "voltage" or something like that), and then we'd just let the normal
> > > cdclk programming sequence do its thing.
> > > 
> > > Is it even safe to do this with other pipes/ports enabled? I would
> > > have assumed no, but maybe I'm mistaken?
> > 
> > Well, this is how the spec is written. So I assume it is safe to change
> > that at that point.
> > 
> > Also we enable CDCLK during boot while we have no way to compute the
> > link rate. So I don't see a better way of hadling that.
> 
> We do a full state readout, which should tell us what voltage we need.
> And if the cdclk isn't even enabled, then obviously we can't have any
> pipes running either, so we can safely enable it with any frequency
> even before the full state readout.
> 
> > So, Art is it ok in the way that I'm proposing here:
> > 
> > on core display init:
> > 
> > - dvfs-pre;
> > - enable cdclk;
> > - dvfs-post considering cdclk only and link rate 0; 
> > 
> > on pll init:
> > 
> > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-pre
> > - enable pll
> > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-post with new level.
> > 
> > This is how I read the spec, but please let me know what I'm missing.
> 
> Hmm. OK, so maybe we can change the voltage while things are up and
> running.
> 
> That said, the voltage level is a global thing, so whenever we change it
> we must consider the state of all ports, and the cdclk. This looks like
> it only considers the current port. Thus if we first enable a port that
> needs high voltage, and later a port that can make due with a lower
> voltage we'll end up with something not working.

ouch! this is true! So, what link rate should we consider on that dvfs spec?
so, always the highest avaiblable at that time?

So we cache somewhere the highest on atomic check? and then during
these dvfs sequences we just check for the highest?

> 
> We migth also end up changing the voltage multiple times if we enable
> multiple ports with one atomic operation. If things went via the normal
> cdclk path we'd just do the voltage change the once.
> 
> Additonally I don't like that the code is getting the current clock by
> means of readout in the middle the modeset. With atomic the design is
> pretty much to pass all state in explicitly, and readout is only used
> to take over the from the BIOS, and to verify that we programmed the
> hardware correctly.

hmm...
I thought the same recently when I was checking that skl Wa...
So, we probably want to cache out link rates specifically on atomic cache?
Or should we just move the functions to parse cfgcr1 on hw_state that was
created during check phase?

>  
> > 
> > Maybe I'm missing the link rate for eDP during core display init like we are currently kind of missing for SKL?
> 
> Hmm. What exactly are we missing?

I just sent a wa for skl... when doing that I saw that we are not following
the spec for initialize the display in many aspects... but specially the dpll0
link rate... We consider the vco for eDP but not the accurate link rate...
and then we update the dpll0 link rate during modeset. Apparently HW never
expects we change dpll0 link rate on runtime like we are currently doing.

But probably a discussion to move to that other patch... or not since all
involve a link rate and possibly a cache of the value.

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

Re: [PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

[Ville Syrjälä]

On Tue, Oct 17, 2017 at 10:45:22AM -0700, Rodrigo Vivi wrote:
> On Tue, Oct 17, 2017 at 05:23:20PM +0000, Ville Syrjälä wrote:
> > On Tue, Oct 17, 2017 at 09:47:05AM -0700, Rodrigo Vivi wrote:
> > > On Tue, Oct 17, 2017 at 03:44:21PM +0000, Ville Syrjälä wrote:
> > > > On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> > > > > From: "Kahola, Mika" <mika.kahola@intel.com>
> > > > > 
> > > > > Display Voltage and Frequency Switching (DVFS) is used to adjust the
> > > > > display voltage to match the display clock frequencies. If voltage is
> > > > > set too low, it will break functionality. If voltage is set too high,
> > > > > it will waste power. Voltage level is selected based on CD clock and
> > > > > DDI clock.
> > > > > 
> > > > > The sequence before frequency change is the following and it requests
> > > > > the power controller to raise voltage to maximum
> > > > > 
> > > > > - Ensure any previous GT Driver Mailbox transaction is complete.
> > > > > - Write GT Driver Mailbox Data Low = 0x3.
> > > > > - Write GT Driver Mailbox Data High = 0x0.
> > > > > - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> > > > > - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
> > > > >   Timeout after 3ms
> > > > > 
> > > > > The sequence after frequency change is the following and it requests
> > > > > the port controller to raise voltage to the requested level.
> > > > > 
> > > > > - Write GT Driver Mailbox Data Low
> > > > >  * For level 0, write 0x0
> > > > >  * For level 1, write 0x1
> > > > >  * For level 2, write 0x2
> > > > >  * For level 3, write 0x3
> > > > >    - Write GT Driver Mailbox Data High = 0x0.
> > > > >    - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > 
> > > > > For Cannonlake, the level 3 is not used and it aliases to level 2.
> > > > > 
> > > > > v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
> > > > >     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> > > > > v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
> > > > > v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
> > > > >     The port clock calculation here was only addressing DP, so let's reuse
> > > > >     the current port calculation that is already in place without any duplication.
> > > > >     Alos fix portclk <= 594000 instead of portclk < 594000.
> > > > > 
> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
> > > > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > index a2a3d93d67bd..6030fbafa580 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
> > > > >  	.dump_hw_state = bxt_dump_hw_state,
> > > > >  };
> > > > >  
> > > > > +static int cnl_get_dvfs_level(int cdclk, int portclk)
> > > > > +{
> > > > > +	if (cdclk == 168000 && portclk <= 594000)
> > > > > +		return 0;
> > > > > +	else if (cdclk == 336000 && portclk <= 594000)
> > > > > +		return 1;
> > > > > +	else
> > > > > +		return 2;
> > > > > +}
> > > > > +
> > > > >  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > >  			       struct intel_shared_dpll *pll)
> > > > >  {
> > > > >  	uint32_t val;
> > > > > +	int ret;
> > > > > +	int level;
> > > > > +	int cdclk, portclk;
> > > > >  
> > > > >  	/* 1. Enable DPLL power in DPLL_ENABLE. */
> > > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > >  	/*
> > > > >  	 * 5. If the frequency will result in a change to the voltage
> > > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > > -	 * Sequence Before Frequency Change
> > > > > -	 *
> > > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > > -	 * display clock frequencies
> > > > > +	 * (DVFS) Sequence Before Frequency Change
> > > > >  	 */
> > > > > +	ret = cnl_dvfs_pre_change(dev_priv);
> > > > >  
> > > > >  	/* 6. Enable DPLL in DPLL_ENABLE. */
> > > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > >  	/*
> > > > >  	 * 8. If the frequency will result in a change to the voltage
> > > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > > -	 * Sequence After Frequency Change
> > > > > -	 *
> > > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > > -	 * display clock frequencies
> > > > > +	 * (DVFS) Sequence After Frequency Change
> > > > >  	 */
> > > > > +	if (ret == 0) {
> > > > > +		cdclk = dev_priv->cdclk.hw.cdclk;
> > > > > +		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > > > > +		level = cnl_get_dvfs_level(cdclk, portclk);
> > > > > +		cnl_dvfs_post_change(dev_priv, level);
> > > > 
> > > > This isn't how I imagined we'd handle this. What I was after is
> > > > pre-computing the correct "dfvfs level" (or what I'd probably just call
> > > > "voltage" or something like that), and then we'd just let the normal
> > > > cdclk programming sequence do its thing.
> > > > 
> > > > Is it even safe to do this with other pipes/ports enabled? I would
> > > > have assumed no, but maybe I'm mistaken?
> > > 
> > > Well, this is how the spec is written. So I assume it is safe to change
> > > that at that point.
> > > 
> > > Also we enable CDCLK during boot while we have no way to compute the
> > > link rate. So I don't see a better way of hadling that.
> > 
> > We do a full state readout, which should tell us what voltage we need.
> > And if the cdclk isn't even enabled, then obviously we can't have any
> > pipes running either, so we can safely enable it with any frequency
> > even before the full state readout.
> > 
> > > So, Art is it ok in the way that I'm proposing here:
> > > 
> > > on core display init:
> > > 
> > > - dvfs-pre;
> > > - enable cdclk;
> > > - dvfs-post considering cdclk only and link rate 0; 
> > > 
> > > on pll init:
> > > 
> > > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-pre
> > > - enable pll
> > > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-post with new level.
> > > 
> > > This is how I read the spec, but please let me know what I'm missing.
> > 
> > Hmm. OK, so maybe we can change the voltage while things are up and
> > running.
> > 
> > That said, the voltage level is a global thing, so whenever we change it
> > we must consider the state of all ports, and the cdclk. This looks like
> > it only considers the current port. Thus if we first enable a port that
> > needs high voltage, and later a port that can make due with a lower
> > voltage we'll end up with something not working.
> 
> ouch! this is true! So, what link rate should we consider on that dvfs spec?
> so, always the highest avaiblable at that time?

Yes.

> 
> So we cache somewhere the highest on atomic check? and then during
> these dvfs sequences we just check for the highest?

IIRC I pastebinned a sketch for this at some point. Hmm. Found it, and
pushed it here:
git://github.com/vsyrjala/linux.git dvfs_voltage_2

I tried to also pimp it a little bit to not force a modeset on all
pipes if just the voltage changes.

I *think* this should work, after someone fills out the code for
DDI .compute_config() and .get_config(). But totally untested of course
so YMMV.

> 
> > 
> > We migth also end up changing the voltage multiple times if we enable
> > multiple ports with one atomic operation. If things went via the normal
> > cdclk path we'd just do the voltage change the once.
> > 
> > Additonally I don't like that the code is getting the current clock by
> > means of readout in the middle the modeset. With atomic the design is
> > pretty much to pass all state in explicitly, and readout is only used
> > to take over the from the BIOS, and to verify that we programmed the
> > hardware correctly.
> 
> hmm...
> I thought the same recently when I was checking that skl Wa...
> So, we probably want to cache out link rates specifically on atomic cache?
> Or should we just move the functions to parse cfgcr1 on hw_state that was
> created during check phase?
> 
> >  
> > > 
> > > Maybe I'm missing the link rate for eDP during core display init like we are currently kind of missing for SKL?
> > 
> > Hmm. What exactly are we missing?
> 
> I just sent a wa for skl... when doing that I saw that we are not following
> the spec for initialize the display in many aspects...

Not sure which part we're not following.

> but specially the dpll0
> link rate... We consider the vco for eDP but not the accurate link rate...
> and then we update the dpll0 link rate during modeset. Apparently HW never
> expects we change dpll0 link rate on runtime like we are currently doing.

We don't change it, except perhaps once if our initial guess was wrong.
And that could only happen when eDP wasn't enabled during boot. After
we've determined the link freq needed by eDP we should be sticking to
the same DPLL0 VCO frequency.

> 
> But probably a discussion to move to that other patch... or not since all
> involve a link rate and possibly a cache of the value.
> 
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

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

Re: [PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

[Ville Syrjälä]

On Tue, Oct 17, 2017 at 09:02:05PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 17, 2017 at 10:45:22AM -0700, Rodrigo Vivi wrote:
> > On Tue, Oct 17, 2017 at 05:23:20PM +0000, Ville Syrjälä wrote:
> > > On Tue, Oct 17, 2017 at 09:47:05AM -0700, Rodrigo Vivi wrote:
> > > > On Tue, Oct 17, 2017 at 03:44:21PM +0000, Ville Syrjälä wrote:
> > > > > On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> > > > > > From: "Kahola, Mika" <mika.kahola@intel.com>
> > > > > > 
> > > > > > Display Voltage and Frequency Switching (DVFS) is used to adjust the
> > > > > > display voltage to match the display clock frequencies. If voltage is
> > > > > > set too low, it will break functionality. If voltage is set too high,
> > > > > > it will waste power. Voltage level is selected based on CD clock and
> > > > > > DDI clock.
> > > > > > 
> > > > > > The sequence before frequency change is the following and it requests
> > > > > > the power controller to raise voltage to maximum
> > > > > > 
> > > > > > - Ensure any previous GT Driver Mailbox transaction is complete.
> > > > > > - Write GT Driver Mailbox Data Low = 0x3.
> > > > > > - Write GT Driver Mailbox Data High = 0x0.
> > > > > > - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > > - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> > > > > > - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
> > > > > >   Timeout after 3ms
> > > > > > 
> > > > > > The sequence after frequency change is the following and it requests
> > > > > > the port controller to raise voltage to the requested level.
> > > > > > 
> > > > > > - Write GT Driver Mailbox Data Low
> > > > > >  * For level 0, write 0x0
> > > > > >  * For level 1, write 0x1
> > > > > >  * For level 2, write 0x2
> > > > > >  * For level 3, write 0x3
> > > > > >    - Write GT Driver Mailbox Data High = 0x0.
> > > > > >    - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > > 
> > > > > > For Cannonlake, the level 3 is not used and it aliases to level 2.
> > > > > > 
> > > > > > v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
> > > > > >     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> > > > > > v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
> > > > > > v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
> > > > > >     The port clock calculation here was only addressing DP, so let's reuse
> > > > > >     the current port calculation that is already in place without any duplication.
> > > > > >     Alos fix portclk <= 594000 instead of portclk < 594000.
> > > > > > 
> > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
> > > > > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > index a2a3d93d67bd..6030fbafa580 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
> > > > > >  	.dump_hw_state = bxt_dump_hw_state,
> > > > > >  };
> > > > > >  
> > > > > > +static int cnl_get_dvfs_level(int cdclk, int portclk)
> > > > > > +{
> > > > > > +	if (cdclk == 168000 && portclk <= 594000)
> > > > > > +		return 0;
> > > > > > +	else if (cdclk == 336000 && portclk <= 594000)
> > > > > > +		return 1;
> > > > > > +	else
> > > > > > +		return 2;
> > > > > > +}
> > > > > > +
> > > > > >  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > >  			       struct intel_shared_dpll *pll)
> > > > > >  {
> > > > > >  	uint32_t val;
> > > > > > +	int ret;
> > > > > > +	int level;
> > > > > > +	int cdclk, portclk;
> > > > > >  
> > > > > >  	/* 1. Enable DPLL power in DPLL_ENABLE. */
> > > > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > > @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > >  	/*
> > > > > >  	 * 5. If the frequency will result in a change to the voltage
> > > > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > > > -	 * Sequence Before Frequency Change
> > > > > > -	 *
> > > > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > > > -	 * display clock frequencies
> > > > > > +	 * (DVFS) Sequence Before Frequency Change
> > > > > >  	 */
> > > > > > +	ret = cnl_dvfs_pre_change(dev_priv);
> > > > > >  
> > > > > >  	/* 6. Enable DPLL in DPLL_ENABLE. */
> > > > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > > @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > >  	/*
> > > > > >  	 * 8. If the frequency will result in a change to the voltage
> > > > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > > > -	 * Sequence After Frequency Change
> > > > > > -	 *
> > > > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > > > -	 * display clock frequencies
> > > > > > +	 * (DVFS) Sequence After Frequency Change
> > > > > >  	 */
> > > > > > +	if (ret == 0) {
> > > > > > +		cdclk = dev_priv->cdclk.hw.cdclk;
> > > > > > +		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > > > > > +		level = cnl_get_dvfs_level(cdclk, portclk);
> > > > > > +		cnl_dvfs_post_change(dev_priv, level);
> > > > > 
> > > > > This isn't how I imagined we'd handle this. What I was after is
> > > > > pre-computing the correct "dfvfs level" (or what I'd probably just call
> > > > > "voltage" or something like that), and then we'd just let the normal
> > > > > cdclk programming sequence do its thing.
> > > > > 
> > > > > Is it even safe to do this with other pipes/ports enabled? I would
> > > > > have assumed no, but maybe I'm mistaken?
> > > > 
> > > > Well, this is how the spec is written. So I assume it is safe to change
> > > > that at that point.
> > > > 
> > > > Also we enable CDCLK during boot while we have no way to compute the
> > > > link rate. So I don't see a better way of hadling that.
> > > 
> > > We do a full state readout, which should tell us what voltage we need.
> > > And if the cdclk isn't even enabled, then obviously we can't have any
> > > pipes running either, so we can safely enable it with any frequency
> > > even before the full state readout.
> > > 
> > > > So, Art is it ok in the way that I'm proposing here:
> > > > 
> > > > on core display init:
> > > > 
> > > > - dvfs-pre;
> > > > - enable cdclk;
> > > > - dvfs-post considering cdclk only and link rate 0; 
> > > > 
> > > > on pll init:
> > > > 
> > > > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-pre
> > > > - enable pll
> > > > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-post with new level.
> > > > 
> > > > This is how I read the spec, but please let me know what I'm missing.
> > > 
> > > Hmm. OK, so maybe we can change the voltage while things are up and
> > > running.
> > > 
> > > That said, the voltage level is a global thing, so whenever we change it
> > > we must consider the state of all ports, and the cdclk. This looks like
> > > it only considers the current port. Thus if we first enable a port that
> > > needs high voltage, and later a port that can make due with a lower
> > > voltage we'll end up with something not working.
> > 
> > ouch! this is true! So, what link rate should we consider on that dvfs spec?
> > so, always the highest avaiblable at that time?
> 
> Yes.
> 
> > 
> > So we cache somewhere the highest on atomic check? and then during
> > these dvfs sequences we just check for the highest?
> 
> IIRC I pastebinned a sketch for this at some point. Hmm. Found it, and
> pushed it here:
> git://github.com/vsyrjala/linux.git dvfs_voltage_2
> 
> I tried to also pimp it a little bit to not force a modeset on all
> pipes if just the voltage changes.
> 
> I *think* this should work, after someone fills out the code for
> DDI .compute_config() and .get_config(). But totally untested of course
> so YMMV.

Except I forgot about the lack of a pcode command to read out the current
voltage setting :( So I had to add some extra kludges to the state readout 
on top. Branch updated.

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

Re: [PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

[Rodrigo Vivi]

On Tue, Oct 17, 2017 at 08:36:14PM +0000, Ville Syrjälä wrote:
> On Tue, Oct 17, 2017 at 09:02:05PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 17, 2017 at 10:45:22AM -0700, Rodrigo Vivi wrote:
> > > On Tue, Oct 17, 2017 at 05:23:20PM +0000, Ville Syrjälä wrote:
> > > > On Tue, Oct 17, 2017 at 09:47:05AM -0700, Rodrigo Vivi wrote:
> > > > > On Tue, Oct 17, 2017 at 03:44:21PM +0000, Ville Syrjälä wrote:
> > > > > > On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> > > > > > > From: "Kahola, Mika" <mika.kahola@intel.com>
> > > > > > > 
> > > > > > > Display Voltage and Frequency Switching (DVFS) is used to adjust the
> > > > > > > display voltage to match the display clock frequencies. If voltage is
> > > > > > > set too low, it will break functionality. If voltage is set too high,
> > > > > > > it will waste power. Voltage level is selected based on CD clock and
> > > > > > > DDI clock.
> > > > > > > 
> > > > > > > The sequence before frequency change is the following and it requests
> > > > > > > the power controller to raise voltage to maximum
> > > > > > > 
> > > > > > > - Ensure any previous GT Driver Mailbox transaction is complete.
> > > > > > > - Write GT Driver Mailbox Data Low = 0x3.
> > > > > > > - Write GT Driver Mailbox Data High = 0x0.
> > > > > > > - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > > > - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> > > > > > > - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
> > > > > > >   Timeout after 3ms
> > > > > > > 
> > > > > > > The sequence after frequency change is the following and it requests
> > > > > > > the port controller to raise voltage to the requested level.
> > > > > > > 
> > > > > > > - Write GT Driver Mailbox Data Low
> > > > > > >  * For level 0, write 0x0
> > > > > > >  * For level 1, write 0x1
> > > > > > >  * For level 2, write 0x2
> > > > > > >  * For level 3, write 0x3
> > > > > > >    - Write GT Driver Mailbox Data High = 0x0.
> > > > > > >    - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > > > 
> > > > > > > For Cannonlake, the level 3 is not used and it aliases to level 2.
> > > > > > > 
> > > > > > > v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
> > > > > > >     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> > > > > > > v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
> > > > > > > v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
> > > > > > >     The port clock calculation here was only addressing DP, so let's reuse
> > > > > > >     the current port calculation that is already in place without any duplication.
> > > > > > >     Alos fix portclk <= 594000 instead of portclk < 594000.
> > > > > > > 
> > > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
> > > > > > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > index a2a3d93d67bd..6030fbafa580 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
> > > > > > >  	.dump_hw_state = bxt_dump_hw_state,
> > > > > > >  };
> > > > > > >  
> > > > > > > +static int cnl_get_dvfs_level(int cdclk, int portclk)
> > > > > > > +{
> > > > > > > +	if (cdclk == 168000 && portclk <= 594000)
> > > > > > > +		return 0;
> > > > > > > +	else if (cdclk == 336000 && portclk <= 594000)
> > > > > > > +		return 1;
> > > > > > > +	else
> > > > > > > +		return 2;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > > >  			       struct intel_shared_dpll *pll)
> > > > > > >  {
> > > > > > >  	uint32_t val;
> > > > > > > +	int ret;
> > > > > > > +	int level;
> > > > > > > +	int cdclk, portclk;
> > > > > > >  
> > > > > > >  	/* 1. Enable DPLL power in DPLL_ENABLE. */
> > > > > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > > > @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > > >  	/*
> > > > > > >  	 * 5. If the frequency will result in a change to the voltage
> > > > > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > > > > -	 * Sequence Before Frequency Change
> > > > > > > -	 *
> > > > > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > > > > -	 * display clock frequencies
> > > > > > > +	 * (DVFS) Sequence Before Frequency Change
> > > > > > >  	 */
> > > > > > > +	ret = cnl_dvfs_pre_change(dev_priv);
> > > > > > >  
> > > > > > >  	/* 6. Enable DPLL in DPLL_ENABLE. */
> > > > > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > > > @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > > >  	/*
> > > > > > >  	 * 8. If the frequency will result in a change to the voltage
> > > > > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > > > > -	 * Sequence After Frequency Change
> > > > > > > -	 *
> > > > > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > > > > -	 * display clock frequencies
> > > > > > > +	 * (DVFS) Sequence After Frequency Change
> > > > > > >  	 */
> > > > > > > +	if (ret == 0) {
> > > > > > > +		cdclk = dev_priv->cdclk.hw.cdclk;
> > > > > > > +		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > > > > > > +		level = cnl_get_dvfs_level(cdclk, portclk);
> > > > > > > +		cnl_dvfs_post_change(dev_priv, level);
> > > > > > 
> > > > > > This isn't how I imagined we'd handle this. What I was after is
> > > > > > pre-computing the correct "dfvfs level" (or what I'd probably just call
> > > > > > "voltage" or something like that), and then we'd just let the normal
> > > > > > cdclk programming sequence do its thing.
> > > > > > 
> > > > > > Is it even safe to do this with other pipes/ports enabled? I would
> > > > > > have assumed no, but maybe I'm mistaken?
> > > > > 
> > > > > Well, this is how the spec is written. So I assume it is safe to change
> > > > > that at that point.
> > > > > 
> > > > > Also we enable CDCLK during boot while we have no way to compute the
> > > > > link rate. So I don't see a better way of hadling that.
> > > > 
> > > > We do a full state readout, which should tell us what voltage we need.
> > > > And if the cdclk isn't even enabled, then obviously we can't have any
> > > > pipes running either, so we can safely enable it with any frequency
> > > > even before the full state readout.
> > > > 
> > > > > So, Art is it ok in the way that I'm proposing here:
> > > > > 
> > > > > on core display init:
> > > > > 
> > > > > - dvfs-pre;
> > > > > - enable cdclk;
> > > > > - dvfs-post considering cdclk only and link rate 0; 
> > > > > 
> > > > > on pll init:
> > > > > 
> > > > > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-pre
> > > > > - enable pll
> > > > > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-post with new level.
> > > > > 
> > > > > This is how I read the spec, but please let me know what I'm missing.
> > > > 
> > > > Hmm. OK, so maybe we can change the voltage while things are up and
> > > > running.
> > > > 
> > > > That said, the voltage level is a global thing, so whenever we change it
> > > > we must consider the state of all ports, and the cdclk. This looks like
> > > > it only considers the current port. Thus if we first enable a port that
> > > > needs high voltage, and later a port that can make due with a lower
> > > > voltage we'll end up with something not working.
> > > 
> > > ouch! this is true! So, what link rate should we consider on that dvfs spec?
> > > so, always the highest avaiblable at that time?
> > 
> > Yes.
> > 
> > > 
> > > So we cache somewhere the highest on atomic check? and then during
> > > these dvfs sequences we just check for the highest?
> > 
> > IIRC I pastebinned a sketch for this at some point. Hmm. Found it, and
> > pushed it here:
> > git://github.com/vsyrjala/linux.git dvfs_voltage_2
> > 
> > I tried to also pimp it a little bit to not force a modeset on all
> > pipes if just the voltage changes.
> > 
> > I *think* this should work, after someone fills out the code for
> > DDI .compute_config() and .get_config(). But totally untested of course
> > so YMMV.
> 
> Except I forgot about the lack of a pcode command to read out the current
> voltage setting :( So I had to add some extra kludges to the state readout 
> on top. Branch updated.

hmmm... thanks!

ok, we need to cache something somewhere.

But for me it seems a bit hard to associate with the port clock on the pll still.

So I wonder if we should cache the link_rate directly into dpll_hw_state,
on same places we set cfgcr0 values...
so whenever we need to figure out the minimal level we do a
for_each_pll
max_link_clock = max(max_link_clock, pll->hw_state->link_clock)
cnl_calc_voltage(cdclk, max_link_clock)

or actually move this for inside cnl_calc_voltage(cdclk).

and also I don't believe we need the full modeset to change the voltage level.
I'd like to stay as close as possible to the spec sequence.

we just need to check the minimal required with the one read from pcode or when
we are writing or to be more atomic when calculating the future link rates
we would also update the minimal level.

same when disabling any pll we would iterate on all plls that are on and check
new max link rate and consequently new min level.
and set pll_hw_state->link_rate=0.

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

Re: [PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

[Ville Syrjälä]

On Tue, Oct 17, 2017 at 04:23:07PM -0700, Rodrigo Vivi wrote:
> On Tue, Oct 17, 2017 at 08:36:14PM +0000, Ville Syrjälä wrote:
> > On Tue, Oct 17, 2017 at 09:02:05PM +0300, Ville Syrjälä wrote:
> > > On Tue, Oct 17, 2017 at 10:45:22AM -0700, Rodrigo Vivi wrote:
> > > > On Tue, Oct 17, 2017 at 05:23:20PM +0000, Ville Syrjälä wrote:
> > > > > On Tue, Oct 17, 2017 at 09:47:05AM -0700, Rodrigo Vivi wrote:
> > > > > > On Tue, Oct 17, 2017 at 03:44:21PM +0000, Ville Syrjälä wrote:
> > > > > > > On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> > > > > > > > From: "Kahola, Mika" <mika.kahola@intel.com>
> > > > > > > > 
> > > > > > > > Display Voltage and Frequency Switching (DVFS) is used to adjust the
> > > > > > > > display voltage to match the display clock frequencies. If voltage is
> > > > > > > > set too low, it will break functionality. If voltage is set too high,
> > > > > > > > it will waste power. Voltage level is selected based on CD clock and
> > > > > > > > DDI clock.
> > > > > > > > 
> > > > > > > > The sequence before frequency change is the following and it requests
> > > > > > > > the power controller to raise voltage to maximum
> > > > > > > > 
> > > > > > > > - Ensure any previous GT Driver Mailbox transaction is complete.
> > > > > > > > - Write GT Driver Mailbox Data Low = 0x3.
> > > > > > > > - Write GT Driver Mailbox Data High = 0x0.
> > > > > > > > - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > > > > - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> > > > > > > > - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
> > > > > > > >   Timeout after 3ms
> > > > > > > > 
> > > > > > > > The sequence after frequency change is the following and it requests
> > > > > > > > the port controller to raise voltage to the requested level.
> > > > > > > > 
> > > > > > > > - Write GT Driver Mailbox Data Low
> > > > > > > >  * For level 0, write 0x0
> > > > > > > >  * For level 1, write 0x1
> > > > > > > >  * For level 2, write 0x2
> > > > > > > >  * For level 3, write 0x3
> > > > > > > >    - Write GT Driver Mailbox Data High = 0x0.
> > > > > > > >    - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > > > > 
> > > > > > > > For Cannonlake, the level 3 is not used and it aliases to level 2.
> > > > > > > > 
> > > > > > > > v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
> > > > > > > >     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> > > > > > > > v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
> > > > > > > > v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
> > > > > > > >     The port clock calculation here was only addressing DP, so let's reuse
> > > > > > > >     the current port calculation that is already in place without any duplication.
> > > > > > > >     Alos fix portclk <= 594000 instead of portclk < 594000.
> > > > > > > > 
> > > > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
> > > > > > > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > > index a2a3d93d67bd..6030fbafa580 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > > @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
> > > > > > > >  	.dump_hw_state = bxt_dump_hw_state,
> > > > > > > >  };
> > > > > > > >  
> > > > > > > > +static int cnl_get_dvfs_level(int cdclk, int portclk)
> > > > > > > > +{
> > > > > > > > +	if (cdclk == 168000 && portclk <= 594000)
> > > > > > > > +		return 0;
> > > > > > > > +	else if (cdclk == 336000 && portclk <= 594000)
> > > > > > > > +		return 1;
> > > > > > > > +	else
> > > > > > > > +		return 2;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > > > >  			       struct intel_shared_dpll *pll)
> > > > > > > >  {
> > > > > > > >  	uint32_t val;
> > > > > > > > +	int ret;
> > > > > > > > +	int level;
> > > > > > > > +	int cdclk, portclk;
> > > > > > > >  
> > > > > > > >  	/* 1. Enable DPLL power in DPLL_ENABLE. */
> > > > > > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > > > > @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > > > >  	/*
> > > > > > > >  	 * 5. If the frequency will result in a change to the voltage
> > > > > > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > > > > > -	 * Sequence Before Frequency Change
> > > > > > > > -	 *
> > > > > > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > > > > > -	 * display clock frequencies
> > > > > > > > +	 * (DVFS) Sequence Before Frequency Change
> > > > > > > >  	 */
> > > > > > > > +	ret = cnl_dvfs_pre_change(dev_priv);
> > > > > > > >  
> > > > > > > >  	/* 6. Enable DPLL in DPLL_ENABLE. */
> > > > > > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > > > > @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > > > >  	/*
> > > > > > > >  	 * 8. If the frequency will result in a change to the voltage
> > > > > > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > > > > > -	 * Sequence After Frequency Change
> > > > > > > > -	 *
> > > > > > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > > > > > -	 * display clock frequencies
> > > > > > > > +	 * (DVFS) Sequence After Frequency Change
> > > > > > > >  	 */
> > > > > > > > +	if (ret == 0) {
> > > > > > > > +		cdclk = dev_priv->cdclk.hw.cdclk;
> > > > > > > > +		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > > > > > > > +		level = cnl_get_dvfs_level(cdclk, portclk);
> > > > > > > > +		cnl_dvfs_post_change(dev_priv, level);
> > > > > > > 
> > > > > > > This isn't how I imagined we'd handle this. What I was after is
> > > > > > > pre-computing the correct "dfvfs level" (or what I'd probably just call
> > > > > > > "voltage" or something like that), and then we'd just let the normal
> > > > > > > cdclk programming sequence do its thing.
> > > > > > > 
> > > > > > > Is it even safe to do this with other pipes/ports enabled? I would
> > > > > > > have assumed no, but maybe I'm mistaken?
> > > > > > 
> > > > > > Well, this is how the spec is written. So I assume it is safe to change
> > > > > > that at that point.
> > > > > > 
> > > > > > Also we enable CDCLK during boot while we have no way to compute the
> > > > > > link rate. So I don't see a better way of hadling that.
> > > > > 
> > > > > We do a full state readout, which should tell us what voltage we need.
> > > > > And if the cdclk isn't even enabled, then obviously we can't have any
> > > > > pipes running either, so we can safely enable it with any frequency
> > > > > even before the full state readout.
> > > > > 
> > > > > > So, Art is it ok in the way that I'm proposing here:
> > > > > > 
> > > > > > on core display init:
> > > > > > 
> > > > > > - dvfs-pre;
> > > > > > - enable cdclk;
> > > > > > - dvfs-post considering cdclk only and link rate 0; 
> > > > > > 
> > > > > > on pll init:
> > > > > > 
> > > > > > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-pre
> > > > > > - enable pll
> > > > > > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-post with new level.
> > > > > > 
> > > > > > This is how I read the spec, but please let me know what I'm missing.
> > > > > 
> > > > > Hmm. OK, so maybe we can change the voltage while things are up and
> > > > > running.
> > > > > 
> > > > > That said, the voltage level is a global thing, so whenever we change it
> > > > > we must consider the state of all ports, and the cdclk. This looks like
> > > > > it only considers the current port. Thus if we first enable a port that
> > > > > needs high voltage, and later a port that can make due with a lower
> > > > > voltage we'll end up with something not working.
> > > > 
> > > > ouch! this is true! So, what link rate should we consider on that dvfs spec?
> > > > so, always the highest avaiblable at that time?
> > > 
> > > Yes.
> > > 
> > > > 
> > > > So we cache somewhere the highest on atomic check? and then during
> > > > these dvfs sequences we just check for the highest?
> > > 
> > > IIRC I pastebinned a sketch for this at some point. Hmm. Found it, and
> > > pushed it here:
> > > git://github.com/vsyrjala/linux.git dvfs_voltage_2
> > > 
> > > I tried to also pimp it a little bit to not force a modeset on all
> > > pipes if just the voltage changes.
> > > 
> > > I *think* this should work, after someone fills out the code for
> > > DDI .compute_config() and .get_config(). But totally untested of course
> > > so YMMV.
> > 
> > Except I forgot about the lack of a pcode command to read out the current
> > voltage setting :( So I had to add some extra kludges to the state readout 
> > on top. Branch updated.
> 
> hmmm... thanks!
> 
> ok, we need to cache something somewhere.
> 
> But for me it seems a bit hard to associate with the port clock on the pll still.

All that should be needed with my branch is set the crtc state 
min_voltage based on port_clock. No need to think about PLLS.

> 
> So I wonder if we should cache the link_rate directly into dpll_hw_state,
> on same places we set cfgcr0 values...

I wouldn't comlicate this by involving PLLs.

> so whenever we need to figure out the minimal level we do a
> for_each_pll
> max_link_clock = max(max_link_clock, pll->hw_state->link_clock)
> cnl_calc_voltage(cdclk, max_link_clock)a
> 
> or actually move this for inside cnl_calc_voltage(cdclk).
> 
> and also I don't believe we need the full modeset to change the voltage level.

That shouldn't happen with the latest stuff I pushed to the branch.

> I'd like to stay as close as possible to the spec sequence.

That would mean more code paths to do the same thing, which means
they'll accumulate different bugs. Not something I would cherish.

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

Re: [PATCH 10/13] drm/i915/cnl: Unify dvfs level selection.

[Paulo Zanoni]

Em Qui, 2017-10-05 às 07:59 -0700, Rodrigo Vivi escreveu:
> On Wed, Oct 04, 2017 at 01:20:57PM +0000, Mika Kahola wrote:
> > On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > > When port clock is zero or undefined we base our
> > > calculation on cdclk. So, same function can be used
> > > for port clock == 0 now that we have the same default "2".
> > > 
> > > v2: s/get/new: When documenting "get" sounded ambiguous
> > >     because we could be getting the current level at pcode.
> > > 
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_cdclk.c    | 28 +++++++++++++------
> > > ----
> > > -----
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 12 +-----------
> > >  drivers/gpu/drm/i915/intel_drv.h      |  1 +
> > >  3 files changed, 15 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index 7e9c4444c844..c62d6e752fb7 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -1535,12 +1535,22 @@ void cnl_dvfs_post_change(struct
> > > drm_i915_private *dev_priv, int level)
> > >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > >  }
> > >  
> > > +int cnl_dvfs_new_level(int cdclk, int portclk)
> > > +{
> > > +	if (cdclk == 168000 && portclk <= 594000)
> > > +		return 0;
> > > +	else if (cdclk == 336000 && portclk <= 594000)
> > > +		return 1;
> > > +	else
> > > +		return 2;
> > > +}
> > > +
> > 
> > Maybe we could leave the function name as cnl_get_dvfs_level()?
> > The original one is stripped down later in this patch.
> 
> Since I was exporting I decided to use the same standard as others
> and move dvfs back a bit
> so cnl_dvfs_get_level...
> But also when I was documenting on the last patch I noticed
> that "get level" is kind of ambiguous... which level? the current
> or the new? the more natural is current, but this function also
> get the new desired/needed.
> 
> But I'm open to other suggestions that fix the ambiguity.

Get current, compute/calculate new.

> 
> >  
> > >  static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> > >  			  const struct intel_cdclk_state
> > > *cdclk_state)
> > >  {
> > >  	int cdclk = cdclk_state->cdclk;
> > >  	int vco = cdclk_state->vco;
> > > -	u32 val, divider, pcu_ack;
> > > +	u32 val, divider, level;
> > >  
> > >  	if (cnl_dvfs_pre_change(dev_priv))
> > >  		return;
> > > @@ -1561,19 +1571,6 @@ static void cnl_set_cdclk(struct
> > > drm_i915_private *dev_priv,
> > >  		break;
> > >  	}
> > >  
> > > -	switch (cdclk) {
> > > -	case 168000:
> > > -		pcu_ack = 0;
> > > -		break;
> > > -	case 336000:
> > > -		pcu_ack = 1;
> > > -		break;
> > > -	case 528000:
> > > -	default:
> > > -		pcu_ack = 2;
> > > -		break;
> > > -	}
> > > -
> > >  	if (dev_priv->cdclk.hw.vco != 0 &&
> > >  	    dev_priv->cdclk.hw.vco != vco)
> > >  		cnl_cdclk_pll_disable(dev_priv);
> > > @@ -1590,7 +1587,8 @@ static void cnl_set_cdclk(struct
> > > drm_i915_private *dev_priv,
> > >  	I915_WRITE(CDCLK_CTL, val);
> > >  
> > >  	/* inform PCU of the change */
> > > -	cnl_dvfs_post_change(dev_priv, pcu_ack);
> > > +	level = cnl_dvfs_new_level(cdclk, 0);
> > > +	cnl_dvfs_post_change(dev_priv, level);
> > >  
> > >  	intel_update_cdclk(dev_priv);
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index a71a6c396bbd..0dddbd3a7a97 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -1966,16 +1966,6 @@ static const struct intel_dpll_mgr
> > > bxt_pll_mgr
> > > = {
> > >  	.dump_hw_state = bxt_dump_hw_state,
> > >  };
> > >  
> > > -static int cnl_get_dvfs_level(int cdclk, int portclk)
> > > -{
> > > -	if (cdclk == 168000 && portclk <= 594000)
> > > -		return 0;
> > > -	else if (cdclk == 336000 && portclk <= 594000)
> > > -		return 1;
> > > -	else
> > > -		return 2;
> > > -}
> > > -
> > >  static void cnl_ddi_pll_enable(struct drm_i915_private
> > > *dev_priv,
> > >  			       struct intel_shared_dpll *pll)
> > >  {
> > > @@ -2044,7 +2034,7 @@ static void cnl_ddi_pll_enable(struct
> > > drm_i915_private *dev_priv,
> > >  	if (ret == 0) {
> > >  		cdclk = dev_priv->cdclk.hw.cdclk;
> > >  		portclk = intel_ddi_port_clock(dev_priv, pll-
> > > >id);
> > > -		level = cnl_get_dvfs_level(cdclk, portclk);
> > > +		level = cnl_dvfs_new_level(cdclk, portclk);
> > >  		cnl_dvfs_post_change(dev_priv, level);
> > >  	}
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 934ccf17f8ab..dec11d7b15ab 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1325,6 +1325,7 @@ void cnl_init_cdclk(struct drm_i915_private
> > > *dev_priv);
> > >  void cnl_uninit_cdclk(struct drm_i915_private *dev_priv);
> > >  int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
> > >  void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int
> > > level);
> > > +int cnl_dvfs_new_level(int cdclk, int portclk);
> > >  void bxt_init_cdclk(struct drm_i915_private *dev_priv);
> > >  void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
> > >  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> > 
> > -- 
> > Mika Kahola - Intel OTC
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx