[Intel-gfx] [PATCH 1/4] drm/i915/bxt: Fix PPS lost state after suspend breaking eDP link training
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Jun 16 14:48:08 UTC 2016
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 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.
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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list