From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com ([134.134.136.65]:38272 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932205AbcLLVGX (ORCPT ); Mon, 12 Dec 2016 16:06:23 -0500 Message-ID: <1481576779.17177.2.camel@intel.com> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Prevent PPS stealing from a normal DP port on VLV/CHV From: Imre Deak Reply-To: imre.deak@intel.com To: ville.syrjala@linux.intel.com, intel-gfx@lists.freedesktop.org Cc: stable@vger.kernel.org Date: Mon, 12 Dec 2016 23:06:19 +0200 In-Reply-To: <1481552505-13828-1-git-send-email-ville.syrjala@linux.intel.com> References: <1481552505-13828-1-git-send-email-ville.syrjala@linux.intel.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: On Mon, 2016-12-12 at 16:21 +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä > > 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ä > --- >  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 > + > + 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. >    */