>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of >Paulo Zanoni >Sent: Wednesday, August 1, 2018 10:35 AM >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Cc: Zanoni, Paulo R <paulo.r.zanoni@xxxxxxxxx>; Vivi, Rodrigo ><rodrigo.vivi@xxxxxxxxx> >Subject: [PATCH v3] drm/i915/icl: implement the tc/legacy HPD {dis, >}connect flows > >Unlike the other ports, TC ports are not available to use as soon as we get a >hotplug. The TC PHYs can be shared between multiple >controllers: 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. > >This patch implements the flow sequences described by our specification. We opt >to grab ownership of the ports as soon as we get the hotplugs in order to simplify >the interactions and avoid surprises in the user space side. We may consider >changing this in the future, once we improve our testing capabilities on this area. > >v2: > * This unifies the DP and HDMI patches so we can discuss everything > at once so people looking at random single patches can actually > understand the direction. > * I found out the spec was updated a while ago. There's a small > difference in the connect flow and the patch was updated for that. > * Our spec also now gives a good explanation on what is really > happening. As a result, comments were added. > * Add some more comments as requested by Rodrigo (Rodrigo). >v3: > * Downgrade a DRM_ERROR that shouldn't ever happen but we can't act > on in case it does (Chris). > >BSpec: 21750, 4250. >Cc: Animesh Manna <animesh.manna@xxxxxxxxx> >Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> >Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >--- > drivers/gpu/drm/i915/i915_reg.h | 6 +++ > drivers/gpu/drm/i915/intel_dp.c | 110 >+++++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_hdmi.c | 11 ++-- > 3 files changed, 123 insertions(+), 4 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >index 1da1c7743785..a5f4dfe9ebdf 100644 >--- a/drivers/gpu/drm/i915/i915_reg.h >+++ b/drivers/gpu/drm/i915/i915_reg.h >@@ -10750,4 +10750,10 @@ enum skl_power_gate { > #define DP_LANE_ASSIGNMENT_MASK(tc_port) (0xf << ((tc_port) * 8)) > #define DP_LANE_ASSIGNMENT(tc_port, x) ((x) << ((tc_port) * 8)) > >+#define PORT_TX_DFLEXDPPMS > _MMIO(0x163890) >+#define DP_PHY_MODE_STATUS_COMPLETED(tc_port) (1 << >(tc_port)) >+ >+#define PORT_TX_DFLEXDPCSSS > _MMIO(0x163894) >+#define DP_PHY_MODE_STATUS_NOT_SAFE(tc_port) (1 << (tc_port)) >+ > #endif /* _I915_REG_H_ */ >diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >index fec21aa3db93..86eded613456 100644 >--- a/drivers/gpu/drm/i915/intel_dp.c >+++ b/drivers/gpu/drm/i915/intel_dp.c >@@ -4818,6 +4818,104 @@ static void icl_update_tc_port_type(struct >drm_i915_private *dev_priv, > type_str); > } > >+/* >+ * This function implements the first part of the Connect Flow >+described by our >+ * specification, Gen11 TypeC Programming chapter. The rest of the flow >+(reading >+ * lanes, EDID, etc) is done as needed in the typical places. >+ * >+ * Unlike the other ports, type-C ports are not available to use as >+soon as we >+ * get a hotplug. The type-C PHYs can be shared between multiple controllers: >+ * 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 bool icl_tc_phy_connect(struct drm_i915_private *dev_priv, >+ struct intel_digital_port *dig_port) { >+ enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); >+ u32 val; >+ >+ if (dig_port->tc_type != TC_PORT_LEGACY && >+ dig_port->tc_type != TC_PORT_TYPEC) >+ return true; >+ >+ val = I915_READ(PORT_TX_DFLEXDPPMS); >+ if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) { >+ DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", >tc_port); >+ return false; >+ } >+ >+ /* >+ * This function may be called many times in a row without an HPD event >+ * in between, so try to avoid the write when we can. >+ */ >+ val = I915_READ(PORT_TX_DFLEXDPCSSS); >+ if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) { >+ val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); >+ I915_WRITE(PORT_TX_DFLEXDPCSSS, val); >+ } >+ >+ /* >+ * Now we have to re-check the live state, in case the port recently >+ * became disconnected. Not necessary for legacy mode. >+ */ >+ if (dig_port->tc_type == TC_PORT_TYPEC && >+ !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) { >+ DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port); >+ val = I915_READ(PORT_TX_DFLEXDPCSSS); >+ val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); >+ I915_WRITE(PORT_TX_DFLEXDPCSSS, val); >+ return false; >+ } >+ >+ return true; >+} >+ >+/* >+ * See the comment at the connect function. This implements the >+Disconnect >+ * Flow. >+ */ >+static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv, >+ struct intel_digital_port *dig_port) { >+ enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); >+ u32 val; >+ >+ if (dig_port->tc_type != TC_PORT_LEGACY && >+ dig_port->tc_type != TC_PORT_TYPEC) Don’t we also need to check for dig_port->tc_type != TC_PORT_TBT? >+ return; >+ >+ /* >+ * This function may be called many times in a row without an HPD event >+ * in between, so try to avoid the write when we can. >+ */ >+ val = I915_READ(PORT_TX_DFLEXDPCSSS); >+ if (val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)) { >+ val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); >+ I915_WRITE(PORT_TX_DFLEXDPCSSS, val); >+ } >+} >+ >+/* >+ * The type-C ports are different because even when they are connected, >+they may >+ * not be available/usable by the graphics driver: see the comment on >+ * icl_tc_phy_connect(). So in our driver instead of adding the >+additional >+ * concept of "usable" and make everything check for "connected and >+usable" we >+ * define a port as "connected" when it is not only connected, but also >+when it >+ * is usable by the rest of the driver. That maintains the old >+assumption that >+ * connected ports are usable, and avoids exposing to the users objects >+they >+ * can't really use. >+ */ > static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, > struct intel_digital_port *intel_dig_port) { @@ >-4836,12 +4934,17 @@ static bool icl_tc_port_connected(struct >drm_i915_private *dev_priv, > is_typec = dpsp & TC_LIVE_STATE_TC(tc_port); > is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port); > >- if (!is_legacy && !is_typec && !is_tbt) >+ if (!is_legacy && !is_typec && !is_tbt) { If it is none of the above, then why not just return false? Anusha >+ icl_tc_phy_disconnect(dev_priv, intel_dig_port); > return false; >+ } > > icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec, > is_tbt); > >+ if (!icl_tc_phy_connect(dev_priv, intel_dig_port)) >+ return false; >+ > return true; > } > >@@ -4869,6 +4972,11 @@ static bool icl_digital_port_connected(struct >intel_encoder *encoder) > * intel_digital_port_connected - is the specified port connected? > * @encoder: intel_encoder > * >+ * In cases where there's a connector physically connected but it can't >+ be used >+ * by our hardware we also return false, since the rest of the driver >+ should >+ * pretty much treat the port as disconnected. This is relevant for >+ type-C >+ * (starting on ICL) where there's ownership involved. >+ * > * Return %true if port is connected, %false otherwise. > */ > bool intel_digital_port_connected(struct intel_encoder *encoder) diff --git >a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >index 8363fbd18ee8..548199e59b6c 100644 >--- a/drivers/gpu/drm/i915/intel_hdmi.c >+++ b/drivers/gpu/drm/i915/intel_hdmi.c >@@ -1905,21 +1905,26 @@ intel_hdmi_set_edid(struct drm_connector >*connector) static enum drm_connector_status intel_hdmi_detect(struct >drm_connector *connector, bool force) { >- enum drm_connector_status status; >+ enum drm_connector_status status = connector_status_disconnected; > struct drm_i915_private *dev_priv = to_i915(connector->dev); >+ struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); >+ struct intel_encoder *encoder = &hdmi_to_dig_port(intel_hdmi)->base; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > connector->base.id, connector->name); > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > >+ if (IS_ICELAKE(dev_priv) && >+ !intel_digital_port_connected(encoder)) >+ goto out; >+ > intel_hdmi_unset_edid(connector); > > if (intel_hdmi_set_edid(connector)) > status = connector_status_connected; >- else >- status = connector_status_disconnected; > >+out: > intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); > > return status; >-- >2.14.4 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
