[Intel-gfx] [PATCH 1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit

[Intel-gfx] [PATCH 1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make intel_dp_check_mst_status() somewhat legible by humans.

Note that the return value of drm_dp_mst_hpd_irq() is always
either 0 or -ENOMEM, and we never did anything with the latter
so we can just ignore the whole thing.

We can also get rid of the direct drm_dp_mst_topology_mgr_set_mst(false)
call since returning -EINVAL causes the caller to do the very same call
for us.

Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 88 ++++++++++++-------------
 1 file changed, 41 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 48397b2c08cf..4d4898db38e9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5628,61 +5628,55 @@ static int
 intel_dp_check_mst_status(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-	bool bret;
 
-	if (intel_dp->is_mst) {
-		u8 esi[DP_DPRX_ESI_LEN] = { 0 };
-		int ret = 0;
+	if (!intel_dp->is_mst)
+		return -EINVAL;
+
+	WARN_ON_ONCE(intel_dp->active_mst_links < 0);
+
+	for (;;) {
+		u8 esi[DP_DPRX_ESI_LEN] = {};
+		bool bret, handled;
 		int retry;
-		bool handled;
 
-		WARN_ON_ONCE(intel_dp->active_mst_links < 0);
 		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
-go_again:
-		if (bret == true) {
-
-			/* check link status - esi[10] = 0x200c */
-			if (intel_dp->active_mst_links > 0 &&
-			    !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);
-			}
-
-			drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi);
-			ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
-
-			if (handled) {
-				for (retry = 0; retry < 3; retry++) {
-					int wret;
-					wret = drm_dp_dpcd_write(&intel_dp->aux,
-								 DP_SINK_COUNT_ESI+1,
-								 &esi[1], 3);
-					if (wret == 3) {
-						break;
-					}
-				}
-
-				bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
-				if (bret == true) {
-					drm_dbg_kms(&i915->drm,
-						    "got esi2 %3ph\n", esi);
-					goto go_again;
-				}
-			} else
-				ret = 0;
-
-			return ret;
-		} else {
+		if (!bret) {
 			drm_dbg_kms(&i915->drm,
 				    "failed to get ESI - device may have failed\n");
-			intel_dp->is_mst = false;
-			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
-							intel_dp->is_mst);
+			return -EINVAL;
+		}
+
+		/* 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 &&
+		    !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);
+		}
+
+		drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi);
+
+		drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
+		if (!handled)
+			break;
+
+		for (retry = 0; retry < 3; retry++) {
+			int wret;
+
+			wret = drm_dp_dpcd_write(&intel_dp->aux,
+						 DP_SINK_COUNT_ESI+1,
+						 &esi[1], 3);
+			if (wret == 3)
+				break;
 		}
 	}
-	return -EINVAL;
+
+	return 0;
 }
 
 static bool
-- 
2.24.1

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

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

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

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.

Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 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;
+
+	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_transcoder(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_transcoder(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_transcoder(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_transcoder(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;
 		}
 	}
 
-- 
2.24.1

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

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

[Lyude Paul]

On Fri, 2020-04-17 at 18:27 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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@redhat.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit

[Lyude Paul]

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Fri, 2020-04-17 at 18:27 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Make intel_dp_check_mst_status() somewhat legible by humans.
> 
> Note that the return value of drm_dp_mst_hpd_irq() is always
> either 0 or -ENOMEM, and we never did anything with the latter
> so we can just ignore the whole thing.
> 
> We can also get rid of the direct drm_dp_mst_topology_mgr_set_mst(false)
> call since returning -EINVAL causes the caller to do the very same call
> for us.
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 88 ++++++++++++-------------
>  1 file changed, 41 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 48397b2c08cf..4d4898db38e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5628,61 +5628,55 @@ static int
>  intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	bool bret;
>  
> -	if (intel_dp->is_mst) {
> -		u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> -		int ret = 0;
> +	if (!intel_dp->is_mst)
> +		return -EINVAL;
> +
> +	WARN_ON_ONCE(intel_dp->active_mst_links < 0);
> +
> +	for (;;) {
> +		u8 esi[DP_DPRX_ESI_LEN] = {};
> +		bool bret, handled;
>  		int retry;
> -		bool handled;
>  
> -		WARN_ON_ONCE(intel_dp->active_mst_links < 0);
>  		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> -go_again:
> -		if (bret == true) {
> -
> -			/* check link status - esi[10] = 0x200c */
> -			if (intel_dp->active_mst_links > 0 &&
> -			    !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);
> -			}
> -
> -			drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi);
> -			ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi,
> &handled);
> -
> -			if (handled) {
> -				for (retry = 0; retry < 3; retry++) {
> -					int wret;
> -					wret = drm_dp_dpcd_write(&intel_dp-
> >aux,
> -								 DP_SINK_COUNT
> _ESI+1,
> -								 &esi[1], 3);
> -					if (wret == 3) {
> -						break;
> -					}
> -				}
> -
> -				bret = intel_dp_get_sink_irq_esi(intel_dp,
> esi);
> -				if (bret == true) {
> -					drm_dbg_kms(&i915->drm,
> -						    "got esi2 %3ph\n", esi);
> -					goto go_again;
> -				}
> -			} else
> -				ret = 0;
> -
> -			return ret;
> -		} else {
> +		if (!bret) {
>  			drm_dbg_kms(&i915->drm,
>  				    "failed to get ESI - device may have
> failed\n");
> -			intel_dp->is_mst = false;
> -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> -							intel_dp->is_mst);
> +			return -EINVAL;
> +		}
> +
> +		/* 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 &&
> +		    !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);
> +		}
> +
> +		drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi);
> +
> +		drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> +		if (!handled)
> +			break;
> +
> +		for (retry = 0; retry < 3; retry++) {
> +			int wret;
> +
> +			wret = drm_dp_dpcd_write(&intel_dp->aux,
> +						 DP_SINK_COUNT_ESI+1,
> +						 &esi[1], 3);
> +			if (wret == 3)
> +				break;
>  		}
>  	}
> -	return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static bool
> -- 
> 2.24.1
> 
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat

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

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

[Lyude Paul]

...completely forgot to actually put my r-b here as well, oops

Reviewed-by: Lyude Paul <lyude@redhat.com>


On Fri, 2020-04-17 at 14:50 -0400, Lyude Paul wrote:
> On Fri, 2020-04-17 at 18:27 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > 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@redhat.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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@linux.intel.com>
> > 
> > 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.

> 
> 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@redhat.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Lyude Paul]

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@linux.intel.com>
> > > 
> > > 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
> 
> > 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@redhat.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  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

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

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

[Ville Syrjälä]

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@linux.intel.com>
> > > > 
> > > > 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@redhat.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit
URL   : https://patchwork.freedesktop.org/series/76105/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
d7fc81461f35 drm/i915: Flatten intel_dp_check_mst_status() a bit
-:117: CHECK:SPACING: spaces preferred around that '+' (ctx:VxV)
#117: FILE: drivers/gpu/drm/i915/display/intel_dp.c:5669:
+						 DP_SINK_COUNT_ESI+1,
 						                  ^

total: 0 errors, 0 warnings, 1 checks, 101 lines checked
bdd0a32afab1 drm/i915: Push MST link retraining to the hotplug work
-:219: WARNING:LONG_LINE: line over 100 characters
#219: FILE: drivers/gpu/drm/i915/display/intel_dp.c:5830:
+							      intel_crtc_pch_transcoder(crtc), false);

-:241: WARNING:LONG_LINE: line over 100 characters
#241: FILE: drivers/gpu/drm/i915/display/intel_dp.c:5846:
+							      intel_crtc_pch_transcoder(crtc), true);

total: 0 errors, 2 warnings, 0 checks, 230 lines checked

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

[Intel-gfx] ✗ Fi.CI.DOCS: warning for series starting with [1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit
URL   : https://patchwork.freedesktop.org/series/76105/
State : warning

== Summary ==

$ make htmldocs 2>&1 > /dev/null | grep i915
/home/cidrm/kernel/Documentation/gpu/i915.rst:610: WARNING: duplicate label gpu/i915:layout, other instance in /home/cidrm/kernel/Documentation/gpu/i915.rst

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

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit
URL   : https://patchwork.freedesktop.org/series/76105/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8322 -> Patchwork_17356
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/index.html


Changes
-------

  No changes found


Participating hosts (51 -> 46)
------------------------------

  Additional (1): fi-cml-u2 
  Missing    (6): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8322 -> Patchwork_17356

  CI-20190529: 20190529
  CI_DRM_8322: fd447e6b1ee13e6a9731bddc7694552640e8a01e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5599: cdb07101dda33e2fcb0f4c2aa199c47159d88f35 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17356: bdd0a32afab1c4234d6476e1e86ea4c0ef73a2f4 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

bdd0a32afab1 drm/i915: Push MST link retraining to the hotplug work
d7fc81461f35 drm/i915: Flatten intel_dp_check_mst_status() a bit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit
URL   : https://patchwork.freedesktop.org/series/76105/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8322_full -> Patchwork_17356_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_17356_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@requests:
    - shard-tglb:         [PASS][1] -> [INCOMPLETE][2] ([i915#1531] / [i915#1658])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-tglb5/igt@i915_selftest@live@requests.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-tglb8/igt@i915_selftest@live@requests.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x42-random:
    - shard-kbl:          [PASS][3] -> [FAIL][4] ([i915#54] / [i915#93] / [i915#95])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-128x42-random.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-kbl7/igt@kms_cursor_crc@pipe-a-cursor-128x42-random.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [PASS][5] -> [SKIP][6] ([fdo#109349])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-iclb6/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-kbl:          [PASS][7] -> [INCOMPLETE][8] ([i915#155])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-kbl7/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-kbl1/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-render:
    - shard-tglb:         [PASS][9] -> [SKIP][10] ([i915#668]) +6 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-tglb1/igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-render.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-tglb6/igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-render.html

  * igt@kms_mmap_write_crc@main:
    - shard-kbl:          [PASS][11] -> [FAIL][12] ([i915#93] / [i915#95])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-kbl4/igt@kms_mmap_write_crc@main.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-kbl1/igt@kms_mmap_write_crc@main.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - shard-skl:          [PASS][13] -> [INCOMPLETE][14] ([i915#69])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-skl3/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-skl9/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-iclb:         [PASS][15] -> [INCOMPLETE][16] ([i915#1185] / [i915#250])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-iclb7/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-iclb3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][17] -> [FAIL][18] ([fdo#108145] / [i915#265])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-skl8/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-apl:          [PASS][19] -> [FAIL][20] ([i915#95])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-apl3/igt@kms_plane_multiple@atomic-pipe-a-tiling-y.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-apl1/igt@kms_plane_multiple@atomic-pipe-a-tiling-y.html

  * igt@kms_psr@psr2_cursor_render:
    - shard-iclb:         [PASS][21] -> [SKIP][22] ([fdo#109441]) +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-iclb2/igt@kms_psr@psr2_cursor_render.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-iclb4/igt@kms_psr@psr2_cursor_render.html

  * igt@kms_setmode@basic:
    - shard-skl:          [PASS][23] -> [FAIL][24] ([i915#31])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-skl7/igt@kms_setmode@basic.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-skl6/igt@kms_setmode@basic.html
    - shard-kbl:          [PASS][25] -> [FAIL][26] ([i915#31])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-kbl6/igt@kms_setmode@basic.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-kbl7/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-apl:          [PASS][27] -> [DMESG-WARN][28] ([i915#180]) +2 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-apl1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-apl2/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  
#### Possible fixes ####

  * igt@gen9_exec_parse@allowed-all:
    - shard-glk:          [DMESG-WARN][29] ([i915#716]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-glk8/igt@gen9_exec_parse@allowed-all.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-glk6/igt@gen9_exec_parse@allowed-all.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x42-offscreen:
    - shard-apl:          [FAIL][31] ([i915#54] / [i915#95]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-apl6/igt@kms_cursor_crc@pipe-a-cursor-128x42-offscreen.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-apl3/igt@kms_cursor_crc@pipe-a-cursor-128x42-offscreen.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-skl:          [INCOMPLETE][33] ([i915#300]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-skl1/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-skl5/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_draw_crc@draw-method-rgb565-render-ytiled:
    - shard-glk:          [FAIL][35] ([i915#52] / [i915#54]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-glk6/igt@kms_draw_crc@draw-method-rgb565-render-ytiled.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-glk8/igt@kms_draw_crc@draw-method-rgb565-render-ytiled.html

  * {igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1}:
    - shard-skl:          [FAIL][37] ([i915#79]) -> [PASS][38] +1 similar issue
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-skl8/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-skl1/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1.html

  * {igt@kms_flip@flip-vs-suspend-interruptible@c-dp1}:
    - shard-apl:          [DMESG-WARN][39] ([i915#180]) -> [PASS][40] +4 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-apl3/igt@kms_flip@flip-vs-suspend-interruptible@c-dp1.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-apl3/igt@kms_flip@flip-vs-suspend-interruptible@c-dp1.html

  * {igt@kms_flip@plain-flip-ts-check@c-edp1}:
    - shard-skl:          [FAIL][41] ([i915#34]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-skl9/igt@kms_flip@plain-flip-ts-check@c-edp1.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-skl8/igt@kms_flip@plain-flip-ts-check@c-edp1.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - shard-kbl:          [DMESG-WARN][43] ([i915#180]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-kbl7/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-kbl6/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          [FAIL][45] ([fdo#108145] / [i915#265]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-skl5/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-skl6/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html

  * igt@kms_psr@psr2_sprite_mmap_gtt:
    - shard-iclb:         [SKIP][47] ([fdo#109441]) -> [PASS][48] +2 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-iclb4/igt@kms_psr@psr2_sprite_mmap_gtt.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-iclb2/igt@kms_psr@psr2_sprite_mmap_gtt.html

  * {igt@perf@polling-parameterized}:
    - shard-tglb:         [FAIL][49] ([i915#1542]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-tglb6/igt@perf@polling-parameterized.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-tglb7/igt@perf@polling-parameterized.html

  * {igt@perf@polling-small-buf}:
    - shard-iclb:         [FAIL][51] -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-iclb3/igt@perf@polling-small-buf.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-iclb8/igt@perf@polling-small-buf.html

  * {igt@sysfs_timeslice_duration@timeout@vecs0}:
    - shard-apl:          [FAIL][53] -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-apl2/igt@sysfs_timeslice_duration@timeout@vecs0.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-apl4/igt@sysfs_timeslice_duration@timeout@vecs0.html

  
#### Warnings ####

  * igt@gem_workarounds@suspend-resume-context:
    - shard-kbl:          [INCOMPLETE][55] ([i915#155]) -> [INCOMPLETE][56] ([i915#155] / [i915#180])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-kbl2/igt@gem_workarounds@suspend-resume-context.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-kbl7/igt@gem_workarounds@suspend-resume-context.html

  * igt@i915_pm_rpm@drm-resources-equal:
    - shard-snb:          [SKIP][57] ([fdo#109271]) -> [INCOMPLETE][58] ([i915#82])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-snb1/igt@i915_pm_rpm@drm-resources-equal.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-snb5/igt@i915_pm_rpm@drm-resources-equal.html

  * igt@kms_plane_alpha_blend@pipe-c-alpha-7efc:
    - shard-apl:          [FAIL][59] ([fdo#108145] / [i915#265] / [i915#95]) -> [FAIL][60] ([fdo#108145] / [i915#265])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8322/shard-apl4/igt@kms_plane_alpha_blend@pipe-c-alpha-7efc.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/shard-apl2/igt@kms_plane_alpha_blend@pipe-c-alpha-7efc.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [i915#1185]: https://gitlab.freedesktop.org/drm/intel/issues/1185
  [i915#1531]: https://gitlab.freedesktop.org/drm/intel/issues/1531
  [i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
  [i915#155]: https://gitlab.freedesktop.org/drm/intel/issues/155
  [i915#1658]: https://gitlab.freedesktop.org/drm/intel/issues/1658
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#250]: https://gitlab.freedesktop.org/drm/intel/issues/250
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#300]: https://gitlab.freedesktop.org/drm/intel/issues/300
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
  [i915#52]: https://gitlab.freedesktop.org/drm/intel/issues/52
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#668]: https://gitlab.freedesktop.org/drm/intel/issues/668
  [i915#69]: https://gitlab.freedesktop.org/drm/intel/issues/69
  [i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#82]: https://gitlab.freedesktop.org/drm/intel/issues/82
  [i915#93]: https://gitlab.freedesktop.org/drm/intel/issues/93
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8322 -> Patchwork_17356

  CI-20190529: 20190529
  CI_DRM_8322: fd447e6b1ee13e6a9731bddc7694552640e8a01e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5599: cdb07101dda33e2fcb0f4c2aa199c47159d88f35 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17356: bdd0a32afab1c4234d6476e1e86ea4c0ef73a2f4 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17356/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Flatten intel_dp_check_mst_status() a bit

[Ville Syrjälä]

On Fri, Apr 17, 2020 at 02:51:39PM -0400, Lyude Paul wrote:
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> 

Thanks. Series pushed to dinq.

> On Fri, 2020-04-17 at 18:27 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Make intel_dp_check_mst_status() somewhat legible by humans.
> > 
> > Note that the return value of drm_dp_mst_hpd_irq() is always
> > either 0 or -ENOMEM, and we never did anything with the latter
> > so we can just ignore the whole thing.
> > 
> > We can also get rid of the direct drm_dp_mst_topology_mgr_set_mst(false)
> > call since returning -EINVAL causes the caller to do the very same call
> > for us.
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 88 ++++++++++++-------------
> >  1 file changed, 41 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 48397b2c08cf..4d4898db38e9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5628,61 +5628,55 @@ static int
> >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > -	bool bret;
> >  
> > -	if (intel_dp->is_mst) {
> > -		u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> > -		int ret = 0;
> > +	if (!intel_dp->is_mst)
> > +		return -EINVAL;
> > +
> > +	WARN_ON_ONCE(intel_dp->active_mst_links < 0);
> > +
> > +	for (;;) {
> > +		u8 esi[DP_DPRX_ESI_LEN] = {};
> > +		bool bret, handled;
> >  		int retry;
> > -		bool handled;
> >  
> > -		WARN_ON_ONCE(intel_dp->active_mst_links < 0);
> >  		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> > -go_again:
> > -		if (bret == true) {
> > -
> > -			/* check link status - esi[10] = 0x200c */
> > -			if (intel_dp->active_mst_links > 0 &&
> > -			    !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);
> > -			}
> > -
> > -			drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi);
> > -			ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi,
> > &handled);
> > -
> > -			if (handled) {
> > -				for (retry = 0; retry < 3; retry++) {
> > -					int wret;
> > -					wret = drm_dp_dpcd_write(&intel_dp-
> > >aux,
> > -								 DP_SINK_COUNT
> > _ESI+1,
> > -								 &esi[1], 3);
> > -					if (wret == 3) {
> > -						break;
> > -					}
> > -				}
> > -
> > -				bret = intel_dp_get_sink_irq_esi(intel_dp,
> > esi);
> > -				if (bret == true) {
> > -					drm_dbg_kms(&i915->drm,
> > -						    "got esi2 %3ph\n", esi);
> > -					goto go_again;
> > -				}
> > -			} else
> > -				ret = 0;
> > -
> > -			return ret;
> > -		} else {
> > +		if (!bret) {
> >  			drm_dbg_kms(&i915->drm,
> >  				    "failed to get ESI - device may have
> > failed\n");
> > -			intel_dp->is_mst = false;
> > -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > -							intel_dp->is_mst);
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* 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 &&
> > +		    !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);
> > +		}
> > +
> > +		drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi);
> > +
> > +		drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> > +		if (!handled)
> > +			break;
> > +
> > +		for (retry = 0; retry < 3; retry++) {
> > +			int wret;
> > +
> > +			wret = drm_dp_dpcd_write(&intel_dp->aux,
> > +						 DP_SINK_COUNT_ESI+1,
> > +						 &esi[1], 3);
> > +			if (wret == 3)
> > +				break;
> >  		}
> >  	}
> > -	return -EINVAL;
> > +
> > +	return 0;
> >  }
> >  
> >  static bool
> > -- 
> > 2.24.1
> > 
> -- 
> Cheers,
> 	Lyude Paul (she/her)
> 	Associate Software Engineer at Red Hat

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx