[Intel-gfx] [PATCH 1/4] drm/i915/bxt: Fix PPS lost state after suspend breaking eDP link training
Imre Deak
imre.deak at intel.com
Thu Jun 16 14:39:35 UTC 2016
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 at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list