| Message ID | 20190604145826.16424-5-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix TypeC port mode switching | expand |
On Tue, 2019-06-04 at 17:58 +0300, Imre Deak wrote: > The TypeC port mode can switch dynamically, to reflect that better > call > the port's mode as 'mode' rather than 'type'. > > While at it: > - s/TC_PORT_TBT/TC_PORT_TBT_ALT/ and s/TC_PORT_TYPEC/TC_PORT_DP_ALT/. > 'TYPEC' is ambiguous, TBT_ALT and DP_ALT better match the reality. > > - Remove the 'unknown' TypeC port mode. The mode is always known, > it's > the TBT-alt/safe mode after HW reset and after disconnecting the > PHY. > Simplify the tc_port/tc_type checks accordingly. > > - Don't WARN if the port mode changes, that can happen normally. > > No functional changes. There is, the default tc_mode value now is TC_PORT_TBT_ALT instead of TC_PORT_UNKNOWN. With the change above: Reviewed-by: José Roberto de Souza < jose.souza@intel.com> Also consider split this patch in two. > > Cc: Animesh Manna <animesh.manna@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 11 +++--- > drivers/gpu/drm/i915/intel_display.h | 7 ++-- > drivers/gpu/drm/i915/intel_dp.c | 2 +- > drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > drivers/gpu/drm/i915/intel_tc.c | 48 +++++++++++------------ > ---- > 6 files changed, 31 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index 5a1c98438375..a3574f14a3d0 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2986,14 +2986,14 @@ static void icl_program_mg_dp_mode(struct > intel_digital_port *intel_dig_port) > enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > u32 ln0, ln1, lane_info; > > - if (tc_port == PORT_TC_NONE || intel_dig_port->tc_type == > TC_PORT_TBT) > + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > return; > > ln0 = I915_READ(MG_DP_MODE(0, port)); > ln1 = I915_READ(MG_DP_MODE(1, port)); > > - switch (intel_dig_port->tc_type) { > - case TC_PORT_TYPEC: > + switch (intel_dig_port->tc_mode) { > + case TC_PORT_DP_ALT: > 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); > > @@ -3036,7 +3036,7 @@ static void icl_program_mg_dp_mode(struct > intel_digital_port *intel_dig_port) > break; > > default: > - MISSING_CASE(intel_dig_port->tc_type); > + MISSING_CASE(intel_dig_port->tc_mode); > return; > } > > @@ -3630,8 +3630,7 @@ intel_ddi_pre_pll_enable(struct intel_encoder > *encoder, > * Program the lane count for static/dynamic connections on > Type-C ports. > * Skip this step for TBT. > */ > - if (dig_port->tc_type == TC_PORT_UNKNOWN || > - dig_port->tc_type == TC_PORT_TBT) > + if (dig_port->tc_mode == TC_PORT_TBT_ALT) > return; > > intel_ddi_set_fia_lane_count(encoder, crtc_state, port); > diff --git a/drivers/gpu/drm/i915/intel_display.h > b/drivers/gpu/drm/i915/intel_display.h > index ee6b8194a459..d296556ed82e 100644 > --- a/drivers/gpu/drm/i915/intel_display.h > +++ b/drivers/gpu/drm/i915/intel_display.h > @@ -189,10 +189,9 @@ enum tc_port { > I915_MAX_TC_PORTS > }; > > -enum tc_port_type { > - TC_PORT_UNKNOWN = 0, > - TC_PORT_TYPEC, > - TC_PORT_TBT, > +enum tc_port_mode { > + TC_PORT_TBT_ALT, > + TC_PORT_DP_ALT, > TC_PORT_LEGACY, > }; > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > index b69310bd9914..e1e27662aa6d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1175,7 +1175,7 @@ static u32 skl_get_aux_send_ctl(struct intel_dp > *intel_dp, > DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(32) | > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > > - if (intel_dig_port->tc_type == TC_PORT_TBT) > + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > ret |= DP_AUX_CH_CTL_TBT_IO; > > return ret; > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > index 69787f259677..f4787650a0d3 100644 > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > @@ -2817,7 +2817,7 @@ icl_get_dpll(struct intel_crtc_state > *crtc_state, > intel_dig_port = enc_to_dig_port(&encoder- > >base); > } > > - if (intel_dig_port->tc_type == TC_PORT_TBT) { > + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) { > min = DPLL_ID_ICL_TBTPLL; > max = min; > ret = icl_calc_dpll_state(crtc_state, encoder); > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 0dcc03592d6e..30cd49dbd0d8 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1225,7 +1225,7 @@ struct intel_digital_port { > enum aux_ch aux_ch; > enum intel_display_power_domain ddi_io_power_domain; > bool tc_legacy_port:1; > - enum tc_port_type tc_type; > + enum tc_port_mode tc_mode; > > void (*write_infoframe)(struct intel_encoder *encoder, > const struct intel_crtc_state > *crtc_state, > diff --git a/drivers/gpu/drm/i915/intel_tc.c > b/drivers/gpu/drm/i915/intel_tc.c > index 7a1b5870945f..a3057c44bec6 100644 > --- a/drivers/gpu/drm/i915/intel_tc.c > +++ b/drivers/gpu/drm/i915/intel_tc.c > @@ -6,19 +6,18 @@ > #include "i915_drv.h" > #include "intel_tc.h" > > -static const char *tc_type_name(enum tc_port_type type) > +static const char *tc_port_mode_name(enum tc_port_mode mode) > { > static const char * const names[] = { > - [TC_PORT_UNKNOWN] = "unknown", > + [TC_PORT_TBT_ALT] = "tbt-alt", > + [TC_PORT_DP_ALT] = "dp-alt", > [TC_PORT_LEGACY] = "legacy", > - [TC_PORT_TYPEC] = "typec", > - [TC_PORT_TBT] = "tbt", > }; > > - if (WARN_ON(type >= ARRAY_SIZE(names))) > - type = TC_PORT_UNKNOWN; > + if (WARN_ON(mode >= ARRAY_SIZE(names))) > + mode = TC_PORT_TBT_ALT; > > - return names[type]; > + return names[mode]; > } > > int intel_tc_port_fia_max_lane_count(struct intel_digital_port > *dig_port) > @@ -28,7 +27,7 @@ int intel_tc_port_fia_max_lane_count(struct > intel_digital_port *dig_port) > intel_wakeref_t wakeref; > u32 lane_info; > > - if (tc_port == PORT_TC_NONE || dig_port->tc_type != > TC_PORT_TYPEC) > + if (dig_port->tc_mode != TC_PORT_DP_ALT) > return 4; > > lane_info = 0; > @@ -80,8 +79,8 @@ static bool icl_tc_phy_connect(struct > intel_digital_port *dig_port) > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > >base.port); > u32 val; > > - if (dig_port->tc_type != TC_PORT_LEGACY && > - dig_port->tc_type != TC_PORT_TYPEC) > + if (dig_port->tc_mode != TC_PORT_LEGACY && > + dig_port->tc_mode != TC_PORT_DP_ALT) > return true; > > val = I915_READ(PORT_TX_DFLEXDPPMS); > @@ -105,7 +104,7 @@ static bool icl_tc_phy_connect(struct > intel_digital_port *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_type == TC_PORT_TYPEC && > + if (dig_port->tc_mode == TC_PORT_DP_ALT && > !(I915_READ(PORT_TX_DFLEXDPSP) & > TC_LIVE_STATE_TC(tc_port))) { > DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", > tc_port); > icl_tc_phy_disconnect(dig_port); > @@ -124,15 +123,12 @@ 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); > > - if (dig_port->tc_type == TC_PORT_UNKNOWN) > - return; > - > /* > * TBT disconnection flow is read the live status, what was > done in > * caller. > */ > - if (dig_port->tc_type == TC_PORT_TYPEC || > - dig_port->tc_type == TC_PORT_LEGACY) { > + if (dig_port->tc_mode == TC_PORT_DP_ALT || > + dig_port->tc_mode == TC_PORT_LEGACY) { > u32 val; > > val = I915_READ(PORT_TX_DFLEXDPCSSS); > @@ -142,9 +138,9 @@ void icl_tc_phy_disconnect(struct > intel_digital_port *dig_port) > > DRM_DEBUG_KMS("Port %c TC type %s disconnected\n", > port_name(dig_port->base.port), > - tc_type_name(dig_port->tc_type)); > + tc_port_mode_name(dig_port->tc_mode)); > > - dig_port->tc_type = TC_PORT_UNKNOWN; > + dig_port->tc_mode = TC_PORT_TBT_ALT; > } > > static void icl_update_tc_port_type(struct drm_i915_private > *dev_priv, > @@ -152,26 +148,22 @@ static void icl_update_tc_port_type(struct > drm_i915_private *dev_priv, > bool is_legacy, bool is_typec, bool > is_tbt) > { > enum port port = intel_dig_port->base.port; > - enum tc_port_type old_type = intel_dig_port->tc_type; > + 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_type = TC_PORT_LEGACY; > + intel_dig_port->tc_mode = TC_PORT_LEGACY; > else if (is_typec) > - intel_dig_port->tc_type = TC_PORT_TYPEC; > + intel_dig_port->tc_mode = TC_PORT_DP_ALT; > else if (is_tbt) > - intel_dig_port->tc_type = TC_PORT_TBT; > + intel_dig_port->tc_mode = TC_PORT_TBT_ALT; > else > return; > > - /* Types are not supposed to be changed at runtime. */ > - WARN_ON(old_type != TC_PORT_UNKNOWN && > - old_type != intel_dig_port->tc_type); > - > - if (old_type != intel_dig_port->tc_type) > + if (old_mode != intel_dig_port->tc_mode) > DRM_DEBUG_KMS("Port %c has TC type %s\n", > port_name(port), > - tc_type_name(intel_dig_port->tc_type)); > + tc_port_mode_name(intel_dig_port- > >tc_mode)); > } > >
On Fri, Jun 07, 2019 at 10:15:12PM +0300, Souza, Jose wrote: > On Tue, 2019-06-04 at 17:58 +0300, Imre Deak wrote: > > The TypeC port mode can switch dynamically, to reflect that better > > call > > the port's mode as 'mode' rather than 'type'. > > > > While at it: > > - s/TC_PORT_TBT/TC_PORT_TBT_ALT/ and s/TC_PORT_TYPEC/TC_PORT_DP_ALT/. > > 'TYPEC' is ambiguous, TBT_ALT and DP_ALT better match the reality. > > > > - Remove the 'unknown' TypeC port mode. The mode is always known, > > it's > > the TBT-alt/safe mode after HW reset and after disconnecting the > > PHY. > > Simplify the tc_port/tc_type checks accordingly. > > > > - Don't WARN if the port mode changes, that can happen normally. > > > > No functional changes. > > There is, the default tc_mode value now is TC_PORT_TBT_ALT instead of > TC_PORT_UNKNOWN. That doesn't change the functionality. Before the change the two modes were treated the same way; that fact motivated the change to remove TC_PORT_UNKNOWN and just use TC_PORT_TBT_ALT instead, which I tried to explain in the commit log. > > With the change above: > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > Also consider split this patch in two. Makes sense to keep it as-is, based on keeping the functionality as described above? > > > > Cc: Animesh Manna <animesh.manna@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 11 +++--- > > drivers/gpu/drm/i915/intel_display.h | 7 ++-- > > drivers/gpu/drm/i915/intel_dp.c | 2 +- > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +- > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_tc.c | 48 +++++++++++------------ > > ---- > > 6 files changed, 31 insertions(+), 41 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index 5a1c98438375..a3574f14a3d0 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2986,14 +2986,14 @@ static void icl_program_mg_dp_mode(struct > > intel_digital_port *intel_dig_port) > > enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > > u32 ln0, ln1, lane_info; > > > > - if (tc_port == PORT_TC_NONE || intel_dig_port->tc_type == > > TC_PORT_TBT) > > + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > > return; > > > > ln0 = I915_READ(MG_DP_MODE(0, port)); > > ln1 = I915_READ(MG_DP_MODE(1, port)); > > > > - switch (intel_dig_port->tc_type) { > > - case TC_PORT_TYPEC: > > + switch (intel_dig_port->tc_mode) { > > + case TC_PORT_DP_ALT: > > 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); > > > > @@ -3036,7 +3036,7 @@ static void icl_program_mg_dp_mode(struct > > intel_digital_port *intel_dig_port) > > break; > > > > default: > > - MISSING_CASE(intel_dig_port->tc_type); > > + MISSING_CASE(intel_dig_port->tc_mode); > > return; > > } > > > > @@ -3630,8 +3630,7 @@ intel_ddi_pre_pll_enable(struct intel_encoder > > *encoder, > > * Program the lane count for static/dynamic connections on > > Type-C ports. > > * Skip this step for TBT. > > */ > > - if (dig_port->tc_type == TC_PORT_UNKNOWN || > > - dig_port->tc_type == TC_PORT_TBT) > > + if (dig_port->tc_mode == TC_PORT_TBT_ALT) > > return; > > > > intel_ddi_set_fia_lane_count(encoder, crtc_state, port); > > diff --git a/drivers/gpu/drm/i915/intel_display.h > > b/drivers/gpu/drm/i915/intel_display.h > > index ee6b8194a459..d296556ed82e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.h > > +++ b/drivers/gpu/drm/i915/intel_display.h > > @@ -189,10 +189,9 @@ enum tc_port { > > I915_MAX_TC_PORTS > > }; > > > > -enum tc_port_type { > > - TC_PORT_UNKNOWN = 0, > > - TC_PORT_TYPEC, > > - TC_PORT_TBT, > > +enum tc_port_mode { > > + TC_PORT_TBT_ALT, > > + TC_PORT_DP_ALT, > > TC_PORT_LEGACY, > > }; > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index b69310bd9914..e1e27662aa6d 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1175,7 +1175,7 @@ static u32 skl_get_aux_send_ctl(struct intel_dp > > *intel_dp, > > DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(32) | > > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > > > > - if (intel_dig_port->tc_type == TC_PORT_TBT) > > + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > > ret |= DP_AUX_CH_CTL_TBT_IO; > > > > return ret; > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > index 69787f259677..f4787650a0d3 100644 > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > @@ -2817,7 +2817,7 @@ icl_get_dpll(struct intel_crtc_state > > *crtc_state, > > intel_dig_port = enc_to_dig_port(&encoder- > > >base); > > } > > > > - if (intel_dig_port->tc_type == TC_PORT_TBT) { > > + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) { > > min = DPLL_ID_ICL_TBTPLL; > > max = min; > > ret = icl_calc_dpll_state(crtc_state, encoder); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 0dcc03592d6e..30cd49dbd0d8 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1225,7 +1225,7 @@ struct intel_digital_port { > > enum aux_ch aux_ch; > > enum intel_display_power_domain ddi_io_power_domain; > > bool tc_legacy_port:1; > > - enum tc_port_type tc_type; > > + enum tc_port_mode tc_mode; > > > > void (*write_infoframe)(struct intel_encoder *encoder, > > const struct intel_crtc_state > > *crtc_state, > > diff --git a/drivers/gpu/drm/i915/intel_tc.c > > b/drivers/gpu/drm/i915/intel_tc.c > > index 7a1b5870945f..a3057c44bec6 100644 > > --- a/drivers/gpu/drm/i915/intel_tc.c > > +++ b/drivers/gpu/drm/i915/intel_tc.c > > @@ -6,19 +6,18 @@ > > #include "i915_drv.h" > > #include "intel_tc.h" > > > > -static const char *tc_type_name(enum tc_port_type type) > > +static const char *tc_port_mode_name(enum tc_port_mode mode) > > { > > static const char * const names[] = { > > - [TC_PORT_UNKNOWN] = "unknown", > > + [TC_PORT_TBT_ALT] = "tbt-alt", > > + [TC_PORT_DP_ALT] = "dp-alt", > > [TC_PORT_LEGACY] = "legacy", > > - [TC_PORT_TYPEC] = "typec", > > - [TC_PORT_TBT] = "tbt", > > }; > > > > - if (WARN_ON(type >= ARRAY_SIZE(names))) > > - type = TC_PORT_UNKNOWN; > > + if (WARN_ON(mode >= ARRAY_SIZE(names))) > > + mode = TC_PORT_TBT_ALT; > > > > - return names[type]; > > + return names[mode]; > > } > > > > int intel_tc_port_fia_max_lane_count(struct intel_digital_port > > *dig_port) > > @@ -28,7 +27,7 @@ int intel_tc_port_fia_max_lane_count(struct > > intel_digital_port *dig_port) > > intel_wakeref_t wakeref; > > u32 lane_info; > > > > - if (tc_port == PORT_TC_NONE || dig_port->tc_type != > > TC_PORT_TYPEC) > > + if (dig_port->tc_mode != TC_PORT_DP_ALT) > > return 4; > > > > lane_info = 0; > > @@ -80,8 +79,8 @@ static bool icl_tc_phy_connect(struct > > intel_digital_port *dig_port) > > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port- > > >base.port); > > u32 val; > > > > - if (dig_port->tc_type != TC_PORT_LEGACY && > > - dig_port->tc_type != TC_PORT_TYPEC) > > + if (dig_port->tc_mode != TC_PORT_LEGACY && > > + dig_port->tc_mode != TC_PORT_DP_ALT) > > return true; > > > > val = I915_READ(PORT_TX_DFLEXDPPMS); > > @@ -105,7 +104,7 @@ static bool icl_tc_phy_connect(struct > > intel_digital_port *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_type == TC_PORT_TYPEC && > > + if (dig_port->tc_mode == TC_PORT_DP_ALT && > > !(I915_READ(PORT_TX_DFLEXDPSP) & > > TC_LIVE_STATE_TC(tc_port))) { > > DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", > > tc_port); > > icl_tc_phy_disconnect(dig_port); > > @@ -124,15 +123,12 @@ 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); > > > > - if (dig_port->tc_type == TC_PORT_UNKNOWN) > > - return; > > - > > /* > > * TBT disconnection flow is read the live status, what was > > done in > > * caller. > > */ > > - if (dig_port->tc_type == TC_PORT_TYPEC || > > - dig_port->tc_type == TC_PORT_LEGACY) { > > + if (dig_port->tc_mode == TC_PORT_DP_ALT || > > + dig_port->tc_mode == TC_PORT_LEGACY) { > > u32 val; > > > > val = I915_READ(PORT_TX_DFLEXDPCSSS); > > @@ -142,9 +138,9 @@ void icl_tc_phy_disconnect(struct > > intel_digital_port *dig_port) > > > > DRM_DEBUG_KMS("Port %c TC type %s disconnected\n", > > port_name(dig_port->base.port), > > - tc_type_name(dig_port->tc_type)); > > + tc_port_mode_name(dig_port->tc_mode)); > > > > - dig_port->tc_type = TC_PORT_UNKNOWN; > > + dig_port->tc_mode = TC_PORT_TBT_ALT; > > } > > > > static void icl_update_tc_port_type(struct drm_i915_private > > *dev_priv, > > @@ -152,26 +148,22 @@ static void icl_update_tc_port_type(struct > > drm_i915_private *dev_priv, > > bool is_legacy, bool is_typec, bool > > is_tbt) > > { > > enum port port = intel_dig_port->base.port; > > - enum tc_port_type old_type = intel_dig_port->tc_type; > > + 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_type = TC_PORT_LEGACY; > > + intel_dig_port->tc_mode = TC_PORT_LEGACY; > > else if (is_typec) > > - intel_dig_port->tc_type = TC_PORT_TYPEC; > > + intel_dig_port->tc_mode = TC_PORT_DP_ALT; > > else if (is_tbt) > > - intel_dig_port->tc_type = TC_PORT_TBT; > > + intel_dig_port->tc_mode = TC_PORT_TBT_ALT; > > else > > return; > > > > - /* Types are not supposed to be changed at runtime. */ > > - WARN_ON(old_type != TC_PORT_UNKNOWN && > > - old_type != intel_dig_port->tc_type); > > - > > - if (old_type != intel_dig_port->tc_type) > > + if (old_mode != intel_dig_port->tc_mode) > > DRM_DEBUG_KMS("Port %c has TC type %s\n", > > port_name(port), > > - tc_type_name(intel_dig_port->tc_type)); > > + tc_port_mode_name(intel_dig_port- > > >tc_mode)); > > } > > > >
On Tue, Jun 04, 2019 at 05:58:07PM +0300, Imre Deak wrote: >The TypeC port mode can switch dynamically, to reflect that better call >the port's mode as 'mode' rather than 'type'. > >While at it: >- s/TC_PORT_TBT/TC_PORT_TBT_ALT/ and s/TC_PORT_TYPEC/TC_PORT_DP_ALT/. > 'TYPEC' is ambiguous, TBT_ALT and DP_ALT better match the reality. \o/ > >- Remove the 'unknown' TypeC port mode. The mode is always known, it's > the TBT-alt/safe mode after HW reset and after disconnecting the PHY. > Simplify the tc_port/tc_type checks accordingly. I think the unknown was there to cover the first time we set the type (aka mode). Since we don't do this during initialization and only when updating the connected state, we needed that to protect some functions. > >- Don't WARN if the port mode changes, that can happen normally. > >No functional changes. > >Cc: Animesh Manna <animesh.manna@intel.com> >Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >Cc: José Roberto de Souza <jose.souza@intel.com> >Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >Signed-off-by: Imre Deak <imre.deak@intel.com> >--- > drivers/gpu/drm/i915/intel_ddi.c | 11 +++--- > drivers/gpu/drm/i915/intel_display.h | 7 ++-- > drivers/gpu/drm/i915/intel_dp.c | 2 +- > drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > drivers/gpu/drm/i915/intel_tc.c | 48 +++++++++++---------------- > 6 files changed, 31 insertions(+), 41 deletions(-) > >diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >index 5a1c98438375..a3574f14a3d0 100644 >--- a/drivers/gpu/drm/i915/intel_ddi.c >+++ b/drivers/gpu/drm/i915/intel_ddi.c >@@ -2986,14 +2986,14 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > u32 ln0, ln1, lane_info; > >- if (tc_port == PORT_TC_NONE || intel_dig_port->tc_type == TC_PORT_TBT) >+ if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > return; > > ln0 = I915_READ(MG_DP_MODE(0, port)); > ln1 = I915_READ(MG_DP_MODE(1, port)); > >- switch (intel_dig_port->tc_type) { >- case TC_PORT_TYPEC: >+ switch (intel_dig_port->tc_mode) { >+ case TC_PORT_DP_ALT: > 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); > >@@ -3036,7 +3036,7 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > break; > > default: >- MISSING_CASE(intel_dig_port->tc_type); >+ MISSING_CASE(intel_dig_port->tc_mode); > return; > } > >@@ -3630,8 +3630,7 @@ intel_ddi_pre_pll_enable(struct intel_encoder *encoder, > * Program the lane count for static/dynamic connections on Type-C ports. > * Skip this step for TBT. > */ >- if (dig_port->tc_type == TC_PORT_UNKNOWN || >- dig_port->tc_type == TC_PORT_TBT) >+ if (dig_port->tc_mode == TC_PORT_TBT_ALT) > return; > > intel_ddi_set_fia_lane_count(encoder, crtc_state, port); >diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h >index ee6b8194a459..d296556ed82e 100644 >--- a/drivers/gpu/drm/i915/intel_display.h >+++ b/drivers/gpu/drm/i915/intel_display.h >@@ -189,10 +189,9 @@ enum tc_port { > I915_MAX_TC_PORTS > }; > >-enum tc_port_type { >- TC_PORT_UNKNOWN = 0, >- TC_PORT_TYPEC, >- TC_PORT_TBT, >+enum tc_port_mode { >+ TC_PORT_TBT_ALT, >+ TC_PORT_DP_ALT, > TC_PORT_LEGACY, > }; > >diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >index b69310bd9914..e1e27662aa6d 100644 >--- a/drivers/gpu/drm/i915/intel_dp.c >+++ b/drivers/gpu/drm/i915/intel_dp.c >@@ -1175,7 +1175,7 @@ static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp, > DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(32) | > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > >- if (intel_dig_port->tc_type == TC_PORT_TBT) >+ if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > ret |= DP_AUX_CH_CTL_TBT_IO; > > return ret; >diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c >index 69787f259677..f4787650a0d3 100644 >--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c >+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c >@@ -2817,7 +2817,7 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, > intel_dig_port = enc_to_dig_port(&encoder->base); > } > >- if (intel_dig_port->tc_type == TC_PORT_TBT) { >+ if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) { > min = DPLL_ID_ICL_TBTPLL; > max = min; > ret = icl_calc_dpll_state(crtc_state, encoder); >diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >index 0dcc03592d6e..30cd49dbd0d8 100644 >--- a/drivers/gpu/drm/i915/intel_drv.h >+++ b/drivers/gpu/drm/i915/intel_drv.h >@@ -1225,7 +1225,7 @@ struct intel_digital_port { > enum aux_ch aux_ch; > enum intel_display_power_domain ddi_io_power_domain; > bool tc_legacy_port:1; not related, but why do we add a hole in the struct on purpose?!? >- enum tc_port_type tc_type; >+ enum tc_port_mode tc_mode; > > void (*write_infoframe)(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state, >diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c >index 7a1b5870945f..a3057c44bec6 100644 >--- a/drivers/gpu/drm/i915/intel_tc.c >+++ b/drivers/gpu/drm/i915/intel_tc.c >@@ -6,19 +6,18 @@ > #include "i915_drv.h" > #include "intel_tc.h" > >-static const char *tc_type_name(enum tc_port_type type) >+static const char *tc_port_mode_name(enum tc_port_mode mode) > { > static const char * const names[] = { >- [TC_PORT_UNKNOWN] = "unknown", >+ [TC_PORT_TBT_ALT] = "tbt-alt", >+ [TC_PORT_DP_ALT] = "dp-alt", > [TC_PORT_LEGACY] = "legacy", >- [TC_PORT_TYPEC] = "typec", >- [TC_PORT_TBT] = "tbt", > }; > >- if (WARN_ON(type >= ARRAY_SIZE(names))) >- type = TC_PORT_UNKNOWN; >+ if (WARN_ON(mode >= ARRAY_SIZE(names))) >+ mode = TC_PORT_TBT_ALT; > >- return names[type]; >+ return names[mode]; > } > > int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) >@@ -28,7 +27,7 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > intel_wakeref_t wakeref; > u32 lane_info; > >- if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC) >+ if (dig_port->tc_mode != TC_PORT_DP_ALT) > return 4; > > lane_info = 0; >@@ -80,8 +79,8 @@ static bool icl_tc_phy_connect(struct intel_digital_port *dig_port) > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > u32 val; > >- if (dig_port->tc_type != TC_PORT_LEGACY && >- dig_port->tc_type != TC_PORT_TYPEC) >+ if (dig_port->tc_mode != TC_PORT_LEGACY && >+ dig_port->tc_mode != TC_PORT_DP_ALT) > return true; > > val = I915_READ(PORT_TX_DFLEXDPPMS); >@@ -105,7 +104,7 @@ static bool icl_tc_phy_connect(struct intel_digital_port *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_type == TC_PORT_TYPEC && >+ if (dig_port->tc_mode == TC_PORT_DP_ALT && > !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) { > DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port); > icl_tc_phy_disconnect(dig_port); >@@ -124,15 +123,12 @@ 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); > >- if (dig_port->tc_type == TC_PORT_UNKNOWN) >- return; >- > /* > * TBT disconnection flow is read the live status, what was done in > * caller. > */ >- if (dig_port->tc_type == TC_PORT_TYPEC || >- dig_port->tc_type == TC_PORT_LEGACY) { >+ if (dig_port->tc_mode == TC_PORT_DP_ALT || >+ dig_port->tc_mode == TC_PORT_LEGACY) { > u32 val; > > val = I915_READ(PORT_TX_DFLEXDPCSSS); >@@ -142,9 +138,9 @@ void icl_tc_phy_disconnect(struct intel_digital_port *dig_port) > > DRM_DEBUG_KMS("Port %c TC type %s disconnected\n", > port_name(dig_port->base.port), >- tc_type_name(dig_port->tc_type)); >+ tc_port_mode_name(dig_port->tc_mode)); > >- dig_port->tc_type = TC_PORT_UNKNOWN; >+ dig_port->tc_mode = TC_PORT_TBT_ALT; humn... if this is what the hw has after reset, then I think it covers my concern above. Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Lucas De Marchi > } > > static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, >@@ -152,26 +148,22 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, > bool is_legacy, bool is_typec, bool is_tbt) > { > enum port port = intel_dig_port->base.port; >- enum tc_port_type old_type = intel_dig_port->tc_type; >+ 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_type = TC_PORT_LEGACY; >+ intel_dig_port->tc_mode = TC_PORT_LEGACY; > else if (is_typec) >- intel_dig_port->tc_type = TC_PORT_TYPEC; >+ intel_dig_port->tc_mode = TC_PORT_DP_ALT; > else if (is_tbt) >- intel_dig_port->tc_type = TC_PORT_TBT; >+ intel_dig_port->tc_mode = TC_PORT_TBT_ALT; > else > return; > >- /* Types are not supposed to be changed at runtime. */ >- WARN_ON(old_type != TC_PORT_UNKNOWN && >- old_type != intel_dig_port->tc_type); >- >- if (old_type != intel_dig_port->tc_type) >+ if (old_mode != intel_dig_port->tc_mode) > DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port), >- tc_type_name(intel_dig_port->tc_type)); >+ tc_port_mode_name(intel_dig_port->tc_mode)); > } > > >-- >2.17.1 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jun 10, 2019 at 04:21:30PM -0700, Lucas De Marchi wrote: > On Tue, Jun 04, 2019 at 05:58:07PM +0300, Imre Deak wrote: > > The TypeC port mode can switch dynamically, to reflect that better call > > the port's mode as 'mode' rather than 'type'. > > > > While at it: > > - s/TC_PORT_TBT/TC_PORT_TBT_ALT/ and s/TC_PORT_TYPEC/TC_PORT_DP_ALT/. > > 'TYPEC' is ambiguous, TBT_ALT and DP_ALT better match the reality. > > \o/ > > > > > - Remove the 'unknown' TypeC port mode. The mode is always known, it's > > the TBT-alt/safe mode after HW reset and after disconnecting the PHY. > > Simplify the tc_port/tc_type checks accordingly. > > I think the unknown was there to cover the first time we set the type > (aka mode). Since we don't do this during initialization and only when > updating the connected state, we needed that to protect some functions. Ok, but the mode is still known even before updating the connected state (that is probing), we just have to make sure that we read out the HW state (which is what this mode is) early enough. At least selecting the proper AUX power well (DP-Alt or TBT-Alt AUX) depends on this. That HW readout happens already now via the intel_ddi_init() function. > > > > - Don't WARN if the port mode changes, that can happen normally. > > > > No functional changes. > > > > Cc: Animesh Manna <animesh.manna@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 11 +++--- > > drivers/gpu/drm/i915/intel_display.h | 7 ++-- > > drivers/gpu/drm/i915/intel_dp.c | 2 +- > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +- > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_tc.c | 48 +++++++++++---------------- > > 6 files changed, 31 insertions(+), 41 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 5a1c98438375..a3574f14a3d0 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2986,14 +2986,14 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > > enum tc_port tc_port = intel_port_to_tc(dev_priv, port); > > u32 ln0, ln1, lane_info; > > > > - if (tc_port == PORT_TC_NONE || intel_dig_port->tc_type == TC_PORT_TBT) > > + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > > return; > > > > ln0 = I915_READ(MG_DP_MODE(0, port)); > > ln1 = I915_READ(MG_DP_MODE(1, port)); > > > > - switch (intel_dig_port->tc_type) { > > - case TC_PORT_TYPEC: > > + switch (intel_dig_port->tc_mode) { > > + case TC_PORT_DP_ALT: > > 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); > > > > @@ -3036,7 +3036,7 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) > > break; > > > > default: > > - MISSING_CASE(intel_dig_port->tc_type); > > + MISSING_CASE(intel_dig_port->tc_mode); > > return; > > } > > > > @@ -3630,8 +3630,7 @@ intel_ddi_pre_pll_enable(struct intel_encoder *encoder, > > * Program the lane count for static/dynamic connections on Type-C ports. > > * Skip this step for TBT. > > */ > > - if (dig_port->tc_type == TC_PORT_UNKNOWN || > > - dig_port->tc_type == TC_PORT_TBT) > > + if (dig_port->tc_mode == TC_PORT_TBT_ALT) > > return; > > > > intel_ddi_set_fia_lane_count(encoder, crtc_state, port); > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h > > index ee6b8194a459..d296556ed82e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.h > > +++ b/drivers/gpu/drm/i915/intel_display.h > > @@ -189,10 +189,9 @@ enum tc_port { > > I915_MAX_TC_PORTS > > }; > > > > -enum tc_port_type { > > - TC_PORT_UNKNOWN = 0, > > - TC_PORT_TYPEC, > > - TC_PORT_TBT, > > +enum tc_port_mode { > > + TC_PORT_TBT_ALT, > > + TC_PORT_DP_ALT, > > TC_PORT_LEGACY, > > }; > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index b69310bd9914..e1e27662aa6d 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1175,7 +1175,7 @@ static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp, > > DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(32) | > > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > > > > - if (intel_dig_port->tc_type == TC_PORT_TBT) > > + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) > > ret |= DP_AUX_CH_CTL_TBT_IO; > > > > return ret; > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > index 69787f259677..f4787650a0d3 100644 > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > @@ -2817,7 +2817,7 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, > > intel_dig_port = enc_to_dig_port(&encoder->base); > > } > > > > - if (intel_dig_port->tc_type == TC_PORT_TBT) { > > + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) { > > min = DPLL_ID_ICL_TBTPLL; > > max = min; > > ret = icl_calc_dpll_state(crtc_state, encoder); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 0dcc03592d6e..30cd49dbd0d8 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1225,7 +1225,7 @@ struct intel_digital_port { > > enum aux_ch aux_ch; > > enum intel_display_power_domain ddi_io_power_domain; > > bool tc_legacy_port:1; > > not related, but why do we add a hole in the struct on purpose?!? The purpose was to group all the TypeC related fields at one spot. We could save some space gathering all the bools in this struct (release_cl2_override, tc_legacy_port) into a bool bitfield. > > > - enum tc_port_type tc_type; > > + enum tc_port_mode tc_mode; > > > > void (*write_infoframe)(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state, > > diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c > > index 7a1b5870945f..a3057c44bec6 100644 > > --- a/drivers/gpu/drm/i915/intel_tc.c > > +++ b/drivers/gpu/drm/i915/intel_tc.c > > @@ -6,19 +6,18 @@ > > #include "i915_drv.h" > > #include "intel_tc.h" > > > > -static const char *tc_type_name(enum tc_port_type type) > > +static const char *tc_port_mode_name(enum tc_port_mode mode) > > { > > static const char * const names[] = { > > - [TC_PORT_UNKNOWN] = "unknown", > > + [TC_PORT_TBT_ALT] = "tbt-alt", > > + [TC_PORT_DP_ALT] = "dp-alt", > > [TC_PORT_LEGACY] = "legacy", > > - [TC_PORT_TYPEC] = "typec", > > - [TC_PORT_TBT] = "tbt", > > }; > > > > - if (WARN_ON(type >= ARRAY_SIZE(names))) > > - type = TC_PORT_UNKNOWN; > > + if (WARN_ON(mode >= ARRAY_SIZE(names))) > > + mode = TC_PORT_TBT_ALT; > > > > - return names[type]; > > + return names[mode]; > > } > > > > int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > @@ -28,7 +27,7 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) > > intel_wakeref_t wakeref; > > u32 lane_info; > > > > - if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC) > > + if (dig_port->tc_mode != TC_PORT_DP_ALT) > > return 4; > > > > lane_info = 0; > > @@ -80,8 +79,8 @@ static bool icl_tc_phy_connect(struct intel_digital_port *dig_port) > > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > u32 val; > > > > - if (dig_port->tc_type != TC_PORT_LEGACY && > > - dig_port->tc_type != TC_PORT_TYPEC) > > + if (dig_port->tc_mode != TC_PORT_LEGACY && > > + dig_port->tc_mode != TC_PORT_DP_ALT) > > return true; > > > > val = I915_READ(PORT_TX_DFLEXDPPMS); > > @@ -105,7 +104,7 @@ static bool icl_tc_phy_connect(struct intel_digital_port *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_type == TC_PORT_TYPEC && > > + if (dig_port->tc_mode == TC_PORT_DP_ALT && > > !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) { > > DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port); > > icl_tc_phy_disconnect(dig_port); > > @@ -124,15 +123,12 @@ 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); > > > > - if (dig_port->tc_type == TC_PORT_UNKNOWN) > > - return; > > - > > /* > > * TBT disconnection flow is read the live status, what was done in > > * caller. > > */ > > - if (dig_port->tc_type == TC_PORT_TYPEC || > > - dig_port->tc_type == TC_PORT_LEGACY) { > > + if (dig_port->tc_mode == TC_PORT_DP_ALT || > > + dig_port->tc_mode == TC_PORT_LEGACY) { > > u32 val; > > > > val = I915_READ(PORT_TX_DFLEXDPCSSS); > > @@ -142,9 +138,9 @@ void icl_tc_phy_disconnect(struct intel_digital_port *dig_port) > > > > DRM_DEBUG_KMS("Port %c TC type %s disconnected\n", > > port_name(dig_port->base.port), > > - tc_type_name(dig_port->tc_type)); > > + tc_port_mode_name(dig_port->tc_mode)); > > > > - dig_port->tc_type = TC_PORT_UNKNOWN; > > + dig_port->tc_mode = TC_PORT_TBT_ALT; > > humn... if this is what the hw has after reset, Yes, IOW if we'd only connect/disconnect a TBT-Alt sink we wouldn't have to do anything to connect/disconnect the PHY (icl_tc_phy_connect() / icl_tc_phy_disconnect() would both become NOPs). That's unlike if we'd had only a DP-Alt/legacy sink where we have the TypeC specific programming steps to connect/disconnect the PHY. > then I think it covers my concern above. > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> > > Lucas De Marchi > > > } > > > > static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, > > @@ -152,26 +148,22 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, > > bool is_legacy, bool is_typec, bool is_tbt) > > { > > enum port port = intel_dig_port->base.port; > > - enum tc_port_type old_type = intel_dig_port->tc_type; > > + 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_type = TC_PORT_LEGACY; > > + intel_dig_port->tc_mode = TC_PORT_LEGACY; > > else if (is_typec) > > - intel_dig_port->tc_type = TC_PORT_TYPEC; > > + intel_dig_port->tc_mode = TC_PORT_DP_ALT; > > else if (is_tbt) > > - intel_dig_port->tc_type = TC_PORT_TBT; > > + intel_dig_port->tc_mode = TC_PORT_TBT_ALT; > > else > > return; > > > > - /* Types are not supposed to be changed at runtime. */ > > - WARN_ON(old_type != TC_PORT_UNKNOWN && > > - old_type != intel_dig_port->tc_type); > > - > > - if (old_type != intel_dig_port->tc_type) > > + if (old_mode != intel_dig_port->tc_mode) > > DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port), > > - tc_type_name(intel_dig_port->tc_type)); > > + tc_port_mode_name(intel_dig_port->tc_mode)); > > } > > > > > > -- > > 2.17.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 5a1c98438375..a3574f14a3d0 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2986,14 +2986,14 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) enum tc_port tc_port = intel_port_to_tc(dev_priv, port); u32 ln0, ln1, lane_info; - if (tc_port == PORT_TC_NONE || intel_dig_port->tc_type == TC_PORT_TBT) + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) return; ln0 = I915_READ(MG_DP_MODE(0, port)); ln1 = I915_READ(MG_DP_MODE(1, port)); - switch (intel_dig_port->tc_type) { - case TC_PORT_TYPEC: + switch (intel_dig_port->tc_mode) { + case TC_PORT_DP_ALT: 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); @@ -3036,7 +3036,7 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port) break; default: - MISSING_CASE(intel_dig_port->tc_type); + MISSING_CASE(intel_dig_port->tc_mode); return; } @@ -3630,8 +3630,7 @@ intel_ddi_pre_pll_enable(struct intel_encoder *encoder, * Program the lane count for static/dynamic connections on Type-C ports. * Skip this step for TBT. */ - if (dig_port->tc_type == TC_PORT_UNKNOWN || - dig_port->tc_type == TC_PORT_TBT) + if (dig_port->tc_mode == TC_PORT_TBT_ALT) return; intel_ddi_set_fia_lane_count(encoder, crtc_state, port); diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h index ee6b8194a459..d296556ed82e 100644 --- a/drivers/gpu/drm/i915/intel_display.h +++ b/drivers/gpu/drm/i915/intel_display.h @@ -189,10 +189,9 @@ enum tc_port { I915_MAX_TC_PORTS }; -enum tc_port_type { - TC_PORT_UNKNOWN = 0, - TC_PORT_TYPEC, - TC_PORT_TBT, +enum tc_port_mode { + TC_PORT_TBT_ALT, + TC_PORT_DP_ALT, TC_PORT_LEGACY, }; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b69310bd9914..e1e27662aa6d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1175,7 +1175,7 @@ static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp, DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(32) | DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); - if (intel_dig_port->tc_type == TC_PORT_TBT) + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) ret |= DP_AUX_CH_CTL_TBT_IO; return ret; diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c index 69787f259677..f4787650a0d3 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2817,7 +2817,7 @@ icl_get_dpll(struct intel_crtc_state *crtc_state, intel_dig_port = enc_to_dig_port(&encoder->base); } - if (intel_dig_port->tc_type == TC_PORT_TBT) { + if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT) { min = DPLL_ID_ICL_TBTPLL; max = min; ret = icl_calc_dpll_state(crtc_state, encoder); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0dcc03592d6e..30cd49dbd0d8 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1225,7 +1225,7 @@ struct intel_digital_port { enum aux_ch aux_ch; enum intel_display_power_domain ddi_io_power_domain; bool tc_legacy_port:1; - enum tc_port_type tc_type; + enum tc_port_mode tc_mode; void (*write_infoframe)(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state, diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c index 7a1b5870945f..a3057c44bec6 100644 --- a/drivers/gpu/drm/i915/intel_tc.c +++ b/drivers/gpu/drm/i915/intel_tc.c @@ -6,19 +6,18 @@ #include "i915_drv.h" #include "intel_tc.h" -static const char *tc_type_name(enum tc_port_type type) +static const char *tc_port_mode_name(enum tc_port_mode mode) { static const char * const names[] = { - [TC_PORT_UNKNOWN] = "unknown", + [TC_PORT_TBT_ALT] = "tbt-alt", + [TC_PORT_DP_ALT] = "dp-alt", [TC_PORT_LEGACY] = "legacy", - [TC_PORT_TYPEC] = "typec", - [TC_PORT_TBT] = "tbt", }; - if (WARN_ON(type >= ARRAY_SIZE(names))) - type = TC_PORT_UNKNOWN; + if (WARN_ON(mode >= ARRAY_SIZE(names))) + mode = TC_PORT_TBT_ALT; - return names[type]; + return names[mode]; } int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) @@ -28,7 +27,7 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port) intel_wakeref_t wakeref; u32 lane_info; - if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC) + if (dig_port->tc_mode != TC_PORT_DP_ALT) return 4; lane_info = 0; @@ -80,8 +79,8 @@ static bool icl_tc_phy_connect(struct intel_digital_port *dig_port) enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); u32 val; - if (dig_port->tc_type != TC_PORT_LEGACY && - dig_port->tc_type != TC_PORT_TYPEC) + if (dig_port->tc_mode != TC_PORT_LEGACY && + dig_port->tc_mode != TC_PORT_DP_ALT) return true; val = I915_READ(PORT_TX_DFLEXDPPMS); @@ -105,7 +104,7 @@ static bool icl_tc_phy_connect(struct intel_digital_port *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_type == TC_PORT_TYPEC && + if (dig_port->tc_mode == TC_PORT_DP_ALT && !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) { DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port); icl_tc_phy_disconnect(dig_port); @@ -124,15 +123,12 @@ 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); - if (dig_port->tc_type == TC_PORT_UNKNOWN) - return; - /* * TBT disconnection flow is read the live status, what was done in * caller. */ - if (dig_port->tc_type == TC_PORT_TYPEC || - dig_port->tc_type == TC_PORT_LEGACY) { + if (dig_port->tc_mode == TC_PORT_DP_ALT || + dig_port->tc_mode == TC_PORT_LEGACY) { u32 val; val = I915_READ(PORT_TX_DFLEXDPCSSS); @@ -142,9 +138,9 @@ void icl_tc_phy_disconnect(struct intel_digital_port *dig_port) DRM_DEBUG_KMS("Port %c TC type %s disconnected\n", port_name(dig_port->base.port), - tc_type_name(dig_port->tc_type)); + tc_port_mode_name(dig_port->tc_mode)); - dig_port->tc_type = TC_PORT_UNKNOWN; + dig_port->tc_mode = TC_PORT_TBT_ALT; } static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, @@ -152,26 +148,22 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, bool is_legacy, bool is_typec, bool is_tbt) { enum port port = intel_dig_port->base.port; - enum tc_port_type old_type = intel_dig_port->tc_type; + 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_type = TC_PORT_LEGACY; + intel_dig_port->tc_mode = TC_PORT_LEGACY; else if (is_typec) - intel_dig_port->tc_type = TC_PORT_TYPEC; + intel_dig_port->tc_mode = TC_PORT_DP_ALT; else if (is_tbt) - intel_dig_port->tc_type = TC_PORT_TBT; + intel_dig_port->tc_mode = TC_PORT_TBT_ALT; else return; - /* Types are not supposed to be changed at runtime. */ - WARN_ON(old_type != TC_PORT_UNKNOWN && - old_type != intel_dig_port->tc_type); - - if (old_type != intel_dig_port->tc_type) + if (old_mode != intel_dig_port->tc_mode) DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port), - tc_type_name(intel_dig_port->tc_type)); + tc_port_mode_name(intel_dig_port->tc_mode)); }
The TypeC port mode can switch dynamically, to reflect that better call the port's mode as 'mode' rather than 'type'. While at it: - s/TC_PORT_TBT/TC_PORT_TBT_ALT/ and s/TC_PORT_TYPEC/TC_PORT_DP_ALT/. 'TYPEC' is ambiguous, TBT_ALT and DP_ALT better match the reality. - Remove the 'unknown' TypeC port mode. The mode is always known, it's the TBT-alt/safe mode after HW reset and after disconnecting the PHY. Simplify the tc_port/tc_type checks accordingly. - Don't WARN if the port mode changes, that can happen normally. No functional changes. Cc: Animesh Manna <animesh.manna@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 11 +++--- drivers/gpu/drm/i915/intel_display.h | 7 ++-- drivers/gpu/drm/i915/intel_dp.c | 2 +- drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_tc.c | 48 +++++++++++---------------- 6 files changed, 31 insertions(+), 41 deletions(-)