| Message ID | 20180801173441.9789-1-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915/icl: implement the tc/legacy HPD {dis, }connect flows | expand |
>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >Paulo Zanoni >Sent: Wednesday, August 1, 2018 10:35 AM >To: intel-gfx@lists.freedesktop.org >Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com>; Vivi, Rodrigo ><rodrigo.vivi@intel.com> >Subject: [Intel-gfx] [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@intel.com> >Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >--- > 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@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Sex, 2018-08-17 às 09:25 -0700, Srivatsa, Anusha escreveu: > > -----Original Message----- > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On > > Behalf Of > > Paulo Zanoni > > Sent: Wednesday, August 1, 2018 10:35 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com>; Vivi, Rodrigo > > <rodrigo.vivi@intel.com> > > Subject: [Intel-gfx] [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@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > 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? We don't need to run these steps for TBT, the TBT sequence is simpler. See the spec. > > + 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? Because we need to run the disconnect sequence in case the port was previously connected so we release ownership of the lanes. That's the very goal of this patch. Thanks, Paulo > > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Aug 01, 2018 at 10:34:41AM -0700, Paulo Zanoni wrote: > 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@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> sorry for taking so long... thanks for the doc and all other changes. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > 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) > + 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) { > + 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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) + 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) { + 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;
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@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- 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(-)