Re: [PATCH v3] drm/i915/icl: implement the tc/legacy HPD {dis, }connect flows

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Sex, 2018-08-17 às 09:25 -0700, Srivatsa, Anusha escreveu:
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
> > Behalf Of
> > Paulo Zanoni
> > Sent: Wednesday, August 1, 2018 10:35 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com>; Vivi, Rodrigo
> > <rodrigo.vivi@intel.com>
> > Subject: [Intel-gfx] [PATCH v3] drm/i915/icl: implement the
> > tc/legacy HPD {dis,
> > }connect flows
> > 
> > Unlike the other ports, TC ports are not available to use as soon
> > as we get a
> > hotplug. The TC PHYs can be shared between multiple
> > controllers: display, USB, etc. As a result, handshaking through
> > FIA is required
> > around connect and disconnect to cleanly transfer ownership with
> > the controller
> > and set the type-C power state.
> > 
> > This patch implements the flow sequences described by our
> > specification. We opt
> > to grab ownership of the ports as soon as we get the hotplugs in
> > order to simplify
> > the interactions and avoid surprises in the user space side. We may
> > consider
> > changing this in the future, once we improve our testing
> > capabilities on this area.
> > 
> > v2:
> > * This unifies the DP and HDMI patches so we can discuss everything
> >   at once so people looking at random single patches can actually
> >   understand the direction.
> > * I found out the spec was updated a while ago. There's a small
> >   difference in the connect flow and the patch was updated for
> > that.
> > * Our spec also now gives a good explanation on what is really
> >   happening. As a result, comments were added.
> > * Add some more comments as requested by Rodrigo (Rodrigo).
> > v3:
> > * Downgrade a DRM_ERROR that shouldn't ever happen but we can't act
> >   on in case it does (Chris).
> > 
> > BSpec: 21750, 4250.
> > Cc: Animesh Manna <animesh.manna@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h   |   6 +++
> > drivers/gpu/drm/i915/intel_dp.c   | 110
> > +++++++++++++++++++++++++++++++++++++-
> > drivers/gpu/drm/i915/intel_hdmi.c |  11 ++--
> > 3 files changed, 123 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 1da1c7743785..a5f4dfe9ebdf 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -10750,4 +10750,10 @@ enum skl_power_gate {
> > #define   DP_LANE_ASSIGNMENT_MASK(tc_port)	(0xf <<
> > ((tc_port) * 8))
> > #define   DP_LANE_ASSIGNMENT(tc_port, x)	((x) << ((tc_port)
> > * 8))
> > 
> > +#define PORT_TX_DFLEXDPPMS
> > 	_MMIO(0x163890)
> > +#define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)		(1
> > <<
> > (tc_port))
> > +
> > +#define PORT_TX_DFLEXDPCSSS
> > 	_MMIO(0x163894)
> > +#define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1
> > << (tc_port))
> > +
> > #endif /* _I915_REG_H_ */
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index fec21aa3db93..86eded613456 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4818,6 +4818,104 @@ static void icl_update_tc_port_type(struct
> > drm_i915_private *dev_priv,
> > 			      type_str);
> > }
> > 
> > +/*
> > + * This function implements the first part of the Connect Flow
> > +described by our
> > + * specification, Gen11 TypeC Programming chapter. The rest of the
> > flow
> > +(reading
> > + * lanes, EDID, etc) is done as needed in the typical places.
> > + *
> > + * Unlike the other ports, type-C ports are not available to use
> > as
> > +soon as we
> > + * get a hotplug. The type-C PHYs can be shared between multiple
> > controllers:
> > + * display, USB, etc. As a result, handshaking through FIA is
> > required
> > +around
> > + * connect and disconnect to cleanly transfer ownership with the
> > +controller and
> > + * set the type-C power state.
> > + *
> > + * We could opt to only do the connect flow when we actually try
> > to use
> > +the AUX
> > + * channels or do a modeset, then immediately run the disconnect
> > flow
> > +after
> > + * usage, but there are some implications on this for a dynamic
> > environment:
> > + * things may go away or change behind our backs. So for now our
> > driver
> > +is
> > + * always trying to acquire ownership of the controller as soon as
> > it
> > +gets an
> > + * interrupt (or polls state and sees a port is connected) and
> > only
> > +gives it
> > + * back when it sees a disconnect. Implementation of a more
> > +fine-grained model
> > + * will require a lot of coordination with user space and thorough
> > +testing for
> > + * the extra possible cases.
> > + */
> > +static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
> > +			       struct intel_digital_port
> > *dig_port) {
> > +	enum tc_port tc_port = intel_port_to_tc(dev_priv,
> > dig_port->base.port);
> > +	u32 val;
> > +
> > +	if (dig_port->tc_type != TC_PORT_LEGACY &&
> > +	    dig_port->tc_type != TC_PORT_TYPEC)
> > +		return true;
> > +
> > +	val = I915_READ(PORT_TX_DFLEXDPPMS);
> > +	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
> > +		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n",
> > tc_port);
> > +		return false;
> > +	}
> > +
> > +	/*
> > +	 * This function may be called many times in a row without
> > an HPD event
> > +	 * in between, so try to avoid the write when we can.
> > +	 */
> > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > +	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
> > +		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > +	}
> > +
> > +	/*
> > +	 * Now we have to re-check the live state, in case the
> > port recently
> > +	 * became disconnected. Not necessary for legacy mode.
> > +	 */
> > +	if (dig_port->tc_type == TC_PORT_TYPEC &&
> > +	    !(I915_READ(PORT_TX_DFLEXDPSP) &
> > TC_LIVE_STATE_TC(tc_port))) {
> > +		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n",
> > tc_port);
> > +		val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > +		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * See the comment at the connect function. This implements the
> > +Disconnect
> > + * Flow.
> > + */
> > +static void icl_tc_phy_disconnect(struct drm_i915_private
> > *dev_priv,
> > +				  struct intel_digital_port
> > *dig_port) {
> > +	enum tc_port tc_port = intel_port_to_tc(dev_priv,
> > dig_port->base.port);
> > +	u32 val;
> > +
> > +	if (dig_port->tc_type != TC_PORT_LEGACY &&
> > +	    dig_port->tc_type != TC_PORT_TYPEC)
> 
> 	Don’t we also need to check for dig_port->tc_type !=
> TC_PORT_TBT?

We don't need to run these steps for TBT, the TBT sequence is simpler.
See the spec.


> > +		return;
> > +
> > +	/*
> > +	 * This function may be called many times in a row without
> > an HPD event
> > +	 * in between, so try to avoid the write when we can.
> > +	 */
> > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > +	if (val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)) {
> > +		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > +	}
> > +}
> > +
> > +/*
> > + * The type-C ports are different because even when they are
> > connected,
> > +they may
> > + * not be available/usable by the graphics driver: see the comment
> > on
> > + * icl_tc_phy_connect(). So in our driver instead of adding the
> > +additional
> > + * concept of "usable" and make everything check for "connected
> > and
> > +usable" we
> > + * define a port as "connected" when it is not only connected, but
> > also
> > +when it
> > + * is usable by the rest of the driver. That maintains the old
> > +assumption that
> > + * connected ports are usable, and avoids exposing to the users
> > objects
> > +they
> > + * can't really use.
> > + */
> > static bool icl_tc_port_connected(struct drm_i915_private
> > *dev_priv,
> > 				  struct intel_digital_port
> > *intel_dig_port)  { @@
> > -4836,12 +4934,17 @@ static bool icl_tc_port_connected(struct
> > drm_i915_private *dev_priv,
> > 	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
> > 	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
> > 
> > -	if (!is_legacy && !is_typec && !is_tbt)
> > +	if (!is_legacy && !is_typec && !is_tbt) {
> 
> If it is none of the above, then why not just return false?

Because we need to run the disconnect sequence in case the port was
previously connected so we release ownership of the lanes. That's the
very goal of this patch.

Thanks,
Paulo


> 	
> Anusha 
> > +		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
> > 		return false;
> > +	}
> > 
> > 	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy,
> > is_typec,
> > 				is_tbt);
> > 
> > +	if (!icl_tc_phy_connect(dev_priv, intel_dig_port))
> > +		return false;
> > +
> > 	return true;
> > }
> > 
> > @@ -4869,6 +4972,11 @@ static bool
> > icl_digital_port_connected(struct
> > intel_encoder *encoder)
> >  * intel_digital_port_connected - is the specified port connected?
> >  * @encoder: intel_encoder
> >  *
> > + * In cases where there's a connector physically connected but it
> > can't
> > + be used
> > + * by our hardware we also return false, since the rest of the
> > driver
> > + should
> > + * pretty much treat the port as disconnected. This is relevant
> > for
> > + type-C
> > + * (starting on ICL) where there's ownership involved.
> > + *
> >  * Return %true if port is connected, %false otherwise.
> >  */
> > bool intel_digital_port_connected(struct intel_encoder *encoder)
> > diff --git
> > a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 8363fbd18ee8..548199e59b6c 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1905,21 +1905,26 @@ intel_hdmi_set_edid(struct drm_connector
> > *connector)  static enum
> > drm_connector_status  intel_hdmi_detect(struct
> > drm_connector *connector, bool force)  {
> > -	enum drm_connector_status status;
> > +	enum drm_connector_status status =
> > connector_status_disconnected;
> > 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > +	struct intel_hdmi *intel_hdmi =
> > intel_attached_hdmi(connector);
> > +	struct intel_encoder *encoder =
> > &hdmi_to_dig_port(intel_hdmi)->base;
> > 
> > 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > 		      connector->base.id, connector->name);
> > 
> > 	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> > 
> > +	if (IS_ICELAKE(dev_priv) &&
> > +	    !intel_digital_port_connected(encoder))
> > +		goto out;
> > +
> > 	intel_hdmi_unset_edid(connector);
> > 
> > 	if (intel_hdmi_set_edid(connector))
> > 		status = connector_status_connected;
> > -	else
> > -		status = connector_status_disconnected;
> > 
> > +out:
> > 	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> > 
> > 	return status;
> > --
> > 2.14.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx