| Message ID | 20190620140600.11357-13-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix TypeC port mode switching | expand |
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@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > 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"? > - 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
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@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > 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. > > > - 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
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@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > 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@linux.intel.com> > > > > > > - 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
On Thu, 2019-06-20 at 17:05 +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@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > 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); > - 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; Marking as not safe even for TBT? > > - 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; You are right about the order that C will execute this but could you add a pair of parenthesis around icl_tc_phy_status_complete(dig_port) && dig_port->tc_legacy_port to make easier to read? > } > > -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)
On Thu, Jun 27, 2019 at 02:55:21AM +0300, Souza, Jose wrote: > On Thu, 2019-06-20 at 17:05 +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@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > 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); > > - 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; > > Marking as not safe even for TBT? You mean if the above will set not-safe mode even for TBT? No, we will try to set not-safe mode only for DP-alt and legacy as the PHY status complete flag was set. > > > > > - 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; > > You are right about the order that C will execute this but could you > add a pair of parenthesis around icl_tc_phy_status_complete(dig_port) > && dig_port->tc_legacy_port to make easier to read? Not sure, I think the use of 'a && b ? c : d' without parens is usual enough, but maybe others have a better idea for a rule-of-thumb for using parens that we should follow everywhere. Jani? Anyway the following would be easier to read (also we can avoid the reg read for the !legacy case): if (dig_port->tc_legacy_port && icl_tc_phy_status_complete(dig_port)) return TC_PORT_LEGACY; return 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)
On Thu, 2019-06-27 at 12:48 +0300, Imre Deak wrote: > On Thu, Jun 27, 2019 at 02:55:21AM +0300, Souza, Jose wrote: > > On Thu, 2019-06-20 at 17:05 +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@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > 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); > > > - 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; > > > > Marking as not safe even for TBT? > > You mean if the above will set not-safe mode even for TBT? No, we > will try to set not-safe mode only for DP-alt and legacy as the PHY > status complete flag was set. Oh okay, as you explained the complete flag will not be set for TBT. Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > > - 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; > > > > You are right about the order that C will execute this but could > > you > > add a pair of parenthesis around > > icl_tc_phy_status_complete(dig_port) > > && dig_port->tc_legacy_port to make easier to read? > > Not sure, I think the use of 'a && b ? c : d' without parens is usual > enough, but maybe others have a better idea for a rule-of-thumb for > using parens that we should follow everywhere. Jani? > > Anyway the following would be easier to read (also we can avoid the > reg > read for the !legacy case): > > if (dig_port->tc_legacy_port && > icl_tc_phy_status_complete(dig_port)) > return TC_PORT_LEGACY; > > return 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)
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); - 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)
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@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_tc.c | 96 ++++++++++++------------- 1 file changed, 47 insertions(+), 49 deletions(-)