On Thu, Jun 16, 2016 at 05:39:35PM +0300, Imre Deak wrote: > On to, 2016-06-16 at 16:58 +0300, Ville Syrjälä wrote: > > On Thu, Jun 16, 2016 at 04:37:20PM +0300, Imre Deak wrote: > > > The PPS registers are backed by power well #0 and as such may be reset > > > after system or runtime suspend (both implying a possible DC9 > > > transition). Fix this by reusing the VLV/CHV PPS pipe-reassignment > > > logic. The difference on BXT is that the PPS instances are not pipe but > > > port (or more accurately pin) specific, so we only need to care about > > > the lost HW state. As opposed to VLV/CHV the SW state is fixed and > > > initialized during connector init. > > > > > > This also paves the way towards using the actual port->PPS instance > > > mapping based on VBT. > > > > > > This fixes eDP link training errors on BXT after suspend, where we > > > started the link training too early due to an incorrect T3 (panel power > > > on) register value. > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96436 > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++---------- > > > drivers/gpu/drm/i915/intel_drv.h | 7 +++- > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +- > > > 3 files changed, 58 insertions(+), 23 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index be08351..19a8bbe 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -426,6 +426,37 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp) > > > return intel_dp->pps_pipe; > > > } > > > > > > +static int > > > +bxt_power_sequencer_idx(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 = dev->dev_private; > > > + > > > + lockdep_assert_held(&dev_priv->pps_mutex); > > > + > > > + /* We should never land here with regular DP ports */ > > > + WARN_ON(!is_edp(intel_dp)); > > > + > > > + /* > > > + * TODO: BXT has 2 PPS instances. The correct port->PPS instance > > > + * mapping needs to be retrieved from VBT, for now just hard-code to > > > + * use instance #0 always. > > > + */ > > > + if (!intel_dp->pps_reset) > > > + return 0; > > > + > > > + intel_dp->pps_reset = false; > > > + > > > + /* > > > + * Only the HW needs to be reprogrammed, the SW state is fixed and > > > + * has been setup during connector init. > > > + */ > > > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp); > > > + > > > + return 0; > > > +} > > > + > > > typedef bool (*vlv_pipe_check)(struct drm_i915_private *dev_priv, > > > enum pipe pipe); > > > > > > @@ -507,12 +538,13 @@ vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp) > > > intel_dp_init_panel_power_sequencer_registers(dev, intel_dp); > > > } > > > > > > -void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv) > > > +void intel_power_sequencer_reset(struct drm_i915_private *dev_priv) > > > { > > > struct drm_device *dev = dev_priv->dev; > > > struct intel_encoder *encoder; > > > > > > - if (WARN_ON(!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev))) > > > + if (WARN_ON(!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) && > > > + !IS_BROXTON(dev))) > > > return; > > > > > > /* > > > @@ -532,7 +564,10 @@ void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv) > > > continue; > > > > > > intel_dp = enc_to_intel_dp(&encoder->base); > > > - intel_dp->pps_pipe = INVALID_PIPE; > > > + if (IS_BROXTON(dev)) > > > + intel_dp->pps_reset = true; > > > + else > > > + intel_dp->pps_pipe = INVALID_PIPE; > > > } > > > } > > > > > > @@ -542,7 +577,7 @@ _pp_ctrl_reg(struct intel_dp *intel_dp) > > > struct drm_device *dev = intel_dp_to_dev(intel_dp); > > > > > > if (IS_BROXTON(dev)) > > > - return BXT_PP_CONTROL(0); > > > + return BXT_PP_CONTROL(bxt_power_sequencer_idx(intel_dp)); > > > else if (HAS_PCH_SPLIT(dev)) > > > return PCH_PP_CONTROL; > > > else > > > @@ -555,7 +590,7 @@ _pp_stat_reg(struct intel_dp *intel_dp) > > > struct drm_device *dev = intel_dp_to_dev(intel_dp); > > > > > > if (IS_BROXTON(dev)) > > > - return BXT_PP_STATUS(0); > > > + return BXT_PP_STATUS(bxt_power_sequencer_idx(intel_dp)); > > > else if (HAS_PCH_SPLIT(dev)) > > > return PCH_PP_STATUS; > > > else > > > @@ -4722,14 +4757,11 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev, > > > return; > > > > > > if (IS_BROXTON(dev)) { > > > - /* > > > - * TODO: BXT has 2 sets of PPS registers. > > > - * Correct Register for Broxton need to be identified > > > - * using VBT. hardcoding for now > > > - */ > > > - pp_ctrl_reg = BXT_PP_CONTROL(0); > > > - pp_on_reg = BXT_PP_ON_DELAYS(0); > > > - pp_off_reg = BXT_PP_OFF_DELAYS(0); > > > + int idx = bxt_power_sequencer_idx(intel_dp); > > > + > > > + pp_ctrl_reg = BXT_PP_CONTROL(idx); > > > + pp_on_reg = BXT_PP_ON_DELAYS(idx); > > > + pp_off_reg = BXT_PP_OFF_DELAYS(idx); > > > } else if (HAS_PCH_SPLIT(dev)) { > > > pp_ctrl_reg = PCH_PP_CONTROL; > > > pp_on_reg = PCH_PP_ON_DELAYS; > > > @@ -4842,14 +4874,11 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, > > > lockdep_assert_held(&dev_priv->pps_mutex); > > > > > > if (IS_BROXTON(dev)) { > > > - /* > > > - * TODO: BXT has 2 sets of PPS registers. > > > - * Correct Register for Broxton need to be identified > > > - * using VBT. hardcoding for now > > > - */ > > > - pp_ctrl_reg = BXT_PP_CONTROL(0); > > > - pp_on_reg = BXT_PP_ON_DELAYS(0); > > > - pp_off_reg = BXT_PP_OFF_DELAYS(0); > > > + int idx = bxt_power_sequencer_idx(intel_dp); > > > + > > > + pp_ctrl_reg = BXT_PP_CONTROL(idx); > > > + pp_on_reg = BXT_PP_ON_DELAYS(idx); > > > + pp_off_reg = BXT_PP_OFF_DELAYS(idx); > > > > > > } else if (HAS_PCH_SPLIT(dev)) { > > > pp_on_reg = PCH_PP_ON_DELAYS; > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > index 8dc67ad..870849e 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -869,6 +869,11 @@ struct intel_dp { > > > * this port. Only relevant on VLV/CHV. > > > */ > > > enum pipe pps_pipe; > > > + /* > > > + * Set if the sequencer may be reset due to a power transition, > > > + * requiring a reinitialization. Only relevant on BXT. > > > + */ > > > + bool pps_reset; > > > struct edp_power_seq pps_delays; > > > > > > bool can_mst; /* this port supports mst */ > > > @@ -1348,7 +1353,7 @@ void intel_dp_mst_resume(struct drm_device *dev); > > > int intel_dp_max_link_rate(struct intel_dp *intel_dp); > > > int intel_dp_rate_select(struct intel_dp *intel_dp, int rate); > > > void intel_dp_hot_plug(struct intel_encoder *intel_encoder); > > > -void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv); > > > +void intel_power_sequencer_reset(struct drm_i915_private *dev_priv); > > > uint32_t intel_dp_pack_aux(const uint8_t *src, int src_bytes); > > > void intel_plane_destroy(struct drm_plane *plane); > > > void intel_edp_drrs_enable(struct intel_dp *intel_dp); > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > index e856d49..22b46f5 100644 > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > @@ -578,6 +578,7 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv) > > > > > > DRM_DEBUG_KMS("Enabling DC9\n"); > > > > > > + intel_power_sequencer_reset(dev_priv); > > > gen9_set_dc_state(dev_priv, DC_STATE_EN_DC9); > > > } > > > > I was wondering how we deal with coming out of S4 now, but I suppose we > > currently enable DC9 in .freeze as well. When/if we remove that (to optimize > > away the blinks during hibernation), I think we'll need something different > > to deal with the unknown PPS state on .restore. > > Hm yea, I assume we could reset the state in .restore then. But we'd > still need the DC9 time reset even in that case. Yeah. > > > Are you going to do something about PCH platforms as well, or are you going > > leave that for someone else? > > I can take a look at that later if no one else does. AFAICS it would > amount to replacing the PPS save/restore in i915_suspend.c with a reset > during system suspend+register re-init based on pps_reset > in intel_pps_get_registers(). I'd have to also check how this would > affect LVDS. I was thinking that might simply move the current save code to LVDS init, and then add a .reset hook for LVDS to restore the registers at resume time. But I msut admit that I didn't spend much time thinking about it. > > > > > > > @@ -1112,7 +1113,7 @@ static void vlv_display_power_well_deinit(struct drm_i915_private *dev_priv) > > > /* make sure we're done processing display irqs */ > > > synchronize_irq(dev_priv->dev->irq); > > > > > > - vlv_power_sequencer_reset(dev_priv); > > > + intel_power_sequencer_reset(dev_priv); > > > } > > > > > > static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv, > > > -- > > > 2.5.0 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
