| Message ID | 20220516085402.3591249-3-vivek.kasireddy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/tc: Prevent system hang when modesetting disconnected Type-C ports | expand |
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.
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 >
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 > >
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 > > >
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 > > > >
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
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
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
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
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.
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(+)