[PATCH 0/4] Small clocking code refactor

[PATCH 0/4] Small clocking code refactor

[Paulo Zanoni]

Hi

I've been trying to understand the clocking code and spotted some
possible improvements. None of these changes are actually necessary
for anything, but IMHO they make the code a little easier to read and
later extend. Feel free to bikeshed or even NAK my proposals.

Thanks,
Paulo

Paulo Zanoni (4):
  drm/i915: kill {bdw,bxt}_modeset_calc_cdclk
  drm/i915: add intel_calc_cdclk()
  drm/i915: remove potentially confusing IS_G4X checks
  drm/i915: reorganize the get_cdclk assignment

 drivers/gpu/drm/i915/intel_cdclk.c | 257 ++++++++++++++++---------------------
 1 file changed, 112 insertions(+), 145 deletions(-)

-- 
2.7.4

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

[PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk

[Paulo Zanoni]

The functions are pretty much the same, except for the CDCLK and VCO
calculations. Add BDW support to vlv_modeset_calc_cdclk() and add
BXT/GLK support to skl_modeset_calc_cdclk(). The two reamining
functions are still very similar, except for the fact that the vlv
version doesn't touch the VCO. Further patches could unify them even
more if that's desired.

While at it, merge some lines that can fit 80 columns in those
functions.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 120 ++++++++++---------------------------
 1 file changed, 30 insertions(+), 90 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index d643c0c..d505ff1 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1499,45 +1499,18 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
 static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
-	int max_pixclk = intel_max_pixel_rate(state);
-	struct intel_atomic_state *intel_state =
-		to_intel_atomic_state(state);
-	int cdclk;
-
-	cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
-
-	if (cdclk > dev_priv->max_cdclk_freq) {
-		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
-			      cdclk, dev_priv->max_cdclk_freq);
-		return -EINVAL;
-	}
-
-	intel_state->cdclk.logical.cdclk = cdclk;
-
-	if (!intel_state->active_crtcs) {
-		cdclk = vlv_calc_cdclk(dev_priv, 0);
-
-		intel_state->cdclk.actual.cdclk = cdclk;
-	} else {
-		intel_state->cdclk.actual =
-			intel_state->cdclk.logical;
-	}
-
-	return 0;
-}
-
-static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	int max_pixclk = intel_max_pixel_rate(state);
 	int cdclk;
 
 	/*
-	 * FIXME should also account for plane ratio
-	 * once 64bpp pixel formats are supported.
+	 * FIXME: Broadwell should also account for plane ratio once 64bpp pixel
+	 * formats are supported.
 	 */
-	cdclk = bdw_calc_cdclk(max_pixclk);
+	if (IS_BROADWELL(dev_priv))
+		cdclk = bdw_calc_cdclk(max_pixclk);
+	else
+		cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
 
 	if (cdclk > dev_priv->max_cdclk_freq) {
 		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
@@ -1548,12 +1521,14 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
 	intel_state->cdclk.logical.cdclk = cdclk;
 
 	if (!intel_state->active_crtcs) {
-		cdclk = bdw_calc_cdclk(0);
+		if (IS_BROADWELL(dev_priv))
+			cdclk = bdw_calc_cdclk(0);
+		else
+			cdclk = vlv_calc_cdclk(dev_priv, 0);
 
 		intel_state->cdclk.actual.cdclk = cdclk;
 	} else {
-		intel_state->cdclk.actual =
-			intel_state->cdclk.logical;
+		intel_state->cdclk.actual = intel_state->cdclk.logical;
 	}
 
 	return 0;
@@ -1561,57 +1536,26 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
 
 static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
 {
-	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
-	const int max_pixclk = intel_max_pixel_rate(state);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	int max_pixclk = intel_max_pixel_rate(state);
 	int cdclk, vco;
 
-	vco = intel_state->cdclk.logical.vco;
-	if (!vco)
-		vco = dev_priv->skl_preferred_vco_freq;
-
 	/*
-	 * FIXME should also account for plane ratio
-	 * once 64bpp pixel formats are supported.
+	 * FIXME: Skylake/Kabylake should also account for plane ratio once
+	 * 64bpp pixel formats are supported.
 	 */
-	cdclk = skl_calc_cdclk(max_pixclk, vco);
-
-	if (cdclk > dev_priv->max_cdclk_freq) {
-		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
-			      cdclk, dev_priv->max_cdclk_freq);
-		return -EINVAL;
-	}
-
-	intel_state->cdclk.logical.vco = vco;
-	intel_state->cdclk.logical.cdclk = cdclk;
-
-	if (!intel_state->active_crtcs) {
-		cdclk = skl_calc_cdclk(0, vco);
-
-		intel_state->cdclk.actual.vco = vco;
-		intel_state->cdclk.actual.cdclk = cdclk;
-	} else {
-		intel_state->cdclk.actual =
-			intel_state->cdclk.logical;
-	}
-
-	return 0;
-}
-
-static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->dev);
-	int max_pixclk = intel_max_pixel_rate(state);
-	struct intel_atomic_state *intel_state =
-		to_intel_atomic_state(state);
-	int cdclk, vco;
-
 	if (IS_GEMINILAKE(dev_priv)) {
 		cdclk = glk_calc_cdclk(max_pixclk);
 		vco = glk_de_pll_vco(dev_priv, cdclk);
-	} else {
+	} else if (IS_BROXTON(dev_priv)) {
 		cdclk = bxt_calc_cdclk(max_pixclk);
 		vco = bxt_de_pll_vco(dev_priv, cdclk);
+	} else {
+		vco = intel_state->cdclk.logical.vco;
+		if (!vco)
+			vco = dev_priv->skl_preferred_vco_freq;
+		cdclk = skl_calc_cdclk(max_pixclk, vco);
 	}
 
 	if (cdclk > dev_priv->max_cdclk_freq) {
@@ -1627,16 +1571,17 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
 		if (IS_GEMINILAKE(dev_priv)) {
 			cdclk = glk_calc_cdclk(0);
 			vco = glk_de_pll_vco(dev_priv, cdclk);
-		} else {
+		} else if (IS_BROXTON(dev_priv)) {
 			cdclk = bxt_calc_cdclk(0);
 			vco = bxt_de_pll_vco(dev_priv, cdclk);
+		} else {
+			cdclk = skl_calc_cdclk(0, vco);
 		}
 
 		intel_state->cdclk.actual.vco = vco;
 		intel_state->cdclk.actual.cdclk = cdclk;
 	} else {
-		intel_state->cdclk.actual =
-			intel_state->cdclk.logical;
+		intel_state->cdclk.actual = intel_state->cdclk.logical;
 	}
 
 	return 0;
@@ -1823,24 +1768,19 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
 {
 	if (IS_CHERRYVIEW(dev_priv)) {
 		dev_priv->display.set_cdclk = chv_set_cdclk;
-		dev_priv->display.modeset_calc_cdclk =
-			vlv_modeset_calc_cdclk;
+		dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk;
 	} else if (IS_VALLEYVIEW(dev_priv)) {
 		dev_priv->display.set_cdclk = vlv_set_cdclk;
-		dev_priv->display.modeset_calc_cdclk =
-			vlv_modeset_calc_cdclk;
+		dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk;
 	} else if (IS_BROADWELL(dev_priv)) {
 		dev_priv->display.set_cdclk = bdw_set_cdclk;
-		dev_priv->display.modeset_calc_cdclk =
-			bdw_modeset_calc_cdclk;
+		dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk;
 	} else if (IS_GEN9_LP(dev_priv)) {
 		dev_priv->display.set_cdclk = bxt_set_cdclk;
-		dev_priv->display.modeset_calc_cdclk =
-			bxt_modeset_calc_cdclk;
+		dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk;
 	} else if (IS_GEN9_BC(dev_priv)) {
 		dev_priv->display.set_cdclk = skl_set_cdclk;
-		dev_priv->display.modeset_calc_cdclk =
-			skl_modeset_calc_cdclk;
+		dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk;
 	}
 
 	if (IS_GEN9_BC(dev_priv))
-- 
2.7.4

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

[PATCH 2/4] drm/i915: add intel_calc_cdclk()

[Paulo Zanoni]

Each x_modeset_calc_cdclk() has to do the same platform checks twice,
so extract them to a single function. This way, the platform checks
are all in the same place, and the platform-common code gets rid of
all the platform-specific checks, which IMHO makes the code easier to
read.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 84 ++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index d505ff1..6efc5f4 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1496,6 +1496,47 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
 	return max_pixel_rate;
 }
 
+static void intel_calc_cdclk(struct intel_atomic_state *state, int max_pixclk,
+			     int *cdclk, int *vco)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+
+	switch (INTEL_INFO(dev_priv)->platform) {
+	case INTEL_VALLEYVIEW:
+	case INTEL_CHERRYVIEW:
+		*cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
+		break;
+	case INTEL_BROADWELL:
+		/*
+		 * FIXME: should also account for plane ratio once 64bpp pixel
+		 * formats are supported.
+		 */
+		*cdclk = bdw_calc_cdclk(max_pixclk);
+		break;
+	case INTEL_SKYLAKE:
+	case INTEL_KABYLAKE:
+		/*
+		 * FIXME: should also account for plane ratio once 64bpp pixel
+		 * formats are supported.
+		 */
+		*vco = state->cdclk.logical.vco;
+		if (!*vco)
+			*vco = dev_priv->skl_preferred_vco_freq;
+		*cdclk = skl_calc_cdclk(max_pixclk, *vco);
+		break;
+	case INTEL_BROXTON:
+		*cdclk = bxt_calc_cdclk(max_pixclk);
+		*vco = bxt_de_pll_vco(dev_priv, *cdclk);
+		break;
+	case INTEL_GEMINILAKE:
+		*cdclk = glk_calc_cdclk(max_pixclk);
+		*vco = glk_de_pll_vco(dev_priv, *cdclk);
+		break;
+	default:
+		MISSING_CASE(INTEL_INFO(dev_priv)->platform);
+	}
+}
+
 static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
@@ -1503,14 +1544,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
 	int max_pixclk = intel_max_pixel_rate(state);
 	int cdclk;
 
-	/*
-	 * FIXME: Broadwell should also account for plane ratio once 64bpp pixel
-	 * formats are supported.
-	 */
-	if (IS_BROADWELL(dev_priv))
-		cdclk = bdw_calc_cdclk(max_pixclk);
-	else
-		cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
+	intel_calc_cdclk(intel_state, max_pixclk, &cdclk, NULL);
 
 	if (cdclk > dev_priv->max_cdclk_freq) {
 		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
@@ -1521,11 +1555,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
 	intel_state->cdclk.logical.cdclk = cdclk;
 
 	if (!intel_state->active_crtcs) {
-		if (IS_BROADWELL(dev_priv))
-			cdclk = bdw_calc_cdclk(0);
-		else
-			cdclk = vlv_calc_cdclk(dev_priv, 0);
-
+		intel_calc_cdclk(intel_state, 0, &cdclk, NULL);
 		intel_state->cdclk.actual.cdclk = cdclk;
 	} else {
 		intel_state->cdclk.actual = intel_state->cdclk.logical;
@@ -1541,22 +1571,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
 	int max_pixclk = intel_max_pixel_rate(state);
 	int cdclk, vco;
 
-	/*
-	 * FIXME: Skylake/Kabylake should also account for plane ratio once
-	 * 64bpp pixel formats are supported.
-	 */
-	if (IS_GEMINILAKE(dev_priv)) {
-		cdclk = glk_calc_cdclk(max_pixclk);
-		vco = glk_de_pll_vco(dev_priv, cdclk);
-	} else if (IS_BROXTON(dev_priv)) {
-		cdclk = bxt_calc_cdclk(max_pixclk);
-		vco = bxt_de_pll_vco(dev_priv, cdclk);
-	} else {
-		vco = intel_state->cdclk.logical.vco;
-		if (!vco)
-			vco = dev_priv->skl_preferred_vco_freq;
-		cdclk = skl_calc_cdclk(max_pixclk, vco);
-	}
+	intel_calc_cdclk(intel_state, max_pixclk, &cdclk, &vco);
 
 	if (cdclk > dev_priv->max_cdclk_freq) {
 		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
@@ -1568,16 +1583,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
 	intel_state->cdclk.logical.cdclk = cdclk;
 
 	if (!intel_state->active_crtcs) {
-		if (IS_GEMINILAKE(dev_priv)) {
-			cdclk = glk_calc_cdclk(0);
-			vco = glk_de_pll_vco(dev_priv, cdclk);
-		} else if (IS_BROXTON(dev_priv)) {
-			cdclk = bxt_calc_cdclk(0);
-			vco = bxt_de_pll_vco(dev_priv, cdclk);
-		} else {
-			cdclk = skl_calc_cdclk(0, vco);
-		}
-
+		intel_calc_cdclk(intel_state, 0, &cdclk, &vco);
 		intel_state->cdclk.actual.vco = vco;
 		intel_state->cdclk.actual.cdclk = cdclk;
 	} else {
-- 
2.7.4

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

[PATCH 3/4] drm/i915: remove potentially confusing IS_G4X checks

[Paulo Zanoni]

The IS_G4X macro is defined as IS_G45 || IS_GM45. We have two points
in our code where we have an if statement checking for GM45 followed
by an else if statement checking for IS_G4X. This can be confusing
since the IS_G4X check won't be catching the previously-checked GM45.
Someone quickly trying to check which functions run on each platform
may end up getting confused while reading the code.

Fix the potential confusion by limiting the else if statements to only
check for the platform that was not already checked earlier in the if
ladder.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 6efc5f4..7c92dc7 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -223,7 +223,7 @@ static unsigned int intel_hpll_vco(struct drm_i915_private *dev_priv)
 	/* FIXME other chipsets? */
 	if (IS_GM45(dev_priv))
 		vco_table = ctg_vco;
-	else if (IS_G4X(dev_priv))
+	else if (IS_G45(dev_priv))
 		vco_table = elk_vco;
 	else if (IS_I965GM(dev_priv))
 		vco_table = cl_vco;
@@ -1805,7 +1805,7 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk;
 	else if (IS_GM45(dev_priv))
 		dev_priv->display.get_cdclk = gm45_get_cdclk;
-	else if (IS_G4X(dev_priv))
+	else if (IS_G45(dev_priv))
 		dev_priv->display.get_cdclk = g33_get_cdclk;
 	else if (IS_I965GM(dev_priv))
 		dev_priv->display.get_cdclk = i965gm_get_cdclk;
-- 
2.7.4

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

[PATCH 4/4] drm/i915: reorganize the get_cdclk assignment

[Paulo Zanoni]

Possible problems of the current if-ladder:
  - It's a huge if ladder with almost a different check for each of
    our platforms.
  - It mixes 3 different types of checks: IS_GENX, IS_PLATFORM and
    IS_GROUP_OF_PLATFORMS.
  - As demonstrated by the recent IS_G4X commit, it's not easy to be
    sure if a platform down on the list isn't also checked earlier.
  - As demonstrated by the WARN at the end, it's not easy to be sure
    if we're actually checking for every single platform.

Possible advantages of the new switch statement:
  - It may be easier for the compiler to optimize stuff (I didn't
    check this), especially since the values are labels of an enum.
  - The compiler will tell us in case we miss some platform.
  - All platforms are explicitly there instead of maybe hidden in some
    check for a certain group of platforms such as IS_GEN9_BC.

Possible disadvantages with the new code:
  - A few lines bigger.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 103 ++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 7c92dc7..58a2f5c 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1789,49 +1789,70 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk;
 	}
 
-	if (IS_GEN9_BC(dev_priv))
-		dev_priv->display.get_cdclk = skl_get_cdclk;
-	else if (IS_GEN9_LP(dev_priv))
-		dev_priv->display.get_cdclk = bxt_get_cdclk;
-	else if (IS_BROADWELL(dev_priv))
-		dev_priv->display.get_cdclk = bdw_get_cdclk;
-	else if (IS_HASWELL(dev_priv))
-		dev_priv->display.get_cdclk = hsw_get_cdclk;
-	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		dev_priv->display.get_cdclk = vlv_get_cdclk;
-	else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
+	switch (INTEL_INFO(dev_priv)->platform) {
+	case INTEL_PLATFORM_UNINITIALIZED:
+		MISSING_CASE(INTEL_INFO(dev_priv)->platform);
+		/* Fall through. */
+	case INTEL_I830:
+		dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk;
+		break;
+	case INTEL_I845G:
+		dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk;
+		break;
+	case INTEL_I85X:
+		dev_priv->display.get_cdclk = i85x_get_cdclk;
+		break;
+	case INTEL_I865G:
+		dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk;
+		break;
+	case INTEL_I915G:
+		dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk;
+		break;
+	case INTEL_I915GM:
+		dev_priv->display.get_cdclk = i915gm_get_cdclk;
+		break;
+	case INTEL_I945G:
+	case INTEL_I965G:
+	case INTEL_SANDYBRIDGE:
+	case INTEL_IVYBRIDGE:
 		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
-	else if (IS_GEN5(dev_priv))
-		dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk;
-	else if (IS_GM45(dev_priv))
-		dev_priv->display.get_cdclk = gm45_get_cdclk;
-	else if (IS_G45(dev_priv))
+		break;
+	case INTEL_I945GM:
+		dev_priv->display.get_cdclk = i945gm_get_cdclk;
+		break;
+	case INTEL_G33:
+	case INTEL_G45:
 		dev_priv->display.get_cdclk = g33_get_cdclk;
-	else if (IS_I965GM(dev_priv))
-		dev_priv->display.get_cdclk = i965gm_get_cdclk;
-	else if (IS_I965G(dev_priv))
-		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
-	else if (IS_PINEVIEW(dev_priv))
+		break;
+	case INTEL_PINEVIEW:
 		dev_priv->display.get_cdclk = pnv_get_cdclk;
-	else if (IS_G33(dev_priv))
-		dev_priv->display.get_cdclk = g33_get_cdclk;
-	else if (IS_I945GM(dev_priv))
-		dev_priv->display.get_cdclk = i945gm_get_cdclk;
-	else if (IS_I945G(dev_priv))
-		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
-	else if (IS_I915GM(dev_priv))
-		dev_priv->display.get_cdclk = i915gm_get_cdclk;
-	else if (IS_I915G(dev_priv))
-		dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk;
-	else if (IS_I865G(dev_priv))
-		dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk;
-	else if (IS_I85X(dev_priv))
-		dev_priv->display.get_cdclk = i85x_get_cdclk;
-	else if (IS_I845G(dev_priv))
-		dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk;
-	else { /* 830 */
-		WARN(!IS_I830(dev_priv),
-		     "Unknown platform. Assuming 133 MHz CDCLK\n");
-		dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk;
+		break;
+	case INTEL_I965GM:
+		dev_priv->display.get_cdclk = i965gm_get_cdclk;
+		break;
+	case INTEL_GM45:
+		dev_priv->display.get_cdclk = gm45_get_cdclk;
+		break;
+	case INTEL_IRONLAKE:
+		dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk;
+		break;
+	case INTEL_VALLEYVIEW:
+	case INTEL_CHERRYVIEW:
+		dev_priv->display.get_cdclk = vlv_get_cdclk;
+		break;
+	case INTEL_HASWELL:
+		dev_priv->display.get_cdclk = hsw_get_cdclk;
+		break;
+	case INTEL_BROADWELL:
+		dev_priv->display.get_cdclk = bdw_get_cdclk;
+		break;
+	case INTEL_SKYLAKE:
+	case INTEL_KABYLAKE:
+		dev_priv->display.get_cdclk = skl_get_cdclk;
+		break;
+	case INTEL_BROXTON:
+	case INTEL_GEMINILAKE:
+		dev_priv->display.get_cdclk = bxt_get_cdclk;
+		break;
 	}
 }
-- 
2.7.4

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

Re: [PATCH 2/4] drm/i915: add intel_calc_cdclk()

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 11:22:05AM -0200, Paulo Zanoni wrote:
> Each x_modeset_calc_cdclk() has to do the same platform checks twice,
> so extract them to a single function. This way, the platform checks
> are all in the same place, and the platform-common code gets rid of
> all the platform-specific checks, which IMHO makes the code easier to
> read.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 84 ++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index d505ff1..6efc5f4 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1496,6 +1496,47 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
>  	return max_pixel_rate;
>  }
>  
> +static void intel_calc_cdclk(struct intel_atomic_state *state, int max_pixclk,
> +			     int *cdclk, int *vco)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +
> +	switch (INTEL_INFO(dev_priv)->platform) {
> +	case INTEL_VALLEYVIEW:
> +	case INTEL_CHERRYVIEW:
> +		*cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> +		break;
> +	case INTEL_BROADWELL:
> +		/*
> +		 * FIXME: should also account for plane ratio once 64bpp pixel
> +		 * formats are supported.
> +		 */
> +		*cdclk = bdw_calc_cdclk(max_pixclk);
> +		break;
> +	case INTEL_SKYLAKE:
> +	case INTEL_KABYLAKE:
> +		/*
> +		 * FIXME: should also account for plane ratio once 64bpp pixel
> +		 * formats are supported.
> +		 */
> +		*vco = state->cdclk.logical.vco;
> +		if (!*vco)
> +			*vco = dev_priv->skl_preferred_vco_freq;
> +		*cdclk = skl_calc_cdclk(max_pixclk, *vco);
> +		break;
> +	case INTEL_BROXTON:
> +		*cdclk = bxt_calc_cdclk(max_pixclk);
> +		*vco = bxt_de_pll_vco(dev_priv, *cdclk);
> +		break;
> +	case INTEL_GEMINILAKE:
> +		*cdclk = glk_calc_cdclk(max_pixclk);
> +		*vco = glk_de_pll_vco(dev_priv, *cdclk);
> +		break;
> +	default:
> +		MISSING_CASE(INTEL_INFO(dev_priv)->platform);
> +	}
> +}

How about just replacing the .modeset_calc_cdclk() vfunc with a slightly
lower level vfunc that just computes the cdclk/vco/whatever without
containing the active_crtcs logic?

Then we should have just

intel_modeset_calc_cdclk()
{
	.calc_cdclk(logical, max_pixclk);

	/*
	 * maybe keep the max_cdclk check here, although it that
	 * happens I think we have a bug somewhere, so perhaps
	 * just convert it into a WARN, or drop entirely.
	 */

	if (!active_crtcs)
		.calc_cdclk(actual, 0);
	else
		actual = logical;
}


> +
>  static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> @@ -1503,14 +1544,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	int max_pixclk = intel_max_pixel_rate(state);
>  	int cdclk;
>  
> -	/*
> -	 * FIXME: Broadwell should also account for plane ratio once 64bpp pixel
> -	 * formats are supported.
> -	 */
> -	if (IS_BROADWELL(dev_priv))
> -		cdclk = bdw_calc_cdclk(max_pixclk);
> -	else
> -		cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> +	intel_calc_cdclk(intel_state, max_pixclk, &cdclk, NULL);
>  
>  	if (cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1521,11 +1555,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
>  	if (!intel_state->active_crtcs) {
> -		if (IS_BROADWELL(dev_priv))
> -			cdclk = bdw_calc_cdclk(0);
> -		else
> -			cdclk = vlv_calc_cdclk(dev_priv, 0);
> -
> +		intel_calc_cdclk(intel_state, 0, &cdclk, NULL);
>  		intel_state->cdclk.actual.cdclk = cdclk;
>  	} else {
>  		intel_state->cdclk.actual = intel_state->cdclk.logical;
> @@ -1541,22 +1571,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	int max_pixclk = intel_max_pixel_rate(state);
>  	int cdclk, vco;
>  
> -	/*
> -	 * FIXME: Skylake/Kabylake should also account for plane ratio once
> -	 * 64bpp pixel formats are supported.
> -	 */
> -	if (IS_GEMINILAKE(dev_priv)) {
> -		cdclk = glk_calc_cdclk(max_pixclk);
> -		vco = glk_de_pll_vco(dev_priv, cdclk);
> -	} else if (IS_BROXTON(dev_priv)) {
> -		cdclk = bxt_calc_cdclk(max_pixclk);
> -		vco = bxt_de_pll_vco(dev_priv, cdclk);
> -	} else {
> -		vco = intel_state->cdclk.logical.vco;
> -		if (!vco)
> -			vco = dev_priv->skl_preferred_vco_freq;
> -		cdclk = skl_calc_cdclk(max_pixclk, vco);
> -	}
> +	intel_calc_cdclk(intel_state, max_pixclk, &cdclk, &vco);
>  
>  	if (cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1568,16 +1583,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
>  	if (!intel_state->active_crtcs) {
> -		if (IS_GEMINILAKE(dev_priv)) {
> -			cdclk = glk_calc_cdclk(0);
> -			vco = glk_de_pll_vco(dev_priv, cdclk);
> -		} else if (IS_BROXTON(dev_priv)) {
> -			cdclk = bxt_calc_cdclk(0);
> -			vco = bxt_de_pll_vco(dev_priv, cdclk);
> -		} else {
> -			cdclk = skl_calc_cdclk(0, vco);
> -		}
> -
> +		intel_calc_cdclk(intel_state, 0, &cdclk, &vco);
>  		intel_state->cdclk.actual.vco = vco;
>  		intel_state->cdclk.actual.cdclk = cdclk;
>  	} else {
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 11:22:04AM -0200, Paulo Zanoni wrote:
> The functions are pretty much the same, except for the CDCLK and VCO
> calculations. Add BDW support to vlv_modeset_calc_cdclk() and add
> BXT/GLK support to skl_modeset_calc_cdclk(). The two reamining
> functions are still very similar, except for the fact that the vlv
> version doesn't touch the VCO. Further patches could unify them even
> more if that's desired.
> 
> While at it, merge some lines that can fit 80 columns in those
> functions.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 120 ++++++++++---------------------------
>  1 file changed, 30 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index d643c0c..d505ff1 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1499,45 +1499,18 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
>  static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	int max_pixclk = intel_max_pixel_rate(state);
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(state);
> -	int cdclk;
> -
> -	cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> -
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> -		return -EINVAL;
> -	}
> -
> -	intel_state->cdclk.logical.cdclk = cdclk;
> -
> -	if (!intel_state->active_crtcs) {
> -		cdclk = vlv_calc_cdclk(dev_priv, 0);
> -
> -		intel_state->cdclk.actual.cdclk = cdclk;
> -	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> -	}
> -
> -	return 0;
> -}
> -
> -static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	int max_pixclk = intel_max_pixel_rate(state);
>  	int cdclk;
>  
>  	/*
> -	 * FIXME should also account for plane ratio
> -	 * once 64bpp pixel formats are supported.
> +	 * FIXME: Broadwell should also account for plane ratio once 64bpp pixel
> +	 * formats are supported.

BTW these restrictions affect pretty much all platforms, so specifying the
platforms in the comment is rather redundant.

>  	 */
> -	cdclk = bdw_calc_cdclk(max_pixclk);
> +	if (IS_BROADWELL(dev_priv))
> +		cdclk = bdw_calc_cdclk(max_pixclk);
> +	else
> +		cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
>  
>  	if (cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1548,12 +1521,14 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
>  	if (!intel_state->active_crtcs) {
> -		cdclk = bdw_calc_cdclk(0);
> +		if (IS_BROADWELL(dev_priv))
> +			cdclk = bdw_calc_cdclk(0);
> +		else
> +			cdclk = vlv_calc_cdclk(dev_priv, 0);
>  
>  		intel_state->cdclk.actual.cdclk = cdclk;
>  	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> +		intel_state->cdclk.actual = intel_state->cdclk.logical;
>  	}
>  
>  	return 0;
> @@ -1561,57 +1536,26 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>  
>  static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
> -	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	const int max_pixclk = intel_max_pixel_rate(state);
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	int max_pixclk = intel_max_pixel_rate(state);
>  	int cdclk, vco;
>  
> -	vco = intel_state->cdclk.logical.vco;
> -	if (!vco)
> -		vco = dev_priv->skl_preferred_vco_freq;
> -
>  	/*
> -	 * FIXME should also account for plane ratio
> -	 * once 64bpp pixel formats are supported.
> +	 * FIXME: Skylake/Kabylake should also account for plane ratio once
> +	 * 64bpp pixel formats are supported.
>  	 */
> -	cdclk = skl_calc_cdclk(max_pixclk, vco);
> -
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> -		return -EINVAL;
> -	}
> -
> -	intel_state->cdclk.logical.vco = vco;
> -	intel_state->cdclk.logical.cdclk = cdclk;
> -
> -	if (!intel_state->active_crtcs) {
> -		cdclk = skl_calc_cdclk(0, vco);
> -
> -		intel_state->cdclk.actual.vco = vco;
> -		intel_state->cdclk.actual.cdclk = cdclk;
> -	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> -	}
> -
> -	return 0;
> -}
> -
> -static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	int max_pixclk = intel_max_pixel_rate(state);
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(state);
> -	int cdclk, vco;
> -
>  	if (IS_GEMINILAKE(dev_priv)) {
>  		cdclk = glk_calc_cdclk(max_pixclk);
>  		vco = glk_de_pll_vco(dev_priv, cdclk);
> -	} else {
> +	} else if (IS_BROXTON(dev_priv)) {
>  		cdclk = bxt_calc_cdclk(max_pixclk);
>  		vco = bxt_de_pll_vco(dev_priv, cdclk);
> +	} else {
> +		vco = intel_state->cdclk.logical.vco;
> +		if (!vco)
> +			vco = dev_priv->skl_preferred_vco_freq;
> +		cdclk = skl_calc_cdclk(max_pixclk, vco);
>  	}
>  
>  	if (cdclk > dev_priv->max_cdclk_freq) {
> @@ -1627,16 +1571,17 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  		if (IS_GEMINILAKE(dev_priv)) {
>  			cdclk = glk_calc_cdclk(0);
>  			vco = glk_de_pll_vco(dev_priv, cdclk);
> -		} else {
> +		} else if (IS_BROXTON(dev_priv)) {
>  			cdclk = bxt_calc_cdclk(0);
>  			vco = bxt_de_pll_vco(dev_priv, cdclk);
> +		} else {
> +			cdclk = skl_calc_cdclk(0, vco);
>  		}
>  
>  		intel_state->cdclk.actual.vco = vco;
>  		intel_state->cdclk.actual.cdclk = cdclk;
>  	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> +		intel_state->cdclk.actual = intel_state->cdclk.logical;
>  	}
>  
>  	return 0;
> @@ -1823,24 +1768,19 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
>  {
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		dev_priv->display.set_cdclk = chv_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			vlv_modeset_calc_cdclk;
> +		dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk;
>  	} else if (IS_VALLEYVIEW(dev_priv)) {
>  		dev_priv->display.set_cdclk = vlv_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			vlv_modeset_calc_cdclk;
> +		dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk;
>  	} else if (IS_BROADWELL(dev_priv)) {
>  		dev_priv->display.set_cdclk = bdw_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			bdw_modeset_calc_cdclk;
> +		dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk;
>  	} else if (IS_GEN9_LP(dev_priv)) {
>  		dev_priv->display.set_cdclk = bxt_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			bxt_modeset_calc_cdclk;
> +		dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk;
>  	} else if (IS_GEN9_BC(dev_priv)) {
>  		dev_priv->display.set_cdclk = skl_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			skl_modeset_calc_cdclk;
> +		dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk;
>  	}
>  
>  	if (IS_GEN9_BC(dev_priv))
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 11:22:07AM -0200, Paulo Zanoni wrote:
> Possible problems of the current if-ladder:
>   - It's a huge if ladder with almost a different check for each of
>     our platforms.
>   - It mixes 3 different types of checks: IS_GENX, IS_PLATFORM and
>     IS_GROUP_OF_PLATFORMS.
>   - As demonstrated by the recent IS_G4X commit, it's not easy to be
>     sure if a platform down on the list isn't also checked earlier.
>   - As demonstrated by the WARN at the end, it's not easy to be sure
>     if we're actually checking for every single platform.
> 
> Possible advantages of the new switch statement:
>   - It may be easier for the compiler to optimize stuff (I didn't
>     check this), especially since the values are labels of an enum.
>   - The compiler will tell us in case we miss some platform.
>   - All platforms are explicitly there instead of maybe hidden in some
>     check for a certain group of platforms such as IS_GEN9_BC.

Performance is a bit of a moot point since this is run exaclty once, but
the IS_GEN9_BC() stuff I tend to agree with. I don't really like those
macros at all since they don't actully mean anything as far as the
hardware features go.

> 
> Possible disadvantages with the new code:
>   - A few lines bigger.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 103 ++++++++++++++++++++++---------------
>  1 file changed, 62 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 7c92dc7..58a2f5c 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1789,49 +1789,70 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
>  		dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk;
>  	}
>  
> -	if (IS_GEN9_BC(dev_priv))
> -		dev_priv->display.get_cdclk = skl_get_cdclk;
> -	else if (IS_GEN9_LP(dev_priv))
> -		dev_priv->display.get_cdclk = bxt_get_cdclk;
> -	else if (IS_BROADWELL(dev_priv))
> -		dev_priv->display.get_cdclk = bdw_get_cdclk;
> -	else if (IS_HASWELL(dev_priv))
> -		dev_priv->display.get_cdclk = hsw_get_cdclk;
> -	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		dev_priv->display.get_cdclk = vlv_get_cdclk;
> -	else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> +	switch (INTEL_INFO(dev_priv)->platform) {
> +	case INTEL_PLATFORM_UNINITIALIZED:

Just default: ?

> +		MISSING_CASE(INTEL_INFO(dev_priv)->platform);
> +		/* Fall through. */
> +	case INTEL_I830:
> +		dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk;
> +		break;
> +	case INTEL_I845G:
> +		dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk;
> +		break;
> +	case INTEL_I85X:
> +		dev_priv->display.get_cdclk = i85x_get_cdclk;
> +		break;
> +	case INTEL_I865G:
> +		dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk;
> +		break;
> +	case INTEL_I915G:
> +		dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk;
> +		break;
> +	case INTEL_I915GM:
> +		dev_priv->display.get_cdclk = i915gm_get_cdclk;
> +		break;
> +	case INTEL_I945G:
> +	case INTEL_I965G:
> +	case INTEL_SANDYBRIDGE:
> +	case INTEL_IVYBRIDGE:

I don't particularly like this disorder. I just managed to get the
list into some sort of sane order recently.

>  		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
> -	else if (IS_GEN5(dev_priv))
> -		dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk;
> -	else if (IS_GM45(dev_priv))
> -		dev_priv->display.get_cdclk = gm45_get_cdclk;
> -	else if (IS_G45(dev_priv))
> +		break;
> +	case INTEL_I945GM:
> +		dev_priv->display.get_cdclk = i945gm_get_cdclk;
> +		break;
> +	case INTEL_G33:
> +	case INTEL_G45:

More disorder.

>  		dev_priv->display.get_cdclk = g33_get_cdclk;
> -	else if (IS_I965GM(dev_priv))
> -		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> -	else if (IS_I965G(dev_priv))
> -		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
> -	else if (IS_PINEVIEW(dev_priv))
> +		break;
> +	case INTEL_PINEVIEW:
>  		dev_priv->display.get_cdclk = pnv_get_cdclk;
> -	else if (IS_G33(dev_priv))
> -		dev_priv->display.get_cdclk = g33_get_cdclk;
> -	else if (IS_I945GM(dev_priv))
> -		dev_priv->display.get_cdclk = i945gm_get_cdclk;
> -	else if (IS_I945G(dev_priv))
> -		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
> -	else if (IS_I915GM(dev_priv))
> -		dev_priv->display.get_cdclk = i915gm_get_cdclk;
> -	else if (IS_I915G(dev_priv))
> -		dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk;
> -	else if (IS_I865G(dev_priv))
> -		dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk;
> -	else if (IS_I85X(dev_priv))
> -		dev_priv->display.get_cdclk = i85x_get_cdclk;
> -	else if (IS_I845G(dev_priv))
> -		dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk;
> -	else { /* 830 */
> -		WARN(!IS_I830(dev_priv),
> -		     "Unknown platform. Assuming 133 MHz CDCLK\n");
> -		dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk;
> +		break;
> +	case INTEL_I965GM:
> +		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> +		break;
> +	case INTEL_GM45:
> +		dev_priv->display.get_cdclk = gm45_get_cdclk;
> +		break;
> +	case INTEL_IRONLAKE:
> +		dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk;
> +		break;
> +	case INTEL_VALLEYVIEW:
> +	case INTEL_CHERRYVIEW:
> +		dev_priv->display.get_cdclk = vlv_get_cdclk;
> +		break;
> +	case INTEL_HASWELL:
> +		dev_priv->display.get_cdclk = hsw_get_cdclk;
> +		break;
> +	case INTEL_BROADWELL:
> +		dev_priv->display.get_cdclk = bdw_get_cdclk;
> +		break;
> +	case INTEL_SKYLAKE:
> +	case INTEL_KABYLAKE:
> +		dev_priv->display.get_cdclk = skl_get_cdclk;
> +		break;
> +	case INTEL_BROXTON:
> +	case INTEL_GEMINILAKE:
> +		dev_priv->display.get_cdclk = bxt_get_cdclk;
> +		break;
>  	}
>  }
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment

[Paulo Zanoni]

Em Sex, 2017-02-17 às 16:05 +0200, Ville Syrjälä escreveu:
> On Fri, Feb 17, 2017 at 11:22:07AM -0200, Paulo Zanoni wrote:
> > 
> > Possible problems of the current if-ladder:
> >   - It's a huge if ladder with almost a different check for each of
> >     our platforms.
> >   - It mixes 3 different types of checks: IS_GENX, IS_PLATFORM and
> >     IS_GROUP_OF_PLATFORMS.
> >   - As demonstrated by the recent IS_G4X commit, it's not easy to
> > be
> >     sure if a platform down on the list isn't also checked earlier.
> >   - As demonstrated by the WARN at the end, it's not easy to be
> > sure
> >     if we're actually checking for every single platform.
> > 
> > Possible advantages of the new switch statement:
> >   - It may be easier for the compiler to optimize stuff (I didn't
> >     check this), especially since the values are labels of an enum.
> >   - The compiler will tell us in case we miss some platform.
> >   - All platforms are explicitly there instead of maybe hidden in
> > some
> >     check for a certain group of platforms such as IS_GEN9_BC.
> 
> Performance is a bit of a moot point since this is run exaclty once,
> but
> the IS_GEN9_BC() stuff I tend to agree with. I don't really like
> those
> macros at all since they don't actully mean anything as far as the
> hardware features go.

I think they make some sense when they're a single check. But when we
have tons of checks for tons of platforms, I don't know.

> 
> > 
> > 
> > Possible disadvantages with the new code:
> >   - A few lines bigger.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 103 ++++++++++++++++++++++---
> > ------------
> >  1 file changed, 62 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 7c92dc7..58a2f5c 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1789,49 +1789,70 @@ void intel_init_cdclk_hooks(struct
> > drm_i915_private *dev_priv)
> >  		dev_priv->display.modeset_calc_cdclk =
> > skl_modeset_calc_cdclk;
> >  	}
> >  
> > -	if (IS_GEN9_BC(dev_priv))
> > -		dev_priv->display.get_cdclk = skl_get_cdclk;
> > -	else if (IS_GEN9_LP(dev_priv))
> > -		dev_priv->display.get_cdclk = bxt_get_cdclk;
> > -	else if (IS_BROADWELL(dev_priv))
> > -		dev_priv->display.get_cdclk = bdw_get_cdclk;
> > -	else if (IS_HASWELL(dev_priv))
> > -		dev_priv->display.get_cdclk = hsw_get_cdclk;
> > -	else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv))
> > -		dev_priv->display.get_cdclk = vlv_get_cdclk;
> > -	else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> > +	switch (INTEL_INFO(dev_priv)->platform) {
> > +	case INTEL_PLATFORM_UNINITIALIZED:
> 
> Just default: ?

If we add a default case the compiler will stop complaining in case we
don't explicitly list every platform. It's a trade-off, I really think
the current way is slightly better, but I won't oppose in case you
still think it's better adding the default case.


> 
> > 
> > +		MISSING_CASE(INTEL_INFO(dev_priv)->platform);
> > +		/* Fall through. */
> > +	case INTEL_I830:
> > +		dev_priv->display.get_cdclk =
> > fixed_133mhz_get_cdclk;
> > +		break;
> > +	case INTEL_I845G:
> > +		dev_priv->display.get_cdclk =
> > fixed_200mhz_get_cdclk;
> > +		break;
> > +	case INTEL_I85X:
> > +		dev_priv->display.get_cdclk = i85x_get_cdclk;
> > +		break;
> > +	case INTEL_I865G:
> > +		dev_priv->display.get_cdclk =
> > fixed_266mhz_get_cdclk;
> > +		break;
> > +	case INTEL_I915G:
> > +		dev_priv->display.get_cdclk =
> > fixed_333mhz_get_cdclk;
> > +		break;
> > +	case INTEL_I915GM:
> > +		dev_priv->display.get_cdclk = i915gm_get_cdclk;
> > +		break;
> > +	case INTEL_I945G:
> > +	case INTEL_I965G:
> > +	case INTEL_SANDYBRIDGE:
> > +	case INTEL_IVYBRIDGE:
> 
> I don't particularly like this disorder. I just managed to get the
> list into some sort of sane order recently.

My original thought here was that since the compiler will actually
complain in case we miss some platform, keeping a strict order is not
as meaningful as it was before. But I was also wondering if this was
actually better or not, so I can change this.

But I did notice you sorted the list. In fact, I originally wrote this
commit against a tree without your improvements, so one of the reasons
I cited in the commit message was the mess of an ordering we had at
that time :).

> 
> > 
> >  		dev_priv->display.get_cdclk =
> > fixed_400mhz_get_cdclk;
> > -	else if (IS_GEN5(dev_priv))
> > -		dev_priv->display.get_cdclk =
> > fixed_450mhz_get_cdclk;
> > -	else if (IS_GM45(dev_priv))
> > -		dev_priv->display.get_cdclk = gm45_get_cdclk;
> > -	else if (IS_G45(dev_priv))
> > +		break;
> > +	case INTEL_I945GM:
> > +		dev_priv->display.get_cdclk = i945gm_get_cdclk;
> > +		break;
> > +	case INTEL_G33:
> > +	case INTEL_G45:
> 
> More disorder.
> 
> > 
> >  		dev_priv->display.get_cdclk = g33_get_cdclk;
> > -	else if (IS_I965GM(dev_priv))
> > -		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> > -	else if (IS_I965G(dev_priv))
> > -		dev_priv->display.get_cdclk =
> > fixed_400mhz_get_cdclk;
> > -	else if (IS_PINEVIEW(dev_priv))
> > +		break;
> > +	case INTEL_PINEVIEW:
> >  		dev_priv->display.get_cdclk = pnv_get_cdclk;
> > -	else if (IS_G33(dev_priv))
> > -		dev_priv->display.get_cdclk = g33_get_cdclk;
> > -	else if (IS_I945GM(dev_priv))
> > -		dev_priv->display.get_cdclk = i945gm_get_cdclk;
> > -	else if (IS_I945G(dev_priv))
> > -		dev_priv->display.get_cdclk =
> > fixed_400mhz_get_cdclk;
> > -	else if (IS_I915GM(dev_priv))
> > -		dev_priv->display.get_cdclk = i915gm_get_cdclk;
> > -	else if (IS_I915G(dev_priv))
> > -		dev_priv->display.get_cdclk =
> > fixed_333mhz_get_cdclk;
> > -	else if (IS_I865G(dev_priv))
> > -		dev_priv->display.get_cdclk =
> > fixed_266mhz_get_cdclk;
> > -	else if (IS_I85X(dev_priv))
> > -		dev_priv->display.get_cdclk = i85x_get_cdclk;
> > -	else if (IS_I845G(dev_priv))
> > -		dev_priv->display.get_cdclk =
> > fixed_200mhz_get_cdclk;
> > -	else { /* 830 */
> > -		WARN(!IS_I830(dev_priv),
> > -		     "Unknown platform. Assuming 133 MHz
> > CDCLK\n");
> > -		dev_priv->display.get_cdclk =
> > fixed_133mhz_get_cdclk;
> > +		break;
> > +	case INTEL_I965GM:
> > +		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> > +		break;
> > +	case INTEL_GM45:
> > +		dev_priv->display.get_cdclk = gm45_get_cdclk;
> > +		break;
> > +	case INTEL_IRONLAKE:
> > +		dev_priv->display.get_cdclk =
> > fixed_450mhz_get_cdclk;
> > +		break;
> > +	case INTEL_VALLEYVIEW:
> > +	case INTEL_CHERRYVIEW:
> > +		dev_priv->display.get_cdclk = vlv_get_cdclk;
> > +		break;
> > +	case INTEL_HASWELL:
> > +		dev_priv->display.get_cdclk = hsw_get_cdclk;
> > +		break;
> > +	case INTEL_BROADWELL:
> > +		dev_priv->display.get_cdclk = bdw_get_cdclk;
> > +		break;
> > +	case INTEL_SKYLAKE:
> > +	case INTEL_KABYLAKE:
> > +		dev_priv->display.get_cdclk = skl_get_cdclk;
> > +		break;
> > +	case INTEL_BROXTON:
> > +	case INTEL_GEMINILAKE:
> > +		dev_priv->display.get_cdclk = bxt_get_cdclk;
> > +		break;
> >  	}
> >  }
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > 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

✓ Fi.CI.BAT: success for Small clocking code refactor

[Patchwork]

== Series Details ==

Series: Small clocking code refactor
URL   : https://patchwork.freedesktop.org/series/19840/
State : success

== Summary ==

Series 19840v1 Small clocking code refactor
https://patchwork.freedesktop.org/api/1.0/series/19840/revisions/1/mbox/

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

2b7ce9512d9770350bc2a59652cc7bf469bc544a drm-tip: 2017y-02m-17d-12h-20m-31s UTC integration manifest
2d2db47 drm/i915: reorganize the get_cdclk assignment
e370ecb drm/i915: remove potentially confusing IS_G4X checks
78520fc drm/i915: add intel_calc_cdclk()
c82d3ef drm/i915: kill {bdw, bxt}_modeset_calc_cdclk

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3880/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 01:17:22PM -0200, Paulo Zanoni wrote:
> Em Sex, 2017-02-17 às 16:05 +0200, Ville Syrjälä escreveu:
> > On Fri, Feb 17, 2017 at 11:22:07AM -0200, Paulo Zanoni wrote:
> > > 
> > > Possible problems of the current if-ladder:
> > >   - It's a huge if ladder with almost a different check for each of
> > >     our platforms.
> > >   - It mixes 3 different types of checks: IS_GENX, IS_PLATFORM and
> > >     IS_GROUP_OF_PLATFORMS.
> > >   - As demonstrated by the recent IS_G4X commit, it's not easy to
> > > be
> > >     sure if a platform down on the list isn't also checked earlier.
> > >   - As demonstrated by the WARN at the end, it's not easy to be
> > > sure
> > >     if we're actually checking for every single platform.
> > > 
> > > Possible advantages of the new switch statement:
> > >   - It may be easier for the compiler to optimize stuff (I didn't
> > >     check this), especially since the values are labels of an enum.
> > >   - The compiler will tell us in case we miss some platform.
> > >   - All platforms are explicitly there instead of maybe hidden in
> > > some
> > >     check for a certain group of platforms such as IS_GEN9_BC.
> > 
> > Performance is a bit of a moot point since this is run exaclty once,
> > but
> > the IS_GEN9_BC() stuff I tend to agree with. I don't really like
> > those
> > macros at all since they don't actully mean anything as far as the
> > hardware features go.
> 
> I think they make some sense when they're a single check. But when we
> have tons of checks for tons of platforms, I don't know.

The problem problem is that they basically mean different things in
different parts of the driver. So you anyway have to mentally expand
the list out to figure out what's really going on.

> 
> > 
> > > 
> > > 
> > > Possible disadvantages with the new code:
> > >   - A few lines bigger.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_cdclk.c | 103 ++++++++++++++++++++++---
> > > ------------
> > >  1 file changed, 62 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index 7c92dc7..58a2f5c 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -1789,49 +1789,70 @@ void intel_init_cdclk_hooks(struct
> > > drm_i915_private *dev_priv)
> > >  		dev_priv->display.modeset_calc_cdclk =
> > > skl_modeset_calc_cdclk;
> > >  	}
> > >  
> > > -	if (IS_GEN9_BC(dev_priv))
> > > -		dev_priv->display.get_cdclk = skl_get_cdclk;
> > > -	else if (IS_GEN9_LP(dev_priv))
> > > -		dev_priv->display.get_cdclk = bxt_get_cdclk;
> > > -	else if (IS_BROADWELL(dev_priv))
> > > -		dev_priv->display.get_cdclk = bdw_get_cdclk;
> > > -	else if (IS_HASWELL(dev_priv))
> > > -		dev_priv->display.get_cdclk = hsw_get_cdclk;
> > > -	else if (IS_VALLEYVIEW(dev_priv) ||
> > > IS_CHERRYVIEW(dev_priv))
> > > -		dev_priv->display.get_cdclk = vlv_get_cdclk;
> > > -	else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> > > +	switch (INTEL_INFO(dev_priv)->platform) {
> > > +	case INTEL_PLATFORM_UNINITIALIZED:
> > 
> > Just default: ?
> 
> If we add a default case the compiler will stop complaining in case we
> don't explicitly list every platform. It's a trade-off, I really think
> the current way is slightly better, but I won't oppose in case you
> still think it's better adding the default case.

Nah. Getting the compiler to do the work for us seems like
a decent idea. 

> 
> 
> > 
> > > 
> > > +		MISSING_CASE(INTEL_INFO(dev_priv)->platform);
> > > +		/* Fall through. */
> > > +	case INTEL_I830:
> > > +		dev_priv->display.get_cdclk =
> > > fixed_133mhz_get_cdclk;
> > > +		break;
> > > +	case INTEL_I845G:
> > > +		dev_priv->display.get_cdclk =
> > > fixed_200mhz_get_cdclk;
> > > +		break;
> > > +	case INTEL_I85X:
> > > +		dev_priv->display.get_cdclk = i85x_get_cdclk;
> > > +		break;
> > > +	case INTEL_I865G:
> > > +		dev_priv->display.get_cdclk =
> > > fixed_266mhz_get_cdclk;
> > > +		break;
> > > +	case INTEL_I915G:
> > > +		dev_priv->display.get_cdclk =
> > > fixed_333mhz_get_cdclk;
> > > +		break;
> > > +	case INTEL_I915GM:
> > > +		dev_priv->display.get_cdclk = i915gm_get_cdclk;
> > > +		break;
> > > +	case INTEL_I945G:
> > > +	case INTEL_I965G:
> > > +	case INTEL_SANDYBRIDGE:
> > > +	case INTEL_IVYBRIDGE:
> > 
> > I don't particularly like this disorder. I just managed to get the
> > list into some sort of sane order recently.
> 
> My original thought here was that since the compiler will actually
> complain in case we miss some platform, keeping a strict order is not
> as meaningful as it was before. But I was also wondering if this was
> actually better or not, so I can change this.

And unsorted list is quite hard to verify visually for correctness. We
might have a function pointer assigned for each platform, but if you
actually want to verify that it's the correct one in each case then
going through the list is IMO much easier when it's in a decent order.

> 
> But I did notice you sorted the list. In fact, I originally wrote this
> commit against a tree without your improvements, so one of the reasons
> I cited in the commit message was the mess of an ordering we had at
> that time :).
> 
> > 
> > > 
> > >  		dev_priv->display.get_cdclk =
> > > fixed_400mhz_get_cdclk;
> > > -	else if (IS_GEN5(dev_priv))
> > > -		dev_priv->display.get_cdclk =
> > > fixed_450mhz_get_cdclk;
> > > -	else if (IS_GM45(dev_priv))
> > > -		dev_priv->display.get_cdclk = gm45_get_cdclk;
> > > -	else if (IS_G45(dev_priv))
> > > +		break;
> > > +	case INTEL_I945GM:
> > > +		dev_priv->display.get_cdclk = i945gm_get_cdclk;
> > > +		break;
> > > +	case INTEL_G33:
> > > +	case INTEL_G45:
> > 
> > More disorder.
> > 
> > > 
> > >  		dev_priv->display.get_cdclk = g33_get_cdclk;
> > > -	else if (IS_I965GM(dev_priv))
> > > -		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> > > -	else if (IS_I965G(dev_priv))
> > > -		dev_priv->display.get_cdclk =
> > > fixed_400mhz_get_cdclk;
> > > -	else if (IS_PINEVIEW(dev_priv))
> > > +		break;
> > > +	case INTEL_PINEVIEW:
> > >  		dev_priv->display.get_cdclk = pnv_get_cdclk;
> > > -	else if (IS_G33(dev_priv))
> > > -		dev_priv->display.get_cdclk = g33_get_cdclk;
> > > -	else if (IS_I945GM(dev_priv))
> > > -		dev_priv->display.get_cdclk = i945gm_get_cdclk;
> > > -	else if (IS_I945G(dev_priv))
> > > -		dev_priv->display.get_cdclk =
> > > fixed_400mhz_get_cdclk;
> > > -	else if (IS_I915GM(dev_priv))
> > > -		dev_priv->display.get_cdclk = i915gm_get_cdclk;
> > > -	else if (IS_I915G(dev_priv))
> > > -		dev_priv->display.get_cdclk =
> > > fixed_333mhz_get_cdclk;
> > > -	else if (IS_I865G(dev_priv))
> > > -		dev_priv->display.get_cdclk =
> > > fixed_266mhz_get_cdclk;
> > > -	else if (IS_I85X(dev_priv))
> > > -		dev_priv->display.get_cdclk = i85x_get_cdclk;
> > > -	else if (IS_I845G(dev_priv))
> > > -		dev_priv->display.get_cdclk =
> > > fixed_200mhz_get_cdclk;
> > > -	else { /* 830 */
> > > -		WARN(!IS_I830(dev_priv),
> > > -		     "Unknown platform. Assuming 133 MHz
> > > CDCLK\n");
> > > -		dev_priv->display.get_cdclk =
> > > fixed_133mhz_get_cdclk;
> > > +		break;
> > > +	case INTEL_I965GM:
> > > +		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> > > +		break;
> > > +	case INTEL_GM45:
> > > +		dev_priv->display.get_cdclk = gm45_get_cdclk;
> > > +		break;
> > > +	case INTEL_IRONLAKE:
> > > +		dev_priv->display.get_cdclk =
> > > fixed_450mhz_get_cdclk;
> > > +		break;
> > > +	case INTEL_VALLEYVIEW:
> > > +	case INTEL_CHERRYVIEW:
> > > +		dev_priv->display.get_cdclk = vlv_get_cdclk;
> > > +		break;
> > > +	case INTEL_HASWELL:
> > > +		dev_priv->display.get_cdclk = hsw_get_cdclk;
> > > +		break;
> > > +	case INTEL_BROADWELL:
> > > +		dev_priv->display.get_cdclk = bdw_get_cdclk;
> > > +		break;
> > > +	case INTEL_SKYLAKE:
> > > +	case INTEL_KABYLAKE:
> > > +		dev_priv->display.get_cdclk = skl_get_cdclk;
> > > +		break;
> > > +	case INTEL_BROXTON:
> > > +	case INTEL_GEMINILAKE:
> > > +		dev_priv->display.get_cdclk = bxt_get_cdclk;
> > > +		break;
> > >  	}
> > >  }
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

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

