[Intel-gfx] [PATCH v1 0/2] drm/i915/tc: Prevent system hang when modesetting disconnected Type-C ports

[Intel-gfx] [PATCH v1 0/2] drm/i915/tc: Prevent system hang when modesetting disconnected Type-C ports

[Vivek Kasireddy]

The following two patches try to prevent a system hang when a modeset
is forced by userspace (Weston) on legacy Type-C ports that are
disconnected. This issue was accidentally discovered while trying
to modeset one of the HDMI ports on the TGL based Gigabyte system
(https://www.gigabyte.com/Mini-PcBarebone/GB-BSi3-1115G4-rev-10#ov)
using the following Weston settings (configured via weston.ini):

[output]
name=HDMI-A-3
mode=173.00 1920 2048 2248 2576  1080 1083 1088 1120 -hsync +vsync
force-on=true

Entering the name of the HDMI connector incorrectly above (for example
HDMI-A-3 (disconnected) instead of HDMI-A-2 (connected)) lead to 
warnings in the log followed by a system hang. To fix this issue,
the first patch prevents the selection of TBT PLL for legacy Type-C
ports and the second one rejects any attempts to modeset disconnected
Type-C ports.

Cc: Imre Deak <imre.deak@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Vivek Kasireddy (2):
  drm/i915/tc: Don't default disconnected legacy Type-C ports to TBT
    mode
  drm/i915: Reject the atomic modeset if an associated Type-C port is
    disconnected

 drivers/gpu/drm/i915/display/intel_atomic.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_tc.c     |  3 ++-
 2 files changed, 22 insertions(+), 1 deletion(-)

-- 
2.35.1

[Intel-gfx] [PATCH v1 1/2] drm/i915/tc: Don't default disconnected legacy Type-C ports to TBT mode

[Vivek Kasireddy]

Commit 30e114ef4b16 ("drm/i915/tc: Check for DP-alt, legacy sinks before
taking PHY ownership") defaults any disconnected Type-C ports to TBT-alt
mode which presents a problem (which could most likely result in a system
hang) when userspace forces a modeset on a Type-C port that is wired for
legacy HDMI. The following warning is seen when Weston forces a modeset
on a disconnected legacy Type-C port (HDMI) on a TGL based Gigabyte system:
(https://www.gigabyte.com/Mini-PcBarebone/GB-BSi3-1115G4-rev-10#ov)

Missing case (clock == 173000)
WARNING: CPU: 1 PID: 438 at drivers/gpu/drm/i915/display/intel_ddi.c:245
icl_ddi_tc_enable_clock.cold+0x16a/0x1cf [i915]
CPU: 1 PID: 438 Comm: kworker/u8:3 Tainted: G     U  W   E
5.18.0-rc5-drm-tip+ #20
Hardware name: GIGABYTE GB-BSi3-1115G4/GB-BSi3-1115G4, BIOS F9
10/16/2021
Workqueue: i915_modeset intel_atomic_commit_work [i915]
RIP: 0010:icl_ddi_tc_enable_clock.cold+0x16a/0x1cf [i915]
Code: 74 6c 7f 10 81 fd d0 78 02 00 74 6d 81 fd b0 1e 04 00 74 70 48 63
d5 48 c7 c6 c0 7b ab c0 48 c7 c7 20 75 ab c0 e8 b8 b5 c1 f0 <0f> 0b 45
31 ed e9 fb fe ff ff 49 63 d5
 48 c7 c6 80 7b ab c0 48 c7
RSP: 0018:ffff8882522c78f0 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000000
RDX: 0000000000000027 RSI: 0000000000000004 RDI: ffffed104a458f10
RBP: 0000000000011558 R08: ffffffffb078de4e R09: ffff888269ca748b
R10: ffffed104d394e91 R11: 0000000000000000 R12: ffff888255a318f8
R13: 0000000000000002 R14: ffff888255a30000 R15: ffff88823ef00348
FS:  0000000000000000(0000) GS:ffff888269c80000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fd7afa42000 CR3: 0000000255c02004 CR4: 00000000007706e0
PKRU: 55555554
Call Trace:
<TASK>
intel_ddi_pre_enable.cold+0x96/0x5bf [i915]
intel_encoders_pre_enable+0x10e/0x140 [i915]
hsw_crtc_enable+0x207/0x99d [i915]
? ilk_crtc_enable.cold+0x2a/0x2a [i915]
? prepare_to_wait_exclusive+0x120/0x120
intel_enable_crtc+0x9a/0xf0 [i915]
skl_commit_modeset_enables+0x466/0x820 [i915]
? intel_commit_modeset_enables+0xd0/0xd0 [i915]
? intel_mbus_dbox_update+0x1ed/0x250 [i915]
intel_atomic_commit_tail+0xf2d/0x3040 [i915]
_raw_spin_lock_irqsave+0x87/0xe0
_raw_read_unlock_irqrestore+0x40/0x40
__update_load_avg_cfs_rq+0x70/0x5c0
__i915_sw_fence_complete+0x85/0x3b0 [i915]
? intel_get_crtc_new_encoder+0x190/0x190 [i915]
? sysvec_irq_work+0x13/0x90
? asm_sysvec_irq_work+0x12/0x20
? _raw_spin_lock_irq+0x82/0xd0
? read_word_at_a_time+0xe/0x20
? process_one_work+0x393/0x690
process_one_work+0x393/0x690
worker_thread+0x2b7/0x620
? process_one_work+0x690/0x690
kthread+0x15a/0x190
? kthread_complete_and_exit+0x20/0x20
ret_from_fork+0x1f/0x30

Continuing with the modeset without setting the DDI clock results in
more warnings and eventually a system hang. This does not seem to
happen with disconnected legacy or DP-alt DP ports because the clock
rate defaults to 162000 (which is a valid TBT clock) during the link
training process. Therefore, to fix this issue, this patch avoids
setting disconnected Type-C legacy ports to TBT-alt mode which prevents
the selection of TBT PLL when a modeset is forced.

Cc: Imre Deak <imre.deak@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index b8b822ea3755..0c3304be0602 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -494,7 +494,8 @@ static void icl_tc_phy_connect(struct intel_digital_port *dig_port,
 	}
 
 	live_status_mask = tc_port_live_status_mask(dig_port);
-	if (!(live_status_mask & (BIT(TC_PORT_DP_ALT) | BIT(TC_PORT_LEGACY)))) {
+	if (!(live_status_mask & BIT(TC_PORT_DP_ALT)) &&
+	    !dig_port->tc_legacy_port) {
 		drm_dbg_kms(&i915->drm, "Port %s: PHY ownership not required (live status %02x)\n",
 			    dig_port->tc_port_name, live_status_mask);
 		goto out_set_tbt_alt_mode;
-- 
2.35.1

[Intel-gfx] [PATCH v1 2/2] drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected

[Vivek Kasireddy]

Although, doing a modeset on any disconnected connector might be futile,
it can be particularly problematic if the connector is a Type-C port
without a sink. And, the spec only says "Display software must not use
a disconnected port" while referring to the Type-C DDI seqeuence, it does
not spell out what happens if such an attempt is made. Experimental results
have shown that this can lead to serious issues including a system hang.
Therefore, reject the atomic modeset if we detect that the Type-C port
is not connected.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index 40da7910f845..40576964b8c1 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -114,6 +114,8 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
 int intel_digital_connector_atomic_check(struct drm_connector *conn,
 					 struct drm_atomic_state *state)
 {
+	struct drm_device *dev = conn->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_connector_state *new_state =
 		drm_atomic_get_new_connector_state(state, conn);
 	struct intel_digital_connector_state *new_conn_state =
@@ -122,6 +124,10 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
 		drm_atomic_get_old_connector_state(state, conn);
 	struct intel_digital_connector_state *old_conn_state =
 		to_intel_digital_connector_state(old_state);
+	struct intel_encoder *encoder =
+		intel_attached_encoder(to_intel_connector(conn));
+	struct intel_digital_port *dig_port =
+		encoder ? enc_to_dig_port(encoder) : NULL;
 	struct drm_crtc_state *crtc_state;
 
 	intel_hdcp_atomic_check(conn, old_state, new_state);
@@ -131,6 +137,20 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
 
 	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
 
+	/*
+	 * The spec says that it is not safe to use a disconnected Type-C port.
+	 * Therefore, check to see if this connector is connected and reject
+	 * the modeset if there is no sink detected.
+	 */
+	if (dig_port && !dig_port->connected(encoder) &&
+	    intel_phy_is_tc(dev_priv,
+	    intel_port_to_phy(dev_priv, encoder->port))) {
+		drm_dbg_atomic(&dev_priv->drm,
+			       "[CONNECTOR:%d:%s] is not connected; rejecting the modeset\n",
+			       conn->base.id, conn->name);
+		return -EINVAL;
+	}
+
 	/*
 	 * These properties are handled by fastset, and might not end
 	 * up in a modeset.
-- 
2.35.1

Re: [Intel-gfx] [PATCH v1 2/2] drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected

[Jani Nikula]

On Mon, 16 May 2022, Vivek Kasireddy <vivek.kasireddy@intel.com> wrote:
> Although, doing a modeset on any disconnected connector might be futile,
> it can be particularly problematic if the connector is a Type-C port
> without a sink. And, the spec only says "Display software must not use
> a disconnected port" while referring to the Type-C DDI seqeuence, it does
> not spell out what happens if such an attempt is made. Experimental results
> have shown that this can lead to serious issues including a system hang.
> Therefore, reject the atomic modeset if we detect that the Type-C port
> is not connected.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 40da7910f845..40576964b8c1 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -114,6 +114,8 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
>  int intel_digital_connector_atomic_check(struct drm_connector *conn,
>  					 struct drm_atomic_state *state)
>  {
> +	struct drm_device *dev = conn->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);

Please replace the above two with:

	struct drm_i915_private *i915 = to_i915(conn->dev);

Please don't add drm_device local vars, and please name new struct
drm_i915_private pointers i915.

BR,
Jani.

>  	struct drm_connector_state *new_state =
>  		drm_atomic_get_new_connector_state(state, conn);
>  	struct intel_digital_connector_state *new_conn_state =
> @@ -122,6 +124,10 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
>  		drm_atomic_get_old_connector_state(state, conn);
>  	struct intel_digital_connector_state *old_conn_state =
>  		to_intel_digital_connector_state(old_state);
> +	struct intel_encoder *encoder =
> +		intel_attached_encoder(to_intel_connector(conn));
> +	struct intel_digital_port *dig_port =
> +		encoder ? enc_to_dig_port(encoder) : NULL;
>  	struct drm_crtc_state *crtc_state;
>  
>  	intel_hdcp_atomic_check(conn, old_state, new_state);
> @@ -131,6 +137,20 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
>  
>  	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
>  
> +	/*
> +	 * The spec says that it is not safe to use a disconnected Type-C port.
> +	 * Therefore, check to see if this connector is connected and reject
> +	 * the modeset if there is no sink detected.
> +	 */
> +	if (dig_port && !dig_port->connected(encoder) &&
> +	    intel_phy_is_tc(dev_priv,
> +	    intel_port_to_phy(dev_priv, encoder->port))) {
> +		drm_dbg_atomic(&dev_priv->drm,
> +			       "[CONNECTOR:%d:%s] is not connected; rejecting the modeset\n",
> +			       conn->base.id, conn->name);
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * These properties are handled by fastset, and might not end
>  	 * up in a modeset.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH v1 2/2] drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected

[Imre Deak]

On Mon, May 16, 2022 at 01:54:02AM -0700, Vivek Kasireddy wrote:
> Although, doing a modeset on any disconnected connector might be futile,
> it can be particularly problematic if the connector is a Type-C port
> without a sink. And, the spec only says "Display software must not use
> a disconnected port" while referring to the Type-C DDI seqeuence, it does
> not spell out what happens if such an attempt is made. Experimental results
> have shown that this can lead to serious issues including a system hang.
> Therefore, reject the atomic modeset if we detect that the Type-C port
> is not connected.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 40da7910f845..40576964b8c1 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -114,6 +114,8 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
>  int intel_digital_connector_atomic_check(struct drm_connector *conn,
>  					 struct drm_atomic_state *state)
>  {
> +	struct drm_device *dev = conn->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_connector_state *new_state =
>  		drm_atomic_get_new_connector_state(state, conn);
>  	struct intel_digital_connector_state *new_conn_state =
> @@ -122,6 +124,10 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
>  		drm_atomic_get_old_connector_state(state, conn);
>  	struct intel_digital_connector_state *old_conn_state =
>  		to_intel_digital_connector_state(old_state);
> +	struct intel_encoder *encoder =
> +		intel_attached_encoder(to_intel_connector(conn));
> +	struct intel_digital_port *dig_port =
> +		encoder ? enc_to_dig_port(encoder) : NULL;
>  	struct drm_crtc_state *crtc_state;
>  
>  	intel_hdcp_atomic_check(conn, old_state, new_state);
> @@ -131,6 +137,20 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
>  
>  	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
>  
> +	/*
> +	 * The spec says that it is not safe to use a disconnected Type-C port.
> +	 * Therefore, check to see if this connector is connected and reject
> +	 * the modeset if there is no sink detected.
> +	 */
> +	if (dig_port && !dig_port->connected(encoder) &&

This check is racy, as right after dig_port->connected() returns true,
the port can become disconnected.

> +	    intel_phy_is_tc(dev_priv,
> +	    intel_port_to_phy(dev_priv, encoder->port))) {
> +		drm_dbg_atomic(&dev_priv->drm,
> +			       "[CONNECTOR:%d:%s] is not connected; rejecting the modeset\n",
> +			       conn->base.id, conn->name);
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * These properties are handled by fastset, and might not end
>  	 * up in a modeset.
> -- 
> 2.35.1
> 

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/tc: Prevent system hang when modesetting disconnected Type-C ports

[Patchwork]

== Series Details ==

Series: drm/i915/tc: Prevent system hang when modesetting disconnected Type-C ports
URL   : https://patchwork.freedesktop.org/series/104019/
State : warning

== Summary ==

Error: dim checkpatch failed
578eba87cd3b drm/i915/tc: Don't default disconnected legacy Type-C ports to TBT mode
b8502c62a890 drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected
-:57: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#57: FILE: drivers/gpu/drm/i915/display/intel_atomic.c:147:
+	    intel_phy_is_tc(dev_priv,
+	    intel_port_to_phy(dev_priv, encoder->port))) {

total: 0 errors, 0 warnings, 1 checks, 38 lines checked

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/tc: Prevent system hang when modesetting disconnected Type-C ports

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 17885 bytes --]

== Series Details ==

Series: drm/i915/tc: Prevent system hang when modesetting disconnected Type-C ports
URL   : https://patchwork.freedesktop.org/series/104019/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11656 -> Patchwork_104019v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_104019v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_104019v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/index.html

Participating hosts (41 -> 43)
------------------------------

  Additional (4): bat-adlm-1 bat-adln-1 fi-rkl-11600 bat-dg2-9 
  Missing    (2): fi-elk-e7500 fi-bsw-n3050 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_busy@basic@modeset:
    - fi-cfl-guc:         [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-cfl-guc/igt@kms_busy@basic@modeset.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-cfl-guc/igt@kms_busy@basic@modeset.html
    - fi-skl-6600u:       [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-skl-6600u/igt@kms_busy@basic@modeset.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-skl-6600u/igt@kms_busy@basic@modeset.html
    - fi-kbl-soraka:      [PASS][5] -> [DMESG-WARN][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-kbl-soraka/igt@kms_busy@basic@modeset.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-kbl-soraka/igt@kms_busy@basic@modeset.html
    - fi-ilk-650:         [PASS][7] -> [DMESG-WARN][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-ilk-650/igt@kms_busy@basic@modeset.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-ilk-650/igt@kms_busy@basic@modeset.html
    - fi-skl-6700k2:      [PASS][9] -> [DMESG-WARN][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-skl-6700k2/igt@kms_busy@basic@modeset.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-skl-6700k2/igt@kms_busy@basic@modeset.html
    - fi-rkl-guc:         [PASS][11] -> [DMESG-WARN][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-rkl-guc/igt@kms_busy@basic@modeset.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-rkl-guc/igt@kms_busy@basic@modeset.html
    - bat-dg1-6:          [PASS][13] -> [DMESG-WARN][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/bat-dg1-6/igt@kms_busy@basic@modeset.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-dg1-6/igt@kms_busy@basic@modeset.html
    - fi-hsw-g3258:       [PASS][15] -> [DMESG-WARN][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-hsw-g3258/igt@kms_busy@basic@modeset.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-hsw-g3258/igt@kms_busy@basic@modeset.html
    - fi-bsw-kefka:       [PASS][17] -> [DMESG-WARN][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-bsw-kefka/igt@kms_busy@basic@modeset.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-bsw-kefka/igt@kms_busy@basic@modeset.html
    - fi-skl-guc:         [PASS][19] -> [DMESG-WARN][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-skl-guc/igt@kms_busy@basic@modeset.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-skl-guc/igt@kms_busy@basic@modeset.html
    - fi-bdw-5557u:       [PASS][21] -> [DMESG-WARN][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-bdw-5557u/igt@kms_busy@basic@modeset.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-bdw-5557u/igt@kms_busy@basic@modeset.html
    - fi-rkl-11600:       NOTRUN -> [DMESG-WARN][23]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-rkl-11600/igt@kms_busy@basic@modeset.html
    - fi-cfl-8109u:       [PASS][24] -> [DMESG-WARN][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-cfl-8109u/igt@kms_busy@basic@modeset.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-cfl-8109u/igt@kms_busy@basic@modeset.html
    - fi-cfl-8700k:       [PASS][26] -> [DMESG-WARN][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-cfl-8700k/igt@kms_busy@basic@modeset.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-cfl-8700k/igt@kms_busy@basic@modeset.html
    - fi-adl-ddr5:        [PASS][28] -> [DMESG-WARN][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-adl-ddr5/igt@kms_busy@basic@modeset.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-adl-ddr5/igt@kms_busy@basic@modeset.html
    - fi-glk-j4005:       [PASS][30] -> [DMESG-WARN][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-glk-j4005/igt@kms_busy@basic@modeset.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-glk-j4005/igt@kms_busy@basic@modeset.html

  * igt@kms_force_connector_basic@force-connector-state:
    - fi-kbl-7567u:       [PASS][32] -> [DMESG-WARN][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-kbl-7567u/igt@kms_force_connector_basic@force-connector-state.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-kbl-7567u/igt@kms_force_connector_basic@force-connector-state.html
    - fi-bdw-gvtdvm:      [PASS][34] -> [DMESG-WARN][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-bdw-gvtdvm/igt@kms_force_connector_basic@force-connector-state.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-bdw-gvtdvm/igt@kms_force_connector_basic@force-connector-state.html
    - bat-dg1-5:          [PASS][36] -> [DMESG-WARN][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/bat-dg1-5/igt@kms_force_connector_basic@force-connector-state.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-dg1-5/igt@kms_force_connector_basic@force-connector-state.html
    - fi-kbl-guc:         [PASS][38] -> [DMESG-WARN][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-kbl-guc/igt@kms_force_connector_basic@force-connector-state.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-kbl-guc/igt@kms_force_connector_basic@force-connector-state.html

  
#### Warnings ####

  * igt@kms_busy@basic@modeset:
    - bat-adlp-4:         [DMESG-WARN][40] ([i915#3576]) -> [DMESG-WARN][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/bat-adlp-4/igt@kms_busy@basic@modeset.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-adlp-4/igt@kms_busy@basic@modeset.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_busy@basic@modeset:
    - {bat-dg2-9}:        NOTRUN -> [DMESG-WARN][42]
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-dg2-9/igt@kms_busy@basic@modeset.html
    - {bat-adlm-1}:       NOTRUN -> [DMESG-WARN][43]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-adlm-1/igt@kms_busy@basic@modeset.html
    - {bat-jsl-2}:        [PASS][44] -> [DMESG-WARN][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/bat-jsl-2/igt@kms_busy@basic@modeset.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-jsl-2/igt@kms_busy@basic@modeset.html
    - {bat-jsl-1}:        [PASS][46] -> [DMESG-WARN][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/bat-jsl-1/igt@kms_busy@basic@modeset.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-jsl-1/igt@kms_busy@basic@modeset.html
    - {fi-ehl-2}:         [PASS][48] -> [DMESG-WARN][49]
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-ehl-2/igt@kms_busy@basic@modeset.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-ehl-2/igt@kms_busy@basic@modeset.html
    - {bat-rpls-2}:       [PASS][50] -> [DMESG-WARN][51]
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/bat-rpls-2/igt@kms_busy@basic@modeset.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-rpls-2/igt@kms_busy@basic@modeset.html
    - {bat-adlp-6}:       [DMESG-WARN][52] ([i915#3576]) -> [DMESG-WARN][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/bat-adlp-6/igt@kms_busy@basic@modeset.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-adlp-6/igt@kms_busy@basic@modeset.html
    - {fi-jsl-1}:         [PASS][54] -> [DMESG-WARN][55]
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-jsl-1/igt@kms_busy@basic@modeset.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-jsl-1/igt@kms_busy@basic@modeset.html

  * igt@kms_flip@basic-flip-vs-dpms@b-hdmi-a1:
    - {bat-adln-1}:       NOTRUN -> [DMESG-WARN][56]
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-adln-1/igt@kms_flip@basic-flip-vs-dpms@b-hdmi-a1.html

  * igt@kms_force_connector_basic@force-connector-state:
    - {bat-dg2-8}:        [PASS][57] -> [DMESG-WARN][58]
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/bat-dg2-8/igt@kms_force_connector_basic@force-connector-state.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-dg2-8/igt@kms_force_connector_basic@force-connector-state.html

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

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

### CI changes ###

#### Issues hit ####

  * boot:
    - fi-snb-2600:        [PASS][59] -> [FAIL][60] ([i915#4338])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-snb-2600/boot.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-snb-2600/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-rkl-11600:       NOTRUN -> [SKIP][61] ([i915#2190])
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-rkl-11600/igt@gem_huc_copy@huc-copy.html

  * igt@gem_tiled_pread_basic:
    - fi-rkl-11600:       NOTRUN -> [SKIP][62] ([i915#3282])
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-rkl-11600/igt@gem_tiled_pread_basic.html

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-4770:        [PASS][63] -> [INCOMPLETE][64] ([i915#4785])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@requests:
    - fi-pnv-d510:        [PASS][65] -> [DMESG-FAIL][66] ([i915#4528])
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/fi-pnv-d510/igt@i915_selftest@live@requests.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-pnv-d510/igt@i915_selftest@live@requests.html

  * igt@runner@aborted:
    - fi-rkl-11600:       NOTRUN -> [FAIL][67] ([i915#4312])
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-rkl-11600/igt@runner@aborted.html
    - fi-ilk-650:         NOTRUN -> [FAIL][68] ([i915#4312])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-ilk-650/igt@runner@aborted.html
    - fi-bsw-kefka:       NOTRUN -> [FAIL][69] ([i915#4312])
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-bsw-kefka/igt@runner@aborted.html
    - fi-bdw-gvtdvm:      NOTRUN -> [FAIL][70] ([i915#4312])
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-bdw-gvtdvm/igt@runner@aborted.html
    - fi-cfl-8700k:       NOTRUN -> [FAIL][71] ([i915#4312] / [i915#5257])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-cfl-8700k/igt@runner@aborted.html
    - fi-skl-6600u:       NOTRUN -> [FAIL][72] ([i915#4312] / [i915#5257])
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-skl-6600u/igt@runner@aborted.html
    - fi-cfl-8109u:       NOTRUN -> [FAIL][73] ([i915#4312] / [i915#5257])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-cfl-8109u/igt@runner@aborted.html
    - bat-dg1-5:          NOTRUN -> [FAIL][74] ([i915#4312] / [i915#5257])
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-dg1-5/igt@runner@aborted.html
    - fi-bdw-5557u:       NOTRUN -> [FAIL][75] ([i915#4312])
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-bdw-5557u/igt@runner@aborted.html
    - fi-hsw-g3258:       NOTRUN -> [FAIL][76] ([i915#4312])
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-hsw-g3258/igt@runner@aborted.html
    - fi-kbl-soraka:      NOTRUN -> [FAIL][77] ([i915#4312] / [i915#5257])
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-kbl-soraka/igt@runner@aborted.html
    - fi-hsw-4770:        NOTRUN -> [FAIL][78] ([fdo#109271] / [i915#4312] / [i915#5594])
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-hsw-4770/igt@runner@aborted.html
    - fi-kbl-guc:         NOTRUN -> [FAIL][79] ([i915#4312] / [i915#5257])
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-kbl-guc/igt@runner@aborted.html
    - bat-adlp-4:         NOTRUN -> [FAIL][80] ([i915#4312])
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-adlp-4/igt@runner@aborted.html
    - fi-rkl-guc:         NOTRUN -> [FAIL][81] ([i915#4312])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-rkl-guc/igt@runner@aborted.html
    - fi-adl-ddr5:        NOTRUN -> [FAIL][82] ([i915#4312])
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-adl-ddr5/igt@runner@aborted.html
    - bat-dg1-6:          NOTRUN -> [FAIL][83] ([i915#4312] / [i915#5257])
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-dg1-6/igt@runner@aborted.html
    - fi-cfl-guc:         NOTRUN -> [FAIL][84] ([i915#4312] / [i915#5257])
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-cfl-guc/igt@runner@aborted.html
    - fi-glk-j4005:       NOTRUN -> [FAIL][85] ([i915#4312] / [i915#5257])
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-glk-j4005/igt@runner@aborted.html
    - fi-kbl-7567u:       NOTRUN -> [FAIL][86] ([i915#4312] / [i915#5257])
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-kbl-7567u/igt@runner@aborted.html
    - fi-skl-guc:         NOTRUN -> [FAIL][87] ([i915#4312] / [i915#5257])
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-skl-guc/igt@runner@aborted.html
    - fi-skl-6700k2:      NOTRUN -> [FAIL][88] ([i915#4312] / [i915#5257])
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/fi-skl-6700k2/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@kms_busy@basic@flip:
    - {bat-adlp-6}:       [DMESG-WARN][89] ([i915#3576]) -> [PASS][90]
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11656/bat-adlp-6/igt@kms_busy@basic@flip.html
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/bat-adlp-6/igt@kms_busy@basic@flip.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3576]: https://gitlab.freedesktop.org/drm/intel/issues/3576
  [i915#3595]: https://gitlab.freedesktop.org/drm/intel/issues/3595
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4338]: https://gitlab.freedesktop.org/drm/intel/issues/4338
  [i915#4528]: https://gitlab.freedesktop.org/drm/intel/issues/4528
  [i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5257]: https://gitlab.freedesktop.org/drm/intel/issues/5257
  [i915#5594]: https://gitlab.freedesktop.org/drm/intel/issues/5594


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

  * Linux: CI_DRM_11656 -> Patchwork_104019v1

  CI-20190529: 20190529
  CI_DRM_11656: 416780e079b848ddd4da752cb90b619b97eb773e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6472: c815c94f0ceb33ae852622538f0136cf44c5725d @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_104019v1: 416780e079b848ddd4da752cb90b619b97eb773e @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

7329eafbafb4 drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected
7bcaf190f5b9 drm/i915/tc: Don't default disconnected legacy Type-C ports to TBT mode

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104019v1/index.html

[-- Attachment #2: Type: text/html, Size: 21117 bytes --]

Re: [Intel-gfx] [PATCH v1 1/2] drm/i915/tc: Don't default disconnected legacy Type-C ports to TBT mode

[Imre Deak]

On Mon, May 16, 2022 at 01:54:01AM -0700, Vivek Kasireddy wrote:
> Commit 30e114ef4b16 ("drm/i915/tc: Check for DP-alt, legacy sinks before
> taking PHY ownership") defaults any disconnected Type-C ports to TBT-alt
> mode which presents a problem (which could most likely result in a system
> hang) when userspace forces a modeset on a Type-C port that is wired for
> legacy HDMI. The following warning is seen when Weston forces a modeset
> on a disconnected legacy Type-C port (HDMI) on a TGL based Gigabyte system:
> (https://www.gigabyte.com/Mini-PcBarebone/GB-BSi3-1115G4-rev-10#ov)
> 
> Missing case (clock == 173000)
> WARNING: CPU: 1 PID: 438 at drivers/gpu/drm/i915/display/intel_ddi.c:245
> icl_ddi_tc_enable_clock.cold+0x16a/0x1cf [i915]
> CPU: 1 PID: 438 Comm: kworker/u8:3 Tainted: G     U  W   E
> 5.18.0-rc5-drm-tip+ #20
> Hardware name: GIGABYTE GB-BSi3-1115G4/GB-BSi3-1115G4, BIOS F9
> 10/16/2021
> Workqueue: i915_modeset intel_atomic_commit_work [i915]
> RIP: 0010:icl_ddi_tc_enable_clock.cold+0x16a/0x1cf [i915]
> Code: 74 6c 7f 10 81 fd d0 78 02 00 74 6d 81 fd b0 1e 04 00 74 70 48 63
> d5 48 c7 c6 c0 7b ab c0 48 c7 c7 20 75 ab c0 e8 b8 b5 c1 f0 <0f> 0b 45
> 31 ed e9 fb fe ff ff 49 63 d5
>  48 c7 c6 80 7b ab c0 48 c7
> RSP: 0018:ffff8882522c78f0 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000000
> RDX: 0000000000000027 RSI: 0000000000000004 RDI: ffffed104a458f10
> RBP: 0000000000011558 R08: ffffffffb078de4e R09: ffff888269ca748b
> R10: ffffed104d394e91 R11: 0000000000000000 R12: ffff888255a318f8
> R13: 0000000000000002 R14: ffff888255a30000 R15: ffff88823ef00348
> FS:  0000000000000000(0000) GS:ffff888269c80000(0000)
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fd7afa42000 CR3: 0000000255c02004 CR4: 00000000007706e0
> PKRU: 55555554
> Call Trace:
> <TASK>
> intel_ddi_pre_enable.cold+0x96/0x5bf [i915]
> intel_encoders_pre_enable+0x10e/0x140 [i915]
> hsw_crtc_enable+0x207/0x99d [i915]
> ? ilk_crtc_enable.cold+0x2a/0x2a [i915]
> ? prepare_to_wait_exclusive+0x120/0x120
> intel_enable_crtc+0x9a/0xf0 [i915]
> skl_commit_modeset_enables+0x466/0x820 [i915]
> ? intel_commit_modeset_enables+0xd0/0xd0 [i915]
> ? intel_mbus_dbox_update+0x1ed/0x250 [i915]
> intel_atomic_commit_tail+0xf2d/0x3040 [i915]
> _raw_spin_lock_irqsave+0x87/0xe0
> _raw_read_unlock_irqrestore+0x40/0x40
> __update_load_avg_cfs_rq+0x70/0x5c0
> __i915_sw_fence_complete+0x85/0x3b0 [i915]
> ? intel_get_crtc_new_encoder+0x190/0x190 [i915]
> ? sysvec_irq_work+0x13/0x90
> ? asm_sysvec_irq_work+0x12/0x20
> ? _raw_spin_lock_irq+0x82/0xd0
> ? read_word_at_a_time+0xe/0x20
> ? process_one_work+0x393/0x690
> process_one_work+0x393/0x690
> worker_thread+0x2b7/0x620
> ? process_one_work+0x690/0x690
> kthread+0x15a/0x190
> ? kthread_complete_and_exit+0x20/0x20
> ret_from_fork+0x1f/0x30
> 
> Continuing with the modeset without setting the DDI clock results in
> more warnings and eventually a system hang. This does not seem to
> happen with disconnected legacy or DP-alt DP ports because the clock
> rate defaults to 162000 (which is a valid TBT clock) during the link
> training process. Therefore, to fix this issue, this patch avoids
> setting disconnected Type-C legacy ports to TBT-alt mode which prevents
> the selection of TBT PLL when a modeset is forced.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_tc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index b8b822ea3755..0c3304be0602 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -494,7 +494,8 @@ static void icl_tc_phy_connect(struct intel_digital_port *dig_port,
>  	}
>  
>  	live_status_mask = tc_port_live_status_mask(dig_port);
> -	if (!(live_status_mask & (BIT(TC_PORT_DP_ALT) | BIT(TC_PORT_LEGACY)))) {
> +	if (!(live_status_mask & BIT(TC_PORT_DP_ALT)) &&
> +	    !dig_port->tc_legacy_port) {

Looks correct, but checking for the legacy hotplug live state should be
kept to account for incorrect VBTs.

>  		drm_dbg_kms(&i915->drm, "Port %s: PHY ownership not required (live status %02x)\n",
>  			    dig_port->tc_port_name, live_status_mask);
>  		goto out_set_tbt_alt_mode;
> -- 
> 2.35.1
> 

Re: [Intel-gfx] [PATCH v1 2/2] drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected

[Kasireddy, Vivek]

Hi Imre,

> On Mon, May 16, 2022 at 01:54:02AM -0700, Vivek Kasireddy wrote:
> > Although, doing a modeset on any disconnected connector might be futile,
> > it can be particularly problematic if the connector is a Type-C port
> > without a sink. And, the spec only says "Display software must not use
> > a disconnected port" while referring to the Type-C DDI seqeuence, it does
> > not spell out what happens if such an attempt is made. Experimental results
> > have shown that this can lead to serious issues including a system hang.
> > Therefore, reject the atomic modeset if we detect that the Type-C port
> > is not connected.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_atomic.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 40da7910f845..40576964b8c1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -114,6 +114,8 @@ int intel_digital_connector_atomic_set_property(struct
> drm_connector *connector,
> >  int intel_digital_connector_atomic_check(struct drm_connector *conn,
> >  					 struct drm_atomic_state *state)
> >  {
> > +	struct drm_device *dev = conn->dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct drm_connector_state *new_state =
> >  		drm_atomic_get_new_connector_state(state, conn);
> >  	struct intel_digital_connector_state *new_conn_state =
> > @@ -122,6 +124,10 @@ int intel_digital_connector_atomic_check(struct
> drm_connector *conn,
> >  		drm_atomic_get_old_connector_state(state, conn);
> >  	struct intel_digital_connector_state *old_conn_state =
> >  		to_intel_digital_connector_state(old_state);
> > +	struct intel_encoder *encoder =
> > +		intel_attached_encoder(to_intel_connector(conn));
> > +	struct intel_digital_port *dig_port =
> > +		encoder ? enc_to_dig_port(encoder) : NULL;
> >  	struct drm_crtc_state *crtc_state;
> >
> >  	intel_hdcp_atomic_check(conn, old_state, new_state);
> > @@ -131,6 +137,20 @@ int intel_digital_connector_atomic_check(struct
> drm_connector *conn,
> >
> >  	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> >
> > +	/*
> > +	 * The spec says that it is not safe to use a disconnected Type-C port.
> > +	 * Therefore, check to see if this connector is connected and reject
> > +	 * the modeset if there is no sink detected.
> > +	 */
> > +	if (dig_port && !dig_port->connected(encoder) &&
> 
> This check is racy, as right after dig_port->connected() returns true,
> the port can become disconnected.
[Kasireddy, Vivek] Given that, do you think the only way to reliably determine
if the Type-C port has a sink is to check the live status and ignore dig_port->tc_mode? 

If that is the case, should I just add a function pointer to dig_port to call
tc_port_live_status_mask()? Or, should I just change intel_tc_port_connected()
to ignore dig_port->tc_mode like below:
@@ -764,8 +764,7 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)

        intel_tc_port_lock(dig_port);

-       is_connected = tc_port_live_status_mask(dig_port) &
-                      BIT(dig_port->tc_mode);
+       is_connected = tc_port_live_status_mask(dig_port);

Or, are there any other elegant ways that you can think of to determine whether 
a tc port has a sink or not?

Thanks,
Vivek

> 
> > +	    intel_phy_is_tc(dev_priv,
> > +	    intel_port_to_phy(dev_priv, encoder->port))) {
> > +		drm_dbg_atomic(&dev_priv->drm,
> > +			       "[CONNECTOR:%d:%s] is not connected; rejecting the
> modeset\n",
> > +			       conn->base.id, conn->name);
> > +		return -EINVAL;
> > +	}
> > +
> >  	/*
> >  	 * These properties are handled by fastset, and might not end
> >  	 * up in a modeset.
> > --
> > 2.35.1
> >

Re: [Intel-gfx] [PATCH v1 2/2] drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected

[Imre Deak]

On Wed, May 18, 2022 at 10:04:14AM +0300, Kasireddy, Vivek wrote:
> Hi Imre,
> 
> > On Mon, May 16, 2022 at 01:54:02AM -0700, Vivek Kasireddy wrote:
> > > Although, doing a modeset on any disconnected connector might be futile,
> > > it can be particularly problematic if the connector is a Type-C port
> > > without a sink. And, the spec only says "Display software must not use
> > > a disconnected port" while referring to the Type-C DDI seqeuence, it does
> > > not spell out what happens if such an attempt is made. Experimental results
> > > have shown that this can lead to serious issues including a system hang.
> > > Therefore, reject the atomic modeset if we detect that the Type-C port
> > > is not connected.
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_atomic.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > index 40da7910f845..40576964b8c1 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > @@ -114,6 +114,8 @@ int intel_digital_connector_atomic_set_property(struct
> > drm_connector *connector,
> > >  int intel_digital_connector_atomic_check(struct drm_connector *conn,
> > >  					 struct drm_atomic_state *state)
> > >  {
> > > +	struct drm_device *dev = conn->dev;
> > > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct drm_connector_state *new_state =
> > >  		drm_atomic_get_new_connector_state(state, conn);
> > >  	struct intel_digital_connector_state *new_conn_state =
> > > @@ -122,6 +124,10 @@ int intel_digital_connector_atomic_check(struct
> > drm_connector *conn,
> > >  		drm_atomic_get_old_connector_state(state, conn);
> > >  	struct intel_digital_connector_state *old_conn_state =
> > >  		to_intel_digital_connector_state(old_state);
> > > +	struct intel_encoder *encoder =
> > > +		intel_attached_encoder(to_intel_connector(conn));
> > > +	struct intel_digital_port *dig_port =
> > > +		encoder ? enc_to_dig_port(encoder) : NULL;
> > >  	struct drm_crtc_state *crtc_state;
> > >
> > >  	intel_hdcp_atomic_check(conn, old_state, new_state);
> > > @@ -131,6 +137,20 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
> > >
> > >  	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> > >
> > > +	/*
> > > +	 * The spec says that it is not safe to use a disconnected Type-C port.
> > > +	 * Therefore, check to see if this connector is connected and reject
> > > +	 * the modeset if there is no sink detected.
> > > +	 */
> > > +	if (dig_port && !dig_port->connected(encoder) &&
> > 
> > This check is racy, as right after dig_port->connected() returns true,
> > the port can become disconnected.
>
> [Kasireddy, Vivek] Given that, do you think the only way to reliably determine
> if the Type-C port has a sink is to check the live status and ignore dig_port->tc_mode? 
>
> If that is the case, should I just add a function pointer to dig_port to call
> tc_port_live_status_mask()? Or, should I just change intel_tc_port_connected()
> to ignore dig_port->tc_mode like below:
> @@ -764,8 +764,7 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> 
>         intel_tc_port_lock(dig_port);
> 
> -       is_connected = tc_port_live_status_mask(dig_port) &
> -                      BIT(dig_port->tc_mode);
> +       is_connected = tc_port_live_status_mask(dig_port);
> 
> Or, are there any other elegant ways that you can think of to determine whether 
> a tc port has a sink or not?

I meant that I don't think there is a way to prevent a modeset on a
disconnected port. Live status is what provides the connected state, but
it can change right after it is read out.

> Thanks,
> Vivek
> 
> > 
> > > +	    intel_phy_is_tc(dev_priv,
> > > +	    intel_port_to_phy(dev_priv, encoder->port))) {
> > > +		drm_dbg_atomic(&dev_priv->drm,
> > > +			       "[CONNECTOR:%d:%s] is not connected; rejecting the
> > modeset\n",
> > > +			       conn->base.id, conn->name);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	/*
> > >  	 * These properties are handled by fastset, and might not end
> > >  	 * up in a modeset.
> > > --
> > > 2.35.1
> > >

Re: [Intel-gfx] [PATCH v1 2/2] drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected

[Kasireddy, Vivek]

Hi Imre,

> On Wed, May 18, 2022 at 10:04:14AM +0300, Kasireddy, Vivek wrote:
> > Hi Imre,
> >
> > > On Mon, May 16, 2022 at 01:54:02AM -0700, Vivek Kasireddy wrote:
> > > > Although, doing a modeset on any disconnected connector might be futile,
> > > > it can be particularly problematic if the connector is a Type-C port
> > > > without a sink. And, the spec only says "Display software must not use
> > > > a disconnected port" while referring to the Type-C DDI seqeuence, it does
> > > > not spell out what happens if such an attempt is made. Experimental results
> > > > have shown that this can lead to serious issues including a system hang.
> > > > Therefore, reject the atomic modeset if we detect that the Type-C port
> > > > is not connected.
> > > >
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_atomic.c | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > index 40da7910f845..40576964b8c1 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > @@ -114,6 +114,8 @@ int intel_digital_connector_atomic_set_property(struct
> > > drm_connector *connector,
> > > >  int intel_digital_connector_atomic_check(struct drm_connector *conn,
> > > >  					 struct drm_atomic_state *state)
> > > >  {
> > > > +	struct drm_device *dev = conn->dev;
> > > > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > > >  	struct drm_connector_state *new_state =
> > > >  		drm_atomic_get_new_connector_state(state, conn);
> > > >  	struct intel_digital_connector_state *new_conn_state =
> > > > @@ -122,6 +124,10 @@ int intel_digital_connector_atomic_check(struct
> > > drm_connector *conn,
> > > >  		drm_atomic_get_old_connector_state(state, conn);
> > > >  	struct intel_digital_connector_state *old_conn_state =
> > > >  		to_intel_digital_connector_state(old_state);
> > > > +	struct intel_encoder *encoder =
> > > > +		intel_attached_encoder(to_intel_connector(conn));
> > > > +	struct intel_digital_port *dig_port =
> > > > +		encoder ? enc_to_dig_port(encoder) : NULL;
> > > >  	struct drm_crtc_state *crtc_state;
> > > >
> > > >  	intel_hdcp_atomic_check(conn, old_state, new_state);
> > > > @@ -131,6 +137,20 @@ int intel_digital_connector_atomic_check(struct
> drm_connector *conn,
> > > >
> > > >  	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> > > >
> > > > +	/*
> > > > +	 * The spec says that it is not safe to use a disconnected Type-C port.
> > > > +	 * Therefore, check to see if this connector is connected and reject
> > > > +	 * the modeset if there is no sink detected.
> > > > +	 */
> > > > +	if (dig_port && !dig_port->connected(encoder) &&
> > >
> > > This check is racy, as right after dig_port->connected() returns true,
> > > the port can become disconnected.
> >
> > [Kasireddy, Vivek] Given that, do you think the only way to reliably determine
> > if the Type-C port has a sink is to check the live status and ignore dig_port->tc_mode?
> >
> > If that is the case, should I just add a function pointer to dig_port to call
> > tc_port_live_status_mask()? Or, should I just change intel_tc_port_connected()
> > to ignore dig_port->tc_mode like below:
> > @@ -764,8 +764,7 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> >
> >         intel_tc_port_lock(dig_port);
> >
> > -       is_connected = tc_port_live_status_mask(dig_port) &
> > -                      BIT(dig_port->tc_mode);
> > +       is_connected = tc_port_live_status_mask(dig_port);
> >
> > Or, are there any other elegant ways that you can think of to determine whether
> > a tc port has a sink or not?
> 
> I meant that I don't think there is a way to prevent a modeset on a
> disconnected port.
But we need to find a way right given that the spec clearly states that the driver
must not use or access (PHY/FIA registers of) a disconnected tc port. 

> Live status is what provides the connected state, but
> it can change right after it is read out.
Does this change happen after giving up the ownership (in icl_tc_phy_disconnect)?
But shouldn't we distinguish between the cases where we are deliberately disconnecting
the phy for power-savings reason vs when the port actually becomes disconnected?
The port can still be considered connected in the former case right?

Under what other situations would the live status change or become unreliable
after the port has a connected sink? And, since we rely on SDEISR to detect the
live status for tc legacy ports, could this not be considered reliable?

Thanks,
Vivek

> 
> > Thanks,
> > Vivek
> >
> > >
> > > > +	    intel_phy_is_tc(dev_priv,
> > > > +	    intel_port_to_phy(dev_priv, encoder->port))) {
> > > > +		drm_dbg_atomic(&dev_priv->drm,
> > > > +			       "[CONNECTOR:%d:%s] is not connected; rejecting the
> > > modeset\n",
> > > > +			       conn->base.id, conn->name);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > >  	/*
> > > >  	 * These properties are handled by fastset, and might not end
> > > >  	 * up in a modeset.
> > > > --
> > > > 2.35.1
> > > >

Re: [Intel-gfx] [PATCH v1 2/2] drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected

[Imre Deak]

On Fri, May 20, 2022 at 10:28:31AM +0300, Kasireddy, Vivek wrote:
> Hi Imre,
> [...]
> > > > > @@ -131,6 +137,20 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
> > > > >
> > > > >  	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> > > > >
> > > > > +	/*
> > > > > +	 * The spec says that it is not safe to use a disconnected Type-C port.
> > > > > +	 * Therefore, check to see if this connector is connected and reject
> > > > > +	 * the modeset if there is no sink detected.
> > > > > +	 */
> > > > > +	if (dig_port && !dig_port->connected(encoder) &&
> > > >
> > > > This check is racy, as right after dig_port->connected() returns true,
> > > > the port can become disconnected.
> > >
> > > [Kasireddy, Vivek] Given that, do you think the only way to reliably determine
> > > if the Type-C port has a sink is to check the live status and ignore dig_port->tc_mode?
> > >
> > > If that is the case, should I just add a function pointer to dig_port to call
> > > tc_port_live_status_mask()? Or, should I just change intel_tc_port_connected()
> > > to ignore dig_port->tc_mode like below:
> > > @@ -764,8 +764,7 @@ bool intel_tc_port_connected(struct intel_encoder *encoder)
> > >
> > >         intel_tc_port_lock(dig_port);
> > >
> > > -       is_connected = tc_port_live_status_mask(dig_port) &
> > > -                      BIT(dig_port->tc_mode);
> > > +       is_connected = tc_port_live_status_mask(dig_port);
> > >
> > > Or, are there any other elegant ways that you can think of to determine whether
> > > a tc port has a sink or not?
> > 
> > I meant that I don't think there is a way to prevent a modeset on a
> > disconnected port.
>
> But we need to find a way right given that the spec clearly states that the driver
> must not use or access (PHY/FIA registers of) a disconnected tc port. 

The driver does not access the PHY/FIA regs on a disconnected port/PHY.

> > Live status is what provides the connected state, but
> > it can change right after it is read out.
>
> Does this change happen after giving up the ownership (in
> icl_tc_phy_disconnect)?

The HPD live status changes whenever a user plugs/unplugs a sink.

> But shouldn't we distinguish between the cases where we are
> deliberately disconnecting the phy for power-savings reason vs when
> the port actually becomes disconnected? The port can still be
> considered connected in the former case right?

The driver - based on the spec - needs to avoid accessing the PHY/FIA
regs whenever the PHY is disconnected either by FW/HW (because the user
unplugged the sink) or the driver (during the suspend, modeset disable
sequence).

> Under what other situations would the live status change or become
> unreliable after the port has a connected sink?

It's not unreliable, it reflects the state of a sink being plugged to
the connector or not.

> And, since we rely on SDEISR to detect the live status for tc legacy
> ports, could this not be considered reliable?

Changes in the HPD live status is used as a hint to user space to
follow up with connector detection and modeset enable/disable requests
as necessary.

--Imre

Re: [Intel-gfx] [PATCH v1 2/2] drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected

[Kasireddy, Vivek]

Hi Imre,

> 
> On Fri, May 20, 2022 at 10:28:31AM +0300, Kasireddy, Vivek wrote:
> > Hi Imre,
> > [...]
> > > > > > @@ -131,6 +137,20 @@ int intel_digital_connector_atomic_check(struct
> drm_connector *conn,
> > > > > >
> > > > > >  	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> > > > > >
> > > > > > +	/*
> > > > > > +	 * The spec says that it is not safe to use a disconnected Type-C port.
> > > > > > +	 * Therefore, check to see if this connector is connected and reject
> > > > > > +	 * the modeset if there is no sink detected.
> > > > > > +	 */
> > > > > > +	if (dig_port && !dig_port->connected(encoder) &&
> > > > >
> > > > > This check is racy, as right after dig_port->connected() returns true,
> > > > > the port can become disconnected.
> > > >
> > > > [Kasireddy, Vivek] Given that, do you think the only way to reliably determine
> > > > if the Type-C port has a sink is to check the live status and ignore dig_port-
> >tc_mode?
> > > >
> > > > If that is the case, should I just add a function pointer to dig_port to call
> > > > tc_port_live_status_mask()? Or, should I just change intel_tc_port_connected()
> > > > to ignore dig_port->tc_mode like below:
> > > > @@ -764,8 +764,7 @@ bool intel_tc_port_connected(struct intel_encoder
> *encoder)
> > > >
> > > >         intel_tc_port_lock(dig_port);
> > > >
> > > > -       is_connected = tc_port_live_status_mask(dig_port) &
> > > > -                      BIT(dig_port->tc_mode);
> > > > +       is_connected = tc_port_live_status_mask(dig_port);
> > > >
> > > > Or, are there any other elegant ways that you can think of to determine whether
> > > > a tc port has a sink or not?
> > >
> > > I meant that I don't think there is a way to prevent a modeset on a
> > > disconnected port.
> >
> > But we need to find a way right given that the spec clearly states that the driver
> > must not use or access (PHY/FIA registers of) a disconnected tc port.
> 
> The driver does not access the PHY/FIA regs on a disconnected port/PHY.
[Kasireddy, Vivek] I think it does after the first patch in this series is applied if
the userspace (Weston) forces a modeset on a disconnected tc legacy port (HDMI).
For instance, some of the FIA/PHY regs accesses I noticed include programming
the lane count (intel_tc_port_set_fia_lane_count() called by intel_ddi_pre_pll_enable()),
reading the pin assignment mask (intel_tc_port_get_pin_assignment_mask() called
by icl_program_mg_dp_mode() which is called by intel_ddi_pre_enable_hdmi()), etc.

Of-course, these accesses would probably not occur if the disconnected tc port
defaults to TBT mode but this brings other problems like I described in the
commit description of the first patch and the cover letter.
 
> 
> > > Live status is what provides the connected state, but
> > > it can change right after it is read out.
> >
> > Does this change happen after giving up the ownership (in
> > icl_tc_phy_disconnect)?
> 
> The HPD live status changes whenever a user plugs/unplugs a sink.
> 
> > But shouldn't we distinguish between the cases where we are
> > deliberately disconnecting the phy for power-savings reason vs when
> > the port actually becomes disconnected? The port can still be
> > considered connected in the former case right?
> 
> The driver - based on the spec - needs to avoid accessing the PHY/FIA
> regs whenever the PHY is disconnected either by FW/HW (because the user
> unplugged the sink) or the driver (during the suspend, modeset disable
> sequence).
[Kasireddy, Vivek] Regardless of whether the PHY/FIA regs are accessed or
not, I don't think the driver should let the userspace's modeset to succeed on
a disconnected tc port. Do you not agree?

> 
> > Under what other situations would the live status change or become
> > unreliable after the port has a connected sink?
> 
> It's not unreliable, it reflects the state of a sink being plugged to
> the connector or not.
[Kasireddy, Vivek] Ok, assuming that the state of the sink is "connected"
during intel_atomic_check() phase (which is where this patch checks for
connected status), are you concerned about the case where the user may
unplug the sink before we get to the intel_atomic_commit() phase? Is
this what you meant when you said this earlier: "This check is racy, as
right after dig_port->connected() returns true, the port can become
disconnected"? I am just trying to figure out the scenarios when this
might happen.

> 
> > And, since we rely on SDEISR to detect the live status for tc legacy
> > ports, could this not be considered reliable?
> 
> Changes in the HPD live status is used as a hint to user space to
> follow up with connector detection and modeset enable/disable requests
> as necessary.
[Kasireddy, Vivek] Right, that is the ideal case but user/userspace can commit
mistakes where for example they can assume that HDMI-A-1 is connected 
(while it is not) instead of HDMI-A-3 which is the one actually connected.
During such cases, I think the driver should not let the userspace hang the
system or lead to unexpected states and instead should return an error.

Thanks,
Vivek

> 
> --Imre

Re: [Intel-gfx] [PATCH v1 2/2] drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected

[Imre Deak]

On Tue, May 24, 2022 at 11:29:54AM +0300, Kasireddy, Vivek wrote:
> Hi Imre,
> 
> > On Fri, May 20, 2022 at 10:28:31AM +0300, Kasireddy, Vivek wrote:
> > > Hi Imre,
> > > [...]
> > > > > > > @@ -131,6 +137,20 @@ int intel_digital_connector_atomic_check(struct
> > drm_connector *conn,
> > > > > > >
> > > > > > >  	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> > > > > > >
> > > > > > > +	/*
> > > > > > > +	 * The spec says that it is not safe to use a disconnected Type-C port.
> > > > > > > +	 * Therefore, check to see if this connector is connected and reject
> > > > > > > +	 * the modeset if there is no sink detected.
> > > > > > > +	 */
> > > > > > > +	if (dig_port && !dig_port->connected(encoder) &&
> > > > > >
> > > > > > This check is racy, as right after dig_port->connected() returns true,
> > > > > > the port can become disconnected.
> > > > >
> > > > > [Kasireddy, Vivek] Given that, do you think the only way to reliably determine
> > > > > if the Type-C port has a sink is to check the live status and ignore dig_port-
> > >tc_mode?
> > > > >
> > > > > If that is the case, should I just add a function pointer to dig_port to call
> > > > > tc_port_live_status_mask()? Or, should I just change intel_tc_port_connected()
> > > > > to ignore dig_port->tc_mode like below:
> > > > > @@ -764,8 +764,7 @@ bool intel_tc_port_connected(struct intel_encoder
> > *encoder)
> > > > >
> > > > >         intel_tc_port_lock(dig_port);
> > > > >
> > > > > -       is_connected = tc_port_live_status_mask(dig_port) &
> > > > > -                      BIT(dig_port->tc_mode);
> > > > > +       is_connected = tc_port_live_status_mask(dig_port);
> > > > >
> > > > > Or, are there any other elegant ways that you can think of to determine whether
> > > > > a tc port has a sink or not?
> > > >
> > > > I meant that I don't think there is a way to prevent a modeset on a
> > > > disconnected port.
> > >
> > > But we need to find a way right given that the spec clearly states that the driver
> > > must not use or access (PHY/FIA registers of) a disconnected tc port.
> > 
> > The driver does not access the PHY/FIA regs on a disconnected port/PHY.
>
> [Kasireddy, Vivek] I think it does after the first patch in this series is applied if
> the userspace (Weston) forces a modeset on a disconnected tc legacy port (HDMI).
> For instance, some of the FIA/PHY regs accesses I noticed include programming
> the lane count (intel_tc_port_set_fia_lane_count() called by intel_ddi_pre_pll_enable()),
> reading the pin assignment mask (intel_tc_port_get_pin_assignment_mask() called
> by icl_program_mg_dp_mode() which is called by intel_ddi_pre_enable_hdmi()), etc.

The FW/HW will setup a legacy TC port's PHY once during system boot and
resume, so I don't see any problem modesetting those later, regardless
of a sink being plugged on them or not. We need the first patch which
fixes a bug selecting the wrong PLL.

> Of-course, these accesses would probably not occur if the disconnected tc port
> defaults to TBT mode but this brings other problems like I described in the
> commit description of the first patch and the cover letter.
>  
> > > > Live status is what provides the connected state, but
> > > > it can change right after it is read out.
> > >
> > > Does this change happen after giving up the ownership (in
> > > icl_tc_phy_disconnect)?
> > 
> > The HPD live status changes whenever a user plugs/unplugs a sink.
> > 
> > > But shouldn't we distinguish between the cases where we are
> > > deliberately disconnecting the phy for power-savings reason vs when
> > > the port actually becomes disconnected? The port can still be
> > > considered connected in the former case right?
> > 
> > The driver - based on the spec - needs to avoid accessing the PHY/FIA
> > regs whenever the PHY is disconnected either by FW/HW (because the user
> > unplugged the sink) or the driver (during the suspend, modeset disable
> > sequence).
>
> [Kasireddy, Vivek] Regardless of whether the PHY/FIA regs are accessed or
> not, I don't think the driver should let the userspace's modeset to succeed on
> a disconnected tc port. Do you not agree?

I don't think a modeset can or should be prevented if the user unplugs a
monitor midway.

> > > Under what other situations would the live status change or become
> > > unreliable after the port has a connected sink?
> > 
> > It's not unreliable, it reflects the state of a sink being plugged to
> > the connector or not.
>
> [Kasireddy, Vivek] Ok, assuming that the state of the sink is "connected"
> during intel_atomic_check() phase (which is where this patch checks for
> connected status), are you concerned about the case where the user may
> unplug the sink before we get to the intel_atomic_commit() phase? Is
> this what you meant when you said this earlier: "This check is racy, as
> right after dig_port->connected() returns true, the port can become
> disconnected"? I am just trying to figure out the scenarios when this
> might happen.

Yes, checking the HPD live state and attempting to prevent a modeset
based on it doesn't work as this state can change at any moment. I don't
see a reason either why this should be done.

> > > And, since we rely on SDEISR to detect the live status for tc legacy
> > > ports, could this not be considered reliable?
> > 
> > Changes in the HPD live status is used as a hint to user space to
> > follow up with connector detection and modeset enable/disable requests
> > as necessary.
>
> [Kasireddy, Vivek] Right, that is the ideal case but user/userspace can commit
> mistakes where for example they can assume that HDMI-A-1 is connected 
> (while it is not) instead of HDMI-A-3 which is the one actually connected.
> During such cases, I think the driver should not let the userspace hang the
> system or lead to unexpected states and instead should return an error.

I can't see a problem modesetting a TC connector, when there is no sink
connected to it or the sink gets unplugged at an arbitrary time during
the modeset.

--Imre

Re: [Intel-gfx] [PATCH v1 2/2] drm/i915: Reject the atomic modeset if an associated Type-C port is disconnected

[Kasireddy, Vivek]

Hi Imre,

> On Tue, May 24, 2022 at 11:29:54AM +0300, Kasireddy, Vivek wrote:
> > Hi Imre,
> >
> > > On Fri, May 20, 2022 at 10:28:31AM +0300, Kasireddy, Vivek wrote:
> > > > Hi Imre,
> > > > [...]
> > > > > > > > @@ -131,6 +137,20 @@ int intel_digital_connector_atomic_check(struct
> > > drm_connector *conn,
> > > > > > > >
> > > > > > > >  	crtc_state = drm_atomic_get_new_crtc_state(state, new_state-
> >crtc);
> > > > > > > >
> > > > > > > > +	/*
> > > > > > > > +	 * The spec says that it is not safe to use a disconnected Type-C port.
> > > > > > > > +	 * Therefore, check to see if this connector is connected and reject
> > > > > > > > +	 * the modeset if there is no sink detected.
> > > > > > > > +	 */
> > > > > > > > +	if (dig_port && !dig_port->connected(encoder) &&
> > > > > > >
> > > > > > > This check is racy, as right after dig_port->connected() returns true,
> > > > > > > the port can become disconnected.
> > > > > >
> > > > > > [Kasireddy, Vivek] Given that, do you think the only way to reliably determine
> > > > > > if the Type-C port has a sink is to check the live status and ignore dig_port-
> > > >tc_mode?
> > > > > >
> > > > > > If that is the case, should I just add a function pointer to dig_port to call
> > > > > > tc_port_live_status_mask()? Or, should I just change intel_tc_port_connected()
> > > > > > to ignore dig_port->tc_mode like below:
> > > > > > @@ -764,8 +764,7 @@ bool intel_tc_port_connected(struct intel_encoder
> > > *encoder)
> > > > > >
> > > > > >         intel_tc_port_lock(dig_port);
> > > > > >
> > > > > > -       is_connected = tc_port_live_status_mask(dig_port) &
> > > > > > -                      BIT(dig_port->tc_mode);
> > > > > > +       is_connected = tc_port_live_status_mask(dig_port);
> > > > > >
> > > > > > Or, are there any other elegant ways that you can think of to determine whether
> > > > > > a tc port has a sink or not?
> > > > >
> > > > > I meant that I don't think there is a way to prevent a modeset on a
> > > > > disconnected port.
> > > >
> > > > But we need to find a way right given that the spec clearly states that the driver
> > > > must not use or access (PHY/FIA registers of) a disconnected tc port.
> > >
> > > The driver does not access the PHY/FIA regs on a disconnected port/PHY.
> >
> > [Kasireddy, Vivek] I think it does after the first patch in this series is applied if
> > the userspace (Weston) forces a modeset on a disconnected tc legacy port (HDMI).
> > For instance, some of the FIA/PHY regs accesses I noticed include programming
> > the lane count (intel_tc_port_set_fia_lane_count() called by intel_ddi_pre_pll_enable()),
> > reading the pin assignment mask (intel_tc_port_get_pin_assignment_mask() called
> > by icl_program_mg_dp_mode() which is called by intel_ddi_pre_enable_hdmi()), etc.
> 
> The FW/HW will setup a legacy TC port's PHY once during system boot and
> resume, so I don't see any problem modesetting those later, regardless
> of a sink being plugged on them or not. We need the first patch which
> fixes a bug selecting the wrong PLL.
> 
> > Of-course, these accesses would probably not occur if the disconnected tc port
> > defaults to TBT mode but this brings other problems like I described in the
> > commit description of the first patch and the cover letter.
> >
> > > > > Live status is what provides the connected state, but
> > > > > it can change right after it is read out.
> > > >
> > > > Does this change happen after giving up the ownership (in
> > > > icl_tc_phy_disconnect)?
> > >
> > > The HPD live status changes whenever a user plugs/unplugs a sink.
> > >
> > > > But shouldn't we distinguish between the cases where we are
> > > > deliberately disconnecting the phy for power-savings reason vs when
> > > > the port actually becomes disconnected? The port can still be
> > > > considered connected in the former case right?
> > >
> > > The driver - based on the spec - needs to avoid accessing the PHY/FIA
> > > regs whenever the PHY is disconnected either by FW/HW (because the user
> > > unplugged the sink) or the driver (during the suspend, modeset disable
> > > sequence).
> >
> > [Kasireddy, Vivek] Regardless of whether the PHY/FIA regs are accessed or
> > not, I don't think the driver should let the userspace's modeset to succeed on
> > a disconnected tc port. Do you not agree?
> 
> I don't think a modeset can or should be prevented if the user unplugs a
> monitor midway.
> 
> > > > Under what other situations would the live status change or become
> > > > unreliable after the port has a connected sink?
> > >
> > > It's not unreliable, it reflects the state of a sink being plugged to
> > > the connector or not.
> >
> > [Kasireddy, Vivek] Ok, assuming that the state of the sink is "connected"
> > during intel_atomic_check() phase (which is where this patch checks for
> > connected status), are you concerned about the case where the user may
> > unplug the sink before we get to the intel_atomic_commit() phase? Is
> > this what you meant when you said this earlier: "This check is racy, as
> > right after dig_port->connected() returns true, the port can become
> > disconnected"? I am just trying to figure out the scenarios when this
> > might happen.
> 
> Yes, checking the HPD live state and attempting to prevent a modeset
> based on it doesn't work as this state can change at any moment. I don't
> see a reason either why this should be done.
> 
> > > > And, since we rely on SDEISR to detect the live status for tc legacy
> > > > ports, could this not be considered reliable?
> > >
> > > Changes in the HPD live status is used as a hint to user space to
> > > follow up with connector detection and modeset enable/disable requests
> > > as necessary.
> >
> > [Kasireddy, Vivek] Right, that is the ideal case but user/userspace can commit
> > mistakes where for example they can assume that HDMI-A-1 is connected
> > (while it is not) instead of HDMI-A-3 which is the one actually connected.
> > During such cases, I think the driver should not let the userspace hang the
> > system or lead to unexpected states and instead should return an error.
> 
> I can't see a problem modesetting a TC connector, when there is no sink
> connected to it or the sink gets unplugged at an arbitrary time during
> the modeset.
[Kasireddy, Vivek] I think modesetting a TC connector with no sink connected
would be equivalent to "using" it which is against what the spec says: "Display
software must not use a disconnected port".

Anyway, let me send a v2 of the first patch to include your suggestion.

Thanks,
Vivek

> 
> --Imre