On Thu, May 04, 2023 at 08:41:49PM +0300, Ville Syrjälä wrote: > On Thu, May 04, 2023 at 02:10:48AM +0300, Imre Deak wrote: > > If the output on a DP-alt link with its sink disconnected is kept > > enabled for too long (about 20 sec), then some IOM/TCSS firmware timeout > > will cause havoc on the PCI bus, at least for other GFX devices on it > > which will stop powering up. Since user space is not guaranteed to do a > > disabling modeset in time, switch such disconnected but active links to > > TBT mode - which is without such shortcomings - with a 2 second delay. > > > > If the above condition is detected already during the driver load/system > > resume sanitization step disable the output instead, as at that point no > > user space or kernel client depends on a consistent output state yet and > > because subsequent atomic modeset on such connectors - without the > > actual sink capabilities available - can fail. > > > > An active/disconnected port as above will also block the HPD status of > > other active/disconnected ports to get updated (stuck in the connected > > state), until the former port is disabled, its PHY is disconnected and > > a ~10 ms delay has elapsed. This means the link state for all TypeC > > ports/CRTCs must be rechecked after a CRTC is disabled due to the above > > reason. For this disconnect the PHY synchronously after the CRTC/port is > > disabled and recheck all CRTCs for the above condition whenever such a > > port is disabled. > > > > To account for a race condition during driver loading where the sink is > > disconnected after the above sanitization step and before the HPD > > interrupts get enabled, do an explicit check/link reset if needed from > > the encoder's late_register hook, which is called after the HPD > > interrupts are enabled already. > > > > v2: > > - Handle an active/disconnected port blocking the HPD state update of > > another active/disconnected port. > > - Cancel the delayed work resetting the link also from the encoder > > enable/suspend/shutdown hooks. > > - Rebase on the earlier intel_modeset_lock_ctx_retry() addition, > > fixing here the missed atomic state reset in case of a retry. > > - Fix handling of an error return from intel_atomic_get_crtc_state(). > > - Recheck if the port needs to be reset after all the atomic state > > is locked and async commits are waited on. > > > > Cc: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> > > Tested-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5860 > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 68 +++++++++++++-- > > .../drm/i915/display/intel_display_types.h | 2 + > > drivers/gpu/drm/i915/display/intel_dp.c | 59 +++++++++++++ > > drivers/gpu/drm/i915/display/intel_dp.h | 1 + > > .../drm/i915/display/intel_modeset_setup.c | 83 ++++++++++++++----- > > drivers/gpu/drm/i915/display/intel_tc.c | 29 +++++++ > > drivers/gpu/drm/i915/display/intel_tc.h | 1 + > > 7 files changed, 215 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index eb391fff0f1be..db390b9c824ec 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -3313,6 +3313,8 @@ static void intel_disable_ddi(struct intel_atomic_state *state, > > const struct intel_crtc_state *old_crtc_state, > > const struct drm_connector_state *old_conn_state) > > { > > + cancel_delayed_work(&enc_to_dig_port(encoder)->reset_link_work); > > + > > intel_hdcp_disable(to_intel_connector(old_conn_state->connector)); > > > > if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI)) > > @@ -3381,6 +3383,8 @@ intel_ddi_pre_pll_enable(struct intel_atomic_state *state, > > enum phy phy = intel_port_to_phy(dev_priv, encoder->port); > > bool is_tc_port = intel_phy_is_tc(dev_priv, phy); > > > > + cancel_delayed_work(&dig_port->reset_link_work); > > + > > if (is_tc_port) { > > struct intel_crtc *master_crtc = > > to_intel_crtc(crtc_state->uapi.crtc); > > @@ -4229,9 +4233,53 @@ static void intel_ddi_encoder_reset(struct drm_encoder *encoder) > > intel_tc_port_init_mode(dig_port); > > } > > > > +static void intel_ddi_tc_port_reset_link_work(struct work_struct *work) > > +{ > > + struct intel_digital_port *dig_port = > > + container_of(work, struct intel_digital_port, reset_link_work.work); > > + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); > > + struct intel_encoder *encoder = &dig_port->base; > > + > > + mutex_lock(&i915->drm.mode_config.mutex); > > + > > + if (intel_tc_port_link_needs_reset(dig_port)) { > > + int ret; > > + > > + drm_dbg_kms(&i915->drm, > > + "[ENCODER:%d:%s] TypeC DP-alt sink disconnected, resetting link\n", > > + encoder->base.base.id, encoder->base.name); > > + ret = intel_dp_reset_link(encoder); > > + drm_WARN_ON(&i915->drm, ret); > > + } > > + > > + mutex_unlock(&i915->drm.mode_config.mutex); > > +} > > + > > +static bool intel_ddi_tc_port_reset_link(struct intel_encoder *encoder) > > +{ > > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > > + > > + if (!intel_tc_port_link_needs_reset(dig_port)) > > + return false; > > + > > + queue_delayed_work(system_unbound_wq, &dig_port->reset_link_work, msecs_to_jiffies(2000)); > > + > > + return true; > > +} > > + > > +static int intel_ddi_encoder_late_register(struct drm_encoder *_encoder) > > +{ > > + struct intel_encoder *encoder = to_intel_encoder(_encoder); > > + > > + intel_ddi_tc_port_reset_link(encoder); > > + > > + return 0; > > +} > > + > > static const struct drm_encoder_funcs intel_ddi_funcs = { > > .reset = intel_ddi_encoder_reset, > > .destroy = intel_ddi_encoder_destroy, > > + .late_register = intel_ddi_encoder_late_register, > > }; > > > > static struct intel_connector * > > @@ -4401,14 +4449,16 @@ intel_ddi_hotplug(struct intel_encoder *encoder, > > > > state = intel_encoder_hotplug(encoder, connector); > > > > - intel_modeset_lock_ctx_retry(&ctx, NULL, 0, ret) { > > - if (connector->base.connector_type == DRM_MODE_CONNECTOR_HDMIA) > > - ret = intel_hdmi_reset_link(encoder, &ctx); > > - else > > - ret = intel_dp_retrain_link(encoder, &ctx); > > - } > > + if (!intel_ddi_tc_port_reset_link(encoder)) { > > + intel_modeset_lock_ctx_retry(&ctx, NULL, 0, ret) { > > + if (connector->base.connector_type == DRM_MODE_CONNECTOR_HDMIA) > > + ret = intel_hdmi_reset_link(encoder, &ctx); > > + else > > + ret = intel_dp_retrain_link(encoder, &ctx); > > + } > > > > - drm_WARN_ON(encoder->base.dev, ret); > > + drm_WARN_ON(encoder->base.dev, ret); > > + } > > > > /* > > * Unpowered type-c dongles can take some time to boot and be > > @@ -4624,6 +4674,7 @@ static void intel_ddi_encoder_suspend(struct intel_encoder *encoder) > > if (!intel_phy_is_tc(i915, phy)) > > return; > > > > + cancel_delayed_work(&dig_port->reset_link_work); > > intel_tc_port_flush_work(dig_port); > > } > > > > @@ -4640,6 +4691,7 @@ static void intel_ddi_encoder_shutdown(struct intel_encoder *encoder) > > if (!intel_phy_is_tc(i915, phy)) > > return; > > > > + cancel_delayed_work(&dig_port->reset_link_work); > > intel_tc_port_cleanup(dig_port); > > } > > > > @@ -4945,6 +4997,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > > encoder->pipe_mask = intel_ddi_splitter_pipe_mask(dev_priv); > > } > > > > + INIT_DELAYED_WORK(&dig_port->reset_link_work, intel_ddi_tc_port_reset_link_work); > > + > > /* > > * In theory we don't need the encoder->type check, > > * but leave it just in case we have some really bad VBTs... > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 96a3183675bee..b4d5424cf95cf 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1817,6 +1817,8 @@ struct intel_digital_port { > > > > struct intel_tc_port *tc; > > > > + struct delayed_work reset_link_work; > > + > > /* protects num_hdcp_streams reference count, hdcp_port_data and hdcp_auth_status */ > > struct mutex hdcp_mutex; > > /* the number of pipes using HDCP signalling out of this port */ > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 7a349cb9fc2e6..e7e4266b314a7 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -70,6 +70,7 @@ > > #include "intel_hotplug.h" > > #include "intel_lspcon.h" > > #include "intel_lvds.h" > > +#include "intel_modeset_lock.h" > > #include "intel_panel.h" > > #include "intel_pch_display.h" > > #include "intel_pps.h" > > @@ -4258,6 +4259,64 @@ int intel_dp_retrain_link(struct intel_encoder *encoder, > > return 0; > > } > > > > +int intel_dp_reset_link(struct intel_encoder *encoder) > > +{ > > + struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > > + struct drm_modeset_acquire_ctx ctx; > > + struct drm_atomic_state *_state; > > + struct intel_atomic_state *state; > > + int ret = 0; > > + > > + _state = drm_atomic_state_alloc(&i915->drm); > > + if (!_state) > > + return -ENOMEM; > > + > > + state = to_intel_atomic_state(_state); > > + > > + intel_modeset_lock_ctx_retry(&ctx, state, 0, ret) { > > + struct intel_crtc *crtc; > > + u8 pipe_mask; > > + > > + ret = drm_modeset_lock(&i915->drm.mode_config.connection_mutex, &ctx); > > + if (ret) > > + continue; > > + > > + ret = intel_dp_get_active_pipes(enc_to_intel_dp(encoder), &ctx, &pipe_mask); > > + if (ret) > > + continue; > > + > > + if (!pipe_mask) > > + continue; > > + > > + state->internal = true; > > Can be set immediately after the state is allocated, which is what > everyone else does. Ok, will move it there. > > + > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) { > > + struct intel_crtc_state *crtc_state; > > + > > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > > + if (IS_ERR(crtc_state)) { > > + ret = PTR_ERR(crtc_state); > > + break; > > + } > > + > > + crtc_state->uapi.connectors_changed = true; > > + } > > + > > + if (ret) > > + continue; > > + > > + if (!intel_tc_port_link_needs_reset(dig_port)) > > + continue; > > + > > + ret = drm_atomic_commit(&state->base); > > + } > > + > > + drm_atomic_state_put(&state->base); > > + > > + return ret; > > +} > > + > > static int intel_dp_prep_phy_test(struct intel_dp *intel_dp, > > struct drm_modeset_acquire_ctx *ctx, > > u8 *pipe_mask) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h > > index ca12a1733df6f..05a8bda8513ee 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.h > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h > > @@ -45,6 +45,7 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > bool intel_dp_is_connected(struct intel_dp *intel_dp); > > int intel_dp_retrain_link(struct intel_encoder *encoder, > > struct drm_modeset_acquire_ctx *ctx); > > +int intel_dp_reset_link(struct intel_encoder *encoder); > > void intel_dp_set_power(struct intel_dp *intel_dp, u8 mode); > > void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp, > > const struct intel_crtc_state *crtc_state); > > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c > > index 1e10580e5ab31..311249a65f8fb 100644 > > --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c > > +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c > > @@ -26,6 +26,7 @@ > > #include "intel_fifo_underrun.h" > > #include "intel_modeset_setup.h" > > #include "intel_pch_display.h" > > +#include "intel_tc.h" > > #include "intel_vblank.h" > > #include "intel_wm.h" > > #include "skl_watermark.h" > > @@ -368,17 +369,6 @@ intel_sanitize_plane_mapping(struct drm_i915_private *i915) > > } > > } > > > > -static bool intel_crtc_has_encoders(struct intel_crtc *crtc) > > -{ > > - struct drm_device *dev = crtc->base.dev; > > - struct intel_encoder *encoder; > > - > > - for_each_encoder_on_crtc(dev, &crtc->base, encoder) > > - return true; > > - > > - return false; > > -} > > Not really seeing why this is removed. > > > - > > static struct intel_connector *intel_encoder_find_connector(struct intel_encoder *encoder) > > { > > struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > @@ -421,11 +411,14 @@ static void intel_sanitize_fifo_underrun_reporting(const struct intel_crtc_state > > !HAS_GMCH(i915)); > > } > > > > -static void intel_sanitize_crtc(struct intel_crtc *crtc, > > +static bool intel_sanitize_crtc(struct intel_crtc *crtc, > > struct drm_modeset_acquire_ctx *ctx) > > { > > struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state); > > + struct intel_encoder *encoder; > > + bool needs_link_reset = false; > > + bool has_encoder = false; > > > > if (crtc_state->hw.active) { > > struct intel_plane *plane; > > @@ -445,13 +438,67 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc, > > intel_color_commit_arm(crtc_state); > > } > > > > + if (!crtc_state->hw.active || > > + intel_crtc_is_bigjoiner_slave(crtc_state)) > > + return false; > > + > > + for_each_encoder_on_crtc(&i915->drm, &crtc->base, encoder) { > > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > > + > > + has_encoder = true; > > + > > + if (!dig_port || !intel_tc_port_link_needs_reset(dig_port)) > > + continue; > > + > > + needs_link_reset = true; > > + break; > > + } > > Hmm. I guess you wanted to combine the loops. Feels like a > rather pointless micro-optimizaiton. So containing this stuff > into sepraate function(s) could be more readable. Although the > fact that you need the needs_link_reset check after the disable > means you still need at least one boolean variable. Yes, just to iterate the encoders only once, but having separate functions makes this more readable, will do that. > > > + > > /* > > * Adjust the state of the output pipe according to whether we have > > * active connectors/encoders. > > */ > > - if (crtc_state->hw.active && !intel_crtc_has_encoders(crtc) && > > - !intel_crtc_is_bigjoiner_slave(crtc_state)) > > + if (!has_encoder || needs_link_reset) > > intel_crtc_disable_noatomic(crtc, ctx); > > + > > + /* > > + * An active and disconnected TypeC port prevents the HPD live state > > + * to get updated on other active/disconnected TypeC ports, so after > > + * a port gets disabled the CRTCs using other TypeC ports must be > > + * rechecked wrt. their link status. After the port is disabled the > > + * HPD state on other ports will get updated in ~10ms, so also wait > > + * here for that. > > + */ > > + if (needs_link_reset) { > > + usleep_range(50000, 100000); I realized this slack is more than required, will reduce it to max 30ms. > > + > > + return true; > > + } > > + > > + return false; > > +} > > + > > +static void intel_sanitize_all_crtcs(struct drm_i915_private *i915, > > + struct drm_modeset_acquire_ctx *ctx) > > +{ > > + int restart_count; > > + > > + for (restart_count = 0; restart_count < I915_MAX_PIPES; restart_count++) { > > + struct intel_crtc *crtc; > > + bool restart = false; > > + > > + for_each_intel_crtc(&i915->drm, crtc) { > > + struct intel_crtc_state *crtc_state = > > + to_intel_crtc_state(crtc->base.state); > > + > > + restart = restart || intel_sanitize_crtc(crtc, ctx); > > + intel_crtc_state_dump(crtc_state, NULL, "setup_hw_state"); > > So this will dump the state for all imtermediate/final states but not > necessarily the initial state? I think we should either just dump the > final states (which was the case previously), or perhaps dump both the > initial and final states but not any intermediate stuff. I meant to keep state dumping as-is from the final state, but botched it, will fix this. > Also this whole restart_count loop is confusing me. I presume it's just > trying to make sure we don't disable the same crtc multiple times or > something? Yes, just to prevent an endless loop due to some bug. > Seems a bit unlikely to me that we'd screw things up so badly, so > maybe just rid of it, or maybe just use a bitmask or something to > track the progress? Ok, tracking which crtcs got disabled makes it clearer, can do that. > > > + } > > + > > + if (!restart) > > + break; > > + } > > + drm_WARN_ON(&i915->drm, restart_count == I915_MAX_PIPES); > > } > > > > static bool has_bogus_dpll_config(const struct intel_crtc_state *crtc_state) > > @@ -871,13 +918,7 @@ void intel_modeset_setup_hw_state(struct drm_i915_private *i915, > > */ > > intel_modeset_update_connector_atomic_state(i915); > > > > - for_each_intel_crtc(&i915->drm, crtc) { > > - struct intel_crtc_state *crtc_state = > > - to_intel_crtc_state(crtc->base.state); > > - > > - intel_sanitize_crtc(crtc, ctx); > > - intel_crtc_state_dump(crtc_state, NULL, "setup_hw_state"); > > - } > > + intel_sanitize_all_crtcs(i915, ctx); > > > > intel_dpll_sanitize_state(i915); > > > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c > > index 4fca711a58bce..8b7f57701c257 100644 > > --- a/drivers/gpu/drm/i915/display/intel_tc.c > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c > > @@ -1572,6 +1572,27 @@ bool intel_tc_port_connected(struct intel_encoder *encoder) > > return is_connected; > > } > > > > +bool intel_tc_port_link_needs_reset(struct intel_digital_port *dig_port) > > +{ > > + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); > > + enum phy phy = intel_port_to_phy(i915, dig_port->base.port); > > + struct intel_tc_port *tc = to_tc_port(dig_port); > > + bool ret; > > + > > + if (!intel_phy_is_tc(i915, phy)) > > + return false; > > + > > + mutex_lock(&tc->lock); > > + > > + ret = tc->link_refcount && > > + intel_tc_port_in_dp_alt_mode(dig_port) && > > + intel_tc_port_needs_reset(tc); > > + > > + mutex_unlock(&tc->lock); > > + > > + return ret; > > +} > > + > > static void __intel_tc_port_lock(struct intel_tc_port *tc, > > int required_lanes) > > { > > @@ -1660,6 +1681,14 @@ void intel_tc_port_put_link(struct intel_digital_port *dig_port) > > intel_tc_port_lock(dig_port); > > __intel_tc_port_put_link(tc); > > intel_tc_port_unlock(dig_port); > > + > > + /* > > + * The firmware will not update the HPD status of other TypeC ports > > + * that are active in DP-alt mode with their sink disconnected, until > > + * this port is disabled and its PHY gets disconnected. Make sure this > > + * happens in a timely manner by disconnecting the PHY synchronously. > > + */ > > + intel_tc_port_flush_work(dig_port); > > } > > > > int intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy) > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h > > index dd0810f9ea95e..c4cf1eac54a1c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_tc.h > > +++ b/drivers/gpu/drm/i915/display/intel_tc.h > > @@ -34,6 +34,7 @@ void intel_tc_port_flush_work(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); > > +bool intel_tc_port_link_needs_reset(struct intel_digital_port *dig_port); > > bool intel_tc_port_ref_held(struct intel_digital_port *dig_port); > > > > int intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy); > > -- > > 2.37.2 > > -- > Ville Syrjälä > Intel