Re: [PATCH 2/4] drm/i915: add intel_calc_cdclk()

[Paulo Zanoni]

Em Sex, 2017-02-17 às 15:49 +0200, Ville Syrjälä escreveu:
> On Fri, Feb 17, 2017 at 11:22:05AM -0200, Paulo Zanoni wrote:
> > 
> > Each x_modeset_calc_cdclk() has to do the same platform checks
> > twice,
> > so extract them to a single function. This way, the platform checks
> > are all in the same place, and the platform-common code gets rid of
> > all the platform-specific checks, which IMHO makes the code easier
> > to
> > read.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 84 ++++++++++++++++++++----
> > --------------
> >  1 file changed, 45 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index d505ff1..6efc5f4 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1496,6 +1496,47 @@ static int intel_max_pixel_rate(struct
> > drm_atomic_state *state)
> >  	return max_pixel_rate;
> >  }
> >  
> > +static void intel_calc_cdclk(struct intel_atomic_state *state, int
> > max_pixclk,
> > +			     int *cdclk, int *vco)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state-
> > >base.dev);
> > +
> > +	switch (INTEL_INFO(dev_priv)->platform) {
> > +	case INTEL_VALLEYVIEW:
> > +	case INTEL_CHERRYVIEW:
> > +		*cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> > +		break;
> > +	case INTEL_BROADWELL:
> > +		/*
> > +		 * FIXME: should also account for plane ratio once
> > 64bpp pixel
> > +		 * formats are supported.
> > +		 */
> > +		*cdclk = bdw_calc_cdclk(max_pixclk);
> > +		break;
> > +	case INTEL_SKYLAKE:
> > +	case INTEL_KABYLAKE:
> > +		/*
> > +		 * FIXME: should also account for plane ratio once
> > 64bpp pixel
> > +		 * formats are supported.
> > +		 */
> > +		*vco = state->cdclk.logical.vco;
> > +		if (!*vco)
> > +			*vco = dev_priv->skl_preferred_vco_freq;
> > +		*cdclk = skl_calc_cdclk(max_pixclk, *vco);
> > +		break;
> > +	case INTEL_BROXTON:
> > +		*cdclk = bxt_calc_cdclk(max_pixclk);
> > +		*vco = bxt_de_pll_vco(dev_priv, *cdclk);
> > +		break;
> > +	case INTEL_GEMINILAKE:
> > +		*cdclk = glk_calc_cdclk(max_pixclk);
> > +		*vco = glk_de_pll_vco(dev_priv, *cdclk);
> > +		break;
> > +	default:
> > +		MISSING_CASE(INTEL_INFO(dev_priv)->platform);
> > +	}
> > +}
> 
> How about just replacing the .modeset_calc_cdclk() vfunc with a
> slightly
> lower level vfunc that just computes the cdclk/vco/whatever without
> containing the active_crtcs logic?
> 
> Then we should have just
> 
> intel_modeset_calc_cdclk()
> {
> 	.calc_cdclk(logical, max_pixclk);
> 
> 	/*
> 	 * maybe keep the max_cdclk check here, although it that
> 	 * happens I think we have a bug somewhere, so perhaps
> 	 * just convert it into a WARN, or drop entirely.
> 	 */
> 
> 	if (!active_crtcs)
> 		.calc_cdclk(actual, 0);
> 	else
> 		actual = logical;
> }

Yeah, the code above is definitely a next step to the changes I did.

I'm just not a big fan of the .calc_cdclk vfunc since it will be just 2
lines for each platform. Unless I inline them with the *real*
x_calc_cdclk() funcs we have today, but then I'll have to check their
other callers. So I'll take a look and try to submit a new patch.

> 
> 
> > 
> > +
> >  static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > @@ -1503,14 +1544,7 @@ static int vlv_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	int max_pixclk = intel_max_pixel_rate(state);
> >  	int cdclk;
> >  
> > -	/*
> > -	 * FIXME: Broadwell should also account for plane ratio
> > once 64bpp pixel
> > -	 * formats are supported.
> > -	 */
> > -	if (IS_BROADWELL(dev_priv))
> > -		cdclk = bdw_calc_cdclk(max_pixclk);
> > -	else
> > -		cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> > +	intel_calc_cdclk(intel_state, max_pixclk, &cdclk, NULL);
> >  
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds
> > max (%d kHz)\n",
> > @@ -1521,11 +1555,7 @@ static int vlv_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		if (IS_BROADWELL(dev_priv))
> > -			cdclk = bdw_calc_cdclk(0);
> > -		else
> > -			cdclk = vlv_calc_cdclk(dev_priv, 0);
> > -
> > +		intel_calc_cdclk(intel_state, 0, &cdclk, NULL);
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> >  	} else {
> >  		intel_state->cdclk.actual = intel_state-
> > >cdclk.logical;
> > @@ -1541,22 +1571,7 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	int max_pixclk = intel_max_pixel_rate(state);
> >  	int cdclk, vco;
> >  
> > -	/*
> > -	 * FIXME: Skylake/Kabylake should also account for plane
> > ratio once
> > -	 * 64bpp pixel formats are supported.
> > -	 */
> > -	if (IS_GEMINILAKE(dev_priv)) {
> > -		cdclk = glk_calc_cdclk(max_pixclk);
> > -		vco = glk_de_pll_vco(dev_priv, cdclk);
> > -	} else if (IS_BROXTON(dev_priv)) {
> > -		cdclk = bxt_calc_cdclk(max_pixclk);
> > -		vco = bxt_de_pll_vco(dev_priv, cdclk);
> > -	} else {
> > -		vco = intel_state->cdclk.logical.vco;
> > -		if (!vco)
> > -			vco = dev_priv->skl_preferred_vco_freq;
> > -		cdclk = skl_calc_cdclk(max_pixclk, vco);
> > -	}
> > +	intel_calc_cdclk(intel_state, max_pixclk, &cdclk, &vco);
> >  
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds
> > max (%d kHz)\n",
> > @@ -1568,16 +1583,7 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		if (IS_GEMINILAKE(dev_priv)) {
> > -			cdclk = glk_calc_cdclk(0);
> > -			vco = glk_de_pll_vco(dev_priv, cdclk);
> > -		} else if (IS_BROXTON(dev_priv)) {
> > -			cdclk = bxt_calc_cdclk(0);
> > -			vco = bxt_de_pll_vco(dev_priv, cdclk);
> > -		} else {
> > -			cdclk = skl_calc_cdclk(0, vco);
> > -		}
> > -
> > +		intel_calc_cdclk(intel_state, 0, &cdclk, &vco);
> >  		intel_state->cdclk.actual.vco = vco;
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> >  	} else {
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > 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 2/4] drm/i915: add intel_calc_cdclk()

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 06:37:23PM -0200, Paulo Zanoni wrote:
> Em Sex, 2017-02-17 às 15:49 +0200, Ville Syrjälä escreveu:
> > On Fri, Feb 17, 2017 at 11:22:05AM -0200, Paulo Zanoni wrote:
> > > 
> > > Each x_modeset_calc_cdclk() has to do the same platform checks
> > > twice,
> > > so extract them to a single function. This way, the platform checks
> > > are all in the same place, and the platform-common code gets rid of
> > > all the platform-specific checks, which IMHO makes the code easier
> > > to
> > > read.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_cdclk.c | 84 ++++++++++++++++++++----
> > > --------------
> > >  1 file changed, 45 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index d505ff1..6efc5f4 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -1496,6 +1496,47 @@ static int intel_max_pixel_rate(struct
> > > drm_atomic_state *state)
> > >  	return max_pixel_rate;
> > >  }
> > >  
> > > +static void intel_calc_cdclk(struct intel_atomic_state *state, int
> > > max_pixclk,
> > > +			     int *cdclk, int *vco)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(state-
> > > >base.dev);
> > > +
> > > +	switch (INTEL_INFO(dev_priv)->platform) {
> > > +	case INTEL_VALLEYVIEW:
> > > +	case INTEL_CHERRYVIEW:
> > > +		*cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> > > +		break;
> > > +	case INTEL_BROADWELL:
> > > +		/*
> > > +		 * FIXME: should also account for plane ratio once
> > > 64bpp pixel
> > > +		 * formats are supported.
> > > +		 */
> > > +		*cdclk = bdw_calc_cdclk(max_pixclk);
> > > +		break;
> > > +	case INTEL_SKYLAKE:
> > > +	case INTEL_KABYLAKE:
> > > +		/*
> > > +		 * FIXME: should also account for plane ratio once
> > > 64bpp pixel
> > > +		 * formats are supported.
> > > +		 */
> > > +		*vco = state->cdclk.logical.vco;
> > > +		if (!*vco)
> > > +			*vco = dev_priv->skl_preferred_vco_freq;
> > > +		*cdclk = skl_calc_cdclk(max_pixclk, *vco);
> > > +		break;
> > > +	case INTEL_BROXTON:
> > > +		*cdclk = bxt_calc_cdclk(max_pixclk);
> > > +		*vco = bxt_de_pll_vco(dev_priv, *cdclk);
> > > +		break;
> > > +	case INTEL_GEMINILAKE:
> > > +		*cdclk = glk_calc_cdclk(max_pixclk);
> > > +		*vco = glk_de_pll_vco(dev_priv, *cdclk);
> > > +		break;
> > > +	default:
> > > +		MISSING_CASE(INTEL_INFO(dev_priv)->platform);
> > > +	}
> > > +}
> > 
> > How about just replacing the .modeset_calc_cdclk() vfunc with a
> > slightly
> > lower level vfunc that just computes the cdclk/vco/whatever without
> > containing the active_crtcs logic?
> > 
> > Then we should have just
> > 
> > intel_modeset_calc_cdclk()
> > {
> > 	.calc_cdclk(logical, max_pixclk);
> > 
> > 	/*
> > 	 * maybe keep the max_cdclk check here, although it that
> > 	 * happens I think we have a bug somewhere, so perhaps
> > 	 * just convert it into a WARN, or drop entirely.
> > 	 */
> > 
> > 	if (!active_crtcs)
> > 		.calc_cdclk(actual, 0);
> > 	else
> > 		actual = logical;
> > }
> 
> Yeah, the code above is definitely a next step to the changes I did.
> 
> I'm just not a big fan of the .calc_cdclk vfunc since it will be just 2
> lines for each platform. Unless I inline them with the *real*
> x_calc_cdclk() funcs we have today, but then I'll have to check their
> other callers. So I'll take a look and try to submit a new patch.

At some point I had this idea of making the cdclk computation more data
driven. As in we'd store the various possible cdclk steps, required
guarbands etc. in some structure and thus avoid all the calc_cdclk()
functions which are mostly just if ladders with slightly different
numbers in them. But I never actually tried it, so not sure how
pretty/ugly it would turn out to be.

In the meantime, I think I'd prefer two line functions over the switch
statement. For one, it would allow us to keep the calculation part right
next to the other cdclk stuff for said platform. One thing that's a
slight concern is the future dvfs stuff we talked about. I've not yet
fully thought out where that needs to be done, but it might be that
some of it might nicely land in these two line function (making them
at least three lines ;).

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

Re: [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk

[David Weinehall]

On Fri, Feb 17, 2017 at 11:22:04AM -0200, Paulo Zanoni wrote:
> The functions are pretty much the same, except for the CDCLK and VCO
> calculations. Add BDW support to vlv_modeset_calc_cdclk() and add
> BXT/GLK support to skl_modeset_calc_cdclk(). The two reamining

s/reamining/remaining/

> functions are still very similar, except for the fact that the vlv
> version doesn't touch the VCO. Further patches could unify them even
> more if that's desired.
> 
> While at it, merge some lines that can fit 80 columns in those
> functions.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 120 ++++++++++---------------------------
>  1 file changed, 30 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index d643c0c..d505ff1 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1499,45 +1499,18 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
>  static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	int max_pixclk = intel_max_pixel_rate(state);
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(state);
> -	int cdclk;
> -
> -	cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> -
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> -		return -EINVAL;
> -	}
> -
> -	intel_state->cdclk.logical.cdclk = cdclk;
> -
> -	if (!intel_state->active_crtcs) {
> -		cdclk = vlv_calc_cdclk(dev_priv, 0);
> -
> -		intel_state->cdclk.actual.cdclk = cdclk;
> -	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> -	}
> -
> -	return 0;
> -}
> -
> -static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	int max_pixclk = intel_max_pixel_rate(state);
>  	int cdclk;
>  
>  	/*
> -	 * FIXME should also account for plane ratio
> -	 * once 64bpp pixel formats are supported.
> +	 * FIXME: Broadwell should also account for plane ratio once 64bpp pixel
> +	 * formats are supported.
>  	 */
> -	cdclk = bdw_calc_cdclk(max_pixclk);
> +	if (IS_BROADWELL(dev_priv))
> +		cdclk = bdw_calc_cdclk(max_pixclk);
> +	else
> +		cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
>  
>  	if (cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1548,12 +1521,14 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
>  	if (!intel_state->active_crtcs) {
> -		cdclk = bdw_calc_cdclk(0);
> +		if (IS_BROADWELL(dev_priv))
> +			cdclk = bdw_calc_cdclk(0);
> +		else
> +			cdclk = vlv_calc_cdclk(dev_priv, 0);
>  
>  		intel_state->cdclk.actual.cdclk = cdclk;
>  	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> +		intel_state->cdclk.actual = intel_state->cdclk.logical;
>  	}
>  
>  	return 0;
> @@ -1561,57 +1536,26 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>  
>  static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
> -	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	const int max_pixclk = intel_max_pixel_rate(state);
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	int max_pixclk = intel_max_pixel_rate(state);
>  	int cdclk, vco;
>  
> -	vco = intel_state->cdclk.logical.vco;
> -	if (!vco)
> -		vco = dev_priv->skl_preferred_vco_freq;
> -
>  	/*
> -	 * FIXME should also account for plane ratio
> -	 * once 64bpp pixel formats are supported.
> +	 * FIXME: Skylake/Kabylake should also account for plane ratio once
> +	 * 64bpp pixel formats are supported.
>  	 */
> -	cdclk = skl_calc_cdclk(max_pixclk, vco);
> -
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> -		return -EINVAL;
> -	}
> -
> -	intel_state->cdclk.logical.vco = vco;
> -	intel_state->cdclk.logical.cdclk = cdclk;
> -
> -	if (!intel_state->active_crtcs) {
> -		cdclk = skl_calc_cdclk(0, vco);
> -
> -		intel_state->cdclk.actual.vco = vco;
> -		intel_state->cdclk.actual.cdclk = cdclk;
> -	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> -	}
> -
> -	return 0;
> -}
> -
> -static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	int max_pixclk = intel_max_pixel_rate(state);
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(state);
> -	int cdclk, vco;
> -
>  	if (IS_GEMINILAKE(dev_priv)) {
>  		cdclk = glk_calc_cdclk(max_pixclk);
>  		vco = glk_de_pll_vco(dev_priv, cdclk);
> -	} else {
> +	} else if (IS_BROXTON(dev_priv)) {
>  		cdclk = bxt_calc_cdclk(max_pixclk);
>  		vco = bxt_de_pll_vco(dev_priv, cdclk);
> +	} else {
> +		vco = intel_state->cdclk.logical.vco;
> +		if (!vco)
> +			vco = dev_priv->skl_preferred_vco_freq;
> +		cdclk = skl_calc_cdclk(max_pixclk, vco);
>  	}
>  
>  	if (cdclk > dev_priv->max_cdclk_freq) {
> @@ -1627,16 +1571,17 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  		if (IS_GEMINILAKE(dev_priv)) {
>  			cdclk = glk_calc_cdclk(0);
>  			vco = glk_de_pll_vco(dev_priv, cdclk);
> -		} else {
> +		} else if (IS_BROXTON(dev_priv)) {
>  			cdclk = bxt_calc_cdclk(0);
>  			vco = bxt_de_pll_vco(dev_priv, cdclk);
> +		} else {
> +			cdclk = skl_calc_cdclk(0, vco);
>  		}
>  
>  		intel_state->cdclk.actual.vco = vco;
>  		intel_state->cdclk.actual.cdclk = cdclk;
>  	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> +		intel_state->cdclk.actual = intel_state->cdclk.logical;
>  	}
>  
>  	return 0;
> @@ -1823,24 +1768,19 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
>  {
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		dev_priv->display.set_cdclk = chv_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			vlv_modeset_calc_cdclk;
> +		dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk;
>  	} else if (IS_VALLEYVIEW(dev_priv)) {
>  		dev_priv->display.set_cdclk = vlv_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			vlv_modeset_calc_cdclk;
> +		dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk;
>  	} else if (IS_BROADWELL(dev_priv)) {
>  		dev_priv->display.set_cdclk = bdw_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			bdw_modeset_calc_cdclk;
> +		dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk;
>  	} else if (IS_GEN9_LP(dev_priv)) {
>  		dev_priv->display.set_cdclk = bxt_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			bxt_modeset_calc_cdclk;
> +		dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk;
>  	} else if (IS_GEN9_BC(dev_priv)) {
>  		dev_priv->display.set_cdclk = skl_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			skl_modeset_calc_cdclk;
> +		dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk;
>  	}
>  
>  	if (IS_GEN9_BC(dev_priv))
> -- 
> 2.7.4
> 
> _______________________________________________
> 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