On Sat, 20 Oct 2012 20:57:41 +0200 Daniel Vetter <[email protected]> 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 <[email protected]> > 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 <[email protected]> > --- > 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 <[email protected]> -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
