On Tue, Jun 25, 2019 at 09:30:43PM +0300, Imre Deak wrote: > On Tue, Jun 25, 2019 at 04:42:01PM +0300, Ville Syrjälä wrote: > > On Thu, Jun 20, 2019 at 05:05:49PM +0300, Imre Deak wrote: > > > Make the order during detection more consistent: first reset the TypeC > > > port mode if needed (adding new helpers for this), then detect any > > > connected sink. > > > > > > To check if a port mode reset is needed determine first the target port > > > mode based on the live status if a sink is already connected or the > > > PHY status complete flag otherwise. > > > > > > Add a WARN in legacy mode if unexpectedly we can't set the unsafe mode > > > or if the FIA doesn't provide the 4 lanes required. > > > > > > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx> > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/display/intel_tc.c | 96 ++++++++++++------------- > > > 1 file changed, 47 insertions(+), 49 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c > > > index fffe4c4a6602..ed2253b21b09 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_tc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c > > > @@ -172,41 +172,43 @@ static bool icl_tc_phy_set_safe_mode(struct intel_digital_port *dig_port, > > > * will require a lot of coordination with user space and thorough testing for > > > * the extra possible cases. > > > */ > > > -static bool icl_tc_phy_connect(struct intel_digital_port *dig_port) > > > +static void icl_tc_phy_connect(struct intel_digital_port *dig_port) > > > { > > > - u32 live_status_mask; > > > - > > > - if (dig_port->tc_mode != TC_PORT_LEGACY && > > > - dig_port->tc_mode != TC_PORT_DP_ALT) > > > - return true; > > > - > > > if (!icl_tc_phy_status_complete(dig_port)) { > > > DRM_DEBUG_KMS("Port %s: PHY not ready\n", > > > dig_port->tc_port_name); > > > - WARN_ON(dig_port->tc_legacy_port); > > > > Are we expecting legacy ports to indicate "not complete"? > > Yes, I think that may happen if during driver loading/resume the sink is > not connected, VBT says the port is legacy, but the firmware would > happen to set the 'phy complete' flag only with some delay. In that case > we'd try to connect the PHY from intel_tc_port_sanitize(), later in the > patchset, so 'phy complete' could be still unset here. We'd only connect > the PHY then when the sink gets plugged. OK. Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > - return false; > > > + goto out_set_tbt_alt_mode; > > > } > > > > > > - if (!icl_tc_phy_set_safe_mode(dig_port, false)) > > > - return false; > > > + if (!icl_tc_phy_set_safe_mode(dig_port, false) && > > > + !WARN_ON(dig_port->tc_legacy_port)) > > > + goto out_set_tbt_alt_mode; > > > > > > - if (dig_port->tc_mode == TC_PORT_LEGACY) > > > - return true; > > > + if (dig_port->tc_legacy_port) { > > > + WARN_ON(intel_tc_port_fia_max_lane_count(dig_port) != 4); > > > + dig_port->tc_mode = TC_PORT_LEGACY; > > > > > > - live_status_mask = tc_port_live_status_mask(dig_port); > > > + return; > > > + } > > > > > > /* > > > * Now we have to re-check the live state, in case the port recently > > > * became disconnected. Not necessary for legacy mode. > > > */ > > > - if (!(live_status_mask & BIT(TC_PORT_DP_ALT))) { > > > + if (!(tc_port_live_status_mask(dig_port) & BIT(TC_PORT_DP_ALT))) { > > > DRM_DEBUG_KMS("Port %s: PHY sudden disconnect\n", > > > dig_port->tc_port_name); > > > - icl_tc_phy_disconnect(dig_port); > > > - return false; > > > + goto out_set_safe_mode; > > > } > > > > > > - return true; > > > + dig_port->tc_mode = TC_PORT_DP_ALT; > > > + > > > + return; > > > + > > > +out_set_safe_mode: > > > + icl_tc_phy_set_safe_mode(dig_port, true); > > > +out_set_tbt_alt_mode: > > > + dig_port->tc_mode = TC_PORT_TBT_ALT; > > > } > > > > > > /* > > > @@ -227,27 +229,37 @@ void icl_tc_phy_disconnect(struct intel_digital_port *dig_port) > > > default: > > > MISSING_CASE(dig_port->tc_mode); > > > } > > > +} > > > > > > - DRM_DEBUG_KMS("Port %s: mode %s disconnected\n", > > > - dig_port->tc_port_name, > > > - tc_port_mode_name(dig_port->tc_mode)); > > > +static enum tc_port_mode > > > +intel_tc_port_get_target_mode(struct intel_digital_port *dig_port) > > > +{ > > > + u32 live_status_mask = tc_port_live_status_mask(dig_port); > > > + > > > + if (live_status_mask) > > > + return fls(live_status_mask) - 1; > > > + > > > + return icl_tc_phy_status_complete(dig_port) && > > > + dig_port->tc_legacy_port ? TC_PORT_LEGACY : > > > + TC_PORT_TBT_ALT; > > > } > > > > > > -static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, > > > - struct intel_digital_port *intel_dig_port, > > > - u32 live_status_mask) > > > +static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port) > > > { > > > - enum tc_port_mode old_mode = intel_dig_port->tc_mode; > > > + enum tc_port_mode old_tc_mode = dig_port->tc_mode; > > > > > > - if (!live_status_mask) > > > - return; > > > + icl_tc_phy_disconnect(dig_port); > > > + icl_tc_phy_connect(dig_port); > > > > > > - intel_dig_port->tc_mode = fls(live_status_mask) - 1; > > > + DRM_DEBUG_KMS("Port %s: TC port mode reset (%s -> %s)\n", > > > + dig_port->tc_port_name, > > > + tc_port_mode_name(old_tc_mode), > > > + tc_port_mode_name(dig_port->tc_mode)); > > > +} > > > > > > - if (old_mode != intel_dig_port->tc_mode) > > > - DRM_DEBUG_KMS("Port %s: port has mode %s\n", > > > - intel_dig_port->tc_port_name, > > > - tc_port_mode_name(intel_dig_port->tc_mode)); > > > +static bool intel_tc_port_needs_reset(struct intel_digital_port *dig_port) > > > +{ > > > + return intel_tc_port_get_target_mode(dig_port) != dig_port->tc_mode; > > > } > > > > > > /* > > > @@ -262,24 +274,10 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, > > > */ > > > bool intel_tc_port_connected(struct intel_digital_port *dig_port) > > > { > > > - struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > > - u32 live_status_mask = tc_port_live_status_mask(dig_port); > > > - > > > - /* > > > - * The spec says we shouldn't be using the ISR bits for detecting > > > - * between TC and TBT. We should use DFLEXDPSP. > > > - */ > > > - if (!live_status_mask && !dig_port->tc_legacy_port) { > > > - icl_tc_phy_disconnect(dig_port); > > > - > > > - return false; > > > - } > > > - > > > - icl_update_tc_port_type(dev_priv, dig_port, live_status_mask); > > > - if (!icl_tc_phy_connect(dig_port)) > > > - return false; > > > + if (intel_tc_port_needs_reset(dig_port)) > > > + intel_tc_port_reset_mode(dig_port); > > > > > > - return true; > > > + return tc_port_live_status_mask(dig_port) & BIT(dig_port->tc_mode); > > > } > > > > > > void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy) > > > -- > > > 2.17.1 > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx