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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> > > ---
> > > 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
> > >
