Re: [PATCH 2/2] drm/i915: Push MST link retraining to the hotplug work

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux