On Fri, Apr 17, 2020 at 03:23:20PM -0400, Lyude Paul wrote:
> On Fri, 2020-04-17 at 22:20 +0300, Ville Syrjälä wrote:
> > On Fri, Apr 17, 2020 at 02:50:39PM -0400, Lyude Paul wrote:
> > > On Fri, 2020-04-17 at 18:27 +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >
> > > > 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.
> >
> > Not sure what else we'd have to do but set connectors_changed=true on
> > all the relevant connectors and commit.
>
> Well that's all that needs to be done in this patch, since it only handles the
> case where we need to retrain the link at the same link rate as before. On
> hardware that ends up having a lower link rate then it reported though (I've
> unfortunately seen this happen with some DP concentrators + MST hubs), we'd
> also need to go through and recalculate the bandwidth requirements for each
> connector on the topology to figure out if they need to have their display
> configuration changed (and as such-we would set link_status=bad) or not in
> order to fit in the new bw constraints
Yeah, the whole MST bandwidth calculation is currently non-existent,
which is why I had to revert the deep color stuff for MST. So far my
simplistic plan for that (should I ever get to it), would be to just
cap the bpp across all streams, then keep looping the state computation
while reducing the bpp cap until everything fits. Not entirely optimal
in all cases but should work and should be pretty easy to implement.
And as far as training at a lower clock/lanes, we should have the driver
provide that information to the topology mgr. Currently it just assumes
it can use as much as the DPCD caps indicate. With that the reduction
stuff i915 already does for SST should just work (tm).
> >
> > > 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 <lyude@xxxxxxxxxx>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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
> --
> Cheers,
> Lyude Paul (she/her)
> Associate Software Engineer at Red Hat
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
