[PATCH 0/3] Small clocking code refactor, v2.

[PATCH 0/3] Small clocking code refactor, v2.

[Paulo Zanoni]

With Ville's comments addressed. The main difference now is that what used to be
patches 1 and 2 are now just patch 1.

Paulo Zanoni (3):
  drm/i915: unify the x_modeset_calc_cdclk() functions
  drm/i915: remove potentially confusing IS_G4X checks
  drm/i915: reorganize the get_cdclk assignment

 drivers/gpu/drm/i915/i915_drv.h      |   4 +-
 drivers/gpu/drm/i915/intel_cdclk.c   | 294 ++++++++++++++---------------------
 drivers/gpu/drm/i915/intel_display.c |   6 +-
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 4 files changed, 127 insertions(+), 178 deletions(-)

-- 
2.7.4

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

[PATCH 1/3] drm/i915: unify the x_modeset_calc_cdclk() functions

[Paulo Zanoni]

There's a lot of duplicated platform-independent logic in the current
modeset_calc_cdclk() functions. Adding cdclk support for more
platforms will only add more copies of this code.

To solve this problem, in this patch we create a new function called
intel_modeset_calc_cdclk() which unifies the platform-independent
logic, and we also create a new vfunc called calc_cdclk_state(), which
is responsible to contain only the platform-dependent code.

The code is now smaller and support for new platforms should be easier
and not require duplicated platform-independent code.

v2: Previously I had 2 different patches addressing these problems,
but wiht Ville's suggestion I think it makes more sense to keep
everything in a single patch (Ville).

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   4 +-
 drivers/gpu/drm/i915/intel_cdclk.c   | 187 ++++++++++-------------------------
 drivers/gpu/drm/i915/intel_display.c |   6 +-
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 4 files changed, 60 insertions(+), 138 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4e7046d..f1c35c7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -644,7 +644,9 @@ struct drm_i915_display_funcs {
 				    struct intel_crtc_state *cstate);
 	int (*compute_global_watermarks)(struct drm_atomic_state *state);
 	void (*update_wm)(struct intel_crtc *crtc);
-	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
+	void (*calc_cdclk_state)(struct drm_i915_private *dev_priv,
+				 int max_pixclk,
+				 struct intel_cdclk_state *cdclk_state);
 	/* Returns the active state of the crtc, and if the crtc is active,
 	 * fills out the pipe-config with the hw state. */
 	bool (*get_pipe_config)(struct intel_crtc *,
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index d643c0c..dd6fe25 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1496,148 +1496,69 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
 	return max_pixel_rate;
 }
 
-static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
+static void vlv_calc_cdclk_state(struct drm_i915_private *dev_priv,
+				 int max_pixclk,
+				 struct intel_cdclk_state *cdclk_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;
+	cdclk_state->cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
 }
 
-static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
+static void bdw_calc_cdclk_state(struct drm_i915_private *dev_priv,
+				 int max_pixclk,
+				 struct intel_cdclk_state *cdclk_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.
-	 */
-	cdclk = bdw_calc_cdclk(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 = bdw_calc_cdclk(0);
-
-		intel_state->cdclk.actual.cdclk = cdclk;
-	} else {
-		intel_state->cdclk.actual =
-			intel_state->cdclk.logical;
-	}
-
-	return 0;
+	cdclk_state->cdclk = bdw_calc_cdclk(max_pixclk);
 }
 
-static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
+static void skl_calc_cdclk_state(struct drm_i915_private *dev_priv,
+				 int max_pixclk,
+				 struct intel_cdclk_state *cdclk_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);
-	int cdclk, vco;
+	if (!cdclk_state->vco)
+		cdclk_state->vco = dev_priv->skl_preferred_vco_freq;
 
-	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.
-	 */
-	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);
+	cdclk_state->cdclk = skl_calc_cdclk(max_pixclk, cdclk_state->vco);
+}
 
-		intel_state->cdclk.actual.vco = vco;
-		intel_state->cdclk.actual.cdclk = cdclk;
-	} else {
-		intel_state->cdclk.actual =
-			intel_state->cdclk.logical;
-	}
+static void bxt_calc_cdclk_state(struct drm_i915_private *dev_priv,
+				 int max_pixclk,
+				 struct intel_cdclk_state *cdclk_state)
+{
+	cdclk_state->cdclk = bxt_calc_cdclk(max_pixclk);
+	cdclk_state->vco = bxt_de_pll_vco(dev_priv, cdclk_state->cdclk);
+}
 
-	return 0;
+static void glk_calc_cdclk_state(struct drm_i915_private *dev_priv,
+				 int max_pixclk,
+				 struct intel_cdclk_state *cdclk_state)
+{
+	cdclk_state->cdclk = glk_calc_cdclk(max_pixclk);
+	cdclk_state->vco = glk_de_pll_vco(dev_priv, cdclk_state->cdclk);
 }
 
-static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
+int intel_modeset_calc_cdclk(struct intel_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;
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct intel_cdclk_state *logical = &state->cdclk.logical;
+	struct intel_cdclk_state *actual = &state->cdclk.actual;
+	int max_pixclk = intel_max_pixel_rate(&state->base);
 
-	if (IS_GEMINILAKE(dev_priv)) {
-		cdclk = glk_calc_cdclk(max_pixclk);
-		vco = glk_de_pll_vco(dev_priv, cdclk);
-	} else {
-		cdclk = bxt_calc_cdclk(max_pixclk);
-		vco = bxt_de_pll_vco(dev_priv, cdclk);
-	}
+	/*
+	 * FIXME: should also account for plane ratio once 64bpp pixel formats
+	 * are supported.
+	 */
+	dev_priv->display.calc_cdclk_state(dev_priv, max_pixclk, logical);
 
-	if (cdclk > dev_priv->max_cdclk_freq) {
+	if (logical->cdclk > dev_priv->max_cdclk_freq) {
 		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
-			      cdclk, dev_priv->max_cdclk_freq);
+			      logical->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) {
-		if (IS_GEMINILAKE(dev_priv)) {
-			cdclk = glk_calc_cdclk(0);
-			vco = glk_de_pll_vco(dev_priv, cdclk);
-		} else {
-			cdclk = bxt_calc_cdclk(0);
-			vco = bxt_de_pll_vco(dev_priv, cdclk);
-		}
-
-		intel_state->cdclk.actual.vco = vco;
-		intel_state->cdclk.actual.cdclk = cdclk;
-	} else {
-		intel_state->cdclk.actual =
-			intel_state->cdclk.logical;
-	}
+	if (!state->active_crtcs)
+		dev_priv->display.calc_cdclk_state(dev_priv, 0, actual);
+	else
+		*actual = *logical;
 
 	return 0;
 }
@@ -1823,24 +1744,22 @@ 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.calc_cdclk_state = vlv_calc_cdclk_state;
 	} 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.calc_cdclk_state = vlv_calc_cdclk_state;
 	} else if (IS_BROADWELL(dev_priv)) {
 		dev_priv->display.set_cdclk = bdw_set_cdclk;
-		dev_priv->display.modeset_calc_cdclk =
-			bdw_modeset_calc_cdclk;
-	} else if (IS_GEN9_LP(dev_priv)) {
+		dev_priv->display.calc_cdclk_state = bdw_calc_cdclk_state;
+	} else if (IS_BROXTON(dev_priv)) {
+		dev_priv->display.set_cdclk = bxt_set_cdclk;
+		dev_priv->display.calc_cdclk_state = bxt_calc_cdclk_state;
+	} else if (IS_GEMINILAKE(dev_priv)) {
 		dev_priv->display.set_cdclk = bxt_set_cdclk;
-		dev_priv->display.modeset_calc_cdclk =
-			bxt_modeset_calc_cdclk;
+		dev_priv->display.calc_cdclk_state = glk_calc_cdclk_state;
 	} 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.calc_cdclk_state = skl_calc_cdclk_state;
 	}
 
 	if (IS_GEN9_BC(dev_priv))
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f6feb93..1d2cb491 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12414,8 +12414,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 	 * mode set on this crtc.  For other crtcs we need to use the
 	 * adjusted_mode bits in the crtc directly.
 	 */
