[PATCH] drm/i915: Prevent PPS stealing from a normal DP port on VLV/CHV

* [PATCH] drm/i915: Prevent PPS stealing from a normal DP port on VLV/CHV
@ 2016-12-12 14:21 ` ville.syrjala
  0 siblings, 0 replies; 11+ messages in thread
From: ville.syrjala @ 2016-12-12 14:21 UTC (permalink / raw)
  To: intel-gfx; +Cc: stable

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

VLV apparently gets upset if the PPS for a pipe currently driving an
external DP port gets used for VDD stuff on another eDP port. The DP
port falls over and fails to retrain when this happens, leaving the
user staring at a black screen.

Let's fix it by also tracking which pipe is driving wich DP/eDP port.
We'll track this under intel_dp so that we'll share the protection
of the pps_mutex alongside the pps_pipe tracking, since the two
things are intimately related.

I had plans to reduce the protection of pps_mutex to cover only eDP
ports, but with this we can't do that. Well, for for VLV/CHV at least.
For other platforms it should still be possible, which would allow
AUX communication to occur in parallel for multiple DP ports.

Cc: stable@vger.kernel.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 151 +++++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h |   6 ++
 2 files changed, 110 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index db75bb924e48..0da7d528c1a9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -454,14 +454,52 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	}
 }
 
+static enum pipe vlv_find_free_pps(struct drm_i915_private *dev_priv)
+{
+	struct intel_encoder *encoder;
+	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
+
+	/*
+	 * We don't have power sequencer currently.
+	 * Pick one that's not used by other ports.
+	 *
+	 * We will
+	 */
+	for_each_intel_encoder(&dev_priv->drm, encoder) {
+		struct intel_dp *intel_dp;
+
+		if (encoder->type != INTEL_OUTPUT_DP &&
+		    encoder->type != INTEL_OUTPUT_EDP)
+			continue;
+
+		intel_dp = enc_to_intel_dp(&encoder->base);
+
+		if (encoder->type == INTEL_OUTPUT_EDP) {
+			WARN_ON(intel_dp->active_pipe != INVALID_PIPE &&
+				intel_dp->active_pipe != intel_dp->pps_pipe);
+
+			if (intel_dp->pps_pipe != INVALID_PIPE)
+				pipes &= ~(1 << intel_dp->pps_pipe);
+		} else {
+			WARN_ON(intel_dp->pps_pipe != INVALID_PIPE);
+
+			if (intel_dp->active_pipe != INVALID_PIPE)
+				pipes &= ~(1 << intel_dp->active_pipe);
+		}
+	}
+
+	if (pipes == 0)
+		return INVALID_PIPE;
+
+	return ffs(pipes) - 1;
+}
+
 static enum pipe
 vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_encoder *encoder;
-	unsigned int pipes = (1 << PIPE_A) | (1 << PIPE_B);
 	enum pipe pipe;
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
@@ -469,33 +507,20 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp)
 	/* We should never land here with regular DP ports */
 	WARN_ON(!is_edp(intel_dp));
 
+	WARN_ON(intel_dp->active_pipe != INVALID_PIPE &&
+		intel_dp->active_pipe != intel_dp->pps_pipe);
+
 	if (intel_dp->pps_pipe != INVALID_PIPE)
 		return intel_dp->pps_pipe;
 
-	/*
-	 * We don't have power sequencer currently.
-	 * Pick one that's not used by other ports.
-	 */
-	for_each_intel_encoder(dev, encoder) {
-		struct intel_dp *tmp;
-
-		if (encoder->type != INTEL_OUTPUT_EDP)
-			continue;
-
-		tmp = enc_to_intel_dp(&encoder->base);
-
-		if (tmp->pps_pipe != INVALID_PIPE)
-			pipes &= ~(1 << tmp->pps_pipe);
-	}
+	pipe = vlv_find_free_pps(dev_priv);
 
 	/*
 	 * Didn't find one. This should not happen since there
 	 * are two power sequencers and up to two eDP ports.
 	 */
-	if (WARN_ON(pipes == 0))
+	if (WARN_ON(pipe == INVALID_PIPE))
 		pipe = PIPE_A;
-	else
-		pipe = ffs(pipes) - 1;
 
 	vlv_steal_power_sequencer(dev, pipe);
 	intel_dp->pps_pipe = pipe;
@@ -651,10 +676,17 @@ void intel_power_sequencer_reset(struct drm_i915_private *dev_priv)
 	for_each_intel_encoder(dev, encoder) {
 		struct intel_dp *intel_dp;
 
-		if (encoder->type != INTEL_OUTPUT_EDP)
+		if (encoder->type != INTEL_OUTPUT_DP &&
+		    encoder->type != INTEL_OUTPUT_EDP)
 			continue;
 
 		intel_dp = enc_to_intel_dp(&encoder->base);
+
+		WARN_ON(intel_dp->active_pipe != INVALID_PIPE);
+
+		if (encoder->type != INTEL_OUTPUT_EDP)
+			continue;
+
 		if (IS_GEN9_LP(dev_priv))
 			intel_dp->pps_reset = true;
 		else
@@ -2814,6 +2846,8 @@ static void vlv_detach_power_sequencer(struct intel_dp *intel_dp)
 	enum pipe pipe = intel_dp->pps_pipe;
 	i915_reg_t pp_on_reg = PP_ON_DELAYS(pipe);
 
+	WARN_ON(intel_dp->active_pipe != INVALID_PIPE);
+
 	edp_panel_vdd_off_sync(intel_dp);
 
 	/*
@@ -2848,22 +2882,23 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 		struct intel_dp *intel_dp;
 		enum port port;
 
-		if (encoder->type != INTEL_OUTPUT_EDP)
+		if (encoder->type != INTEL_OUTPUT_EDP &&
+		    encoder->type != INTEL_OUTPUT_DP)
 			continue;
 
 		intel_dp = enc_to_intel_dp(&encoder->base);
 		port = dp_to_dig_port(intel_dp)->port;
 
+		WARN(intel_dp->active_pipe == pipe,
+		     "stealing pipe %c power sequencer from active (e)DP port %c\n",
+		     pipe_name(pipe), port_name(port));
+
 		if (intel_dp->pps_pipe != pipe)
 			continue;
 
 		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
 			      pipe_name(pipe), port_name(port));
 
-		WARN(encoder->base.crtc,
-		     "stealing pipe %c power sequencer from active eDP port %c\n",
-		     pipe_name(pipe), port_name(port));
-
 		/* make sure vdd is off before we steal it */
 		vlv_detach_power_sequencer(intel_dp);
 	}
@@ -2879,19 +2914,17 @@ static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp)
 
 	lockdep_assert_held(&dev_priv->pps_mutex);
 
-	if (!is_edp(intel_dp))
-		return;
-
-	if (intel_dp->pps_pipe == crtc->pipe)
-		return;
+	WARN_ON(intel_dp->active_pipe != INVALID_PIPE);
 
-	/*
-	 * If another power sequencer was being used on this
-	 * port previously make sure to turn off vdd there while
-	 * we still have control of it.
-	 */
-	if (intel_dp->pps_pipe != INVALID_PIPE)
+	if (intel_dp->pps_pipe != INVALID_PIPE &&
+	    intel_dp->pps_pipe != crtc->pipe) {
+		/*
+		 * If another power sequencer was being used on this
+		 * port previously make sure to turn off vdd there while
+		 * we still have control of it.
+		 */
 		vlv_detach_power_sequencer(intel_dp);
+	}
 
 	/*
 	 * We may be stealing the power
@@ -2899,6 +2932,11 @@ static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp)
 	 */
 	vlv_steal_power_sequencer(dev, crtc->pipe);
 
+	intel_dp->active_pipe = crtc->pipe;
+
+	if (!is_edp(intel_dp))
+		return;
+
 	/* now it's all ours */
 	intel_dp->pps_pipe = crtc->pipe;
 
@@ -3485,6 +3523,9 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 	msleep(intel_dp->panel_power_down_delay);
 
 	intel_dp->DP = DP;
+
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		intel_dp->active_pipe = INVALID_PIPE;
 }
 
 bool
@@ -4750,6 +4791,19 @@ static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
 	edp_panel_vdd_schedule_off(intel_dp);
 }
 
+enum pipe vlv_active_pipe(struct intel_dp *intel_dp)
+{
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+
+	if ((intel_dp->DP & DP_PORT_EN) == 0)
+		return INVALID_PIPE;
+
+	if (IS_CHERRYVIEW(dev_priv))
+		return DP_PORT_TO_PIPE_CHV(intel_dp->DP);
+	else
+		return PORT_TO_PIPE(intel_dp->DP);
+}
+
 void intel_dp_encoder_reset(struct drm_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
@@ -4762,14 +4816,16 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
 	if (lspcon->active)
 		lspcon_resume(lspcon);
 
-	if (to_intel_encoder(encoder)->type != INTEL_OUTPUT_EDP)
-		return;
-
 	pps_lock(intel_dp);
 
-	/* Reinit the power sequencer, in case BIOS did something with it. */
-	intel_dp_pps_init(encoder->dev, intel_dp);
-	intel_edp_panel_vdd_sanitize(intel_dp);
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		intel_dp->active_pipe = vlv_active_pipe(intel_dp);
+
+	if (is_edp(intel_dp)) {
+		/* Reinit the power sequencer, in case BIOS did something with it. */
+		intel_dp_pps_init(encoder->dev, intel_dp);
+		intel_edp_panel_vdd_sanitize(intel_dp);
+	}
 
 	pps_unlock(intel_dp);
 }
@@ -5596,10 +5652,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 		 * If the current pipe isn't valid, try the PPS pipe, and if that
 		 * fails just assume pipe A.
 		 */
-		if (IS_CHERRYVIEW(dev_priv))
-			pipe = DP_PORT_TO_PIPE_CHV(intel_dp->DP);
-		else
-			pipe = PORT_TO_PIPE(intel_dp->DP);
+		pipe = vlv_active_pipe(intel_dp);
 
 		if (pipe != PIPE_A && pipe != PIPE_B)
 			pipe = intel_dp->pps_pipe;
@@ -5648,6 +5701,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		return false;
 
 	intel_dp->pps_pipe = INVALID_PIPE;
+	intel_dp->active_pipe = INVALID_PIPE;
 
 	/* intel_dp vfuncs */
 	if (INTEL_GEN(dev_priv) >= 9)
@@ -5676,6 +5730,9 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	else
 		type = DRM_MODE_CONNECTOR_DisplayPort;
 
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		intel_dp->active_pipe = vlv_active_pipe(intel_dp);
+
 	/*
 	 * For eDP we always set the encoder type to INTEL_OUTPUT_EDP, but
 	 * for DP the encoder type can be set by the caller to
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8f4ddca0f521..85b39d3a6dff 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -929,6 +929,12 @@ struct intel_dp {
 	 */
 	enum pipe pps_pipe;
 	/*
+	 * Pipe currently driving the port. Used for preventing
+	 * the use of the PPS for any pipe currentrly driving
+	 * external DP as that will mess things up on VLV.
+	 */
+	enum pipe active_pipe;
+	/*
 	 * Set if the sequencer may be reset due to a power transition,
 	 * requiring a reinitialization. Only relevant on BXT.
 	 */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 11+ messages in thread