[CI,4/4] drm/i915: Skip modeset for cdclk changes if possible

Submitted by Imre Deak on March 20, 2019, 1:54 p.m.

Details

Message ID 20190320135439.12201-4-imre.deak@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak March 20, 2019, 1:54 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If we have only a single active pipe and the cdclk change only requires
the cd2x divider to be updated bxt+ can do the update with forcing a full
modeset on the pipe. Try to hook that up.

v2:
- Wait for vblank after an optimized CDCLK change.
- Avoid optimization if the pipe needs a modeset (or was disabled).
- Split CDCLK change to a pre/post plane update step.
v3:
- Use correct version of CDCLK state as old state. (Ville)
- Remove unused intel_cdclk_can_skip_modeset()
v4:
- For consistency call intel_set_cdclk_post_plane_update() only during
  modesets (and not fastsets).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
Tested-by: Abhay Kumar <abhay.kumar@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   3 +-
 drivers/gpu/drm/i915/i915_reg.h      |   3 +-
 drivers/gpu/drm/i915/intel_cdclk.c   | 142 +++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_display.c |  42 ++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  17 ++++-
 5 files changed, 170 insertions(+), 37 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6b10cee4e77f..d8f91525c94c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -277,7 +277,8 @@  struct drm_i915_display_funcs {
 	void (*get_cdclk)(struct drm_i915_private *dev_priv,
 			  struct intel_cdclk_state *cdclk_state);
 	void (*set_cdclk)(struct drm_i915_private *dev_priv,
-			  const struct intel_cdclk_state *cdclk_state);
+			  const struct intel_cdclk_state *cdclk_state,
+			  enum pipe pipe);
 	int (*get_fifo_size)(struct drm_i915_private *dev_priv,
 			     enum i9xx_plane_id i9xx_plane);
 	int (*compute_pipe_wm)(struct intel_crtc_state *cstate);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9b69cec21f7b..12b8170ced96 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9521,7 +9521,8 @@  enum skl_power_gate {
 #define  BXT_CDCLK_CD2X_PIPE(pipe)	((pipe) << 20)
 #define  CDCLK_DIVMUX_CD_OVERRIDE	(1 << 19)
 #define  BXT_CDCLK_CD2X_PIPE_NONE	BXT_CDCLK_CD2X_PIPE(3)
-#define  ICL_CDCLK_CD2X_PIPE_NONE	(7 << 19)
+#define  ICL_CDCLK_CD2X_PIPE(pipe)	((pipe) << 19)
+#define  ICL_CDCLK_CD2X_PIPE_NONE	ICL_CDCLK_CD2X_PIPE(7)
 #define  BXT_CDCLK_SSA_PRECHARGE_ENABLE	(1 << 16)
 #define  CDCLK_FREQ_DECIMAL_MASK	(0x7ff)
 
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 5c25626f8cf0..48cc42a7ef4f 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -516,7 +516,8 @@  static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
 }
 
 static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
-			  const struct intel_cdclk_state *cdclk_state)
+			  const struct intel_cdclk_state *cdclk_state,
+			  enum pipe pipe)
 {
 	int cdclk = cdclk_state->cdclk;
 	u32 val, cmd = cdclk_state->voltage_level;
@@ -598,7 +599,8 @@  static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
 }
 
 static void chv_set_cdclk(struct drm_i915_private *dev_priv,
-			  const struct intel_cdclk_state *cdclk_state)
+			  const struct intel_cdclk_state *cdclk_state,
+			  enum pipe pipe)
 {
 	int cdclk = cdclk_state->cdclk;
 	u32 val, cmd = cdclk_state->voltage_level;
@@ -697,7 +699,8 @@  static void bdw_get_cdclk(struct drm_i915_private *dev_priv,
 }
 
 static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
-			  const struct intel_cdclk_state *cdclk_state)
+			  const struct intel_cdclk_state *cdclk_state,
+			  enum pipe pipe)
 {
 	int cdclk = cdclk_state->cdclk;
 	u32 val;
@@ -987,7 +990,8 @@  static void skl_dpll0_disable(struct drm_i915_private *dev_priv)
 }
 
 static void skl_set_cdclk(struct drm_i915_private *dev_priv,
-			  const struct intel_cdclk_state *cdclk_state)
+			  const struct intel_cdclk_state *cdclk_state,
+			  enum pipe pipe)
 {
 	int cdclk = cdclk_state->cdclk;
 	int vco = cdclk_state->vco;
@@ -1158,7 +1162,7 @@  void skl_init_cdclk(struct drm_i915_private *dev_priv)
 	cdclk_state.cdclk = skl_calc_cdclk(0, cdclk_state.vco);
 	cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
 
-	skl_set_cdclk(dev_priv, &cdclk_state);
+	skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
 }
 
 /**
@@ -1176,7 +1180,7 @@  void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
 	cdclk_state.vco = 0;
 	cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
 
-	skl_set_cdclk(dev_priv, &cdclk_state);
+	skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
 }
 
 static int bxt_calc_cdclk(int min_cdclk)
@@ -1355,7 +1359,8 @@  static void bxt_de_pll_enable(struct drm_i915_private *dev_priv, int vco)
 }
 
 static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
-			  const struct intel_cdclk_state *cdclk_state)
+			  const struct intel_cdclk_state *cdclk_state,
+			  enum pipe pipe)
 {
 	int cdclk = cdclk_state->cdclk;
 	int vco = cdclk_state->vco;
@@ -1408,11 +1413,10 @@  static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 		bxt_de_pll_enable(dev_priv, vco);
 
 	val = divider | skl_cdclk_decimal(cdclk);
-	/*
-	 * FIXME if only the cd2x divider needs changing, it could be done
-	 * without shutting off the pipe (if only one pipe is active).
-	 */
-	val |= BXT_CDCLK_CD2X_PIPE_NONE;
+	if (pipe == INVALID_PIPE)
+		val |= BXT_CDCLK_CD2X_PIPE_NONE;
+	else
+		val |= BXT_CDCLK_CD2X_PIPE(pipe);
 	/*
 	 * Disable SSA Precharge when CD clock frequency < 500 MHz,
 	 * enable otherwise.
@@ -1421,6 +1425,9 @@  static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 		val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
 	I915_WRITE(CDCLK_CTL, val);
 
+	if (pipe != INVALID_PIPE)
+		intel_wait_for_vblank(dev_priv, pipe);
+
 	mutex_lock(&dev_priv->pcu_lock);
 	/*
 	 * The timeout isn't specified, the 2ms used here is based on
@@ -1525,7 +1532,7 @@  void bxt_init_cdclk(struct drm_i915_private *dev_priv)
 	}
 	cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
 
-	bxt_set_cdclk(dev_priv, &cdclk_state);
+	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
 }
 
 /**
@@ -1543,7 +1550,7 @@  void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
 	cdclk_state.vco = 0;
 	cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
 
-	bxt_set_cdclk(dev_priv, &cdclk_state);
+	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
 }
 
 static int cnl_calc_cdclk(int min_cdclk)
@@ -1663,7 +1670,8 @@  static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
 }
 
 static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
-			  const struct intel_cdclk_state *cdclk_state)
+			  const struct intel_cdclk_state *cdclk_state,
+			  enum pipe pipe)
 {
 	int cdclk = cdclk_state->cdclk;
 	int vco = cdclk_state->vco;
@@ -1704,13 +1712,15 @@  static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
 		cnl_cdclk_pll_enable(dev_priv, vco);
 
 	val = divider | skl_cdclk_decimal(cdclk);
-	/*
-	 * FIXME if only the cd2x divider needs changing, it could be done
-	 * without shutting off the pipe (if only one pipe is active).
-	 */
-	val |= BXT_CDCLK_CD2X_PIPE_NONE;
+	if (pipe == INVALID_PIPE)
+		val |= BXT_CDCLK_CD2X_PIPE_NONE;
+	else
+		val |= BXT_CDCLK_CD2X_PIPE(pipe);
 	I915_WRITE(CDCLK_CTL, val);
 
+	if (pipe != INVALID_PIPE)
+		intel_wait_for_vblank(dev_priv, pipe);
+
 	/* inform PCU of the change */
 	mutex_lock(&dev_priv->pcu_lock);
 	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
@@ -1847,10 +1857,12 @@  static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
 }
 
 static void icl_set_cdclk(struct drm_i915_private *dev_priv,
-			  const struct intel_cdclk_state *cdclk_state)
+			  const struct intel_cdclk_state *cdclk_state,
+			  enum pipe pipe)
 {
 	unsigned int cdclk = cdclk_state->cdclk;
 	unsigned int vco = cdclk_state->vco;
+	u32 val;
 	int ret;
 
 	mutex_lock(&dev_priv->pcu_lock);
@@ -1872,8 +1884,15 @@  static void icl_set_cdclk(struct drm_i915_private *dev_priv,
 	if (dev_priv->cdclk.hw.vco != vco)
 		cnl_cdclk_pll_enable(dev_priv, vco);
 
-	I915_WRITE(CDCLK_CTL, ICL_CDCLK_CD2X_PIPE_NONE |
-			      skl_cdclk_decimal(cdclk));
+	val = skl_cdclk_decimal(cdclk);
+	if (pipe == INVALID_PIPE)
+		val |= ICL_CDCLK_CD2X_PIPE_NONE;
+	else
+		val |= ICL_CDCLK_CD2X_PIPE(pipe);
+	I915_WRITE(CDCLK_CTL, val);
+
+	if (pipe != INVALID_PIPE)
+		intel_wait_for_vblank(dev_priv, pipe);
 
 	mutex_lock(&dev_priv->pcu_lock);
 	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
@@ -2002,7 +2021,7 @@  void icl_init_cdclk(struct drm_i915_private *dev_priv)
 	sanitized_state.voltage_level =
 				icl_calc_voltage_level(sanitized_state.cdclk);
 
-	icl_set_cdclk(dev_priv, &sanitized_state);
+	icl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE);
 }
 
 /**
@@ -2020,7 +2039,7 @@  void icl_uninit_cdclk(struct drm_i915_private *dev_priv)
 	cdclk_state.vco = 0;
 	cdclk_state.voltage_level = icl_calc_voltage_level(cdclk_state.cdclk);
 
-	icl_set_cdclk(dev_priv, &cdclk_state);
+	icl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
 }
 
 /**
@@ -2048,7 +2067,7 @@  void cnl_init_cdclk(struct drm_i915_private *dev_priv)
 	cdclk_state.vco = cnl_cdclk_pll_vco(dev_priv, cdclk_state.cdclk);
 	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
 
-	cnl_set_cdclk(dev_priv, &cdclk_state);
+	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
 }
 
 /**
@@ -2066,7 +2085,7 @@  void cnl_uninit_cdclk(struct drm_i915_private *dev_priv)
 	cdclk_state.vco = 0;
 	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
 
-	cnl_set_cdclk(dev_priv, &cdclk_state);
+	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
 }
 
 /**
@@ -2086,6 +2105,27 @@  bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
 }
 
 /**
+ * intel_cdclk_needs_cd2x_update - Determine if two CDCLK states require a cd2x divider update
+ * @a: first CDCLK state
+ * @b: second CDCLK state
+ *
+ * Returns:
+ * True if the CDCLK states require just a cd2x divider update, false if not.
+ */
+bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv,
+				   const struct intel_cdclk_state *a,
+				   const struct intel_cdclk_state *b)
+{
+	/* Older hw doesn't have the capability */
+	if (INTEL_GEN(dev_priv) < 10 && !IS_GEN9_LP(dev_priv))
+		return false;
+
+	return a->cdclk != b->cdclk &&
+		a->vco == b->vco &&
+		a->ref == b->ref;
+}
+
+/**
  * intel_cdclk_changed - Determine if two CDCLK states are different
  * @a: first CDCLK state
  * @b: second CDCLK state
@@ -2133,12 +2173,14 @@  void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
  * intel_set_cdclk - Push the CDCLK state to the hardware
  * @dev_priv: i915 device
  * @cdclk_state: new CDCLK state
+ * @pipe: pipe with which to synchronize the update
  *
  * Program the hardware based on the passed in CDCLK state,
  * if necessary.
  */
-void intel_set_cdclk(struct drm_i915_private *dev_priv,
-		     const struct intel_cdclk_state *cdclk_state)
+static void intel_set_cdclk(struct drm_i915_private *dev_priv,
+			    const struct intel_cdclk_state *cdclk_state,
+			    enum pipe pipe)
 {
 	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state))
 		return;
@@ -2148,7 +2190,7 @@  void intel_set_cdclk(struct drm_i915_private *dev_priv,
 
 	intel_dump_cdclk_state(cdclk_state, "Changing CDCLK to");
 
-	dev_priv->display.set_cdclk(dev_priv, cdclk_state);
+	dev_priv->display.set_cdclk(dev_priv, cdclk_state, pipe);
 
 	if (WARN(intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state),
 		 "cdclk state doesn't match!\n")) {
@@ -2157,6 +2199,46 @@  void intel_set_cdclk(struct drm_i915_private *dev_priv,
 	}
 }
 
+/**
+ * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
+ * @dev_priv: i915 device
+ * @old_state: old CDCLK state
+ * @new_state: new CDCLK state
+ * @pipe: pipe with which to synchronize the update
+ *
+ * Program the hardware before updating the HW plane state based on the passed
+ * in CDCLK state, if necessary.
+ */
+void
+intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv,
+				 const struct intel_cdclk_state *old_state,
+				 const struct intel_cdclk_state *new_state,
+				 enum pipe pipe)
+{
+	if (pipe == INVALID_PIPE || old_state->cdclk <= new_state->cdclk)
+		intel_set_cdclk(dev_priv, new_state, pipe);
+}
+
+/**
+ * intel_set_cdclk_post_plane_update - Push the CDCLK state to the hardware
+ * @dev_priv: i915 device
+ * @old_state: old CDCLK state
+ * @new_state: new CDCLK state
+ * @pipe: pipe with which to synchronize the update
+ *
+ * Program the hardware after updating the HW plane state based on the passed
+ * in CDCLK state, if necessary.
+ */
+void
+intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
+				  const struct intel_cdclk_state *old_state,
+				  const struct intel_cdclk_state *new_state,
+				  enum pipe pipe)
+{
+	if (pipe != INVALID_PIPE && old_state->cdclk > new_state->cdclk)
+		intel_set_cdclk(dev_priv, new_state, pipe);
+}
+
 static int intel_pixel_rate_to_cdclk(struct drm_i915_private *dev_priv,
 				     int pixel_rate)
 {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f7b44773e1e7..bcec03f43d3a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12982,6 +12982,7 @@  static int intel_modeset_checks(struct drm_atomic_state *state)
 	intel_state->active_crtcs = dev_priv->active_crtcs;
 	intel_state->cdclk.logical = dev_priv->cdclk.logical;
 	intel_state->cdclk.actual = dev_priv->cdclk.actual;
+	intel_state->cdclk.pipe = INVALID_PIPE;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		if (new_crtc_state->active)
@@ -13001,6 +13002,8 @@  static int intel_modeset_checks(struct drm_atomic_state *state)
 	 * adjusted_mode bits in the crtc directly.
 	 */
 	if (dev_priv->display.modeset_calc_cdclk) {
+		enum pipe pipe;
+
 		ret = dev_priv->display.modeset_calc_cdclk(state);
 		if (ret < 0)
 			return ret;
@@ -13017,12 +13020,36 @@  static int intel_modeset_checks(struct drm_atomic_state *state)
 				return ret;
 		}
 
+		if (is_power_of_2(intel_state->active_crtcs)) {
+			struct drm_crtc *crtc;
+			struct drm_crtc_state *crtc_state;
+
+			pipe = ilog2(intel_state->active_crtcs);
+			crtc = &intel_get_crtc_for_pipe(dev_priv, pipe)->base;
+			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+			if (crtc_state && needs_modeset(crtc_state))
+				pipe = INVALID_PIPE;
+		} else {
+			pipe = INVALID_PIPE;
+		}
+
 		/* All pipes must be switched off while we change the cdclk. */
-		if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
-					      &intel_state->cdclk.actual)) {
+		if (pipe != INVALID_PIPE &&
+		    intel_cdclk_needs_cd2x_update(dev_priv,
+						  &dev_priv->cdclk.actual,
+						  &intel_state->cdclk.actual)) {
+			ret = intel_lock_all_pipes(state);
+			if (ret < 0)
+				return ret;
+
+			intel_state->cdclk.pipe = pipe;
+		} else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
+						     &intel_state->cdclk.actual)) {
 			ret = intel_modeset_all_pipes(state);
 			if (ret < 0)
 				return ret;
+
+			intel_state->cdclk.pipe = INVALID_PIPE;
 		}
 
 		DRM_DEBUG_KMS("New cdclk calculated to be logical %u kHz, actual %u kHz\n",
@@ -13431,7 +13458,10 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	if (intel_state->modeset) {
 		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
 
-		intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
+		intel_set_cdclk_pre_plane_update(dev_priv,
+						 &intel_state->cdclk.actual,
+						 &dev_priv->cdclk.actual,
+						 intel_state->cdclk.pipe);
 
 		/*
 		 * SKL workaround: bspec recommends we disable the SAGV when we
@@ -13460,6 +13490,12 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	dev_priv->display.update_crtcs(state);
 
+	if (intel_state->modeset)
+		intel_set_cdclk_post_plane_update(dev_priv,
+						  &intel_state->cdclk.actual,
+						  &dev_priv->cdclk.actual,
+						  intel_state->cdclk.pipe);
+
 	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
 	 * already, but still need the state for the delayed optimization. To
 	 * fix this:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 85dd6a9d1e42..d22a0e92e0d4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -559,6 +559,8 @@  struct intel_atomic_state {
 
 		int force_min_cdclk;
 		bool force_min_cdclk_changed;
+		/* pipe to which cd2x update is synchronized */
+		enum pipe pipe;
 	} cdclk;
 
 	bool dpll_set, modeset;
@@ -1694,13 +1696,24 @@  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
 void intel_update_max_cdclk(struct drm_i915_private *dev_priv);
 void intel_update_cdclk(struct drm_i915_private *dev_priv);
 void intel_update_rawclk(struct drm_i915_private *dev_priv);
+bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv,
+				   const struct intel_cdclk_state *a,
+				   const struct intel_cdclk_state *b);
 bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
 			       const struct intel_cdclk_state *b);
 bool intel_cdclk_changed(const struct intel_cdclk_state *a,
 			 const struct intel_cdclk_state *b);
 void intel_cdclk_swap_state(struct intel_atomic_state *state);
-void intel_set_cdclk(struct drm_i915_private *dev_priv,
-		     const struct intel_cdclk_state *cdclk_state);
+void
+intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv,
+				 const struct intel_cdclk_state *old_state,
+				 const struct intel_cdclk_state *new_state,
+				 enum pipe pipe);
+void
+intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
+				  const struct intel_cdclk_state *old_state,
+				  const struct intel_cdclk_state *new_state,
+				  enum pipe pipe);
 void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
 			    const char *context);
 

Comments

