[v6,2/2] drm/i915: Skip modeset for cdclk changes if possible

Submitted by Imre Deak on March 18, 2019, 2:34 p.m.

Details

Message ID 20190318143435.1686-3-imre.deak@intel.com
State New
Headers show
Series "drm/i915: Ensure minimum CDCLK requirement for audio" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak March 18, 2019, 2:34 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.

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   | 155 ++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_display.c |  39 ++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  15 +++-
 5 files changed, 178 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 7dcca84f31d1..d074517288ea 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,42 @@  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_can_skip_modeset - Determine if two CDCLK states can a modeset on all pipes
+ * @a: first CDCLK state
+ * @b: second CDCLK state
+ *
+ * Returns:
+ * True if the CDCLK states require pipes to be off during reprogramming,
+ * false if not.
+ */
+bool intel_cdclk_can_skip_modeset(const struct intel_cdclk_state *a,
+				  const struct intel_cdclk_state *b)
+{
+	return 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
@@ -2113,12 +2168,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;
@@ -2128,7 +2185,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")) {
@@ -2137,6 +2194,44 @@  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
+ * @cdclk_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 *cdclk_state,
+				 enum pipe pipe)
+{
+	if (pipe == INVALID_PIPE ||
+	    dev_priv->cdclk.actual.cdclk >= cdclk_state->cdclk)
+		intel_set_cdclk(dev_priv, cdclk_state, pipe);
+}
+
+/**
+ * intel_set_cdclk_post_plane_update - 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 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 *cdclk_state,
+				  enum pipe pipe)
+{
+	if (pipe != INVALID_PIPE &&
+	    dev_priv->cdclk.actual.cdclk < cdclk_state->cdclk)
+		intel_set_cdclk(dev_priv, cdclk_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 b4199cd53349..0f9884ec7e8e 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",
@@ -13433,7 +13460,9 @@  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,
+						 &dev_priv->cdclk.actual,
+						 intel_state->cdclk.pipe);
 
 		/*
 		 * SKL workaround: bspec recommends we disable the SAGV when we
@@ -13462,6 +13491,10 @@  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);
 
+	intel_set_cdclk_post_plane_update(dev_priv,
+					  &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 0b84e557c267..e5a1e245ee65 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,12 +1696,21 @@  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_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 *cdclk_state,
+				 enum pipe pipe);
+void
+intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
+				  const struct intel_cdclk_state *cdclk_state,
+				  enum pipe pipe);
 void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
 			    const char *context);
 

Comments

On Mon, Mar 18, 2019 at 04:34:35PM +0200, Imre Deak wrote:
> @@ -2137,6 +2194,44 @@ 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
> + * @cdclk_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 *cdclk_state,
> +				 enum pipe pipe)
> +{
> +	if (pipe == INVALID_PIPE ||
> +	    dev_priv->cdclk.actual.cdclk >= cdclk_state->cdclk)

cdclk_state == &dev_priv->cdclk.actual
so these look a bit buggered.

Would be somewhat nice to use the private obj stuff for this but
the locking now enforced by that make it difficult. Perhaps a
swap(dev_priv->cdlk, state->cdclk) would be possible? A quick scan
through the code didn't reveal any uses of state->cdclk outside the
compute code paths, so looks safeish. Another option is just
comparing against dev_priv->cdclk.hw since we still have that.

> +		intel_set_cdclk(dev_priv, cdclk_state, pipe);
> +}
> +
> +/**
> + * intel_set_cdclk_post_plane_update - 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 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 *cdclk_state,
> +				  enum pipe pipe)
> +{
> +	if (pipe != INVALID_PIPE &&
> +	    dev_priv->cdclk.actual.cdclk < cdclk_state->cdclk)
> +		intel_set_cdclk(dev_priv, cdclk_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 b4199cd53349..0f9884ec7e8e 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",
> @@ -13433,7 +13460,9 @@ 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,
> +						 &dev_priv->cdclk.actual,
> +						 intel_state->cdclk.pipe);
>  
>  		/*
>  		 * SKL workaround: bspec recommends we disable the SAGV when we
> @@ -13462,6 +13491,10 @@ 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);
>  
> +	intel_set_cdclk_post_plane_update(dev_priv,
> +					  &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 0b84e557c267..e5a1e245ee65 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,12 +1696,21 @@ 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_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 *cdclk_state,
> +				 enum pipe pipe);
> +void
> +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
> +				  const struct intel_cdclk_state *cdclk_state,
> +				  enum pipe pipe);
>  void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
>  			    const char *context);
>  
> -- 
> 2.13.2
On Mon, Mar 18, 2019 at 08:37:33PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 18, 2019 at 04:34:35PM +0200, Imre Deak wrote:
> > @@ -2137,6 +2194,44 @@ 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
> > + * @cdclk_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 *cdclk_state,
> > +				 enum pipe pipe)
> > +{
> > +	if (pipe == INVALID_PIPE ||
> > +	    dev_priv->cdclk.actual.cdclk >= cdclk_state->cdclk)
> 
> cdclk_state == &dev_priv->cdclk.actual
> so these look a bit buggered.

Yep, screwed this up..

> Would be somewhat nice to use the private obj stuff for this but
> the locking now enforced by that make it difficult. Perhaps a
> swap(dev_priv->cdlk, state->cdclk) would be possible?

I suppose you mean in intel_atomic_commit() already, where the swap is
done already halfway, so in the end state->cdclk would become
old_cdclk_state. That sounds fine to me.

> A quick scan through the code didn't reveal any uses of state->cdclk
> outside the compute code paths, so looks safeish. Another option is
> just comparing against dev_priv->cdclk.hw since we still have that.

> 
> > +		intel_set_cdclk(dev_priv, cdclk_state, pipe);
> > +}
> > +
> > +/**
> > + * intel_set_cdclk_post_plane_update - 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 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 *cdclk_state,
> > +				  enum pipe pipe)
> > +{
> > +	if (pipe != INVALID_PIPE &&
> > +	    dev_priv->cdclk.actual.cdclk < cdclk_state->cdclk)
> > +		intel_set_cdclk(dev_priv, cdclk_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 b4199cd53349..0f9884ec7e8e 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",
> > @@ -13433,7 +13460,9 @@ 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,
> > +						 &dev_priv->cdclk.actual,
> > +						 intel_state->cdclk.pipe);
> >  
> >  		/*
> >  		 * SKL workaround: bspec recommends we disable the SAGV when we
> > @@ -13462,6 +13491,10 @@ 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);
> >  
> > +	intel_set_cdclk_post_plane_update(dev_priv,
> > +					  &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 0b84e557c267..e5a1e245ee65 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,12 +1696,21 @@ 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_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 *cdclk_state,
> > +				 enum pipe pipe);
> > +void
> > +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
> > +				  const struct intel_cdclk_state *cdclk_state,
> > +				  enum pipe pipe);
> >  void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
> >  			    const char *context);
> >  
> > -- 
> > 2.13.2
> 
> -- 
> Ville Syrjälä
> Intel