On Sat, 20 Oct 2012 20:57:41 +0200
Daniel Vetter <daniel.vetter at 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 at 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 at 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 at virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
