[Intel-gfx] [PATCH 4/4] drm/i915: Sanity check PPS HW state

[Imre Deak]

On to, 2016-06-16 at 17:01 +0300, Ville Syrjälä wrote:
> On Thu, Jun 16, 2016 at 04:37:23PM +0300, Imre Deak wrote:
> > The wait for panel status helper will only function correctly if the
> > HW panel timings are programmed correctly. Returning prematurely from
> > this helper may lead to obscure bugs later, so sanity check the HW
> > timing registers.
> > 
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 36 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index caad825..163dcad 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1772,6 +1772,9 @@ static void intel_dp_prepare(struct intel_encoder *encoder)
> >  #define IDLE_CYCLE_MASK		(PP_ON | PP_SEQUENCE_MASK | PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK)
> >  #define IDLE_CYCLE_VALUE	(0     | PP_SEQUENCE_NONE | 0                     | PP_SEQUENCE_STATE_OFF_IDLE)
> >  
> > +static void intel_pps_verify_state(struct drm_i915_private *dev_priv,
> > +				   struct intel_dp *intel_dp);
> > +
> >  static void wait_panel_status(struct intel_dp *intel_dp,
> >  				       u32 mask,
> >  				       u32 value)
> > @@ -1782,6 +1785,8 @@ static void wait_panel_status(struct intel_dp *intel_dp,
> >  
> >  	lockdep_assert_held(&dev_priv->pps_mutex);
> >  
> > +	intel_pps_verify_state(dev_priv, intel_dp);
> > +
> >  	pp_stat_reg = _pp_stat_reg(intel_dp);
> >  	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
> >  
> > @@ -4821,6 +4826,35 @@ intel_pps_readout_hw_state(struct drm_i915_private *dev_priv,
> >  }
> >  
> >  static void
> > +intel_pps_dump_state(const char *state_name, const struct edp_power_seq *seq)
> > +{
> > +	DRM_DEBUG_KMS("%s t1_t3 %d t8 %d t9 %d t10 %d t11_t12 %d\n",
> > +		      state_name,
> > +		      seq->t1_t3, seq->t8, seq->t9, seq->t10, seq->t11_t12);
> > +}
> > +
> > +static void
> > +intel_pps_verify_state(struct drm_i915_private *dev_priv,
> > +		       struct intel_dp *intel_dp)
> > +{
> > +	struct edp_power_seq hw;
> > +	struct edp_power_seq *sw = &intel_dp->pps_delays;
> > +
> > +	intel_pps_readout_hw_state(dev_priv, intel_dp, &hw);
> > +
> > +	/*
> > +	 * We don't use/program the HW T8 and T9 timings as we use SW based
> > +	 * delays for these, so the HW state of these fields are dont-care.
> > +	 */
> 
> I don't think they should be treated as "don't care". We want them to
> be 1 to avoid needless delays.

Ah right, didn't notice that we program these. I'll fix this.

> 
> > +	if (hw.t1_t3 != sw->t1_t3 || hw.t10 != sw->t10 ||
> > +	    hw.t11_t12 != sw->t11_t12) {
> > +		DRM_ERROR("PPS state mismatch\n");
> > +		intel_pps_dump_state("sw", sw);
> > +		intel_pps_dump_state("hw", &hw);
> > +	}
> > +}
> > +
> > +static void
> >  intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> >  				    struct intel_dp *intel_dp)
> >  {
> > @@ -4836,8 +4870,7 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> >  
> >  	intel_pps_readout_hw_state(dev_priv, intel_dp, &cur);
> >  
> > -	DRM_DEBUG_KMS("cur t1_t3 %d t8 %d t9 %d t10 %d t11_t12 %d\n",
> > -		      cur.t1_t3, cur.t8, cur.t9, cur.t10, cur.t11_t12);
> > +	intel_pps_dump_state("cur", &cur);
> >  
> >  	vbt = dev_priv->vbt.edp.pps;
> >  
> > @@ -4853,8 +4886,7 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> >  	 * too. */
> >  	spec.t11_t12 = (510 + 100) * 10;
> >  
> > -	DRM_DEBUG_KMS("vbt t1_t3 %d t8 %d t9 %d t10 %d t11_t12 %d\n",
> > -		      vbt.t1_t3, vbt.t8, vbt.t9, vbt.t10, vbt.t11_t12);
> > +	intel_pps_dump_state("vbt", &vbt);
> >  
> >  	/* Use the max of the register settings and vbt. If both are
> >  	 * unset, fall back to the spec limits. */
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>