[v2,19/23] drm/i915/icl: Reserve all required PLLs for TypeC ports

Submitted by Imre Deak on June 7, 2019, 5:41 p.m.

Details

Message ID 20190607174129.18725-1-imre.deak@intel.com
State New
Headers show
Series "drm/i915: Fix TypeC port mode switching" ( rev: 2 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak June 7, 2019, 5:41 p.m.
When enabling a TypeC port we need to reserve all the required PLLs for
it, the TBT PLL for TBT-alt and the MG PHY PLL for DP-alt/legacy sinks.
We can select the proper PLL for the current port mode from the reserved
PLLs only once we selected and locked down the port mode for the whole
duration of the port's active state. Resetting and locking down the port
mode can in turn happen only during the modeset commit phase once we
disabled the given port and the PLL it used.

To support the above reserve-and-select PLL semantic we store the
reserved PLLs along with their HW state in the CRTC state and provide a
way to select the active PLL from these. The selected PLL along with its
HW state will be pointed at by crtc_state->shared_dpll/dpll_hw_state as
in the case of other port types.

Besides reserving all required PLLs no functional changes.

v2:
- Fix releasing the ICL PLLs, not clearing the PLLs from the old
  crtc_state.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |  11 +-
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 151 +++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |   9 ++
 drivers/gpu/drm/i915/intel_drv.h      |   9 ++
 4 files changed, 138 insertions(+), 42 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7381fb2e1240..006be3c3f1bd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9880,6 +9880,7 @@  static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
 				enum port port,
 				struct intel_crtc_state *pipe_config)
 {
+	enum icl_port_dpll_id port_dpll_id;
 	enum intel_dpll_id id;
 	u32 temp;
 
@@ -9887,22 +9888,28 @@  static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
 		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
 		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
 		id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
+		port_dpll_id = ICL_PORT_DPLL_DEFAULT;
 	} else if (intel_port_is_tc(dev_priv, port)) {
 		u32 clk_sel = I915_READ(DDI_CLK_SEL(port)) & DDI_CLK_SEL_MASK;
 
 		if (clk_sel == DDI_CLK_SEL_MG) {
 			id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv,
 								    port));
+			port_dpll_id = ICL_PORT_DPLL_MG_PHY;
 		} else {
 			WARN_ON(clk_sel < DDI_CLK_SEL_TBT_162);
 			id = DPLL_ID_ICL_TBTPLL;
+			port_dpll_id = ICL_PORT_DPLL_DEFAULT;
 		}
 	} else {
 		WARN(1, "Invalid port %x\n", port);
 		return;
 	}
 
-	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
+	pipe_config->icl_port_dplls[port_dpll_id].pll =
+		intel_get_shared_dpll_by_id(dev_priv, id);
+
+	icl_set_active_port_dpll(pipe_config, port_dpll_id);
 }
 
 static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
@@ -12041,6 +12048,8 @@  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	saved_state->scaler_state = crtc_state->scaler_state;
 	saved_state->shared_dpll = crtc_state->shared_dpll;
 	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
+	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
+	       sizeof(saved_state->icl_port_dplls));
 	saved_state->crc_enabled = crtc_state->crc_enabled;
 	if (IS_G4X(dev_priv) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 8ac293db43a5..17441d5f990e 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -2856,34 +2856,79 @@  static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state,
 	return true;
 }
 
+/**
+ * icl_set_active_port_dpll - select the active port DPLL for a given CRTC
+ * @crtc_state: state for the CRTC to select the DPLL for
+ * @port_dpll_id: the active @port_dpll_id to select
+ *
+ * Select the given @port_dpll_id instance from the DPLLs reserved for the
+ * CRTC.
+ */
+void icl_set_active_port_dpll(struct intel_crtc_state *crtc_state,
+			      enum icl_port_dpll_id port_dpll_id)
+{
+	struct icl_port_dpll *port_dpll =
+		&crtc_state->icl_port_dplls[port_dpll_id];
+
+	crtc_state->shared_dpll = port_dpll->pll;
+	crtc_state->dpll_hw_state = port_dpll->hw_state;
+}
+
+static void icl_update_active_dpll(struct intel_atomic_state *state,
+				   struct intel_crtc *crtc,
+				   struct intel_encoder *encoder)
+{
+	struct intel_crtc_state *crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+	struct intel_digital_port *primary_port;
+	enum icl_port_dpll_id port_dpll_id;
+
+	primary_port = encoder->type == INTEL_OUTPUT_DP_MST ?
+		enc_to_mst(&encoder->base)->primary :
+		enc_to_dig_port(&encoder->base);
+
+	switch (primary_port->tc_mode) {
+	case TC_PORT_TBT_ALT:
+		port_dpll_id = ICL_PORT_DPLL_DEFAULT;
+		break;
+	case TC_PORT_DP_ALT:
+	case TC_PORT_LEGACY:
+		port_dpll_id = ICL_PORT_DPLL_MG_PHY;
+		break;
+	}
+
+	icl_set_active_port_dpll(crtc_state, port_dpll_id);
+}
+
 static bool icl_get_combo_phy_dpll(struct intel_atomic_state *state,
 				   struct intel_crtc *crtc,
 				   struct intel_encoder *encoder)
 {
 	struct intel_crtc_state *crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
-	struct intel_shared_dpll *pll;
+	struct icl_port_dpll *port_dpll =
+		&crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
 
-	if (!icl_calc_dpll_state(crtc_state, encoder,
-				 &crtc_state->dpll_hw_state)) {
+	if (!icl_calc_dpll_state(crtc_state, encoder, &port_dpll->hw_state)) {
 		DRM_DEBUG_KMS("Could not calculate combo PHY PLL state.\n");
 
 		return false;
 	}
 
-	pll = intel_find_shared_dpll(state, crtc, &crtc_state->dpll_hw_state,
-				     DPLL_ID_ICL_DPLL0,
-				     DPLL_ID_ICL_DPLL1);
-	if (!pll) {
+	port_dpll->pll = intel_find_shared_dpll(state, crtc,
+						&port_dpll->hw_state,
+						DPLL_ID_ICL_DPLL0,
+						DPLL_ID_ICL_DPLL1);
+	if (!port_dpll->pll) {
 		DRM_DEBUG_KMS("No combo PHY PLL found for port %c\n",
 			      port_name(encoder->port));
 		return false;
 	}
 
 	intel_reference_shared_dpll(state, crtc,
-				    pll, &crtc_state->dpll_hw_state);
+				    port_dpll->pll, &port_dpll->hw_state);
 
-	crtc_state->shared_dpll = pll;
+	icl_update_active_dpll(state, crtc, encoder);
 
 	return true;
 }
@@ -2895,49 +2940,55 @@  static bool icl_get_tc_phy_dplls(struct intel_atomic_state *state,
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	struct intel_crtc_state *crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
-	enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder->port);
-	struct intel_digital_port *dig_port;
-	struct intel_shared_dpll *pll;
-	enum intel_dpll_id min, max;
-	bool ret;
-
-	if (encoder->type == INTEL_OUTPUT_DP_MST)
-		dig_port = enc_to_mst(&encoder->base)->primary;
-	else
-		dig_port = enc_to_dig_port(&encoder->base);
+	struct icl_port_dpll *port_dpll =
+		&crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
+	enum intel_dpll_id dpll_id;
 
-	if (dig_port->tc_mode == TC_PORT_TBT_ALT) {
-		min = DPLL_ID_ICL_TBTPLL;
-		max = min;
-		ret = icl_calc_dpll_state(crtc_state, encoder,
-					  &crtc_state->dpll_hw_state);
-	} else {
-		min = icl_tc_port_to_pll_id(tc_port);
-		max = min;
-		ret = icl_calc_mg_pll_state(crtc_state,
-					    &crtc_state->dpll_hw_state);
+	if (!icl_calc_dpll_state(crtc_state, encoder, &port_dpll->hw_state)) {
+		DRM_DEBUG_KMS("Could not calculate TBT PLL state.\n");
+		return false;
 	}
 
-	if (!ret) {
-		DRM_DEBUG_KMS("Could not calculate PLL state.\n");
+	port_dpll->pll = intel_find_shared_dpll(state, crtc,
+						&port_dpll->hw_state,
+						DPLL_ID_ICL_TBTPLL,
+						DPLL_ID_ICL_TBTPLL);
+	if (!port_dpll->pll) {
+		DRM_DEBUG_KMS("No TBT-ALT PLL found\n");
 		return false;
 	}
+	intel_reference_shared_dpll(state, crtc,
+				    port_dpll->pll, &port_dpll->hw_state);
 
 
-	pll = intel_find_shared_dpll(state, crtc,
-				     &crtc_state->dpll_hw_state,
-				     min, max);
-	if (!pll) {
-		DRM_DEBUG_KMS("No PLL selected\n");
-		return false;
+	port_dpll = &crtc_state->icl_port_dplls[ICL_PORT_DPLL_MG_PHY];
+	if (!icl_calc_mg_pll_state(crtc_state, &port_dpll->hw_state)) {
+		DRM_DEBUG_KMS("Could not calculate MG PHY PLL state.\n");
+		goto err_unreference_tbt_pll;
 	}
 
+	dpll_id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv,
+							 encoder->port));
+	port_dpll->pll = intel_find_shared_dpll(state, crtc,
+						&port_dpll->hw_state,
+						dpll_id,
+						dpll_id);
+	if (!port_dpll->pll) {
+		DRM_DEBUG_KMS("No MG PHY PLL found\n");
+		goto err_unreference_tbt_pll;
+	}
 	intel_reference_shared_dpll(state, crtc,
-				    pll, &crtc_state->dpll_hw_state);
+				    port_dpll->pll, &port_dpll->hw_state);
 
-	crtc_state->shared_dpll = pll;
+	icl_update_active_dpll(state, crtc, encoder);
 
 	return true;
+
+err_unreference_tbt_pll:
+	port_dpll = &crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
+	intel_unreference_shared_dpll(state, crtc, port_dpll->pll);
+
+	return false;
 }
 
 static bool icl_get_dplls(struct intel_atomic_state *state,
@@ -2957,6 +3008,24 @@  static bool icl_get_dplls(struct intel_atomic_state *state,
 	return false;
 }
 
+static void icl_put_dplls(struct intel_atomic_state *state,
+			  struct intel_crtc *crtc)
+{
+	struct intel_crtc_state *crtc_state =
+		intel_atomic_get_old_crtc_state(state, crtc);
+	enum icl_port_dpll_id id;
+
+	for (id = ICL_PORT_DPLL_DEFAULT; id < ICL_PORT_DPLL_COUNT; id++) {
+		struct icl_port_dpll *port_dpll =
+			&crtc_state->icl_port_dplls[id];
+
+		if (!port_dpll->pll)
+			continue;
+
+		intel_unreference_shared_dpll(state, crtc, port_dpll->pll);
+	}
+}
+
 static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
 				struct intel_shared_dpll *pll,
 				struct intel_dpll_hw_state *hw_state)
@@ -3330,7 +3399,7 @@  static const struct dpll_info icl_plls[] = {
 static const struct intel_dpll_mgr icl_pll_mgr = {
 	.dpll_info = icl_plls,
 	.get_dplls = icl_get_dplls,
-	.put_dplls = intel_put_dpll,
+	.put_dplls = icl_put_dplls,
 	.dump_hw_state = icl_dump_hw_state,
 };
 
@@ -3343,7 +3412,7 @@  static const struct dpll_info ehl_plls[] = {
 static const struct intel_dpll_mgr ehl_pll_mgr = {
 	.dpll_info = ehl_plls,
 	.get_dplls = icl_get_dplls,
-	.put_dplls = intel_put_dpll,
+	.put_dplls = icl_put_dplls,
 	.dump_hw_state = icl_dump_hw_state,
 };
 
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 6ffdcc06ad23..3bea81bde343 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -141,6 +141,13 @@  enum intel_dpll_id {
 };
 #define I915_NUM_PLLS 7
 
+enum icl_port_dpll_id {
+	ICL_PORT_DPLL_DEFAULT,
+	ICL_PORT_DPLL_MG_PHY,
+
+	ICL_PORT_DPLL_COUNT,
+};
+
 struct intel_dpll_hw_state {
 	/* i9xx, pch plls */
 	u32 dpll;
@@ -337,6 +344,8 @@  bool intel_reserve_shared_dplls(struct intel_atomic_state *state,
 				struct intel_encoder *encoder);
 void intel_release_shared_dplls(struct intel_atomic_state *state,
 				struct intel_crtc *crtc);
+void icl_set_active_port_dpll(struct intel_crtc_state *crtc_state,
+			      enum icl_port_dpll_id port_dpll_id);
 void intel_prepare_shared_dpll(const struct intel_crtc_state *crtc_state);
 void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state);
 void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f9b6f63ebbfe..7f1dcb8d8a28 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -812,6 +812,15 @@  struct intel_crtc_state {
 	/* Actual register state of the dpll, for shared dpll cross-checking. */
 	struct intel_dpll_hw_state dpll_hw_state;
 
+	/*
+	 * ICL reserved DPLLs for the CRTC/port. The active PLL is selected by
+	 * setting shared_dpll and dpll_hw_state to one of these reserved ones.
+	 */
+	struct icl_port_dpll {
+		struct intel_shared_dpll *pll;
+		struct intel_dpll_hw_state hw_state;
+	} icl_port_dplls[ICL_PORT_DPLL_COUNT];
+
 	/* DSI PLL registers */
 	struct {
 		u32 ctrl, div;

Comments

On Fri, Jun 07, 2019 at 08:41:29PM +0300, Imre Deak wrote:
> When enabling a TypeC port we need to reserve all the required PLLs for
> it, the TBT PLL for TBT-alt and the MG PHY PLL for DP-alt/legacy sinks.
> We can select the proper PLL for the current port mode from the reserved
> PLLs only once we selected and locked down the port mode for the whole
> duration of the port's active state. Resetting and locking down the port
> mode can in turn happen only during the modeset commit phase once we
> disabled the given port and the PLL it used.
> 
> To support the above reserve-and-select PLL semantic we store the
> reserved PLLs along with their HW state in the CRTC state and provide a
> way to select the active PLL from these. The selected PLL along with its
> HW state will be pointed at by crtc_state->shared_dpll/dpll_hw_state as
> in the case of other port types.
> 
> Besides reserving all required PLLs no functional changes.
> 
> v2:
> - Fix releasing the ICL PLLs, not clearing the PLLs from the old
>   crtc_state.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  11 +-
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 151 +++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |   9 ++
>  drivers/gpu/drm/i915/intel_drv.h      |   9 ++
>  4 files changed, 138 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7381fb2e1240..006be3c3f1bd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9880,6 +9880,7 @@ static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
>  				enum port port,
>  				struct intel_crtc_state *pipe_config)
>  {
> +	enum icl_port_dpll_id port_dpll_id;
>  	enum intel_dpll_id id;
>  	u32 temp;
>  
> @@ -9887,22 +9888,28 @@ static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
>  		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
>  		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
>  		id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> +		port_dpll_id = ICL_PORT_DPLL_DEFAULT;
>  	} else if (intel_port_is_tc(dev_priv, port)) {
>  		u32 clk_sel = I915_READ(DDI_CLK_SEL(port)) & DDI_CLK_SEL_MASK;
>  
>  		if (clk_sel == DDI_CLK_SEL_MG) {
>  			id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv,
>  								    port));
> +			port_dpll_id = ICL_PORT_DPLL_MG_PHY;
>  		} else {
>  			WARN_ON(clk_sel < DDI_CLK_SEL_TBT_162);
>  			id = DPLL_ID_ICL_TBTPLL;
> +			port_dpll_id = ICL_PORT_DPLL_DEFAULT;
>  		}
>  	} else {
>  		WARN(1, "Invalid port %x\n", port);
>  		return;
>  	}
>  
> -	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
> +	pipe_config->icl_port_dplls[port_dpll_id].pll =
> +		intel_get_shared_dpll_by_id(dev_priv, id);
> +
> +	icl_set_active_port_dpll(pipe_config, port_dpll_id);
>  }
>  
>  static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
> @@ -12041,6 +12048,8 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	saved_state->scaler_state = crtc_state->scaler_state;
>  	saved_state->shared_dpll = crtc_state->shared_dpll;
>  	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> +	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
> +	       sizeof(saved_state->icl_port_dplls));
>  	saved_state->crc_enabled = crtc_state->crc_enabled;
>  	if (IS_G4X(dev_priv) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 8ac293db43a5..17441d5f990e 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2856,34 +2856,79 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state,
>  	return true;
>  }
>  
> +/**
> + * icl_set_active_port_dpll - select the active port DPLL for a given CRTC
> + * @crtc_state: state for the CRTC to select the DPLL for
> + * @port_dpll_id: the active @port_dpll_id to select
> + *
> + * Select the given @port_dpll_id instance from the DPLLs reserved for the
> + * CRTC.
> + */
> +void icl_set_active_port_dpll(struct intel_crtc_state *crtc_state,
> +			      enum icl_port_dpll_id port_dpll_id)
> +{
> +	struct icl_port_dpll *port_dpll =
> +		&crtc_state->icl_port_dplls[port_dpll_id];
> +
> +	crtc_state->shared_dpll = port_dpll->pll;
> +	crtc_state->dpll_hw_state = port_dpll->hw_state;
> +}
> +
> +static void icl_update_active_dpll(struct intel_atomic_state *state,
> +				   struct intel_crtc *crtc,
> +				   struct intel_encoder *encoder)
> +{
> +	struct intel_crtc_state *crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	struct intel_digital_port *primary_port;
> +	enum icl_port_dpll_id port_dpll_id;
> +
> +	primary_port = encoder->type == INTEL_OUTPUT_DP_MST ?
> +		enc_to_mst(&encoder->base)->primary :
> +		enc_to_dig_port(&encoder->base);
> +
> +	switch (primary_port->tc_mode) {
> +	case TC_PORT_TBT_ALT:
> +		port_dpll_id = ICL_PORT_DPLL_DEFAULT;
> +		break;
> +	case TC_PORT_DP_ALT:
> +	case TC_PORT_LEGACY:
> +		port_dpll_id = ICL_PORT_DPLL_MG_PHY;
> +		break;
> +	}
> +
> +	icl_set_active_port_dpll(crtc_state, port_dpll_id);
> +}
> +
>  static bool icl_get_combo_phy_dpll(struct intel_atomic_state *state,
>  				   struct intel_crtc *crtc,
>  				   struct intel_encoder *encoder)
>  {
>  	struct intel_crtc_state *crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
> -	struct intel_shared_dpll *pll;
> +	struct icl_port_dpll *port_dpll =
> +		&crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
>  
> -	if (!icl_calc_dpll_state(crtc_state, encoder,
> -				 &crtc_state->dpll_hw_state)) {
> +	if (!icl_calc_dpll_state(crtc_state, encoder, &port_dpll->hw_state)) {
>  		DRM_DEBUG_KMS("Could not calculate combo PHY PLL state.\n");
>  
>  		return false;
>  	}
>  
> -	pll = intel_find_shared_dpll(state, crtc, &crtc_state->dpll_hw_state,
> -				     DPLL_ID_ICL_DPLL0,
> -				     DPLL_ID_ICL_DPLL1);
> -	if (!pll) {
> +	port_dpll->pll = intel_find_shared_dpll(state, crtc,
> +						&port_dpll->hw_state,
> +						DPLL_ID_ICL_DPLL0,
> +						DPLL_ID_ICL_DPLL1);
> +	if (!port_dpll->pll) {
>  		DRM_DEBUG_KMS("No combo PHY PLL found for port %c\n",
>  			      port_name(encoder->port));
>  		return false;
>  	}
>  
>  	intel_reference_shared_dpll(state, crtc,
> -				    pll, &crtc_state->dpll_hw_state);
> +				    port_dpll->pll, &port_dpll->hw_state);
>  
> -	crtc_state->shared_dpll = pll;
> +	icl_update_active_dpll(state, crtc, encoder);
>  
>  	return true;
>  }
> @@ -2895,49 +2940,55 @@ static bool icl_get_tc_phy_dplls(struct intel_atomic_state *state,
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct intel_crtc_state *crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder->port);
> -	struct intel_digital_port *dig_port;
> -	struct intel_shared_dpll *pll;
> -	enum intel_dpll_id min, max;
> -	bool ret;
> -
> -	if (encoder->type == INTEL_OUTPUT_DP_MST)
> -		dig_port = enc_to_mst(&encoder->base)->primary;
> -	else
> -		dig_port = enc_to_dig_port(&encoder->base);
> +	struct icl_port_dpll *port_dpll =
> +		&crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];

I would move that initialization next to the code so that the
tbt vs. mg cases look more like twins.

> +	enum intel_dpll_id dpll_id;
>  
> -	if (dig_port->tc_mode == TC_PORT_TBT_ALT) {
> -		min = DPLL_ID_ICL_TBTPLL;
> -		max = min;
> -		ret = icl_calc_dpll_state(crtc_state, encoder,
> -					  &crtc_state->dpll_hw_state);
> -	} else {
> -		min = icl_tc_port_to_pll_id(tc_port);
> -		max = min;
> -		ret = icl_calc_mg_pll_state(crtc_state,
> -					    &crtc_state->dpll_hw_state);
> +	if (!icl_calc_dpll_state(crtc_state, encoder, &port_dpll->hw_state)) {
> +		DRM_DEBUG_KMS("Could not calculate TBT PLL state.\n");
> +		return false;
>  	}
>  
> -	if (!ret) {
> -		DRM_DEBUG_KMS("Could not calculate PLL state.\n");
> +	port_dpll->pll = intel_find_shared_dpll(state, crtc,
> +						&port_dpll->hw_state,
> +						DPLL_ID_ICL_TBTPLL,
> +						DPLL_ID_ICL_TBTPLL);
> +	if (!port_dpll->pll) {
> +		DRM_DEBUG_KMS("No TBT-ALT PLL found\n");
>  		return false;
>  	}
> +	intel_reference_shared_dpll(state, crtc,
> +				    port_dpll->pll, &port_dpll->hw_state);
>  
>  
> -	pll = intel_find_shared_dpll(state, crtc,
> -				     &crtc_state->dpll_hw_state,
> -				     min, max);
> -	if (!pll) {
> -		DRM_DEBUG_KMS("No PLL selected\n");
> -		return false;
> +	port_dpll = &crtc_state->icl_port_dplls[ICL_PORT_DPLL_MG_PHY];
> +	if (!icl_calc_mg_pll_state(crtc_state, &port_dpll->hw_state)) {
> +		DRM_DEBUG_KMS("Could not calculate MG PHY PLL state.\n");
> +		goto err_unreference_tbt_pll;
>  	}
>  
> +	dpll_id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv,
> +							 encoder->port));
> +	port_dpll->pll = intel_find_shared_dpll(state, crtc,
> +						&port_dpll->hw_state,
> +						dpll_id,
> +						dpll_id);
> +	if (!port_dpll->pll) {
> +		DRM_DEBUG_KMS("No MG PHY PLL found\n");
> +		goto err_unreference_tbt_pll;
> +	}
>  	intel_reference_shared_dpll(state, crtc,
> -				    pll, &crtc_state->dpll_hw_state);
> +				    port_dpll->pll, &port_dpll->hw_state);
>  
> -	crtc_state->shared_dpll = pll;
> +	icl_update_active_dpll(state, crtc, encoder);
>  
>  	return true;
> +
> +err_unreference_tbt_pll:
> +	port_dpll = &crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
> +	intel_unreference_shared_dpll(state, crtc, port_dpll->pll);
> +
> +	return false;
>  }
>  
>  static bool icl_get_dplls(struct intel_atomic_state *state,
> @@ -2957,6 +3008,24 @@ static bool icl_get_dplls(struct intel_atomic_state *state,
>  	return false;
>  }
>  
> +static void icl_put_dplls(struct intel_atomic_state *state,
> +			  struct intel_crtc *crtc)
> +{
> +	struct intel_crtc_state *crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
> +	enum icl_port_dpll_id id;
> +
> +	for (id = ICL_PORT_DPLL_DEFAULT; id < ICL_PORT_DPLL_COUNT; id++) {
> +		struct icl_port_dpll *port_dpll =
> +			&crtc_state->icl_port_dplls[id];
> +
> +		if (!port_dpll->pll)
> +			continue;
> +
> +		intel_unreference_shared_dpll(state, crtc, port_dpll->pll);
> +	}
> +}
> +
>  static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
>  				struct intel_shared_dpll *pll,
>  				struct intel_dpll_hw_state *hw_state)
> @@ -3330,7 +3399,7 @@ static const struct dpll_info icl_plls[] = {
>  static const struct intel_dpll_mgr icl_pll_mgr = {
>  	.dpll_info = icl_plls,
>  	.get_dplls = icl_get_dplls,
> -	.put_dplls = intel_put_dpll,
> +	.put_dplls = icl_put_dplls,
>  	.dump_hw_state = icl_dump_hw_state,
>  };
>  
> @@ -3343,7 +3412,7 @@ static const struct dpll_info ehl_plls[] = {
>  static const struct intel_dpll_mgr ehl_pll_mgr = {
>  	.dpll_info = ehl_plls,
>  	.get_dplls = icl_get_dplls,
> -	.put_dplls = intel_put_dpll,
> +	.put_dplls = icl_put_dplls,
>  	.dump_hw_state = icl_dump_hw_state,
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 6ffdcc06ad23..3bea81bde343 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -141,6 +141,13 @@ enum intel_dpll_id {
>  };
>  #define I915_NUM_PLLS 7
>  
> +enum icl_port_dpll_id {
> +	ICL_PORT_DPLL_DEFAULT,
> +	ICL_PORT_DPLL_MG_PHY,
> +
> +	ICL_PORT_DPLL_COUNT,
> +};
> +
>  struct intel_dpll_hw_state {
>  	/* i9xx, pch plls */
>  	u32 dpll;
> @@ -337,6 +344,8 @@ bool intel_reserve_shared_dplls(struct intel_atomic_state *state,
>  				struct intel_encoder *encoder);
>  void intel_release_shared_dplls(struct intel_atomic_state *state,
>  				struct intel_crtc *crtc);
> +void icl_set_active_port_dpll(struct intel_crtc_state *crtc_state,
> +			      enum icl_port_dpll_id port_dpll_id);
>  void intel_prepare_shared_dpll(const struct intel_crtc_state *crtc_state);
>  void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state);
>  void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f9b6f63ebbfe..7f1dcb8d8a28 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -812,6 +812,15 @@ struct intel_crtc_state {
>  	/* Actual register state of the dpll, for shared dpll cross-checking. */
>  	struct intel_dpll_hw_state dpll_hw_state;
>  
> +	/*
> +	 * ICL reserved DPLLs for the CRTC/port. The active PLL is selected by
> +	 * setting shared_dpll and dpll_hw_state to one of these reserved ones.
> +	 */
> +	struct icl_port_dpll {
> +		struct intel_shared_dpll *pll;
> +		struct intel_dpll_hw_state hw_state;

I was pondering if we should use this array on non-icl too.
Would avoid having three instances of this state embedded in
our crtc state. But I don't know how big the struct is so not
sure if it's worth it. 

Anyways, something to think about for a future optimization.

> +	} icl_port_dplls[ICL_PORT_DPLL_COUNT];
> +
>  	/* DSI PLL registers */
>  	struct {
>  		u32 ctrl, div;
> -- 
> 2.17.1
On Tue, Jun 18, 2019 at 08:25:52PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 07, 2019 at 08:41:29PM +0300, Imre Deak wrote:
> > When enabling a TypeC port we need to reserve all the required PLLs for
> > it, the TBT PLL for TBT-alt and the MG PHY PLL for DP-alt/legacy sinks.
> > We can select the proper PLL for the current port mode from the reserved
> > PLLs only once we selected and locked down the port mode for the whole
> > duration of the port's active state. Resetting and locking down the port
> > mode can in turn happen only during the modeset commit phase once we
> > disabled the given port and the PLL it used.
> > 
> > To support the above reserve-and-select PLL semantic we store the
> > reserved PLLs along with their HW state in the CRTC state and provide a
> > way to select the active PLL from these. The selected PLL along with its
> > HW state will be pointed at by crtc_state->shared_dpll/dpll_hw_state as
> > in the case of other port types.
> > 
> > Besides reserving all required PLLs no functional changes.
> > 
> > v2:
> > - Fix releasing the ICL PLLs, not clearing the PLLs from the old
> >   crtc_state.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  |  11 +-
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 151 +++++++++++++++++++-------
> >  drivers/gpu/drm/i915/intel_dpll_mgr.h |   9 ++
> >  drivers/gpu/drm/i915/intel_drv.h      |   9 ++
> >  4 files changed, 138 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7381fb2e1240..006be3c3f1bd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9880,6 +9880,7 @@ static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
> >  				enum port port,
> >  				struct intel_crtc_state *pipe_config)
> >  {
> > +	enum icl_port_dpll_id port_dpll_id;
> >  	enum intel_dpll_id id;
> >  	u32 temp;
> >  
> > @@ -9887,22 +9888,28 @@ static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
> >  		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
> >  		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
> >  		id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> > +		port_dpll_id = ICL_PORT_DPLL_DEFAULT;
> >  	} else if (intel_port_is_tc(dev_priv, port)) {
> >  		u32 clk_sel = I915_READ(DDI_CLK_SEL(port)) & DDI_CLK_SEL_MASK;
> >  
> >  		if (clk_sel == DDI_CLK_SEL_MG) {
> >  			id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv,
> >  								    port));
> > +			port_dpll_id = ICL_PORT_DPLL_MG_PHY;
> >  		} else {
> >  			WARN_ON(clk_sel < DDI_CLK_SEL_TBT_162);
> >  			id = DPLL_ID_ICL_TBTPLL;
> > +			port_dpll_id = ICL_PORT_DPLL_DEFAULT;
> >  		}
> >  	} else {
> >  		WARN(1, "Invalid port %x\n", port);
> >  		return;
> >  	}
> >  
> > -	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
> > +	pipe_config->icl_port_dplls[port_dpll_id].pll =
> > +		intel_get_shared_dpll_by_id(dev_priv, id);
> > +
> > +	icl_set_active_port_dpll(pipe_config, port_dpll_id);
> >  }
> >  
> >  static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
> > @@ -12041,6 +12048,8 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> >  	saved_state->scaler_state = crtc_state->scaler_state;
> >  	saved_state->shared_dpll = crtc_state->shared_dpll;
> >  	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> > +	memcpy(saved_state->icl_port_dplls, crtc_state->icl_port_dplls,
> > +	       sizeof(saved_state->icl_port_dplls));
> >  	saved_state->crc_enabled = crtc_state->crc_enabled;
> >  	if (IS_G4X(dev_priv) ||
> >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index 8ac293db43a5..17441d5f990e 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -2856,34 +2856,79 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state,
> >  	return true;
> >  }
> >  
> > +/**
> > + * icl_set_active_port_dpll - select the active port DPLL for a given CRTC
> > + * @crtc_state: state for the CRTC to select the DPLL for
> > + * @port_dpll_id: the active @port_dpll_id to select
> > + *
> > + * Select the given @port_dpll_id instance from the DPLLs reserved for the
> > + * CRTC.
> > + */
> > +void icl_set_active_port_dpll(struct intel_crtc_state *crtc_state,
> > +			      enum icl_port_dpll_id port_dpll_id)
> > +{
> > +	struct icl_port_dpll *port_dpll =
> > +		&crtc_state->icl_port_dplls[port_dpll_id];
> > +
> > +	crtc_state->shared_dpll = port_dpll->pll;
> > +	crtc_state->dpll_hw_state = port_dpll->hw_state;
> > +}
> > +
> > +static void icl_update_active_dpll(struct intel_atomic_state *state,
> > +				   struct intel_crtc *crtc,
> > +				   struct intel_encoder *encoder)
> > +{
> > +	struct intel_crtc_state *crtc_state =
> > +		intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_digital_port *primary_port;
> > +	enum icl_port_dpll_id port_dpll_id;
> > +
> > +	primary_port = encoder->type == INTEL_OUTPUT_DP_MST ?
> > +		enc_to_mst(&encoder->base)->primary :
> > +		enc_to_dig_port(&encoder->base);
> > +
> > +	switch (primary_port->tc_mode) {
> > +	case TC_PORT_TBT_ALT:
> > +		port_dpll_id = ICL_PORT_DPLL_DEFAULT;
> > +		break;
> > +	case TC_PORT_DP_ALT:
> > +	case TC_PORT_LEGACY:
> > +		port_dpll_id = ICL_PORT_DPLL_MG_PHY;
> > +		break;
> > +	}
> > +
> > +	icl_set_active_port_dpll(crtc_state, port_dpll_id);
> > +}
> > +
> >  static bool icl_get_combo_phy_dpll(struct intel_atomic_state *state,
> >  				   struct intel_crtc *crtc,
> >  				   struct intel_encoder *encoder)
> >  {
> >  	struct intel_crtc_state *crtc_state =
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> > -	struct intel_shared_dpll *pll;
> > +	struct icl_port_dpll *port_dpll =
> > +		&crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
> >  
> > -	if (!icl_calc_dpll_state(crtc_state, encoder,
> > -				 &crtc_state->dpll_hw_state)) {
> > +	if (!icl_calc_dpll_state(crtc_state, encoder, &port_dpll->hw_state)) {
> >  		DRM_DEBUG_KMS("Could not calculate combo PHY PLL state.\n");
> >  
> >  		return false;
> >  	}
> >  
> > -	pll = intel_find_shared_dpll(state, crtc, &crtc_state->dpll_hw_state,
> > -				     DPLL_ID_ICL_DPLL0,
> > -				     DPLL_ID_ICL_DPLL1);
> > -	if (!pll) {
> > +	port_dpll->pll = intel_find_shared_dpll(state, crtc,
> > +						&port_dpll->hw_state,
> > +						DPLL_ID_ICL_DPLL0,
> > +						DPLL_ID_ICL_DPLL1);
> > +	if (!port_dpll->pll) {
> >  		DRM_DEBUG_KMS("No combo PHY PLL found for port %c\n",
> >  			      port_name(encoder->port));
> >  		return false;
> >  	}
> >  
> >  	intel_reference_shared_dpll(state, crtc,
> > -				    pll, &crtc_state->dpll_hw_state);
> > +				    port_dpll->pll, &port_dpll->hw_state);
> >  
> > -	crtc_state->shared_dpll = pll;
> > +	icl_update_active_dpll(state, crtc, encoder);
> >  
> >  	return true;
> >  }
> > @@ -2895,49 +2940,55 @@ static bool icl_get_tc_phy_dplls(struct intel_atomic_state *state,
> >  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> >  	struct intel_crtc_state *crtc_state =
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> > -	enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder->port);
> > -	struct intel_digital_port *dig_port;
> > -	struct intel_shared_dpll *pll;
> > -	enum intel_dpll_id min, max;
> > -	bool ret;
> > -
> > -	if (encoder->type == INTEL_OUTPUT_DP_MST)
> > -		dig_port = enc_to_mst(&encoder->base)->primary;
> > -	else
> > -		dig_port = enc_to_dig_port(&encoder->base);
> > +	struct icl_port_dpll *port_dpll =
> > +		&crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
> 
> I would move that initialization next to the code so that the
> tbt vs. mg cases look more like twins.

Ok.

> 
> > +	enum intel_dpll_id dpll_id;
> >  
> > -	if (dig_port->tc_mode == TC_PORT_TBT_ALT) {
> > -		min = DPLL_ID_ICL_TBTPLL;
> > -		max = min;
> > -		ret = icl_calc_dpll_state(crtc_state, encoder,
> > -					  &crtc_state->dpll_hw_state);
> > -	} else {
> > -		min = icl_tc_port_to_pll_id(tc_port);
> > -		max = min;
> > -		ret = icl_calc_mg_pll_state(crtc_state,
> > -					    &crtc_state->dpll_hw_state);
> > +	if (!icl_calc_dpll_state(crtc_state, encoder, &port_dpll->hw_state)) {
> > +		DRM_DEBUG_KMS("Could not calculate TBT PLL state.\n");
> > +		return false;
> >  	}
> >  
> > -	if (!ret) {
> > -		DRM_DEBUG_KMS("Could not calculate PLL state.\n");
> > +	port_dpll->pll = intel_find_shared_dpll(state, crtc,
> > +						&port_dpll->hw_state,
> > +						DPLL_ID_ICL_TBTPLL,
> > +						DPLL_ID_ICL_TBTPLL);
> > +	if (!port_dpll->pll) {
> > +		DRM_DEBUG_KMS("No TBT-ALT PLL found\n");
> >  		return false;
> >  	}
> > +	intel_reference_shared_dpll(state, crtc,
> > +				    port_dpll->pll, &port_dpll->hw_state);
> >  
> >  
> > -	pll = intel_find_shared_dpll(state, crtc,
> > -				     &crtc_state->dpll_hw_state,
> > -				     min, max);
> > -	if (!pll) {
> > -		DRM_DEBUG_KMS("No PLL selected\n");
> > -		return false;
> > +	port_dpll = &crtc_state->icl_port_dplls[ICL_PORT_DPLL_MG_PHY];
> > +	if (!icl_calc_mg_pll_state(crtc_state, &port_dpll->hw_state)) {
> > +		DRM_DEBUG_KMS("Could not calculate MG PHY PLL state.\n");
> > +		goto err_unreference_tbt_pll;
> >  	}
> >  
> > +	dpll_id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv,
> > +							 encoder->port));
> > +	port_dpll->pll = intel_find_shared_dpll(state, crtc,
> > +						&port_dpll->hw_state,
> > +						dpll_id,
> > +						dpll_id);
> > +	if (!port_dpll->pll) {
> > +		DRM_DEBUG_KMS("No MG PHY PLL found\n");
> > +		goto err_unreference_tbt_pll;
> > +	}
> >  	intel_reference_shared_dpll(state, crtc,
> > -				    pll, &crtc_state->dpll_hw_state);
> > +				    port_dpll->pll, &port_dpll->hw_state);
> >  
> > -	crtc_state->shared_dpll = pll;
> > +	icl_update_active_dpll(state, crtc, encoder);
> >  
> >  	return true;
> > +
> > +err_unreference_tbt_pll:
> > +	port_dpll = &crtc_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
> > +	intel_unreference_shared_dpll(state, crtc, port_dpll->pll);
> > +
> > +	return false;
> >  }
> >  
> >  static bool icl_get_dplls(struct intel_atomic_state *state,
> > @@ -2957,6 +3008,24 @@ static bool icl_get_dplls(struct intel_atomic_state *state,
> >  	return false;
> >  }
> >  
> > +static void icl_put_dplls(struct intel_atomic_state *state,
> > +			  struct intel_crtc *crtc)
> > +{
> > +	struct intel_crtc_state *crtc_state =
> > +		intel_atomic_get_old_crtc_state(state, crtc);
> > +	enum icl_port_dpll_id id;
> > +
> > +	for (id = ICL_PORT_DPLL_DEFAULT; id < ICL_PORT_DPLL_COUNT; id++) {
> > +		struct icl_port_dpll *port_dpll =
> > +			&crtc_state->icl_port_dplls[id];
> > +
> > +		if (!port_dpll->pll)
> > +			continue;
> > +
> > +		intel_unreference_shared_dpll(state, crtc, port_dpll->pll);
> > +	}
> > +}
> > +
> >  static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >  				struct intel_shared_dpll *pll,
> >  				struct intel_dpll_hw_state *hw_state)
> > @@ -3330,7 +3399,7 @@ static const struct dpll_info icl_plls[] = {
> >  static const struct intel_dpll_mgr icl_pll_mgr = {
> >  	.dpll_info = icl_plls,
> >  	.get_dplls = icl_get_dplls,
> > -	.put_dplls = intel_put_dpll,
> > +	.put_dplls = icl_put_dplls,
> >  	.dump_hw_state = icl_dump_hw_state,
> >  };
> >  
> > @@ -3343,7 +3412,7 @@ static const struct dpll_info ehl_plls[] = {
> >  static const struct intel_dpll_mgr ehl_pll_mgr = {
> >  	.dpll_info = ehl_plls,
> >  	.get_dplls = icl_get_dplls,
> > -	.put_dplls = intel_put_dpll,
> > +	.put_dplls = icl_put_dplls,
> >  	.dump_hw_state = icl_dump_hw_state,
> >  };
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > index 6ffdcc06ad23..3bea81bde343 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > @@ -141,6 +141,13 @@ enum intel_dpll_id {
> >  };
> >  #define I915_NUM_PLLS 7
> >  
> > +enum icl_port_dpll_id {
> > +	ICL_PORT_DPLL_DEFAULT,
> > +	ICL_PORT_DPLL_MG_PHY,
> > +
> > +	ICL_PORT_DPLL_COUNT,
> > +};
> > +
> >  struct intel_dpll_hw_state {
> >  	/* i9xx, pch plls */
> >  	u32 dpll;
> > @@ -337,6 +344,8 @@ bool intel_reserve_shared_dplls(struct intel_atomic_state *state,
> >  				struct intel_encoder *encoder);
> >  void intel_release_shared_dplls(struct intel_atomic_state *state,
> >  				struct intel_crtc *crtc);
> > +void icl_set_active_port_dpll(struct intel_crtc_state *crtc_state,
> > +			      enum icl_port_dpll_id port_dpll_id);
> >  void intel_prepare_shared_dpll(const struct intel_crtc_state *crtc_state);
> >  void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state);
> >  void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index f9b6f63ebbfe..7f1dcb8d8a28 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -812,6 +812,15 @@ struct intel_crtc_state {
> >  	/* Actual register state of the dpll, for shared dpll cross-checking. */
> >  	struct intel_dpll_hw_state dpll_hw_state;
> >  
> > +	/*
> > +	 * ICL reserved DPLLs for the CRTC/port. The active PLL is selected by
> > +	 * setting shared_dpll and dpll_hw_state to one of these reserved ones.
> > +	 */
> > +	struct icl_port_dpll {
> > +		struct intel_shared_dpll *pll;
> > +		struct intel_dpll_hw_state hw_state;
> 
> I was pondering if we should use this array on non-icl too.
> Would avoid having three instances of this state embedded in
> our crtc state. But I don't know how big the struct is so not
> sure if it's worth it. 
> 
> Anyways, something to think about for a future optimization.

Yep, would be good to make it more generic, expressing it also better
that hw_state is pll's state.

I suppose that would be instead of the above

	struct intel_port_dpll {
		struct intel_shared_dpll *pll;
		struct intel_dpll_hw_state hw_state;
	} port_dplls[PORT_DPLL_COUNT];

	struct intel_port_dpll *active_dpll;

removing also intel_crtc_state::dpll_hw_state and shared_dpll.

I chickened out of doing that too now, affecting many platforms, but
would be worth doing as a follow-up.

> 
> > +	} icl_port_dplls[ICL_PORT_DPLL_COUNT];
> > +
> >  	/* DSI PLL registers */
> >  	struct {
> >  		u32 ctrl, div;
> > -- 
> > 2.17.1
> 
> -- 
> Ville Syrjälä
> Intel