| Message ID | 1466084243-5388-5-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. > + 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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. + */ + 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. */
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@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-)