-	if (dev_priv->display.modeset_calc_cdclk) {
-		ret = dev_priv->display.modeset_calc_cdclk(state);
+	if (dev_priv->display.calc_cdclk_state) {
+		ret = intel_modeset_calc_cdclk(intel_state);
 		if (ret < 0)
 			return ret;
 
@@ -15468,7 +15468,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 				pixclk = crtc_state->pixel_rate;
 			else
-				WARN_ON(dev_priv->display.modeset_calc_cdclk);
+				WARN_ON(dev_priv->display.calc_cdclk_state);
 
 			/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
 			if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 821c57c..3e112fe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1260,6 +1260,7 @@ bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
 			       const struct intel_cdclk_state *b);
 void intel_set_cdclk(struct drm_i915_private *dev_priv,
 		     const struct intel_cdclk_state *cdclk_state);
+int intel_modeset_calc_cdclk(struct intel_atomic_state *state);
 
 /* intel_display.c */
 enum transcoder intel_crtc_pch_transcoder(struct intel_crtc *crtc);
-- 
2.7.4

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

[PATCH 2/3] 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 dd6fe25..942adf0 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;
@@ -1778,7 +1778,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 3/3] 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.

v2: Don't unsort the list. Now the list almost matches the enum
definition, with the exception of CHV, KBL and GLK, which are listed
along their predecessors (Ville).

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

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 942adf0..e1921fe7 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1762,49 +1762,76 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.calc_cdclk_state = skl_calc_cdclk_state;
 	}
 
-	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:
 		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:
 		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))
+		break;
+	case INTEL_I965G:
+		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
+		break;
+	case INTEL_I965GM:
+		dev_priv->display.get_cdclk = i965gm_get_cdclk;
+		break;
+	case INTEL_G45:
 		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))
+		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_SANDYBRIDGE:
+	case INTEL_IVYBRIDGE:
 		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_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

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

[Patchwork]

== Series Details ==

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

== Summary ==

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

fi-bdw-5557u     total:253  pass:242  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:253  pass:214  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:253  pass:234  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:253  pass:226  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:253  pass:222  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:253  pass:203  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:253  pass:236  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:253  pass:231  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:253  pass:225  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:253  pass:224  dwarn:0   dfail:0   fail:0   skip:29 

0a03ea9496b49746b4964d05cc1f483385d1df8a drm-tip: 2017y-02m-20d-17h-19m-56s UTC integration manifest
172baa0 drm/i915: reorganize the get_cdclk assignment
25d43f8 drm/i915: remove potentially confusing IS_G4X checks
4f12bd0 drm/i915: unify the x_modeset_calc_cdclk() functions

== Logs ==

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

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

[Ville Syrjälä]

On Mon, Feb 20, 2017 at 05:00:41PM -0300, Paulo Zanoni wrote:
> 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>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.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 dd6fe25..942adf0 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;
> @@ -1778,7 +1778,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

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

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

[Ville Syrjälä]

On Mon, Feb 20, 2017 at 05:00:42PM -0300, 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.
> 
> Possible disadvantages with the new code:
>   - A few lines bigger.
> 
> v2: Don't unsort the list. Now the list almost matches the enum
> definition, with the exception of CHV, KBL and GLK, which are listed
> along their predecessors (Ville).
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 105 +++++++++++++++++++++++--------------
>  1 file changed, 66 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 942adf0..e1921fe7 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1762,49 +1762,76 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
>  		dev_priv->display.calc_cdclk_state = skl_calc_cdclk_state;
>  	}
>  
> -	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:

The diff might come out nicer if you didn't reverse the list. Right now
it's quite hard to read.

> +		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:
>  		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:
>  		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))
> +		break;
> +	case INTEL_I965G:
> +		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
> +		break;
> +	case INTEL_I965GM:
> +		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> +		break;
> +	case INTEL_G45:
>  		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))
> +		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_SANDYBRIDGE:
> +	case INTEL_IVYBRIDGE:
>  		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_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

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

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

[Ander Conselvan De Oliveira]

On Mon, 2017-02-20 at 17:00 -0300, 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.
> 
> Possible disadvantages with the new code:
>   - A few lines bigger.

I had suggested something different at some point, but didn't get around to
implementing it yet:

https://lists.freedesktop.org/archives/intel-gfx/2016-December/115078.html

I think that if we need almost one case for each single platform, might as well
put these in device info. Anyway, I didn't check how well that proposal works
with the changes in this series, but figured I mention it anyway in case it
helps.

Ander


> 
> v2: Don't unsort the list. Now the list almost matches the enum
> definition, with the exception of CHV, KBL and GLK, which are listed
> along their predecessors (Ville).
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c | 105 +++++++++++++++++++++++--------------
>  1 file changed, 66 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 942adf0..e1921fe7 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1762,49 +1762,76 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
>  		dev_priv->display.calc_cdclk_state = skl_calc_cdclk_state;
>  	}
>  
> -	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:
>  		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:
>  		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))
> +		break;
> +	case INTEL_I965G:
> +		dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
> +		break;
> +	case INTEL_I965GM:
> +		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> +		break;
> +	case INTEL_G45:
>  		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))
> +		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_SANDYBRIDGE:
> +	case INTEL_IVYBRIDGE:
>  		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_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;
>  	}
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Paulo Zanoni]

Em Ter, 2017-02-21 às 13:51 +0200, Ville Syrjälä escreveu:
> On Mon, Feb 20, 2017 at 05:00:42PM -0300, 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.
> > 
> > Possible disadvantages with the new code:
> >   - A few lines bigger.
> > 
> > v2: Don't unsort the list. Now the list almost matches the enum
> > definition, with the exception of CHV, KBL and GLK, which are
> > listed
> > along their predecessors (Ville).
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 105 +++++++++++++++++++++++
> > --------------
> >  1 file changed, 66 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 942adf0..e1921fe7 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1762,49 +1762,76 @@ void intel_init_cdclk_hooks(struct
> > drm_i915_private *dev_priv)
> >  		dev_priv->display.calc_cdclk_state =
> > skl_calc_cdclk_state;
> >  	}
> >  
> > -	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:
> 
> The diff might come out nicer if you didn't reverse the list. Right
> now
> it's quite hard to read.

On the other hand, the labels in the switch statement match the enum
definition (with the exception of CHV/KBL/GLK). So IMHO this is an
"easier to review the patch now vs easier to review the code later"
problem. Still, I can change this, it won't hurt.

> 
> > 
> > +		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:
> >  		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:
> >  		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))
> > +		break;
> > +	case INTEL_I965G:
> > +		dev_priv->display.get_cdclk =
> > fixed_400mhz_get_cdclk;
> > +		break;
> > +	case INTEL_I965GM:
> > +		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> > +		break;
> > +	case INTEL_G45:
> >  		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))
> > +		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_SANDYBRIDGE:
> > +	case INTEL_IVYBRIDGE:
> >  		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_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 3/3] drm/i915: reorganize the get_cdclk assignment

[Paulo Zanoni]

Em Ter, 2017-02-21 às 14:26 +0200, Ander Conselvan De Oliveira
escreveu:
> On Mon, 2017-02-20 at 17:00 -0300, 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.
> > 
> > Possible disadvantages with the new code:
> >   - A few lines bigger.
> 
> I had suggested something different at some point, but didn't get
> around to
> implementing it yet:
> 
> https://lists.freedesktop.org/archives/intel-gfx/2016-December/115078
> .html
> 
> I think that if we need almost one case for each single platform,
> might as well
> put these in device info. Anyway, I didn't check how well that
> proposal works
> with the changes in this series, but figured I mention it anyway in
> case it
> helps.

That approach would still work. I can drop this patch if you're still
planning to implement that, no problem. The only patch from this series
that I really want to upstream is patch 1.

The value of this specific 3/3 patch was greatly decreased after Ville
sorted the platform if ladder, but I decided to send it anyway since I
already had it (and since I wanted to test people's reactions to using
the platform enum in a switch statement).

> 
> Ander
> 
> 
> > 
> > 
> > v2: Don't unsort the list. Now the list almost matches the enum
> > definition, with the exception of CHV, KBL and GLK, which are
> > listed
> > along their predecessors (Ville).
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 105 +++++++++++++++++++++++
> > --------------
> >  1 file changed, 66 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 942adf0..e1921fe7 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1762,49 +1762,76 @@ void intel_init_cdclk_hooks(struct
> > drm_i915_private *dev_priv)
> >  		dev_priv->display.calc_cdclk_state =
> > skl_calc_cdclk_state;
> >  	}
> >  
> > -	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:
> >  		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:
> >  		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))
> > +		break;
> > +	case INTEL_I965G:
> > +		dev_priv->display.get_cdclk =
> > fixed_400mhz_get_cdclk;
> > +		break;
> > +	case INTEL_I965GM:
> > +		dev_priv->display.get_cdclk = i965gm_get_cdclk;
> > +		break;
> > +	case INTEL_G45:
> >  		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))
> > +		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_SANDYBRIDGE:
> > +	case INTEL_IVYBRIDGE:
> >  		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_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;
> >  	}
> >  }
> _______________________________________________
> 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 1/3] drm/i915: unify the x_modeset_calc_cdclk() functions

[Ville Syrjälä]

On Mon, Feb 20, 2017 at 05:00:40PM -0300, Paulo Zanoni wrote:
> There's a lot of duplicated platform-independent logic in the current
> modeset_calc_cdclk() functions. Adding cdclk support for more
> platforms will only add more copies of this code.
> 
> To solve this problem, in this patch we create a new function called
> intel_modeset_calc_cdclk() which unifies the platform-independent
> logic, and we also create a new vfunc called calc_cdclk_state(), which
> is responsible to contain only the platform-dependent code.
> 
> The code is now smaller and support for new platforms should be easier
> and not require duplicated platform-independent code.
> 
> v2: Previously I had 2 different patches addressing these problems,
> but wiht Ville's suggestion I think it makes more sense to keep
> everything in a single patch (Ville).
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   4 +-
>  drivers/gpu/drm/i915/intel_cdclk.c   | 187 ++++++++++-------------------------
>  drivers/gpu/drm/i915/intel_display.c |   6 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   1 +
>  4 files changed, 60 insertions(+), 138 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4e7046d..f1c35c7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -644,7 +644,9 @@ struct drm_i915_display_funcs {
>  				    struct intel_crtc_state *cstate);
>  	int (*compute_global_watermarks)(struct drm_atomic_state *state);
>  	void (*update_wm)(struct intel_crtc *crtc);
> -	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> +	void (*calc_cdclk_state)(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_state);
>  	/* Returns the active state of the crtc, and if the crtc is active,
>  	 * fills out the pipe-config with the hw state. */
>  	bool (*get_pipe_config)(struct intel_crtc *,
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index d643c0c..dd6fe25 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1496,148 +1496,69 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
>  	return max_pixel_rate;
>  }
>  
> -static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> +static void vlv_calc_cdclk_state(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_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;
> +	cdclk_state->cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
>  }
>  
> -static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> +static void bdw_calc_cdclk_state(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_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.
> -	 */
> -	cdclk = bdw_calc_cdclk(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 = bdw_calc_cdclk(0);
> -
> -		intel_state->cdclk.actual.cdclk = cdclk;
> -	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> -	}
> -
> -	return 0;
> +	cdclk_state->cdclk = bdw_calc_cdclk(max_pixclk);
>  }
>  
> -static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> +static void skl_calc_cdclk_state(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_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);
> -	int cdclk, vco;
> +	if (!cdclk_state->vco)
> +		cdclk_state->vco = dev_priv->skl_preferred_vco_freq;

Hmm. To maintain the behaviour of the old code we would need to
explicitly pluck the vco from the logicial cdclk state. Otherwise
when we're computing the actual cdclk state we might choose the
wrong vco.

Well, I must admit I'm not 100% sure that we could get the wrong
answer. I'd have to go through the entire thing again, thinking
about all the cases when the vco might change. But since I'm feeling
a little lazy I don't really want to do that. So I think it would
be safer to keep to the current way of doing things.

An alternative would be to always do the actual=logical assignment
in intel_modeset_calc_cdclk() so that the actual.vco==logical.vco
by the time we compute the actual cdclk state. This feels a little
cleaner perhaps, but could probably use a comment to avoid someone
optimizing the copy away again.

>  
> -	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.
> -	 */
> -	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);
> +	cdclk_state->cdclk = skl_calc_cdclk(max_pixclk, cdclk_state->vco);
> +}
>  
> -		intel_state->cdclk.actual.vco = vco;
> -		intel_state->cdclk.actual.cdclk = cdclk;
> -	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> -	}
> +static void bxt_calc_cdclk_state(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_state)
> +{
> +	cdclk_state->cdclk = bxt_calc_cdclk(max_pixclk);
> +	cdclk_state->vco = bxt_de_pll_vco(dev_priv, cdclk_state->cdclk);
> +}
>  
> -	return 0;
> +static void glk_calc_cdclk_state(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_state)
> +{
> +	cdclk_state->cdclk = glk_calc_cdclk(max_pixclk);
> +	cdclk_state->vco = glk_de_pll_vco(dev_priv, cdclk_state->cdclk);
>  }
>  
> -static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> +int intel_modeset_calc_cdclk(struct intel_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;
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct intel_cdclk_state *logical = &state->cdclk.logical;
> +	struct intel_cdclk_state *actual = &state->cdclk.actual;
> +	int max_pixclk = intel_max_pixel_rate(&state->base);
>  
> -	if (IS_GEMINILAKE(dev_priv)) {
> -		cdclk = glk_calc_cdclk(max_pixclk);
> -		vco = glk_de_pll_vco(dev_priv, cdclk);
> -	} else {
> -		cdclk = bxt_calc_cdclk(max_pixclk);
> -		vco = bxt_de_pll_vco(dev_priv, cdclk);
> -	}
> +	/*
> +	 * FIXME: should also account for plane ratio once 64bpp pixel formats
> +	 * are supported.
> +	 */
> +	dev_priv->display.calc_cdclk_state(dev_priv, max_pixclk, logical);
>  
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> +	if (logical->cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> +			      logical->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) {
> -		if (IS_GEMINILAKE(dev_priv)) {
> -			cdclk = glk_calc_cdclk(0);
> -			vco = glk_de_pll_vco(dev_priv, cdclk);
> -		} else {
> -			cdclk = bxt_calc_cdclk(0);
> -			vco = bxt_de_pll_vco(dev_priv, cdclk);
> -		}
> -
> -		intel_state->cdclk.actual.vco = vco;
> -		intel_state->cdclk.actual.cdclk = cdclk;
> -	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> -	}
> +	if (!state->active_crtcs)
> +		dev_priv->display.calc_cdclk_state(dev_priv, 0, actual);
> +	else
> +		*actual = *logical;
>  
>  	return 0;
>  }
> @@ -1823,24 +1744,22 @@ 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.calc_cdclk_state = vlv_calc_cdclk_state;
>  	} 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.calc_cdclk_state = vlv_calc_cdclk_state;
>  	} else if (IS_BROADWELL(dev_priv)) {
>  		dev_priv->display.set_cdclk = bdw_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			bdw_modeset_calc_cdclk;
> -	} else if (IS_GEN9_LP(dev_priv)) {
> +		dev_priv->display.calc_cdclk_state = bdw_calc_cdclk_state;
> +	} else if (IS_BROXTON(dev_priv)) {
> +		dev_priv->display.set_cdclk = bxt_set_cdclk;
> +		dev_priv->display.calc_cdclk_state = bxt_calc_cdclk_state;
> +	} else if (IS_GEMINILAKE(dev_priv)) {
>  		dev_priv->display.set_cdclk = bxt_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			bxt_modeset_calc_cdclk;
> +		dev_priv->display.calc_cdclk_state = glk_calc_cdclk_state;
>  	} 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.calc_cdclk_state = skl_calc_cdclk_state;
>  	}
>  
>  	if (IS_GEN9_BC(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f6feb93..1d2cb491 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12414,8 +12414,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  	 * mode set on this crtc.  For other crtcs we need to use the
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
> -	if (dev_priv->display.modeset_calc_cdclk) {
> -		ret = dev_priv->display.modeset_calc_cdclk(state);
> +	if (dev_priv->display.calc_cdclk_state) {
> +		ret = intel_modeset_calc_cdclk(intel_state);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -15468,7 +15468,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  				pixclk = crtc_state->pixel_rate;
>  			else
> -				WARN_ON(dev_priv->display.modeset_calc_cdclk);
> +				WARN_ON(dev_priv->display.calc_cdclk_state);
>  
>  			/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>  			if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 821c57c..3e112fe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1260,6 +1260,7 @@ bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
>  			       const struct intel_cdclk_state *b);
>  void intel_set_cdclk(struct drm_i915_private *dev_priv,
>  		     const struct intel_cdclk_state *cdclk_state);
> +int intel_modeset_calc_cdclk(struct intel_atomic_state *state);
>  
>  /* intel_display.c */
>  enum transcoder intel_crtc_pch_transcoder(struct intel_crtc *crtc);
> -- 
> 2.7.4

-- 
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/3] drm/i915: unify the x_modeset_calc_cdclk() functions

[Ander Conselvan De Oliveira]

On Mon, 2017-02-20 at 17:00 -0300, Paulo Zanoni wrote:
> There's a lot of duplicated platform-independent logic in the current
> modeset_calc_cdclk() functions. Adding cdclk support for more
> platforms will only add more copies of this code.
> 
> To solve this problem, in this patch we create a new function called
> intel_modeset_calc_cdclk() which unifies the platform-independent
> logic, and we also create a new vfunc called calc_cdclk_state(), which
> is responsible to contain only the platform-dependent code.
> 
> The code is now smaller and support for new platforms should be easier
> and not require duplicated platform-independent code.
> 
> v2: Previously I had 2 different patches addressing these problems,
> but wiht Ville's suggestion I think it makes more sense to keep
> everything in a single patch (Ville).
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   4 +-
>  drivers/gpu/drm/i915/intel_cdclk.c   | 187 ++++++++++-------------------------
>  drivers/gpu/drm/i915/intel_display.c |   6 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   1 +
>  4 files changed, 60 insertions(+), 138 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4e7046d..f1c35c7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -644,7 +644,9 @@ struct drm_i915_display_funcs {
>  				    struct intel_crtc_state *cstate);
>  	int (*compute_global_watermarks)(struct drm_atomic_state *state);
>  	void (*update_wm)(struct intel_crtc *crtc);
> -	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> +	void (*calc_cdclk_state)(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_state);
>  	/* Returns the active state of the crtc, and if the crtc is active,
>  	 * fills out the pipe-config with the hw state. */
>  	bool (*get_pipe_config)(struct intel_crtc *,
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index d643c0c..dd6fe25 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1496,148 +1496,69 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
>  	return max_pixel_rate;
>  }
>  
> -static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> +static void vlv_calc_cdclk_state(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_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;
> +	cdclk_state->cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
>  }
>  
> -static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> +static void bdw_calc_cdclk_state(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_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.
> -	 */
> -	cdclk = bdw_calc_cdclk(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 = bdw_calc_cdclk(0);
> -
> -		intel_state->cdclk.actual.cdclk = cdclk;
> -	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> -	}
> -
> -	return 0;
> +	cdclk_state->cdclk = bdw_calc_cdclk(max_pixclk);
>  }
>  
> -static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> +static void skl_calc_cdclk_state(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_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);
> -	int cdclk, vco;
> +	if (!cdclk_state->vco)
> +		cdclk_state->vco = dev_priv->skl_preferred_vco_freq;
>  
> -	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.
> -	 */
> -	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);
> +	cdclk_state->cdclk = skl_calc_cdclk(max_pixclk, cdclk_state->vco);
> +}
>  
> -		intel_state->cdclk.actual.vco = vco;
> -		intel_state->cdclk.actual.cdclk = cdclk;
> -	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> -	}
> +static void bxt_calc_cdclk_state(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_state)
> +{
> +	cdclk_state->cdclk = bxt_calc_cdclk(max_pixclk);
> +	cdclk_state->vco = bxt_de_pll_vco(dev_priv, cdclk_state->cdclk);
> +}
>  
> -	return 0;
> +static void glk_calc_cdclk_state(struct drm_i915_private *dev_priv,
> +				 int max_pixclk,
> +				 struct intel_cdclk_state *cdclk_state)
> +{
> +	cdclk_state->cdclk = glk_calc_cdclk(max_pixclk);
> +	cdclk_state->vco = glk_de_pll_vco(dev_priv, cdclk_state->cdclk);
>  }
>  
> -static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> +int intel_modeset_calc_cdclk(struct intel_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;
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct intel_cdclk_state *logical = &state->cdclk.logical;
> +	struct intel_cdclk_state *actual = &state->cdclk.actual;
> +	int max_pixclk = intel_max_pixel_rate(&state->base);
>  
> -	if (IS_GEMINILAKE(dev_priv)) {
> -		cdclk = glk_calc_cdclk(max_pixclk);
> -		vco = glk_de_pll_vco(dev_priv, cdclk);
> -	} else {
> -		cdclk = bxt_calc_cdclk(max_pixclk);
> -		vco = bxt_de_pll_vco(dev_priv, cdclk);
> -	}
> +	/*
> +	 * FIXME: should also account for plane ratio once 64bpp pixel formats
> +	 * are supported.
> +	 */
> +	dev_priv->display.calc_cdclk_state(dev_priv, max_pixclk, logical);

I was looking at DK's patch adding minimum cdclk restrictions for glk [1] and
that led me to think the code before the patch should look like

	cdclk = calc_cdclk()
	if (cdclk < min_cdclk)
		cdclk = min_cdclk;

	vco = de_pll_vco(cdclk);

Now that patch will conflict with this and I think we either will have to
replicate that min_cdclk check in every calc_cdclk_state() implementation or
then split that into two hooks: one for the calc_cdclk() part and another for
the vco part. Or something else?


[1] https://patchwork.freedesktop.org/patch/141371/


Ander

>  
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> +	if (logical->cdclk > dev_priv->max_cdclk_freq) {
>  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> +			      logical->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) {
> -		if (IS_GEMINILAKE(dev_priv)) {
> -			cdclk = glk_calc_cdclk(0);
> -			vco = glk_de_pll_vco(dev_priv, cdclk);
> -		} else {
> -			cdclk = bxt_calc_cdclk(0);
> -			vco = bxt_de_pll_vco(dev_priv, cdclk);
> -		}
> -
> -		intel_state->cdclk.actual.vco = vco;
> -		intel_state->cdclk.actual.cdclk = cdclk;
> -	} else {
> -		intel_state->cdclk.actual =
> -			intel_state->cdclk.logical;
> -	}
> +	if (!state->active_crtcs)
> +		dev_priv->display.calc_cdclk_state(dev_priv, 0, actual);
> +	else
> +		*actual = *logical;
>  
>  	return 0;
>  }
> @@ -1823,24 +1744,22 @@ 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.calc_cdclk_state = vlv_calc_cdclk_state;
>  	} 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.calc_cdclk_state = vlv_calc_cdclk_state;
>  	} else if (IS_BROADWELL(dev_priv)) {
>  		dev_priv->display.set_cdclk = bdw_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			bdw_modeset_calc_cdclk;
> -	} else if (IS_GEN9_LP(dev_priv)) {
> +		dev_priv->display.calc_cdclk_state = bdw_calc_cdclk_state;
> +	} else if (IS_BROXTON(dev_priv)) {
> +		dev_priv->display.set_cdclk = bxt_set_cdclk;
> +		dev_priv->display.calc_cdclk_state = bxt_calc_cdclk_state;
> +	} else if (IS_GEMINILAKE(dev_priv)) {
>  		dev_priv->display.set_cdclk = bxt_set_cdclk;
> -		dev_priv->display.modeset_calc_cdclk =
> -			bxt_modeset_calc_cdclk;
> +		dev_priv->display.calc_cdclk_state = glk_calc_cdclk_state;
>  	} 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.calc_cdclk_state = skl_calc_cdclk_state;
>  	}
>  
>  	if (IS_GEN9_BC(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f6feb93..1d2cb491 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12414,8 +12414,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  	 * mode set on this crtc.  For other crtcs we need to use the
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
> -	if (dev_priv->display.modeset_calc_cdclk) {
> -		ret = dev_priv->display.modeset_calc_cdclk(state);
> +	if (dev_priv->display.calc_cdclk_state) {
> +		ret = intel_modeset_calc_cdclk(intel_state);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -15468,7 +15468,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  				pixclk = crtc_state->pixel_rate;
>  			else
> -				WARN_ON(dev_priv->display.modeset_calc_cdclk);
> +				WARN_ON(dev_priv->display.calc_cdclk_state);
>  
>  			/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>  			if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 821c57c..3e112fe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1260,6 +1260,7 @@ bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
>  			       const struct intel_cdclk_state *b);
>  void intel_set_cdclk(struct drm_i915_private *dev_priv,
>  		     const struct intel_cdclk_state *cdclk_state);
> +int intel_modeset_calc_cdclk(struct intel_atomic_state *state);
>  
>  /* intel_display.c */
>  enum transcoder intel_crtc_pch_transcoder(struct intel_crtc *crtc);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx