On Fri, 2020-04-17 at 18:27 +0300, Ville Syrjala wrote: > From: Ville Syrjälä <[email protected]> > > We shouldn't try to do link retraining from the short hpd handler. > We can't take any modeset locks there so this is racy as hell. > Push the whole thing into the hotplug work like we do with SST. > > We'll just have to adjust the SST retraining code to deal with > the MST encoders and multiple pipes. > > TODO: I have a feeling we should just rip this all out and > do a full modeset instead. Stuff like port sync and the tgl+ > MST master transcoder stuff maybe doesn't work well if we > try to retrain without following the proper modeset sequence. > So far haven't done any actual tests to confirm that though.
To answer your feeling here: yes-we should, I have some branches for doing this sort of thing with i915 but they are ancient at this point. Once I get to fallback link retraining we should be able to use this in i915 to handle figuring out what all needs to be reset for an MST training. fwiw - If you have some need for fallback link retraining soon I can move it up on my todo list for MST. I've got the hard design parts already figured out from the last time I tried implementing it, so writing new patches shouldn't be too difficult. (note - this patch is still worth merging I'd imagine, since this looks like it should at least handle retraining an MST topology at the same link rate just fine) > > Cc: Lyude Paul <[email protected]> > Signed-off-by: Ville Syrjälä <[email protected]> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 165 ++++++++++++++++++------ > 1 file changed, 122 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 4d4898db38e9..556371338aa9 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -5628,6 +5628,7 @@ static int > intel_dp_check_mst_status(struct intel_dp *intel_dp) > { > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + bool need_retrain = false; > > if (!intel_dp->is_mst) > return -EINVAL; > @@ -5647,16 +5648,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) > } > > /* check link status - esi[10] = 0x200c */ > - /* > - * FIXME kill this and use the SST retraining approach > - * for MST as well. > - */ > - if (intel_dp->active_mst_links > 0 && > + if (intel_dp->active_mst_links > 0 && !need_retrain && > !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > drm_dbg_kms(&i915->drm, > "channel EQ not ok, retraining\n"); > - intel_dp_start_link_train(intel_dp); > - intel_dp_stop_link_train(intel_dp); > + need_retrain = true; > } > > drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi); > @@ -5676,7 +5672,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) > } > } > > - return 0; > + return need_retrain; > } > > static bool > @@ -5713,20 +5709,102 @@ intel_dp_needs_link_retrain(struct intel_dp > *intel_dp) > return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count); > } > > +static bool intel_dp_has_connector(struct intel_dp *intel_dp, > + const struct drm_connector_state > *conn_state) > +{ > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + struct intel_encoder *encoder; > + enum pipe pipe; > + > + if (!conn_state->best_encoder) > + return false; > + > + /* SST */ > + encoder = &dp_to_dig_port(intel_dp)->base; > + if (conn_state->best_encoder == &encoder->base) > + return true; > + > + /* MST */ > + for_each_pipe(i915, pipe) { > + encoder = &intel_dp->mst_encoders[pipe]->base; > + if (conn_state->best_encoder == &encoder->base) > + return true; > + } > + > + return false; > +} > + > +static int intel_dp_prep_link_retrain(struct intel_dp *intel_dp, > + struct drm_modeset_acquire_ctx *ctx, > + u32 *crtc_mask) > +{ > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > + struct drm_connector_list_iter conn_iter; > + struct intel_connector *connector; > + int ret = 0; > + > + *crtc_mask = 0; > + > + if (!intel_dp_needs_link_retrain(intel_dp)) > + return 0; > + > + drm_connector_list_iter_begin(&i915->drm, &conn_iter); > + for_each_intel_connector_iter(connector, &conn_iter) { > + struct drm_connector_state *conn_state = > + connector->base.state; > + struct intel_crtc_state *crtc_state; > + struct intel_crtc *crtc; > + > + if (!intel_dp_has_connector(intel_dp, conn_state)) > + continue; > + > + crtc = to_intel_crtc(conn_state->crtc); > + if (!crtc) > + continue; > + > + ret = drm_modeset_lock(&crtc->base.mutex, ctx); > + if (ret) > + break; > + > + crtc_state = to_intel_crtc_state(crtc->base.state); > + > + drm_WARN_ON(&i915->drm, > !intel_crtc_has_dp_encoder(crtc_state)); > + > + if (!crtc_state->hw.active) > + continue; > + > + if (conn_state->commit && > + !try_wait_for_completion(&conn_state->commit->hw_done)) > + continue; > + > + *crtc_mask |= drm_crtc_mask(&crtc->base); > + } > + drm_connector_list_iter_end(&conn_iter); > + > + if (!intel_dp_needs_link_retrain(intel_dp)) > + *crtc_mask = 0; Also fwiw ^ this is the kind of logic I was thinking for the MST helpers (e.g. having helpers (+ setting link_status for each affected connector, etc. et.). again though-it's fine if this stays in i915 for now, but we should move it in the future. > + > + return ret; > +} > + > +static bool intel_dp_is_connected(struct intel_dp *intel_dp) > +{ > + struct intel_connector *connector = intel_dp->attached_connector; > + > + return connector->base.status == connector_status_connected || > + intel_dp->is_mst; > +} > + > int intel_dp_retrain_link(struct intel_encoder *encoder, > struct drm_modeset_acquire_ctx *ctx) > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > - struct intel_connector *connector = intel_dp->attached_connector; > - struct drm_connector_state *conn_state; > - struct intel_crtc_state *crtc_state; > struct intel_crtc *crtc; > + u32 crtc_mask; > int ret; > > - /* FIXME handle the MST connectors as well */ > - > - if (!connector || connector->base.status != > connector_status_connected) > + if (!intel_dp_is_connected(intel_dp)) > return 0; > > ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, > @@ -5734,46 +5812,42 @@ int intel_dp_retrain_link(struct intel_encoder > *encoder, > if (ret) > return ret; > > - conn_state = connector->base.state; > - > - crtc = to_intel_crtc(conn_state->crtc); > - if (!crtc) > - return 0; > - > - ret = drm_modeset_lock(&crtc->base.mutex, ctx); > + ret = intel_dp_prep_link_retrain(intel_dp, ctx, &crtc_mask); > if (ret) > return ret; > > - crtc_state = to_intel_crtc_state(crtc->base.state); > - > - drm_WARN_ON(&dev_priv->drm, !intel_crtc_has_dp_encoder(crtc_state)); > - > - if (!crtc_state->hw.active) > + if (crtc_mask == 0) > return 0; > > - if (conn_state->commit && > - !try_wait_for_completion(&conn_state->commit->hw_done)) > - return 0; > + drm_dbg_kms(&dev_priv->drm, "[ENCODER:%d:%s] retraining link\n", > + encoder->base.base.id, encoder->base.name); > > - if (!intel_dp_needs_link_retrain(intel_dp)) > - return 0; > + for_each_intel_crtc_mask(&dev_priv->drm, crtc, crtc_mask) { > + const struct intel_crtc_state *crtc_state = > + to_intel_crtc_state(crtc->base.state); > > - /* Suppress underruns caused by re-training */ > - intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false); > - if (crtc_state->has_pch_encoder) > - intel_set_pch_fifo_underrun_reporting(dev_priv, > - intel_crtc_pch_transcode > r(crtc), false); > + /* Suppress underruns caused by re-training */ > + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, > false); > + if (crtc_state->has_pch_encoder) > + intel_set_pch_fifo_underrun_reporting(dev_priv, > + intel_crtc_pch_t > ranscoder(crtc), false); > + } > > intel_dp_start_link_train(intel_dp); > intel_dp_stop_link_train(intel_dp); > > - /* Keep underrun reporting disabled until things are stable */ > - intel_wait_for_vblank(dev_priv, crtc->pipe); > + for_each_intel_crtc_mask(&dev_priv->drm, crtc, crtc_mask) { > + const struct intel_crtc_state *crtc_state = > + to_intel_crtc_state(crtc->base.state); > > - intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); > - if (crtc_state->has_pch_encoder) > - intel_set_pch_fifo_underrun_reporting(dev_priv, > - intel_crtc_pch_transcode > r(crtc), true); > + /* Keep underrun reporting disabled until things are stable */ > + intel_wait_for_vblank(dev_priv, crtc->pipe); > + > + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, > true); > + if (crtc_state->has_pch_encoder) > + intel_set_pch_fifo_underrun_reporting(dev_priv, > + intel_crtc_pch_t > ranscoder(crtc), true); > + } > > return 0; > } > @@ -7415,7 +7489,8 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > } > > if (intel_dp->is_mst) { > - if (intel_dp_check_mst_status(intel_dp) == -EINVAL) { > + switch (intel_dp_check_mst_status(intel_dp)) { > + case -EINVAL: > /* > * If we were in MST mode, and device is not > * there, get out of MST mode > @@ -7429,6 +7504,10 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > intel_dp->is_mst); > > return IRQ_NONE; > + case 1: > + return IRQ_NONE; > + default: > + break; > } > } > -- Cheers, Lyude Paul (she/her) Associate Software Engineer at Red Hat _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
