| Message ID | 20190604145826.16424-10-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix TypeC port mode switching | expand |
On Tue, 04 Jun 2019, Imre Deak <imre.deak@intel.com> wrote: > Factor out helpers reading/parsing the TypeC specific registers, making > current users of them clearer and letting us use them later. > > While at it also: > - Simplify icl_tc_phy_connect() with an early return in legacy mode. > - Simplify the live status check using one bitmask for all HPD bits. > - Remove a micro-optimisation of the repeated safe-mode clearing. > - Make sure we fix the legacy port flag in all cases. > > Except for the last two, no functional changes. > > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 5 +- > drivers/gpu/drm/i915/intel_tc.c | 166 +++++++++++++++++++------------ > drivers/gpu/drm/i915/intel_tc.h | 1 + > 3 files changed, 103 insertions(+), 69 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 8f223d48d562..d236839bee19 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2983,7 +2983,6 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > { > struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev); > enum port port = intel_dig_port->base.port; > - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > u32 ln0, ln1, lane_info; > > if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > @@ -2997,9 +2996,7 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > ln0 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE); > ln1 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE); > > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > + lane_info = intel_tc_port_get_lane_info(intel_dig_port); > > switch (lane_info) { > case 0x1: > diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c > index 07488235b67a..3fdcfa2bbaee 100644 > --- a/drivers/gpu/drm/i915/intel_tc.c > +++ b/drivers/gpu/drm/i915/intel_tc.c > @@ -43,10 +43,19 @@ static const char *tc_port_mode_name(enum tc_port_mode mode) > return names[mode]; > } > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port) > { > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > + u32 lane_info = I915_READ(PORT_TX_DFLEXDPSP); Please don't do register reads in declarations. Ditto below. > + > + return (lane_info & DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > + DP_LANE_ASSIGNMENT_SHIFT(tc_port); > +} > + > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > intel_wakeref_t wakeref; > u32 lane_info; > > @@ -55,9 +64,7 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > lane_info = 0; > with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref) > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > + lane_info = intel_tc_port_get_lane_info(dig_port); > > switch (lane_info) { > default: > @@ -75,6 +82,69 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > } > } > > +static void tc_port_fixup_legacy_flag(struct intel_digital_port *dig_port, > + u32 live_status_mask) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > + u32 valid_hpd_mask = dig_port->tc_legacy_port ? BIT(TC_PORT_LEGACY) : > + ~BIT(TC_PORT_LEGACY); > + > + if (!(live_status_mask & ~valid_hpd_mask)) > + return; > + > + /* If live status mismatches the VBT flag, trust the live status. */ > + DRM_ERROR("Port %s: live status %08x mismatches the legacy port flag, fix flag\n", > + tc_port_name(dev_priv, tc_port), live_status_mask); > + > + dig_port->tc_legacy_port = !dig_port->tc_legacy_port; > +} > + > +static u32 tc_port_live_status_mask(struct intel_digital_port *dig_port) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > + u32 val = I915_READ(PORT_TX_DFLEXDPSP); > + u32 mask = 0; > + > + if (val & TC_LIVE_STATE_TBT(tc_port)) > + mask |= BIT(TC_PORT_TBT_ALT); > + if (val & TC_LIVE_STATE_TC(tc_port)) > + mask |= BIT(TC_PORT_DP_ALT); > + > + if (I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) > + mask |= BIT(TC_PORT_LEGACY); > + > + /* The sink can be connected only in a single mode. */ > + if (!WARN_ON(hweight32(mask) > 1)) > + tc_port_fixup_legacy_flag(dig_port, mask); > + > + return mask; > +} > + > +static bool icl_tc_phy_status_complete(struct intel_digital_port *dig_port) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > + > + return I915_READ(PORT_TX_DFLEXDPPMS) & > + DP_PHY_MODE_STATUS_COMPLETED(tc_port); > +} > + > +static void icl_tc_phy_set_safe_mode(struct intel_digital_port *dig_port, > + bool enable) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > + u32 val = I915_READ(PORT_TX_DFLEXDPCSSS); > + > + val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > + if (!enable) > + val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > + > + I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > +} > + > /* > * This function implements the first part of the Connect Flow described by our > * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading > @@ -100,36 +170,31 @@ static bool icl_tc_phy_connect(struct intel_digital_port *dig_port) > { > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > - u32 val; > + u32 live_status_mask; > > if (dig_port->tc_mode != TC_PORT_LEGACY && > dig_port->tc_mode != TC_PORT_DP_ALT) > return true; > > - val = I915_READ(PORT_TX_DFLEXDPPMS); > - if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) { > + if (!icl_tc_phy_status_complete(dig_port)) { > DRM_DEBUG_KMS("Port %s: PHY not ready\n", > tc_port_name(dev_priv, tc_port)); > WARN_ON(dig_port->tc_legacy_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); > - } > + icl_tc_phy_set_safe_mode(dig_port, false); > + > + if (dig_port->tc_mode == TC_PORT_LEGACY) > + return true; > + > + live_status_mask = tc_port_live_status_mask(dig_port); > > /* > * 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_mode == TC_PORT_DP_ALT && > - !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) { > + if (!(live_status_mask & BIT(TC_PORT_DP_ALT))) { > DRM_DEBUG_KMS("Port %s: PHY sudden disconnect\n", > tc_port_name(dev_priv, tc_port)); > icl_tc_phy_disconnect(dig_port); > @@ -148,44 +213,36 @@ void icl_tc_phy_disconnect(struct intel_digital_port *dig_port) > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > - /* > - * TBT disconnection flow is read the live status, what was done in > - * caller. > - */ > - if (dig_port->tc_mode == TC_PORT_DP_ALT || > - dig_port->tc_mode == TC_PORT_LEGACY) { > - u32 val; > - > - val = I915_READ(PORT_TX_DFLEXDPCSSS); > - val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > - I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > + switch (dig_port->tc_mode) { > + case TC_PORT_LEGACY: > + case TC_PORT_DP_ALT: > + icl_tc_phy_set_safe_mode(dig_port, true); > + dig_port->tc_mode = TC_PORT_TBT_ALT; > + break; > + case TC_PORT_TBT_ALT: > + /* Nothing to do, we stay in TBT-alt mode */ > + break; > + default: > + MISSING_CASE(dig_port->tc_mode); > } > > DRM_DEBUG_KMS("Port %s: mode %s disconnected\n", > tc_port_name(dev_priv, tc_port), > tc_port_mode_name(dig_port->tc_mode)); > - > - dig_port->tc_mode = TC_PORT_TBT_ALT; > } > > static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, > struct intel_digital_port *intel_dig_port, > - bool is_legacy, bool is_typec, bool is_tbt) > + u32 live_status_mask) > { > enum port port = intel_dig_port->base.port; > enum tc_port_mode old_mode = intel_dig_port->tc_mode; > > - WARN_ON(is_legacy + is_typec + is_tbt != 1); > - > - if (is_legacy) > - intel_dig_port->tc_mode = TC_PORT_LEGACY; > - else if (is_typec) > - intel_dig_port->tc_mode = TC_PORT_DP_ALT; > - else if (is_tbt) > - intel_dig_port->tc_mode = TC_PORT_TBT_ALT; > - else > + if (!live_status_mask) > return; > > + intel_dig_port->tc_mode = fls(live_status_mask) - 1; > + > if (old_mode != intel_dig_port->tc_mode) > DRM_DEBUG_KMS("Port %s: port has mode %s\n", > tc_port_name(dev_priv, > @@ -207,40 +264,19 @@ 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); > - enum port port = dig_port->base.port; > - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > - bool is_legacy, is_typec, is_tbt; > - u32 dpsp; > - > - /* > - * Complain if we got a legacy port HPD, but VBT didn't mark the port as > - * legacy. Treat the port as legacy from now on. > - */ > - if (!dig_port->tc_legacy_port && > - I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) { > - DRM_ERROR("Port %s: VBT incorrectly claims port is not TypeC legacy\n", > - tc_port_name(dev_priv, tc_port)); > - dig_port->tc_legacy_port = true; > - } > - is_legacy = dig_port->tc_legacy_port; > + 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. > */ > - dpsp = I915_READ(PORT_TX_DFLEXDPSP); > - 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 (!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, is_legacy, is_typec, > - is_tbt); > - > + icl_update_tc_port_type(dev_priv, dig_port, live_status_mask); > if (!icl_tc_phy_connect(dig_port)) > return false; > > diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h > index 94c62ac4a162..e937f5326959 100644 > --- a/drivers/gpu/drm/i915/intel_tc.h > +++ b/drivers/gpu/drm/i915/intel_tc.h > @@ -8,6 +8,7 @@ struct intel_digital_port; > void icl_tc_phy_disconnect(struct intel_digital_port *dig_port); > > bool intel_tc_port_connected(struct intel_digital_port *dig_port); > +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port); > int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port); > > #endif /* __INTEL_TC_H__ */
On Thu, Jun 06, 2019 at 11:47:20AM +0300, Jani Nikula wrote: > On Tue, 04 Jun 2019, Imre Deak <imre.deak@intel.com> wrote: > > Factor out helpers reading/parsing the TypeC specific registers, making > > current users of them clearer and letting us use them later. > > > > While at it also: > > - Simplify icl_tc_phy_connect() with an early return in legacy mode. > > - Simplify the live status check using one bitmask for all HPD bits. > > - Remove a micro-optimisation of the repeated safe-mode clearing. > > - Make sure we fix the legacy port flag in all cases. > > > > Except for the last two, no functional changes. > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 5 +- > > drivers/gpu/drm/i915/intel_tc.c | 166 +++++++++++++++++++------------ > > drivers/gpu/drm/i915/intel_tc.h | 1 + > > 3 files changed, 103 insertions(+), 69 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 8f223d48d562..d236839bee19 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2983,7 +2983,6 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > > { > > struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev); > > enum port port = intel_dig_port->base.port; > > - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > > u32 ln0, ln1, lane_info; > > > > if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > > @@ -2997,9 +2996,7 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > > ln0 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE); > > ln1 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE); > > > > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > > + lane_info = intel_tc_port_get_lane_info(intel_dig_port); > > > > switch (lane_info) { > > case 0x1: > > diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c > > index 07488235b67a..3fdcfa2bbaee 100644 > > --- a/drivers/gpu/drm/i915/intel_tc.c > > +++ b/drivers/gpu/drm/i915/intel_tc.c > > @@ -43,10 +43,19 @@ static const char *tc_port_mode_name(enum tc_port_mode mode) > > return names[mode]; > > } > > > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port) > > { > > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > + u32 lane_info = I915_READ(PORT_TX_DFLEXDPSP); > > Please don't do register reads in declarations. Ditto below. Ok. > > > + > > + return (lane_info & DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > > + DP_LANE_ASSIGNMENT_SHIFT(tc_port); > > +} > > + > > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > intel_wakeref_t wakeref; > > u32 lane_info; > > > > @@ -55,9 +64,7 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > > > lane_info = 0; > > with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref) > > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > > + lane_info = intel_tc_port_get_lane_info(dig_port); > > > > switch (lane_info) { > > default: > > @@ -75,6 +82,69 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > } > > } > > > > +static void tc_port_fixup_legacy_flag(struct intel_digital_port *dig_port, > > + u32 live_status_mask) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > + u32 valid_hpd_mask = dig_port->tc_legacy_port ? BIT(TC_PORT_LEGACY) : > > + ~BIT(TC_PORT_LEGACY); > > + > > + if (!(live_status_mask & ~valid_hpd_mask)) > > + return; > > + > > + /* If live status mismatches the VBT flag, trust the live status. */ > > + DRM_ERROR("Port %s: live status %08x mismatches the legacy port flag, fix flag\n", > > + tc_port_name(dev_priv, tc_port), live_status_mask); > > + > > + dig_port->tc_legacy_port = !dig_port->tc_legacy_port; > > +} > > + > > +static u32 tc_port_live_status_mask(struct intel_digital_port *dig_port) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > + u32 val = I915_READ(PORT_TX_DFLEXDPSP); > > + u32 mask = 0; > > + > > + if (val & TC_LIVE_STATE_TBT(tc_port)) > > + mask |= BIT(TC_PORT_TBT_ALT); > > + if (val & TC_LIVE_STATE_TC(tc_port)) > > + mask |= BIT(TC_PORT_DP_ALT); > > + > > + if (I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) > > + mask |= BIT(TC_PORT_LEGACY); > > + > > + /* The sink can be connected only in a single mode. */ > > + if (!WARN_ON(hweight32(mask) > 1)) > > + tc_port_fixup_legacy_flag(dig_port, mask); > > + > > + return mask; > > +} > > + > > +static bool icl_tc_phy_status_complete(struct intel_digital_port *dig_port) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > + > > + return I915_READ(PORT_TX_DFLEXDPPMS) & > > + DP_PHY_MODE_STATUS_COMPLETED(tc_port); > > +} > > + > > +static void icl_tc_phy_set_safe_mode(struct intel_digital_port *dig_port, > > + bool enable) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > + u32 val = I915_READ(PORT_TX_DFLEXDPCSSS); > > + > > + val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > > + if (!enable) > > + val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > > + > > + I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > > +} > > + > > /* > > * This function implements the first part of the Connect Flow described by our > > * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading > > @@ -100,36 +170,31 @@ static bool icl_tc_phy_connect(struct intel_digital_port *dig_port) > > { > > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > - u32 val; > > + u32 live_status_mask; > > > > if (dig_port->tc_mode != TC_PORT_LEGACY && > > dig_port->tc_mode != TC_PORT_DP_ALT) > > return true; > > > > - val = I915_READ(PORT_TX_DFLEXDPPMS); > > - if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) { > > + if (!icl_tc_phy_status_complete(dig_port)) { > > DRM_DEBUG_KMS("Port %s: PHY not ready\n", > > tc_port_name(dev_priv, tc_port)); > > WARN_ON(dig_port->tc_legacy_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); > > - } > > + icl_tc_phy_set_safe_mode(dig_port, false); > > + > > + if (dig_port->tc_mode == TC_PORT_LEGACY) > > + return true; > > + > > + live_status_mask = tc_port_live_status_mask(dig_port); > > > > /* > > * 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_mode == TC_PORT_DP_ALT && > > - !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) { > > + if (!(live_status_mask & BIT(TC_PORT_DP_ALT))) { > > DRM_DEBUG_KMS("Port %s: PHY sudden disconnect\n", > > tc_port_name(dev_priv, tc_port)); > > icl_tc_phy_disconnect(dig_port); > > @@ -148,44 +213,36 @@ void icl_tc_phy_disconnect(struct intel_digital_port *dig_port) > > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > > > - /* > > - * TBT disconnection flow is read the live status, what was done in > > - * caller. > > - */ > > - if (dig_port->tc_mode == TC_PORT_DP_ALT || > > - dig_port->tc_mode == TC_PORT_LEGACY) { > > - u32 val; > > - > > - val = I915_READ(PORT_TX_DFLEXDPCSSS); > > - val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > > - I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > > + switch (dig_port->tc_mode) { > > + case TC_PORT_LEGACY: > > + case TC_PORT_DP_ALT: > > + icl_tc_phy_set_safe_mode(dig_port, true); > > + dig_port->tc_mode = TC_PORT_TBT_ALT; > > + break; > > + case TC_PORT_TBT_ALT: > > + /* Nothing to do, we stay in TBT-alt mode */ > > + break; > > + default: > > + MISSING_CASE(dig_port->tc_mode); > > } > > > > DRM_DEBUG_KMS("Port %s: mode %s disconnected\n", > > tc_port_name(dev_priv, tc_port), > > tc_port_mode_name(dig_port->tc_mode)); > > - > > - dig_port->tc_mode = TC_PORT_TBT_ALT; > > } > > > > static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, > > struct intel_digital_port *intel_dig_port, > > - bool is_legacy, bool is_typec, bool is_tbt) > > + u32 live_status_mask) > > { > > enum port port = intel_dig_port->base.port; > > enum tc_port_mode old_mode = intel_dig_port->tc_mode; > > > > - WARN_ON(is_legacy + is_typec + is_tbt != 1); > > - > > - if (is_legacy) > > - intel_dig_port->tc_mode = TC_PORT_LEGACY; > > - else if (is_typec) > > - intel_dig_port->tc_mode = TC_PORT_DP_ALT; > > - else if (is_tbt) > > - intel_dig_port->tc_mode = TC_PORT_TBT_ALT; > > - else > > + if (!live_status_mask) > > return; > > > > + intel_dig_port->tc_mode = fls(live_status_mask) - 1; > > + > > if (old_mode != intel_dig_port->tc_mode) > > DRM_DEBUG_KMS("Port %s: port has mode %s\n", > > tc_port_name(dev_priv, > > @@ -207,40 +264,19 @@ 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); > > - enum port port = dig_port->base.port; > > - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > > - bool is_legacy, is_typec, is_tbt; > > - u32 dpsp; > > - > > - /* > > - * Complain if we got a legacy port HPD, but VBT didn't mark the port as > > - * legacy. Treat the port as legacy from now on. > > - */ > > - if (!dig_port->tc_legacy_port && > > - I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) { > > - DRM_ERROR("Port %s: VBT incorrectly claims port is not TypeC legacy\n", > > - tc_port_name(dev_priv, tc_port)); > > - dig_port->tc_legacy_port = true; > > - } > > - is_legacy = dig_port->tc_legacy_port; > > + 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. > > */ > > - dpsp = I915_READ(PORT_TX_DFLEXDPSP); > > - 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 (!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, is_legacy, is_typec, > > - is_tbt); > > - > > + icl_update_tc_port_type(dev_priv, dig_port, live_status_mask); > > if (!icl_tc_phy_connect(dig_port)) > > return false; > > > > diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h > > index 94c62ac4a162..e937f5326959 100644 > > --- a/drivers/gpu/drm/i915/intel_tc.h > > +++ b/drivers/gpu/drm/i915/intel_tc.h > > @@ -8,6 +8,7 @@ struct intel_digital_port; > > void icl_tc_phy_disconnect(struct intel_digital_port *dig_port); > > > > bool intel_tc_port_connected(struct intel_digital_port *dig_port); > > +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port); > > int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port); > > > > #endif /* __INTEL_TC_H__ */ > > -- > Jani Nikula, Intel Open Source Graphics Center
On Tue, 2019-06-04 at 17:58 +0300, Imre Deak wrote: > Factor out helpers reading/parsing the TypeC specific registers, > making > current users of them clearer and letting us use them later. > > While at it also: > - Simplify icl_tc_phy_connect() with an early return in legacy mode. > - Simplify the live status check using one bitmask for all HPD bits. > - Remove a micro-optimisation of the repeated safe-mode clearing. > - Make sure we fix the legacy port flag in all cases. > > Except for the last two, no functional changes. > > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 5 +- > drivers/gpu/drm/i915/intel_tc.c | 166 +++++++++++++++++++-------- > ---- > drivers/gpu/drm/i915/intel_tc.h | 1 + > 3 files changed, 103 insertions(+), 69 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index 8f223d48d562..d236839bee19 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2983,7 +2983,6 @@ static void icl_program_mg_dp_mode(struct > intel_digital_port *intel_dig_port) > { > struct drm_i915_private *dev_priv = to_i915(intel_dig_port- > >base.base.dev); > enum port port = intel_dig_port->base.port; > - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > u32 ln0, ln1, lane_info; > > if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > @@ -2997,9 +2996,7 @@ static void icl_program_mg_dp_mode(struct > intel_digital_port *intel_dig_port) > ln0 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | > MG_DP_MODE_CFG_DP_X2_MODE); > ln1 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | > MG_DP_MODE_CFG_DP_X2_MODE); > > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > + lane_info = > intel_tc_port_get_lane_info(intel_dig_port); > > switch (lane_info) { > case 0x1: > diff --git a/drivers/gpu/drm/i915/intel_tc.c > b/drivers/gpu/drm/i915/intel_tc.c > index 07488235b67a..3fdcfa2bbaee 100644 > --- a/drivers/gpu/drm/i915/intel_tc.c > +++ b/drivers/gpu/drm/i915/intel_tc.c > @@ -43,10 +43,19 @@ static const char *tc_port_mode_name(enum > tc_port_mode mode) > return names[mode]; > } > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port > *dig_port) > +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port) > { > struct drm_i915_private *dev_priv = to_i915(dig_port- > >base.base.dev); > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > >base.port); > + u32 lane_info = I915_READ(PORT_TX_DFLEXDPSP); > + > + return (lane_info & DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > + DP_LANE_ASSIGNMENT_SHIFT(tc_port); > +} > + > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port > *dig_port) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port- > >base.base.dev); > intel_wakeref_t wakeref; > u32 lane_info; > > @@ -55,9 +64,7 @@ int intel_tc_port_fia_max_lane_count(struct > intel_digital_port *dig_port) > > lane_info = 0; > with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, > wakeref) > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > + lane_info = intel_tc_port_get_lane_info(dig_port); > > switch (lane_info) { > default: > @@ -75,6 +82,69 @@ int intel_tc_port_fia_max_lane_count(struct > intel_digital_port *dig_port) > } > } > > +static void tc_port_fixup_legacy_flag(struct intel_digital_port > *dig_port, > + u32 live_status_mask) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port- > >base.base.dev); > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > >base.port); > + u32 valid_hpd_mask = dig_port->tc_legacy_port ? > BIT(TC_PORT_LEGACY) : > + ~BIT(TC_PORT_LE > GACY); > + > + if (!(live_status_mask & ~valid_hpd_mask)) > + return; > + > + /* If live status mismatches the VBT flag, trust the live > status. */ > + DRM_ERROR("Port %s: live status %08x mismatches the legacy port > flag, fix flag\n", > + tc_port_name(dev_priv, tc_port), live_status_mask); > + > + dig_port->tc_legacy_port = !dig_port->tc_legacy_port; > +} > + > +static u32 tc_port_live_status_mask(struct intel_digital_port > *dig_port) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port- > >base.base.dev); > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > >base.port); > + u32 val = I915_READ(PORT_TX_DFLEXDPSP); > + u32 mask = 0; > + > + if (val & TC_LIVE_STATE_TBT(tc_port)) > + mask |= BIT(TC_PORT_TBT_ALT); > + if (val & TC_LIVE_STATE_TC(tc_port)) > + mask |= BIT(TC_PORT_DP_ALT); > + > + if (I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) > + mask |= BIT(TC_PORT_LEGACY); > + > + /* The sink can be connected only in a single mode. */ > + if (!WARN_ON(hweight32(mask) > 1)) > + tc_port_fixup_legacy_flag(dig_port, mask); The mask should be updated after the fixup > + > + return mask; > +} > + > +static bool icl_tc_phy_status_complete(struct intel_digital_port > *dig_port) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port- > >base.base.dev); > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > >base.port); > + > + return I915_READ(PORT_TX_DFLEXDPPMS) & > + DP_PHY_MODE_STATUS_COMPLETED(tc_port); > +} > + > +static void icl_tc_phy_set_safe_mode(struct intel_digital_port > *dig_port, > + bool enable) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port- > >base.base.dev); > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > >base.port); > + u32 val = I915_READ(PORT_TX_DFLEXDPCSSS); > + > + val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > + if (!enable) > + val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); Why complete remove the optimizations? You could do: old_val = val = I915_READ(PORT_TX_DFLEXDPCSSS); val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); if (!enable) val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); if (val != old_val) I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > + > + I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > +} > + > /* > * This function implements the first part of the Connect Flow > described by our > * specification, Gen11 TypeC Programming chapter. The rest of the > flow (reading > @@ -100,36 +170,31 @@ static bool icl_tc_phy_connect(struct > intel_digital_port *dig_port) > { > struct drm_i915_private *dev_priv = to_i915(dig_port- > >base.base.dev); > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > >base.port); > - u32 val; > + u32 live_status_mask; > > if (dig_port->tc_mode != TC_PORT_LEGACY && > dig_port->tc_mode != TC_PORT_DP_ALT) > return true; > > - val = I915_READ(PORT_TX_DFLEXDPPMS); > - if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) { > + if (!icl_tc_phy_status_complete(dig_port)) { > DRM_DEBUG_KMS("Port %s: PHY not ready\n", > tc_port_name(dev_priv, tc_port)); > WARN_ON(dig_port->tc_legacy_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); > - } > + icl_tc_phy_set_safe_mode(dig_port, false); > + > + if (dig_port->tc_mode == TC_PORT_LEGACY) > + return true; > + > + live_status_mask = tc_port_live_status_mask(dig_port); > > /* > * 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_mode == TC_PORT_DP_ALT && > - !(I915_READ(PORT_TX_DFLEXDPSP) & > TC_LIVE_STATE_TC(tc_port))) { > + if (!(live_status_mask & BIT(TC_PORT_DP_ALT))) { When live status is TC_PORT_TBT_ALT it will cause the "sudden disconnect" to be printed each time although icl_tc_phy_disconnect() will do nothing for TBT. > DRM_DEBUG_KMS("Port %s: PHY sudden disconnect\n", > tc_port_name(dev_priv, tc_port)); > icl_tc_phy_disconnect(dig_port); > @@ -148,44 +213,36 @@ void icl_tc_phy_disconnect(struct > intel_digital_port *dig_port) > struct drm_i915_private *dev_priv = to_i915(dig_port- > >base.base.dev); > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > >base.port); > > - /* > - * TBT disconnection flow is read the live status, what was > done in > - * caller. > - */ > - if (dig_port->tc_mode == TC_PORT_DP_ALT || > - dig_port->tc_mode == TC_PORT_LEGACY) { > - u32 val; > - > - val = I915_READ(PORT_TX_DFLEXDPCSSS); > - val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > - I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > + switch (dig_port->tc_mode) { > + case TC_PORT_LEGACY: > + case TC_PORT_DP_ALT: > + icl_tc_phy_set_safe_mode(dig_port, true); > + dig_port->tc_mode = TC_PORT_TBT_ALT; > + break; > + case TC_PORT_TBT_ALT: > + /* Nothing to do, we stay in TBT-alt mode */ > + break; > + default: > + MISSING_CASE(dig_port->tc_mode); > } > > DRM_DEBUG_KMS("Port %s: mode %s disconnected\n", > tc_port_name(dev_priv, tc_port), > tc_port_mode_name(dig_port->tc_mode)); > - > - dig_port->tc_mode = TC_PORT_TBT_ALT; > } > > static void icl_update_tc_port_type(struct drm_i915_private > *dev_priv, > struct intel_digital_port > *intel_dig_port, > - bool is_legacy, bool is_typec, bool > is_tbt) > + u32 live_status_mask) > { > enum port port = intel_dig_port->base.port; > enum tc_port_mode old_mode = intel_dig_port->tc_mode; > > - WARN_ON(is_legacy + is_typec + is_tbt != 1); > - > - if (is_legacy) > - intel_dig_port->tc_mode = TC_PORT_LEGACY; > - else if (is_typec) > - intel_dig_port->tc_mode = TC_PORT_DP_ALT; > - else if (is_tbt) > - intel_dig_port->tc_mode = TC_PORT_TBT_ALT; > - else > + if (!live_status_mask) > return; > > + intel_dig_port->tc_mode = fls(live_status_mask) - 1; > + > if (old_mode != intel_dig_port->tc_mode) > DRM_DEBUG_KMS("Port %s: port has mode %s\n", > tc_port_name(dev_priv, > @@ -207,40 +264,19 @@ 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); > - enum port port = dig_port->base.port; > - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > - bool is_legacy, is_typec, is_tbt; > - u32 dpsp; > - > - /* > - * Complain if we got a legacy port HPD, but VBT didn't mark > the port as > - * legacy. Treat the port as legacy from now on. > - */ > - if (!dig_port->tc_legacy_port && > - I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) { > - DRM_ERROR("Port %s: VBT incorrectly claims port is not > TypeC legacy\n", > - tc_port_name(dev_priv, tc_port)); > - dig_port->tc_legacy_port = true; > - } > - is_legacy = dig_port->tc_legacy_port; > + 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. > */ > - dpsp = I915_READ(PORT_TX_DFLEXDPSP); > - 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 (!live_status_mask && !dig_port->tc_legacy_port) { So now it will keep legacy port as unsafe even when disconnected. Maybe worth mention in commit message. > icl_tc_phy_disconnect(dig_port); > > return false; > } > > - icl_update_tc_port_type(dev_priv, dig_port, is_legacy, > is_typec, > - is_tbt); > - > + icl_update_tc_port_type(dev_priv, dig_port, live_status_mask); > if (!icl_tc_phy_connect(dig_port)) > return false; > > diff --git a/drivers/gpu/drm/i915/intel_tc.h > b/drivers/gpu/drm/i915/intel_tc.h > index 94c62ac4a162..e937f5326959 100644 > --- a/drivers/gpu/drm/i915/intel_tc.h > +++ b/drivers/gpu/drm/i915/intel_tc.h > @@ -8,6 +8,7 @@ struct intel_digital_port; > void icl_tc_phy_disconnect(struct intel_digital_port *dig_port); > > bool intel_tc_port_connected(struct intel_digital_port *dig_port); > +u32 intel_tc_port_get_lane_info(struct intel_digital_port > *dig_port); > int intel_tc_port_fia_max_lane_count(struct intel_digital_port > *dig_port); > > #endif /* __INTEL_TC_H__ */
On Sat, Jun 08, 2019 at 12:22:46AM +0300, Souza, Jose wrote: > On Tue, 2019-06-04 at 17:58 +0300, Imre Deak wrote: > > Factor out helpers reading/parsing the TypeC specific registers, > > making > > current users of them clearer and letting us use them later. > > > > While at it also: > > - Simplify icl_tc_phy_connect() with an early return in legacy mode. > > - Simplify the live status check using one bitmask for all HPD bits. > > - Remove a micro-optimisation of the repeated safe-mode clearing. > > - Make sure we fix the legacy port flag in all cases. > > > > Except for the last two, no functional changes. > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 5 +- > > drivers/gpu/drm/i915/intel_tc.c | 166 +++++++++++++++++++-------- > > ---- > > drivers/gpu/drm/i915/intel_tc.h | 1 + > > 3 files changed, 103 insertions(+), 69 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index 8f223d48d562..d236839bee19 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2983,7 +2983,6 @@ static void icl_program_mg_dp_mode(struct > > intel_digital_port *intel_dig_port) > > { > > struct drm_i915_private *dev_priv = to_i915(intel_dig_port- > > >base.base.dev); > > enum port port = intel_dig_port->base.port; > > - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > > u32 ln0, ln1, lane_info; > > > > if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > > @@ -2997,9 +2996,7 @@ static void icl_program_mg_dp_mode(struct > > intel_digital_port *intel_dig_port) > > ln0 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | > > MG_DP_MODE_CFG_DP_X2_MODE); > > ln1 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | > > MG_DP_MODE_CFG_DP_X2_MODE); > > > > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > > + lane_info = > > intel_tc_port_get_lane_info(intel_dig_port); > > > > switch (lane_info) { > > case 0x1: > > diff --git a/drivers/gpu/drm/i915/intel_tc.c > > b/drivers/gpu/drm/i915/intel_tc.c > > index 07488235b67a..3fdcfa2bbaee 100644 > > --- a/drivers/gpu/drm/i915/intel_tc.c > > +++ b/drivers/gpu/drm/i915/intel_tc.c > > @@ -43,10 +43,19 @@ static const char *tc_port_mode_name(enum > > tc_port_mode mode) > > return names[mode]; > > } > > > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port > > *dig_port) > > +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port) > > { > > struct drm_i915_private *dev_priv = to_i915(dig_port- > > >base.base.dev); > > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > > >base.port); > > + u32 lane_info = I915_READ(PORT_TX_DFLEXDPSP); > > + > > + return (lane_info & DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > > + DP_LANE_ASSIGNMENT_SHIFT(tc_port); > > +} > > + > > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port > > *dig_port) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port- > > >base.base.dev); > > intel_wakeref_t wakeref; > > u32 lane_info; > > > > @@ -55,9 +64,7 @@ int intel_tc_port_fia_max_lane_count(struct > > intel_digital_port *dig_port) > > > > lane_info = 0; > > with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, > > wakeref) > > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > > + lane_info = intel_tc_port_get_lane_info(dig_port); > > > > switch (lane_info) { > > default: > > @@ -75,6 +82,69 @@ int intel_tc_port_fia_max_lane_count(struct > > intel_digital_port *dig_port) > > } > > } > > > > +static void tc_port_fixup_legacy_flag(struct intel_digital_port > > *dig_port, > > + u32 live_status_mask) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port- > > >base.base.dev); > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > > >base.port); > > + u32 valid_hpd_mask = dig_port->tc_legacy_port ? > > BIT(TC_PORT_LEGACY) : > > + ~BIT(TC_PORT_LE > > GACY); > > + > > + if (!(live_status_mask & ~valid_hpd_mask)) > > + return; > > + > > + /* If live status mismatches the VBT flag, trust the live > > status. */ > > + DRM_ERROR("Port %s: live status %08x mismatches the legacy port > > flag, fix flag\n", > > + tc_port_name(dev_priv, tc_port), live_status_mask); > > + > > + dig_port->tc_legacy_port = !dig_port->tc_legacy_port; > > +} > > + > > +static u32 tc_port_live_status_mask(struct intel_digital_port > > *dig_port) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port- > > >base.base.dev); > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > > >base.port); > > + u32 val = I915_READ(PORT_TX_DFLEXDPSP); > > + u32 mask = 0; > > + > > + if (val & TC_LIVE_STATE_TBT(tc_port)) > > + mask |= BIT(TC_PORT_TBT_ALT); > > + if (val & TC_LIVE_STATE_TC(tc_port)) > > + mask |= BIT(TC_PORT_DP_ALT); > > + > > + if (I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) > > + mask |= BIT(TC_PORT_LEGACY); > > + > > + /* The sink can be connected only in a single mode. */ > > + if (!WARN_ON(hweight32(mask) > 1)) > > + tc_port_fixup_legacy_flag(dig_port, mask); > > The mask should be updated after the fixup tc_port_fixup_legacy_flag() will only fix dig_port->tc_legacy_port based on the mask, but mask itself will remain the same. (We trust the live status based mask what is the HW's opinion about what kind of sink got connected.) If hweight32(mask)>1 we can't really do anything, it'd be some HW issue which we haven't seen so far and that's handled anyway whenever converting the mask to tc_mode by a priority scheme (tc_mode=fls(mask)-1). > > > + > > + return mask; > > +} > > + > > +static bool icl_tc_phy_status_complete(struct intel_digital_port > > *dig_port) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port- > > >base.base.dev); > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > > >base.port); > > + > > + return I915_READ(PORT_TX_DFLEXDPPMS) & > > + DP_PHY_MODE_STATUS_COMPLETED(tc_port); > > +} > > + > > +static void icl_tc_phy_set_safe_mode(struct intel_digital_port > > *dig_port, > > + bool enable) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port- > > >base.base.dev); > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > > >base.port); > > + u32 val = I915_READ(PORT_TX_DFLEXDPCSSS); > > + > > + val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > > + if (!enable) > > + val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > > Why complete remove the optimizations? > You could do: > > old_val = val = I915_READ(PORT_TX_DFLEXDPCSSS); > val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > > if (!enable) > val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > > if (val != old_val) > I915_WRITE(PORT_TX_DFLEXDPCSSS, val); You could optimize register writes where there is a requirement for it, but there isn't any need for that here. > > + > > + I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > > +} > > + > > /* > > * This function implements the first part of the Connect Flow > > described by our > > * specification, Gen11 TypeC Programming chapter. The rest of the > > flow (reading > > @@ -100,36 +170,31 @@ static bool icl_tc_phy_connect(struct > > intel_digital_port *dig_port) > > { > > struct drm_i915_private *dev_priv = to_i915(dig_port- > > >base.base.dev); > > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > > >base.port); > > - u32 val; > > + u32 live_status_mask; > > > > if (dig_port->tc_mode != TC_PORT_LEGACY && > > dig_port->tc_mode != TC_PORT_DP_ALT) > > return true; > > > > - val = I915_READ(PORT_TX_DFLEXDPPMS); > > - if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) { > > + if (!icl_tc_phy_status_complete(dig_port)) { > > DRM_DEBUG_KMS("Port %s: PHY not ready\n", > > tc_port_name(dev_priv, tc_port)); > > WARN_ON(dig_port->tc_legacy_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); > > - } > > + icl_tc_phy_set_safe_mode(dig_port, false); > > + > > + if (dig_port->tc_mode == TC_PORT_LEGACY) > > + return true; > > + > > + live_status_mask = tc_port_live_status_mask(dig_port); > > > > /* > > * 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_mode == TC_PORT_DP_ALT && > > - !(I915_READ(PORT_TX_DFLEXDPSP) & > > TC_LIVE_STATE_TC(tc_port))) { > > + if (!(live_status_mask & BIT(TC_PORT_DP_ALT))) { > > When live status is TC_PORT_TBT_ALT it will cause the "sudden > disconnect" to be printed each time That can't really happen. There was first a DP-alt HPD, after which icl_update_tc_port_type() set tc_mode = TC_PORT_DP_ALT. The only reason for this live status check is to detect a quick unplug of the DP-alt sink (as required by BSpec), after which live status will be 0 here. > although icl_tc_phy_disconnect() will do nothing for TBT. We'll have here tc_mode = TC_PORT_DP_ALT, in which case icl_tc_phy_disconnect() will have to switch back to safe mode (since above we switched to unsafe mode already). Note that there is no change in behaviour here wrt. the current code. > > > DRM_DEBUG_KMS("Port %s: PHY sudden disconnect\n", > > tc_port_name(dev_priv, tc_port)); > > icl_tc_phy_disconnect(dig_port); > > @@ -148,44 +213,36 @@ void icl_tc_phy_disconnect(struct > > intel_digital_port *dig_port) > > struct drm_i915_private *dev_priv = to_i915(dig_port- > > >base.base.dev); > > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > > >base.port); > > > > - /* > > - * TBT disconnection flow is read the live status, what was > > done in > > - * caller. > > - */ > > - if (dig_port->tc_mode == TC_PORT_DP_ALT || > > - dig_port->tc_mode == TC_PORT_LEGACY) { > > - u32 val; > > - > > - val = I915_READ(PORT_TX_DFLEXDPCSSS); > > - val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); > > - I915_WRITE(PORT_TX_DFLEXDPCSSS, val); > > + switch (dig_port->tc_mode) { > > + case TC_PORT_LEGACY: > > + case TC_PORT_DP_ALT: > > + icl_tc_phy_set_safe_mode(dig_port, true); > > + dig_port->tc_mode = TC_PORT_TBT_ALT; > > + break; > > + case TC_PORT_TBT_ALT: > > + /* Nothing to do, we stay in TBT-alt mode */ > > + break; > > + default: > > + MISSING_CASE(dig_port->tc_mode); > > } > > > > DRM_DEBUG_KMS("Port %s: mode %s disconnected\n", > > tc_port_name(dev_priv, tc_port), > > tc_port_mode_name(dig_port->tc_mode)); > > - > > - dig_port->tc_mode = TC_PORT_TBT_ALT; > > } > > > > static void icl_update_tc_port_type(struct drm_i915_private > > *dev_priv, > > struct intel_digital_port > > *intel_dig_port, > > - bool is_legacy, bool is_typec, bool > > is_tbt) > > + u32 live_status_mask) > > { > > enum port port = intel_dig_port->base.port; > > enum tc_port_mode old_mode = intel_dig_port->tc_mode; > > > > - WARN_ON(is_legacy + is_typec + is_tbt != 1); > > - > > - if (is_legacy) > > - intel_dig_port->tc_mode = TC_PORT_LEGACY; > > - else if (is_typec) > > - intel_dig_port->tc_mode = TC_PORT_DP_ALT; > > - else if (is_tbt) > > - intel_dig_port->tc_mode = TC_PORT_TBT_ALT; > > - else > > + if (!live_status_mask) > > return; > > > > + intel_dig_port->tc_mode = fls(live_status_mask) - 1; > > + > > if (old_mode != intel_dig_port->tc_mode) > > DRM_DEBUG_KMS("Port %s: port has mode %s\n", > > tc_port_name(dev_priv, > > @@ -207,40 +264,19 @@ 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); > > - enum port port = dig_port->base.port; > > - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > > - bool is_legacy, is_typec, is_tbt; > > - u32 dpsp; > > - > > - /* > > - * Complain if we got a legacy port HPD, but VBT didn't mark > > the port as > > - * legacy. Treat the port as legacy from now on. > > - */ > > - if (!dig_port->tc_legacy_port && > > - I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) { > > - DRM_ERROR("Port %s: VBT incorrectly claims port is not > > TypeC legacy\n", > > - tc_port_name(dev_priv, tc_port)); > > - dig_port->tc_legacy_port = true; > > - } > > - is_legacy = dig_port->tc_legacy_port; > > + 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. > > */ > > - dpsp = I915_READ(PORT_TX_DFLEXDPSP); > > - 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 (!live_status_mask && !dig_port->tc_legacy_port) { > > So now it will keep legacy port as unsafe even when disconnected. > Maybe worth mention in commit message. That is how it was also before, since above we had is_legacy = dig_port->tc_legacy_port So no change in behaviour. > > > icl_tc_phy_disconnect(dig_port); > > > > return false; > > } > > > > - icl_update_tc_port_type(dev_priv, dig_port, is_legacy, > > is_typec, > > - is_tbt); > > - > > + icl_update_tc_port_type(dev_priv, dig_port, live_status_mask); > > if (!icl_tc_phy_connect(dig_port)) > > return false; > > > > diff --git a/drivers/gpu/drm/i915/intel_tc.h > > b/drivers/gpu/drm/i915/intel_tc.h > > index 94c62ac4a162..e937f5326959 100644 > > --- a/drivers/gpu/drm/i915/intel_tc.h > > +++ b/drivers/gpu/drm/i915/intel_tc.h > > @@ -8,6 +8,7 @@ struct intel_digital_port; > > void icl_tc_phy_disconnect(struct intel_digital_port *dig_port); > > > > bool intel_tc_port_connected(struct intel_digital_port *dig_port); > > +u32 intel_tc_port_get_lane_info(struct intel_digital_port > > *dig_port); > > int intel_tc_port_fia_max_lane_count(struct intel_digital_port > > *dig_port); > > > > #endif /* __INTEL_TC_H__ */
On Tue, Jun 04, 2019 at 05:58:12PM +0300, Imre Deak wrote: > Factor out helpers reading/parsing the TypeC specific registers, making > current users of them clearer and letting us use them later. > > While at it also: > - Simplify icl_tc_phy_connect() with an early return in legacy mode. > - Simplify the live status check using one bitmask for all HPD bits. > - Remove a micro-optimisation of the repeated safe-mode clearing. > - Make sure we fix the legacy port flag in all cases. > > Except for the last two, no functional changes. > > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 5 +- > drivers/gpu/drm/i915/intel_tc.c | 166 +++++++++++++++++++------------ > drivers/gpu/drm/i915/intel_tc.h | 1 + > 3 files changed, 103 insertions(+), 69 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 8f223d48d562..d236839bee19 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2983,7 +2983,6 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > { > struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev); > enum port port = intel_dig_port->base.port; > - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > u32 ln0, ln1, lane_info; > > if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > @@ -2997,9 +2996,7 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > ln0 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE); > ln1 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE); > > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > + lane_info = intel_tc_port_get_lane_info(intel_dig_port); > > switch (lane_info) { > case 0x1: > diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c > index 07488235b67a..3fdcfa2bbaee 100644 > --- a/drivers/gpu/drm/i915/intel_tc.c > +++ b/drivers/gpu/drm/i915/intel_tc.c > @@ -43,10 +43,19 @@ static const char *tc_port_mode_name(enum tc_port_mode mode) > return names[mode]; > } > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port) get_lane_mask() or something? > { > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > + u32 lane_info = I915_READ(PORT_TX_DFLEXDPSP); > + > + return (lane_info & DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > + DP_LANE_ASSIGNMENT_SHIFT(tc_port); > +} > + > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > intel_wakeref_t wakeref; > u32 lane_info; > > @@ -55,9 +64,7 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > lane_info = 0; > with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref) > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > + lane_info = intel_tc_port_get_lane_info(dig_port); > > switch (lane_info) { > default: > @@ -75,6 +82,69 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > } > } > > +static void tc_port_fixup_legacy_flag(struct intel_digital_port *dig_port, > + u32 live_status_mask) > +{ > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > + u32 valid_hpd_mask = dig_port->tc_legacy_port ? BIT(TC_PORT_LEGACY) : > + ~BIT(TC_PORT_LEGACY); > + > + if (!(live_status_mask & ~valid_hpd_mask)) > + return; A bit too many nots for me to follow. I guess what it's doing is checking if any of the other bits are set, and if so it assumes the mode was misdeclared in vbt? Actually, won't this end up ping-ponging back and forth if the hw really reports both legacy and non-legacy bits as set? > + > + /* If live status mismatches the VBT flag, trust the live status. */ > + DRM_ERROR("Port %s: live status %08x mismatches the legacy port flag, fix flag\n", > + tc_port_name(dev_priv, tc_port), live_status_mask); > + > + dig_port->tc_legacy_port = !dig_port->tc_legacy_port; > +} > +
On Tue, Jun 18, 2019 at 07:33:13PM +0300, Ville Syrjälä wrote: > On Tue, Jun 04, 2019 at 05:58:12PM +0300, Imre Deak wrote: > > Factor out helpers reading/parsing the TypeC specific registers, making > > current users of them clearer and letting us use them later. > > > > While at it also: > > - Simplify icl_tc_phy_connect() with an early return in legacy mode. > > - Simplify the live status check using one bitmask for all HPD bits. > > - Remove a micro-optimisation of the repeated safe-mode clearing. > > - Make sure we fix the legacy port flag in all cases. > > > > Except for the last two, no functional changes. > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 5 +- > > drivers/gpu/drm/i915/intel_tc.c | 166 +++++++++++++++++++------------ > > drivers/gpu/drm/i915/intel_tc.h | 1 + > > 3 files changed, 103 insertions(+), 69 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 8f223d48d562..d236839bee19 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2983,7 +2983,6 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > > { > > struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev); > > enum port port = intel_dig_port->base.port; > > - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > > u32 ln0, ln1, lane_info; > > > > if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > > @@ -2997,9 +2996,7 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > > ln0 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE); > > ln1 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE); > > > > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > > + lane_info = intel_tc_port_get_lane_info(intel_dig_port); > > > > switch (lane_info) { > > case 0x1: > > diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c > > index 07488235b67a..3fdcfa2bbaee 100644 > > --- a/drivers/gpu/drm/i915/intel_tc.c > > +++ b/drivers/gpu/drm/i915/intel_tc.c > > @@ -43,10 +43,19 @@ static const char *tc_port_mode_name(enum tc_port_mode mode) > > return names[mode]; > > } > > > > -int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port) > > get_lane_mask() or something? Ok. > > > { > > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > + u32 lane_info = I915_READ(PORT_TX_DFLEXDPSP); > > + > > + return (lane_info & DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > > + DP_LANE_ASSIGNMENT_SHIFT(tc_port); > > +} > > + > > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > intel_wakeref_t wakeref; > > u32 lane_info; > > > > @@ -55,9 +64,7 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > > > lane_info = 0; > > with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref) > > - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & > > - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> > > - DP_LANE_ASSIGNMENT_SHIFT(tc_port); > > + lane_info = intel_tc_port_get_lane_info(dig_port); > > > > switch (lane_info) { > > default: > > @@ -75,6 +82,69 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > } > > } > > > > +static void tc_port_fixup_legacy_flag(struct intel_digital_port *dig_port, > > + u32 live_status_mask) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > + u32 valid_hpd_mask = dig_port->tc_legacy_port ? BIT(TC_PORT_LEGACY) : > > + ~BIT(TC_PORT_LEGACY); > > + > > + if (!(live_status_mask & ~valid_hpd_mask)) > > + return; > > A bit too many nots for me to follow. I guess what it's doing is > checking if any of the other bits are set, and if so it assumes the > mode was misdeclared in vbt? Yes. > > Actually, won't this end up ping-ponging back and forth if the > hw really reports both legacy and non-legacy bits as set? If you mean multiple HPD bits being set by HW, we won't call this function to fix up the flag. > > > + > > + /* If live status mismatches the VBT flag, trust the live status. */ > > + DRM_ERROR("Port %s: live status %08x mismatches the legacy port flag, fix flag\n", > > + tc_port_name(dev_priv, tc_port), live_status_mask); > > + > > + dig_port->tc_legacy_port = !dig_port->tc_legacy_port; > > +} > > + > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 8f223d48d562..d236839bee19 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2983,7 +2983,6 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) { struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev); enum port port = intel_dig_port->base.port; - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); u32 ln0, ln1, lane_info; if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) @@ -2997,9 +2996,7 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) ln0 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE); ln1 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE); - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> - DP_LANE_ASSIGNMENT_SHIFT(tc_port); + lane_info = intel_tc_port_get_lane_info(intel_dig_port); switch (lane_info) { case 0x1: diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c index 07488235b67a..3fdcfa2bbaee 100644 --- a/drivers/gpu/drm/i915/intel_tc.c +++ b/drivers/gpu/drm/i915/intel_tc.c @@ -43,10 +43,19 @@ static const char *tc_port_mode_name(enum tc_port_mode mode) return names[mode]; } -int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port) { struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); + u32 lane_info = I915_READ(PORT_TX_DFLEXDPSP); + + return (lane_info & DP_LANE_ASSIGNMENT_MASK(tc_port)) >> + DP_LANE_ASSIGNMENT_SHIFT(tc_port); +} + +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) +{ + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); intel_wakeref_t wakeref; u32 lane_info; @@ -55,9 +64,7 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) lane_info = 0; with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref) - lane_info = (I915_READ(PORT_TX_DFLEXDPSP) & - DP_LANE_ASSIGNMENT_MASK(tc_port)) >> - DP_LANE_ASSIGNMENT_SHIFT(tc_port); + lane_info = intel_tc_port_get_lane_info(dig_port); switch (lane_info) { default: @@ -75,6 +82,69 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) } } +static void tc_port_fixup_legacy_flag(struct intel_digital_port *dig_port, + u32 live_status_mask) +{ + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); + u32 valid_hpd_mask = dig_port->tc_legacy_port ? BIT(TC_PORT_LEGACY) : + ~BIT(TC_PORT_LEGACY); + + if (!(live_status_mask & ~valid_hpd_mask)) + return; + + /* If live status mismatches the VBT flag, trust the live status. */ + DRM_ERROR("Port %s: live status %08x mismatches the legacy port flag, fix flag\n", + tc_port_name(dev_priv, tc_port), live_status_mask); + + dig_port->tc_legacy_port = !dig_port->tc_legacy_port; +} + +static u32 tc_port_live_status_mask(struct intel_digital_port *dig_port) +{ + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); + u32 val = I915_READ(PORT_TX_DFLEXDPSP); + u32 mask = 0; + + if (val & TC_LIVE_STATE_TBT(tc_port)) + mask |= BIT(TC_PORT_TBT_ALT); + if (val & TC_LIVE_STATE_TC(tc_port)) + mask |= BIT(TC_PORT_DP_ALT); + + if (I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) + mask |= BIT(TC_PORT_LEGACY); + + /* The sink can be connected only in a single mode. */ + if (!WARN_ON(hweight32(mask) > 1)) + tc_port_fixup_legacy_flag(dig_port, mask); + + return mask; +} + +static bool icl_tc_phy_status_complete(struct intel_digital_port *dig_port) +{ + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); + + return I915_READ(PORT_TX_DFLEXDPPMS) & + DP_PHY_MODE_STATUS_COMPLETED(tc_port); +} + +static void icl_tc_phy_set_safe_mode(struct intel_digital_port *dig_port, + bool enable) +{ + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); + enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); + u32 val = I915_READ(PORT_TX_DFLEXDPCSSS); + + val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); + if (!enable) + val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); + + I915_WRITE(PORT_TX_DFLEXDPCSSS, val); +} + /* * This function implements the first part of the Connect Flow described by our * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading @@ -100,36 +170,31 @@ static bool icl_tc_phy_connect(struct intel_digital_port *dig_port) { struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); - u32 val; + u32 live_status_mask; if (dig_port->tc_mode != TC_PORT_LEGACY && dig_port->tc_mode != TC_PORT_DP_ALT) return true; - val = I915_READ(PORT_TX_DFLEXDPPMS); - if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) { + if (!icl_tc_phy_status_complete(dig_port)) { DRM_DEBUG_KMS("Port %s: PHY not ready\n", tc_port_name(dev_priv, tc_port)); WARN_ON(dig_port->tc_legacy_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); - } + icl_tc_phy_set_safe_mode(dig_port, false); + + if (dig_port->tc_mode == TC_PORT_LEGACY) + return true; + + live_status_mask = tc_port_live_status_mask(dig_port); /* * 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_mode == TC_PORT_DP_ALT && - !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) { + if (!(live_status_mask & BIT(TC_PORT_DP_ALT))) { DRM_DEBUG_KMS("Port %s: PHY sudden disconnect\n", tc_port_name(dev_priv, tc_port)); icl_tc_phy_disconnect(dig_port); @@ -148,44 +213,36 @@ void icl_tc_phy_disconnect(struct intel_digital_port *dig_port) struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); - /* - * TBT disconnection flow is read the live status, what was done in - * caller. - */ - if (dig_port->tc_mode == TC_PORT_DP_ALT || - dig_port->tc_mode == TC_PORT_LEGACY) { - u32 val; - - val = I915_READ(PORT_TX_DFLEXDPCSSS); - val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port); - I915_WRITE(PORT_TX_DFLEXDPCSSS, val); + switch (dig_port->tc_mode) { + case TC_PORT_LEGACY: + case TC_PORT_DP_ALT: + icl_tc_phy_set_safe_mode(dig_port, true); + dig_port->tc_mode = TC_PORT_TBT_ALT; + break; + case TC_PORT_TBT_ALT: + /* Nothing to do, we stay in TBT-alt mode */ + break; + default: + MISSING_CASE(dig_port->tc_mode); } DRM_DEBUG_KMS("Port %s: mode %s disconnected\n", tc_port_name(dev_priv, tc_port), tc_port_mode_name(dig_port->tc_mode)); - - dig_port->tc_mode = TC_PORT_TBT_ALT; } static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, struct intel_digital_port *intel_dig_port, - bool is_legacy, bool is_typec, bool is_tbt) + u32 live_status_mask) { enum port port = intel_dig_port->base.port; enum tc_port_mode old_mode = intel_dig_port->tc_mode; - WARN_ON(is_legacy + is_typec + is_tbt != 1); - - if (is_legacy) - intel_dig_port->tc_mode = TC_PORT_LEGACY; - else if (is_typec) - intel_dig_port->tc_mode = TC_PORT_DP_ALT; - else if (is_tbt) - intel_dig_port->tc_mode = TC_PORT_TBT_ALT; - else + if (!live_status_mask) return; + intel_dig_port->tc_mode = fls(live_status_mask) - 1; + if (old_mode != intel_dig_port->tc_mode) DRM_DEBUG_KMS("Port %s: port has mode %s\n", tc_port_name(dev_priv, @@ -207,40 +264,19 @@ 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); - enum port port = dig_port->base.port; - enum tc_port tc_port = intel_port_to_tc(dev_priv, port); - bool is_legacy, is_typec, is_tbt; - u32 dpsp; - - /* - * Complain if we got a legacy port HPD, but VBT didn't mark the port as - * legacy. Treat the port as legacy from now on. - */ - if (!dig_port->tc_legacy_port && - I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) { - DRM_ERROR("Port %s: VBT incorrectly claims port is not TypeC legacy\n", - tc_port_name(dev_priv, tc_port)); - dig_port->tc_legacy_port = true; - } - is_legacy = dig_port->tc_legacy_port; + 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. */ - dpsp = I915_READ(PORT_TX_DFLEXDPSP); - 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 (!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, is_legacy, is_typec, - is_tbt); - + icl_update_tc_port_type(dev_priv, dig_port, live_status_mask); if (!icl_tc_phy_connect(dig_port)) return false; diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h index 94c62ac4a162..e937f5326959 100644 --- a/drivers/gpu/drm/i915/intel_tc.h +++ b/drivers/gpu/drm/i915/intel_tc.h @@ -8,6 +8,7 @@ struct intel_digital_port; void icl_tc_phy_disconnect(struct intel_digital_port *dig_port); bool intel_tc_port_connected(struct intel_digital_port *dig_port); +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port); int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port); #endif /* __INTEL_TC_H__ */
Factor out helpers reading/parsing the TypeC specific registers, making current users of them clearer and letting us use them later. While at it also: - Simplify icl_tc_phy_connect() with an early return in legacy mode. - Simplify the live status check using one bitmask for all HPD bits. - Remove a micro-optimisation of the repeated safe-mode clearing. - Make sure we fix the legacy port flag in all cases. Except for the last two, no functional changes. Cc: José Roberto de Souza <jose.souza@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 5 +- drivers/gpu/drm/i915/intel_tc.c | 166 +++++++++++++++++++------------ drivers/gpu/drm/i915/intel_tc.h | 1 + 3 files changed, 103 insertions(+), 69 deletions(-)