| Message ID | 1350759465-7171-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 20 Oct 2012 20:57:41 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > 3 changes: > - If a given value is unset, use the maximal limits from the eDP spec. > - Write back the new values, since otherwise the panel power sequencing > hw will not dtrt. > - Revert the early bail-out in case the register values are unset. > > The last change reverts > > commit bfa3384a9a84aaaa59443bbd776c142e7dba4b0f > Author: Jesse Barnes <jbarnes@virtuousgeek.org> > Date: Tue Apr 10 11:58:04 2012 -0700 > > drm/i915: check PPS regs for sanity when using eDP > > v2: > - Unlock the PP regs as the very first thing. This is a required w/a > for cpu eDP on port A, and generally a good idea. > - Fixup the panel power control port selection bits. > > v3: Paulo Zanoni noticed that I've fumbled the computation of the spec > limit values. Fix them up. We've also noticed that the t8/t9 values in > the vbt/bios-programmed pp are much larger than any limits. My guess > is that this is to conceal any backlight enable/disable delays. So by > using the much shorter limits from the spec, which only concerns the > sink, we risk that we might display before the backlight is fully on, > or disable the output while the backlight still has afterglow. I've > figured I don't care too much, since this will only happen when both > the pp regs are not programmed, and the vbt tables don't contain > anything useful. > > v4: Don't set the port selection bits on hsw/LPT, they don't exist any > more. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_reg.h | 5 +++ > drivers/gpu/drm/i915/intel_dp.c | 71 ++++++++++++++++++++++++++++++++++------- > 2 files changed, 65 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index b428fbb..3ecd8c3 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4018,6 +4018,11 @@ > #define PANEL_LIGHT_ON_DELAY_SHIFT 0 > > #define PCH_PP_OFF_DELAYS 0xc720c > +#define PANEL_POWER_PORT_SELECT_MASK (0x3 << 30) > +#define PANEL_POWER_PORT_LVDS (0 << 30) > +#define PANEL_POWER_PORT_DP_A (1 << 30) > +#define PANEL_POWER_PORT_DP_C (2 << 30) > +#define PANEL_POWER_PORT_DP_D (3 << 30) > #define PANEL_POWER_DOWN_DELAY_MASK (0x1fff0000) > #define PANEL_POWER_DOWN_DELAY_SHIFT 16 > #define PANEL_LIGHT_OFF_DELAY_MASK (0x1fff) > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index f2c9ea6..265cec1 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2671,20 +2671,18 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) > > /* Cache some DPCD data in the eDP case */ > if (is_edp(intel_dp)) { > - struct edp_power_seq cur, vbt; > - u32 pp_on, pp_off, pp_div; > + struct edp_power_seq cur, vbt, spec, final; > + u32 pp_on, pp_off, pp_div, pp; > + > + /* Workaround: Need to write PP_CONTROL with the unlock key as > + * the very first thing. */ > + pp = ironlake_get_pp_control(dev_priv); > + I915_WRITE(PCH_PP_CONTROL, pp); > > pp_on = I915_READ(PCH_PP_ON_DELAYS); > pp_off = I915_READ(PCH_PP_OFF_DELAYS); > pp_div = I915_READ(PCH_PP_DIVISOR); > > - if (!pp_on || !pp_off || !pp_div) { > - DRM_INFO("bad panel power sequencing delays, disabling panel\n"); > - intel_dp_encoder_destroy(&intel_dp->base.base); > - intel_dp_destroy(&intel_connector->base); > - return; > - } > - > /* Pull timing values out of registers */ > cur.t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >> > PANEL_POWER_UP_DELAY_SHIFT; > @@ -2706,16 +2704,62 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) > > vbt = dev_priv->edp.pps; > > + /* Upper limits from eDP 1.3 spec. Note that we the clunky units > + * of our hw here, which are all in 100usec. */ "we use the clunky" would be more sensible than "we the clunky" > + spec.t1_t3 = 210 * 10; > + spec.t8 = 50 * 10; /* no limit for t8, use t7 instead */ > + spec.t9 = 50 * 10; /* no limit for t9, make it symmetric with t8 */ > + spec.t10 = 500 * 10; > + /* This one is special and actually in units of 100ms, but zero > + * based in the hw (so we need to add 100 ms). But the sw vbt > + * table multiplies it with 1000 to make it in units of 100usec, > + * 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); > > -#define get_delay(field) ((max(cur.field, vbt.field) + 9) / 10) > - > + /* Use the max of the register setttings and vbt. If both are > + * unset, fall back to the spec limits. */ settings only needs two "t"s. > +#define assign_final(field) final.field = (max(cur.field, vbt.field) == 0 ? \ > + spec.field : \ > + max(cur.field, vbt.field)) > + assign_final(t1_t3); > + assign_final(t8); > + assign_final(t9); > + assign_final(t10); > + assign_final(t11_t12); > +#undef assign_final If the current field is not zero and doesn't match the VBT, we might add a debug statement. It could indicate a BIOS programmed value that didn't involve a VBT update (I can imagine some vendors might do this). Overwriting the current with the different VBT value may lead to breakage or sub-optimal timings. > + > +#define get_delay(field) (DIV_ROUND_UP(final.field, 10)) > intel_dp->panel_power_up_delay = get_delay(t1_t3); > intel_dp->backlight_on_delay = get_delay(t8); > intel_dp->backlight_off_delay = get_delay(t9); > intel_dp->panel_power_down_delay = get_delay(t10); > intel_dp->panel_power_cycle_delay = get_delay(t11_t12); > +#undef get_delay > + > + /* And finally store the new values in the power sequencer. */ > + pp_on = (final.t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) | > + (final.t8 << PANEL_LIGHT_ON_DELAY_SHIFT); > + pp_off = (final.t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) | > + (final.t10 << PANEL_POWER_DOWN_DELAY_SHIFT); > + pp_div = (pp_div & PP_REFERENCE_DIVIDER_MASK) | > + (DIV_ROUND_UP(final.t11_t12, 1000) << PANEL_POWER_CYCLE_DELAY_SHIFT); > + > + /* Haswell doesn't have any port selection bits for the panel > + * power sequence any more. */ > + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { > + if (is_cpu_edp(intel_dp)) > + pp_on |= PANEL_POWER_PORT_DP_A; > + else > + pp_on |= PANEL_POWER_PORT_DP_D; > + } > + > + I915_WRITE(PCH_PP_ON_DELAYS, pp_on); > + I915_WRITE(PCH_PP_OFF_DELAYS, pp_off); > + I915_WRITE(PCH_PP_DIVISOR, pp_div); > + > > DRM_DEBUG_KMS("panel power up delay %d, power down delay %d, power cycle delay %d\n", > intel_dp->panel_power_up_delay, intel_dp->panel_power_down_delay, > @@ -2723,6 +2767,11 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) > > DRM_DEBUG_KMS("backlight on delay %d, off delay %d\n", > intel_dp->backlight_on_delay, intel_dp->backlight_off_delay); > + > + DRM_DEBUG_KMS("panel power sequencer register settings: PP_ON %#x, PP_OFF %#x, PP_DIV %#x\n", > + I915_READ(PCH_PP_ON_DELAYS), > + I915_READ(PCH_PP_OFF_DELAYS), > + I915_READ(PCH_PP_DIVISOR)); > } > > intel_dp_i2c_init(intel_dp, intel_connector, name); I really hate the inline macros, but that's just me. It might be good to factor out this stuff into a separate function too (I see you do that in a later patch). Assuming you take care of the above. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Tue, Oct 23, 2012 at 12:04 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > If the current field is not zero and doesn't match the VBT, we might > add a debug statement. It could indicate a BIOS programmed value that > didn't involve a VBT update (I can imagine some vendors might do > this). Overwriting the current with the different VBT value may lead > to breakage or sub-optimal timings. We dump the current values (read out from the PP regs), the vbt values and now also the new values we write back into the regs. That not good enough? -Daniel
On Tue, 23 Oct 2012 09:23:00 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Tue, Oct 23, 2012 at 12:04 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > If the current field is not zero and doesn't match the VBT, we might > > add a debug statement. It could indicate a BIOS programmed value that > > didn't involve a VBT update (I can imagine some vendors might do > > this). Overwriting the current with the different VBT value may lead > > to breakage or sub-optimal timings. > > We dump the current values (read out from the PP regs), the vbt values > and now also the new values we write back into the regs. That not good > enough? I suppose, it just means more digging if there's a mismatch.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b428fbb..3ecd8c3 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4018,6 +4018,11 @@ #define PANEL_LIGHT_ON_DELAY_SHIFT 0 #define PCH_PP_OFF_DELAYS 0xc720c +#define PANEL_POWER_PORT_SELECT_MASK (0x3 << 30) +#define PANEL_POWER_PORT_LVDS (0 << 30) +#define PANEL_POWER_PORT_DP_A (1 << 30) +#define PANEL_POWER_PORT_DP_C (2 << 30) +#define PANEL_POWER_PORT_DP_D (3 << 30) #define PANEL_POWER_DOWN_DELAY_MASK (0x1fff0000) #define PANEL_POWER_DOWN_DELAY_SHIFT 16 #define PANEL_LIGHT_OFF_DELAY_MASK (0x1fff) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index f2c9ea6..265cec1 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2671,20 +2671,18 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) /* Cache some DPCD data in the eDP case */ if (is_edp(intel_dp)) { - struct edp_power_seq cur, vbt; - u32 pp_on, pp_off, pp_div; + struct edp_power_seq cur, vbt, spec, final; + u32 pp_on, pp_off, pp_div, pp; + + /* Workaround: Need to write PP_CONTROL with the unlock key as + * the very first thing. */ + pp = ironlake_get_pp_control(dev_priv); + I915_WRITE(PCH_PP_CONTROL, pp); pp_on = I915_READ(PCH_PP_ON_DELAYS); pp_off = I915_READ(PCH_PP_OFF_DELAYS); pp_div = I915_READ(PCH_PP_DIVISOR); - if (!pp_on || !pp_off || !pp_div) { - DRM_INFO("bad panel power sequencing delays, disabling panel\n"); - intel_dp_encoder_destroy(&intel_dp->base.base); - intel_dp_destroy(&intel_connector->base); - return; - } - /* Pull timing values out of registers */ cur.t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >> PANEL_POWER_UP_DELAY_SHIFT; @@ -2706,16 +2704,62 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) vbt = dev_priv->edp.pps; + /* Upper limits from eDP 1.3 spec. Note that we the clunky units + * of our hw here, which are all in 100usec. */ + spec.t1_t3 = 210 * 10; + spec.t8 = 50 * 10; /* no limit for t8, use t7 instead */ + spec.t9 = 50 * 10; /* no limit for t9, make it symmetric with t8 */ + spec.t10 = 500 * 10; + /* This one is special and actually in units of 100ms, but zero + * based in the hw (so we need to add 100 ms). But the sw vbt + * table multiplies it with 1000 to make it in units of 100usec, + * 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); -#define get_delay(field) ((max(cur.field, vbt.field) + 9) / 10) - + /* Use the max of the register setttings and vbt. If both are + * unset, fall back to the spec limits. */ +#define assign_final(field) final.field = (max(cur.field, vbt.field) == 0 ? \ + spec.field : \ + max(cur.field, vbt.field)) + assign_final(t1_t3); + assign_final(t8); + assign_final(t9); + assign_final(t10); + assign_final(t11_t12); +#undef assign_final + +#define get_delay(field) (DIV_ROUND_UP(final.field, 10)) intel_dp->panel_power_up_delay = get_delay(t1_t3); intel_dp->backlight_on_delay = get_delay(t8); intel_dp->backlight_off_delay = get_delay(t9); intel_dp->panel_power_down_delay = get_delay(t10); intel_dp->panel_power_cycle_delay = get_delay(t11_t12); +#undef get_delay + + /* And finally store the new values in the power sequencer. */ + pp_on = (final.t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) | + (final.t8 << PANEL_LIGHT_ON_DELAY_SHIFT); + pp_off = (final.t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) | + (final.t10 << PANEL_POWER_DOWN_DELAY_SHIFT); + pp_div = (pp_div & PP_REFERENCE_DIVIDER_MASK) | + (DIV_ROUND_UP(final.t11_t12, 1000) << PANEL_POWER_CYCLE_DELAY_SHIFT); + + /* Haswell doesn't have any port selection bits for the panel + * power sequence any more. */ + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) { + if (is_cpu_edp(intel_dp)) + pp_on |= PANEL_POWER_PORT_DP_A; + else + pp_on |= PANEL_POWER_PORT_DP_D; + } + + I915_WRITE(PCH_PP_ON_DELAYS, pp_on); + I915_WRITE(PCH_PP_OFF_DELAYS, pp_off); + I915_WRITE(PCH_PP_DIVISOR, pp_div); + DRM_DEBUG_KMS("panel power up delay %d, power down delay %d, power cycle delay %d\n", intel_dp->panel_power_up_delay, intel_dp->panel_power_down_delay, @@ -2723,6 +2767,11 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) DRM_DEBUG_KMS("backlight on delay %d, off delay %d\n", intel_dp->backlight_on_delay, intel_dp->backlight_off_delay); + + DRM_DEBUG_KMS("panel power sequencer register settings: PP_ON %#x, PP_OFF %#x, PP_DIV %#x\n", + I915_READ(PCH_PP_ON_DELAYS), + I915_READ(PCH_PP_OFF_DELAYS), + I915_READ(PCH_PP_DIVISOR)); } intel_dp_i2c_init(intel_dp, intel_connector, name);
3 changes: - If a given value is unset, use the maximal limits from the eDP spec. - Write back the new values, since otherwise the panel power sequencing hw will not dtrt. - Revert the early bail-out in case the register values are unset. The last change reverts commit bfa3384a9a84aaaa59443bbd776c142e7dba4b0f Author: Jesse Barnes <jbarnes@virtuousgeek.org> Date: Tue Apr 10 11:58:04 2012 -0700 drm/i915: check PPS regs for sanity when using eDP v2: - Unlock the PP regs as the very first thing. This is a required w/a for cpu eDP on port A, and generally a good idea. - Fixup the panel power control port selection bits. v3: Paulo Zanoni noticed that I've fumbled the computation of the spec limit values. Fix them up. We've also noticed that the t8/t9 values in the vbt/bios-programmed pp are much larger than any limits. My guess is that this is to conceal any backlight enable/disable delays. So by using the much shorter limits from the spec, which only concerns the sink, we risk that we might display before the backlight is fully on, or disable the output while the backlight still has afterglow. I've figured I don't care too much, since this will only happen when both the pp regs are not programmed, and the vbt tables don't contain anything useful. v4: Don't set the port selection bits on hsw/LPT, they don't exist any more. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_reg.h | 5 +++ drivers/gpu/drm/i915/intel_dp.c | 71 ++++++++++++++++++++++++++++++++++------- 2 files changed, 65 insertions(+), 11 deletions(-)