Re: [PATCH 5/6] drm/i915: Apply the PPS register unlock workaround more consistently

[Imre Deak <imre.deak@intel.com>]

On ti, 2016-08-09 at 16:01 +0300, Ville Syrjälä wrote:
> On Tue, Aug 09, 2016 at 02:34:11PM +0300, Imre Deak wrote:
> > Atm, we apply this workaround somewhat inconsistently at the following
> > points: driver loading, LVDS init, eDP PPS init, system resume. As this
> > workaround also affects registers other than PPS (timing, PLL) a more
> > consistent way is to apply it early after the PPS HW context is known to
> > be lost: driver loading, system resume and on VLV/CHV/BXT when turning
> > on power domains.
> > 
> > This is needed by the next patch that removes saving/restoring of the
> > PP_CONTROL register.
> > 
> > This also removes the incorrect programming of the workaround on HSW+
> > PCH platforms which don't have the register locking mechanism.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c         |  1 +
> >  drivers/gpu/drm/i915/intel_display.c    | 28 ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c         |  4 +++-
> >  drivers/gpu/drm/i915/intel_drv.h        |  1 +
> >  drivers/gpu/drm/i915/intel_lvds.c       |  8 --------
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  4 ++++
> >  6 files changed, 37 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 8cfc264..0fcd1c0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1560,6 +1560,7 @@ static int i915_drm_resume(struct drm_device *dev)
> >  	i915_gem_resume(dev);
> >  
> >  	i915_restore_state(dev);
> > +	intel_pps_unlock_regs_wa(dev_priv);
> >  	intel_opregion_setup(dev_priv);
> >  
> >  	intel_init_pch_refclk(dev);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3d5fd06..dc4e600 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14635,6 +14635,32 @@ static bool intel_crt_present(struct drm_device *dev)
> >  	return true;
> >  }
> >  
> > +void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv)
> > +{
> > +	int pps_num;
> > +	int pps_idx;
> > +
> > +	/*
> > +	 * This w/a is needed at least on CPT/PPT, but to be sure apply it
> > +	 * everywhere where registers can be write protected.
> > +	 */
> > +	if (INTEL_GEN(dev_priv) <= 4 ||
> > +	    HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv))
> > +		pps_num = 1;
> > +	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv) ||
> > +		 IS_BROXTON(dev_priv))
> > +		pps_num = 2;
> > +	else
> > +		pps_num = 0;
> 
> BXT shouldn't need the unlock, I think.

Yes, got that wrong.

> 
> So I believe we just want
> 
> if (HAS_DDI())
> 	return;
> 
> if (VLV||CHV)
> 	num = 2;
> else
> 	num = 1;

Ok.

> 
> > +
> > +	for (pps_idx = 0; pps_idx < pps_num; pps_idx++) {
> > +		u32 val = I915_READ(PP_CONTROL(pps_idx));
> > +
> > +		val = (val & ~PANEL_UNLOCK_MASK) | PANEL_UNLOCK_REGS;
> > +		I915_WRITE(PP_CONTROL(pps_idx), val);
> > +	}
> > +}
> > +
> >  static void intel_pps_init(struct drm_i915_private *dev_priv)
> >  {
> >  	if (HAS_PCH_SPLIT(dev_priv) || IS_BROXTON(dev_priv))
> > @@ -14643,6 +14669,8 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
> >  		dev_priv->pps_mmio_base = VLV_PPS_BASE;
> >  	else
> >  		dev_priv->pps_mmio_base = PPS_BASE;
> > +
> > +	intel_pps_unlock_regs_wa(dev_priv);
> >  }
> >  
> >  static void intel_setup_outputs(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 76f5b72..b27f1c5 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1829,7 +1829,9 @@ static  u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
> >  	lockdep_assert_held(&dev_priv->pps_mutex);
> >  
> >  	control = I915_READ(_pp_ctrl_reg(intel_dp));
> > -	if (!IS_BROXTON(dev)) {
> > +	if (WARN_ON((HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv) ||
> > +		     IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> 
> !HAS_DDI

Heh, was thinking of a shorter way, but this didn't occur to me. Will
change it.

> 
> > +		     (control & PANEL_UNLOCK_MASK) != PANEL_UNLOCK_REGS)) {
> >  		control &= ~PANEL_UNLOCK_MASK;
> >  		control |= PANEL_UNLOCK_REGS;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index c29a429..cbce786 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1159,6 +1159,7 @@ void intel_mark_busy(struct drm_i915_private *dev_priv);
> >  void intel_mark_idle(struct drm_i915_private *dev_priv);
> >  void intel_crtc_restore_mode(struct drm_crtc *crtc);
> >  int intel_display_suspend(struct drm_device *dev);
> > +void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv);
> >  void intel_encoder_destroy(struct drm_encoder *encoder);
> >  int intel_connector_init(struct intel_connector *);
> >  struct intel_connector *intel_connector_alloc(void);
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index d5158e5..9a6e1ad 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -967,14 +967,6 @@ void intel_lvds_init(struct drm_device *dev)
> >  	int pipe;
> >  	u8 pin;
> >  
> > -	/*
> > -	 * Unlock registers and just leave them unlocked. Do this before
> > -	 * checking quirk lists to avoid bogus WARNINGs.
> > -	 */
> > -	if (HAS_PCH_SPLIT(dev_priv) || INTEL_GEN(dev_priv) <= 4)
> > -		I915_WRITE(PP_CONTROL(0),
> > -			   I915_READ(PP_CONTROL(0)) | PANEL_UNLOCK_REGS);
> > -
> >  	if (!intel_lvds_supported(dev))
> >  		return;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 1c603bb..8f51ae3 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -592,6 +592,8 @@ void bxt_disable_dc9(struct drm_i915_private *dev_priv)
> >  	DRM_DEBUG_KMS("Disabling DC9\n");
> >  
> >  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > +
> > +	intel_pps_unlock_regs_wa(dev_priv);
> >  }
> >  
> >  static void assert_csr_loaded(struct drm_i915_private *dev_priv)
> > @@ -1145,6 +1147,8 @@ static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,
> >  	vlv_set_power_well(dev_priv, power_well, true);
> >  
> >  	vlv_display_power_well_init(dev_priv);
> > +
> > +	intel_pps_unlock_regs_wa(dev_priv);
> >  }
> >  
> >  static void vlv_display_power_well_disable(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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx