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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Dec 14, 2016 at 08:00:23PM +0200, ville.syrjala@linux.intel.com wrote:
> 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 which 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.
> 
> v2: Drop stray crap from a comment (Imre)
>     Grab pps_mutex when clearing active_pipe
>     Fix a typo in the commit message
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Pushed to dinq. Thanks for the review.

I did notice that I had neglected to make vlv_active_pipe() static,
but I just fixed that up while applying the patch.

> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 152 +++++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h |   6 ++
>  2 files changed, 111 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index db75bb924e48..fe830e3736cf 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -454,14 +454,50 @@ 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.
> +	 */
> +	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 +505,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 +674,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 +2844,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 +2880,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_DP &&
> +		    encoder->type != INTEL_OUTPUT_EDP)
>  			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 +2912,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;
> +	WARN_ON(intel_dp->active_pipe != INVALID_PIPE);
>  
> -	if (intel_dp->pps_pipe == crtc->pipe)
> -		return;
> -
> -	/*
> -	 * 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 +2930,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 +3521,12 @@ 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)) {
> +		pps_lock(intel_dp);
> +		intel_dp->active_pipe = INVALID_PIPE;
> +		pps_unlock(intel_dp);
> +	}
>  }
>  
>  bool
> @@ -4750,6 +4792,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 +4817,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 +5653,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 +5702,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 +5731,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

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