Re: [PATCH v3] drm/i915/icl: implement the tc/legacy HPD {dis, }connect flows

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux