From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= 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 Message-ID: <20161213130301.GJ31595@intel.com> References: <1481552505-13828-1-git-send-email-ville.syrjala@linux.intel.com> <1481576779.17177.2.camel@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <1481576779.17177.2.camel@intel.com> Sender: stable-owner@vger.kernel.org To: Imre Deak Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org 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ä > > > > 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. 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 > > > + > > + 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