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@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > 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__ */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
