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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
