[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 at linux.intel.com>
> > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy at 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
> > > >