drm/i915/icl: Fix TypeC legacy HPD handling

PATCH Superseded Imre Deak 3 participants 14 messages Dec 13, 2018 – Dec 17, 2018

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Imre Deak

author

2018-12-13 19:48 UTC #0 lore raw

cover drm/i915/icl: Fix TypeC legacy HPD handling

This patchset fixes the HPD handling for TypeC legacy ports. It depends on an indirect detection method described in patch 2 and 3, which will be replaced by a direct method once the BIOS/HW/FW team delivers a promised SW/HW flag for this purpose.

There is no issue with the indirect method I know of, except for the DP legacy case (as opposed to the HDMI case), where doing a modeset/DP AUX read before the first HPD connect event will still result in mayham. But that is already the situation now and we have a plan in place (the direct method mentioned above), so the change is an improvement even for the DP case.

This will leave the Thunderbolt and USB DP alternate mode HPD handling still disfunctional, but that's another story, we have some plan to fix that too (in cooperation with the HW/FW team).

Cc Paulo Zanoni <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc José Roberto de Souza <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Imre Deak (3):
drm/i915/icl: Add a debug print for TypeC port disconnection drm/i915/icl: Add fix for TypeC legacy HDMI HPD handling drm/i915/icl: Add fix for TypeC legacy DP HPD handling

 drivers/gpu/drm/i915/intel_ddi.c | 54 +++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_dp.c  | 69 +++++++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h |  5 ++-
 3 files changed, 105 insertions(+), 23 deletions(-)

Signature · 5 lines

2.13.2
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Imre Deak

author

2018-12-13 19:48 UTC #1 lore raw

PATCH 1/3 drm/i915/icl: Add a debug print for TypeC port disconnection

It's useful to see at which point a TypeC port gets disconnected, so add add a debug print for it.

Cc Paulo Zanoni <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc José Roberto de Souza <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +24 ~~-10~~

index e94faa0a42eb..082594bb65a7 100644
@@ -5066,28 +5066,38 @@ static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
 	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
 }
 
+static const char *tc_type_name(enum tc_port_type type)
+{
+	static const char *names[] = {
+		[TC_PORT_UNKNOWN] = "unknown",
+		[TC_PORT_LEGACY] = "legacy",
+		[TC_PORT_TYPEC] = "typec",
+		[TC_PORT_TBT] = "tbt",
+	};
+
+	if (WARN_ON(type >= ARRAY_SIZE(names)))
+		type = TC_PORT_UNKNOWN;
+
+	return names[type];
+}
+
 static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 				    struct intel_digital_port *intel_dig_port,
 				    bool is_legacy, bool is_typec, bool is_tbt)
 {
 	enum port port = intel_dig_port->base.port;
 	enum tc_port_type old_type = intel_dig_port->tc_type;
-	const char *type_str;
 
 	WARN_ON(is_legacy + is_typec + is_tbt != 1);
 
-	if (is_legacy) {
+	if (is_legacy)
 		intel_dig_port->tc_type = TC_PORT_LEGACY;
-		type_str = "legacy";
-	} else if (is_typec) {
+	else if (is_typec)
 		intel_dig_port->tc_type = TC_PORT_TYPEC;
-		type_str = "typec";
-	} else if (is_tbt) {
+	else if (is_tbt)
 		intel_dig_port->tc_type = TC_PORT_TBT;
-		type_str = "tbt";
-	} else {
+	else
 		return;
-	}
 
 	/* Types are not supposed to be changed at runtime. */
 	WARN_ON(old_type != TC_PORT_UNKNOWN &&

@@ -5095,7 +5105,7 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 
 	if (old_type != intel_dig_port->tc_type)
 		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
-			      type_str);
+			      tc_type_name(intel_dig_port->tc_type));
 }
 
 static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,

@@ -5187,6 +5197,10 @@ static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
 		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
 	}
 
+	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
+		      port_name(dig_port->base.port),
+		      tc_type_name(dig_port->tc_type));
+
 	dig_port->tc_type = TC_PORT_UNKNOWN;
 }

Signature · 6 lines

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi

maintainer Reviewed-by on v1 1/3

2018-12-13 20:27 UTC #2 lore raw

Re: [PATCH 1/3] drm/i915/icl: Add a debug print for TypeC port disconnection

Quoted 8 lines · Imre Deak

It's useful to see at which point a TypeC port gets disconnected, so add add a debug print for it.
Cc Paulo Zanoni <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc José Roberto de Souza <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Signed-off-by Imre Deak <[email protected]>

Reviewed-by Rodrigo Vivi <[email protected]>

Quoted 83 lines

---
 drivers/gpu/drm/i915/intel_dp.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +24 ~~-10~~

index e94faa0a42eb..082594bb65a7 100644
@@ -5066,28 +5066,38 @@ static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
 	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
 }
 
+static const char *tc_type_name(enum tc_port_type type)
+{
+	static const char *names[] = {
+		[TC_PORT_UNKNOWN] = "unknown",
+		[TC_PORT_LEGACY] = "legacy",
+		[TC_PORT_TYPEC] = "typec",
+		[TC_PORT_TBT] = "tbt",
+	};
+
+	if (WARN_ON(type >= ARRAY_SIZE(names)))
+		type = TC_PORT_UNKNOWN;
+
+	return names[type];
+}
+
 static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 				    struct intel_digital_port *intel_dig_port,
 				    bool is_legacy, bool is_typec, bool is_tbt)
 {
 	enum port port = intel_dig_port->base.port;
 	enum tc_port_type old_type = intel_dig_port->tc_type;
-	const char *type_str;
 
 	WARN_ON(is_legacy + is_typec + is_tbt != 1);
 
-	if (is_legacy) {
+	if (is_legacy)
 		intel_dig_port->tc_type = TC_PORT_LEGACY;
-		type_str = "legacy";
-	} else if (is_typec) {
+	else if (is_typec)
 		intel_dig_port->tc_type = TC_PORT_TYPEC;
-		type_str = "typec";
-	} else if (is_tbt) {
+	else if (is_tbt)
 		intel_dig_port->tc_type = TC_PORT_TBT;
-		type_str = "tbt";
-	} else {
+	else
 		return;
-	}
 
 	/* Types are not supposed to be changed at runtime. */
 	WARN_ON(old_type != TC_PORT_UNKNOWN &&

@@ -5095,7 +5105,7 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 
 	if (old_type != intel_dig_port->tc_type)
 		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
-			      type_str);
+			      tc_type_name(intel_dig_port->tc_type));
 }
 
 static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,

@@ -5187,6 +5197,10 @@ static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
 		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
 	}
 
+	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
+		      port_name(dig_port->base.port),
+		      tc_type_name(dig_port->tc_type));
+
 	dig_port->tc_type = TC_PORT_UNKNOWN;
 }

Signature · 6 lines

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Imre Deak

author

2018-12-13 19:48 UTC #3 lore raw

PATCH 2/3 drm/i915/icl: Fix TypeC legacy HDMI HPD handling

In reply to Imre Deak

Atm HPD disconnect events on TypeC ports will break things, since we'll switch the TypeC mode (between Legacy and disconnected modes as well as among USB DP alternate, Thunderbolt alternate and disconnected modes) on the fly from the HPD disconnect interrupt work while the port may be still active.

Even if the port happens to be not active during the disconnect we'd still have a problem during a subsequent modeset or AUX transfer that could happen regardless of the port's connected state. For instance the system resume display mode restore code and userspace could perform a modeset on the port or userspace could start an AUX transfer even if the port is in disconnected state.

To fix this keep legacy TypeC ports in TypeC legacy mode whenever we're not suspended. The legacy mode is a static configuration as opposed to the Thunderbolt and USB DP alternate modes between which we can switch dynamically.

We don't have yet an explicit way to detect TypeC legacy ports, but we can imply that at least in case of HDMI enabled ports, since HDMI can only be provided in legacy mode. So mark TypeC HDMI enabled ports as legacy and run the TypeC PHY connect sequence explicitly during driver loading and system resume. The connect will succeed even if the display is not connected to begin with (or disappears during the suspended state) since for legacy ports the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag is always set (as opposed to the USB DP alternate mode where it gets set only when a display is connected).

Correspondingly run the TypeC PHY disconnect sequence during system suspend and driver unloading, but disconnect during suspend only for legacy TypeC mode. We will need to disconnect even in USB DP alternate mode in the future, but atm we don't have a way to reconnect the port in this mode during resume if the display disappears while being suspended. So for now punt on this case.

Note that we do not disconnect the port during runtime suspend; in legacy mode there are no shared HW resources (PHY lanes) with other HW blocks (USB), so no need to release / reacquire these resources as with USB DP alternate mode. The only reason to disconnect legacy ports during system suspend is that the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag must be rechecked and the port must be connected again during system resume. We may also need to turn the check for this flag into a poll, depending on further tests and clarifications we are expecting from HW/FW people.

If VBT says that the port provides only DP functionality then we can't assume that it's a legacy port as for HDMI (since it could as well be a TBT/DP Alt connector), so we'll solve HPD handling for the DP case with a different method in the next patch.

Eventually - instead of the above method - we'll depend on an explicit detection method provided either via a VBT flag or a FW/HW register both for the HDMI and DP case.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108070
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108924

Cc Paulo Zanoni <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc José Roberto de Souza <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/intel_ddi.c | 54 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp.c  | 29 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  5 +++-
 3 files changed, 75 insertions(+), 13 deletions(-)

drivers/gpu/drm/i915/intel_ddi.c +51 -3

index f3e1d6a0b7dd..2e47ffa1c95a 100644
@@ -3901,9 +3901,50 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 
 }
 
+static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+
+	intel_dp_encoder_suspend(encoder);
+
+	/*
+	 * TODO: disconnect also from USB DP alternate mode once we have a
+	 * way to handle the modeset restore in that mode during resume
+	 * even if the sink has disappeared while being suspended.
+	 */
+	if (dig_port->tc_legacy_port)
+		icl_tc_phy_disconnect(i915, dig_port);
+}
+
+static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(drm_encoder);
+	struct drm_i915_private *i915 = to_i915(drm_encoder->dev);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		intel_digital_port_connected(&dig_port->base);
+
+	intel_dp_encoder_reset(drm_encoder);
+}
+
+static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	struct drm_i915_private *i915 = to_i915(encoder->dev);
+
+	intel_dp_encoder_flush_work(encoder);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		icl_tc_phy_disconnect(i915, dig_port);
+
+	drm_encoder_cleanup(encoder);
+	kfree(dig_port);
+}
+
 static const struct drm_encoder_funcs intel_ddi_funcs = {
-	.reset = intel_dp_encoder_reset,
-	.destroy = intel_dp_encoder_destroy,
+	.reset = intel_ddi_encoder_reset,
+	.destroy = intel_ddi_encoder_destroy,
 };
 
 static struct intel_connector *

@@ -4197,7 +4238,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->post_disable = intel_ddi_post_disable;
 	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
 	intel_encoder->get_config = intel_ddi_get_config;
-	intel_encoder->suspend = intel_dp_encoder_suspend;
+	intel_encoder->suspend = intel_ddi_encoder_suspend;
 	intel_encoder->get_power_domains = intel_ddi_get_power_domains;
 	intel_encoder->type = INTEL_OUTPUT_DDI;
 	intel_encoder->power_domain = intel_port_to_power_domain(port);

@@ -4257,6 +4298,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
 		if (!intel_ddi_init_hdmi_connector(intel_dig_port))
 			goto err;
+
+		if (intel_port_is_tc(dev_priv, port))
+			intel_dig_port->tc_legacy_port = true;
 	}
 
 	if (init_lspcon) {

@@ -4274,6 +4318,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	}
 
 	intel_infoframe_init(intel_dig_port);
+
+	if (intel_port_is_tc(dev_priv, port))
+		intel_digital_port_connected(intel_encoder);
+
 	return;
 
 err:

drivers/gpu/drm/i915/intel_dp.c +20 -9

index 082594bb65a7..19e49adab548 100644
@@ -5108,9 +5108,6 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 			      tc_type_name(intel_dig_port->tc_type));
 }
 
-static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
-				  struct intel_digital_port *dig_port);
-
 /*
  * This function implements the first part of the Connect Flow described by our
  * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading

@@ -5145,6 +5142,7 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
 	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);
+		WARN_ON(dig_port->tc_legacy_port);
 		return false;
 	}

@@ -5176,8 +5174,8 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
  * 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)
+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);

@@ -5222,7 +5220,8 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	bool is_legacy, is_typec, is_tbt;
 	u32 dpsp;
 
-	is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
+	is_legacy = intel_dig_port->tc_legacy_port ||
+		I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
 
 	/*
 	 * The spec says we shouldn't be using the ISR bits for detecting

@@ -5233,7 +5232,14 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
 
 	if (!is_legacy && !is_typec && !is_tbt) {
-		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+		/*
+		 * We disconnect from legacy mode only during system
+		 * suspend and driver unloading, otherwise we keep the port in
+		 * legacy mode even after an HPD disconnect event.
+		 */
+		if (intel_dig_port->tc_type != TC_PORT_LEGACY)
+			icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+
 		return false;
 	}

@@ -5542,7 +5548,7 @@ intel_dp_connector_unregister(struct drm_connector *connector)
 	intel_connector_unregister(connector);
 }
 
-void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;

@@ -5565,9 +5571,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	}
 
 	intel_dp_aux_fini(intel_dp);
+}
+
+static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+{
+	intel_dp_encoder_flush_work(encoder);
 
 	drm_encoder_cleanup(encoder);
-	kfree(intel_dig_port);
+	kfree(enc_to_dig_port(encoder));
 }
 
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)

drivers/gpu/drm/i915/intel_drv.h +4 -1

index d08f08f607dd..97c422bea7fb 100644
@@ -1234,6 +1234,7 @@ struct intel_digital_port {
 	/* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */
 	enum aux_ch aux_ch;
 	enum intel_display_power_domain ddi_io_power_domain;
+	bool tc_legacy_port:1;
 	enum tc_port_type tc_type;
 
 	void (*write_infoframe)(struct intel_encoder *encoder,

@@ -1806,7 +1807,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 					   bool enable);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
-void intel_dp_encoder_destroy(struct drm_encoder *encoder);
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config,
 			     struct drm_connector_state *conn_state);

@@ -1874,6 +1875,8 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
+			   struct intel_digital_port *dig_port);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);

Signature · 6 lines

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi

maintainer

2018-12-13 21:06 UTC #4 lore raw

Re: [PATCH 2/3] drm/i915/icl: Fix TypeC legacy HDMI HPD handling

Quoted 90 lines · Imre Deak

Atm HPD disconnect events on TypeC ports will break things, since we'll switch the TypeC mode (between Legacy and disconnected modes as well as among USB DP alternate, Thunderbolt alternate and disconnected modes) on the fly from the HPD disconnect interrupt work while the port may be still active.
Even if the port happens to be not active during the disconnect we'd still have a problem during a subsequent modeset or AUX transfer that could happen regardless of the port's connected state. For instance the system resume display mode restore code and userspace could perform a modeset on the port or userspace could start an AUX transfer even if the port is in disconnected state.
To fix this keep legacy TypeC ports in TypeC legacy mode whenever we're not suspended. The legacy mode is a static configuration as opposed to the Thunderbolt and USB DP alternate modes between which we can switch dynamically.
We don't have yet an explicit way to detect TypeC legacy ports, but we can imply that at least in case of HDMI enabled ports, since HDMI can only be provided in legacy mode. So mark TypeC HDMI enabled ports as legacy and run the TypeC PHY connect sequence explicitly during driver loading and system resume. The connect will succeed even if the display is not connected to begin with (or disappears during the suspended state) since for legacy ports the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag is always set (as opposed to the USB DP alternate mode where it gets set only when a display is connected).
Correspondingly run the TypeC PHY disconnect sequence during system suspend and driver unloading, but disconnect during suspend only for legacy TypeC mode. We will need to disconnect even in USB DP alternate mode in the future, but atm we don't have a way to reconnect the port in this mode during resume if the display disappears while being suspended. So for now punt on this case.
Note that we do not disconnect the port during runtime suspend; in legacy mode there are no shared HW resources (PHY lanes) with other HW blocks (USB), so no need to release / reacquire these resources as with USB DP alternate mode. The only reason to disconnect legacy ports during system suspend is that the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag must be rechecked and the port must be connected again during system resume. We may also need to turn the check for this flag into a poll, depending on further tests and clarifications we are expecting from HW/FW people.
If VBT says that the port provides only DP functionality then we can't assume that it's a legacy port as for HDMI (since it could as well be a TBT/DP Alt connector), so we'll solve HPD handling for the DP case with a different method in the next patch.
Eventually - instead of the above method - we'll depend on an explicit detection method provided either via a VBT flag or a FW/HW register both for the HDMI and DP case.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108070
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108924
Cc Paulo Zanoni <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc José Roberto de Souza <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Signed-off-by Imre Deak <[email protected]>
---
 drivers/gpu/drm/i915/intel_ddi.c | 54 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp.c  | 29 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  5 +++-
 3 files changed, 75 insertions(+), 13 deletions(-)
drivers/gpu/drm/i915/intel_ddi.c +15 -0
index f3e1d6a0b7dd..2e47ffa1c95a 100644
@@ -3901,9 +3901,50 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 
 }
 
+static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+
+	intel_dp_encoder_suspend(encoder);
+
+	/*
+	 * TODO: disconnect also from USB DP alternate mode once we have a
+	 * way to handle the modeset restore in that mode during resume
+	 * even if the sink has disappeared while being suspended.
+	 */
+	if (dig_port->tc_legacy_port)
+		icl_tc_phy_disconnect(i915, dig_port);
+}

drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3928

Quoted 10 lines

drivers/gpu/drm/i915/intel_ddi.c +10 -0

+
+static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(drm_encoder);
+	struct drm_i915_private *i915 = to_i915(drm_encoder->dev);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		intel_digital_port_connected(&dig_port->base);
+
+	intel_dp_encoder_reset(drm_encoder);

do we need all of that? or could simply

intel_dp->reset_link_params = true;

instead?!

drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3941

Quoted 13 lines

drivers/gpu/drm/i915/intel_ddi.c +13 -0

+}
+
+static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	struct drm_i915_private *i915 = to_i915(encoder->dev);
+
+	intel_dp_encoder_flush_work(encoder);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		icl_tc_phy_disconnect(i915, dig_port);
+
+	drm_encoder_cleanup(encoder);

and here don't we need stuff on intel_dp_encoder destroy here like intel_dp_aux_fini?

Quoted 162 lines

drivers/gpu/drm/i915/intel_ddi.c +13 -3

+	kfree(dig_port);
+}
+
 static const struct drm_encoder_funcs intel_ddi_funcs = {
-	.reset = intel_dp_encoder_reset,
-	.destroy = intel_dp_encoder_destroy,
+	.reset = intel_ddi_encoder_reset,
+	.destroy = intel_ddi_encoder_destroy,
 };
 
 static struct intel_connector *

@@ -4197,7 +4238,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->post_disable = intel_ddi_post_disable;
 	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
 	intel_encoder->get_config = intel_ddi_get_config;
-	intel_encoder->suspend = intel_dp_encoder_suspend;
+	intel_encoder->suspend = intel_ddi_encoder_suspend;
 	intel_encoder->get_power_domains = intel_ddi_get_power_domains;
 	intel_encoder->type = INTEL_OUTPUT_DDI;
 	intel_encoder->power_domain = intel_port_to_power_domain(port);

@@ -4257,6 +4298,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
 		if (!intel_ddi_init_hdmi_connector(intel_dig_port))
 			goto err;
+
+		if (intel_port_is_tc(dev_priv, port))
+			intel_dig_port->tc_legacy_port = true;
 	}
 
 	if (init_lspcon) {

@@ -4274,6 +4318,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	}
 
 	intel_infoframe_init(intel_dig_port);
+
+	if (intel_port_is_tc(dev_priv, port))
+		intel_digital_port_connected(intel_encoder);
+
 	return;
 
 err:

drivers/gpu/drm/i915/intel_dp.c +20 -9

index 082594bb65a7..19e49adab548 100644
@@ -5108,9 +5108,6 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 			      tc_type_name(intel_dig_port->tc_type));
 }
 
-static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
-				  struct intel_digital_port *dig_port);
-
 /*
  * This function implements the first part of the Connect Flow described by our
  * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading

@@ -5145,6 +5142,7 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
 	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);
+		WARN_ON(dig_port->tc_legacy_port);
 		return false;
 	}

@@ -5176,8 +5174,8 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
  * 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)
+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);

@@ -5222,7 +5220,8 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	bool is_legacy, is_typec, is_tbt;
 	u32 dpsp;
 
-	is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
+	is_legacy = intel_dig_port->tc_legacy_port ||
+		I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
 
 	/*
 	 * The spec says we shouldn't be using the ISR bits for detecting

@@ -5233,7 +5232,14 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
 
 	if (!is_legacy && !is_typec && !is_tbt) {
-		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+		/*
+		 * We disconnect from legacy mode only during system
+		 * suspend and driver unloading, otherwise we keep the port in
+		 * legacy mode even after an HPD disconnect event.
+		 */
+		if (intel_dig_port->tc_type != TC_PORT_LEGACY)
+			icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+
 		return false;
 	}

@@ -5542,7 +5548,7 @@ intel_dp_connector_unregister(struct drm_connector *connector)
 	intel_connector_unregister(connector);
 }
 
-void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;

@@ -5565,9 +5571,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	}
 
 	intel_dp_aux_fini(intel_dp);
+}
+
+static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+{
+	intel_dp_encoder_flush_work(encoder);
 
 	drm_encoder_cleanup(encoder);
-	kfree(intel_dig_port);
+	kfree(enc_to_dig_port(encoder));
 }
 
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)

drivers/gpu/drm/i915/intel_drv.h +4 -1

index d08f08f607dd..97c422bea7fb 100644
@@ -1234,6 +1234,7 @@ struct intel_digital_port {
 	/* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */
 	enum aux_ch aux_ch;
 	enum intel_display_power_domain ddi_io_power_domain;
+	bool tc_legacy_port:1;
 	enum tc_port_type tc_type;
 
 	void (*write_infoframe)(struct intel_encoder *encoder,

@@ -1806,7 +1807,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 					   bool enable);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
-void intel_dp_encoder_destroy(struct drm_encoder *encoder);
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config,
 			     struct drm_connector_state *conn_state);

@@ -1874,6 +1875,8 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
+			   struct intel_digital_port *dig_port);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);

Signature · 6 lines

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Imre Deak

author

2018-12-13 23:25 UTC #5 lore raw

Re: [PATCH 2/3] drm/i915/icl: Fix TypeC legacy HDMI HPD handling

Quoted 95 lines · Rodrigo Vivi

Quoted 94 lines · Imre Deak
Atm HPD disconnect events on TypeC ports will break things, since we'll switch the TypeC mode (between Legacy and disconnected modes as well as among USB DP alternate, Thunderbolt alternate and disconnected modes) on the fly from the HPD disconnect interrupt work while the port may be still active.
Even if the port happens to be not active during the disconnect we'd still have a problem during a subsequent modeset or AUX transfer that could happen regardless of the port's connected state. For instance the system resume display mode restore code and userspace could perform a modeset on the port or userspace could start an AUX transfer even if the port is in disconnected state.
To fix this keep legacy TypeC ports in TypeC legacy mode whenever we're not suspended. The legacy mode is a static configuration as opposed to the Thunderbolt and USB DP alternate modes between which we can switch dynamically.
We don't have yet an explicit way to detect TypeC legacy ports, but we can imply that at least in case of HDMI enabled ports, since HDMI can only be provided in legacy mode. So mark TypeC HDMI enabled ports as legacy and run the TypeC PHY connect sequence explicitly during driver loading and system resume. The connect will succeed even if the display is not connected to begin with (or disappears during the suspended state) since for legacy ports the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag is always set (as opposed to the USB DP alternate mode where it gets set only when a display is connected).
Correspondingly run the TypeC PHY disconnect sequence during system suspend and driver unloading, but disconnect during suspend only for legacy TypeC mode. We will need to disconnect even in USB DP alternate mode in the future, but atm we don't have a way to reconnect the port in this mode during resume if the display disappears while being suspended. So for now punt on this case.
Note that we do not disconnect the port during runtime suspend; in legacy mode there are no shared HW resources (PHY lanes) with other HW blocks (USB), so no need to release / reacquire these resources as with USB DP alternate mode. The only reason to disconnect legacy ports during system suspend is that the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag must be rechecked and the port must be connected again during system resume. We may also need to turn the check for this flag into a poll, depending on further tests and clarifications we are expecting from HW/FW people.
If VBT says that the port provides only DP functionality then we can't assume that it's a legacy port as for HDMI (since it could as well be a TBT/DP Alt connector), so we'll solve HPD handling for the DP case with a different method in the next patch.
Eventually - instead of the above method - we'll depend on an explicit detection method provided either via a VBT flag or a FW/HW register both for the HDMI and DP case.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108070
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108924
Cc Paulo Zanoni <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc José Roberto de Souza <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Signed-off-by Imre Deak <[email protected]>
---
 drivers/gpu/drm/i915/intel_ddi.c | 54 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp.c  | 29 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  5 +++-
 3 files changed, 75 insertions(+), 13 deletions(-)
drivers/gpu/drm/i915/intel_ddi.c +19 -0
index f3e1d6a0b7dd..2e47ffa1c95a 100644
@@ -3901,9 +3901,50 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 
 }
 
+static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+
+	intel_dp_encoder_suspend(encoder);
+
+	/*
+	 * TODO: disconnect also from USB DP alternate mode once we have a
+	 * way to handle the modeset restore in that mode during resume
+	 * even if the sink has disappeared while being suspended.
+	 */
+	if (dig_port->tc_legacy_port)
+		icl_tc_phy_disconnect(i915, dig_port);
+}
+
+static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(drm_encoder);

Quoted 8 lines

drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3928
Quoted 6 lines
drivers/gpu/drm/i915/intel_ddi.c +6 -0
+	struct drm_i915_private *i915 = to_i915(drm_encoder->dev);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		intel_digital_port_connected(&dig_port->base);
+
+	intel_dp_encoder_reset(drm_encoder);
do we need all of that?

You mean if we need to call intel_dp_encoder_reset()? That was the original state, as before this change we had

.reset = intel_dp_encoder_reset

in intel_ddi_funcs.

Quoted 5 lines

or could simply
intel_dp->reset_link_params = true;
instead?!

Not sure how we could avoid calling intel_dp_encoder_reset() here, it has required LSPCON and eDP specific parts.

Quoted 17 lines

Quoted 4 lines
drivers/gpu/drm/i915/intel_ddi.c +3 -0
+}
+
+static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3941
Quoted 10 lines
drivers/gpu/drm/i915/intel_ddi.c +10 -0
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	struct drm_i915_private *i915 = to_i915(encoder->dev);
+
+	intel_dp_encoder_flush_work(encoder);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		icl_tc_phy_disconnect(i915, dig_port);
+
+	drm_encoder_cleanup(encoder);
and here don't we need stuff on intel_dp_encoder destroy here like intel_dp_aux_fini?

Yes we do, that's part of the new intel_dp_encoder_flush_work() func. Had to split up intel_dp_encoder_destroy(), since we want to flush any pending work on the encoder first (for instance MST specific stuff) then disconnect the TC PHY, then do the DRM core cleanup and kfree. Probably should've been in the commit log.

Quoted 159 lines

drivers/gpu/drm/i915/intel_ddi.c +13 -3

+	kfree(dig_port);
+}
+
 static const struct drm_encoder_funcs intel_ddi_funcs = {
-	.reset = intel_dp_encoder_reset,
-	.destroy = intel_dp_encoder_destroy,
+	.reset = intel_ddi_encoder_reset,
+	.destroy = intel_ddi_encoder_destroy,
 };
 
 static struct intel_connector *

@@ -4197,7 +4238,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->post_disable = intel_ddi_post_disable;
 	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
 	intel_encoder->get_config = intel_ddi_get_config;
-	intel_encoder->suspend = intel_dp_encoder_suspend;
+	intel_encoder->suspend = intel_ddi_encoder_suspend;
 	intel_encoder->get_power_domains = intel_ddi_get_power_domains;
 	intel_encoder->type = INTEL_OUTPUT_DDI;
 	intel_encoder->power_domain = intel_port_to_power_domain(port);

@@ -4257,6 +4298,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
 		if (!intel_ddi_init_hdmi_connector(intel_dig_port))
 			goto err;
+
+		if (intel_port_is_tc(dev_priv, port))
+			intel_dig_port->tc_legacy_port = true;
 	}
 
 	if (init_lspcon) {

@@ -4274,6 +4318,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	}
 
 	intel_infoframe_init(intel_dig_port);
+
+	if (intel_port_is_tc(dev_priv, port))
+		intel_digital_port_connected(intel_encoder);
+
 	return;
 
 err:

drivers/gpu/drm/i915/intel_dp.c +20 -9

index 082594bb65a7..19e49adab548 100644
@@ -5108,9 +5108,6 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 			      tc_type_name(intel_dig_port->tc_type));
 }
 
-static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
-				  struct intel_digital_port *dig_port);
-
 /*
  * This function implements the first part of the Connect Flow described by our
  * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading

@@ -5145,6 +5142,7 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
 	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);
+		WARN_ON(dig_port->tc_legacy_port);
 		return false;
 	}

@@ -5176,8 +5174,8 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
  * 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)
+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);

@@ -5222,7 +5220,8 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	bool is_legacy, is_typec, is_tbt;
 	u32 dpsp;
 
-	is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
+	is_legacy = intel_dig_port->tc_legacy_port ||
+		I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
 
 	/*
 	 * The spec says we shouldn't be using the ISR bits for detecting

@@ -5233,7 +5232,14 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
 
 	if (!is_legacy && !is_typec && !is_tbt) {
-		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+		/*
+		 * We disconnect from legacy mode only during system
+		 * suspend and driver unloading, otherwise we keep the port in
+		 * legacy mode even after an HPD disconnect event.
+		 */
+		if (intel_dig_port->tc_type != TC_PORT_LEGACY)
+			icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+
 		return false;
 	}

@@ -5542,7 +5548,7 @@ intel_dp_connector_unregister(struct drm_connector *connector)
 	intel_connector_unregister(connector);
 }
 
-void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;

@@ -5565,9 +5571,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	}
 
 	intel_dp_aux_fini(intel_dp);
+}
+
+static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+{
+	intel_dp_encoder_flush_work(encoder);
 
 	drm_encoder_cleanup(encoder);
-	kfree(intel_dig_port);
+	kfree(enc_to_dig_port(encoder));
 }
 
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)

drivers/gpu/drm/i915/intel_drv.h +4 -1

index d08f08f607dd..97c422bea7fb 100644
@@ -1234,6 +1234,7 @@ struct intel_digital_port {
 	/* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */
 	enum aux_ch aux_ch;
 	enum intel_display_power_domain ddi_io_power_domain;
+	bool tc_legacy_port:1;
 	enum tc_port_type tc_type;
 
 	void (*write_infoframe)(struct intel_encoder *encoder,

@@ -1806,7 +1807,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 					   bool enable);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
-void intel_dp_encoder_destroy(struct drm_encoder *encoder);
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config,
 			     struct drm_connector_state *conn_state);

@@ -1874,6 +1875,8 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
+			   struct intel_digital_port *dig_port);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);

Signature · 1 line

2.13.2

Quoted 4 lines

Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi

maintainer

2018-12-14 22:22 UTC #6 lore raw

Re: [PATCH 2/3] drm/i915/icl: Fix TypeC legacy HDMI HPD handling

Quoted 105 lines · Imre Deak

Quoted 95 lines · Rodrigo Vivi
Quoted 94 lines · Imre Deak
Atm HPD disconnect events on TypeC ports will break things, since we'll switch the TypeC mode (between Legacy and disconnected modes as well as among USB DP alternate, Thunderbolt alternate and disconnected modes) on the fly from the HPD disconnect interrupt work while the port may be still active.
Even if the port happens to be not active during the disconnect we'd still have a problem during a subsequent modeset or AUX transfer that could happen regardless of the port's connected state. For instance the system resume display mode restore code and userspace could perform a modeset on the port or userspace could start an AUX transfer even if the port is in disconnected state.
To fix this keep legacy TypeC ports in TypeC legacy mode whenever we're not suspended. The legacy mode is a static configuration as opposed to the Thunderbolt and USB DP alternate modes between which we can switch dynamically.
We don't have yet an explicit way to detect TypeC legacy ports, but we can imply that at least in case of HDMI enabled ports, since HDMI can only be provided in legacy mode. So mark TypeC HDMI enabled ports as legacy and run the TypeC PHY connect sequence explicitly during driver loading and system resume. The connect will succeed even if the display is not connected to begin with (or disappears during the suspended state) since for legacy ports the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag is always set (as opposed to the USB DP alternate mode where it gets set only when a display is connected).
Correspondingly run the TypeC PHY disconnect sequence during system suspend and driver unloading, but disconnect during suspend only for legacy TypeC mode. We will need to disconnect even in USB DP alternate mode in the future, but atm we don't have a way to reconnect the port in this mode during resume if the display disappears while being suspended. So for now punt on this case.
Note that we do not disconnect the port during runtime suspend; in legacy mode there are no shared HW resources (PHY lanes) with other HW blocks (USB), so no need to release / reacquire these resources as with USB DP alternate mode. The only reason to disconnect legacy ports during system suspend is that the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag must be rechecked and the port must be connected again during system resume. We may also need to turn the check for this flag into a poll, depending on further tests and clarifications we are expecting from HW/FW people.
If VBT says that the port provides only DP functionality then we can't assume that it's a legacy port as for HDMI (since it could as well be a TBT/DP Alt connector), so we'll solve HPD handling for the DP case with a different method in the next patch.
Eventually - instead of the above method - we'll depend on an explicit detection method provided either via a VBT flag or a FW/HW register both for the HDMI and DP case.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108070
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108924
Cc Paulo Zanoni <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc José Roberto de Souza <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Signed-off-by Imre Deak <[email protected]>
---
 drivers/gpu/drm/i915/intel_ddi.c | 54 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp.c  | 29 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  5 +++-
 3 files changed, 75 insertions(+), 13 deletions(-)
drivers/gpu/drm/i915/intel_ddi.c +19 -0
index f3e1d6a0b7dd..2e47ffa1c95a 100644
@@ -3901,9 +3901,50 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 
 }
 
+static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+
+	intel_dp_encoder_suspend(encoder);
+
+	/*
+	 * TODO: disconnect also from USB DP alternate mode once we have a
+	 * way to handle the modeset restore in that mode during resume
+	 * even if the sink has disappeared while being suspended.
+	 */
+	if (dig_port->tc_legacy_port)
+		icl_tc_phy_disconnect(i915, dig_port);
+}
+
+static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(drm_encoder);
Quoted 8 lines
drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3928
Quoted 6 lines
drivers/gpu/drm/i915/intel_ddi.c +6 -0
+	struct drm_i915_private *i915 = to_i915(drm_encoder->dev);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		intel_digital_port_connected(&dig_port->base);
+
+	intel_dp_encoder_reset(drm_encoder);
do we need all of that?

Quoted 2 lines

You mean if we need to call intel_dp_encoder_reset()? That was the original state, as before this change we had

yes.

Quoted 5 lines

.reset = intel_dp_encoder_reset
in intel_ddi_funcs.

Quoted 8 lines

Quoted 5 lines
or could simply
intel_dp->reset_link_params = true;
instead?!
Not sure how we could avoid calling intel_dp_encoder_reset() here, it has required LSPCON and eDP specific parts.

I had in my mind the type_c only, sorry.

But now what I don't understand is why we don't need to do the same and call intel_dp_encoder_destroy from inside intel_ddi_encoder_destroy. it seems that we will loose some LSPCON and eDP there as well...

Quoted 184 lines

Quoted 18 lines

Quoted 5 lines
drivers/gpu/drm/i915/intel_ddi.c +3 -0
+}
+
+static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3941
Quoted 10 lines
drivers/gpu/drm/i915/intel_ddi.c +10 -0
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	struct drm_i915_private *i915 = to_i915(encoder->dev);
+
+	intel_dp_encoder_flush_work(encoder);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		icl_tc_phy_disconnect(i915, dig_port);
+
+	drm_encoder_cleanup(encoder);
and here don't we need stuff on intel_dp_encoder destroy here like intel_dp_aux_fini?

Quoted 159 lines

drivers/gpu/drm/i915/intel_ddi.c +13 -3

+	kfree(dig_port);
+}
+
 static const struct drm_encoder_funcs intel_ddi_funcs = {
-	.reset = intel_dp_encoder_reset,
-	.destroy = intel_dp_encoder_destroy,
+	.reset = intel_ddi_encoder_reset,
+	.destroy = intel_ddi_encoder_destroy,
 };
 
 static struct intel_connector *

@@ -4197,7 +4238,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->post_disable = intel_ddi_post_disable;
 	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
 	intel_encoder->get_config = intel_ddi_get_config;
-	intel_encoder->suspend = intel_dp_encoder_suspend;
+	intel_encoder->suspend = intel_ddi_encoder_suspend;
 	intel_encoder->get_power_domains = intel_ddi_get_power_domains;
 	intel_encoder->type = INTEL_OUTPUT_DDI;
 	intel_encoder->power_domain = intel_port_to_power_domain(port);

@@ -4257,6 +4298,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
 		if (!intel_ddi_init_hdmi_connector(intel_dig_port))
 			goto err;
+
+		if (intel_port_is_tc(dev_priv, port))
+			intel_dig_port->tc_legacy_port = true;
 	}
 
 	if (init_lspcon) {

@@ -4274,6 +4318,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	}
 
 	intel_infoframe_init(intel_dig_port);
+
+	if (intel_port_is_tc(dev_priv, port))
+		intel_digital_port_connected(intel_encoder);
+
 	return;
 
 err:

drivers/gpu/drm/i915/intel_dp.c +20 -9

index 082594bb65a7..19e49adab548 100644
@@ -5108,9 +5108,6 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 			      tc_type_name(intel_dig_port->tc_type));
 }
 
-static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
-				  struct intel_digital_port *dig_port);
-
 /*
  * This function implements the first part of the Connect Flow described by our
  * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading

@@ -5145,6 +5142,7 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
 	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);
+		WARN_ON(dig_port->tc_legacy_port);
 		return false;
 	}

@@ -5176,8 +5174,8 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
  * 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)
+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);

@@ -5222,7 +5220,8 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	bool is_legacy, is_typec, is_tbt;
 	u32 dpsp;
 
-	is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
+	is_legacy = intel_dig_port->tc_legacy_port ||
+		I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
 
 	/*
 	 * The spec says we shouldn't be using the ISR bits for detecting

@@ -5233,7 +5232,14 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
 
 	if (!is_legacy && !is_typec && !is_tbt) {
-		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+		/*
+		 * We disconnect from legacy mode only during system
+		 * suspend and driver unloading, otherwise we keep the port in
+		 * legacy mode even after an HPD disconnect event.
+		 */
+		if (intel_dig_port->tc_type != TC_PORT_LEGACY)
+			icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+
 		return false;
 	}

@@ -5542,7 +5548,7 @@ intel_dp_connector_unregister(struct drm_connector *connector)
 	intel_connector_unregister(connector);
 }
 
-void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;

@@ -5565,9 +5571,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	}
 
 	intel_dp_aux_fini(intel_dp);
+}
+
+static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+{
+	intel_dp_encoder_flush_work(encoder);
 
 	drm_encoder_cleanup(encoder);
-	kfree(intel_dig_port);
+	kfree(enc_to_dig_port(encoder));
 }
 
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)

drivers/gpu/drm/i915/intel_drv.h +4 -1

index d08f08f607dd..97c422bea7fb 100644
@@ -1234,6 +1234,7 @@ struct intel_digital_port {
 	/* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */
 	enum aux_ch aux_ch;
 	enum intel_display_power_domain ddi_io_power_domain;
+	bool tc_legacy_port:1;
 	enum tc_port_type tc_type;
 
 	void (*write_infoframe)(struct intel_encoder *encoder,

@@ -1806,7 +1807,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 					   bool enable);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
-void intel_dp_encoder_destroy(struct drm_encoder *encoder);
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config,
 			     struct drm_connector_state *conn_state);

@@ -1874,6 +1875,8 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
+			   struct intel_digital_port *dig_port);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);

Signature · 1 line

2.13.2

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Imre Deak

author

2018-12-14 23:25 UTC #7 lore raw

Re: [PATCH 2/3] drm/i915/icl: Fix TypeC legacy HDMI HPD handling

Quoted 122 lines · Rodrigo Vivi

Quoted 105 lines · Imre Deak
Quoted 95 lines · Rodrigo Vivi
Quoted 94 lines · Imre Deak
Atm HPD disconnect events on TypeC ports will break things, since we'll switch the TypeC mode (between Legacy and disconnected modes as well as among USB DP alternate, Thunderbolt alternate and disconnected modes) on the fly from the HPD disconnect interrupt work while the port may be still active.
Even if the port happens to be not active during the disconnect we'd still have a problem during a subsequent modeset or AUX transfer that could happen regardless of the port's connected state. For instance the system resume display mode restore code and userspace could perform a modeset on the port or userspace could start an AUX transfer even if the port is in disconnected state.
To fix this keep legacy TypeC ports in TypeC legacy mode whenever we're not suspended. The legacy mode is a static configuration as opposed to the Thunderbolt and USB DP alternate modes between which we can switch dynamically.
We don't have yet an explicit way to detect TypeC legacy ports, but we can imply that at least in case of HDMI enabled ports, since HDMI can only be provided in legacy mode. So mark TypeC HDMI enabled ports as legacy and run the TypeC PHY connect sequence explicitly during driver loading and system resume. The connect will succeed even if the display is not connected to begin with (or disappears during the suspended state) since for legacy ports the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag is always set (as opposed to the USB DP alternate mode where it gets set only when a display is connected).
Correspondingly run the TypeC PHY disconnect sequence during system suspend and driver unloading, but disconnect during suspend only for legacy TypeC mode. We will need to disconnect even in USB DP alternate mode in the future, but atm we don't have a way to reconnect the port in this mode during resume if the display disappears while being suspended. So for now punt on this case.
Note that we do not disconnect the port during runtime suspend; in legacy mode there are no shared HW resources (PHY lanes) with other HW blocks (USB), so no need to release / reacquire these resources as with USB DP alternate mode. The only reason to disconnect legacy ports during system suspend is that the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag must be rechecked and the port must be connected again during system resume. We may also need to turn the check for this flag into a poll, depending on further tests and clarifications we are expecting from HW/FW people.
If VBT says that the port provides only DP functionality then we can't assume that it's a legacy port as for HDMI (since it could as well be a TBT/DP Alt connector), so we'll solve HPD handling for the DP case with a different method in the next patch.
Eventually - instead of the above method - we'll depend on an explicit detection method provided either via a VBT flag or a FW/HW register both for the HDMI and DP case.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108070
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108924
Cc Paulo Zanoni <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc José Roberto de Souza <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Signed-off-by Imre Deak <[email protected]>
---
 drivers/gpu/drm/i915/intel_ddi.c | 54 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp.c  | 29 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  5 +++-
 3 files changed, 75 insertions(+), 13 deletions(-)
drivers/gpu/drm/i915/intel_ddi.c +19 -0
index f3e1d6a0b7dd..2e47ffa1c95a 100644
@@ -3901,9 +3901,50 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 
 }
 
+static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+
+	intel_dp_encoder_suspend(encoder);
+
+	/*
+	 * TODO: disconnect also from USB DP alternate mode once we have a
+	 * way to handle the modeset restore in that mode during resume
+	 * even if the sink has disappeared while being suspended.
+	 */
+	if (dig_port->tc_legacy_port)
+		icl_tc_phy_disconnect(i915, dig_port);
+}
+
+static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(drm_encoder);
Quoted 8 lines
drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3928
Quoted 6 lines
drivers/gpu/drm/i915/intel_ddi.c +6 -0
+	struct drm_i915_private *i915 = to_i915(drm_encoder->dev);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		intel_digital_port_connected(&dig_port->base);
+
+	intel_dp_encoder_reset(drm_encoder);
do we need all of that?
Quoted 2 lines
You mean if we need to call intel_dp_encoder_reset()? That was the original state, as before this change we had
yes.
Quoted 11 lines
.reset = intel_dp_encoder_reset
in intel_ddi_funcs.
Quoted 5 lines
or could simply
intel_dp->reset_link_params = true;
instead?!

Quoted 8 lines

Quoted 2 lines
Not sure how we could avoid calling intel_dp_encoder_reset() here, it has required LSPCON and eDP specific parts.
I had in my mind the type_c only, sorry.
But now what I don't understand is why we don't need to do the same and call intel_dp_encoder_destroy from inside intel_ddi_encoder_destroy. it seems that we will loose some LSPCON and eDP there as well...

That stuff moved to intel_dp_encoder_flush_work() as explained below, which is called from intel_ddi_encoder_destroy().

Quoted 185 lines

Quoted 19 lines

Quoted 6 lines
drivers/gpu/drm/i915/intel_ddi.c +3 -0
+}
+
+static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3941
Quoted 10 lines
drivers/gpu/drm/i915/intel_ddi.c +10 -0
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	struct drm_i915_private *i915 = to_i915(encoder->dev);
+
+	intel_dp_encoder_flush_work(encoder);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		icl_tc_phy_disconnect(i915, dig_port);
+
+	drm_encoder_cleanup(encoder);
and here don't we need stuff on intel_dp_encoder destroy here like intel_dp_aux_fini?

Quoted 159 lines

drivers/gpu/drm/i915/intel_ddi.c +13 -3

+	kfree(dig_port);
+}
+
 static const struct drm_encoder_funcs intel_ddi_funcs = {
-	.reset = intel_dp_encoder_reset,
-	.destroy = intel_dp_encoder_destroy,
+	.reset = intel_ddi_encoder_reset,
+	.destroy = intel_ddi_encoder_destroy,
 };
 
 static struct intel_connector *

@@ -4197,7 +4238,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->post_disable = intel_ddi_post_disable;
 	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
 	intel_encoder->get_config = intel_ddi_get_config;
-	intel_encoder->suspend = intel_dp_encoder_suspend;
+	intel_encoder->suspend = intel_ddi_encoder_suspend;
 	intel_encoder->get_power_domains = intel_ddi_get_power_domains;
 	intel_encoder->type = INTEL_OUTPUT_DDI;
 	intel_encoder->power_domain = intel_port_to_power_domain(port);

@@ -4257,6 +4298,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
 		if (!intel_ddi_init_hdmi_connector(intel_dig_port))
 			goto err;
+
+		if (intel_port_is_tc(dev_priv, port))
+			intel_dig_port->tc_legacy_port = true;
 	}
 
 	if (init_lspcon) {

@@ -4274,6 +4318,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	}
 
 	intel_infoframe_init(intel_dig_port);
+
+	if (intel_port_is_tc(dev_priv, port))
+		intel_digital_port_connected(intel_encoder);
+
 	return;
 
 err:

drivers/gpu/drm/i915/intel_dp.c +20 -9

index 082594bb65a7..19e49adab548 100644
@@ -5108,9 +5108,6 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 			      tc_type_name(intel_dig_port->tc_type));
 }
 
-static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
-				  struct intel_digital_port *dig_port);
-
 /*
  * This function implements the first part of the Connect Flow described by our
  * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading

@@ -5145,6 +5142,7 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
 	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);
+		WARN_ON(dig_port->tc_legacy_port);
 		return false;
 	}

@@ -5176,8 +5174,8 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
  * 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)
+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);

@@ -5222,7 +5220,8 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	bool is_legacy, is_typec, is_tbt;
 	u32 dpsp;
 
-	is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
+	is_legacy = intel_dig_port->tc_legacy_port ||
+		I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
 
 	/*
 	 * The spec says we shouldn't be using the ISR bits for detecting

@@ -5233,7 +5232,14 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
 
 	if (!is_legacy && !is_typec && !is_tbt) {
-		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+		/*
+		 * We disconnect from legacy mode only during system
+		 * suspend and driver unloading, otherwise we keep the port in
+		 * legacy mode even after an HPD disconnect event.
+		 */
+		if (intel_dig_port->tc_type != TC_PORT_LEGACY)
+			icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+
 		return false;
 	}

@@ -5542,7 +5548,7 @@ intel_dp_connector_unregister(struct drm_connector *connector)
 	intel_connector_unregister(connector);
 }
 
-void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;

@@ -5565,9 +5571,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	}
 
 	intel_dp_aux_fini(intel_dp);
+}
+
+static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+{
+	intel_dp_encoder_flush_work(encoder);
 
 	drm_encoder_cleanup(encoder);
-	kfree(intel_dig_port);
+	kfree(enc_to_dig_port(encoder));
 }
 
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)

drivers/gpu/drm/i915/intel_drv.h +4 -1

index d08f08f607dd..97c422bea7fb 100644
@@ -1234,6 +1234,7 @@ struct intel_digital_port {
 	/* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */
 	enum aux_ch aux_ch;
 	enum intel_display_power_domain ddi_io_power_domain;
+	bool tc_legacy_port:1;
 	enum tc_port_type tc_type;
 
 	void (*write_infoframe)(struct intel_encoder *encoder,

@@ -1806,7 +1807,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 					   bool enable);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
-void intel_dp_encoder_destroy(struct drm_encoder *encoder);
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config,
 			     struct drm_connector_state *conn_state);

@@ -1874,6 +1875,8 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
+			   struct intel_digital_port *dig_port);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);

Signature · 1 line

2.13.2

Quoted 8 lines

Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi

maintainer Reviewed-by

2018-12-17 07:05 UTC #8 lore raw

Re: [PATCH 2/3] drm/i915/icl: Fix TypeC legacy HDMI HPD handling

Quoted 126 lines · Imre Deak

Quoted 125 lines · Rodrigo Vivi
Quoted 105 lines · Imre Deak
Quoted 95 lines · Rodrigo Vivi
Quoted 94 lines · Imre Deak
Atm HPD disconnect events on TypeC ports will break things, since we'll switch the TypeC mode (between Legacy and disconnected modes as well as among USB DP alternate, Thunderbolt alternate and disconnected modes) on the fly from the HPD disconnect interrupt work while the port may be still active.
Even if the port happens to be not active during the disconnect we'd still have a problem during a subsequent modeset or AUX transfer that could happen regardless of the port's connected state. For instance the system resume display mode restore code and userspace could perform a modeset on the port or userspace could start an AUX transfer even if the port is in disconnected state.
To fix this keep legacy TypeC ports in TypeC legacy mode whenever we're not suspended. The legacy mode is a static configuration as opposed to the Thunderbolt and USB DP alternate modes between which we can switch dynamically.
We don't have yet an explicit way to detect TypeC legacy ports, but we can imply that at least in case of HDMI enabled ports, since HDMI can only be provided in legacy mode. So mark TypeC HDMI enabled ports as legacy and run the TypeC PHY connect sequence explicitly during driver loading and system resume. The connect will succeed even if the display is not connected to begin with (or disappears during the suspended state) since for legacy ports the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag is always set (as opposed to the USB DP alternate mode where it gets set only when a display is connected).
Correspondingly run the TypeC PHY disconnect sequence during system suspend and driver unloading, but disconnect during suspend only for legacy TypeC mode. We will need to disconnect even in USB DP alternate mode in the future, but atm we don't have a way to reconnect the port in this mode during resume if the display disappears while being suspended. So for now punt on this case.
Note that we do not disconnect the port during runtime suspend; in legacy mode there are no shared HW resources (PHY lanes) with other HW blocks (USB), so no need to release / reacquire these resources as with USB DP alternate mode. The only reason to disconnect legacy ports during system suspend is that the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag must be rechecked and the port must be connected again during system resume. We may also need to turn the check for this flag into a poll, depending on further tests and clarifications we are expecting from HW/FW people.
If VBT says that the port provides only DP functionality then we can't assume that it's a legacy port as for HDMI (since it could as well be a TBT/DP Alt connector), so we'll solve HPD handling for the DP case with a different method in the next patch.
Eventually - instead of the above method - we'll depend on an explicit detection method provided either via a VBT flag or a FW/HW register both for the HDMI and DP case.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108070
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108924
Cc Paulo Zanoni <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc José Roberto de Souza <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Signed-off-by Imre Deak <[email protected]>
---
 drivers/gpu/drm/i915/intel_ddi.c | 54 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp.c  | 29 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  5 +++-
 3 files changed, 75 insertions(+), 13 deletions(-)
drivers/gpu/drm/i915/intel_ddi.c +19 -0
index f3e1d6a0b7dd..2e47ffa1c95a 100644
@@ -3901,9 +3901,50 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 
 }
 
+static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+
+	intel_dp_encoder_suspend(encoder);
+
+	/*
+	 * TODO: disconnect also from USB DP alternate mode once we have a
+	 * way to handle the modeset restore in that mode during resume
+	 * even if the sink has disappeared while being suspended.
+	 */
+	if (dig_port->tc_legacy_port)
+		icl_tc_phy_disconnect(i915, dig_port);
+}
+
+static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(drm_encoder);
Quoted 8 lines
drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3928
Quoted 6 lines
drivers/gpu/drm/i915/intel_ddi.c +6 -0
+	struct drm_i915_private *i915 = to_i915(drm_encoder->dev);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		intel_digital_port_connected(&dig_port->base);
+
+	intel_dp_encoder_reset(drm_encoder);
do we need all of that?
Quoted 2 lines
You mean if we need to call intel_dp_encoder_reset()? That was the original state, as before this change we had
yes.
Quoted 13 lines
.reset = intel_dp_encoder_reset
in intel_ddi_funcs.
Quoted 5 lines
or could simply
intel_dp->reset_link_params = true;
instead?!
Not sure how we could avoid calling intel_dp_encoder_reset() here, it has required LSPCON and eDP specific parts.

Quoted 8 lines

Quoted 5 lines
I had in my mind the type_c only, sorry.
But now what I don't understand is why we don't need to do the same and call intel_dp_encoder_destroy from inside intel_ddi_encoder_destroy. it seems that we will loose some LSPCON and eDP there as well...
That stuff moved to intel_dp_encoder_flush_work() as explained below, which is called from intel_ddi_encoder_destroy().

hmmm, now I saw this part of the move. sorry...
I think the non-symmetric name got a bit confusing, but the code is indeed right, so:

Reviewed-by Rodrigo Vivi <[email protected]>

Quoted 186 lines

Quoted 4 lines

Quoted 16 lines

drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3941
Quoted 13 lines
drivers/gpu/drm/i915/intel_ddi.c +13 -0
+}
+
+static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	struct drm_i915_private *i915 = to_i915(encoder->dev);
+
+	intel_dp_encoder_flush_work(encoder);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		icl_tc_phy_disconnect(i915, dig_port);
+
+	drm_encoder_cleanup(encoder);
and here don't we need stuff on intel_dp_encoder destroy here like intel_dp_aux_fini?

Quoted 159 lines

drivers/gpu/drm/i915/intel_ddi.c +13 -3

+	kfree(dig_port);
+}
+
 static const struct drm_encoder_funcs intel_ddi_funcs = {
-	.reset = intel_dp_encoder_reset,
-	.destroy = intel_dp_encoder_destroy,
+	.reset = intel_ddi_encoder_reset,
+	.destroy = intel_ddi_encoder_destroy,
 };
 
 static struct intel_connector *

@@ -4197,7 +4238,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->post_disable = intel_ddi_post_disable;
 	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
 	intel_encoder->get_config = intel_ddi_get_config;
-	intel_encoder->suspend = intel_dp_encoder_suspend;
+	intel_encoder->suspend = intel_ddi_encoder_suspend;
 	intel_encoder->get_power_domains = intel_ddi_get_power_domains;
 	intel_encoder->type = INTEL_OUTPUT_DDI;
 	intel_encoder->power_domain = intel_port_to_power_domain(port);

@@ -4257,6 +4298,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
 		if (!intel_ddi_init_hdmi_connector(intel_dig_port))
 			goto err;
+
+		if (intel_port_is_tc(dev_priv, port))
+			intel_dig_port->tc_legacy_port = true;
 	}
 
 	if (init_lspcon) {

@@ -4274,6 +4318,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	}
 
 	intel_infoframe_init(intel_dig_port);
+
+	if (intel_port_is_tc(dev_priv, port))
+		intel_digital_port_connected(intel_encoder);
+
 	return;
 
 err:

drivers/gpu/drm/i915/intel_dp.c +20 -9

index 082594bb65a7..19e49adab548 100644
@@ -5108,9 +5108,6 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 			      tc_type_name(intel_dig_port->tc_type));
 }
 
-static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
-				  struct intel_digital_port *dig_port);
-
 /*
  * This function implements the first part of the Connect Flow described by our
  * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading

@@ -5145,6 +5142,7 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
 	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);
+		WARN_ON(dig_port->tc_legacy_port);
 		return false;
 	}

@@ -5176,8 +5174,8 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
  * 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)
+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);

@@ -5222,7 +5220,8 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	bool is_legacy, is_typec, is_tbt;
 	u32 dpsp;
 
-	is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
+	is_legacy = intel_dig_port->tc_legacy_port ||
+		I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
 
 	/*
 	 * The spec says we shouldn't be using the ISR bits for detecting

@@ -5233,7 +5232,14 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
 
 	if (!is_legacy && !is_typec && !is_tbt) {
-		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+		/*
+		 * We disconnect from legacy mode only during system
+		 * suspend and driver unloading, otherwise we keep the port in
+		 * legacy mode even after an HPD disconnect event.
+		 */
+		if (intel_dig_port->tc_type != TC_PORT_LEGACY)
+			icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+
 		return false;
 	}

@@ -5542,7 +5548,7 @@ intel_dp_connector_unregister(struct drm_connector *connector)
 	intel_connector_unregister(connector);
 }
 
-void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;

@@ -5565,9 +5571,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	}
 
 	intel_dp_aux_fini(intel_dp);
+}
+
+static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+{
+	intel_dp_encoder_flush_work(encoder);
 
 	drm_encoder_cleanup(encoder);
-	kfree(intel_dig_port);
+	kfree(enc_to_dig_port(encoder));
 }
 
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)

drivers/gpu/drm/i915/intel_drv.h +4 -1

index d08f08f607dd..97c422bea7fb 100644
@@ -1234,6 +1234,7 @@ struct intel_digital_port {
 	/* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */
 	enum aux_ch aux_ch;
 	enum intel_display_power_domain ddi_io_power_domain;
+	bool tc_legacy_port:1;
 	enum tc_port_type tc_type;
 
 	void (*write_infoframe)(struct intel_encoder *encoder,

@@ -1806,7 +1807,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 					   bool enable);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
-void intel_dp_encoder_destroy(struct drm_encoder *encoder);
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config,
 			     struct drm_connector_state *conn_state);

@@ -1874,6 +1875,8 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
+			   struct intel_digital_port *dig_port);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);

Signature · 1 line

2.13.2

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Imre Deak

author

2018-12-17 17:38 UTC #9 lore raw

Re: [PATCH 2/3] drm/i915/icl: Fix TypeC legacy HDMI HPD handling

Quoted 133 lines · Rodrigo Vivi

Quoted 132 lines · Imre Deak
Quoted 130 lines · Rodrigo Vivi
Quoted 105 lines · Imre Deak
Quoted 95 lines · Rodrigo Vivi
Quoted 94 lines · Imre Deak
Atm HPD disconnect events on TypeC ports will break things, since we'll switch the TypeC mode (between Legacy and disconnected modes as well as among USB DP alternate, Thunderbolt alternate and disconnected modes) on the fly from the HPD disconnect interrupt work while the port may be still active.
Even if the port happens to be not active during the disconnect we'd still have a problem during a subsequent modeset or AUX transfer that could happen regardless of the port's connected state. For instance the system resume display mode restore code and userspace could perform a modeset on the port or userspace could start an AUX transfer even if the port is in disconnected state.
To fix this keep legacy TypeC ports in TypeC legacy mode whenever we're not suspended. The legacy mode is a static configuration as opposed to the Thunderbolt and USB DP alternate modes between which we can switch dynamically.
We don't have yet an explicit way to detect TypeC legacy ports, but we can imply that at least in case of HDMI enabled ports, since HDMI can only be provided in legacy mode. So mark TypeC HDMI enabled ports as legacy and run the TypeC PHY connect sequence explicitly during driver loading and system resume. The connect will succeed even if the display is not connected to begin with (or disappears during the suspended state) since for legacy ports the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag is always set (as opposed to the USB DP alternate mode where it gets set only when a display is connected).
Correspondingly run the TypeC PHY disconnect sequence during system suspend and driver unloading, but disconnect during suspend only for legacy TypeC mode. We will need to disconnect even in USB DP alternate mode in the future, but atm we don't have a way to reconnect the port in this mode during resume if the display disappears while being suspended. So for now punt on this case.
Note that we do not disconnect the port during runtime suspend; in legacy mode there are no shared HW resources (PHY lanes) with other HW blocks (USB), so no need to release / reacquire these resources as with USB DP alternate mode. The only reason to disconnect legacy ports during system suspend is that the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED flag must be rechecked and the port must be connected again during system resume. We may also need to turn the check for this flag into a poll, depending on further tests and clarifications we are expecting from HW/FW people.
If VBT says that the port provides only DP functionality then we can't assume that it's a legacy port as for HDMI (since it could as well be a TBT/DP Alt connector), so we'll solve HPD handling for the DP case with a different method in the next patch.
Eventually - instead of the above method - we'll depend on an explicit detection method provided either via a VBT flag or a FW/HW register both for the HDMI and DP case.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108070
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108924
Cc Paulo Zanoni <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc José Roberto de Souza <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Signed-off-by Imre Deak <[email protected]>
---
 drivers/gpu/drm/i915/intel_ddi.c | 54 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp.c  | 29 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  5 +++-
 3 files changed, 75 insertions(+), 13 deletions(-)
drivers/gpu/drm/i915/intel_ddi.c +19 -0
index f3e1d6a0b7dd..2e47ffa1c95a 100644
@@ -3901,9 +3901,50 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 
 }
 
+static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+
+	intel_dp_encoder_suspend(encoder);
+
+	/*
+	 * TODO: disconnect also from USB DP alternate mode once we have a
+	 * way to handle the modeset restore in that mode during resume
+	 * even if the sink has disappeared while being suspended.
+	 */
+	if (dig_port->tc_legacy_port)
+		icl_tc_phy_disconnect(i915, dig_port);
+}
+
+static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(drm_encoder);
Quoted 8 lines
drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3928
Quoted 6 lines
drivers/gpu/drm/i915/intel_ddi.c +6 -0
+	struct drm_i915_private *i915 = to_i915(drm_encoder->dev);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		intel_digital_port_connected(&dig_port->base);
+
+	intel_dp_encoder_reset(drm_encoder);
do we need all of that?
Quoted 2 lines
You mean if we need to call intel_dp_encoder_reset()? That was the original state, as before this change we had
yes.
Quoted 5 lines
.reset = intel_dp_encoder_reset
in intel_ddi_funcs.
Quoted 8 lines
Quoted 5 lines
or could simply
intel_dp->reset_link_params = true;
instead?!
Not sure how we could avoid calling intel_dp_encoder_reset() here, it has required LSPCON and eDP specific parts.
I had in my mind the type_c only, sorry.
But now what I don't understand is why we don't need to do the same and call intel_dp_encoder_destroy from inside intel_ddi_encoder_destroy. it seems that we will loose some LSPCON and eDP there as well...

Quoted 5 lines

Quoted 2 lines
That stuff moved to intel_dp_encoder_flush_work() as explained below, which is called from intel_ddi_encoder_destroy().
hmmm, now I saw this part of the move. sorry... I think the non-symmetric name got a bit confusing,

hm, not sure what you mean. After this change we have

intel_dp_encoder_destroy() that pre-DDI platforms set as their encoder .destroy hook

and we have

intel_ddi_encoder_destroy() that DDI platforms set as their encoder .destroy hook.

Both intel_dp_encoder_destroy() and intel_ddi_encoder_destroy() will call in turn intel_dp_encoder_flush_work(). So not sure what part you think is non-symmetric.

Quoted 3 lines

but the code is indeed right, so:
Reviewed-by Rodrigo Vivi <[email protected]>

Thanks. I sent a v2 patchset with a minor change in this and the last patch, does the r-b still apply on those?

--Imre

Quoted 187 lines

Quoted 5 lines

Quoted 16 lines

drivers/gpu/drm/i915/intel_ddi.c static bool intel_ddi_compute_config(struct intel_encoder *encoder, · line 3941
Quoted 13 lines
drivers/gpu/drm/i915/intel_ddi.c +13 -0
+}
+
+static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
+{
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	struct drm_i915_private *i915 = to_i915(encoder->dev);
+
+	intel_dp_encoder_flush_work(encoder);
+
+	if (intel_port_is_tc(i915, dig_port->base.port))
+		icl_tc_phy_disconnect(i915, dig_port);
+
+	drm_encoder_cleanup(encoder);
and here don't we need stuff on intel_dp_encoder destroy here like intel_dp_aux_fini?

Quoted 159 lines

drivers/gpu/drm/i915/intel_ddi.c +13 -3

+	kfree(dig_port);
+}
+
 static const struct drm_encoder_funcs intel_ddi_funcs = {
-	.reset = intel_dp_encoder_reset,
-	.destroy = intel_dp_encoder_destroy,
+	.reset = intel_ddi_encoder_reset,
+	.destroy = intel_ddi_encoder_destroy,
 };
 
 static struct intel_connector *

@@ -4197,7 +4238,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->post_disable = intel_ddi_post_disable;
 	intel_encoder->get_hw_state = intel_ddi_get_hw_state;
 	intel_encoder->get_config = intel_ddi_get_config;
-	intel_encoder->suspend = intel_dp_encoder_suspend;
+	intel_encoder->suspend = intel_ddi_encoder_suspend;
 	intel_encoder->get_power_domains = intel_ddi_get_power_domains;
 	intel_encoder->type = INTEL_OUTPUT_DDI;
 	intel_encoder->power_domain = intel_port_to_power_domain(port);

@@ -4257,6 +4298,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) {
 		if (!intel_ddi_init_hdmi_connector(intel_dig_port))
 			goto err;
+
+		if (intel_port_is_tc(dev_priv, port))
+			intel_dig_port->tc_legacy_port = true;
 	}
 
 	if (init_lspcon) {

@@ -4274,6 +4318,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	}
 
 	intel_infoframe_init(intel_dig_port);
+
+	if (intel_port_is_tc(dev_priv, port))
+		intel_digital_port_connected(intel_encoder);
+
 	return;
 
 err:

drivers/gpu/drm/i915/intel_dp.c +20 -9

index 082594bb65a7..19e49adab548 100644
@@ -5108,9 +5108,6 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
 			      tc_type_name(intel_dig_port->tc_type));
 }
 
-static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
-				  struct intel_digital_port *dig_port);
-
 /*
  * This function implements the first part of the Connect Flow described by our
  * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading

@@ -5145,6 +5142,7 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
 	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);
+		WARN_ON(dig_port->tc_legacy_port);
 		return false;
 	}

@@ -5176,8 +5174,8 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
  * 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)
+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);

@@ -5222,7 +5220,8 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	bool is_legacy, is_typec, is_tbt;
 	u32 dpsp;
 
-	is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
+	is_legacy = intel_dig_port->tc_legacy_port ||
+		I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
 
 	/*
 	 * The spec says we shouldn't be using the ISR bits for detecting

@@ -5233,7 +5232,14 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
 
 	if (!is_legacy && !is_typec && !is_tbt) {
-		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+		/*
+		 * We disconnect from legacy mode only during system
+		 * suspend and driver unloading, otherwise we keep the port in
+		 * legacy mode even after an HPD disconnect event.
+		 */
+		if (intel_dig_port->tc_type != TC_PORT_LEGACY)
+			icl_tc_phy_disconnect(dev_priv, intel_dig_port);
+
 		return false;
 	}

@@ -5542,7 +5548,7 @@ intel_dp_connector_unregister(struct drm_connector *connector)
 	intel_connector_unregister(connector);
 }
 
-void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder)
 {
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;

@@ -5565,9 +5571,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	}
 
 	intel_dp_aux_fini(intel_dp);
+}
+
+static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
+{
+	intel_dp_encoder_flush_work(encoder);
 
 	drm_encoder_cleanup(encoder);
-	kfree(intel_dig_port);
+	kfree(enc_to_dig_port(encoder));
 }
 
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)

drivers/gpu/drm/i915/intel_drv.h +4 -1

index d08f08f607dd..97c422bea7fb 100644
@@ -1234,6 +1234,7 @@ struct intel_digital_port {
 	/* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */
 	enum aux_ch aux_ch;
 	enum intel_display_power_domain ddi_io_power_domain;
+	bool tc_legacy_port:1;
 	enum tc_port_type tc_type;
 
 	void (*write_infoframe)(struct intel_encoder *encoder,

@@ -1806,7 +1807,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 					   bool enable);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
-void intel_dp_encoder_destroy(struct drm_encoder *encoder);
+void intel_dp_encoder_flush_work(struct drm_encoder *encoder);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config,
 			     struct drm_connector_state *conn_state);

@@ -1874,6 +1875,8 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
+void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
+			   struct intel_digital_port *dig_port);
 
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);

Signature · 1 line

2.13.2

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Imre Deak

author

2018-12-13 19:48 UTC #10 lore raw

PATCH 3/3 drm/i915/icl: Fix TypeC legacy DP HPD handling

In reply to Imre Deak

TypeC legacy DP ports can't be implied the same way we implied TypeC legacy HDMI ports in the previous patch. So that we still have functioning DP legacy ports, mark them as legacy at the first connect event. After that we treat the port the same way as in the HDMI case, that is keep it in legacy mode whenever we are not suspended.

Eventually - instead of the methods in this and the previous patch - we'll depend on an explicit way to detect both HDMI and DP TypeC legacy ports either via a VBT option or a HW/FW register.

Cc Paulo Zanoni <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc José Roberto de Souza <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Signed-off-by Imre Deak <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +7 -1

index 19e49adab548..f5de0d079ab5 100644
@@ -5220,8 +5220,14 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	bool is_legacy, is_typec, is_tbt;
 	u32 dpsp;
 
-	is_legacy = intel_dig_port->tc_legacy_port ||
+	/*
+	 * TODO: Depend only on the tc_legacy_port flag to identify legacy
+	 * ports, once we have an explicit detection method for legacy mode
+	 * (via VBT or a HW/FW register).
+	 */
+	intel_dig_port->tc_legacy_port |=
 		I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
+	is_legacy = intel_dig_port->tc_legacy_port;
 
 	/*
 	 * The spec says we shouldn't be using the ISR bits for detecting

Signature · 6 lines

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi

maintainer Reviewed-by on v1 3/3

2018-12-13 21:08 UTC #11 lore raw

Re: [PATCH 3/3] drm/i915/icl: Fix TypeC legacy DP HPD handling

Quoted 10 lines · Imre Deak

TypeC legacy DP ports can't be implied the same way we implied TypeC legacy HDMI ports in the previous patch. So that we still have functioning DP legacy ports, mark them as legacy at the first connect event. After that we treat the port the same way as in the HDMI case, that is keep it in legacy mode whenever we are not suspended.
Eventually - instead of the methods in this and the previous patch - we'll depend on an explicit way to detect both HDMI and DP TypeC legacy ports either via a VBT option or a HW/FW register.

Quoted 6 lines

Suggested-by Ville Syrjälä <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc José Roberto de Souza <[email protected]>
Cc Rodrigo Vivi <[email protected]>
Signed-off-by Imre Deak <[email protected]>

Reviewed-by Rodrigo Vivi <[email protected]>

Quoted 31 lines

---
 drivers/gpu/drm/i915/intel_dp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +7 -1

index 19e49adab548..f5de0d079ab5 100644
@@ -5220,8 +5220,14 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
 	bool is_legacy, is_typec, is_tbt;
 	u32 dpsp;
 
-	is_legacy = intel_dig_port->tc_legacy_port ||
+	/*
+	 * TODO: Depend only on the tc_legacy_port flag to identify legacy
+	 * ports, once we have an explicit detection method for legacy mode
+	 * (via VBT or a HW/FW register).
+	 */
+	intel_dig_port->tc_legacy_port |=
 		I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port);
+	is_legacy = intel_dig_port->tc_legacy_port;
 
 	/*
 	 * The spec says we shouldn't be using the ISR bits for detecting

Signature · 6 lines

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patchwork

2018-12-13 19:54 UTC #12 lore raw

✗ Fi.CI.CHECKPATCH: warning for drm/i915/icl: Fix TypeC legacy HPD handling

In reply to Imre Deak

== Series Details ==

Series: drm/i915/icl: Fix TypeC legacy HPD handling
URL : https://patchwork.freedesktop.org/series/54017/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
865ff59f3e7d drm/i915/icl: Add a debug print for TypeC port disconnection
-:28: WARNING:STATIC_CONST_CHAR_ARRAY: static const char * array should probably be static const char * const
#28: FILE: drivers/gpu/drm/i915/intel_dp.c:5071:
+	static const char *names[] = {

total: 0 errors, 1 warnings, 0 checks, 65 lines checked
f67b633f6c34 drm/i915/icl: Fix TypeC legacy HDMI HPD handling
-:250: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield. Prefer bool bitfields as unsigned int or u<8|16|32> #250: FILE: drivers/gpu/drm/i915/intel_drv.h:1237:
+ bool tc_legacy_port:1;

total: 0 errors, 1 warnings, 0 checks, 175 lines checked 6dcacf85c9ee drm/i915/icl: Fix TypeC legacy DP HPD handling

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patchwork

2018-12-13 20:18 UTC #13 lore raw

✗ Fi.CI.BAT: failure for drm/i915/icl: Fix TypeC legacy HPD handling

In reply to Imre Deak

== Series Details ==

Series: drm/i915/icl: Fix TypeC legacy HPD handling
URL : https://patchwork.freedesktop.org/series/54017/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5313 -> Patchwork_11091
====================================================

Summary
-------

**FAILURE**

Serious unknown changes coming with Patchwork_11091 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in Patchwork_11091, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://patchwork.freedesktop.org/api/1.0/series/54017/revisions/1/mbox/

Possible new issues
-------------------

Here are the unknown changes that may have been introduced in Patchwork_11091:

### IGT changes ###

#### Possible regressions ####

* igt@pm_rpm@basic-rte:
- fi-byt-j1900: PASS -> FAIL
- fi-bsw-kefka: PASS -> FAIL

#### Warnings ####

* igt@kms_busy@basic-flip-a:
- fi-kbl-7567u: SKIP -> PASS +2

* igt@pm_rpm@basic-pci-d3-state:
- fi-byt-j1900: PASS -> SKIP
- fi-bsw-kefka: PASS -> SKIP

Known issues
------------

Here are the changes found in Patchwork_11091 that come from known issues:

### IGT changes ###

#### Issues hit ####

* igt@gem_ctx_create@basic-files:
- fi-bsw-kefka: PASS -> FAIL [fdo#108656]

* igt@i915_selftest@live_contexts:
- fi-icl-u3: NOTRUN -> DMESG-FAIL [fdo#108569]
- fi-icl-u2: NOTRUN -> DMESG-FAIL [fdo#108569]

* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: PASS -> FAIL [fdo#108767]

* igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
- fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362] +1

#### Possible fixes ####

* igt@gem_exec_suspend@basic-s3:
- fi-blb-e6850: INCOMPLETE [fdo#107718] -> PASS

* igt@kms_chamelium@common-hpd-after-suspend:
- fi-icl-u2: DMESG-FAIL [fdo#103375] / [fdo#107732] / [fdo#108070] / [fdo#108924] -> PASS

* igt@kms_flip@basic-flip-vs-dpms:
- fi-icl-u3: DMESG-WARN [fdo#108924] / [fdo#109044] -> PASS

* igt@kms_frontbuffer_tracking@basic:
- fi-byt-clapper: FAIL [fdo#103167] -> PASS

* igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
- fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS

Participating hosts (50 -> 46)
------------------------------

Additional (2): fi-glk-dsi fi-bsw-n3050
Missing (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-y

Build changes
-------------

* Linux: CI_DRM_5313 -> Patchwork_11091

CI_DRM_5313: 539600d0a77b59892c5c29edacd2637580d094fe @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4747: ad821d1dc5d0eea4ac3a0e8e29c56c7f66191108 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_11091: 6dcacf85c9ee02659815fcfd43b9595ddf1e7f2e @ git://anongit.freedesktop.org/gfx-ci/linux

== Linux commits ==

6dcacf85c9ee drm/i915/icl: Fix TypeC legacy DP HPD handling f67b633f6c34 drm/i915/icl: Fix TypeC legacy HDMI HPD handling 865ff59f3e7d drm/i915/icl: Add a debug print for TypeC port disconnection

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11091/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx