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

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

 



On Mon, Dec 12, 2016 at 11:06:19PM +0200, Imre Deak wrote:
> On Mon, 2016-12-12 at 16:21 +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> >  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
> 
> Remnant line.

Will nuke.

> 
> > +	 */
> > +	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);
> 
> In theory BIOS could enable eDP with active_pipe != pps_pipe, but it's
> better to WARN for that case.

That would likely mean the display wouldn't actually work though,
so not something I'd expect to see.

> I suppose there could be an early check for
> that already at the end of intel_dp_init_connector(). Either way looks ok:
> 
> Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
> 
> > +
> > +			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.
> >  	 */

-- 
Ville Syrjälä
Intel OTC
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]