On 3/20/19 6:54 AM, Imre Deak wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> If we have only a single active pipe and the cdclk change only requires
> the cd2x divider to be updated bxt+ can do the update with forcing a full
> modeset on the pipe. Try to hook that up.
>
> v2:
> - Wait for vblank after an optimized CDCLK change.
> - Avoid optimization if the pipe needs a modeset (or was disabled).
> - Split CDCLK change to a pre/post plane update step.
> v3:
> - Use correct version of CDCLK state as old state. (Ville)
> - Remove unused intel_cdclk_can_skip_modeset()
> v4:
> - For consistency call intel_set_cdclk_post_plane_update() only during
>    modesets (and not fastsets).
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> Tested-by: Abhay Kumar <abhay.kumar@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |   3 +-
>   drivers/gpu/drm/i915/i915_reg.h      |   3 +-
>   drivers/gpu/drm/i915/intel_cdclk.c   | 142 +++++++++++++++++++++++++++--------
>   drivers/gpu/drm/i915/intel_display.c |  42 ++++++++++-
>   drivers/gpu/drm/i915/intel_drv.h     |  17 ++++-
>   5 files changed, 170 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6b10cee4e77f..d8f91525c94c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -277,7 +277,8 @@ struct drm_i915_display_funcs {
>   	void (*get_cdclk)(struct drm_i915_private *dev_priv,
>   			  struct intel_cdclk_state *cdclk_state);
>   	void (*set_cdclk)(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state);
> +			  const struct intel_cdclk_state *cdclk_state,
> +			  enum pipe pipe);
>   	int (*get_fifo_size)(struct drm_i915_private *dev_priv,
>   			     enum i9xx_plane_id i9xx_plane);
>   	int (*compute_pipe_wm)(struct intel_crtc_state *cstate);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9b69cec21f7b..12b8170ced96 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9521,7 +9521,8 @@ enum skl_power_gate {
>   #define  BXT_CDCLK_CD2X_PIPE(pipe)	((pipe) << 20)
>   #define  CDCLK_DIVMUX_CD_OVERRIDE	(1 << 19)
>   #define  BXT_CDCLK_CD2X_PIPE_NONE	BXT_CDCLK_CD2X_PIPE(3)
> -#define  ICL_CDCLK_CD2X_PIPE_NONE	(7 << 19)
> +#define  ICL_CDCLK_CD2X_PIPE(pipe)	((pipe) << 19)

Unfortunately bits 21:19 of CDCLK_CTL don't match the pipe enum. This 
MACRO will not work except for PIPE_A.

Pipe_A is 0x00, PIPE_B is 0x02, PIPE_C is 0x06.

In BXT only bits 21:20 were used for CD2X pipe select and the pipe enum 
does work.


-Clint



> +#define  ICL_CDCLK_CD2X_PIPE_NONE	ICL_CDCLK_CD2X_PIPE(7)
>   #define  BXT_CDCLK_SSA_PRECHARGE_ENABLE	(1 << 16)
>   #define  CDCLK_FREQ_DECIMAL_MASK	(0x7ff)
>   
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 5c25626f8cf0..48cc42a7ef4f 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -516,7 +516,8 @@ static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
>   }
>   
>   static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state)
> +			  const struct intel_cdclk_state *cdclk_state,
> +			  enum pipe pipe)
>   {
>   	int cdclk = cdclk_state->cdclk;
>   	u32 val, cmd = cdclk_state->voltage_level;
> @@ -598,7 +599,8 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
>   }
>   
>   static void chv_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state)
> +			  const struct intel_cdclk_state *cdclk_state,
> +			  enum pipe pipe)
>   {
>   	int cdclk = cdclk_state->cdclk;
>   	u32 val, cmd = cdclk_state->voltage_level;
> @@ -697,7 +699,8 @@ static void bdw_get_cdclk(struct drm_i915_private *dev_priv,
>   }
>   
>   static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state)
> +			  const struct intel_cdclk_state *cdclk_state,
> +			  enum pipe pipe)
>   {
>   	int cdclk = cdclk_state->cdclk;
>   	u32 val;
> @@ -987,7 +990,8 @@ static void skl_dpll0_disable(struct drm_i915_private *dev_priv)
>   }
>   
>   static void skl_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state)
> +			  const struct intel_cdclk_state *cdclk_state,
> +			  enum pipe pipe)
>   {
>   	int cdclk = cdclk_state->cdclk;
>   	int vco = cdclk_state->vco;
> @@ -1158,7 +1162,7 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
>   	cdclk_state.cdclk = skl_calc_cdclk(0, cdclk_state.vco);
>   	cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
>   
> -	skl_set_cdclk(dev_priv, &cdclk_state);
> +	skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>   }
>   
>   /**
> @@ -1176,7 +1180,7 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>   	cdclk_state.vco = 0;
>   	cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
>   
> -	skl_set_cdclk(dev_priv, &cdclk_state);
> +	skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>   }
>   
>   static int bxt_calc_cdclk(int min_cdclk)
> @@ -1355,7 +1359,8 @@ static void bxt_de_pll_enable(struct drm_i915_private *dev_priv, int vco)
>   }
>   
>   static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state)
> +			  const struct intel_cdclk_state *cdclk_state,
> +			  enum pipe pipe)
>   {
>   	int cdclk = cdclk_state->cdclk;
>   	int vco = cdclk_state->vco;
> @@ -1408,11 +1413,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>   		bxt_de_pll_enable(dev_priv, vco);
>   
>   	val = divider | skl_cdclk_decimal(cdclk);
> -	/*
> -	 * FIXME if only the cd2x divider needs changing, it could be done
> -	 * without shutting off the pipe (if only one pipe is active).
> -	 */
> -	val |= BXT_CDCLK_CD2X_PIPE_NONE;
> +	if (pipe == INVALID_PIPE)
> +		val |= BXT_CDCLK_CD2X_PIPE_NONE;
> +	else
> +		val |= BXT_CDCLK_CD2X_PIPE(pipe);
>   	/*
>   	 * Disable SSA Precharge when CD clock frequency < 500 MHz,
>   	 * enable otherwise.
> @@ -1421,6 +1425,9 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>   		val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
>   	I915_WRITE(CDCLK_CTL, val);
>   
> +	if (pipe != INVALID_PIPE)
> +		intel_wait_for_vblank(dev_priv, pipe);
> +
>   	mutex_lock(&dev_priv->pcu_lock);
>   	/*
>   	 * The timeout isn't specified, the 2ms used here is based on
> @@ -1525,7 +1532,7 @@ void bxt_init_cdclk(struct drm_i915_private *dev_priv)
>   	}
>   	cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
>   
> -	bxt_set_cdclk(dev_priv, &cdclk_state);
> +	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>   }
>   
>   /**
> @@ -1543,7 +1550,7 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
>   	cdclk_state.vco = 0;
>   	cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
>   
> -	bxt_set_cdclk(dev_priv, &cdclk_state);
> +	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>   }
>   
>   static int cnl_calc_cdclk(int min_cdclk)
> @@ -1663,7 +1670,8 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
>   }
>   
>   static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state)
> +			  const struct intel_cdclk_state *cdclk_state,
> +			  enum pipe pipe)
>   {
>   	int cdclk = cdclk_state->cdclk;
>   	int vco = cdclk_state->vco;
> @@ -1704,13 +1712,15 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
>   		cnl_cdclk_pll_enable(dev_priv, vco);
>   
>   	val = divider | skl_cdclk_decimal(cdclk);
> -	/*
> -	 * FIXME if only the cd2x divider needs changing, it could be done
> -	 * without shutting off the pipe (if only one pipe is active).
> -	 */
> -	val |= BXT_CDCLK_CD2X_PIPE_NONE;
> +	if (pipe == INVALID_PIPE)
> +		val |= BXT_CDCLK_CD2X_PIPE_NONE;
> +	else
> +		val |= BXT_CDCLK_CD2X_PIPE(pipe);
>   	I915_WRITE(CDCLK_CTL, val);
>   
> +	if (pipe != INVALID_PIPE)
> +		intel_wait_for_vblank(dev_priv, pipe);
> +
>   	/* inform PCU of the change */
>   	mutex_lock(&dev_priv->pcu_lock);
>   	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> @@ -1847,10 +1857,12 @@ static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
>   }
>   
>   static void icl_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state)
> +			  const struct intel_cdclk_state *cdclk_state,
> +			  enum pipe pipe)
>   {
>   	unsigned int cdclk = cdclk_state->cdclk;
>   	unsigned int vco = cdclk_state->vco;
> +	u32 val;
>   	int ret;
>   
>   	mutex_lock(&dev_priv->pcu_lock);
> @@ -1872,8 +1884,15 @@ static void icl_set_cdclk(struct drm_i915_private *dev_priv,
>   	if (dev_priv->cdclk.hw.vco != vco)
>   		cnl_cdclk_pll_enable(dev_priv, vco);
>   
> -	I915_WRITE(CDCLK_CTL, ICL_CDCLK_CD2X_PIPE_NONE |
> -			      skl_cdclk_decimal(cdclk));
> +	val = skl_cdclk_decimal(cdclk);
> +	if (pipe == INVALID_PIPE)
> +		val |= ICL_CDCLK_CD2X_PIPE_NONE;
> +	else
> +		val |= ICL_CDCLK_CD2X_PIPE(pipe);
> +	I915_WRITE(CDCLK_CTL, val);
> +
> +	if (pipe != INVALID_PIPE)
> +		intel_wait_for_vblank(dev_priv, pipe);
>   
>   	mutex_lock(&dev_priv->pcu_lock);
>   	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> @@ -2002,7 +2021,7 @@ void icl_init_cdclk(struct drm_i915_private *dev_priv)
>   	sanitized_state.voltage_level =
>   				icl_calc_voltage_level(sanitized_state.cdclk);
>   
> -	icl_set_cdclk(dev_priv, &sanitized_state);
> +	icl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE);
>   }
>   
>   /**
> @@ -2020,7 +2039,7 @@ void icl_uninit_cdclk(struct drm_i915_private *dev_priv)
>   	cdclk_state.vco = 0;
>   	cdclk_state.voltage_level = icl_calc_voltage_level(cdclk_state.cdclk);
>   
> -	icl_set_cdclk(dev_priv, &cdclk_state);
> +	icl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>   }
>   
>   /**
> @@ -2048,7 +2067,7 @@ void cnl_init_cdclk(struct drm_i915_private *dev_priv)
>   	cdclk_state.vco = cnl_cdclk_pll_vco(dev_priv, cdclk_state.cdclk);
>   	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
>   
> -	cnl_set_cdclk(dev_priv, &cdclk_state);
> +	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>   }
>   
>   /**
> @@ -2066,7 +2085,7 @@ void cnl_uninit_cdclk(struct drm_i915_private *dev_priv)
>   	cdclk_state.vco = 0;
>   	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
>   
> -	cnl_set_cdclk(dev_priv, &cdclk_state);
> +	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>   }
>   
>   /**
> @@ -2086,6 +2105,27 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
>   }
>   
>   /**
> + * intel_cdclk_needs_cd2x_update - Determine if two CDCLK states require a cd2x divider update
> + * @a: first CDCLK state
> + * @b: second CDCLK state
> + *
> + * Returns:
> + * True if the CDCLK states require just a cd2x divider update, false if not.
> + */
> +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv,
> +				   const struct intel_cdclk_state *a,
> +				   const struct intel_cdclk_state *b)
> +{
> +	/* Older hw doesn't have the capability */
> +	if (INTEL_GEN(dev_priv) < 10 && !IS_GEN9_LP(dev_priv))
> +		return false;
> +
> +	return a->cdclk != b->cdclk &&
> +		a->vco == b->vco &&
> +		a->ref == b->ref;
> +}
> +
> +/**
>    * intel_cdclk_changed - Determine if two CDCLK states are different
>    * @a: first CDCLK state
>    * @b: second CDCLK state
> @@ -2133,12 +2173,14 @@ void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
>    * intel_set_cdclk - Push the CDCLK state to the hardware
>    * @dev_priv: i915 device
>    * @cdclk_state: new CDCLK state
> + * @pipe: pipe with which to synchronize the update
>    *
>    * Program the hardware based on the passed in CDCLK state,
>    * if necessary.
>    */
> -void intel_set_cdclk(struct drm_i915_private *dev_priv,
> -		     const struct intel_cdclk_state *cdclk_state)
> +static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> +			    const struct intel_cdclk_state *cdclk_state,
> +			    enum pipe pipe)
>   {
>   	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state))
>   		return;
> @@ -2148,7 +2190,7 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
>   
>   	intel_dump_cdclk_state(cdclk_state, "Changing CDCLK to");
>   
> -	dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> +	dev_priv->display.set_cdclk(dev_priv, cdclk_state, pipe);
>   
>   	if (WARN(intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state),
>   		 "cdclk state doesn't match!\n")) {
> @@ -2157,6 +2199,46 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
>   	}
>   }
>   
> +/**
> + * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> + * @dev_priv: i915 device
> + * @old_state: old CDCLK state
> + * @new_state: new CDCLK state
> + * @pipe: pipe with which to synchronize the update
> + *
> + * Program the hardware before updating the HW plane state based on the passed
> + * in CDCLK state, if necessary.
> + */
> +void
> +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv,
> +				 const struct intel_cdclk_state *old_state,
> +				 const struct intel_cdclk_state *new_state,
> +				 enum pipe pipe)
> +{
> +	if (pipe == INVALID_PIPE || old_state->cdclk <= new_state->cdclk)
> +		intel_set_cdclk(dev_priv, new_state, pipe);
> +}
> +
> +/**
> + * intel_set_cdclk_post_plane_update - Push the CDCLK state to the hardware
> + * @dev_priv: i915 device
> + * @old_state: old CDCLK state
> + * @new_state: new CDCLK state
> + * @pipe: pipe with which to synchronize the update
> + *
> + * Program the hardware after updating the HW plane state based on the passed
> + * in CDCLK state, if necessary.
> + */
> +void
> +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
> +				  const struct intel_cdclk_state *old_state,
> +				  const struct intel_cdclk_state *new_state,
> +				  enum pipe pipe)
> +{
> +	if (pipe != INVALID_PIPE && old_state->cdclk > new_state->cdclk)
> +		intel_set_cdclk(dev_priv, new_state, pipe);
> +}
> +
>   static int intel_pixel_rate_to_cdclk(struct drm_i915_private *dev_priv,
>   				     int pixel_rate)
>   {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f7b44773e1e7..bcec03f43d3a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12982,6 +12982,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>   	intel_state->active_crtcs = dev_priv->active_crtcs;
>   	intel_state->cdclk.logical = dev_priv->cdclk.logical;
>   	intel_state->cdclk.actual = dev_priv->cdclk.actual;
> +	intel_state->cdclk.pipe = INVALID_PIPE;
>   
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>   		if (new_crtc_state->active)
> @@ -13001,6 +13002,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>   	 * adjusted_mode bits in the crtc directly.
>   	 */
>   	if (dev_priv->display.modeset_calc_cdclk) {
> +		enum pipe pipe;
> +
>   		ret = dev_priv->display.modeset_calc_cdclk(state);
>   		if (ret < 0)
>   			return ret;
> @@ -13017,12 +13020,36 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>   				return ret;
>   		}
>   
> +		if (is_power_of_2(intel_state->active_crtcs)) {
> +			struct drm_crtc *crtc;
> +			struct drm_crtc_state *crtc_state;
> +
> +			pipe = ilog2(intel_state->active_crtcs);
> +			crtc = &intel_get_crtc_for_pipe(dev_priv, pipe)->base;
> +			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +			if (crtc_state && needs_modeset(crtc_state))
> +				pipe = INVALID_PIPE;
> +		} else {
> +			pipe = INVALID_PIPE;
> +		}
> +
>   		/* All pipes must be switched off while we change the cdclk. */
> -		if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> -					      &intel_state->cdclk.actual)) {
> +		if (pipe != INVALID_PIPE &&
> +		    intel_cdclk_needs_cd2x_update(dev_priv,
> +						  &dev_priv->cdclk.actual,
> +						  &intel_state->cdclk.actual)) {
> +			ret = intel_lock_all_pipes(state);
> +			if (ret < 0)
> +				return ret;
> +
> +			intel_state->cdclk.pipe = pipe;
> +		} else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> +						     &intel_state->cdclk.actual)) {
>   			ret = intel_modeset_all_pipes(state);
>   			if (ret < 0)
>   				return ret;
> +
> +			intel_state->cdclk.pipe = INVALID_PIPE;
>   		}
>   
>   		DRM_DEBUG_KMS("New cdclk calculated to be logical %u kHz, actual %u kHz\n",
> @@ -13431,7 +13458,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>   	if (intel_state->modeset) {
>   		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>   
> -		intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> +		intel_set_cdclk_pre_plane_update(dev_priv,
> +						 &intel_state->cdclk.actual,
> +						 &dev_priv->cdclk.actual,
> +						 intel_state->cdclk.pipe);
>   
>   		/*
>   		 * SKL workaround: bspec recommends we disable the SAGV when we
> @@ -13460,6 +13490,12 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>   	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>   	dev_priv->display.update_crtcs(state);
>   
> +	if (intel_state->modeset)
> +		intel_set_cdclk_post_plane_update(dev_priv,
> +						  &intel_state->cdclk.actual,
> +						  &dev_priv->cdclk.actual,
> +						  intel_state->cdclk.pipe);
> +
>   	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
>   	 * already, but still need the state for the delayed optimization. To
>   	 * fix this:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 85dd6a9d1e42..d22a0e92e0d4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -559,6 +559,8 @@ struct intel_atomic_state {
>   
>   		int force_min_cdclk;
>   		bool force_min_cdclk_changed;
> +		/* pipe to which cd2x update is synchronized */
> +		enum pipe pipe;
>   	} cdclk;
>   
>   	bool dpll_set, modeset;
> @@ -1694,13 +1696,24 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
>   void intel_update_max_cdclk(struct drm_i915_private *dev_priv);
>   void intel_update_cdclk(struct drm_i915_private *dev_priv);
>   void intel_update_rawclk(struct drm_i915_private *dev_priv);
> +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv,
> +				   const struct intel_cdclk_state *a,
> +				   const struct intel_cdclk_state *b);
>   bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
>   			       const struct intel_cdclk_state *b);
>   bool intel_cdclk_changed(const struct intel_cdclk_state *a,
>   			 const struct intel_cdclk_state *b);
>   void intel_cdclk_swap_state(struct intel_atomic_state *state);
> -void intel_set_cdclk(struct drm_i915_private *dev_priv,
> -		     const struct intel_cdclk_state *cdclk_state);
> +void
> +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv,
> +				 const struct intel_cdclk_state *old_state,
> +				 const struct intel_cdclk_state *new_state,
> +				 enum pipe pipe);
> +void
> +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
> +				  const struct intel_cdclk_state *old_state,
> +				  const struct intel_cdclk_state *new_state,
> +				  enum pipe pipe);
>   void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
>   			    const char *context);
>
On Thu, Mar 21, 2019 at 02:53:00PM -0700, Clinton Taylor wrote:
> 
> On 3/20/19 6:54 AM, Imre Deak wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If we have only a single active pipe and the cdclk change only requires
> > the cd2x divider to be updated bxt+ can do the update with forcing a full
> > modeset on the pipe. Try to hook that up.
> > 
> > v2:
> > - Wait for vblank after an optimized CDCLK change.
> > - Avoid optimization if the pipe needs a modeset (or was disabled).
> > - Split CDCLK change to a pre/post plane update step.
> > v3:
> > - Use correct version of CDCLK state as old state. (Ville)
> > - Remove unused intel_cdclk_can_skip_modeset()
> > v4:
> > - For consistency call intel_set_cdclk_post_plane_update() only during
> >    modesets (and not fastsets).
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> > Tested-by: Abhay Kumar <abhay.kumar@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h      |   3 +-
> >   drivers/gpu/drm/i915/i915_reg.h      |   3 +-
> >   drivers/gpu/drm/i915/intel_cdclk.c   | 142 +++++++++++++++++++++++++++--------
> >   drivers/gpu/drm/i915/intel_display.c |  42 ++++++++++-
> >   drivers/gpu/drm/i915/intel_drv.h     |  17 ++++-
> >   5 files changed, 170 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 6b10cee4e77f..d8f91525c94c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -277,7 +277,8 @@ struct drm_i915_display_funcs {
> >   	void (*get_cdclk)(struct drm_i915_private *dev_priv,
> >   			  struct intel_cdclk_state *cdclk_state);
> >   	void (*set_cdclk)(struct drm_i915_private *dev_priv,
> > -			  const struct intel_cdclk_state *cdclk_state);
> > +			  const struct intel_cdclk_state *cdclk_state,
> > +			  enum pipe pipe);
> >   	int (*get_fifo_size)(struct drm_i915_private *dev_priv,
> >   			     enum i9xx_plane_id i9xx_plane);
> >   	int (*compute_pipe_wm)(struct intel_crtc_state *cstate);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 9b69cec21f7b..12b8170ced96 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -9521,7 +9521,8 @@ enum skl_power_gate {
> >   #define  BXT_CDCLK_CD2X_PIPE(pipe)	((pipe) << 20)
> >   #define  CDCLK_DIVMUX_CD_OVERRIDE	(1 << 19)
> >   #define  BXT_CDCLK_CD2X_PIPE_NONE	BXT_CDCLK_CD2X_PIPE(3)
> > -#define  ICL_CDCLK_CD2X_PIPE_NONE	(7 << 19)
> > +#define  ICL_CDCLK_CD2X_PIPE(pipe)	((pipe) << 19)
> 
> Unfortunately bits 21:19 of CDCLK_CTL don't match the pipe enum. This MACRO
> will not work except for PIPE_A.
> 
> Pipe_A is 0x00, PIPE_B is 0x02, PIPE_C is 0x06.

Good catch.

The pipe B and C encodings could be typoed though and I couldn't trigger
any problems with either encodings (i.e. using either 0x1 or 0x2 for
pipe B updates while pipe B was the only pipe enabled). So I opened a
ticket for this in BSpec, let's wait for a clarification on this.

> In BXT only bits 21:20 were used for CD2X pipe select and the pipe enum does
> work.
> 
> 
> -Clint
> 
> 
> 
> > +#define  ICL_CDCLK_CD2X_PIPE_NONE	ICL_CDCLK_CD2X_PIPE(7)
> >   #define  BXT_CDCLK_SSA_PRECHARGE_ENABLE	(1 << 16)
> >   #define  CDCLK_FREQ_DECIMAL_MASK	(0x7ff)
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 5c25626f8cf0..48cc42a7ef4f 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -516,7 +516,8 @@ static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> >   }
> >   static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
> > -			  const struct intel_cdclk_state *cdclk_state)
> > +			  const struct intel_cdclk_state *cdclk_state,
> > +			  enum pipe pipe)
> >   {
> >   	int cdclk = cdclk_state->cdclk;
> >   	u32 val, cmd = cdclk_state->voltage_level;
> > @@ -598,7 +599,8 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
> >   }
> >   static void chv_set_cdclk(struct drm_i915_private *dev_priv,
> > -			  const struct intel_cdclk_state *cdclk_state)
> > +			  const struct intel_cdclk_state *cdclk_state,
> > +			  enum pipe pipe)
> >   {
> >   	int cdclk = cdclk_state->cdclk;
> >   	u32 val, cmd = cdclk_state->voltage_level;
> > @@ -697,7 +699,8 @@ static void bdw_get_cdclk(struct drm_i915_private *dev_priv,
> >   }
> >   static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
> > -			  const struct intel_cdclk_state *cdclk_state)
> > +			  const struct intel_cdclk_state *cdclk_state,
> > +			  enum pipe pipe)
> >   {
> >   	int cdclk = cdclk_state->cdclk;
> >   	u32 val;
> > @@ -987,7 +990,8 @@ static void skl_dpll0_disable(struct drm_i915_private *dev_priv)
> >   }
> >   static void skl_set_cdclk(struct drm_i915_private *dev_priv,
> > -			  const struct intel_cdclk_state *cdclk_state)
> > +			  const struct intel_cdclk_state *cdclk_state,
> > +			  enum pipe pipe)
> >   {
> >   	int cdclk = cdclk_state->cdclk;
> >   	int vco = cdclk_state->vco;
> > @@ -1158,7 +1162,7 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
> >   	cdclk_state.cdclk = skl_calc_cdclk(0, cdclk_state.vco);
> >   	cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
> > -	skl_set_cdclk(dev_priv, &cdclk_state);
> > +	skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> >   }
> >   /**
> > @@ -1176,7 +1180,7 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
> >   	cdclk_state.vco = 0;
> >   	cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
> > -	skl_set_cdclk(dev_priv, &cdclk_state);
> > +	skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> >   }
> >   static int bxt_calc_cdclk(int min_cdclk)
> > @@ -1355,7 +1359,8 @@ static void bxt_de_pll_enable(struct drm_i915_private *dev_priv, int vco)
> >   }
> >   static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > -			  const struct intel_cdclk_state *cdclk_state)
> > +			  const struct intel_cdclk_state *cdclk_state,
> > +			  enum pipe pipe)
> >   {
> >   	int cdclk = cdclk_state->cdclk;
> >   	int vco = cdclk_state->vco;
> > @@ -1408,11 +1413,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >   		bxt_de_pll_enable(dev_priv, vco);
> >   	val = divider | skl_cdclk_decimal(cdclk);
> > -	/*
> > -	 * FIXME if only the cd2x divider needs changing, it could be done
> > -	 * without shutting off the pipe (if only one pipe is active).
> > -	 */
> > -	val |= BXT_CDCLK_CD2X_PIPE_NONE;
> > +	if (pipe == INVALID_PIPE)
> > +		val |= BXT_CDCLK_CD2X_PIPE_NONE;
> > +	else
> > +		val |= BXT_CDCLK_CD2X_PIPE(pipe);
> >   	/*
> >   	 * Disable SSA Precharge when CD clock frequency < 500 MHz,
> >   	 * enable otherwise.
> > @@ -1421,6 +1425,9 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >   		val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> >   	I915_WRITE(CDCLK_CTL, val);
> > +	if (pipe != INVALID_PIPE)
> > +		intel_wait_for_vblank(dev_priv, pipe);
> > +
> >   	mutex_lock(&dev_priv->pcu_lock);
> >   	/*
> >   	 * The timeout isn't specified, the 2ms used here is based on
> > @@ -1525,7 +1532,7 @@ void bxt_init_cdclk(struct drm_i915_private *dev_priv)
> >   	}
> >   	cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
> > -	bxt_set_cdclk(dev_priv, &cdclk_state);
> > +	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> >   }
> >   /**
> > @@ -1543,7 +1550,7 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
> >   	cdclk_state.vco = 0;
> >   	cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
> > -	bxt_set_cdclk(dev_priv, &cdclk_state);
> > +	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> >   }
> >   static int cnl_calc_cdclk(int min_cdclk)
> > @@ -1663,7 +1670,8 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
> >   }
> >   static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> > -			  const struct intel_cdclk_state *cdclk_state)
> > +			  const struct intel_cdclk_state *cdclk_state,
> > +			  enum pipe pipe)
> >   {
> >   	int cdclk = cdclk_state->cdclk;
> >   	int vco = cdclk_state->vco;
> > @@ -1704,13 +1712,15 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> >   		cnl_cdclk_pll_enable(dev_priv, vco);
> >   	val = divider | skl_cdclk_decimal(cdclk);
> > -	/*
> > -	 * FIXME if only the cd2x divider needs changing, it could be done
> > -	 * without shutting off the pipe (if only one pipe is active).
> > -	 */
> > -	val |= BXT_CDCLK_CD2X_PIPE_NONE;
> > +	if (pipe == INVALID_PIPE)
> > +		val |= BXT_CDCLK_CD2X_PIPE_NONE;
> > +	else
> > +		val |= BXT_CDCLK_CD2X_PIPE(pipe);
> >   	I915_WRITE(CDCLK_CTL, val);
> > +	if (pipe != INVALID_PIPE)
> > +		intel_wait_for_vblank(dev_priv, pipe);
> > +
> >   	/* inform PCU of the change */
> >   	mutex_lock(&dev_priv->pcu_lock);
> >   	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> > @@ -1847,10 +1857,12 @@ static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
> >   }
> >   static void icl_set_cdclk(struct drm_i915_private *dev_priv,
> > -			  const struct intel_cdclk_state *cdclk_state)
> > +			  const struct intel_cdclk_state *cdclk_state,
> > +			  enum pipe pipe)
> >   {
> >   	unsigned int cdclk = cdclk_state->cdclk;
> >   	unsigned int vco = cdclk_state->vco;
> > +	u32 val;
> >   	int ret;
> >   	mutex_lock(&dev_priv->pcu_lock);
> > @@ -1872,8 +1884,15 @@ static void icl_set_cdclk(struct drm_i915_private *dev_priv,
> >   	if (dev_priv->cdclk.hw.vco != vco)
> >   		cnl_cdclk_pll_enable(dev_priv, vco);
> > -	I915_WRITE(CDCLK_CTL, ICL_CDCLK_CD2X_PIPE_NONE |
> > -			      skl_cdclk_decimal(cdclk));
> > +	val = skl_cdclk_decimal(cdclk);
> > +	if (pipe == INVALID_PIPE)
> > +		val |= ICL_CDCLK_CD2X_PIPE_NONE;
> > +	else
> > +		val |= ICL_CDCLK_CD2X_PIPE(pipe);
> > +	I915_WRITE(CDCLK_CTL, val);
> > +
> > +	if (pipe != INVALID_PIPE)
> > +		intel_wait_for_vblank(dev_priv, pipe);
> >   	mutex_lock(&dev_priv->pcu_lock);
> >   	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> > @@ -2002,7 +2021,7 @@ void icl_init_cdclk(struct drm_i915_private *dev_priv)
> >   	sanitized_state.voltage_level =
> >   				icl_calc_voltage_level(sanitized_state.cdclk);
> > -	icl_set_cdclk(dev_priv, &sanitized_state);
> > +	icl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE);
> >   }
> >   /**
> > @@ -2020,7 +2039,7 @@ void icl_uninit_cdclk(struct drm_i915_private *dev_priv)
> >   	cdclk_state.vco = 0;
> >   	cdclk_state.voltage_level = icl_calc_voltage_level(cdclk_state.cdclk);
> > -	icl_set_cdclk(dev_priv, &cdclk_state);
> > +	icl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> >   }
> >   /**
> > @@ -2048,7 +2067,7 @@ void cnl_init_cdclk(struct drm_i915_private *dev_priv)
> >   	cdclk_state.vco = cnl_cdclk_pll_vco(dev_priv, cdclk_state.cdclk);
> >   	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
> > -	cnl_set_cdclk(dev_priv, &cdclk_state);
> > +	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> >   }
> >   /**
> > @@ -2066,7 +2085,7 @@ void cnl_uninit_cdclk(struct drm_i915_private *dev_priv)
> >   	cdclk_state.vco = 0;
> >   	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
> > -	cnl_set_cdclk(dev_priv, &cdclk_state);
> > +	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> >   }
> >   /**
> > @@ -2086,6 +2105,27 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
> >   }
> >   /**
> > + * intel_cdclk_needs_cd2x_update - Determine if two CDCLK states require a cd2x divider update
> > + * @a: first CDCLK state
> > + * @b: second CDCLK state
> > + *
> > + * Returns:
> > + * True if the CDCLK states require just a cd2x divider update, false if not.
> > + */
> > +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv,
> > +				   const struct intel_cdclk_state *a,
> > +				   const struct intel_cdclk_state *b)
> > +{
> > +	/* Older hw doesn't have the capability */
> > +	if (INTEL_GEN(dev_priv) < 10 && !IS_GEN9_LP(dev_priv))
> > +		return false;
> > +
> > +	return a->cdclk != b->cdclk &&
> > +		a->vco == b->vco &&
> > +		a->ref == b->ref;
> > +}
> > +
> > +/**
> >    * intel_cdclk_changed - Determine if two CDCLK states are different
> >    * @a: first CDCLK state
> >    * @b: second CDCLK state
> > @@ -2133,12 +2173,14 @@ void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
> >    * intel_set_cdclk - Push the CDCLK state to the hardware
> >    * @dev_priv: i915 device
> >    * @cdclk_state: new CDCLK state
> > + * @pipe: pipe with which to synchronize the update
> >    *
> >    * Program the hardware based on the passed in CDCLK state,
> >    * if necessary.
> >    */
> > -void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > -		     const struct intel_cdclk_state *cdclk_state)
> > +static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > +			    const struct intel_cdclk_state *cdclk_state,
> > +			    enum pipe pipe)
> >   {
> >   	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state))
> >   		return;
> > @@ -2148,7 +2190,7 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
> >   	intel_dump_cdclk_state(cdclk_state, "Changing CDCLK to");
> > -	dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> > +	dev_priv->display.set_cdclk(dev_priv, cdclk_state, pipe);
> >   	if (WARN(intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state),
> >   		 "cdclk state doesn't match!\n")) {
> > @@ -2157,6 +2199,46 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
> >   	}
> >   }
> > +/**
> > + * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> > + * @dev_priv: i915 device
> > + * @old_state: old CDCLK state
> > + * @new_state: new CDCLK state
> > + * @pipe: pipe with which to synchronize the update
> > + *
> > + * Program the hardware before updating the HW plane state based on the passed
> > + * in CDCLK state, if necessary.
> > + */
> > +void
> > +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv,
> > +				 const struct intel_cdclk_state *old_state,
> > +				 const struct intel_cdclk_state *new_state,
> > +				 enum pipe pipe)
> > +{
> > +	if (pipe == INVALID_PIPE || old_state->cdclk <= new_state->cdclk)
> > +		intel_set_cdclk(dev_priv, new_state, pipe);
> > +}
> > +
> > +/**
> > + * intel_set_cdclk_post_plane_update - Push the CDCLK state to the hardware
> > + * @dev_priv: i915 device
> > + * @old_state: old CDCLK state
> > + * @new_state: new CDCLK state
> > + * @pipe: pipe with which to synchronize the update
> > + *
> > + * Program the hardware after updating the HW plane state based on the passed
> > + * in CDCLK state, if necessary.
> > + */
> > +void
> > +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
> > +				  const struct intel_cdclk_state *old_state,
> > +				  const struct intel_cdclk_state *new_state,
> > +				  enum pipe pipe)
> > +{
> > +	if (pipe != INVALID_PIPE && old_state->cdclk > new_state->cdclk)
> > +		intel_set_cdclk(dev_priv, new_state, pipe);
> > +}
> > +
> >   static int intel_pixel_rate_to_cdclk(struct drm_i915_private *dev_priv,
> >   				     int pixel_rate)
> >   {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f7b44773e1e7..bcec03f43d3a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12982,6 +12982,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >   	intel_state->active_crtcs = dev_priv->active_crtcs;
> >   	intel_state->cdclk.logical = dev_priv->cdclk.logical;
> >   	intel_state->cdclk.actual = dev_priv->cdclk.actual;
> > +	intel_state->cdclk.pipe = INVALID_PIPE;
> >   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> >   		if (new_crtc_state->active)
> > @@ -13001,6 +13002,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >   	 * adjusted_mode bits in the crtc directly.
> >   	 */
> >   	if (dev_priv->display.modeset_calc_cdclk) {
> > +		enum pipe pipe;
> > +
> >   		ret = dev_priv->display.modeset_calc_cdclk(state);
> >   		if (ret < 0)
> >   			return ret;
> > @@ -13017,12 +13020,36 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >   				return ret;
> >   		}
> > +		if (is_power_of_2(intel_state->active_crtcs)) {
> > +			struct drm_crtc *crtc;
> > +			struct drm_crtc_state *crtc_state;
> > +
> > +			pipe = ilog2(intel_state->active_crtcs);
> > +			crtc = &intel_get_crtc_for_pipe(dev_priv, pipe)->base;
> > +			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > +			if (crtc_state && needs_modeset(crtc_state))
> > +				pipe = INVALID_PIPE;
> > +		} else {
> > +			pipe = INVALID_PIPE;
> > +		}
> > +
> >   		/* All pipes must be switched off while we change the cdclk. */
> > -		if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> > -					      &intel_state->cdclk.actual)) {
> > +		if (pipe != INVALID_PIPE &&
> > +		    intel_cdclk_needs_cd2x_update(dev_priv,
> > +						  &dev_priv->cdclk.actual,
> > +						  &intel_state->cdclk.actual)) {
> > +			ret = intel_lock_all_pipes(state);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			intel_state->cdclk.pipe = pipe;
> > +		} else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> > +						     &intel_state->cdclk.actual)) {
> >   			ret = intel_modeset_all_pipes(state);
> >   			if (ret < 0)
> >   				return ret;
> > +
> > +			intel_state->cdclk.pipe = INVALID_PIPE;
> >   		}
> >   		DRM_DEBUG_KMS("New cdclk calculated to be logical %u kHz, actual %u kHz\n",
> > @@ -13431,7 +13458,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >   	if (intel_state->modeset) {
> >   		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
> > -		intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> > +		intel_set_cdclk_pre_plane_update(dev_priv,
> > +						 &intel_state->cdclk.actual,
> > +						 &dev_priv->cdclk.actual,
> > +						 intel_state->cdclk.pipe);
> >   		/*
> >   		 * SKL workaround: bspec recommends we disable the SAGV when we
> > @@ -13460,6 +13490,12 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >   	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> >   	dev_priv->display.update_crtcs(state);
> > +	if (intel_state->modeset)
> > +		intel_set_cdclk_post_plane_update(dev_priv,
> > +						  &intel_state->cdclk.actual,
> > +						  &dev_priv->cdclk.actual,
> > +						  intel_state->cdclk.pipe);
> > +
> >   	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
> >   	 * already, but still need the state for the delayed optimization. To
> >   	 * fix this:
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 85dd6a9d1e42..d22a0e92e0d4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -559,6 +559,8 @@ struct intel_atomic_state {
> >   		int force_min_cdclk;
> >   		bool force_min_cdclk_changed;
> > +		/* pipe to which cd2x update is synchronized */
> > +		enum pipe pipe;
> >   	} cdclk;
> >   	bool dpll_set, modeset;
> > @@ -1694,13 +1696,24 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> >   void intel_update_max_cdclk(struct drm_i915_private *dev_priv);
> >   void intel_update_cdclk(struct drm_i915_private *dev_priv);
> >   void intel_update_rawclk(struct drm_i915_private *dev_priv);
> > +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv,
> > +				   const struct intel_cdclk_state *a,
> > +				   const struct intel_cdclk_state *b);
> >   bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
> >   			       const struct intel_cdclk_state *b);
> >   bool intel_cdclk_changed(const struct intel_cdclk_state *a,
> >   			 const struct intel_cdclk_state *b);
> >   void intel_cdclk_swap_state(struct intel_atomic_state *state);
> > -void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > -		     const struct intel_cdclk_state *cdclk_state);
> > +void
> > +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv,
> > +				 const struct intel_cdclk_state *old_state,
> > +				 const struct intel_cdclk_state *new_state,
> > +				 enum pipe pipe);
> > +void
> > +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
> > +				  const struct intel_cdclk_state *old_state,
> > +				  const struct intel_cdclk_state *new_state,
> > +				  enum pipe pipe);
> >   void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
> >   			    const char *context);
On Fri, Mar 22, 2019 at 10:23:07PM +0200, Imre Deak wrote:
> On Thu, Mar 21, 2019 at 02:53:00PM -0700, Clinton Taylor wrote:
> > 
> > On 3/20/19 6:54 AM, Imre Deak wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > If we have only a single active pipe and the cdclk change only requires
> > > the cd2x divider to be updated bxt+ can do the update with forcing a full
> > > modeset on the pipe. Try to hook that up.
> > > 
> > > v2:
> > > - Wait for vblank after an optimized CDCLK change.
> > > - Avoid optimization if the pipe needs a modeset (or was disabled).
> > > - Split CDCLK change to a pre/post plane update step.
> > > v3:
> > > - Use correct version of CDCLK state as old state. (Ville)
> > > - Remove unused intel_cdclk_can_skip_modeset()
> > > v4:
> > > - For consistency call intel_set_cdclk_post_plane_update() only during
> > >    modesets (and not fastsets).
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> > > Tested-by: Abhay Kumar <abhay.kumar@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_drv.h      |   3 +-
> > >   drivers/gpu/drm/i915/i915_reg.h      |   3 +-
> > >   drivers/gpu/drm/i915/intel_cdclk.c   | 142 +++++++++++++++++++++++++++--------
> > >   drivers/gpu/drm/i915/intel_display.c |  42 ++++++++++-
> > >   drivers/gpu/drm/i915/intel_drv.h     |  17 ++++-
> > >   5 files changed, 170 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 6b10cee4e77f..d8f91525c94c 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -277,7 +277,8 @@ struct drm_i915_display_funcs {
> > >   	void (*get_cdclk)(struct drm_i915_private *dev_priv,
> > >   			  struct intel_cdclk_state *cdclk_state);
> > >   	void (*set_cdclk)(struct drm_i915_private *dev_priv,
> > > -			  const struct intel_cdclk_state *cdclk_state);
> > > +			  const struct intel_cdclk_state *cdclk_state,
> > > +			  enum pipe pipe);
> > >   	int (*get_fifo_size)(struct drm_i915_private *dev_priv,
> > >   			     enum i9xx_plane_id i9xx_plane);
> > >   	int (*compute_pipe_wm)(struct intel_crtc_state *cstate);
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 9b69cec21f7b..12b8170ced96 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -9521,7 +9521,8 @@ enum skl_power_gate {
> > >   #define  BXT_CDCLK_CD2X_PIPE(pipe)	((pipe) << 20)
> > >   #define  CDCLK_DIVMUX_CD_OVERRIDE	(1 << 19)
> > >   #define  BXT_CDCLK_CD2X_PIPE_NONE	BXT_CDCLK_CD2X_PIPE(3)
> > > -#define  ICL_CDCLK_CD2X_PIPE_NONE	(7 << 19)
> > > +#define  ICL_CDCLK_CD2X_PIPE(pipe)	((pipe) << 19)
> > 
> > Unfortunately bits 21:19 of CDCLK_CTL don't match the pipe enum. This MACRO
> > will not work except for PIPE_A.
> > 
> > Pipe_A is 0x00, PIPE_B is 0x02, PIPE_C is 0x06.
> 
> Good catch.
> 
> The pipe B and C encodings could be typoed though and I couldn't trigger
> any problems with either encodings (i.e. using either 0x1 or 0x2 for
> pipe B updates while pipe B was the only pipe enabled). So I opened a
> ticket for this in BSpec, let's wait for a clarification on this.

There is no conclusion on this in BSpec yet. However imo, we don't need
to wait for that since on ICL there is actually no support for a CD clock
divider other than 1, see the description for CDCLK_CTL in the spec.

So on ICL we'd never change the CDCLK rate while it's active and so we
don't actually need to ever set the pipe select bits to other than the
current no-pipe setting.

Ville pointed out that in the future we'll still call icl_set_cdclk()
while the CD clock is active, but only to change the voltage level
(as required by port clock rates), keeping the CD clock rate the same.
But that doesn't need the pipe select bits either nor the vblank wait.

Based on the above I'll remove the ICL specifc parts from this patch to
set the pipe select bits and wait for vblank in icl_set_cdclk().

> 
> > In BXT only bits 21:20 were used for CD2X pipe select and the pipe enum does
> > work.
> > 
> > 
> > -Clint
> > 
> > 
> > 
> > > +#define  ICL_CDCLK_CD2X_PIPE_NONE	ICL_CDCLK_CD2X_PIPE(7)
> > >   #define  BXT_CDCLK_SSA_PRECHARGE_ENABLE	(1 << 16)
> > >   #define  CDCLK_FREQ_DECIMAL_MASK	(0x7ff)
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index 5c25626f8cf0..48cc42a7ef4f 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -516,7 +516,8 @@ static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> > >   }
> > >   static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
> > > -			  const struct intel_cdclk_state *cdclk_state)
> > > +			  const struct intel_cdclk_state *cdclk_state,
> > > +			  enum pipe pipe)
> > >   {
> > >   	int cdclk = cdclk_state->cdclk;
> > >   	u32 val, cmd = cdclk_state->voltage_level;
> > > @@ -598,7 +599,8 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
> > >   }
> > >   static void chv_set_cdclk(struct drm_i915_private *dev_priv,
> > > -			  const struct intel_cdclk_state *cdclk_state)
> > > +			  const struct intel_cdclk_state *cdclk_state,
> > > +			  enum pipe pipe)
> > >   {
> > >   	int cdclk = cdclk_state->cdclk;
> > >   	u32 val, cmd = cdclk_state->voltage_level;
> > > @@ -697,7 +699,8 @@ static void bdw_get_cdclk(struct drm_i915_private *dev_priv,
> > >   }
> > >   static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
> > > -			  const struct intel_cdclk_state *cdclk_state)
> > > +			  const struct intel_cdclk_state *cdclk_state,
> > > +			  enum pipe pipe)
> > >   {
> > >   	int cdclk = cdclk_state->cdclk;
> > >   	u32 val;
> > > @@ -987,7 +990,8 @@ static void skl_dpll0_disable(struct drm_i915_private *dev_priv)
> > >   }
> > >   static void skl_set_cdclk(struct drm_i915_private *dev_priv,
> > > -			  const struct intel_cdclk_state *cdclk_state)
> > > +			  const struct intel_cdclk_state *cdclk_state,
> > > +			  enum pipe pipe)
> > >   {
> > >   	int cdclk = cdclk_state->cdclk;
> > >   	int vco = cdclk_state->vco;
> > > @@ -1158,7 +1162,7 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
> > >   	cdclk_state.cdclk = skl_calc_cdclk(0, cdclk_state.vco);
> > >   	cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
> > > -	skl_set_cdclk(dev_priv, &cdclk_state);
> > > +	skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> > >   }
> > >   /**
> > > @@ -1176,7 +1180,7 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
> > >   	cdclk_state.vco = 0;
> > >   	cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
> > > -	skl_set_cdclk(dev_priv, &cdclk_state);
> > > +	skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> > >   }
> > >   static int bxt_calc_cdclk(int min_cdclk)
> > > @@ -1355,7 +1359,8 @@ static void bxt_de_pll_enable(struct drm_i915_private *dev_priv, int vco)
> > >   }
> > >   static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > -			  const struct intel_cdclk_state *cdclk_state)
> > > +			  const struct intel_cdclk_state *cdclk_state,
> > > +			  enum pipe pipe)
> > >   {
> > >   	int cdclk = cdclk_state->cdclk;
> > >   	int vco = cdclk_state->vco;
> > > @@ -1408,11 +1413,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > >   		bxt_de_pll_enable(dev_priv, vco);
> > >   	val = divider | skl_cdclk_decimal(cdclk);
> > > -	/*
> > > -	 * FIXME if only the cd2x divider needs changing, it could be done
> > > -	 * without shutting off the pipe (if only one pipe is active).
> > > -	 */
> > > -	val |= BXT_CDCLK_CD2X_PIPE_NONE;
> > > +	if (pipe == INVALID_PIPE)
> > > +		val |= BXT_CDCLK_CD2X_PIPE_NONE;
> > > +	else
> > > +		val |= BXT_CDCLK_CD2X_PIPE(pipe);
> > >   	/*
> > >   	 * Disable SSA Precharge when CD clock frequency < 500 MHz,
> > >   	 * enable otherwise.
> > > @@ -1421,6 +1425,9 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > >   		val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> > >   	I915_WRITE(CDCLK_CTL, val);
> > > +	if (pipe != INVALID_PIPE)
> > > +		intel_wait_for_vblank(dev_priv, pipe);
> > > +
> > >   	mutex_lock(&dev_priv->pcu_lock);
> > >   	/*
> > >   	 * The timeout isn't specified, the 2ms used here is based on
> > > @@ -1525,7 +1532,7 @@ void bxt_init_cdclk(struct drm_i915_private *dev_priv)
> > >   	}
> > >   	cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
> > > -	bxt_set_cdclk(dev_priv, &cdclk_state);
> > > +	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> > >   }
> > >   /**
> > > @@ -1543,7 +1550,7 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
> > >   	cdclk_state.vco = 0;
> > >   	cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
> > > -	bxt_set_cdclk(dev_priv, &cdclk_state);
> > > +	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> > >   }
> > >   static int cnl_calc_cdclk(int min_cdclk)
> > > @@ -1663,7 +1670,8 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
> > >   }
> > >   static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> > > -			  const struct intel_cdclk_state *cdclk_state)
> > > +			  const struct intel_cdclk_state *cdclk_state,
> > > +			  enum pipe pipe)
> > >   {
> > >   	int cdclk = cdclk_state->cdclk;
> > >   	int vco = cdclk_state->vco;
> > > @@ -1704,13 +1712,15 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> > >   		cnl_cdclk_pll_enable(dev_priv, vco);
> > >   	val = divider | skl_cdclk_decimal(cdclk);
> > > -	/*
> > > -	 * FIXME if only the cd2x divider needs changing, it could be done
> > > -	 * without shutting off the pipe (if only one pipe is active).
> > > -	 */
> > > -	val |= BXT_CDCLK_CD2X_PIPE_NONE;
> > > +	if (pipe == INVALID_PIPE)
> > > +		val |= BXT_CDCLK_CD2X_PIPE_NONE;
> > > +	else
> > > +		val |= BXT_CDCLK_CD2X_PIPE(pipe);
> > >   	I915_WRITE(CDCLK_CTL, val);
> > > +	if (pipe != INVALID_PIPE)
> > > +		intel_wait_for_vblank(dev_priv, pipe);
> > > +
> > >   	/* inform PCU of the change */
> > >   	mutex_lock(&dev_priv->pcu_lock);
> > >   	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> > > @@ -1847,10 +1857,12 @@ static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
> > >   }
> > >   static void icl_set_cdclk(struct drm_i915_private *dev_priv,
> > > -			  const struct intel_cdclk_state *cdclk_state)
> > > +			  const struct intel_cdclk_state *cdclk_state,
> > > +			  enum pipe pipe)
> > >   {
> > >   	unsigned int cdclk = cdclk_state->cdclk;
> > >   	unsigned int vco = cdclk_state->vco;
> > > +	u32 val;
> > >   	int ret;
> > >   	mutex_lock(&dev_priv->pcu_lock);
> > > @@ -1872,8 +1884,15 @@ static void icl_set_cdclk(struct drm_i915_private *dev_priv,
> > >   	if (dev_priv->cdclk.hw.vco != vco)
> > >   		cnl_cdclk_pll_enable(dev_priv, vco);
> > > -	I915_WRITE(CDCLK_CTL, ICL_CDCLK_CD2X_PIPE_NONE |
> > > -			      skl_cdclk_decimal(cdclk));
> > > +	val = skl_cdclk_decimal(cdclk);
> > > +	if (pipe == INVALID_PIPE)
> > > +		val |= ICL_CDCLK_CD2X_PIPE_NONE;
> > > +	else
> > > +		val |= ICL_CDCLK_CD2X_PIPE(pipe);
> > > +	I915_WRITE(CDCLK_CTL, val);
> > > +
> > > +	if (pipe != INVALID_PIPE)
> > > +		intel_wait_for_vblank(dev_priv, pipe);
> > >   	mutex_lock(&dev_priv->pcu_lock);
> > >   	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> > > @@ -2002,7 +2021,7 @@ void icl_init_cdclk(struct drm_i915_private *dev_priv)
> > >   	sanitized_state.voltage_level =
> > >   				icl_calc_voltage_level(sanitized_state.cdclk);
> > > -	icl_set_cdclk(dev_priv, &sanitized_state);
> > > +	icl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE);
> > >   }
> > >   /**
> > > @@ -2020,7 +2039,7 @@ void icl_uninit_cdclk(struct drm_i915_private *dev_priv)
> > >   	cdclk_state.vco = 0;
> > >   	cdclk_state.voltage_level = icl_calc_voltage_level(cdclk_state.cdclk);
> > > -	icl_set_cdclk(dev_priv, &cdclk_state);
> > > +	icl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> > >   }
> > >   /**
> > > @@ -2048,7 +2067,7 @@ void cnl_init_cdclk(struct drm_i915_private *dev_priv)
> > >   	cdclk_state.vco = cnl_cdclk_pll_vco(dev_priv, cdclk_state.cdclk);
> > >   	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
> > > -	cnl_set_cdclk(dev_priv, &cdclk_state);
> > > +	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> > >   }
> > >   /**
> > > @@ -2066,7 +2085,7 @@ void cnl_uninit_cdclk(struct drm_i915_private *dev_priv)
> > >   	cdclk_state.vco = 0;
> > >   	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
> > > -	cnl_set_cdclk(dev_priv, &cdclk_state);
> > > +	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> > >   }
> > >   /**
> > > @@ -2086,6 +2105,27 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
> > >   }
> > >   /**
> > > + * intel_cdclk_needs_cd2x_update - Determine if two CDCLK states require a cd2x divider update
> > > + * @a: first CDCLK state
> > > + * @b: second CDCLK state
> > > + *
> > > + * Returns:
> > > + * True if the CDCLK states require just a cd2x divider update, false if not.
> > > + */
> > > +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv,
> > > +				   const struct intel_cdclk_state *a,
> > > +				   const struct intel_cdclk_state *b)
> > > +{
> > > +	/* Older hw doesn't have the capability */
> > > +	if (INTEL_GEN(dev_priv) < 10 && !IS_GEN9_LP(dev_priv))
> > > +		return false;
> > > +
> > > +	return a->cdclk != b->cdclk &&
> > > +		a->vco == b->vco &&
> > > +		a->ref == b->ref;
> > > +}
> > > +
> > > +/**
> > >    * intel_cdclk_changed - Determine if two CDCLK states are different
> > >    * @a: first CDCLK state
> > >    * @b: second CDCLK state
> > > @@ -2133,12 +2173,14 @@ void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
> > >    * intel_set_cdclk - Push the CDCLK state to the hardware
> > >    * @dev_priv: i915 device
> > >    * @cdclk_state: new CDCLK state
> > > + * @pipe: pipe with which to synchronize the update
> > >    *
> > >    * Program the hardware based on the passed in CDCLK state,
> > >    * if necessary.
> > >    */
> > > -void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > > -		     const struct intel_cdclk_state *cdclk_state)
> > > +static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > > +			    const struct intel_cdclk_state *cdclk_state,
> > > +			    enum pipe pipe)
> > >   {
> > >   	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state))
> > >   		return;
> > > @@ -2148,7 +2190,7 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > >   	intel_dump_cdclk_state(cdclk_state, "Changing CDCLK to");
> > > -	dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> > > +	dev_priv->display.set_cdclk(dev_priv, cdclk_state, pipe);
> > >   	if (WARN(intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state),
> > >   		 "cdclk state doesn't match!\n")) {
> > > @@ -2157,6 +2199,46 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > >   	}
> > >   }
> > > +/**
> > > + * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> > > + * @dev_priv: i915 device
> > > + * @old_state: old CDCLK state
> > > + * @new_state: new CDCLK state
> > > + * @pipe: pipe with which to synchronize the update
> > > + *
> > > + * Program the hardware before updating the HW plane state based on the passed
> > > + * in CDCLK state, if necessary.
> > > + */
> > > +void
> > > +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv,
> > > +				 const struct intel_cdclk_state *old_state,
> > > +				 const struct intel_cdclk_state *new_state,
> > > +				 enum pipe pipe)
> > > +{
> > > +	if (pipe == INVALID_PIPE || old_state->cdclk <= new_state->cdclk)
> > > +		intel_set_cdclk(dev_priv, new_state, pipe);
> > > +}
> > > +
> > > +/**
> > > + * intel_set_cdclk_post_plane_update - Push the CDCLK state to the hardware
> > > + * @dev_priv: i915 device
> > > + * @old_state: old CDCLK state
> > > + * @new_state: new CDCLK state
> > > + * @pipe: pipe with which to synchronize the update
> > > + *
> > > + * Program the hardware after updating the HW plane state based on the passed
> > > + * in CDCLK state, if necessary.
> > > + */
> > > +void
> > > +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
> > > +				  const struct intel_cdclk_state *old_state,
> > > +				  const struct intel_cdclk_state *new_state,
> > > +				  enum pipe pipe)
> > > +{
> > > +	if (pipe != INVALID_PIPE && old_state->cdclk > new_state->cdclk)
> > > +		intel_set_cdclk(dev_priv, new_state, pipe);
> > > +}
> > > +
> > >   static int intel_pixel_rate_to_cdclk(struct drm_i915_private *dev_priv,
> > >   				     int pixel_rate)
> > >   {
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index f7b44773e1e7..bcec03f43d3a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12982,6 +12982,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> > >   	intel_state->active_crtcs = dev_priv->active_crtcs;
> > >   	intel_state->cdclk.logical = dev_priv->cdclk.logical;
> > >   	intel_state->cdclk.actual = dev_priv->cdclk.actual;
> > > +	intel_state->cdclk.pipe = INVALID_PIPE;
> > >   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > >   		if (new_crtc_state->active)
> > > @@ -13001,6 +13002,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> > >   	 * adjusted_mode bits in the crtc directly.
> > >   	 */
> > >   	if (dev_priv->display.modeset_calc_cdclk) {
> > > +		enum pipe pipe;
> > > +
> > >   		ret = dev_priv->display.modeset_calc_cdclk(state);
> > >   		if (ret < 0)
> > >   			return ret;
> > > @@ -13017,12 +13020,36 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> > >   				return ret;
> > >   		}
> > > +		if (is_power_of_2(intel_state->active_crtcs)) {
> > > +			struct drm_crtc *crtc;
> > > +			struct drm_crtc_state *crtc_state;
> > > +
> > > +			pipe = ilog2(intel_state->active_crtcs);
> > > +			crtc = &intel_get_crtc_for_pipe(dev_priv, pipe)->base;
> > > +			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > +			if (crtc_state && needs_modeset(crtc_state))
> > > +				pipe = INVALID_PIPE;
> > > +		} else {
> > > +			pipe = INVALID_PIPE;
> > > +		}
> > > +
> > >   		/* All pipes must be switched off while we change the cdclk. */
> > > -		if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> > > -					      &intel_state->cdclk.actual)) {
> > > +		if (pipe != INVALID_PIPE &&
> > > +		    intel_cdclk_needs_cd2x_update(dev_priv,
> > > +						  &dev_priv->cdclk.actual,
> > > +						  &intel_state->cdclk.actual)) {
> > > +			ret = intel_lock_all_pipes(state);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +
> > > +			intel_state->cdclk.pipe = pipe;
> > > +		} else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> > > +						     &intel_state->cdclk.actual)) {
> > >   			ret = intel_modeset_all_pipes(state);
> > >   			if (ret < 0)
> > >   				return ret;
> > > +
> > > +			intel_state->cdclk.pipe = INVALID_PIPE;
> > >   		}
> > >   		DRM_DEBUG_KMS("New cdclk calculated to be logical %u kHz, actual %u kHz\n",
> > > @@ -13431,7 +13458,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> > >   	if (intel_state->modeset) {
> > >   		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
> > > -		intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> > > +		intel_set_cdclk_pre_plane_update(dev_priv,
> > > +						 &intel_state->cdclk.actual,
> > > +						 &dev_priv->cdclk.actual,
> > > +						 intel_state->cdclk.pipe);
> > >   		/*
> > >   		 * SKL workaround: bspec recommends we disable the SAGV when we
> > > @@ -13460,6 +13490,12 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> > >   	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> > >   	dev_priv->display.update_crtcs(state);
> > > +	if (intel_state->modeset)
> > > +		intel_set_cdclk_post_plane_update(dev_priv,
> > > +						  &intel_state->cdclk.actual,
> > > +						  &dev_priv->cdclk.actual,
> > > +						  intel_state->cdclk.pipe);
> > > +
> > >   	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
> > >   	 * already, but still need the state for the delayed optimization. To
> > >   	 * fix this:
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 85dd6a9d1e42..d22a0e92e0d4 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -559,6 +559,8 @@ struct intel_atomic_state {
> > >   		int force_min_cdclk;
> > >   		bool force_min_cdclk_changed;
> > > +		/* pipe to which cd2x update is synchronized */
> > > +		enum pipe pipe;
> > >   	} cdclk;
> > >   	bool dpll_set, modeset;
> > > @@ -1694,13 +1696,24 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> > >   void intel_update_max_cdclk(struct drm_i915_private *dev_priv);
> > >   void intel_update_cdclk(struct drm_i915_private *dev_priv);
> > >   void intel_update_rawclk(struct drm_i915_private *dev_priv);
> > > +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv,
> > > +				   const struct intel_cdclk_state *a,
> > > +				   const struct intel_cdclk_state *b);
> > >   bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
> > >   			       const struct intel_cdclk_state *b);
> > >   bool intel_cdclk_changed(const struct intel_cdclk_state *a,
> > >   			 const struct intel_cdclk_state *b);
> > >   void intel_cdclk_swap_state(struct intel_atomic_state *state);
> > > -void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > > -		     const struct intel_cdclk_state *cdclk_state);
> > > +void
> > > +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv,
> > > +				 const struct intel_cdclk_state *old_state,
> > > +				 const struct intel_cdclk_state *new_state,
> > > +				 enum pipe pipe);
> > > +void
> > > +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
> > > +				  const struct intel_cdclk_state *old_state,
> > > +				  const struct intel_cdclk_state *new_state,
> > > +				  enum pipe pipe);
> > >   void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
> > >   			    const char *context);
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx