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

["Kasireddy, Vivek" <vivek.kasireddy@intel.com>]

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
> >