Re: [PATCH 1/5] drm/i915: make edp panel power sequence setup more robust

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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>

-- 
Jesse Barnes, Intel Open Source Technology Center