[20/23] drm/i915: Keep the TypeC port mode fixed when the port is active

Submitted by Imre Deak on June 4, 2019, 2:58 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Imre Deak June 4, 2019, 2:58 p.m.
The TypeC port mode needs to stay fixed whenever the port is active. Do
that by introducing a tc_link_refcount to account for active ports,
avoiding changing the port mode if a reference is held.

During the modeset commit phase we also have to reset the port mode and
update the active PLL reflecting the new port mode. We can do this only
once the port and its old PLL has been already disabled. Add the new
connector_update_prepare/complete hooks that are called around the whole
enabling sequence. The TypeC specific hooks of these will reset the port
mode, update the active PLL if the port will be active and ensure that
the port mode will stay fixed for the duration of the whole enabling
sequence by holding a tc_link_refcount.

During the port enabling, the pre_pll_enable/post_pll_disable hooks will
take/release a tc_link_refcount to ensure the port mode stays fixed
while the port is active.

Changing the port mode should also be avoided during connector detection
and AUX transfers if the port is active, we'll do that by checking the
port's tc_link_refcount.

When resetting the port mode we also have to take into account the
maximum lanes provided by the FIA. It's guaranteed to be 4 in TBT-alt
and legacy modes, but there may be less lanes available in DP-alt mode,
in which case we have to fall back to TBT-alt mode.

While at it also update icl_tc_phy_connect()'s code comment, reflecting
the current way of switching the port mode.

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_ddi.c      | 46 +++++++++++--
 drivers/gpu/drm/i915/intel_ddi.h      |  7 ++
 drivers/gpu/drm/i915/intel_display.c  | 90 +++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp.c       |  7 ++
 drivers/gpu/drm/i915/intel_dp_mst.c   |  6 ++
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 28 +++++++-
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  3 +
 drivers/gpu/drm/i915/intel_drv.h      |  9 +++
 drivers/gpu/drm/i915/intel_hdmi.c     |  7 ++
 drivers/gpu/drm/i915/intel_tc.c       | 93 ++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_tc.h       |  3 +
 11 files changed, 269 insertions(+), 30 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ad2f7bb2f50b..138950941246 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3610,6 +3610,38 @@  static void intel_ddi_set_fia_lane_count(struct intel_encoder *encoder,
 	I915_WRITE(PORT_TX_DFLEXDPMLE1, val);
 }
 
+void
+intel_ddi_connector_update_prepare(struct intel_atomic_state *state,
+				   struct intel_connector *connector)
+{
+	struct drm_connector_state *conn_state =
+		drm_atomic_get_new_connector_state(&state->base,
+						   &connector->base);
+	struct intel_crtc *crtc = conn_state->crtc ?
+		to_intel_crtc(conn_state->crtc) : NULL;
+	struct intel_crtc_state *crtc_state =
+		crtc ? intel_atomic_get_new_crtc_state(state, crtc) : NULL;
+	struct intel_digital_port *primary_port =
+		intel_connector_primary_digital_port(connector);
+	int required_lanes = crtc_state ? crtc_state->lane_count : 1;
+
+	WARN_ON(crtc && crtc->active);
+
+	intel_tc_port_get_link(primary_port, required_lanes);
+	if (crtc_state && crtc_state->base.active)
+		intel_update_active_dpll(state, crtc, &primary_port->base);
+}
+
+void
+intel_ddi_connector_update_complete(struct intel_atomic_state *state,
+				    struct intel_connector *connector)
+{
+	struct intel_digital_port *primary_port =
+		intel_connector_primary_digital_port(connector);
+
+	intel_tc_port_put_link(primary_port);
+}
+
 static void
 intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
 			 const struct intel_crtc_state *crtc_state,
@@ -3617,10 +3649,13 @@  intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	bool is_tc_port = intel_port_is_tc(dev_priv, encoder->port);
 	enum port port = encoder->port;
 
-	if (intel_crtc_has_dp_encoder(crtc_state) ||
-	    intel_port_is_tc(dev_priv, encoder->port))
+	if (is_tc_port)
+		intel_tc_port_get_link(dig_port, crtc_state->lane_count);
+
+	if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port)
 		intel_display_power_get(dev_priv,
 					intel_ddi_main_link_aux_domain(dig_port));
 
@@ -3645,11 +3680,14 @@  intel_ddi_post_pll_disable(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	bool is_tc_port = intel_port_is_tc(dev_priv, encoder->port);
 
-	if (intel_crtc_has_dp_encoder(crtc_state) ||
-	    intel_port_is_tc(dev_priv, encoder->port))
+	if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port)
 		intel_display_power_put_unchecked(dev_priv,
 						  intel_ddi_main_link_aux_domain(dig_port));
+
+	if (is_tc_port)
+		intel_tc_port_put_link(dig_port);
 }
 
 void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
diff --git a/drivers/gpu/drm/i915/intel_ddi.h b/drivers/gpu/drm/i915/intel_ddi.h
index 9cf69175942e..1559d1fbf7bd 100644
--- a/drivers/gpu/drm/i915/intel_ddi.h
+++ b/drivers/gpu/drm/i915/intel_ddi.h
@@ -12,6 +12,7 @@ 
 
 struct drm_connector_state;
 struct drm_i915_private;
+struct intel_atomic_state;
 struct intel_connector;
 struct intel_crtc;
 struct intel_crtc_state;
@@ -35,6 +36,12 @@  void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp);
 bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
 void intel_ddi_get_config(struct intel_encoder *encoder,
 			  struct intel_crtc_state *pipe_config);
+void
+intel_ddi_connector_update_prepare(struct intel_atomic_state *state,
+				   struct intel_connector *connector);
+void
+intel_ddi_connector_update_complete(struct intel_atomic_state *state,
+				    struct intel_connector *connector);
 void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state *crtc_state,
 				    bool state);
 void intel_ddi_compute_min_voltage_level(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f86b5b848cbc..2c65587d1622 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -520,6 +520,20 @@  needs_modeset(const struct drm_crtc_state *state)
 	return drm_atomic_crtc_needs_modeset(state);
 }
 
+static bool
+intel_connector_needs_modeset(struct intel_atomic_state *state,
+			      const struct drm_connector_state *old_conn_state,
+			      const struct drm_connector_state *new_conn_state)
+{
+	if (new_conn_state->crtc != old_conn_state->crtc ||
+	    (new_conn_state->crtc &&
+	     needs_modeset(drm_atomic_get_new_crtc_state(&state->base,
+							 new_conn_state->crtc))))
+		return true;
+
+	return false;
+}
+
 /*
  * Platform specific helpers to calculate the port PLL loopback- (clock.m),
  * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
@@ -6032,6 +6046,52 @@  static void intel_crtc_disable_planes(struct intel_atomic_state *state,
 	intel_frontbuffer_flip(dev_priv, fb_bits);
 }
 
+static void intel_connectors_update_prepare(struct intel_atomic_state *state)
+{
+	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *new_conn_state;
+	struct drm_connector *conn;
+	int i;
+
+	for_each_oldnew_connector_in_state(&state->base, conn,
+					   old_conn_state, new_conn_state, i) {
+		struct intel_connector *connector = to_intel_connector(conn);
+
+		if (!connector->update_prepare)
+			continue;
+
+		if (!intel_connector_needs_modeset(state,
+						   old_conn_state,
+						   new_conn_state))
+			continue;
+
+		connector->update_prepare(state, connector);
+	}
+}
+
+static void intel_connectors_update_complete(struct intel_atomic_state *state)
+{
+	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *new_conn_state;
+	struct drm_connector *conn;
+	int i;
+
+	for_each_oldnew_connector_in_state(&state->base, conn,
+					   old_conn_state, new_conn_state, i) {
+		struct intel_connector *connector = to_intel_connector(conn);
+
+		if (!connector->update_complete)
+			continue;
+
+		if (!intel_connector_needs_modeset(state,
+						   old_conn_state,
+						   new_conn_state))
+			continue;
+
+		connector->update_complete(state, connector);
+	}
+}
+
 static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
 					  struct intel_crtc_state *crtc_state,
 					  struct drm_atomic_state *old_state)
@@ -6556,6 +6616,28 @@  static void i9xx_pfit_enable(const struct intel_crtc_state *crtc_state)
 	I915_WRITE(BCLRPAT(crtc->pipe), 0);
 }
 
+/*
+ * intel_connector_primary_digital_port - get the primary port for a connector
+ * @connector: connector for which to return the port
+ *
+ * Returns the primary digital port for a DP MST, the single digital port for
+ * DP SST and HDMI and NULL for all other connector types.
+ */
+struct intel_digital_port *
+intel_connector_primary_digital_port(struct intel_connector *connector)
+{
+	struct intel_encoder *encoder;
+
+	if (connector->mst_port)
+		return dp_to_dig_port(connector->mst_port);
+
+	encoder = intel_attached_encoder(&connector->base);
+	if (WARN_ON(!encoder))
+		return NULL;
+
+	return enc_to_dig_port(&encoder->base);
+}
+
 bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port)
 {
 	if (port == PORT_NONE)
@@ -13805,14 +13887,20 @@  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		}
 	}
 
+	if (intel_state->modeset)
+		intel_connectors_update_prepare(intel_state);
+
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	dev_priv->display.update_crtcs(state);
 
-	if (intel_state->modeset)
+	if (intel_state->modeset) {
+		intel_connectors_update_complete(intel_state);
+
 		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
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b984410f41a4..2f63476e3cf2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -7195,6 +7195,13 @@  intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	else
 		intel_connector->get_hw_state = intel_connector_get_hw_state;
 
+	if (intel_port_is_tc(dev_priv, intel_encoder->port)) {
+		intel_connector->update_prepare =
+			intel_ddi_connector_update_prepare;
+		intel_connector->update_complete =
+			intel_ddi_connector_update_complete;
+	}
+
 	/* init MST on ports that can support it */
 	if (HAS_DP_MST(dev_priv) && !intel_dp_is_edp(intel_dp) &&
 	    (port == PORT_B || port == PORT_C ||
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 0caf645fbbb8..9d5b048b9c96 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -505,6 +505,12 @@  static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	if (!intel_connector)
 		return NULL;
 
+	if (intel_port_is_tc(dev_priv, intel_dig_port->base.port)) {
+		intel_connector->update_prepare =
+			intel_ddi_connector_update_prepare;
+		intel_connector->update_complete =
+			intel_ddi_connector_update_complete;
+	}
 	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
 	intel_connector->mst_port = intel_dp;
 	intel_connector->port = port;
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index ce08a2eee55f..ce397b69b1d6 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -1939,7 +1939,9 @@  struct intel_dpll_mgr {
 			  struct intel_encoder *encoder);
 	void (*put_dplls)(struct intel_atomic_state *state,
 			  struct intel_crtc *crtc);
-
+	void (*update_active_dpll)(struct intel_atomic_state *state,
+				   struct intel_crtc *crtc,
+				   struct intel_encoder *encoder);
 	void (*dump_hw_state)(struct drm_i915_private *dev_priv,
 			      const struct intel_dpll_hw_state *hw_state);
 };
@@ -3401,6 +3403,7 @@  static const struct intel_dpll_mgr icl_pll_mgr = {
 	.dpll_info = icl_plls,
 	.get_dplls = icl_get_dplls,
 	.put_dplls = icl_put_dplls,
+	.update_active_dpll = icl_update_active_dpll,
 	.dump_hw_state = icl_dump_hw_state,
 };
 
@@ -3525,6 +3528,29 @@  void intel_release_shared_dplls(struct intel_atomic_state *state,
 	dpll_mgr->put_dplls(state, crtc);
 }
 
+/**
+ * intel_update_active_dpll - update the active DPLL for a CRTC/encoder
+ * @state: atomic state
+ * @crtc: the CRTC for which to update the active DPLL
+ * @encoder: encoder determining the type of port DPLL
+ *
+ * Update the active DPLL for the given @crtc/@encoder in @crtc's atomic state,
+ * from the port DPLLs reserved previously by intel_reserve_shared_dplls(). The
+ * DPLL selected will be based on the current mode of the encoder's port.
+ */
+void intel_update_active_dpll(struct intel_atomic_state *state,
+			      struct intel_crtc *crtc,
+			      struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr;
+
+	if (WARN_ON(!dpll_mgr))
+		return;
+
+	dpll_mgr->update_active_dpll(state, crtc, encoder);
+}
+
 /**
  * intel_shared_dpll_dump_hw_state - write hw_state to dmesg
  * @dev_priv: i915 drm device
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 3bea81bde343..5817faa129d5 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -346,6 +346,9 @@  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_update_active_dpll(struct intel_atomic_state *state,
+			      struct intel_crtc *crtc,
+			      struct intel_encoder *encoder);
 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 c61955c41976..b96656f1b8d4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -375,6 +375,11 @@  struct intel_connector {
 	 * and active (i.e. dpms ON state). */
 	bool (*get_hw_state)(struct intel_connector *);
 
+	void (*update_prepare)(struct intel_atomic_state *state,
+			       struct intel_connector *connector);
+	void (*update_complete)(struct intel_atomic_state *state,
+				struct intel_connector *connector);
+
 	/* Panel info for eDP and LVDS */
 	struct intel_panel panel;
 
@@ -1234,6 +1239,8 @@  struct intel_digital_port {
 	enum aux_ch aux_ch;
 	enum intel_display_power_domain ddi_io_power_domain;
 	struct mutex tc_lock;
+	intel_wakeref_t tc_lock_wakeref;
+	int tc_link_refcount;
 	bool tc_legacy_port:1;
 	enum tc_port_mode tc_mode;
 
@@ -1485,6 +1492,8 @@  void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 struct drm_display_mode *
 intel_encoder_current_mode(struct intel_encoder *encoder);
+struct intel_digital_port *
+intel_connector_primary_digital_port(struct intel_connector *connector);
 bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port);
 bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
 enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 097bfa504ece..89f09e27b741 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -3092,6 +3092,13 @@  void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 	else
 		intel_connector->get_hw_state = intel_connector_get_hw_state;
 
+	if (intel_port_is_tc(dev_priv, intel_encoder->port)) {
+		intel_connector->update_prepare =
+			intel_ddi_connector_update_prepare;
+		intel_connector->update_complete =
+			intel_ddi_connector_update_complete;
+	}
+
 	intel_hdmi_add_properties(intel_hdmi, connector);
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c
index 4b2f525bc2a6..e79f6ceb26f3 100644
--- a/drivers/gpu/drm/i915/intel_tc.c
+++ b/drivers/gpu/drm/i915/intel_tc.c
@@ -188,21 +188,13 @@  static bool icl_tc_phy_is_in_safe_mode(struct intel_digital_port *dig_port)
  * display, USB, etc. As a result, handshaking through FIA is required around
  * connect and disconnect to cleanly transfer ownership with the controller and
  * set the type-C power state.
- *
- * We could opt to only do the connect flow when we actually try to use the AUX
- * channels or do a modeset, then immediately run the disconnect flow after
- * usage, but there are some implications on this for a dynamic environment:
- * things may go away or change behind our backs. So for now our driver is
- * always trying to acquire ownership of the controller as soon as it gets an
- * interrupt (or polls state and sees a port is connected) and only gives it
- * back when it sees a disconnect. Implementation of a more fine-grained model
- * will require a lot of coordination with user space and thorough testing for
- * the extra possible cases.
  */
-static void icl_tc_phy_connect(struct intel_digital_port *dig_port)
+static void icl_tc_phy_connect(struct intel_digital_port *dig_port,
+			       int required_lanes)
 {
 	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
 	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
+	int max_lanes;
 
 	if (!icl_tc_phy_status_complete(dig_port)) {
 		DRM_DEBUG_KMS("Port %s: PHY not ready\n",
@@ -214,8 +206,9 @@  static void icl_tc_phy_connect(struct intel_digital_port *dig_port)
 	    !WARN_ON(dig_port->tc_legacy_port))
 		goto out_set_tbt_alt_mode;
 
+	max_lanes = intel_tc_port_fia_max_lane_count(dig_port);
 	if (dig_port->tc_legacy_port) {
-		WARN_ON(intel_tc_port_fia_max_lane_count(dig_port) != 4);
+		WARN_ON(max_lanes != 4);
 		dig_port->tc_mode = TC_PORT_LEGACY;
 
 		return;
@@ -231,6 +224,13 @@  static void icl_tc_phy_connect(struct intel_digital_port *dig_port)
 		goto out_set_safe_mode;
 	}
 
+	if (max_lanes < required_lanes) {
+		DRM_DEBUG_KMS("Port %s: PHY max lanes %d < required lanes %d\n",
+			      tc_port_name(dev_priv, tc_port),
+			      max_lanes, required_lanes);
+		goto out_set_safe_mode;
+	}
+
 	dig_port->tc_mode = TC_PORT_DP_ALT;
 
 	return;
@@ -317,7 +317,8 @@  intel_tc_port_get_target_mode(struct intel_digital_port *dig_port)
 					  TC_PORT_TBT_ALT;
 }
 
-static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port)
+static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port,
+				     int required_lanes)
 {
 	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
 	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
@@ -326,7 +327,7 @@  static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port)
 	intel_display_power_flush_work(dev_priv);
 
 	icl_tc_phy_disconnect(dig_port);
-	icl_tc_phy_connect(dig_port);
+	icl_tc_phy_connect(dig_port, required_lanes);
 
 	DRM_DEBUG_KMS("Port %s: TC port mode reset (%s -> %s)\n",
 		      tc_port_name(dev_priv, tc_port),
@@ -334,6 +335,14 @@  static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port)
 		      tc_port_mode_name(dig_port->tc_mode));
 }
 
+static void
+intel_tc_port_link_init_refcount(struct intel_digital_port *dig_port,
+				 int refcount)
+{
+	WARN_ON(dig_port->tc_link_refcount);
+	dig_port->tc_link_refcount = refcount;
+}
+
 void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
 {
 	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
@@ -354,11 +363,13 @@  void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
 			DRM_DEBUG_DRIVER("Port %s: PHY disconnected with %d active link(s)\n",
 					 tc_port_name(dev_priv, tc_port),
 					 active_links);
+		intel_tc_port_link_init_refcount(dig_port, active_links);
+
 		goto out;
 	}
 
 	if (dig_port->tc_legacy_port)
-		icl_tc_phy_connect(dig_port);
+		icl_tc_phy_connect(dig_port, 1);
 
 out:
 	DRM_DEBUG_DRIVER("Port %s: sanitize mode (%s)\n",
@@ -388,27 +399,60 @@  bool intel_tc_port_connected(struct intel_digital_port *dig_port)
 {
 	bool is_connected;
 
-	mutex_lock(&dig_port->tc_lock);
-
-	if (intel_tc_port_needs_reset(dig_port))
-		intel_tc_port_reset_mode(dig_port);
-
+	intel_tc_port_lock(dig_port);
 	is_connected = tc_port_live_status_mask(dig_port) &
 		       BIT(dig_port->tc_mode);
-
-	mutex_unlock(&dig_port->tc_lock);
+	intel_tc_port_unlock(dig_port);
 
 	return is_connected;
 }
 
-void intel_tc_port_lock(struct intel_digital_port *dig_port)
+static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
+				 int required_lanes)
 {
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	intel_wakeref_t wakeref;
+
+	wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
+
 	mutex_lock(&dig_port->tc_lock);
-	/* TODO: reset the TypeC port mode if needed */
+
+	if (!dig_port->tc_link_refcount &&
+	    intel_tc_port_needs_reset(dig_port))
+		intel_tc_port_reset_mode(dig_port, required_lanes);
+
+	WARN_ON(dig_port->tc_lock_wakeref);
+	dig_port->tc_lock_wakeref = wakeref;
+}
+
+void intel_tc_port_lock(struct intel_digital_port *dig_port)
+{
+	__intel_tc_port_lock(dig_port, 1);
 }
 
 void intel_tc_port_unlock(struct intel_digital_port *dig_port)
 {
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	intel_wakeref_t wakeref = fetch_and_zero(&dig_port->tc_lock_wakeref);
+
+	mutex_unlock(&dig_port->tc_lock);
+
+	intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
+				      wakeref);
+}
+
+void intel_tc_port_get_link(struct intel_digital_port *dig_port,
+			    int required_lanes)
+{
+	__intel_tc_port_lock(dig_port, required_lanes);
+	dig_port->tc_link_refcount++;
+	intel_tc_port_unlock(dig_port);
+}
+
+void intel_tc_port_put_link(struct intel_digital_port *dig_port)
+{
+	mutex_lock(&dig_port->tc_lock);
+	dig_port->tc_link_refcount--;
 	mutex_unlock(&dig_port->tc_lock);
 }
 
@@ -417,4 +461,5 @@  intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
 {
 	mutex_init(&dig_port->tc_lock);
 	dig_port->tc_legacy_port = is_legacy;
+	dig_port->tc_link_refcount = 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h
index 91c6e7459cc9..c1870acf6516 100644
--- a/drivers/gpu/drm/i915/intel_tc.h
+++ b/drivers/gpu/drm/i915/intel_tc.h
@@ -14,6 +14,9 @@  int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
 void intel_tc_port_sanitize(struct intel_digital_port *dig_port);
 void intel_tc_port_lock(struct intel_digital_port *dig_port);
 void intel_tc_port_unlock(struct intel_digital_port *dig_port);
+void intel_tc_port_get_link(struct intel_digital_port *dig_port,
+			    int required_lanes);
+void intel_tc_port_put_link(struct intel_digital_port *dig_port);
 
 void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy);
 

Comments

On Tue, Jun 04, 2019 at 05:58:23PM +0300, Imre Deak wrote:
> The TypeC port mode needs to stay fixed whenever the port is active. Do
> that by introducing a tc_link_refcount to account for active ports,
> avoiding changing the port mode if a reference is held.
> 
> During the modeset commit phase we also have to reset the port mode and
> update the active PLL reflecting the new port mode. We can do this only
> once the port and its old PLL has been already disabled. Add the new
> connector_update_prepare/complete hooks that are called around the whole
> enabling sequence. The TypeC specific hooks of these will reset the port
> mode, update the active PLL if the port will be active and ensure that
> the port mode will stay fixed for the duration of the whole enabling
> sequence by holding a tc_link_refcount.
> 
> During the port enabling, the pre_pll_enable/post_pll_disable hooks will
> take/release a tc_link_refcount to ensure the port mode stays fixed
> while the port is active.
> 
> Changing the port mode should also be avoided during connector detection
> and AUX transfers if the port is active, we'll do that by checking the
> port's tc_link_refcount.
> 
> When resetting the port mode we also have to take into account the
> maximum lanes provided by the FIA. It's guaranteed to be 4 in TBT-alt
> and legacy modes, but there may be less lanes available in DP-alt mode,
> in which case we have to fall back to TBT-alt mode.
> 
> While at it also update icl_tc_phy_connect()'s code comment, reflecting
> the current way of switching the port mode.
> 
> 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_ddi.c      | 46 +++++++++++--
>  drivers/gpu/drm/i915/intel_ddi.h      |  7 ++
>  drivers/gpu/drm/i915/intel_display.c  | 90 +++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_dp.c       |  7 ++
>  drivers/gpu/drm/i915/intel_dp_mst.c   |  6 ++
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 28 +++++++-
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  3 +
>  drivers/gpu/drm/i915/intel_drv.h      |  9 +++
>  drivers/gpu/drm/i915/intel_hdmi.c     |  7 ++
>  drivers/gpu/drm/i915/intel_tc.c       | 93 ++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_tc.h       |  3 +
>  11 files changed, 269 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ad2f7bb2f50b..138950941246 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3610,6 +3610,38 @@ static void intel_ddi_set_fia_lane_count(struct intel_encoder *encoder,
>  	I915_WRITE(PORT_TX_DFLEXDPMLE1, val);
>  }
>  
> +void
> +intel_ddi_connector_update_prepare(struct intel_atomic_state *state,
> +				   struct intel_connector *connector)
> +{
> +	struct drm_connector_state *conn_state =
> +		drm_atomic_get_new_connector_state(&state->base,
> +						   &connector->base);
> +	struct intel_crtc *crtc = conn_state->crtc ?
> +		to_intel_crtc(conn_state->crtc) : NULL;
> +	struct intel_crtc_state *crtc_state =
> +		crtc ? intel_atomic_get_new_crtc_state(state, crtc) : NULL;
> +	struct intel_digital_port *primary_port =
> +		intel_connector_primary_digital_port(connector);
> +	int required_lanes = crtc_state ? crtc_state->lane_count : 1;
> +
> +	WARN_ON(crtc && crtc->active);
> +
> +	intel_tc_port_get_link(primary_port, required_lanes);
> +	if (crtc_state && crtc_state->base.active)
> +		intel_update_active_dpll(state, crtc, &primary_port->base);
> +}

Having this as a connector hook feels a bit strange. I guess moving it
to encoder level would feel more in line with the rest of the hooks, and
would avoid having that intel_connector_primary_digital_port() thing
because then the mst code could just pass in the right thing itself.
Hmm. Or maybe that wouldn't actually work because of
intel_connector->encoder being wonky on MST :(

Although if we just put the hooks on the main encoder and not on the
MST ones it could maybe work. Would still need
intel_connector_primary_digital_port() to dig out the encoder though.

Not sure.

> +
> +void
> +intel_ddi_connector_update_complete(struct intel_atomic_state *state,
> +				    struct intel_connector *connector)
> +{
> +	struct intel_digital_port *primary_port =
> +		intel_connector_primary_digital_port(connector);
> +
> +	intel_tc_port_put_link(primary_port);
> +}
> +
>  static void
>  intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
>  			 const struct intel_crtc_state *crtc_state,
> @@ -3617,10 +3649,13 @@ intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> +	bool is_tc_port = intel_port_is_tc(dev_priv, encoder->port);
>  	enum port port = encoder->port;
>  
> -	if (intel_crtc_has_dp_encoder(crtc_state) ||
> -	    intel_port_is_tc(dev_priv, encoder->port))
> +	if (is_tc_port)
> +		intel_tc_port_get_link(dig_port, crtc_state->lane_count);
> +
> +	if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port)
>  		intel_display_power_get(dev_priv,
>  					intel_ddi_main_link_aux_domain(dig_port));
>  
> @@ -3645,11 +3680,14 @@ intel_ddi_post_pll_disable(struct intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> +	bool is_tc_port = intel_port_is_tc(dev_priv, encoder->port);
>  
> -	if (intel_crtc_has_dp_encoder(crtc_state) ||
> -	    intel_port_is_tc(dev_priv, encoder->port))
> +	if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port)
>  		intel_display_power_put_unchecked(dev_priv,
>  						  intel_ddi_main_link_aux_domain(dig_port));
> +
> +	if (is_tc_port)
> +		intel_tc_port_put_link(dig_port);
>  }
>  
>  void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.h b/drivers/gpu/drm/i915/intel_ddi.h
> index 9cf69175942e..1559d1fbf7bd 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.h
> +++ b/drivers/gpu/drm/i915/intel_ddi.h
> @@ -12,6 +12,7 @@
>  
>  struct drm_connector_state;
>  struct drm_i915_private;
> +struct intel_atomic_state;
>  struct intel_connector;
>  struct intel_crtc;
>  struct intel_crtc_state;
> @@ -35,6 +36,12 @@ void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp);
>  bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
>  void intel_ddi_get_config(struct intel_encoder *encoder,
>  			  struct intel_crtc_state *pipe_config);
> +void
> +intel_ddi_connector_update_prepare(struct intel_atomic_state *state,
> +				   struct intel_connector *connector);
> +void
> +intel_ddi_connector_update_complete(struct intel_atomic_state *state,
> +				    struct intel_connector *connector);
>  void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state *crtc_state,
>  				    bool state);
>  void intel_ddi_compute_min_voltage_level(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f86b5b848cbc..2c65587d1622 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -520,6 +520,20 @@ needs_modeset(const struct drm_crtc_state *state)
>  	return drm_atomic_crtc_needs_modeset(state);
>  }
>  
> +static bool
> +intel_connector_needs_modeset(struct intel_atomic_state *state,
> +			      const struct drm_connector_state *old_conn_state,
> +			      const struct drm_connector_state *new_conn_state)
> +{
> +	if (new_conn_state->crtc != old_conn_state->crtc ||
> +	    (new_conn_state->crtc &&
> +	     needs_modeset(drm_atomic_get_new_crtc_state(&state->base,
> +							 new_conn_state->crtc))))
> +		return true;
> +
> +	return false;

Pointless if statement. Or is there more coming here?

> +}
> +
>  /*
>   * Platform specific helpers to calculate the port PLL loopback- (clock.m),
>   * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> @@ -6032,6 +6046,52 @@ static void intel_crtc_disable_planes(struct intel_atomic_state *state,
>  	intel_frontbuffer_flip(dev_priv, fb_bits);
>  }
>  
> +static void intel_connectors_update_prepare(struct intel_atomic_state *state)
> +{
> +	struct drm_connector_state *old_conn_state;
> +	struct drm_connector_state *new_conn_state;
> +	struct drm_connector *conn;
> +	int i;
> +
> +	for_each_oldnew_connector_in_state(&state->base, conn,
> +					   old_conn_state, new_conn_state, i) {
> +		struct intel_connector *connector = to_intel_connector(conn);
> +
> +		if (!connector->update_prepare)
> +			continue;
> +
> +		if (!intel_connector_needs_modeset(state,
> +						   old_conn_state,
> +						   new_conn_state))
> +			continue;
> +
> +		connector->update_prepare(state, connector);
> +	}
> +}
> +
> +static void intel_connectors_update_complete(struct intel_atomic_state *state)
> +{
> +	struct drm_connector_state *old_conn_state;
> +	struct drm_connector_state *new_conn_state;
> +	struct drm_connector *conn;
> +	int i;
> +
> +	for_each_oldnew_connector_in_state(&state->base, conn,
> +					   old_conn_state, new_conn_state, i) {
> +		struct intel_connector *connector = to_intel_connector(conn);
> +
> +		if (!connector->update_complete)
> +			continue;
> +
> +		if (!intel_connector_needs_modeset(state,
> +						   old_conn_state,
> +						   new_conn_state))
> +			continue;
> +
> +		connector->update_complete(state, connector);
> +	}
> +}
> +
>  static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
>  					  struct intel_crtc_state *crtc_state,
>  					  struct drm_atomic_state *old_state)
> @@ -6556,6 +6616,28 @@ static void i9xx_pfit_enable(const struct intel_crtc_state *crtc_state)
>  	I915_WRITE(BCLRPAT(crtc->pipe), 0);
>  }
>  
> +/*
> + * intel_connector_primary_digital_port - get the primary port for a connector
> + * @connector: connector for which to return the port
> + *
> + * Returns the primary digital port for a DP MST, the single digital port for
> + * DP SST and HDMI and NULL for all other connector types.
> + */
> +struct intel_digital_port *
> +intel_connector_primary_digital_port(struct intel_connector *connector)
> +{
> +	struct intel_encoder *encoder;
> +
> +	if (connector->mst_port)
> +		return dp_to_dig_port(connector->mst_port);
> +
> +	encoder = intel_attached_encoder(&connector->base);
> +	if (WARN_ON(!encoder))
> +		return NULL;
> +
> +	return enc_to_dig_port(&encoder->base);
> +}
> +
>  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port)
>  {
>  	if (port == PORT_NONE)
> @@ -13805,14 +13887,20 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		}
>  	}
>  
> +	if (intel_state->modeset)
> +		intel_connectors_update_prepare(intel_state);
> +
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	dev_priv->display.update_crtcs(state);
>  
> -	if (intel_state->modeset)
> +	if (intel_state->modeset) {
> +		intel_connectors_update_complete(intel_state);
> +
>  		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
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b984410f41a4..2f63476e3cf2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -7195,6 +7195,13 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	else
>  		intel_connector->get_hw_state = intel_connector_get_hw_state;
>  
> +	if (intel_port_is_tc(dev_priv, intel_encoder->port)) {
> +		intel_connector->update_prepare =
> +			intel_ddi_connector_update_prepare;
> +		intel_connector->update_complete =
> +			intel_ddi_connector_update_complete;
> +	}
> +
>  	/* init MST on ports that can support it */
>  	if (HAS_DP_MST(dev_priv) && !intel_dp_is_edp(intel_dp) &&
>  	    (port == PORT_B || port == PORT_C ||
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 0caf645fbbb8..9d5b048b9c96 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -505,6 +505,12 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>  	if (!intel_connector)
>  		return NULL;
>  
> +	if (intel_port_is_tc(dev_priv, intel_dig_port->base.port)) {
> +		intel_connector->update_prepare =
> +			intel_ddi_connector_update_prepare;
> +		intel_connector->update_complete =
> +			intel_ddi_connector_update_complete;
> +	}
>  	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
>  	intel_connector->mst_port = intel_dp;
>  	intel_connector->port = port;
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index ce08a2eee55f..ce397b69b1d6 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -1939,7 +1939,9 @@ struct intel_dpll_mgr {
>  			  struct intel_encoder *encoder);
>  	void (*put_dplls)(struct intel_atomic_state *state,
>  			  struct intel_crtc *crtc);
> -
> +	void (*update_active_dpll)(struct intel_atomic_state *state,
> +				   struct intel_crtc *crtc,
> +				   struct intel_encoder *encoder);
>  	void (*dump_hw_state)(struct drm_i915_private *dev_priv,
>  			      const struct intel_dpll_hw_state *hw_state);
>  };
> @@ -3401,6 +3403,7 @@ static const struct intel_dpll_mgr icl_pll_mgr = {
>  	.dpll_info = icl_plls,
>  	.get_dplls = icl_get_dplls,
>  	.put_dplls = icl_put_dplls,
> +	.update_active_dpll = icl_update_active_dpll,
>  	.dump_hw_state = icl_dump_hw_state,
>  };
>  
> @@ -3525,6 +3528,29 @@ void intel_release_shared_dplls(struct intel_atomic_state *state,
>  	dpll_mgr->put_dplls(state, crtc);
>  }
>  
> +/**
> + * intel_update_active_dpll - update the active DPLL for a CRTC/encoder
> + * @state: atomic state
> + * @crtc: the CRTC for which to update the active DPLL
> + * @encoder: encoder determining the type of port DPLL
> + *
> + * Update the active DPLL for the given @crtc/@encoder in @crtc's atomic state,
> + * from the port DPLLs reserved previously by intel_reserve_shared_dplls(). The
> + * DPLL selected will be based on the current mode of the encoder's port.
> + */
> +void intel_update_active_dpll(struct intel_atomic_state *state,
> +			      struct intel_crtc *crtc,
> +			      struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr;
> +
> +	if (WARN_ON(!dpll_mgr))
> +		return;
> +
> +	dpll_mgr->update_active_dpll(state, crtc, encoder);
> +}
> +
>  /**
>   * intel_shared_dpll_dump_hw_state - write hw_state to dmesg
>   * @dev_priv: i915 drm device
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 3bea81bde343..5817faa129d5 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -346,6 +346,9 @@ 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_update_active_dpll(struct intel_atomic_state *state,
> +			      struct intel_crtc *crtc,
> +			      struct intel_encoder *encoder);
>  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 c61955c41976..b96656f1b8d4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -375,6 +375,11 @@ struct intel_connector {
>  	 * and active (i.e. dpms ON state). */
>  	bool (*get_hw_state)(struct intel_connector *);
>  
> +	void (*update_prepare)(struct intel_atomic_state *state,
> +			       struct intel_connector *connector);
> +	void (*update_complete)(struct intel_atomic_state *state,
> +				struct intel_connector *connector);
> +
>  	/* Panel info for eDP and LVDS */
>  	struct intel_panel panel;
>  
> @@ -1234,6 +1239,8 @@ struct intel_digital_port {
>  	enum aux_ch aux_ch;
>  	enum intel_display_power_domain ddi_io_power_domain;
>  	struct mutex tc_lock;
> +	intel_wakeref_t tc_lock_wakeref;
> +	int tc_link_refcount;
>  	bool tc_legacy_port:1;
>  	enum tc_port_mode tc_mode;
>  
> @@ -1485,6 +1492,8 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  struct drm_display_mode *
>  intel_encoder_current_mode(struct intel_encoder *encoder);
> +struct intel_digital_port *
> +intel_connector_primary_digital_port(struct intel_connector *connector);
>  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port);
>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 097bfa504ece..89f09e27b741 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -3092,6 +3092,13 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  	else
>  		intel_connector->get_hw_state = intel_connector_get_hw_state;
>  
> +	if (intel_port_is_tc(dev_priv, intel_encoder->port)) {
> +		intel_connector->update_prepare =
> +			intel_ddi_connector_update_prepare;
> +		intel_connector->update_complete =
> +			intel_ddi_connector_update_complete;
> +	}
> +
>  	intel_hdmi_add_properties(intel_hdmi, connector);
>  
>  	intel_connector_attach_encoder(intel_connector, intel_encoder);
> diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c
> index 4b2f525bc2a6..e79f6ceb26f3 100644
> --- a/drivers/gpu/drm/i915/intel_tc.c
> +++ b/drivers/gpu/drm/i915/intel_tc.c
> @@ -188,21 +188,13 @@ static bool icl_tc_phy_is_in_safe_mode(struct intel_digital_port *dig_port)
>   * display, USB, etc. As a result, handshaking through FIA is required around
>   * connect and disconnect to cleanly transfer ownership with the controller and
>   * set the type-C power state.
> - *
> - * We could opt to only do the connect flow when we actually try to use the AUX
> - * channels or do a modeset, then immediately run the disconnect flow after
> - * usage, but there are some implications on this for a dynamic environment:
> - * things may go away or change behind our backs. So for now our driver is
> - * always trying to acquire ownership of the controller as soon as it gets an
> - * interrupt (or polls state and sees a port is connected) and only gives it
> - * back when it sees a disconnect. Implementation of a more fine-grained model
> - * will require a lot of coordination with user space and thorough testing for
> - * the extra possible cases.
>   */
> -static void icl_tc_phy_connect(struct intel_digital_port *dig_port)
> +static void icl_tc_phy_connect(struct intel_digital_port *dig_port,
> +			       int required_lanes)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>  	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> +	int max_lanes;
>  
>  	if (!icl_tc_phy_status_complete(dig_port)) {
>  		DRM_DEBUG_KMS("Port %s: PHY not ready\n",
> @@ -214,8 +206,9 @@ static void icl_tc_phy_connect(struct intel_digital_port *dig_port)
>  	    !WARN_ON(dig_port->tc_legacy_port))
>  		goto out_set_tbt_alt_mode;
>  
> +	max_lanes = intel_tc_port_fia_max_lane_count(dig_port);
>  	if (dig_port->tc_legacy_port) {
> -		WARN_ON(intel_tc_port_fia_max_lane_count(dig_port) != 4);
> +		WARN_ON(max_lanes != 4);
>  		dig_port->tc_mode = TC_PORT_LEGACY;
>  
>  		return;
> @@ -231,6 +224,13 @@ static void icl_tc_phy_connect(struct intel_digital_port *dig_port)
>  		goto out_set_safe_mode;
>  	}
>  
> +	if (max_lanes < required_lanes) {
> +		DRM_DEBUG_KMS("Port %s: PHY max lanes %d < required lanes %d\n",
> +			      tc_port_name(dev_priv, tc_port),
> +			      max_lanes, required_lanes);
> +		goto out_set_safe_mode;
> +	}
> +
>  	dig_port->tc_mode = TC_PORT_DP_ALT;
>  
>  	return;
> @@ -317,7 +317,8 @@ intel_tc_port_get_target_mode(struct intel_digital_port *dig_port)
>  					  TC_PORT_TBT_ALT;
>  }
>  
> -static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port)
> +static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port,
> +				     int required_lanes)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>  	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> @@ -326,7 +327,7 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port)
>  	intel_display_power_flush_work(dev_priv);
>  
>  	icl_tc_phy_disconnect(dig_port);
> -	icl_tc_phy_connect(dig_port);
> +	icl_tc_phy_connect(dig_port, required_lanes);
>  
>  	DRM_DEBUG_KMS("Port %s: TC port mode reset (%s -> %s)\n",
>  		      tc_port_name(dev_priv, tc_port),
> @@ -334,6 +335,14 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port)
>  		      tc_port_mode_name(dig_port->tc_mode));
>  }
>  
> +static void
> +intel_tc_port_link_init_refcount(struct intel_digital_port *dig_port,
> +				 int refcount)
> +{
> +	WARN_ON(dig_port->tc_link_refcount);
> +	dig_port->tc_link_refcount = refcount;
> +}
> +
>  void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> @@ -354,11 +363,13 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
>  			DRM_DEBUG_DRIVER("Port %s: PHY disconnected with %d active link(s)\n",
>  					 tc_port_name(dev_priv, tc_port),
>  					 active_links);
> +		intel_tc_port_link_init_refcount(dig_port, active_links);
> +
>  		goto out;
>  	}
>  
>  	if (dig_port->tc_legacy_port)
> -		icl_tc_phy_connect(dig_port);
> +		icl_tc_phy_connect(dig_port, 1);
>  
>  out:
>  	DRM_DEBUG_DRIVER("Port %s: sanitize mode (%s)\n",
> @@ -388,27 +399,60 @@ bool intel_tc_port_connected(struct intel_digital_port *dig_port)
>  {
>  	bool is_connected;
>  
> -	mutex_lock(&dig_port->tc_lock);
> -
> -	if (intel_tc_port_needs_reset(dig_port))
> -		intel_tc_port_reset_mode(dig_port);
> -
> +	intel_tc_port_lock(dig_port);
>  	is_connected = tc_port_live_status_mask(dig_port) &
>  		       BIT(dig_port->tc_mode);
> -
> -	mutex_unlock(&dig_port->tc_lock);
> +	intel_tc_port_unlock(dig_port);
>  
>  	return is_connected;
>  }
>  
> -void intel_tc_port_lock(struct intel_digital_port *dig_port)
> +static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> +				 int required_lanes)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	intel_wakeref_t wakeref;
> +
> +	wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> +
>  	mutex_lock(&dig_port->tc_lock);
> -	/* TODO: reset the TypeC port mode if needed */
> +
> +	if (!dig_port->tc_link_refcount &&
> +	    intel_tc_port_needs_reset(dig_port))
> +		intel_tc_port_reset_mode(dig_port, required_lanes);
> +
> +	WARN_ON(dig_port->tc_lock_wakeref);
> +	dig_port->tc_lock_wakeref = wakeref;
> +}
> +
> +void intel_tc_port_lock(struct intel_digital_port *dig_port)
> +{
> +	__intel_tc_port_lock(dig_port, 1);
>  }
>  
>  void intel_tc_port_unlock(struct intel_digital_port *dig_port)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	intel_wakeref_t wakeref = fetch_and_zero(&dig_port->tc_lock_wakeref);
> +
> +	mutex_unlock(&dig_port->tc_lock);
> +
> +	intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> +				      wakeref);
> +}
> +
> +void intel_tc_port_get_link(struct intel_digital_port *dig_port,
> +			    int required_lanes)
> +{
> +	__intel_tc_port_lock(dig_port, required_lanes);
> +	dig_port->tc_link_refcount++;
> +	intel_tc_port_unlock(dig_port);
> +}
> +
> +void intel_tc_port_put_link(struct intel_digital_port *dig_port)
> +{
> +	mutex_lock(&dig_port->tc_lock);
> +	dig_port->tc_link_refcount--;
>  	mutex_unlock(&dig_port->tc_lock);
>  }
>  
> @@ -417,4 +461,5 @@ intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
>  {
>  	mutex_init(&dig_port->tc_lock);
>  	dig_port->tc_legacy_port = is_legacy;
> +	dig_port->tc_link_refcount = 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h
> index 91c6e7459cc9..c1870acf6516 100644
> --- a/drivers/gpu/drm/i915/intel_tc.h
> +++ b/drivers/gpu/drm/i915/intel_tc.h
> @@ -14,6 +14,9 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
>  void intel_tc_port_sanitize(struct intel_digital_port *dig_port);
>  void intel_tc_port_lock(struct intel_digital_port *dig_port);
>  void intel_tc_port_unlock(struct intel_digital_port *dig_port);
> +void intel_tc_port_get_link(struct intel_digital_port *dig_port,
> +			    int required_lanes);
> +void intel_tc_port_put_link(struct intel_digital_port *dig_port);
>  
>  void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy);
>  
> -- 
> 2.17.1
On Wed, Jun 19, 2019 at 03:58:46PM +0300, Ville Syrjälä wrote:
> On Tue, Jun 04, 2019 at 05:58:23PM +0300, Imre Deak wrote:
> > The TypeC port mode needs to stay fixed whenever the port is active. Do
> > that by introducing a tc_link_refcount to account for active ports,
> > avoiding changing the port mode if a reference is held.
> > 
> > During the modeset commit phase we also have to reset the port mode and
> > update the active PLL reflecting the new port mode. We can do this only
> > once the port and its old PLL has been already disabled. Add the new
> > connector_update_prepare/complete hooks that are called around the whole
> > enabling sequence. The TypeC specific hooks of these will reset the port
> > mode, update the active PLL if the port will be active and ensure that
> > the port mode will stay fixed for the duration of the whole enabling
> > sequence by holding a tc_link_refcount.
> > 
> > During the port enabling, the pre_pll_enable/post_pll_disable hooks will
> > take/release a tc_link_refcount to ensure the port mode stays fixed
> > while the port is active.
> > 
> > Changing the port mode should also be avoided during connector detection
> > and AUX transfers if the port is active, we'll do that by checking the
> > port's tc_link_refcount.
> > 
> > When resetting the port mode we also have to take into account the
> > maximum lanes provided by the FIA. It's guaranteed to be 4 in TBT-alt
> > and legacy modes, but there may be less lanes available in DP-alt mode,
> > in which case we have to fall back to TBT-alt mode.
> > 
> > While at it also update icl_tc_phy_connect()'s code comment, reflecting
> > the current way of switching the port mode.
> > 
> > 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_ddi.c      | 46 +++++++++++--
> >  drivers/gpu/drm/i915/intel_ddi.h      |  7 ++
> >  drivers/gpu/drm/i915/intel_display.c  | 90 +++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_dp.c       |  7 ++
> >  drivers/gpu/drm/i915/intel_dp_mst.c   |  6 ++
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 28 +++++++-
> >  drivers/gpu/drm/i915/intel_dpll_mgr.h |  3 +
> >  drivers/gpu/drm/i915/intel_drv.h      |  9 +++
> >  drivers/gpu/drm/i915/intel_hdmi.c     |  7 ++
> >  drivers/gpu/drm/i915/intel_tc.c       | 93 ++++++++++++++++++++-------
> >  drivers/gpu/drm/i915/intel_tc.h       |  3 +
> >  11 files changed, 269 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index ad2f7bb2f50b..138950941246 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3610,6 +3610,38 @@ static void intel_ddi_set_fia_lane_count(struct intel_encoder *encoder,
> >  	I915_WRITE(PORT_TX_DFLEXDPMLE1, val);
> >  }
> >  
> > +void
> > +intel_ddi_connector_update_prepare(struct intel_atomic_state *state,
> > +				   struct intel_connector *connector)
> > +{
> > +	struct drm_connector_state *conn_state =
> > +		drm_atomic_get_new_connector_state(&state->base,
> > +						   &connector->base);
> > +	struct intel_crtc *crtc = conn_state->crtc ?
> > +		to_intel_crtc(conn_state->crtc) : NULL;
> > +	struct intel_crtc_state *crtc_state =
> > +		crtc ? intel_atomic_get_new_crtc_state(state, crtc) : NULL;
> > +	struct intel_digital_port *primary_port =
> > +		intel_connector_primary_digital_port(connector);
> > +	int required_lanes = crtc_state ? crtc_state->lane_count : 1;
> > +
> > +	WARN_ON(crtc && crtc->active);
> > +
> > +	intel_tc_port_get_link(primary_port, required_lanes);
> > +	if (crtc_state && crtc_state->base.active)
> > +		intel_update_active_dpll(state, crtc, &primary_port->base);
> > +}
> 
> Having this as a connector hook feels a bit strange. I guess moving it
> to encoder level would feel more in line with the rest of the hooks, and
> would avoid having that intel_connector_primary_digital_port() thing
> because then the mst code could just pass in the right thing itself.
> Hmm. Or maybe that wouldn't actually work because of
> intel_connector->encoder being wonky on MST :(

Yep, it took a while to wrap my mind around the tracking of the MST
connector->encoder mapping. The only certainty here is that we have a
fixed encoder for all non-MST connectors and the primary encoder for MST
connectors. Things would simplify a lot even by keeping the above hooks
connector specific, but adding separate MST versions of them to deal
with the connector->primary encoder mapping, which would remove the need
for intel_connector_primary_digital_port().

I see now though, that it makes more sense to add these hooks to the
encoder, which also simplifies things a lot, so I chose that instead. 

> 
> Although if we just put the hooks on the main encoder and not on the
> MST ones it could maybe work. Would still need
> intel_connector_primary_digital_port() to dig out the encoder though.

Yep, this turned out to be simplifying things a lot (by adding a new
intel_connector_primary_encoder() helper instead):
https://github.com/ideak/linux/commit/49dc011132

This way it's enough to assign the hooks to the common DDI encoder,
instead of assigning them to each type of connectors used with a DDI
encoder.

Will post a v2 patchset with all the comments addressed after some
testing, an updated version is at:
https://github.com/ideak/linux/commits/typec-mode-switch

> 
> Not sure.
> 
> > +
> > +void
> > +intel_ddi_connector_update_complete(struct intel_atomic_state *state,
> > +				    struct intel_connector *connector)
> > +{
> > +	struct intel_digital_port *primary_port =
> > +		intel_connector_primary_digital_port(connector);
> > +
> > +	intel_tc_port_put_link(primary_port);
> > +}
> > +
> >  static void
> >  intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
> >  			 const struct intel_crtc_state *crtc_state,
> > @@ -3617,10 +3649,13 @@ intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> > +	bool is_tc_port = intel_port_is_tc(dev_priv, encoder->port);
> >  	enum port port = encoder->port;
> >  
> > -	if (intel_crtc_has_dp_encoder(crtc_state) ||
> > -	    intel_port_is_tc(dev_priv, encoder->port))
> > +	if (is_tc_port)
> > +		intel_tc_port_get_link(dig_port, crtc_state->lane_count);
> > +
> > +	if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port)
> >  		intel_display_power_get(dev_priv,
> >  					intel_ddi_main_link_aux_domain(dig_port));
> >  
> > @@ -3645,11 +3680,14 @@ intel_ddi_post_pll_disable(struct intel_encoder *encoder,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> > +	bool is_tc_port = intel_port_is_tc(dev_priv, encoder->port);
> >  
> > -	if (intel_crtc_has_dp_encoder(crtc_state) ||
> > -	    intel_port_is_tc(dev_priv, encoder->port))
> > +	if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port)
> >  		intel_display_power_put_unchecked(dev_priv,
> >  						  intel_ddi_main_link_aux_domain(dig_port));
> > +
> > +	if (is_tc_port)
> > +		intel_tc_port_put_link(dig_port);
> >  }
> >  
> >  void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.h b/drivers/gpu/drm/i915/intel_ddi.h
> > index 9cf69175942e..1559d1fbf7bd 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.h
> > +++ b/drivers/gpu/drm/i915/intel_ddi.h
> > @@ -12,6 +12,7 @@
> >  
> >  struct drm_connector_state;
> >  struct drm_i915_private;
> > +struct intel_atomic_state;
> >  struct intel_connector;
> >  struct intel_crtc;
> >  struct intel_crtc_state;
> > @@ -35,6 +36,12 @@ void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp);
> >  bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
> >  void intel_ddi_get_config(struct intel_encoder *encoder,
> >  			  struct intel_crtc_state *pipe_config);
> > +void
> > +intel_ddi_connector_update_prepare(struct intel_atomic_state *state,
> > +				   struct intel_connector *connector);
> > +void
> > +intel_ddi_connector_update_complete(struct intel_atomic_state *state,
> > +				    struct intel_connector *connector);
> >  void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state *crtc_state,
> >  				    bool state);
> >  void intel_ddi_compute_min_voltage_level(struct drm_i915_private *dev_priv,
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f86b5b848cbc..2c65587d1622 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -520,6 +520,20 @@ needs_modeset(const struct drm_crtc_state *state)
> >  	return drm_atomic_crtc_needs_modeset(state);
> >  }
> >  
> > +static bool
> > +intel_connector_needs_modeset(struct intel_atomic_state *state,
> > +			      const struct drm_connector_state *old_conn_state,
> > +			      const struct drm_connector_state *new_conn_state)
> > +{
> > +	if (new_conn_state->crtc != old_conn_state->crtc ||
> > +	    (new_conn_state->crtc &&
> > +	     needs_modeset(drm_atomic_get_new_crtc_state(&state->base,
> > +							 new_conn_state->crtc))))
> > +		return true;
> > +
> > +	return false;
> 
> Pointless if statement. Or is there more coming here?

Nope, it can be simplified.

> 
> > +}
> > +
> >  /*
> >   * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> >   * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> > @@ -6032,6 +6046,52 @@ static void intel_crtc_disable_planes(struct intel_atomic_state *state,
> >  	intel_frontbuffer_flip(dev_priv, fb_bits);
> >  }
> >  
> > +static void intel_connectors_update_prepare(struct intel_atomic_state *state)
> > +{
> > +	struct drm_connector_state *old_conn_state;
> > +	struct drm_connector_state *new_conn_state;
> > +	struct drm_connector *conn;
> > +	int i;
> > +
> > +	for_each_oldnew_connector_in_state(&state->base, conn,
> > +					   old_conn_state, new_conn_state, i) {
> > +		struct intel_connector *connector = to_intel_connector(conn);
> > +
> > +		if (!connector->update_prepare)
> > +			continue;
> > +
> > +		if (!intel_connector_needs_modeset(state,
> > +						   old_conn_state,
> > +						   new_conn_state))
> > +			continue;
> > +
> > +		connector->update_prepare(state, connector);
> > +	}
> > +}
> > +
> > +static void intel_connectors_update_complete(struct intel_atomic_state *state)
> > +{
> > +	struct drm_connector_state *old_conn_state;
> > +	struct drm_connector_state *new_conn_state;
> > +	struct drm_connector *conn;
> > +	int i;
> > +
> > +	for_each_oldnew_connector_in_state(&state->base, conn,
> > +					   old_conn_state, new_conn_state, i) {
> > +		struct intel_connector *connector = to_intel_connector(conn);
> > +
> > +		if (!connector->update_complete)
> > +			continue;
> > +
> > +		if (!intel_connector_needs_modeset(state,
> > +						   old_conn_state,
> > +						   new_conn_state))
> > +			continue;
> > +
> > +		connector->update_complete(state, connector);
> > +	}
> > +}
> > +
> >  static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
> >  					  struct intel_crtc_state *crtc_state,
> >  					  struct drm_atomic_state *old_state)
> > @@ -6556,6 +6616,28 @@ static void i9xx_pfit_enable(const struct intel_crtc_state *crtc_state)
> >  	I915_WRITE(BCLRPAT(crtc->pipe), 0);
> >  }
> >  
> > +/*
> > + * intel_connector_primary_digital_port - get the primary port for a connector
> > + * @connector: connector for which to return the port
> > + *
> > + * Returns the primary digital port for a DP MST, the single digital port for
> > + * DP SST and HDMI and NULL for all other connector types.
> > + */
> > +struct intel_digital_port *
> > +intel_connector_primary_digital_port(struct intel_connector *connector)
> > +{
> > +	struct intel_encoder *encoder;
> > +
> > +	if (connector->mst_port)
> > +		return dp_to_dig_port(connector->mst_port);
> > +
> > +	encoder = intel_attached_encoder(&connector->base);
> > +	if (WARN_ON(!encoder))
> > +		return NULL;
> > +
> > +	return enc_to_dig_port(&encoder->base);
> > +}
> > +
> >  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port)
> >  {
> >  	if (port == PORT_NONE)
> > @@ -13805,14 +13887,20 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >  		}
> >  	}
> >  
> > +	if (intel_state->modeset)
> > +		intel_connectors_update_prepare(intel_state);
> > +
> >  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> >  	dev_priv->display.update_crtcs(state);
> >  
> > -	if (intel_state->modeset)
> > +	if (intel_state->modeset) {
> > +		intel_connectors_update_complete(intel_state);
> > +
> >  		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
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b984410f41a4..2f63476e3cf2 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -7195,6 +7195,13 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >  	else
> >  		intel_connector->get_hw_state = intel_connector_get_hw_state;
> >  
> > +	if (intel_port_is_tc(dev_priv, intel_encoder->port)) {
> > +		intel_connector->update_prepare =
> > +			intel_ddi_connector_update_prepare;
> > +		intel_connector->update_complete =
> > +			intel_ddi_connector_update_complete;
> > +	}
> > +
> >  	/* init MST on ports that can support it */
> >  	if (HAS_DP_MST(dev_priv) && !intel_dp_is_edp(intel_dp) &&
> >  	    (port == PORT_B || port == PORT_C ||
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 0caf645fbbb8..9d5b048b9c96 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -505,6 +505,12 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
> >  	if (!intel_connector)
> >  		return NULL;
> >  
> > +	if (intel_port_is_tc(dev_priv, intel_dig_port->base.port)) {
> > +		intel_connector->update_prepare =
> > +			intel_ddi_connector_update_prepare;
> > +		intel_connector->update_complete =
> > +			intel_ddi_connector_update_complete;
> > +	}
> >  	intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
> >  	intel_connector->mst_port = intel_dp;
> >  	intel_connector->port = port;
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index ce08a2eee55f..ce397b69b1d6 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -1939,7 +1939,9 @@ struct intel_dpll_mgr {
> >  			  struct intel_encoder *encoder);
> >  	void (*put_dplls)(struct intel_atomic_state *state,
> >  			  struct intel_crtc *crtc);
> > -
> > +	void (*update_active_dpll)(struct intel_atomic_state *state,
> > +				   struct intel_crtc *crtc,
> > +				   struct intel_encoder *encoder);
> >  	void (*dump_hw_state)(struct drm_i915_private *dev_priv,
> >  			      const struct intel_dpll_hw_state *hw_state);
> >  };
> > @@ -3401,6 +3403,7 @@ static const struct intel_dpll_mgr icl_pll_mgr = {
> >  	.dpll_info = icl_plls,
> >  	.get_dplls = icl_get_dplls,
> >  	.put_dplls = icl_put_dplls,
> > +	.update_active_dpll = icl_update_active_dpll,
> >  	.dump_hw_state = icl_dump_hw_state,
> >  };
> >  
> > @@ -3525,6 +3528,29 @@ void intel_release_shared_dplls(struct intel_atomic_state *state,
> >  	dpll_mgr->put_dplls(state, crtc);
> >  }
> >  
> > +/**
> > + * intel_update_active_dpll - update the active DPLL for a CRTC/encoder
> > + * @state: atomic state
> > + * @crtc: the CRTC for which to update the active DPLL
> > + * @encoder: encoder determining the type of port DPLL
> > + *
> > + * Update the active DPLL for the given @crtc/@encoder in @crtc's atomic state,
> > + * from the port DPLLs reserved previously by intel_reserve_shared_dplls(). The
> > + * DPLL selected will be based on the current mode of the encoder's port.
> > + */
> > +void intel_update_active_dpll(struct intel_atomic_state *state,
> > +			      struct intel_crtc *crtc,
> > +			      struct intel_encoder *encoder)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr;
> > +
> > +	if (WARN_ON(!dpll_mgr))
> > +		return;
> > +
> > +	dpll_mgr->update_active_dpll(state, crtc, encoder);
> > +}
> > +
> >  /**
> >   * intel_shared_dpll_dump_hw_state - write hw_state to dmesg
> >   * @dev_priv: i915 drm device
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > index 3bea81bde343..5817faa129d5 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > @@ -346,6 +346,9 @@ 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_update_active_dpll(struct intel_atomic_state *state,
> > +			      struct intel_crtc *crtc,
> > +			      struct intel_encoder *encoder);
> >  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 c61955c41976..b96656f1b8d4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -375,6 +375,11 @@ struct intel_connector {
> >  	 * and active (i.e. dpms ON state). */
> >  	bool (*get_hw_state)(struct intel_connector *);
> >  
> > +	void (*update_prepare)(struct intel_atomic_state *state,
> > +			       struct intel_connector *connector);
> > +	void (*update_complete)(struct intel_atomic_state *state,
> > +				struct intel_connector *connector);
> > +
> >  	/* Panel info for eDP and LVDS */
> >  	struct intel_panel panel;
> >  
> > @@ -1234,6 +1239,8 @@ struct intel_digital_port {
> >  	enum aux_ch aux_ch;
> >  	enum intel_display_power_domain ddi_io_power_domain;
> >  	struct mutex tc_lock;
> > +	intel_wakeref_t tc_lock_wakeref;
> > +	int tc_link_refcount;
> >  	bool tc_legacy_port:1;
> >  	enum tc_port_mode tc_mode;
> >  
> > @@ -1485,6 +1492,8 @@ void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
> >  void intel_encoder_destroy(struct drm_encoder *encoder);
> >  struct drm_display_mode *
> >  intel_encoder_current_mode(struct intel_encoder *encoder);
> > +struct intel_digital_port *
> > +intel_connector_primary_digital_port(struct intel_connector *connector);
> >  bool intel_port_is_combophy(struct drm_i915_private *dev_priv, enum port port);
> >  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
> >  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 097bfa504ece..89f09e27b741 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -3092,6 +3092,13 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >  	else
> >  		intel_connector->get_hw_state = intel_connector_get_hw_state;
> >  
> > +	if (intel_port_is_tc(dev_priv, intel_encoder->port)) {
> > +		intel_connector->update_prepare =
> > +			intel_ddi_connector_update_prepare;
> > +		intel_connector->update_complete =
> > +			intel_ddi_connector_update_complete;
> > +	}
> > +
> >  	intel_hdmi_add_properties(intel_hdmi, connector);
> >  
> >  	intel_connector_attach_encoder(intel_connector, intel_encoder);
> > diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c
> > index 4b2f525bc2a6..e79f6ceb26f3 100644
> > --- a/drivers/gpu/drm/i915/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/intel_tc.c
> > @@ -188,21 +188,13 @@ static bool icl_tc_phy_is_in_safe_mode(struct intel_digital_port *dig_port)
> >   * display, USB, etc. As a result, handshaking through FIA is required around
> >   * connect and disconnect to cleanly transfer ownership with the controller and
> >   * set the type-C power state.
> > - *
> > - * We could opt to only do the connect flow when we actually try to use the AUX
> > - * channels or do a modeset, then immediately run the disconnect flow after
> > - * usage, but there are some implications on this for a dynamic environment:
> > - * things may go away or change behind our backs. So for now our driver is
> > - * always trying to acquire ownership of the controller as soon as it gets an
> > - * interrupt (or polls state and sees a port is connected) and only gives it
> > - * back when it sees a disconnect. Implementation of a more fine-grained model
> > - * will require a lot of coordination with user space and thorough testing for
> > - * the extra possible cases.
> >   */
> > -static void icl_tc_phy_connect(struct intel_digital_port *dig_port)
> > +static void icl_tc_phy_connect(struct intel_digital_port *dig_port,
> > +			       int required_lanes)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> >  	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> > +	int max_lanes;
> >  
> >  	if (!icl_tc_phy_status_complete(dig_port)) {
> >  		DRM_DEBUG_KMS("Port %s: PHY not ready\n",
> > @@ -214,8 +206,9 @@ static void icl_tc_phy_connect(struct intel_digital_port *dig_port)
> >  	    !WARN_ON(dig_port->tc_legacy_port))
> >  		goto out_set_tbt_alt_mode;
> >  
> > +	max_lanes = intel_tc_port_fia_max_lane_count(dig_port);
> >  	if (dig_port->tc_legacy_port) {
> > -		WARN_ON(intel_tc_port_fia_max_lane_count(dig_port) != 4);
> > +		WARN_ON(max_lanes != 4);
> >  		dig_port->tc_mode = TC_PORT_LEGACY;
> >  
> >  		return;
> > @@ -231,6 +224,13 @@ static void icl_tc_phy_connect(struct intel_digital_port *dig_port)
> >  		goto out_set_safe_mode;
> >  	}
> >  
> > +	if (max_lanes < required_lanes) {
> > +		DRM_DEBUG_KMS("Port %s: PHY max lanes %d < required lanes %d\n",
> > +			      tc_port_name(dev_priv, tc_port),
> > +			      max_lanes, required_lanes);
> > +		goto out_set_safe_mode;
> > +	}
> > +
> >  	dig_port->tc_mode = TC_PORT_DP_ALT;
> >  
> >  	return;
> > @@ -317,7 +317,8 @@ intel_tc_port_get_target_mode(struct intel_digital_port *dig_port)
> >  					  TC_PORT_TBT_ALT;
> >  }
> >  
> > -static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port)
> > +static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port,
> > +				     int required_lanes)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> >  	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> > @@ -326,7 +327,7 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port)
> >  	intel_display_power_flush_work(dev_priv);
> >  
> >  	icl_tc_phy_disconnect(dig_port);
> > -	icl_tc_phy_connect(dig_port);
> > +	icl_tc_phy_connect(dig_port, required_lanes);
> >  
> >  	DRM_DEBUG_KMS("Port %s: TC port mode reset (%s -> %s)\n",
> >  		      tc_port_name(dev_priv, tc_port),
> > @@ -334,6 +335,14 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port)
> >  		      tc_port_mode_name(dig_port->tc_mode));
> >  }
> >  
> > +static void
> > +intel_tc_port_link_init_refcount(struct intel_digital_port *dig_port,
> > +				 int refcount)
> > +{
> > +	WARN_ON(dig_port->tc_link_refcount);
> > +	dig_port->tc_link_refcount = refcount;
> > +}
> > +
> >  void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > @@ -354,11 +363,13 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> >  			DRM_DEBUG_DRIVER("Port %s: PHY disconnected with %d active link(s)\n",
> >  					 tc_port_name(dev_priv, tc_port),
> >  					 active_links);
> > +		intel_tc_port_link_init_refcount(dig_port, active_links);
> > +
> >  		goto out;
> >  	}
> >  
> >  	if (dig_port->tc_legacy_port)
> > -		icl_tc_phy_connect(dig_port);
> > +		icl_tc_phy_connect(dig_port, 1);
> >  
> >  out:
> >  	DRM_DEBUG_DRIVER("Port %s: sanitize mode (%s)\n",
> > @@ -388,27 +399,60 @@ bool intel_tc_port_connected(struct intel_digital_port *dig_port)
> >  {
> >  	bool is_connected;
> >  
> > -	mutex_lock(&dig_port->tc_lock);
> > -
> > -	if (intel_tc_port_needs_reset(dig_port))
> > -		intel_tc_port_reset_mode(dig_port);
> > -
> > +	intel_tc_port_lock(dig_port);
> >  	is_connected = tc_port_live_status_mask(dig_port) &
> >  		       BIT(dig_port->tc_mode);
> > -
> > -	mutex_unlock(&dig_port->tc_lock);
> > +	intel_tc_port_unlock(dig_port);
> >  
> >  	return is_connected;
> >  }
> >  
> > -void intel_tc_port_lock(struct intel_digital_port *dig_port)
> > +static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> > +				 int required_lanes)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > +	intel_wakeref_t wakeref;
> > +
> > +	wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> > +
> >  	mutex_lock(&dig_port->tc_lock);
> > -	/* TODO: reset the TypeC port mode if needed */
> > +
> > +	if (!dig_port->tc_link_refcount &&
> > +	    intel_tc_port_needs_reset(dig_port))
> > +		intel_tc_port_reset_mode(dig_port, required_lanes);
> > +
> > +	WARN_ON(dig_port->tc_lock_wakeref);
> > +	dig_port->tc_lock_wakeref = wakeref;
> > +}
> > +
> > +void intel_tc_port_lock(struct intel_digital_port *dig_port)
> > +{
> > +	__intel_tc_port_lock(dig_port, 1);
> >  }
> >  
> >  void intel_tc_port_unlock(struct intel_digital_port *dig_port)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > +	intel_wakeref_t wakeref = fetch_and_zero(&dig_port->tc_lock_wakeref);
> > +
> > +	mutex_unlock(&dig_port->tc_lock);
> > +
> > +	intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> > +				      wakeref);
> > +}
> > +
> > +void intel_tc_port_get_link(struct intel_digital_port *dig_port,
> > +			    int required_lanes)
> > +{
> > +	__intel_tc_port_lock(dig_port, required_lanes);
> > +	dig_port->tc_link_refcount++;
> > +	intel_tc_port_unlock(dig_port);
> > +}
> > +
> > +void intel_tc_port_put_link(struct intel_digital_port *dig_port)
> > +{
> > +	mutex_lock(&dig_port->tc_lock);
> > +	dig_port->tc_link_refcount--;
> >  	mutex_unlock(&dig_port->tc_lock);
> >  }
> >  
> > @@ -417,4 +461,5 @@ intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
> >  {
> >  	mutex_init(&dig_port->tc_lock);
> >  	dig_port->tc_legacy_port = is_legacy;
> > +	dig_port->tc_link_refcount = 0;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h
> > index 91c6e7459cc9..c1870acf6516 100644
> > --- a/drivers/gpu/drm/i915/intel_tc.h
> > +++ b/drivers/gpu/drm/i915/intel_tc.h
> > @@ -14,6 +14,9 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
> >  void intel_tc_port_sanitize(struct intel_digital_port *dig_port);
> >  void intel_tc_port_lock(struct intel_digital_port *dig_port);
> >  void intel_tc_port_unlock(struct intel_digital_port *dig_port);
> > +void intel_tc_port_get_link(struct intel_digital_port *dig_port,
> > +			    int required_lanes);
> > +void intel_tc_port_put_link(struct intel_digital_port *dig_port);
> >  
> >  void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy);
> >  
> > -- 
> > 2.17.1
> 
> -- 
> Ville Syrjälä
> Intel