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