[Intel-gfx] [PATCH] drm/i915: Prevent PPS stealing from a normal DP port on VLV/CHV
Imre Deak
imre.deak at intel.com
Mon Dec 12 21:06:19 UTC 2016
On Mon, 2016-12-12 at 16:21 +0200, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at 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 at vger.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala at 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.
> + */
> + 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. 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 at 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.
> */
More information about the Intel-gfx
mailing list