drm/i915: Keep the TypeC port mode fixed when the port is active
authorImre Deak <imre.deak@intel.com>
Fri, 28 Jun 2019 14:36:32 +0000 (17:36 +0300)
committerImre Deak <imre.deak@intel.com>
Mon, 1 Jul 2019 12:02:34 +0000 (15:02 +0300)
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
encoder 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.

v2:
- Add the update_prepare/complete hooks to the encoder instead of the
  connector. (Ville)
- Simplify intel_connector_needs_modeset() by removing redundant if.
  (Ville)
v3:
- Fix sparse warning, marking static functions as such.
v4:
- Rebase on drm-tip.

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>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190628143635.22066-21-imre.deak@intel.com
drivers/gpu/drm/i915/display/intel_ddi.c
drivers/gpu/drm/i915/display/intel_display.c
drivers/gpu/drm/i915/display/intel_dpll_mgr.c
drivers/gpu/drm/i915/display/intel_dpll_mgr.h
drivers/gpu/drm/i915/display/intel_tc.c
drivers/gpu/drm/i915/display/intel_tc.h
drivers/gpu/drm/i915/intel_drv.h

index 0c5bfbd66b196821467723efa53f5852f071893a..404f555126a13b5e3d0cb5fb9466c1d047a0d39e 100644 (file)
@@ -3623,6 +3623,30 @@ static void intel_ddi_set_fia_lane_count(struct intel_encoder *encoder,
        I915_WRITE(PORT_TX_DFLEXDPMLE1, val);
 }
 
+static void
+intel_ddi_update_prepare(struct intel_atomic_state *state,
+                        struct intel_encoder *encoder,
+                        struct intel_crtc *crtc)
+{
+       struct intel_crtc_state *crtc_state =
+               crtc ? intel_atomic_get_new_crtc_state(state, crtc) : NULL;
+       int required_lanes = crtc_state ? crtc_state->lane_count : 1;
+
+       WARN_ON(crtc && crtc->active);
+
+       intel_tc_port_get_link(enc_to_dig_port(&encoder->base), required_lanes);
+       if (crtc_state && crtc_state->base.active)
+               intel_update_active_dpll(state, crtc, encoder);
+}
+
+static void
+intel_ddi_update_complete(struct intel_atomic_state *state,
+                         struct intel_encoder *encoder,
+                         struct intel_crtc *crtc)
+{
+       intel_tc_port_put_link(enc_to_dig_port(&encoder->base));
+}
+
 static void
 intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
                         const struct intel_crtc_state *crtc_state,
@@ -3630,10 +3654,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));
 
@@ -3658,11 +3685,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);
 }
 
 static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
@@ -4256,6 +4286,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
                                 !port_info->supports_tbt;
 
                intel_tc_port_init(intel_dig_port, is_legacy);
+
+               intel_encoder->update_prepare = intel_ddi_update_prepare;
+               intel_encoder->update_complete = intel_ddi_update_complete;
        }
 
        switch (port) {
index 3417d86284f118fbf7351455b3a7c49e61127c55..919f5ac844c81a5dc54c6a183e7f553a31834bc6 100644 (file)
@@ -6037,6 +6037,98 @@ static void intel_crtc_disable_planes(struct intel_atomic_state *state,
        intel_frontbuffer_flip(dev_priv, fb_bits);
 }
 
+/*
+ * intel_connector_primary_encoder - get the primary encoder for a connector
+ * @connector: connector for which to return the encoder
+ *
+ * Returns the primary encoder for a connector. There is a 1:1 mapping from
+ * all connectors to their encoder, except for DP-MST connectors which have
+ * both a virtual and a primary encoder. These DP-MST primary encoders can be
+ * pointed to by as many DP-MST connectors as there are pipes.
+ */
+static struct intel_encoder *
+intel_connector_primary_encoder(struct intel_connector *connector)
+{
+       struct intel_encoder *encoder;
+
+       if (connector->mst_port)
+               return &dp_to_dig_port(connector->mst_port)->base;
+
+       encoder = intel_attached_encoder(&connector->base);
+       WARN_ON(!encoder);
+
+       return encoder;
+}
+
+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)
+{
+       struct intel_crtc *old_crtc = old_conn_state->crtc ?
+                                     to_intel_crtc(old_conn_state->crtc) : NULL;
+       struct intel_crtc *new_crtc = new_conn_state->crtc ?
+                                     to_intel_crtc(new_conn_state->crtc) : NULL;
+
+       return new_crtc != old_crtc ||
+              (new_crtc &&
+               needs_modeset(intel_atomic_get_new_crtc_state(state, new_crtc)));
+}
+
+static void intel_encoders_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_encoder *encoder;
+               struct intel_crtc *crtc;
+
+               if (!intel_connector_needs_modeset(state,
+                                                  old_conn_state,
+                                                  new_conn_state))
+                       continue;
+
+               encoder = intel_connector_primary_encoder(to_intel_connector(conn));
+               if (!encoder->update_prepare)
+                       continue;
+
+               crtc = new_conn_state->crtc ?
+                       to_intel_crtc(new_conn_state->crtc) : NULL;
+               encoder->update_prepare(state, encoder, crtc);
+       }
+}
+
+static void intel_encoders_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_encoder *encoder;
+               struct intel_crtc *crtc;
+
+               if (!intel_connector_needs_modeset(state,
+                                                  old_conn_state,
+                                                  new_conn_state))
+                       continue;
+
+               encoder = intel_connector_primary_encoder(to_intel_connector(conn));
+               if (!encoder->update_complete)
+                       continue;
+
+               crtc = new_conn_state->crtc ?
+                       to_intel_crtc(new_conn_state->crtc) : NULL;
+               encoder->update_complete(state, encoder, crtc);
+       }
+}
+
 static void intel_encoders_pre_pll_enable(struct intel_crtc *crtc,
                                          struct intel_crtc_state *crtc_state,
                                          struct intel_atomic_state *state)
@@ -13859,14 +13951,20 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
                }
        }
 
+       if (state->modeset)
+               intel_encoders_update_prepare(state);
+
        /* Now enable the clocks, plane, pipe, and connectors that we set up. */
        dev_priv->display.update_crtcs(state);
 
-       if (state->modeset)
+       if (state->modeset) {
+               intel_encoders_update_complete(state);
+
                intel_set_cdclk_post_plane_update(dev_priv,
                                                  &state->cdclk.actual,
                                                  &dev_priv->cdclk.actual,
                                                  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
index bdc7150dbfacac1aa3fd9650687ee71fc6ad2544..76a2c879efc241080dff9352c9cf5aade53f1d48 100644 (file)
@@ -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);
 };
@@ -3402,6 +3404,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,
 };
 
@@ -3526,6 +3529,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
index 579f2ceafba36068fd88238ea1dc46b206f9113f..1668f811690884cf4c675b6b619c07430de0e306 100644 (file)
@@ -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);
index 58a228ba7696f62621230b90183de3c66a11d839..ba6492bc0ee09a878dc2fc1d6952133d680e73c6 100644 (file)
@@ -188,19 +188,12 @@ 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)
 {
+       int max_lanes;
+
        if (!icl_tc_phy_status_complete(dig_port)) {
                DRM_DEBUG_KMS("Port %s: PHY not ready\n",
                              dig_port->tc_port_name);
@@ -211,8 +204,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;
@@ -228,6 +222,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",
+                             dig_port->tc_port_name,
+                             max_lanes, required_lanes);
+               goto out_set_safe_mode;
+       }
+
        dig_port->tc_mode = TC_PORT_DP_ALT;
 
        return;
@@ -311,7 +312,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_mode old_tc_mode = dig_port->tc_mode;
@@ -319,7 +321,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",
                      dig_port->tc_port_name,
@@ -327,6 +329,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 intel_encoder *encoder = &dig_port->base;
@@ -344,11 +354,13 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
                if (!icl_tc_phy_is_connected(dig_port))
                        DRM_DEBUG_KMS("Port %s: PHY disconnected with %d active link(s)\n",
                                      dig_port->tc_port_name, 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_KMS("Port %s: sanitize mode (%s)\n",
@@ -377,27 +389,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);
 }
 
@@ -415,4 +460,5 @@ void 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;
 }
index b5af2fe60b22ba66b8d8049dd34401ea1387629b..31af7be96070e4a4fb9879a6b45a7399c1550c24 100644 (file)
@@ -19,6 +19,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);
 
index 12a102e239c5efa84e797ca26d70211ed723ba9a..24c63ed45c6fa4cd65652c2f56401303c4b42ba0 100644 (file)
@@ -115,6 +115,9 @@ struct intel_encoder {
        int (*compute_config)(struct intel_encoder *,
                              struct intel_crtc_state *,
                              struct drm_connector_state *);
+       void (*update_prepare)(struct intel_atomic_state *,
+                              struct intel_encoder *,
+                              struct intel_crtc *);
        void (*pre_pll_enable)(struct intel_encoder *,
                               const struct intel_crtc_state *,
                               const struct drm_connector_state *);
@@ -124,6 +127,9 @@ struct intel_encoder {
        void (*enable)(struct intel_encoder *,
                       const struct intel_crtc_state *,
                       const struct drm_connector_state *);
+       void (*update_complete)(struct intel_atomic_state *,
+                               struct intel_encoder *,
+                               struct intel_crtc *);
        void (*disable)(struct intel_encoder *,
                        const struct intel_crtc_state *,
                        const struct drm_connector_state *);
@@ -1234,6 +1240,8 @@ struct intel_digital_port {
        enum aux_ch aux_ch;
        enum intel_display_power_domain ddi_io_power_domain;
        struct mutex tc_lock;   /* protects the TypeC port mode */
+       intel_wakeref_t tc_lock_wakeref;
+       int tc_link_refcount;
        bool tc_legacy_port:1;
        char tc_port_name[8];
        enum tc_port_mode tc_mode;