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

stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Prevent PPS stealing from a normal DP port on VLV/CHV
Date: Tue, 13 Dec 2016 15:03:01 +0200	[thread overview]
Message-ID: <20161213130301.GJ31595@intel.com> (raw)
In-Reply-To: <1481576779.17177.2.camel@intel.com>

On Mon, Dec 12, 2016 at 11:06:19PM +0200, Imre Deak wrote:
> On Mon, 2016-12-12 at 16:21 +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 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
> 
> 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@intel.com>
> 
> > +
> > +			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

next prev parent reply	other threads:[~2016-12-13 13:03 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-12 14:21 [PATCH] drm/i915: Prevent PPS stealing from a normal DP port on VLV/CHV ville.syrjala
2016-12-12 21:06 ` [Intel-gfx] " Imre Deak
2016-12-13 13:03   ` Ville Syrjälä [this message]
2016-12-14 18:00 ` [PATCH v2] " ville.syrjala
2016-12-19 13:01   ` Ville Syrjälä

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161213130301.GJ31595@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=imre.deak@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Be sure your reply has a Subject: header at the top and a blank line before the message body.

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).