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

[Imre Deak <imre.deak@intel.com>]

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