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