[PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[Imre Deak]

The TypeC PHY HW readout during driver loading and system resume
determines which TypeC mode the PHY is in (legacy/DP-alt/TBT-alt) and
whether the PHY is connected, based on the PHY's Owned and Ready flags.
For the PHY to be in DP-alt or legacy mode and for the PHY to be in the
connected state in these modes, both the Owned (set by the BIOS/driver)
and the Ready (set by the HW) flags should be set.

On ICL-MTL the HW kept the PHY's Ready flag set after the driver
connected the PHY by acquiring the PHY ownership (by setting the Owned
flag), until the driver disconnected the PHY by releasing the PHY
ownership (by clearing the Owned flag). On LNL+ this has changed, in
that the HW clears the Ready flag as soon as the sink gets disconnected,
even if the PHY ownership was acquired already and hence the PHY is
being used by the display.

When inheriting the HW state from BIOS for a PHY connected in DP-alt
mode on which the sink got disconnected - i.e. in a case where the sink
was connected while BIOS/GOP was running and so the sink got enabled
connecting the PHY, but the user disconnected the sink by the time the
driver loaded - the PHY Owned but not Ready state must be accounted for
on LNL+ according to the above. Do that by assuming on LNL+ that the PHY
is connected in DP-alt mode whenever the PHY Owned flag is set,
regardless of the PHY Ready flag.

This fixes a problem on LNL+, where the PHY TypeC mode / connected state
was detected incorrectly for a DP-alt sink, which got connected and then
disconnected by the user in the above way.

Cc: stable@vger.kernel.org # v6.8+
Reported-by: Charlton Lin <charlton.lin@intel.com>
Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 3bc57579fe53e..73a08bd84a70a 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -1226,14 +1226,18 @@ static void tc_phy_get_hw_state(struct intel_tc_port *tc)
 	tc->phy_ops->get_hw_state(tc);
 }
 
-static bool tc_phy_is_ready_and_owned(struct intel_tc_port *tc,
-				      bool phy_is_ready, bool phy_is_owned)
+static bool tc_phy_in_legacy_or_dp_alt_mode(struct intel_tc_port *tc,
+					    bool phy_is_ready, bool phy_is_owned)
 {
 	struct intel_display *display = to_intel_display(tc->dig_port);
 
-	drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
+	if (DISPLAY_VER(display) < 20) {
+		drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
 
-	return phy_is_ready && phy_is_owned;
+		return phy_is_ready && phy_is_owned;
+	} else {
+		return phy_is_owned;
+	}
 }
 
 static bool tc_phy_is_connected(struct intel_tc_port *tc,
@@ -1244,7 +1248,7 @@ static bool tc_phy_is_connected(struct intel_tc_port *tc,
 	bool phy_is_owned = tc_phy_is_owned(tc);
 	bool is_connected;
 
-	if (tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned))
+	if (tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned))
 		is_connected = port_pll_type == ICL_PORT_DPLL_MG_PHY;
 	else
 		is_connected = port_pll_type == ICL_PORT_DPLL_DEFAULT;
@@ -1352,7 +1356,7 @@ tc_phy_get_current_mode(struct intel_tc_port *tc)
 	phy_is_ready = tc_phy_is_ready(tc);
 	phy_is_owned = tc_phy_is_owned(tc);
 
-	if (!tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned)) {
+	if (!tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned)) {
 		mode = get_tc_mode_in_phy_not_owned_state(tc, live_mode);
 	} else {
 		drm_WARN_ON(display->drm, live_mode == TC_PORT_TBT_ALT);
-- 
2.49.1

[PATCH 02/19] drm/i915/icl+/tc: Cache the max lane count value

[Imre Deak]

The PHY's pin assignment value in the TCSS_DDI_STATUS register - as set
by the HW/FW based on the connected DP-alt sink's TypeC/PD pin
assignment negotiation - gets cleared by the HW/FW on LNL+ as soon as
the sink gets disconnected, even if the PHY ownership got acquired
already by the driver (and hence the PHY itself is still connected and
used by the display). This is similar to how the PHY Ready flag gets
cleared on LNL+ in the same register.

To be able to query the max lane count value on LNL+ - which is based on
the above pin assignment - at all times even after the sink gets
disconnected, the max lane count must be determined and cached during
the PHY's HW readout and connect sequences. Do that here, leaving the
actual use of the cached value to a follow-up change.

Cc: stable@vger.kernel.org # v6.8+
Reported-by: Charlton Lin <charlton.lin@intel.com>
Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 48 +++++++++++++++++++++----
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 73a08bd84a70a..ea6c73af683a0 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -66,6 +66,7 @@ struct intel_tc_port {
 	enum tc_port_mode init_mode;
 	enum phy_fia phy_fia;
 	u8 phy_fia_idx;
+	u8 max_lane_count;
 };
 
 static enum intel_display_power_domain
@@ -365,12 +366,12 @@ static int intel_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
 	}
 }
 
-int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
+static int get_max_lane_count(struct intel_tc_port *tc)
 {
-	struct intel_display *display = to_intel_display(dig_port);
-	struct intel_tc_port *tc = to_tc_port(dig_port);
+	struct intel_display *display = to_intel_display(tc->dig_port);
+	struct intel_digital_port *dig_port = tc->dig_port;
 
-	if (!intel_encoder_is_tc(&dig_port->base) || tc->mode != TC_PORT_DP_ALT)
+	if (tc->mode != TC_PORT_DP_ALT)
 		return 4;
 
 	assert_tc_cold_blocked(tc);
@@ -384,6 +385,21 @@ int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
 	return intel_tc_port_get_max_lane_count(dig_port);
 }
 
+static void read_pin_configuration(struct intel_tc_port *tc)
+{
+	tc->max_lane_count = get_max_lane_count(tc);
+}
+
+int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
+{
+	struct intel_tc_port *tc = to_tc_port(dig_port);
+
+	if (!intel_encoder_is_tc(&dig_port->base))
+		return 4;
+
+	return get_max_lane_count(tc);
+}
+
 void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
 				      int required_lanes)
 {
@@ -599,6 +615,8 @@ static void icl_tc_phy_get_hw_state(struct intel_tc_port *tc)
 	if (tc->mode != TC_PORT_DISCONNECTED)
 		tc->lock_wakeref = tc_cold_block(tc);
 
+	read_pin_configuration(tc);
+
 	__tc_cold_unblock(tc, domain, tc_cold_wref);
 }
 
@@ -656,8 +674,11 @@ static bool icl_tc_phy_connect(struct intel_tc_port *tc,
 
 	tc->lock_wakeref = tc_cold_block(tc);
 
-	if (tc->mode == TC_PORT_TBT_ALT)
+	if (tc->mode == TC_PORT_TBT_ALT) {
+		read_pin_configuration(tc);
+
 		return true;
+	}
 
 	if ((!tc_phy_is_ready(tc) ||
 	     !icl_tc_phy_take_ownership(tc, true)) &&
@@ -668,6 +689,7 @@ static bool icl_tc_phy_connect(struct intel_tc_port *tc,
 		goto out_unblock_tc_cold;
 	}
 
+	read_pin_configuration(tc);
 
 	if (!tc_phy_verify_legacy_or_dp_alt_mode(tc, required_lanes))
 		goto out_release_phy;
@@ -861,6 +883,8 @@ static void adlp_tc_phy_get_hw_state(struct intel_tc_port *tc)
 	if (tc->mode != TC_PORT_DISCONNECTED)
 		tc->lock_wakeref = tc_cold_block(tc);
 
+	read_pin_configuration(tc);
+
 	intel_display_power_put(display, port_power_domain, port_wakeref);
 }
 
@@ -873,6 +897,9 @@ static bool adlp_tc_phy_connect(struct intel_tc_port *tc, int required_lanes)
 
 	if (tc->mode == TC_PORT_TBT_ALT) {
 		tc->lock_wakeref = tc_cold_block(tc);
+
+		read_pin_configuration(tc);
+
 		return true;
 	}
 
@@ -894,6 +921,8 @@ static bool adlp_tc_phy_connect(struct intel_tc_port *tc, int required_lanes)
 
 	tc->lock_wakeref = tc_cold_block(tc);
 
+	read_pin_configuration(tc);
+
 	if (!tc_phy_verify_legacy_or_dp_alt_mode(tc, required_lanes))
 		goto out_unblock_tc_cold;
 
@@ -1127,6 +1156,8 @@ static void xelpdp_tc_phy_get_hw_state(struct intel_tc_port *tc)
 	if (tc->mode != TC_PORT_DISCONNECTED)
 		tc->lock_wakeref = tc_cold_block(tc);
 
+	read_pin_configuration(tc);
+
 	drm_WARN_ON(display->drm,
 		    (tc->mode == TC_PORT_DP_ALT || tc->mode == TC_PORT_LEGACY) &&
 		    !xelpdp_tc_phy_tcss_power_is_enabled(tc));
@@ -1138,14 +1169,19 @@ static bool xelpdp_tc_phy_connect(struct intel_tc_port *tc, int required_lanes)
 {
 	tc->lock_wakeref = tc_cold_block(tc);
 
-	if (tc->mode == TC_PORT_TBT_ALT)
+	if (tc->mode == TC_PORT_TBT_ALT) {
+		read_pin_configuration(tc);
+
 		return true;
+	}
 
 	if (!xelpdp_tc_phy_enable_tcss_power(tc, true))
 		goto out_unblock_tccold;
 
 	xelpdp_tc_phy_take_ownership(tc, true);
 
+	read_pin_configuration(tc);
+
 	if (!tc_phy_verify_legacy_or_dp_alt_mode(tc, required_lanes))
 		goto out_release_phy;
 
-- 
2.49.1

[PATCH 03/19] drm/i915/lnl+/tc: Fix max lane count HW readout

[Imre Deak]

On LNL+ for a disconnected sink the pin assignment value gets cleared by
the HW/FW as soon as the sink gets disconnected, even if the PHY
ownership got acquired already by the BIOS/driver (and hence the PHY
itself is still connected and used by the display). During HW readout
this can result in detecting the PHY's max lane count as 0 - matching
the above cleared aka NONE pin assignment HW state. For a connected PHY
the driver in general (outside of intel_tc.c) expects the max lane count
value to be valid for the video mode enabled on the corresponding output
(1, 2 or 4). Ensure this by setting the max lane count to 4 in this
case. Note, that it doesn't matter if this lane count happened to be
more than the max lane count with which the PHY got connected and
enabled, since the only thing the driver can do with such an output -
where the DP-alt sink is disconnected - is to disable the output.

Cc: stable@vger.kernel.org # v6.8+
Reported-by: Charlton Lin <charlton.lin@intel.com>
Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index ea6c73af683a0..ea93893980e17 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -23,6 +23,7 @@
 #include "intel_modeset_lock.h"
 #include "intel_tc.h"
 
+#define DP_PIN_ASSIGNMENT_NONE	0x0
 #define DP_PIN_ASSIGNMENT_C	0x3
 #define DP_PIN_ASSIGNMENT_D	0x4
 #define DP_PIN_ASSIGNMENT_E	0x5
@@ -308,6 +309,8 @@ static int lnl_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
 		REG_FIELD_GET(TCSS_DDI_STATUS_PIN_ASSIGNMENT_MASK, val);
 
 	switch (pin_assignment) {
+	case DP_PIN_ASSIGNMENT_NONE:
+		return 0;
 	default:
 		MISSING_CASE(pin_assignment);
 		fallthrough;
@@ -1157,6 +1160,12 @@ static void xelpdp_tc_phy_get_hw_state(struct intel_tc_port *tc)
 		tc->lock_wakeref = tc_cold_block(tc);
 
 	read_pin_configuration(tc);
+	/*
+	 * Set a valid lane count value for a DP-alt sink which got
+	 * disconnected. The driver can only disable the output on this PHY.
+	 */
+	if (tc->max_lane_count == 0)
+		tc->max_lane_count = 4;
 
 	drm_WARN_ON(display->drm,
 		    (tc->mode == TC_PORT_DP_ALT || tc->mode == TC_PORT_LEGACY) &&
-- 
2.49.1

[PATCH 04/19] drm/i915/lnl+/tc: Use the cached max lane count value

[Imre Deak]

Use the cached max lane count value on LNL+, to account for scenarios
where this value is queried after the HW cleared the corresponding pin
assignment value in the TCSS_DDI_STATUS register after the sink got
disconnected.

For consistency, follow-up changes will use the cached max lane count
value on other platforms as well and will also cache the pin assignment
value in a similar way.

Cc: stable@vger.kernel.org # v6.8+
Reported-by: Charlton Lin <charlton.lin@intel.com>
Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index ea93893980e17..14042a64375e1 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -395,12 +395,16 @@ static void read_pin_configuration(struct intel_tc_port *tc)
 
 int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
 {
+	struct intel_display *display = to_intel_display(dig_port);
 	struct intel_tc_port *tc = to_tc_port(dig_port);
 
 	if (!intel_encoder_is_tc(&dig_port->base))
 		return 4;
 
-	return get_max_lane_count(tc);
+	if (DISPLAY_VER(display) < 20)
+		return get_max_lane_count(tc);
+
+	return tc->max_lane_count;
 }
 
 void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
-- 
2.49.1

[PATCH 05/19] drm/i915/icl+/tc: Convert AUX powered WARN to a debug message

[Imre Deak]

The BIOS can leave the AUX power well enabled on an output, even if this
isn't required (on platforms where the AUX power is only needed for an
AUX access). This was observed at least on PTL. To avoid the WARN which
would be triggered by this during the HW readout, convert the WARN to a
debug message.

Cc: stable@vger.kernel.org # v6.8+
Reported-by: Charlton Lin <charlton.lin@intel.com>
Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 14042a64375e1..dec54cb0c8c63 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -1494,11 +1494,11 @@ static void intel_tc_port_reset_mode(struct intel_tc_port *tc,
 	intel_display_power_flush_work(display);
 	if (!intel_tc_cold_requires_aux_pw(dig_port)) {
 		enum intel_display_power_domain aux_domain;
-		bool aux_powered;
 
 		aux_domain = intel_aux_power_domain(dig_port);
-		aux_powered = intel_display_power_is_enabled(display, aux_domain);
-		drm_WARN_ON(display->drm, aux_powered);
+		if (intel_display_power_is_enabled(display, aux_domain))
+			drm_dbg_kms(display->drm, "Port %s: AUX unexpectedly powered\n",
+				    tc->port_name);
 	}
 
 	tc_phy_disconnect(tc);
-- 
2.49.1

[PATCH v2 02/19] drm/i915/icl+/tc: Cache the max lane count value

[Imre Deak]

The PHY's pin assignment value in the TCSS_DDI_STATUS register - as set
by the HW/FW based on the connected DP-alt sink's TypeC/PD pin
assignment negotiation - gets cleared by the HW/FW on LNL+ as soon as
the sink gets disconnected, even if the PHY ownership got acquired
already by the driver (and hence the PHY itself is still connected and
used by the display). This is similar to how the PHY Ready flag gets
cleared on LNL+ in the same register.

To be able to query the max lane count value on LNL+ - which is based on
the above pin assignment - at all times even after the sink gets
disconnected, the max lane count must be determined and cached during
the PHY's HW readout and connect sequences. Do that here, leaving the
actual use of the cached value to a follow-up change.

v2: Don't read out the pin configuration if the PHY is disconnected.

Cc: stable@vger.kernel.org # v6.8+
Reported-by: Charlton Lin <charlton.lin@intel.com>
Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 57 +++++++++++++++++++++----
 1 file changed, 48 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 73a08bd84a70a..b8453fc3ab688 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -66,6 +66,7 @@ struct intel_tc_port {
 	enum tc_port_mode init_mode;
 	enum phy_fia phy_fia;
 	u8 phy_fia_idx;
+	u8 max_lane_count;
 };
 
 static enum intel_display_power_domain
@@ -365,12 +366,12 @@ static int intel_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
 	}
 }
 
-int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
+static int get_max_lane_count(struct intel_tc_port *tc)
 {
-	struct intel_display *display = to_intel_display(dig_port);
-	struct intel_tc_port *tc = to_tc_port(dig_port);
+	struct intel_display *display = to_intel_display(tc->dig_port);
+	struct intel_digital_port *dig_port = tc->dig_port;
 
-	if (!intel_encoder_is_tc(&dig_port->base) || tc->mode != TC_PORT_DP_ALT)
+	if (tc->mode != TC_PORT_DP_ALT)
 		return 4;
 
 	assert_tc_cold_blocked(tc);
@@ -384,6 +385,21 @@ int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
 	return intel_tc_port_get_max_lane_count(dig_port);
 }
 
+static void read_pin_configuration(struct intel_tc_port *tc)
+{
+	tc->max_lane_count = get_max_lane_count(tc);
+}
+
+int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
+{
+	struct intel_tc_port *tc = to_tc_port(dig_port);
+
+	if (!intel_encoder_is_tc(&dig_port->base))
+		return 4;
+
+	return get_max_lane_count(tc);
+}
+
 void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
 				      int required_lanes)
 {
@@ -596,9 +612,12 @@ static void icl_tc_phy_get_hw_state(struct intel_tc_port *tc)
 	tc_cold_wref = __tc_cold_block(tc, &domain);
 
 	tc->mode = tc_phy_get_current_mode(tc);
-	if (tc->mode != TC_PORT_DISCONNECTED)
+	if (tc->mode != TC_PORT_DISCONNECTED) {
 		tc->lock_wakeref = tc_cold_block(tc);
 
+		read_pin_configuration(tc);
+	}
+
 	__tc_cold_unblock(tc, domain, tc_cold_wref);
 }
 
@@ -656,8 +675,11 @@ static bool icl_tc_phy_connect(struct intel_tc_port *tc,
 
 	tc->lock_wakeref = tc_cold_block(tc);
 
-	if (tc->mode == TC_PORT_TBT_ALT)
+	if (tc->mode == TC_PORT_TBT_ALT) {
+		read_pin_configuration(tc);
+
 		return true;
+	}
 
 	if ((!tc_phy_is_ready(tc) ||
 	     !icl_tc_phy_take_ownership(tc, true)) &&
@@ -668,6 +690,7 @@ static bool icl_tc_phy_connect(struct intel_tc_port *tc,
 		goto out_unblock_tc_cold;
 	}
 
+	read_pin_configuration(tc);
 
 	if (!tc_phy_verify_legacy_or_dp_alt_mode(tc, required_lanes))
 		goto out_release_phy;
@@ -858,9 +881,12 @@ static void adlp_tc_phy_get_hw_state(struct intel_tc_port *tc)
 	port_wakeref = intel_display_power_get(display, port_power_domain);
 
 	tc->mode = tc_phy_get_current_mode(tc);
-	if (tc->mode != TC_PORT_DISCONNECTED)
+	if (tc->mode != TC_PORT_DISCONNECTED) {
 		tc->lock_wakeref = tc_cold_block(tc);
 
+		read_pin_configuration(tc);
+	}
+
 	intel_display_power_put(display, port_power_domain, port_wakeref);
 }
 
@@ -873,6 +899,9 @@ static bool adlp_tc_phy_connect(struct intel_tc_port *tc, int required_lanes)
 
 	if (tc->mode == TC_PORT_TBT_ALT) {
 		tc->lock_wakeref = tc_cold_block(tc);
+
+		read_pin_configuration(tc);
+
 		return true;
 	}
 
@@ -894,6 +923,8 @@ static bool adlp_tc_phy_connect(struct intel_tc_port *tc, int required_lanes)
 
 	tc->lock_wakeref = tc_cold_block(tc);
 
+	read_pin_configuration(tc);
+
 	if (!tc_phy_verify_legacy_or_dp_alt_mode(tc, required_lanes))
 		goto out_unblock_tc_cold;
 
@@ -1124,9 +1155,12 @@ static void xelpdp_tc_phy_get_hw_state(struct intel_tc_port *tc)
 	tc_cold_wref = __tc_cold_block(tc, &domain);
 
 	tc->mode = tc_phy_get_current_mode(tc);
-	if (tc->mode != TC_PORT_DISCONNECTED)
+	if (tc->mode != TC_PORT_DISCONNECTED) {
 		tc->lock_wakeref = tc_cold_block(tc);
 
+		read_pin_configuration(tc);
+	}
+
 	drm_WARN_ON(display->drm,
 		    (tc->mode == TC_PORT_DP_ALT || tc->mode == TC_PORT_LEGACY) &&
 		    !xelpdp_tc_phy_tcss_power_is_enabled(tc));
@@ -1138,14 +1172,19 @@ static bool xelpdp_tc_phy_connect(struct intel_tc_port *tc, int required_lanes)
 {
 	tc->lock_wakeref = tc_cold_block(tc);
 
-	if (tc->mode == TC_PORT_TBT_ALT)
+	if (tc->mode == TC_PORT_TBT_ALT) {
+		read_pin_configuration(tc);
+
 		return true;
+	}
 
 	if (!xelpdp_tc_phy_enable_tcss_power(tc, true))
 		goto out_unblock_tccold;
 
 	xelpdp_tc_phy_take_ownership(tc, true);
 
+	read_pin_configuration(tc);
+
 	if (!tc_phy_verify_legacy_or_dp_alt_mode(tc, required_lanes))
 		goto out_release_phy;
 
-- 
2.49.1

[PATCH v2 03/19] drm/i915/lnl+/tc: Fix max lane count HW readout

[Imre Deak]

On LNL+ for a disconnected sink the pin assignment value gets cleared by
the HW/FW as soon as the sink gets disconnected, even if the PHY
ownership got acquired already by the BIOS/driver (and hence the PHY
itself is still connected and used by the display). During HW readout
this can result in detecting the PHY's max lane count as 0 - matching
the above cleared aka NONE pin assignment HW state. For a connected PHY
the driver in general (outside of intel_tc.c) expects the max lane count
value to be valid for the video mode enabled on the corresponding output
(1, 2 or 4). Ensure this by setting the max lane count to 4 in this
case. Note, that it doesn't matter if this lane count happened to be
more than the max lane count with which the PHY got connected and
enabled, since the only thing the driver can do with such an output -
where the DP-alt sink is disconnected - is to disable the output.

v2: Rebased on change reading out the pin configuration only if the PHY
    is connected.

Cc: stable@vger.kernel.org # v6.8+
Reported-by: Charlton Lin <charlton.lin@intel.com>
Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index b8453fc3ab688..a89fbbf848d7d 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -23,6 +23,7 @@
 #include "intel_modeset_lock.h"
 #include "intel_tc.h"
 
+#define DP_PIN_ASSIGNMENT_NONE	0x0
 #define DP_PIN_ASSIGNMENT_C	0x3
 #define DP_PIN_ASSIGNMENT_D	0x4
 #define DP_PIN_ASSIGNMENT_E	0x5
@@ -308,6 +309,8 @@ static int lnl_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
 		REG_FIELD_GET(TCSS_DDI_STATUS_PIN_ASSIGNMENT_MASK, val);
 
 	switch (pin_assignment) {
+	case DP_PIN_ASSIGNMENT_NONE:
+		return 0;
 	default:
 		MISSING_CASE(pin_assignment);
 		fallthrough;
@@ -1159,6 +1162,12 @@ static void xelpdp_tc_phy_get_hw_state(struct intel_tc_port *tc)
 		tc->lock_wakeref = tc_cold_block(tc);
 
 		read_pin_configuration(tc);
+		/*
+		 * Set a valid lane count value for a DP-alt sink which got
+		 * disconnected. The driver can only disable the output on this PHY.
+		 */
+		if (tc->max_lane_count == 0)
+			tc->max_lane_count = 4;
 	}
 
 	drm_WARN_ON(display->drm,
-- 
2.49.1

RE: [PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[Kahola, Mika]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Imre Deak
> Sent: Tuesday, 5 August 2025 10.37
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: stable@vger.kernel.org; Lin, Charlton <charlton.lin@intel.com>; Almahallawy, Khaled <khaled.almahallawy@intel.com>
> Subject: [PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink
> 
> The TypeC PHY HW readout during driver loading and system resume determines which TypeC mode the PHY is in (legacy/DP-
> alt/TBT-alt) and whether the PHY is connected, based on the PHY's Owned and Ready flags.
> For the PHY to be in DP-alt or legacy mode and for the PHY to be in the connected state in these modes, both the Owned (set by
> the BIOS/driver) and the Ready (set by the HW) flags should be set.
> 
> On ICL-MTL the HW kept the PHY's Ready flag set after the driver connected the PHY by acquiring the PHY ownership (by setting
> the Owned flag), until the driver disconnected the PHY by releasing the PHY ownership (by clearing the Owned flag). On LNL+ this
> has changed, in that the HW clears the Ready flag as soon as the sink gets disconnected, even if the PHY ownership was acquired
> already and hence the PHY is being used by the display.
> 
> When inheriting the HW state from BIOS for a PHY connected in DP-alt mode on which the sink got disconnected - i.e. in a case
> where the sink was connected while BIOS/GOP was running and so the sink got enabled connecting the PHY, but the user
> disconnected the sink by the time the driver loaded - the PHY Owned but not Ready state must be accounted for on LNL+
> according to the above. Do that by assuming on LNL+ that the PHY is connected in DP-alt mode whenever the PHY Owned flag is
> set, regardless of the PHY Ready flag.
> 
> This fixes a problem on LNL+, where the PHY TypeC mode / connected state was detected incorrectly for a DP-alt sink, which got
> connected and then disconnected by the user in the above way.
> 
> Cc: stable@vger.kernel.org # v6.8+
> Reported-by: Charlton Lin <charlton.lin@intel.com>
> Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index 3bc57579fe53e..73a08bd84a70a 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -1226,14 +1226,18 @@ static void tc_phy_get_hw_state(struct intel_tc_port *tc)
>  	tc->phy_ops->get_hw_state(tc);
>  }
> 
> -static bool tc_phy_is_ready_and_owned(struct intel_tc_port *tc,
> -				      bool phy_is_ready, bool phy_is_owned)
> +static bool tc_phy_in_legacy_or_dp_alt_mode(struct intel_tc_port *tc,
> +					    bool phy_is_ready, bool phy_is_owned)
>  {
>  	struct intel_display *display = to_intel_display(tc->dig_port);
> 
> -	drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
> +	if (DISPLAY_VER(display) < 20) {
> +		drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
> 
> -	return phy_is_ready && phy_is_owned;
> +		return phy_is_ready && phy_is_owned;
> +	} else {
> +		return phy_is_owned;
> +	}
>  }
> 
>  static bool tc_phy_is_connected(struct intel_tc_port *tc, @@ -1244,7 +1248,7 @@ static bool tc_phy_is_connected(struct
> intel_tc_port *tc,
>  	bool phy_is_owned = tc_phy_is_owned(tc);
>  	bool is_connected;
> 
> -	if (tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned))
> +	if (tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned))
>  		is_connected = port_pll_type == ICL_PORT_DPLL_MG_PHY;
>  	else
>  		is_connected = port_pll_type == ICL_PORT_DPLL_DEFAULT; @@ -1352,7 +1356,7 @@
> tc_phy_get_current_mode(struct intel_tc_port *tc)
>  	phy_is_ready = tc_phy_is_ready(tc);
>  	phy_is_owned = tc_phy_is_owned(tc);
> 
> -	if (!tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned)) {
> +	if (!tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned))
> +{
>  		mode = get_tc_mode_in_phy_not_owned_state(tc, live_mode);
>  	} else {
>  		drm_WARN_ON(display->drm, live_mode == TC_PORT_TBT_ALT);
> --
> 2.49.1

RE: [PATCH v2 02/19] drm/i915/icl+/tc: Cache the max lane count value

[Kahola, Mika]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre Deak
> Sent: Tuesday, 5 August 2025 12.34
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: stable@vger.kernel.org; Lin, Charlton <charlton.lin@intel.com>; Almahallawy, Khaled <khaled.almahallawy@intel.com>
> Subject: [PATCH v2 02/19] drm/i915/icl+/tc: Cache the max lane count value
> 
> The PHY's pin assignment value in the TCSS_DDI_STATUS register - as set by the HW/FW based on the connected DP-alt sink's
> TypeC/PD pin assignment negotiation - gets cleared by the HW/FW on LNL+ as soon as the sink gets disconnected, even if the PHY
> ownership got acquired already by the driver (and hence the PHY itself is still connected and used by the display). This is similar to
> how the PHY Ready flag gets cleared on LNL+ in the same register.
> 
> To be able to query the max lane count value on LNL+ - which is based on the above pin assignment - at all times even after the
> sink gets disconnected, the max lane count must be determined and cached during the PHY's HW readout and connect sequences.
> Do that here, leaving the actual use of the cached value to a follow-up change.
> 
> v2: Don't read out the pin configuration if the PHY is disconnected.
> 
> Cc: stable@vger.kernel.org # v6.8+
> Reported-by: Charlton Lin <charlton.lin@intel.com>
> Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 57 +++++++++++++++++++++----
>  1 file changed, 48 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index 73a08bd84a70a..b8453fc3ab688 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -66,6 +66,7 @@ struct intel_tc_port {
>  	enum tc_port_mode init_mode;
>  	enum phy_fia phy_fia;
>  	u8 phy_fia_idx;
> +	u8 max_lane_count;
>  };
> 
>  static enum intel_display_power_domain
> @@ -365,12 +366,12 @@ static int intel_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
>  	}
>  }
> 
> -int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
> +static int get_max_lane_count(struct intel_tc_port *tc)
>  {
> -	struct intel_display *display = to_intel_display(dig_port);
> -	struct intel_tc_port *tc = to_tc_port(dig_port);
> +	struct intel_display *display = to_intel_display(tc->dig_port);
> +	struct intel_digital_port *dig_port = tc->dig_port;
> 
> -	if (!intel_encoder_is_tc(&dig_port->base) || tc->mode != TC_PORT_DP_ALT)
> +	if (tc->mode != TC_PORT_DP_ALT)
>  		return 4;
> 
>  	assert_tc_cold_blocked(tc);
> @@ -384,6 +385,21 @@ int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)
>  	return intel_tc_port_get_max_lane_count(dig_port);
>  }
> 
> +static void read_pin_configuration(struct intel_tc_port *tc) {
> +	tc->max_lane_count = get_max_lane_count(tc); }
> +
> +int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port) {
> +	struct intel_tc_port *tc = to_tc_port(dig_port);
> +
> +	if (!intel_encoder_is_tc(&dig_port->base))
> +		return 4;
> +
> +	return get_max_lane_count(tc);
> +}
> +
>  void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
>  				      int required_lanes)
>  {
> @@ -596,9 +612,12 @@ static void icl_tc_phy_get_hw_state(struct intel_tc_port *tc)
>  	tc_cold_wref = __tc_cold_block(tc, &domain);
> 
>  	tc->mode = tc_phy_get_current_mode(tc);
> -	if (tc->mode != TC_PORT_DISCONNECTED)
> +	if (tc->mode != TC_PORT_DISCONNECTED) {
>  		tc->lock_wakeref = tc_cold_block(tc);
> 
> +		read_pin_configuration(tc);
> +	}
> +
>  	__tc_cold_unblock(tc, domain, tc_cold_wref);  }
> 
> @@ -656,8 +675,11 @@ static bool icl_tc_phy_connect(struct intel_tc_port *tc,
> 
>  	tc->lock_wakeref = tc_cold_block(tc);
> 
> -	if (tc->mode == TC_PORT_TBT_ALT)
> +	if (tc->mode == TC_PORT_TBT_ALT) {
> +		read_pin_configuration(tc);
> +
>  		return true;
> +	}
> 
>  	if ((!tc_phy_is_ready(tc) ||
>  	     !icl_tc_phy_take_ownership(tc, true)) && @@ -668,6 +690,7 @@ static bool icl_tc_phy_connect(struct intel_tc_port
> *tc,
>  		goto out_unblock_tc_cold;
>  	}
> 
> +	read_pin_configuration(tc);
> 
>  	if (!tc_phy_verify_legacy_or_dp_alt_mode(tc, required_lanes))
>  		goto out_release_phy;
> @@ -858,9 +881,12 @@ static void adlp_tc_phy_get_hw_state(struct intel_tc_port *tc)
>  	port_wakeref = intel_display_power_get(display, port_power_domain);
> 
>  	tc->mode = tc_phy_get_current_mode(tc);
> -	if (tc->mode != TC_PORT_DISCONNECTED)
> +	if (tc->mode != TC_PORT_DISCONNECTED) {
>  		tc->lock_wakeref = tc_cold_block(tc);
> 
> +		read_pin_configuration(tc);
> +	}
> +
>  	intel_display_power_put(display, port_power_domain, port_wakeref);  }
> 
> @@ -873,6 +899,9 @@ static bool adlp_tc_phy_connect(struct intel_tc_port *tc, int required_lanes)
> 
>  	if (tc->mode == TC_PORT_TBT_ALT) {
>  		tc->lock_wakeref = tc_cold_block(tc);
> +
> +		read_pin_configuration(tc);
> +
>  		return true;
>  	}
> 
> @@ -894,6 +923,8 @@ static bool adlp_tc_phy_connect(struct intel_tc_port *tc, int required_lanes)
> 
>  	tc->lock_wakeref = tc_cold_block(tc);
> 
> +	read_pin_configuration(tc);
> +
>  	if (!tc_phy_verify_legacy_or_dp_alt_mode(tc, required_lanes))
>  		goto out_unblock_tc_cold;
> 
> @@ -1124,9 +1155,12 @@ static void xelpdp_tc_phy_get_hw_state(struct intel_tc_port *tc)
>  	tc_cold_wref = __tc_cold_block(tc, &domain);
> 
>  	tc->mode = tc_phy_get_current_mode(tc);
> -	if (tc->mode != TC_PORT_DISCONNECTED)
> +	if (tc->mode != TC_PORT_DISCONNECTED) {
>  		tc->lock_wakeref = tc_cold_block(tc);
> 
> +		read_pin_configuration(tc);
> +	}
> +
>  	drm_WARN_ON(display->drm,
>  		    (tc->mode == TC_PORT_DP_ALT || tc->mode == TC_PORT_LEGACY) &&
>  		    !xelpdp_tc_phy_tcss_power_is_enabled(tc));
> @@ -1138,14 +1172,19 @@ static bool xelpdp_tc_phy_connect(struct intel_tc_port *tc, int required_lanes)  {
>  	tc->lock_wakeref = tc_cold_block(tc);
> 
> -	if (tc->mode == TC_PORT_TBT_ALT)
> +	if (tc->mode == TC_PORT_TBT_ALT) {
> +		read_pin_configuration(tc);
> +
>  		return true;
> +	}
> 
>  	if (!xelpdp_tc_phy_enable_tcss_power(tc, true))
>  		goto out_unblock_tccold;
> 
>  	xelpdp_tc_phy_take_ownership(tc, true);
> 
> +	read_pin_configuration(tc);
> +
>  	if (!tc_phy_verify_legacy_or_dp_alt_mode(tc, required_lanes))
>  		goto out_release_phy;
> 
> --
> 2.49.1

RE: [PATCH v2 03/19] drm/i915/lnl+/tc: Fix max lane count HW readout

[Kahola, Mika]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre Deak
> Sent: Tuesday, 5 August 2025 12.34
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: stable@vger.kernel.org; Lin, Charlton <charlton.lin@intel.com>; Almahallawy, Khaled <khaled.almahallawy@intel.com>
> Subject: [PATCH v2 03/19] drm/i915/lnl+/tc: Fix max lane count HW readout
> 
> On LNL+ for a disconnected sink the pin assignment value gets cleared by the HW/FW as soon as the sink gets disconnected, even
> if the PHY ownership got acquired already by the BIOS/driver (and hence the PHY itself is still connected and used by the display).
> During HW readout this can result in detecting the PHY's max lane count as 0 - matching the above cleared aka NONE pin
> assignment HW state. For a connected PHY the driver in general (outside of intel_tc.c) expects the max lane count value to be
> valid for the video mode enabled on the corresponding output (1, 2 or 4). Ensure this by setting the max lane count to 4 in this case.
> Note, that it doesn't matter if this lane count happened to be more than the max lane count with which the PHY got connected and
> enabled, since the only thing the driver can do with such an output - where the DP-alt sink is disconnected - is to disable the
> output.
> 
> v2: Rebased on change reading out the pin configuration only if the PHY
>     is connected.
> 
> Cc: stable@vger.kernel.org # v6.8+
> Reported-by: Charlton Lin <charlton.lin@intel.com>
> Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index b8453fc3ab688..a89fbbf848d7d 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -23,6 +23,7 @@
>  #include "intel_modeset_lock.h"
>  #include "intel_tc.h"
> 
> +#define DP_PIN_ASSIGNMENT_NONE	0x0
>  #define DP_PIN_ASSIGNMENT_C	0x3
>  #define DP_PIN_ASSIGNMENT_D	0x4
>  #define DP_PIN_ASSIGNMENT_E	0x5
> @@ -308,6 +309,8 @@ static int lnl_tc_port_get_max_lane_count(struct intel_digital_port *dig_port)
>  		REG_FIELD_GET(TCSS_DDI_STATUS_PIN_ASSIGNMENT_MASK, val);
> 
>  	switch (pin_assignment) {
> +	case DP_PIN_ASSIGNMENT_NONE:
> +		return 0;
>  	default:
>  		MISSING_CASE(pin_assignment);
>  		fallthrough;
> @@ -1159,6 +1162,12 @@ static void xelpdp_tc_phy_get_hw_state(struct intel_tc_port *tc)
>  		tc->lock_wakeref = tc_cold_block(tc);
> 
>  		read_pin_configuration(tc);
> +		/*
> +		 * Set a valid lane count value for a DP-alt sink which got
> +		 * disconnected. The driver can only disable the output on this PHY.
> +		 */
> +		if (tc->max_lane_count == 0)
> +			tc->max_lane_count = 4;
>  	}
> 
>  	drm_WARN_ON(display->drm,
> --
> 2.49.1

RE: [PATCH 04/19] drm/i915/lnl+/tc: Use the cached max lane count value

[Kahola, Mika]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Imre Deak
> Sent: Tuesday, 5 August 2025 10.37
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: stable@vger.kernel.org; Lin, Charlton <charlton.lin@intel.com>; Almahallawy, Khaled <khaled.almahallawy@intel.com>
> Subject: [PATCH 04/19] drm/i915/lnl+/tc: Use the cached max lane count value
> 
> Use the cached max lane count value on LNL+, to account for scenarios where this value is queried after the HW cleared the
> corresponding pin assignment value in the TCSS_DDI_STATUS register after the sink got disconnected.
> 
> For consistency, follow-up changes will use the cached max lane count value on other platforms as well and will also cache the pin
> assignment value in a similar way.
> 
> Cc: stable@vger.kernel.org # v6.8+
> Reported-by: Charlton Lin <charlton.lin@intel.com>
> Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index ea93893980e17..14042a64375e1 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -395,12 +395,16 @@ static void read_pin_configuration(struct intel_tc_port *tc)
> 
>  int intel_tc_port_max_lane_count(struct intel_digital_port *dig_port)  {
> +	struct intel_display *display = to_intel_display(dig_port);
>  	struct intel_tc_port *tc = to_tc_port(dig_port);
> 
>  	if (!intel_encoder_is_tc(&dig_port->base))
>  		return 4;
> 
> -	return get_max_lane_count(tc);
> +	if (DISPLAY_VER(display) < 20)
> +		return get_max_lane_count(tc);
> +
> +	return tc->max_lane_count;
>  }
> 
>  void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
> --
> 2.49.1

Re: [PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[Luca Coelho]

On Tue, 2025-08-05 at 10:36 +0300, Imre Deak wrote:
> The TypeC PHY HW readout during driver loading and system resume
> determines which TypeC mode the PHY is in (legacy/DP-alt/TBT-alt) and
> whether the PHY is connected, based on the PHY's Owned and Ready flags.
> For the PHY to be in DP-alt or legacy mode and for the PHY to be in the
> connected state in these modes, both the Owned (set by the BIOS/driver)
> and the Ready (set by the HW) flags should be set.
> 
> On ICL-MTL the HW kept the PHY's Ready flag set after the driver
> connected the PHY by acquiring the PHY ownership (by setting the Owned
> flag), until the driver disconnected the PHY by releasing the PHY
> ownership (by clearing the Owned flag). On LNL+ this has changed, in
> that the HW clears the Ready flag as soon as the sink gets disconnected,
> even if the PHY ownership was acquired already and hence the PHY is
> being used by the display.
> 
> When inheriting the HW state from BIOS for a PHY connected in DP-alt
> mode on which the sink got disconnected - i.e. in a case where the sink
> was connected while BIOS/GOP was running and so the sink got enabled
> connecting the PHY, but the user disconnected the sink by the time the
> driver loaded - the PHY Owned but not Ready state must be accounted for
> on LNL+ according to the above. Do that by assuming on LNL+ that the PHY
> is connected in DP-alt mode whenever the PHY Owned flag is set,
> regardless of the PHY Ready flag.
> 
> This fixes a problem on LNL+, where the PHY TypeC mode / connected state
> was detected incorrectly for a DP-alt sink, which got connected and then
> disconnected by the user in the above way.
> 
> Cc: stable@vger.kernel.org # v6.8+
> Reported-by: Charlton Lin <charlton.lin@intel.com>
> Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index 3bc57579fe53e..73a08bd84a70a 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -1226,14 +1226,18 @@ static void tc_phy_get_hw_state(struct intel_tc_port *tc)
>  	tc->phy_ops->get_hw_state(tc);
>  }
>  
> -static bool tc_phy_is_ready_and_owned(struct intel_tc_port *tc,
> -				      bool phy_is_ready, bool phy_is_owned)
> +static bool tc_phy_in_legacy_or_dp_alt_mode(struct intel_tc_port *tc,
> +					    bool phy_is_ready, bool phy_is_owned)

Personally I don't like the "or" in the function name.  You're
returning a boolean which is true or false.  The return value is akin
to answering "Yes/No" to the question "Is it black or white".

This is a nitpick, obviously, so I'll leave it up to you.

Regardless:

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

>  {
>  	struct intel_display *display = to_intel_display(tc->dig_port);
>  
> -	drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
> +	if (DISPLAY_VER(display) < 20) {
> +		drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
>  
> -	return phy_is_ready && phy_is_owned;
> +		return phy_is_ready && phy_is_owned;
> +	} else {
> +		return phy_is_owned;
> +	}
>  }
>  
>  static bool tc_phy_is_connected(struct intel_tc_port *tc,
> @@ -1244,7 +1248,7 @@ static bool tc_phy_is_connected(struct intel_tc_port *tc,
>  	bool phy_is_owned = tc_phy_is_owned(tc);
>  	bool is_connected;
>  
> -	if (tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned))
> +	if (tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned))
>  		is_connected = port_pll_type == ICL_PORT_DPLL_MG_PHY;
>  	else
>  		is_connected = port_pll_type == ICL_PORT_DPLL_DEFAULT;
> @@ -1352,7 +1356,7 @@ tc_phy_get_current_mode(struct intel_tc_port *tc)
>  	phy_is_ready = tc_phy_is_ready(tc);
>  	phy_is_owned = tc_phy_is_owned(tc);
>  
> -	if (!tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned)) {
> +	if (!tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned)) {
>  		mode = get_tc_mode_in_phy_not_owned_state(tc, live_mode);
>  	} else {
>  		drm_WARN_ON(display->drm, live_mode == TC_PORT_TBT_ALT);

Re: [PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[Imre Deak]

On Thu, Aug 07, 2025 at 01:59:21PM +0300, Luca Coelho wrote:
> On Tue, 2025-08-05 at 10:36 +0300, Imre Deak wrote:
> > The TypeC PHY HW readout during driver loading and system resume
> > determines which TypeC mode the PHY is in (legacy/DP-alt/TBT-alt) and
> > whether the PHY is connected, based on the PHY's Owned and Ready flags.
> > For the PHY to be in DP-alt or legacy mode and for the PHY to be in the
> > connected state in these modes, both the Owned (set by the BIOS/driver)
> > and the Ready (set by the HW) flags should be set.
> > 
> > On ICL-MTL the HW kept the PHY's Ready flag set after the driver
> > connected the PHY by acquiring the PHY ownership (by setting the Owned
> > flag), until the driver disconnected the PHY by releasing the PHY
> > ownership (by clearing the Owned flag). On LNL+ this has changed, in
> > that the HW clears the Ready flag as soon as the sink gets disconnected,
> > even if the PHY ownership was acquired already and hence the PHY is
> > being used by the display.
> > 
> > When inheriting the HW state from BIOS for a PHY connected in DP-alt
> > mode on which the sink got disconnected - i.e. in a case where the sink
> > was connected while BIOS/GOP was running and so the sink got enabled
> > connecting the PHY, but the user disconnected the sink by the time the
> > driver loaded - the PHY Owned but not Ready state must be accounted for
> > on LNL+ according to the above. Do that by assuming on LNL+ that the PHY
> > is connected in DP-alt mode whenever the PHY Owned flag is set,
> > regardless of the PHY Ready flag.
> > 
> > This fixes a problem on LNL+, where the PHY TypeC mode / connected state
> > was detected incorrectly for a DP-alt sink, which got connected and then
> > disconnected by the user in the above way.
> > 
> > Cc: stable@vger.kernel.org # v6.8+
> > Reported-by: Charlton Lin <charlton.lin@intel.com>
> > Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_tc.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 3bc57579fe53e..73a08bd84a70a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -1226,14 +1226,18 @@ static void tc_phy_get_hw_state(struct intel_tc_port *tc)
> >  	tc->phy_ops->get_hw_state(tc);
> >  }
> >  
> > -static bool tc_phy_is_ready_and_owned(struct intel_tc_port *tc,
> > -				      bool phy_is_ready, bool phy_is_owned)
> > +static bool tc_phy_in_legacy_or_dp_alt_mode(struct intel_tc_port *tc,
> > +					    bool phy_is_ready, bool phy_is_owned)
> 
> Personally I don't like the "or" in the function name.  You're
> returning a boolean which is true or false.  The return value is akin
> to answering "Yes/No" to the question "Is it black or white".

The question the function is meant to answer is "Is the PHY in legacy or
DP-alt mode?". The return value is true (yes) if the PHY is in either
legacy or DP-alt mode, false (no) if the PHY is neither in legacy or
DP-alt mode. There are many other uses of "or" in function names in this
sense, so not sure how else I'd name this one. Simply leaving out "or"
would make it less clear that the legacy and DP-alt modes are two
separate modes.

> This is a nitpick, obviously, so I'll leave it up to you.
> 
> Regardless:
> 
> Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
> 
> --
> Cheers,
> Luca.
> 
> >  {
> >  	struct intel_display *display = to_intel_display(tc->dig_port);
> >  
> > -	drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
> > +	if (DISPLAY_VER(display) < 20) {
> > +		drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
> >  
> > -	return phy_is_ready && phy_is_owned;
> > +		return phy_is_ready && phy_is_owned;
> > +	} else {
> > +		return phy_is_owned;
> > +	}
> >  }
> >  
> >  static bool tc_phy_is_connected(struct intel_tc_port *tc,
> > @@ -1244,7 +1248,7 @@ static bool tc_phy_is_connected(struct intel_tc_port *tc,
> >  	bool phy_is_owned = tc_phy_is_owned(tc);
> >  	bool is_connected;
> >  
> > -	if (tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned))
> > +	if (tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned))
> >  		is_connected = port_pll_type == ICL_PORT_DPLL_MG_PHY;
> >  	else
> >  		is_connected = port_pll_type == ICL_PORT_DPLL_DEFAULT;
> > @@ -1352,7 +1356,7 @@ tc_phy_get_current_mode(struct intel_tc_port *tc)
> >  	phy_is_ready = tc_phy_is_ready(tc);
> >  	phy_is_owned = tc_phy_is_owned(tc);
> >  
> > -	if (!tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned)) {
> > +	if (!tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned)) {
> >  		mode = get_tc_mode_in_phy_not_owned_state(tc, live_mode);
> >  	} else {
> >  		drm_WARN_ON(display->drm, live_mode == TC_PORT_TBT_ALT);

Re: [PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[Jani Nikula]

On Thu, 07 Aug 2025, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, Aug 07, 2025 at 01:59:21PM +0300, Luca Coelho wrote:
>> On Tue, 2025-08-05 at 10:36 +0300, Imre Deak wrote:
>> > The TypeC PHY HW readout during driver loading and system resume
>> > determines which TypeC mode the PHY is in (legacy/DP-alt/TBT-alt) and
>> > whether the PHY is connected, based on the PHY's Owned and Ready flags.
>> > For the PHY to be in DP-alt or legacy mode and for the PHY to be in the
>> > connected state in these modes, both the Owned (set by the BIOS/driver)
>> > and the Ready (set by the HW) flags should be set.
>> > 
>> > On ICL-MTL the HW kept the PHY's Ready flag set after the driver
>> > connected the PHY by acquiring the PHY ownership (by setting the Owned
>> > flag), until the driver disconnected the PHY by releasing the PHY
>> > ownership (by clearing the Owned flag). On LNL+ this has changed, in
>> > that the HW clears the Ready flag as soon as the sink gets disconnected,
>> > even if the PHY ownership was acquired already and hence the PHY is
>> > being used by the display.
>> > 
>> > When inheriting the HW state from BIOS for a PHY connected in DP-alt
>> > mode on which the sink got disconnected - i.e. in a case where the sink
>> > was connected while BIOS/GOP was running and so the sink got enabled
>> > connecting the PHY, but the user disconnected the sink by the time the
>> > driver loaded - the PHY Owned but not Ready state must be accounted for
>> > on LNL+ according to the above. Do that by assuming on LNL+ that the PHY
>> > is connected in DP-alt mode whenever the PHY Owned flag is set,
>> > regardless of the PHY Ready flag.
>> > 
>> > This fixes a problem on LNL+, where the PHY TypeC mode / connected state
>> > was detected incorrectly for a DP-alt sink, which got connected and then
>> > disconnected by the user in the above way.
>> > 
>> > Cc: stable@vger.kernel.org # v6.8+
>> > Reported-by: Charlton Lin <charlton.lin@intel.com>
>> > Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_tc.c | 16 ++++++++++------
>> >  1 file changed, 10 insertions(+), 6 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
>> > index 3bc57579fe53e..73a08bd84a70a 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
>> > @@ -1226,14 +1226,18 @@ static void tc_phy_get_hw_state(struct intel_tc_port *tc)
>> >  	tc->phy_ops->get_hw_state(tc);
>> >  }
>> >  
>> > -static bool tc_phy_is_ready_and_owned(struct intel_tc_port *tc,
>> > -				      bool phy_is_ready, bool phy_is_owned)
>> > +static bool tc_phy_in_legacy_or_dp_alt_mode(struct intel_tc_port *tc,
>> > +					    bool phy_is_ready, bool phy_is_owned)
>> 
>> Personally I don't like the "or" in the function name.  You're
>> returning a boolean which is true or false.  The return value is akin
>> to answering "Yes/No" to the question "Is it black or white".
>
> The question the function is meant to answer is "Is the PHY in legacy or
> DP-alt mode?". The return value is true (yes) if the PHY is in either
> legacy or DP-alt mode, false (no) if the PHY is neither in legacy or
> DP-alt mode. There are many other uses of "or" in function names in this
> sense, so not sure how else I'd name this one. Simply leaving out "or"
> would make it less clear that the legacy and DP-alt modes are two
> separate modes.

What's the opposite of "Is the PHY in legacy or DP-alt mode"?

Would that lead to a simpler name, with the reversed return value?

BR,
Jani.



>
>> This is a nitpick, obviously, so I'll leave it up to you.
>> 
>> Regardless:
>> 
>> Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
>> 
>> --
>> Cheers,
>> Luca.
>> 
>> >  {
>> >  	struct intel_display *display = to_intel_display(tc->dig_port);
>> >  
>> > -	drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
>> > +	if (DISPLAY_VER(display) < 20) {
>> > +		drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
>> >  
>> > -	return phy_is_ready && phy_is_owned;
>> > +		return phy_is_ready && phy_is_owned;
>> > +	} else {
>> > +		return phy_is_owned;
>> > +	}
>> >  }
>> >  
>> >  static bool tc_phy_is_connected(struct intel_tc_port *tc,
>> > @@ -1244,7 +1248,7 @@ static bool tc_phy_is_connected(struct intel_tc_port *tc,
>> >  	bool phy_is_owned = tc_phy_is_owned(tc);
>> >  	bool is_connected;
>> >  
>> > -	if (tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned))
>> > +	if (tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned))
>> >  		is_connected = port_pll_type == ICL_PORT_DPLL_MG_PHY;
>> >  	else
>> >  		is_connected = port_pll_type == ICL_PORT_DPLL_DEFAULT;
>> > @@ -1352,7 +1356,7 @@ tc_phy_get_current_mode(struct intel_tc_port *tc)
>> >  	phy_is_ready = tc_phy_is_ready(tc);
>> >  	phy_is_owned = tc_phy_is_owned(tc);
>> >  
>> > -	if (!tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned)) {
>> > +	if (!tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned)) {
>> >  		mode = get_tc_mode_in_phy_not_owned_state(tc, live_mode);
>> >  	} else {
>> >  		drm_WARN_ON(display->drm, live_mode == TC_PORT_TBT_ALT);

-- 
Jani Nikula, Intel

RE: [PATCH 05/19] drm/i915/icl+/tc: Convert AUX powered WARN to a debug message

[Kahola, Mika]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre Deak
> Sent: Tuesday, 5 August 2025 10.37
> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Cc: stable@vger.kernel.org; Lin, Charlton <charlton.lin@intel.com>; Almahallawy, Khaled <khaled.almahallawy@intel.com>
> Subject: [PATCH 05/19] drm/i915/icl+/tc: Convert AUX powered WARN to a debug message
> 
> The BIOS can leave the AUX power well enabled on an output, even if this isn't required (on platforms where the AUX power is
> only needed for an AUX access). This was observed at least on PTL. To avoid the WARN which would be triggered by this during the
> HW readout, convert the WARN to a debug message.
> 
> Cc: stable@vger.kernel.org # v6.8+
> Reported-by: Charlton Lin <charlton.lin@intel.com>
> Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index 14042a64375e1..dec54cb0c8c63 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -1494,11 +1494,11 @@ static void intel_tc_port_reset_mode(struct intel_tc_port *tc,
>  	intel_display_power_flush_work(display);
>  	if (!intel_tc_cold_requires_aux_pw(dig_port)) {
>  		enum intel_display_power_domain aux_domain;
> -		bool aux_powered;
> 
>  		aux_domain = intel_aux_power_domain(dig_port);
> -		aux_powered = intel_display_power_is_enabled(display, aux_domain);
> -		drm_WARN_ON(display->drm, aux_powered);
> +		if (intel_display_power_is_enabled(display, aux_domain))
> +			drm_dbg_kms(display->drm, "Port %s: AUX unexpectedly powered\n",
> +				    tc->port_name);
>  	}
> 
>  	tc_phy_disconnect(tc);
> --
> 2.49.1

Re: [PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[Imre Deak]

On Thu, Aug 07, 2025 at 03:19:17PM +0300, Jani Nikula wrote:
> On Thu, 07 Aug 2025, Imre Deak <imre.deak@intel.com> wrote:
> > On Thu, Aug 07, 2025 at 01:59:21PM +0300, Luca Coelho wrote:
> >> On Tue, 2025-08-05 at 10:36 +0300, Imre Deak wrote:
> >> > The TypeC PHY HW readout during driver loading and system resume
> >> > determines which TypeC mode the PHY is in (legacy/DP-alt/TBT-alt) and
> >> > whether the PHY is connected, based on the PHY's Owned and Ready flags.
> >> > For the PHY to be in DP-alt or legacy mode and for the PHY to be in the
> >> > connected state in these modes, both the Owned (set by the BIOS/driver)
> >> > and the Ready (set by the HW) flags should be set.
> >> > 
> >> > On ICL-MTL the HW kept the PHY's Ready flag set after the driver
> >> > connected the PHY by acquiring the PHY ownership (by setting the Owned
> >> > flag), until the driver disconnected the PHY by releasing the PHY
> >> > ownership (by clearing the Owned flag). On LNL+ this has changed, in
> >> > that the HW clears the Ready flag as soon as the sink gets disconnected,
> >> > even if the PHY ownership was acquired already and hence the PHY is
> >> > being used by the display.
> >> > 
> >> > When inheriting the HW state from BIOS for a PHY connected in DP-alt
> >> > mode on which the sink got disconnected - i.e. in a case where the sink
> >> > was connected while BIOS/GOP was running and so the sink got enabled
> >> > connecting the PHY, but the user disconnected the sink by the time the
> >> > driver loaded - the PHY Owned but not Ready state must be accounted for
> >> > on LNL+ according to the above. Do that by assuming on LNL+ that the PHY
> >> > is connected in DP-alt mode whenever the PHY Owned flag is set,
> >> > regardless of the PHY Ready flag.
> >> > 
> >> > This fixes a problem on LNL+, where the PHY TypeC mode / connected state
> >> > was detected incorrectly for a DP-alt sink, which got connected and then
> >> > disconnected by the user in the above way.
> >> > 
> >> > Cc: stable@vger.kernel.org # v6.8+
> >> > Reported-by: Charlton Lin <charlton.lin@intel.com>
> >> > Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
> >> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_tc.c | 16 ++++++++++------
> >> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >> > 
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> >> > index 3bc57579fe53e..73a08bd84a70a 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> >> > @@ -1226,14 +1226,18 @@ static void tc_phy_get_hw_state(struct intel_tc_port *tc)
> >> >  	tc->phy_ops->get_hw_state(tc);
> >> >  }
> >> >  
> >> > -static bool tc_phy_is_ready_and_owned(struct intel_tc_port *tc,
> >> > -				      bool phy_is_ready, bool phy_is_owned)
> >> > +static bool tc_phy_in_legacy_or_dp_alt_mode(struct intel_tc_port *tc,
> >> > +					    bool phy_is_ready, bool phy_is_owned)
> >> 
> >> Personally I don't like the "or" in the function name.  You're
> >> returning a boolean which is true or false.  The return value is akin
> >> to answering "Yes/No" to the question "Is it black or white".
> >
> > The question the function is meant to answer is "Is the PHY in legacy or
> > DP-alt mode?". The return value is true (yes) if the PHY is in either
> > legacy or DP-alt mode, false (no) if the PHY is neither in legacy or
> > DP-alt mode. There are many other uses of "or" in function names in this
> > sense, so not sure how else I'd name this one. Simply leaving out "or"
> > would make it less clear that the legacy and DP-alt modes are two
> > separate modes.
> 
> What's the opposite of "Is the PHY in legacy or DP-alt mode"?
>
> Would that lead to a simpler name, with the reversed return value?

The opposite is either TBT-alt or disconnected mode, so the reversal
would result in the same function name format.

> BR,
> Jani.
> 
> 
> 
> >
> >> This is a nitpick, obviously, so I'll leave it up to you.
> >> 
> >> Regardless:
> >> 
> >> Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
> >> 
> >> --
> >> Cheers,
> >> Luca.
> >> 
> >> >  {
> >> >  	struct intel_display *display = to_intel_display(tc->dig_port);
> >> >  
> >> > -	drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
> >> > +	if (DISPLAY_VER(display) < 20) {
> >> > +		drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
> >> >  
> >> > -	return phy_is_ready && phy_is_owned;
> >> > +		return phy_is_ready && phy_is_owned;
> >> > +	} else {
> >> > +		return phy_is_owned;
> >> > +	}
> >> >  }
> >> >  
> >> >  static bool tc_phy_is_connected(struct intel_tc_port *tc,
> >> > @@ -1244,7 +1248,7 @@ static bool tc_phy_is_connected(struct intel_tc_port *tc,
> >> >  	bool phy_is_owned = tc_phy_is_owned(tc);
> >> >  	bool is_connected;
> >> >  
> >> > -	if (tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned))
> >> > +	if (tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned))
> >> >  		is_connected = port_pll_type == ICL_PORT_DPLL_MG_PHY;
> >> >  	else
> >> >  		is_connected = port_pll_type == ICL_PORT_DPLL_DEFAULT;
> >> > @@ -1352,7 +1356,7 @@ tc_phy_get_current_mode(struct intel_tc_port *tc)
> >> >  	phy_is_ready = tc_phy_is_ready(tc);
> >> >  	phy_is_owned = tc_phy_is_owned(tc);
> >> >  
> >> > -	if (!tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned)) {
> >> > +	if (!tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned)) {
> >> >  		mode = get_tc_mode_in_phy_not_owned_state(tc, live_mode);
> >> >  	} else {
> >> >  		drm_WARN_ON(display->drm, live_mode == TC_PORT_TBT_ALT);
> 
> -- 
> Jani Nikula, Intel

Re: [PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[Imre Deak]

On Thu, Aug 07, 2025 at 03:33:04PM +0300, Imre Deak wrote:
> On Thu, Aug 07, 2025 at 03:19:17PM +0300, Jani Nikula wrote:
> > On Thu, 07 Aug 2025, Imre Deak <imre.deak@intel.com> wrote:
> > > On Thu, Aug 07, 2025 at 01:59:21PM +0300, Luca Coelho wrote:
> > >> On Tue, 2025-08-05 at 10:36 +0300, Imre Deak wrote:
> > >> > The TypeC PHY HW readout during driver loading and system resume
> > >> > determines which TypeC mode the PHY is in (legacy/DP-alt/TBT-alt) and
> > >> > whether the PHY is connected, based on the PHY's Owned and Ready flags.
> > >> > For the PHY to be in DP-alt or legacy mode and for the PHY to be in the
> > >> > connected state in these modes, both the Owned (set by the BIOS/driver)
> > >> > and the Ready (set by the HW) flags should be set.
> > >> > 
> > >> > On ICL-MTL the HW kept the PHY's Ready flag set after the driver
> > >> > connected the PHY by acquiring the PHY ownership (by setting the Owned
> > >> > flag), until the driver disconnected the PHY by releasing the PHY
> > >> > ownership (by clearing the Owned flag). On LNL+ this has changed, in
> > >> > that the HW clears the Ready flag as soon as the sink gets disconnected,
> > >> > even if the PHY ownership was acquired already and hence the PHY is
> > >> > being used by the display.
> > >> > 
> > >> > When inheriting the HW state from BIOS for a PHY connected in DP-alt
> > >> > mode on which the sink got disconnected - i.e. in a case where the sink
> > >> > was connected while BIOS/GOP was running and so the sink got enabled
> > >> > connecting the PHY, but the user disconnected the sink by the time the
> > >> > driver loaded - the PHY Owned but not Ready state must be accounted for
> > >> > on LNL+ according to the above. Do that by assuming on LNL+ that the PHY
> > >> > is connected in DP-alt mode whenever the PHY Owned flag is set,
> > >> > regardless of the PHY Ready flag.
> > >> > 
> > >> > This fixes a problem on LNL+, where the PHY TypeC mode / connected state
> > >> > was detected incorrectly for a DP-alt sink, which got connected and then
> > >> > disconnected by the user in the above way.
> > >> > 
> > >> > Cc: stable@vger.kernel.org # v6.8+
> > >> > Reported-by: Charlton Lin <charlton.lin@intel.com>
> > >> > Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
> > >> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > >> > ---
> > >> >  drivers/gpu/drm/i915/display/intel_tc.c | 16 ++++++++++------
> > >> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > >> > 
> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > >> > index 3bc57579fe53e..73a08bd84a70a 100644
> > >> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > >> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > >> > @@ -1226,14 +1226,18 @@ static void tc_phy_get_hw_state(struct intel_tc_port *tc)
> > >> >  	tc->phy_ops->get_hw_state(tc);
> > >> >  }
> > >> >  
> > >> > -static bool tc_phy_is_ready_and_owned(struct intel_tc_port *tc,
> > >> > -				      bool phy_is_ready, bool phy_is_owned)
> > >> > +static bool tc_phy_in_legacy_or_dp_alt_mode(struct intel_tc_port *tc,
> > >> > +					    bool phy_is_ready, bool phy_is_owned)
> > >> 
> > >> Personally I don't like the "or" in the function name.  You're
> > >> returning a boolean which is true or false.  The return value is akin
> > >> to answering "Yes/No" to the question "Is it black or white".
> > >
> > > The question the function is meant to answer is "Is the PHY in legacy or
> > > DP-alt mode?". The return value is true (yes) if the PHY is in either
> > > legacy or DP-alt mode, false (no) if the PHY is neither in legacy or
> > > DP-alt mode. There are many other uses of "or" in function names in this
> > > sense, so not sure how else I'd name this one. Simply leaving out "or"
> > > would make it less clear that the legacy and DP-alt modes are two
> > > separate modes.
> > 
> > What's the opposite of "Is the PHY in legacy or DP-alt mode"?
> >
> > Would that lead to a simpler name, with the reversed return value?
> 
> The opposite is either TBT-alt or disconnected mode, so the reversal
> would result in the same function name format.

Would you be ok with

tc_phy_owned_by_display()

?

> 
> > BR,
> > Jani.
> > 
> > 
> > 
> > >
> > >> This is a nitpick, obviously, so I'll leave it up to you.
> > >> 
> > >> Regardless:
> > >> 
> > >> Reviewed-by: Luca Coelho <luciano.coelho@intel.com>
> > >> 
> > >> --
> > >> Cheers,
> > >> Luca.
> > >> 
> > >> >  {
> > >> >  	struct intel_display *display = to_intel_display(tc->dig_port);
> > >> >  
> > >> > -	drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
> > >> > +	if (DISPLAY_VER(display) < 20) {
> > >> > +		drm_WARN_ON(display->drm, phy_is_owned && !phy_is_ready);
> > >> >  
> > >> > -	return phy_is_ready && phy_is_owned;
> > >> > +		return phy_is_ready && phy_is_owned;
> > >> > +	} else {
> > >> > +		return phy_is_owned;
> > >> > +	}
> > >> >  }
> > >> >  
> > >> >  static bool tc_phy_is_connected(struct intel_tc_port *tc,
> > >> > @@ -1244,7 +1248,7 @@ static bool tc_phy_is_connected(struct intel_tc_port *tc,
> > >> >  	bool phy_is_owned = tc_phy_is_owned(tc);
> > >> >  	bool is_connected;
> > >> >  
> > >> > -	if (tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned))
> > >> > +	if (tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned))
> > >> >  		is_connected = port_pll_type == ICL_PORT_DPLL_MG_PHY;
> > >> >  	else
> > >> >  		is_connected = port_pll_type == ICL_PORT_DPLL_DEFAULT;
> > >> > @@ -1352,7 +1356,7 @@ tc_phy_get_current_mode(struct intel_tc_port *tc)
> > >> >  	phy_is_ready = tc_phy_is_ready(tc);
> > >> >  	phy_is_owned = tc_phy_is_owned(tc);
> > >> >  
> > >> > -	if (!tc_phy_is_ready_and_owned(tc, phy_is_ready, phy_is_owned)) {
> > >> > +	if (!tc_phy_in_legacy_or_dp_alt_mode(tc, phy_is_ready, phy_is_owned)) {
> > >> >  		mode = get_tc_mode_in_phy_not_owned_state(tc, live_mode);
> > >> >  	} else {
> > >> >  		drm_WARN_ON(display->drm, live_mode == TC_PORT_TBT_ALT);
> > 
> > -- 
> > Jani Nikula, Intel

Re: [PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[Jani Nikula]

On Thu, 07 Aug 2025, Imre Deak <imre.deak@intel.com> wrote:
> Would you be ok with
>
> tc_phy_owned_by_display()

Sounds good to me. Maybe add a brief comment "Is the PHY in legacy or
DP-alt mode" or something above the function.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[Imre Deak]

On Thu, Aug 07, 2025 at 04:05:14PM +0300, Jani Nikula wrote:
> On Thu, 07 Aug 2025, Imre Deak <imre.deak@intel.com> wrote:
> > Would you be ok with
> >
> > tc_phy_owned_by_display()
> 
> Sounds good to me. Maybe add a brief comment "Is the PHY in legacy or
> DP-alt mode" or something above the function.

Ok.

> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel

Re: [PATCH 01/19] drm/i915/lnl+/tc: Fix handling of an enabled/disconnected dp-alt sink

[Luca Coelho]

On Thu, 2025-08-07 at 15:50 +0300, Imre Deak wrote:
> On Thu, Aug 07, 2025 at 03:33:04PM +0300, Imre Deak wrote:
> > On Thu, Aug 07, 2025 at 03:19:17PM +0300, Jani Nikula wrote:
> > > On Thu, 07 Aug 2025, Imre Deak <imre.deak@intel.com> wrote:
> > > > On Thu, Aug 07, 2025 at 01:59:21PM +0300, Luca Coelho wrote:
> > > > > On Tue, 2025-08-05 at 10:36 +0300, Imre Deak wrote:
> > > > > > The TypeC PHY HW readout during driver loading and system resume
> > > > > > determines which TypeC mode the PHY is in (legacy/DP-alt/TBT-alt) and
> > > > > > whether the PHY is connected, based on the PHY's Owned and Ready flags.
> > > > > > For the PHY to be in DP-alt or legacy mode and for the PHY to be in the
> > > > > > connected state in these modes, both the Owned (set by the BIOS/driver)
> > > > > > and the Ready (set by the HW) flags should be set.
> > > > > > 
> > > > > > On ICL-MTL the HW kept the PHY's Ready flag set after the driver
> > > > > > connected the PHY by acquiring the PHY ownership (by setting the Owned
> > > > > > flag), until the driver disconnected the PHY by releasing the PHY
> > > > > > ownership (by clearing the Owned flag). On LNL+ this has changed, in
> > > > > > that the HW clears the Ready flag as soon as the sink gets disconnected,
> > > > > > even if the PHY ownership was acquired already and hence the PHY is
> > > > > > being used by the display.
> > > > > > 
> > > > > > When inheriting the HW state from BIOS for a PHY connected in DP-alt
> > > > > > mode on which the sink got disconnected - i.e. in a case where the sink
> > > > > > was connected while BIOS/GOP was running and so the sink got enabled
> > > > > > connecting the PHY, but the user disconnected the sink by the time the
> > > > > > driver loaded - the PHY Owned but not Ready state must be accounted for
> > > > > > on LNL+ according to the above. Do that by assuming on LNL+ that the PHY
> > > > > > is connected in DP-alt mode whenever the PHY Owned flag is set,
> > > > > > regardless of the PHY Ready flag.
> > > > > > 
> > > > > > This fixes a problem on LNL+, where the PHY TypeC mode / connected state
> > > > > > was detected incorrectly for a DP-alt sink, which got connected and then
> > > > > > disconnected by the user in the above way.
> > > > > > 
> > > > > > Cc: stable@vger.kernel.org # v6.8+
> > > > > > Reported-by: Charlton Lin <charlton.lin@intel.com>
> > > > > > Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_tc.c | 16 ++++++++++------
> > > > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > index 3bc57579fe53e..73a08bd84a70a 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > > > > @@ -1226,14 +1226,18 @@ static void tc_phy_get_hw_state(struct intel_tc_port *tc)
> > > > > >  	tc->phy_ops->get_hw_state(tc);
> > > > > >  }
> > > > > >  
> > > > > > -static bool tc_phy_is_ready_and_owned(struct intel_tc_port *tc,
> > > > > > -				      bool phy_is_ready, bool phy_is_owned)
> > > > > > +static bool tc_phy_in_legacy_or_dp_alt_mode(struct intel_tc_port *tc,
> > > > > > +					    bool phy_is_ready, bool phy_is_owned)
> > > > > 
> > > > > Personally I don't like the "or" in the function name.  You're
> > > > > returning a boolean which is true or false.  The return value is akin
> > > > > to answering "Yes/No" to the question "Is it black or white".
> > > > 
> > > > The question the function is meant to answer is "Is the PHY in legacy or
> > > > DP-alt mode?". The return value is true (yes) if the PHY is in either
> > > > legacy or DP-alt mode, false (no) if the PHY is neither in legacy or
> > > > DP-alt mode. There are many other uses of "or" in function names in this
> > > > sense, so not sure how else I'd name this one. Simply leaving out "or"
> > > > would make it less clear that the legacy and DP-alt modes are two
> > > > separate modes.

Right, I missed that.  But one shouldn't have to go read the function
implementation to know what it's doing.


> > > 
> > > What's the opposite of "Is the PHY in legacy or DP-alt mode"?
> > > 
> > > Would that lead to a simpler name, with the reversed return value?
> > 
> > The opposite is either TBT-alt or disconnected mode, so the reversal
> > would result in the same function name format.
> 
> Would you be ok with
> 
> tc_phy_owned_by_display()
> 
> ?

This sounds much better! And it's indeed what you're looking for.  It
doesn't matter if it's legacy DP-alt, TBT-alt or whatever.  What you're
checking is actually whether display owns the PHY.  So this is a much
better choice IMHO. :)

--
Cheers,
Luca.