On Thu, Jun 06, 2019 at 11:47:20AM +0300, Jani Nikula wrote:
> On Tue, 04 Jun 2019, Imre Deak <[email protected]> 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 <[email protected]>
> > Cc: Rodrigo Vivi <[email protected]>
> > Cc: Paulo Zanoni <[email protected]>
> > Signed-off-by: Imre Deak <[email protected]>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
