On Thu, Jun 20, 2019 at 05:05:57PM +0300, Imre Deak wrote: > The TypeC port mode needs to stay fixed whenever the port is active. Do > that by introducing a tc_link_refcount to account for active ports, > avoiding changing the port mode if a reference is held. > > During the modeset commit phase we also have to reset the port mode and > update the active PLL reflecting the new port mode. We can do this only > once the port and its old PLL has been already disabled. Add the new > encoder update_prepare/complete hooks that are called around the whole > enabling sequence. The TypeC specific hooks of these will reset the port > mode, update the active PLL if the port will be active and ensure that > the port mode will stay fixed for the duration of the whole enabling > sequence by holding a tc_link_refcount. > > During the port enabling, the pre_pll_enable/post_pll_disable hooks will > take/release a tc_link_refcount to ensure the port mode stays fixed > while the port is active. > > Changing the port mode should also be avoided during connector detection > and AUX transfers if the port is active, we'll do that by checking the > port's tc_link_refcount. > > When resetting the port mode we also have to take into account the > maximum lanes provided by the FIA. It's guaranteed to be 4 in TBT-alt > and legacy modes, but there may be less lanes available in DP-alt mode, > in which case we have to fall back to TBT-alt mode. > > While at it also update icl_tc_phy_connect()'s code comment, reflecting > the current way of switching the port mode. > > v2: > - Add the update_prepare/complete hooks to the encoder instead of the > connector. (Ville) > - Simplify intel_connector_needs_modeset() by removing redundant if. > (Ville) > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 41 +++++++- > drivers/gpu/drm/i915/display/intel_display.c | 96 ++++++++++++++++++- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 28 +++++- > drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 3 + > drivers/gpu/drm/i915/display/intel_tc.c | 94 +++++++++++++----- > drivers/gpu/drm/i915/display/intel_tc.h | 3 + > drivers/gpu/drm/i915/intel_drv.h | 8 ++ > 7 files changed, 243 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 0c5bfbd66b19..ddbc4944b4e0 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -3623,6 +3623,30 @@ static void intel_ddi_set_fia_lane_count(struct intel_encoder *encoder, > I915_WRITE(PORT_TX_DFLEXDPMLE1, val); > } > > +void > +intel_ddi_update_prepare(struct intel_atomic_state *state, > + struct intel_encoder *encoder, > + struct intel_crtc *crtc) > +{ > + struct intel_crtc_state *crtc_state = > + crtc ? intel_atomic_get_new_crtc_state(state, crtc) : NULL; > + int required_lanes = crtc_state ? crtc_state->lane_count : 1; > + > + WARN_ON(crtc && crtc->active); > + > + intel_tc_port_get_link(enc_to_dig_port(&encoder->base), required_lanes); > + if (crtc_state && crtc_state->base.active) > + intel_update_active_dpll(state, crtc, encoder); > +} > + > +void > +intel_ddi_update_complete(struct intel_atomic_state *state, > + struct intel_encoder *encoder, > + struct intel_crtc *crtc) > +{ > + intel_tc_port_put_link(enc_to_dig_port(&encoder->base)); > +} > + Yeah, looks quite a bit cleaner than the v1. Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > static void > intel_ddi_pre_pll_enable(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state, > @@ -3630,10 +3654,13 @@ intel_ddi_pre_pll_enable(struct intel_encoder *encoder, > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > + bool is_tc_port = intel_port_is_tc(dev_priv, encoder->port); > enum port port = encoder->port; > > - if (intel_crtc_has_dp_encoder(crtc_state) || > - intel_port_is_tc(dev_priv, encoder->port)) > + if (is_tc_port) > + intel_tc_port_get_link(dig_port, crtc_state->lane_count); > + > + if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port) > intel_display_power_get(dev_priv, > intel_ddi_main_link_aux_domain(dig_port)); > > @@ -3658,11 +3685,14 @@ intel_ddi_post_pll_disable(struct intel_encoder *encoder, > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > + bool is_tc_port = intel_port_is_tc(dev_priv, encoder->port); > > - if (intel_crtc_has_dp_encoder(crtc_state) || > - intel_port_is_tc(dev_priv, encoder->port)) > + if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port) > intel_display_power_put_unchecked(dev_priv, > intel_ddi_main_link_aux_domain(dig_port)); > + > + if (is_tc_port) > + intel_tc_port_put_link(dig_port); > } > > static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp) > @@ -4256,6 +4286,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > !port_info->supports_tbt; > > intel_tc_port_init(intel_dig_port, is_legacy); > + > + intel_encoder->update_prepare = intel_ddi_update_prepare; > + intel_encoder->update_complete = intel_ddi_update_complete; > } > > switch (port) { > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 93e3f568d7db..679307a84062 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -6037,6 +6037,94 @@ static void intel_crtc_disable_planes(struct intel_atomic_state *state, > intel_frontbuffer_flip(dev_priv, fb_bits); > } > > +/* > + * intel_connector_primary_encoder - get the primary encoder for a connector > + * @connector: connector for which to return the encoder > + * > + * Returns the primary encoder for a connector. There is a 1:1 mapping from > + * all connectors to their encoder, except for DP-MST connectors which have > + * both a virtual and a primary encoder. These DP-MST primary encoders can be > + * pointed to by as many DP-MST connectors as there are pipes. > + */ > +static struct intel_encoder * > +intel_connector_primary_encoder(struct intel_connector *connector) > +{ > + struct intel_encoder *encoder; > + > + if (connector->mst_port) > + return &dp_to_dig_port(connector->mst_port)->base; > + > + encoder = intel_attached_encoder(&connector->base); > + WARN_ON(!encoder); > + > + return encoder; > +} > + > +static bool > +intel_connector_needs_modeset(struct intel_atomic_state *state, > + const struct drm_connector_state *old_conn_state, > + const struct drm_connector_state *new_conn_state) > +{ > + return new_conn_state->crtc != old_conn_state->crtc || > + (new_conn_state->crtc && > + needs_modeset(drm_atomic_get_new_crtc_state(&state->base, > + new_conn_state->crtc))); > +} > + > +static void intel_encoders_update_prepare(struct intel_atomic_state *state) > +{ > + struct drm_connector_state *old_conn_state; > + struct drm_connector_state *new_conn_state; > + struct drm_connector *conn; > + int i; > + > + for_each_oldnew_connector_in_state(&state->base, conn, > + old_conn_state, new_conn_state, i) { > + struct intel_encoder *encoder; > + struct intel_crtc *crtc; > + > + if (!intel_connector_needs_modeset(state, > + old_conn_state, > + new_conn_state)) > + continue; > + > + encoder = intel_connector_primary_encoder(to_intel_connector(conn)); > + if (!encoder->update_prepare) > + continue; > + > + crtc = new_conn_state->crtc ? > + to_intel_crtc(new_conn_state->crtc) : NULL; > + encoder->update_prepare(state, encoder, crtc); > + } > +} > + > +static void intel_encoders_update_complete(struct intel_atomic_state *state) > +{ > + struct drm_connector_state *old_conn_state; > + struct drm_connector_state *new_conn_state; > + struct drm_connector *conn; > + int i; > + > + for_each_oldnew_connector_in_state(&state->base, conn, > + old_conn_state, new_conn_state, i) { > + struct intel_encoder *encoder; > + struct intel_crtc *crtc; > + > + if (!intel_connector_needs_modeset(state, > + old_conn_state, > + new_conn_state)) > + continue; > + > + encoder = intel_connector_primary_encoder(to_intel_connector(conn)); > + if (!encoder->update_complete) > + continue; > + > + crtc = new_conn_state->crtc ? > + to_intel_crtc(new_conn_state->crtc) : NULL; > + encoder->update_complete(state, encoder, crtc); > + } > +} > + > static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc, > struct intel_crtc_state *crtc_state, > struct drm_atomic_state *old_state) > @@ -13898,14 +13986,20 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) > } > } > > + if (intel_state->modeset) > + intel_encoders_update_prepare(intel_state); > + > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > dev_priv->display.update_crtcs(state); > > - if (intel_state->modeset) > + if (intel_state->modeset) { > + intel_encoders_update_complete(intel_state); > + > intel_set_cdclk_post_plane_update(dev_priv, > &intel_state->cdclk.actual, > &dev_priv->cdclk.actual, > intel_state->cdclk.pipe); > + } > > /* FIXME: We should call drm_atomic_helper_commit_hw_done() here > * already, but still need the state for the delayed optimization. To > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > index a996a3fad48c..cbb9aa458f19 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > @@ -1939,7 +1939,9 @@ struct intel_dpll_mgr { > struct intel_encoder *encoder); > void (*put_dplls)(struct intel_atomic_state *state, > struct intel_crtc *crtc); > - > + void (*update_active_dpll)(struct intel_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder); > void (*dump_hw_state)(struct drm_i915_private *dev_priv, > const struct intel_dpll_hw_state *hw_state); > }; > @@ -3400,6 +3402,7 @@ static const struct intel_dpll_mgr icl_pll_mgr = { > .dpll_info = icl_plls, > .get_dplls = icl_get_dplls, > .put_dplls = icl_put_dplls, > + .update_active_dpll = icl_update_active_dpll, > .dump_hw_state = icl_dump_hw_state, > }; > > @@ -3524,6 +3527,29 @@ void intel_release_shared_dplls(struct intel_atomic_state *state, > dpll_mgr->put_dplls(state, crtc); > } > > +/** > + * intel_update_active_dpll - update the active DPLL for a CRTC/encoder > + * @state: atomic state > + * @crtc: the CRTC for which to update the active DPLL > + * @encoder: encoder determining the type of port DPLL > + * > + * Update the active DPLL for the given @crtc/@encoder in @crtc's atomic state, > + * from the port DPLLs reserved previously by intel_reserve_shared_dplls(). The > + * DPLL selected will be based on the current mode of the encoder's port. > + */ > +void intel_update_active_dpll(struct intel_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder) > +{ > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; > + > + if (WARN_ON(!dpll_mgr)) > + return; > + > + dpll_mgr->update_active_dpll(state, crtc, encoder); > +} > + > /** > * intel_shared_dpll_dump_hw_state - write hw_state to dmesg > * @dev_priv: i915 drm device > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > index 579f2ceafba3..1668f8116908 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > @@ -346,6 +346,9 @@ void intel_release_shared_dplls(struct intel_atomic_state *state, > struct intel_crtc *crtc); > void icl_set_active_port_dpll(struct intel_crtc_state *crtc_state, > enum icl_port_dpll_id port_dpll_id); > +void intel_update_active_dpll(struct intel_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder); > void intel_prepare_shared_dpll(const struct intel_crtc_state *crtc_state); > void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state); > void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state); > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c > index 7190b57376d6..c8904911e841 100644 > --- a/drivers/gpu/drm/i915/display/intel_tc.c > +++ b/drivers/gpu/drm/i915/display/intel_tc.c > @@ -172,19 +172,12 @@ static bool icl_tc_phy_is_in_safe_mode(struct intel_digital_port *dig_port) > * display, USB, etc. As a result, handshaking through FIA is required around > * connect and disconnect to cleanly transfer ownership with the controller and > * set the type-C power state. > - * > - * We could opt to only do the connect flow when we actually try to use the AUX > - * channels or do a modeset, then immediately run the disconnect flow after > - * usage, but there are some implications on this for a dynamic environment: > - * things may go away or change behind our backs. So for now our driver is > - * always trying to acquire ownership of the controller as soon as it gets an > - * interrupt (or polls state and sees a port is connected) and only gives it > - * back when it sees a disconnect. Implementation of a more fine-grained model > - * will require a lot of coordination with user space and thorough testing for > - * the extra possible cases. > */ > -static void icl_tc_phy_connect(struct intel_digital_port *dig_port) > +static void icl_tc_phy_connect(struct intel_digital_port *dig_port, > + int required_lanes) > { > + int max_lanes; > + > if (!icl_tc_phy_status_complete(dig_port)) { > DRM_DEBUG_KMS("Port %s: PHY not ready\n", > dig_port->tc_port_name); > @@ -195,8 +188,9 @@ static void icl_tc_phy_connect(struct intel_digital_port *dig_port) > !WARN_ON(dig_port->tc_legacy_port)) > goto out_set_tbt_alt_mode; > > + max_lanes = intel_tc_port_fia_max_lane_count(dig_port); > if (dig_port->tc_legacy_port) { > - WARN_ON(intel_tc_port_fia_max_lane_count(dig_port) != 4); > + WARN_ON(max_lanes != 4); > dig_port->tc_mode = TC_PORT_LEGACY; > > return; > @@ -212,6 +206,13 @@ static void icl_tc_phy_connect(struct intel_digital_port *dig_port) > goto out_set_safe_mode; > } > > + if (max_lanes < required_lanes) { > + DRM_DEBUG_KMS("Port %s: PHY max lanes %d < required lanes %d\n", > + dig_port->tc_port_name, > + max_lanes, required_lanes); > + goto out_set_safe_mode; > + } > + > dig_port->tc_mode = TC_PORT_DP_ALT; > > return; > @@ -295,7 +296,8 @@ intel_tc_port_get_target_mode(struct intel_digital_port *dig_port) > TC_PORT_TBT_ALT; > } > > -static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port) > +static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port, > + int required_lanes) > { > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > enum tc_port_mode old_tc_mode = dig_port->tc_mode; > @@ -303,7 +305,7 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port) > intel_display_power_flush_work(dev_priv); > > icl_tc_phy_disconnect(dig_port); > - icl_tc_phy_connect(dig_port); > + icl_tc_phy_connect(dig_port, required_lanes); > > DRM_DEBUG_KMS("Port %s: TC port mode reset (%s -> %s)\n", > dig_port->tc_port_name, > @@ -311,6 +313,14 @@ static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port) > tc_port_mode_name(dig_port->tc_mode)); > } > > +static void > +intel_tc_port_link_init_refcount(struct intel_digital_port *dig_port, > + int refcount) > +{ > + WARN_ON(dig_port->tc_link_refcount); > + dig_port->tc_link_refcount = refcount; > +} > + > void intel_tc_port_sanitize(struct intel_digital_port *dig_port) > { > struct intel_encoder *encoder = &dig_port->base; > @@ -328,11 +338,13 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port) > if (!icl_tc_phy_is_connected(dig_port)) > DRM_DEBUG_KMS("Port %s: PHY disconnected with %d active link(s)\n", > dig_port->tc_port_name, active_links); > + intel_tc_port_link_init_refcount(dig_port, active_links); > + > goto out; > } > > if (dig_port->tc_legacy_port) > - icl_tc_phy_connect(dig_port); > + icl_tc_phy_connect(dig_port, 1); > > out: > DRM_DEBUG_KMS("Port %s: sanitize mode (%s)\n", > @@ -361,27 +373,60 @@ bool intel_tc_port_connected(struct intel_digital_port *dig_port) > { > bool is_connected; > > - mutex_lock(&dig_port->tc_lock); > - > - if (intel_tc_port_needs_reset(dig_port)) > - intel_tc_port_reset_mode(dig_port); > - > + intel_tc_port_lock(dig_port); > is_connected = tc_port_live_status_mask(dig_port) & > BIT(dig_port->tc_mode); > - > - mutex_unlock(&dig_port->tc_lock); > + intel_tc_port_unlock(dig_port); > > return is_connected; > } > > -void intel_tc_port_lock(struct intel_digital_port *dig_port) > +static void __intel_tc_port_lock(struct intel_digital_port *dig_port, > + int required_lanes) > { > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + intel_wakeref_t wakeref; > + > + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE); > + > mutex_lock(&dig_port->tc_lock); > - /* TODO: reset the TypeC port mode if needed */ > + > + if (!dig_port->tc_link_refcount && > + intel_tc_port_needs_reset(dig_port)) > + intel_tc_port_reset_mode(dig_port, required_lanes); > + > + WARN_ON(dig_port->tc_lock_wakeref); > + dig_port->tc_lock_wakeref = wakeref; > +} > + > +void intel_tc_port_lock(struct intel_digital_port *dig_port) > +{ > + __intel_tc_port_lock(dig_port, 1); > } > > void intel_tc_port_unlock(struct intel_digital_port *dig_port) > { > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + intel_wakeref_t wakeref = fetch_and_zero(&dig_port->tc_lock_wakeref); > + > + mutex_unlock(&dig_port->tc_lock); > + > + intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE, > + wakeref); > +} > + > +void intel_tc_port_get_link(struct intel_digital_port *dig_port, > + int required_lanes) > +{ > + __intel_tc_port_lock(dig_port, required_lanes); > + dig_port->tc_link_refcount++; > + intel_tc_port_unlock(dig_port); > +} > + > +void intel_tc_port_put_link(struct intel_digital_port *dig_port) > +{ > + mutex_lock(&dig_port->tc_lock); > + dig_port->tc_link_refcount--; > mutex_unlock(&dig_port->tc_lock); > } > > @@ -399,4 +444,5 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy) > > mutex_init(&dig_port->tc_lock); > dig_port->tc_legacy_port = is_legacy; > + dig_port->tc_link_refcount = 0; > } > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h > index b5af2fe60b22..31af7be96070 100644 > --- a/drivers/gpu/drm/i915/display/intel_tc.h > +++ b/drivers/gpu/drm/i915/display/intel_tc.h > @@ -19,6 +19,9 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port); > void intel_tc_port_sanitize(struct intel_digital_port *dig_port); > void intel_tc_port_lock(struct intel_digital_port *dig_port); > void intel_tc_port_unlock(struct intel_digital_port *dig_port); > +void intel_tc_port_get_link(struct intel_digital_port *dig_port, > + int required_lanes); > +void intel_tc_port_put_link(struct intel_digital_port *dig_port); > > void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 12a102e239c5..24c63ed45c6f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -115,6 +115,9 @@ struct intel_encoder { > int (*compute_config)(struct intel_encoder *, > struct intel_crtc_state *, > struct drm_connector_state *); > + void (*update_prepare)(struct intel_atomic_state *, > + struct intel_encoder *, > + struct intel_crtc *); > void (*pre_pll_enable)(struct intel_encoder *, > const struct intel_crtc_state *, > const struct drm_connector_state *); > @@ -124,6 +127,9 @@ struct intel_encoder { > void (*enable)(struct intel_encoder *, > const struct intel_crtc_state *, > const struct drm_connector_state *); > + void (*update_complete)(struct intel_atomic_state *, > + struct intel_encoder *, > + struct intel_crtc *); > void (*disable)(struct intel_encoder *, > const struct intel_crtc_state *, > const struct drm_connector_state *); > @@ -1234,6 +1240,8 @@ struct intel_digital_port { > enum aux_ch aux_ch; > enum intel_display_power_domain ddi_io_power_domain; > struct mutex tc_lock; /* protects the TypeC port mode */ > + intel_wakeref_t tc_lock_wakeref; > + int tc_link_refcount; > bool tc_legacy_port:1; > char tc_port_name[8]; > enum tc_port_mode tc_mode; > -- > 2.17.1 -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx