| Message ID | 1466084243-5388-2-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > --- > 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. Are you going to do something about PCH platforms as well, or are you going leave that for someone else? > > @@ -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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > --- > > 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. > 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. > > > > @@ -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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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@intel.com> > > > --- > > > 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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
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); } @@ -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,
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@intel.com> --- 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(-)