drm/i915: eDP cleanup patch series -- fixes SNB MacBook Air

drm/i915: eDP cleanup patch series -- fixes SNB MacBook Air

[Keith Packard]

Here's a sequence of eDP patches which are necessary to make the
driver work on the Sandybridge MacBook Air

The key trick was to make sure that the eDP power was applied before
trying to communicate with the device.

drm/i915: eDP cleanup patch series -- fixes SNB MacBook Air

[Keith Packard]

Here's a sequence of eDP patches which are necessary to make the
driver work on the Sandybridge MacBook Air

The key trick was to make sure that the eDP power was applied before
trying to communicate with the device.

[PATCH 1/9] drm/i915: Enable digital port hotplug on PCH systems

[Keith Packard]

We were relying on the BIOS to set these bits, which doesn't always
happen.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |    5 ++++-
 drivers/gpu/drm/i915/intel_display.c |   12 ++++++++++++
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 542453f..b7fbb74 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2903,12 +2903,13 @@
 #define SDEIER  0xc400c
 
 /* digital port hotplug */
-#define PCH_PORT_HOTPLUG        0xc4030
+#define PCH_PORT_HOTPLUG        0xc4030		/* SHOTPLUG_CTL */
 #define PORTD_HOTPLUG_ENABLE            (1 << 20)
 #define PORTD_PULSE_DURATION_2ms        (0)
 #define PORTD_PULSE_DURATION_4_5ms      (1 << 18)
 #define PORTD_PULSE_DURATION_6ms        (2 << 18)
 #define PORTD_PULSE_DURATION_100ms      (3 << 18)
+#define PORTD_PULSE_DURATION_MASK	(3 << 18)
 #define PORTD_HOTPLUG_NO_DETECT         (0)
 #define PORTD_HOTPLUG_SHORT_DETECT      (1 << 16)
 #define PORTD_HOTPLUG_LONG_DETECT       (1 << 17)
@@ -2917,6 +2918,7 @@
 #define PORTC_PULSE_DURATION_4_5ms      (1 << 10)
 #define PORTC_PULSE_DURATION_6ms        (2 << 10)
 #define PORTC_PULSE_DURATION_100ms      (3 << 10)
+#define PORTC_PULSE_DURATION_MASK	(3 << 10)
 #define PORTC_HOTPLUG_NO_DETECT         (0)
 #define PORTC_HOTPLUG_SHORT_DETECT      (1 << 8)
 #define PORTC_HOTPLUG_LONG_DETECT       (1 << 9)
@@ -2925,6 +2927,7 @@
 #define PORTB_PULSE_DURATION_4_5ms      (1 << 2)
 #define PORTB_PULSE_DURATION_6ms        (2 << 2)
 #define PORTB_PULSE_DURATION_100ms      (3 << 2)
+#define PORTB_PULSE_DURATION_MASK	(3 << 2)
 #define PORTB_HOTPLUG_NO_DETECT         (0)
 #define PORTB_HOTPLUG_SHORT_DETECT      (1 << 0)
 #define PORTB_HOTPLUG_LONG_DETECT       (1 << 1)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9fb4a40..54403dd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7151,6 +7151,18 @@ static void intel_setup_outputs(struct drm_device *dev)
 		I915_WRITE(PFIT_CONTROL, 0);
 	}
 
+	/* Enable digital port hotplug detect on PCH hardware */
+	if (HAS_PCH_SPLIT(dev)) {
+		u32	hotplug;
+
+		hotplug = I915_READ(PCH_PORT_HOTPLUG);
+		hotplug &= ~(PORTD_PULSE_DURATION_MASK|PORTC_PULSE_DURATION_MASK|PORTB_PULSE_DURATION_MASK);
+		hotplug |= PORTD_HOTPLUG_ENABLE | PORTD_PULSE_DURATION_2ms;
+		hotplug |= PORTC_HOTPLUG_ENABLE | PORTC_PULSE_DURATION_2ms;
+		hotplug |= PORTB_HOTPLUG_ENABLE | PORTB_PULSE_DURATION_2ms;
+		I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
+	}
+
 	if (HAS_PCH_SPLIT(dev)) {
 		dpd_is_edp = intel_dpd_is_edp(dev);
 
-- 
1.7.6.3

[PATCH 2/9] drm/i915: Remove extra 300ms delay during eDP mode setting

[Keith Packard]

There's no reason to enforce a 300ms delay during eDP mode setting.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 44fef5e..f0cfb6b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -903,13 +903,6 @@ static void ironlake_edp_backlight_on (struct drm_device *dev)
 	u32 pp;
 
 	DRM_DEBUG_KMS("\n");
-	/*
-	 * If we enable the backlight right away following a panel power
-	 * on, we may see slight flicker as the panel syncs with the eDP
-	 * link.  So delay a bit to make sure the image is solid before
-	 * allowing it to appear.
-	 */
-	msleep(300);
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp |= EDP_BLC_ENABLE;
 	I915_WRITE(PCH_PP_CONTROL, pp);
-- 
1.7.6.3

[PATCH 3/9] drm/i915: Only use VBT panel mode on eDP if no EDID is found

[Keith Packard]

We're going to assume that EDID is more reliable than the VBT tables
for eDP panels, which is notably true on MacBook machines where the
VBT contains completely bogus data.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f0cfb6b..8ab2a88 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1748,7 +1748,16 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 
 	/* if eDP has no EDID, try to use fixed panel mode from VBT */
 	if (is_edp(intel_dp)) {
-		if (dev_priv->panel_fixed_mode != NULL) {
+		/* initialize panel mode from VBT if available for eDP */
+		if (dev_priv->panel_fixed_mode == NULL && dev_priv->lfp_lvds_vbt_mode != NULL) {
+			dev_priv->panel_fixed_mode =
+				drm_mode_duplicate(dev, dev_priv->lfp_lvds_vbt_mode);
+			if (dev_priv->panel_fixed_mode) {
+				dev_priv->panel_fixed_mode->type |=
+					DRM_MODE_TYPE_PREFERRED;
+			}
+		}
+		if (dev_priv->panel_fixed_mode) {
 			struct drm_display_mode *mode;
 			mode = drm_mode_duplicate(dev, dev_priv->panel_fixed_mode);
 			drm_mode_probed_add(connector, mode);
@@ -2061,15 +2070,6 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
 	if (is_edp(intel_dp)) {
-		/* initialize panel mode from VBT if available for eDP */
-		if (dev_priv->lfp_lvds_vbt_mode) {
-			dev_priv->panel_fixed_mode =
-				drm_mode_duplicate(dev, dev_priv->lfp_lvds_vbt_mode);
-			if (dev_priv->panel_fixed_mode) {
-				dev_priv->panel_fixed_mode->type |=
-					DRM_MODE_TYPE_PREFERRED;
-			}
-		}
 		dev_priv->int_edp_connector = connector;
 		intel_panel_setup_backlight(dev);
 	}
-- 
1.7.6.3

[PATCH 4/9] drm/i915: Check eDP power when doing aux channel communications

[Keith Packard]

Verify that the eDP VDD is on, either with the panel being on or with
the VDD force-on bit being set.

This demonstrates that in many instances, VDD is not on when needed,
which leads to failed EDID communications.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8ab2a88..3b29a6f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -279,6 +279,24 @@ intel_hrawclk(struct drm_device *dev)
 	}
 }
 
+static void
+intel_dp_check_edp(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 pp_status, pp_control;
+	if (!is_edp(intel_dp))
+		return;
+	pp_status = I915_READ(PCH_PP_STATUS);
+	pp_control = I915_READ(PCH_PP_CONTROL);
+	if ((pp_status & PP_ON) == 0 && (pp_control & EDP_FORCE_VDD) == 0) {
+		WARN(1, "eDP powered off while attempting aux channel communication.\n");
+		DRM_DEBUG_KMS("Status 0x%08x Control 0x%08x\n",
+			      pp_status,
+			      I915_READ(PCH_PP_CONTROL));
+	}
+}
+
 static int
 intel_dp_aux_ch(struct intel_dp *intel_dp,
 		uint8_t *send, int send_bytes,
@@ -295,6 +313,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	uint32_t aux_clock_divider;
 	int try, precharge;
 
+	intel_dp_check_edp(intel_dp);
 	/* The clock divider is based off the hrawclk,
 	 * and would like to run at 2MHz. So, take the
 	 * hrawclk value and divide by 2 and use that
@@ -408,6 +427,7 @@ intel_dp_aux_native_write(struct intel_dp *intel_dp,
 	int msg_bytes;
 	uint8_t	ack;
 
+	intel_dp_check_edp(intel_dp);
 	if (send_bytes > 16)
 		return -1;
 	msg[0] = AUX_NATIVE_WRITE << 4;
@@ -450,6 +470,7 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 	uint8_t ack;
 	int ret;
 
+	intel_dp_check_edp(intel_dp);
 	msg[0] = AUX_NATIVE_READ << 4;
 	msg[1] = address >> 8;
 	msg[2] = address & 0xff;
@@ -493,6 +514,7 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 	int reply_bytes;
 	int ret;
 
+	intel_dp_check_edp(intel_dp);
 	/* Set up the command byte */
 	if (mode & MODE_I2C_READ)
 		msg[0] = AUX_I2C_READ << 4;
-- 
1.7.6.3

[PATCH 5/9] drm/i915: Unlock PCH_PP_CONTROL always

[Keith Packard]

Avoid any question about locked registers by just writing the unlock
pattern with every write to the register.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_reg.h |    1 +
 drivers/gpu/drm/i915/intel_dp.c |   14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b7fbb74..5596e8e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3311,6 +3311,7 @@
 #define PCH_PP_STATUS		0xc7200
 #define PCH_PP_CONTROL		0xc7204
 #define  PANEL_UNLOCK_REGS	(0xabcd << 16)
+#define  PANEL_UNLOCK_MASK	(0xffff << 16)
 #define  EDP_FORCE_VDD		(1 << 3)
 #define  EDP_BLC_ENABLE		(1 << 2)
 #define  PANEL_POWER_RESET	(1 << 1)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3b29a6f..a983d0f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -840,6 +840,8 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 		msleep(dev_priv->panel_t3);
 
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 	pp |= EDP_FORCE_VDD;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
@@ -852,6 +854,8 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 	u32 pp;
 
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 	pp &= ~EDP_FORCE_VDD;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
@@ -871,13 +875,15 @@ static bool ironlake_edp_panel_on (struct intel_dp *intel_dp)
 		return true;
 
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 
 	/* ILK workaround: disable reset around power sequence */
 	pp &= ~PANEL_POWER_RESET;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
 
-	pp |= PANEL_UNLOCK_REGS | POWER_TARGET_ON;
+	pp |= POWER_TARGET_ON;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
 
@@ -900,6 +906,8 @@ static void ironlake_edp_panel_off (struct drm_device *dev)
 		PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK;
 
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 
 	/* ILK workaround: disable reset around power sequence */
 	pp &= ~PANEL_POWER_RESET;
@@ -926,6 +934,8 @@ static void ironlake_edp_backlight_on (struct drm_device *dev)
 
 	DRM_DEBUG_KMS("\n");
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 	pp |= EDP_BLC_ENABLE;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 }
@@ -937,6 +947,8 @@ static void ironlake_edp_backlight_off (struct drm_device *dev)
 
 	DRM_DEBUG_KMS("\n");
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 	pp &= ~EDP_BLC_ENABLE;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 }
-- 
1.7.6.3

[PATCH 6/9] drm/i915: Make sure eDP power is on before using aux channel

[Keith Packard]

The eDP panel may not be able to respond to aux channel communications
unless it has power supplied. During mode setting, power may be
cut-off during panel power sequencing. Make sure that any aux channel
communications will work by forcing vdd power active as needed.

This also delays after turning power on and off to ensure that the
panel is keeping up.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   84 +++++++++++++++++++++++++++++---------
 1 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a983d0f..41b1e05 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -595,10 +595,15 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 	return -EREMOTEIO;
 }
 
+static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp);
+static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp);
+
 static int
 intel_dp_i2c_init(struct intel_dp *intel_dp,
 		  struct intel_connector *intel_connector, const char *name)
 {
+	int	ret;
+
 	DRM_DEBUG_KMS("i2c_init %s\n", name);
 	intel_dp->algo.running = false;
 	intel_dp->algo.address = 0;
@@ -612,7 +617,10 @@ intel_dp_i2c_init(struct intel_dp *intel_dp,
 	intel_dp->adapter.algo_data = &intel_dp->algo;
 	intel_dp->adapter.dev.parent = &intel_connector->base.kdev;
 
-	return i2c_dp_aux_add_bus(&intel_dp->adapter);
+	ironlake_edp_panel_vdd_on(intel_dp);
+	ret = i2c_dp_aux_add_bus(&intel_dp->adapter);
+	ironlake_edp_panel_vdd_off(intel_dp);
+	return ret;
 }
 
 static bool
@@ -830,14 +838,20 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 pp;
+	u32 pp, pp_status;
 
+	if (!is_edp(intel_dp))
+		return;
+	DRM_DEBUG_KMS("Turn eDP VDD on\n");
 	/*
 	 * If the panel wasn't on, make sure there's not a currently
 	 * active PP sequence before enabling AUX VDD.
 	 */
-	if (!(I915_READ(PCH_PP_STATUS) & PP_ON))
+	pp_status = I915_READ(PCH_PP_STATUS);
+	if (!(pp_status & PP_ON)) {
+		DRM_DEBUG_KMS("eDP VDD was not on\n");
 		msleep(dev_priv->panel_t3);
+	}
 
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
@@ -845,6 +859,9 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	pp |= EDP_FORCE_VDD;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
+	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
+		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
+	msleep(1000);
 }
 
 static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
@@ -853,6 +870,9 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp;
 
+	if (!is_edp(intel_dp))
+		return;
+	DRM_DEBUG_KMS("Turn eDP VDD off\n");
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
@@ -862,6 +882,9 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 
 	/* Make sure sequencer is idle before allowing subsequent activity */
 	msleep(dev_priv->panel_t12);
+	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
+		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
+	msleep(1000);
 }
 
 /* Returns true if the panel was already on when called */
@@ -1016,7 +1039,9 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
 	struct drm_device *dev = encoder->dev;
 
 	/* Wake up the sink first */
+	ironlake_edp_panel_vdd_on(intel_dp);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
+	ironlake_edp_panel_vdd_off(intel_dp);
 
 	if (is_edp(intel_dp)) {
 		ironlake_edp_backlight_off(dev);
@@ -1034,21 +1059,17 @@ static void intel_dp_commit(struct drm_encoder *encoder)
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	struct drm_device *dev = encoder->dev;
 
-	if (is_edp(intel_dp))
-		ironlake_edp_panel_vdd_on(intel_dp);
-
+	ironlake_edp_panel_vdd_on(intel_dp);
 	intel_dp_start_link_train(intel_dp);
 
-	if (is_edp(intel_dp)) {
+	if (is_edp(intel_dp))
 		ironlake_edp_panel_on(intel_dp);
-		ironlake_edp_panel_vdd_off(intel_dp);
-	}
 
 	intel_dp_complete_link_train(intel_dp);
 
 	if (is_edp(intel_dp))
 		ironlake_edp_backlight_on(dev);
-
+	ironlake_edp_panel_vdd_off(intel_dp);
 	intel_dp->dpms_mode = DRM_MODE_DPMS_ON;
 }
 
@@ -1060,6 +1081,7 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
 
+	ironlake_edp_panel_vdd_on(intel_dp);
 	if (mode != DRM_MODE_DPMS_ON) {
 		if (is_edp(intel_dp))
 			ironlake_edp_backlight_off(dev);
@@ -1070,20 +1092,17 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
 			ironlake_edp_pll_off(encoder);
 	} else {
-		if (is_edp(intel_dp))
-			ironlake_edp_panel_vdd_on(intel_dp);
 		intel_dp_sink_dpms(intel_dp, mode);
 		if (!(dp_reg & DP_PORT_EN)) {
 			intel_dp_start_link_train(intel_dp);
-			if (is_edp(intel_dp)) {
+			if (is_edp(intel_dp))
 				ironlake_edp_panel_on(intel_dp);
-				ironlake_edp_panel_vdd_off(intel_dp);
-			}
 			intel_dp_complete_link_train(intel_dp);
 		}
 		if (is_edp(intel_dp))
 			ironlake_edp_backlight_on(dev);
 	}
+	ironlake_edp_panel_vdd_off(intel_dp);
 	intel_dp->dpms_mode = mode;
 }
 
@@ -1710,6 +1729,31 @@ g4x_dp_detect(struct intel_dp *intel_dp)
 	return intel_dp_detect_dpcd(intel_dp);
 }
 
+static struct edid *
+intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct edid	*edid;
+
+	ironlake_edp_panel_vdd_on(intel_dp);
+	edid = drm_get_edid(connector, adapter);
+	ironlake_edp_panel_vdd_off(intel_dp);
+	return edid;
+}
+
+static int
+intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *adapter)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	int	ret;
+
+	ironlake_edp_panel_vdd_on(intel_dp);
+	ret = intel_ddc_get_modes(connector, adapter);
+	ironlake_edp_panel_vdd_off(intel_dp);
+	return ret;
+}
+
+
 /**
  * Uses CRT_HOTPLUG_EN and CRT_HOTPLUG_STAT to detect DP connection.
  *
@@ -1742,7 +1786,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	if (intel_dp->force_audio) {
 		intel_dp->has_audio = intel_dp->force_audio > 0;
 	} else {
-		edid = drm_get_edid(connector, &intel_dp->adapter);
+		edid = intel_dp_get_edid(connector, &intel_dp->adapter);
 		if (edid) {
 			intel_dp->has_audio = drm_detect_monitor_audio(edid);
 			connector->display_info.raw_edid = NULL;
@@ -1763,7 +1807,7 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 	/* We should parse the EDID data and find out if it has an audio sink
 	 */
 
-	ret = intel_ddc_get_modes(connector, &intel_dp->adapter);
+	ret = intel_dp_get_edid_modes(connector, &intel_dp->adapter);
 	if (ret) {
 		if (is_edp(intel_dp) && !dev_priv->panel_fixed_mode) {
 			struct drm_display_mode *newmode;
@@ -1808,7 +1852,7 @@ intel_dp_detect_audio(struct drm_connector *connector)
 	struct edid *edid;
 	bool has_audio = false;
 
-	edid = drm_get_edid(connector, &intel_dp->adapter);
+	edid = intel_dp_get_edid(connector, &intel_dp->adapter);
 	if (edid) {
 		has_audio = drm_detect_monitor_audio(edid);
 
@@ -2068,8 +2112,6 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			break;
 	}
 
-	intel_dp_i2c_init(intel_dp, intel_connector, name);
-
 	/* Cache some DPCD data in the eDP case */
 	if (is_edp(intel_dp)) {
 		bool ret;
@@ -2101,6 +2143,8 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 		}
 	}
 
+	intel_dp_i2c_init(intel_dp, intel_connector, name);
+
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
 	if (is_edp(intel_dp)) {
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Make sure eDP power is on before using aux channel

[Jesse Barnes]

On Mon, 19 Sep 2011 15:22:00 -0700
Keith Packard <keithp@keithp.com> wrote:

> The eDP panel may not be able to respond to aux channel communications
> unless it has power supplied. During mode setting, power may be
> cut-off during panel power sequencing. Make sure that any aux channel
> communications will work by forcing vdd power active as needed.
> 
> This also delays after turning power on and off to ensure that the
> panel is keeping up.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

This one mixes up lots of cleanups plus the EDID read with the power
changes.  I'm worried about the VDD smashing as well; we have lots of
bugs in the PPS hardware around VDD vs full PPS.  We need to make sure
appropriate delays are in place when transitioning from one to another.

In what paths are we trying to do accesses without power applied?
Looks like mainly edid?

I see the next patch handles the timing stuff, I assume it's ok.

Jesse

Re: [PATCH 6/9] drm/i915: Make sure eDP power is on before using aux channel

[Jesse Barnes]

On Mon, 19 Sep 2011 15:22:00 -0700
Keith Packard <keithp@keithp.com> wrote:

> The eDP panel may not be able to respond to aux channel communications
> unless it has power supplied. During mode setting, power may be
> cut-off during panel power sequencing. Make sure that any aux channel
> communications will work by forcing vdd power active as needed.
> 
> This also delays after turning power on and off to ensure that the
> panel is keeping up.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

This one mixes up lots of cleanups plus the EDID read with the power
changes.  I'm worried about the VDD smashing as well; we have lots of
bugs in the PPS hardware around VDD vs full PPS.  We need to make sure
appropriate delays are in place when transitioning from one to another.

In what paths are we trying to do accesses without power applied?
Looks like mainly edid?

I see the next patch handles the timing stuff, I assume it's ok.

Jesse

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Make sure eDP power is on before using aux channel

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 4498 bytes --]

On Wed, 21 Sep 2011 09:20:01 +0530, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> This one mixes up lots of cleanups plus the EDID read with the power
> changes.

I think the cleanups are;

 1) edp checks inside vdd_on and vdd_off to make the other code a bit
    easier to read.

 2) Hold VDD on until the end of dp_commit. I think this isn't necessary
    and could be removed -- once the panel is on, we don't need to hold
    vdd up.

 3) Hold VDD up through the whole dpms sequence. Also probably
    unnecessary.

 4) Move intel_dp_i2c_init past the computation of the power sequencing
    delays. Necessary as i2c_init makes an i2c transaction, thus
    powering up the panel.

I can remove the middle two and split the others out if you like.

> I'm worried about the VDD smashing as well; we have lots of
> bugs in the PPS hardware around VDD vs full PPS.

Are you worried that we should never have VDD up while running a panel
power sequence? As far as I can tell from the eDP specs, bringing VDD up
is part of the normal PPS, and the delay from VDD up to other panel
sequencing is shorter (T1+T2) than the delay to start using the aux channel
that I used in the later patch (T1+T2+T3).

One thing I learned for certain -- we don't want a synchronous delay
between turning the panel off and back on. The required interval between
these two operations is in units of 100ms; my machine spends 600ms doing
this; if we end up doing this in the middle of a mode set, it's gonna be
painful.

Can we replace any of the current VDD hacks with a full PPS? I can
easily imagine moving the call to ironlake_edp_panel_on to
intel_dp_prepare, except that if if the mode_set fails, dp_commit will
not be called (that looks like a potential source of failure at the DRM
level to me).

Getting the panel turned on complete as early as possible seems like a
good idea, instead of fussing around with the VDD force bits.

Alternatively, we can eliminate use of the VDD force hack and do a full
PPS and simply use the delayed work proc to turn that off when the
screen goes idle.

> We need to make sure appropriate delays are in place when
> transitioning from one to another.

As you can see, I stuck 1000ms delays in the vdd_on and vdd_off
functions to ensure that there was 'sufficient' delay in these cases. I
didn't touch the existing delays for regular panel_on/panel_off. I think
that the later patch, which computes 'correct' delays and applies those
uniformly should ensure that we're always waiting long enough.

The later patches track the jiffies value when the panel was turned off
and then wait the appropriate amount of time before turning it back
on.

> In what paths are we trying to do accesses without power applied?
> Looks like mainly edid?

(the previously broken cases are marked with '*')

* intel_dp_i2c_init

        This probes the i2c bus to see if there's someone home. Failing
        to have VDD up at this point causes a timeout, but no other
        obvious problem.

        As you can see, I moved the call to this function after the
        computation of the power sequencing delay values.

* intel_dp_prepare

        This wakes up the eDP panel out of a low power DPMS state. I
        think I'd actually like to leave VDD high starting at this point
        and ending after the panel is running.

intel_dp_commit

        Calls intel_dp_start_link_train. before ironlake_edp_panel_on.
        I think the call to ironlake_edp_panel_vdd_off could move back
        where it was, right after ironlake_edp_panel_on.

intel_dp_dpms

*       In the DPMS_ON case, this calls intel_dp_sink_dpms and
        intel_dp_start_link_train before calling
        ironlake_edp_panel_on. I wonder if we shouldn't just turn the
        panel on instead?

        In the DPMS_OFF case, I don't think we need to mess with vdd at
        all; the panel is definitely running until
        ironlake_edp_panel_off is called, at which point we're done
        using the aux channel.

* intel_dp_get_edid,
* intel_dp_get_edid_modes

        These are called while the display is off (sometimes).

> I see the next patch handles the timing stuff, I assume it's ok.

I hope so; the actual timing values required by the eDP spec aren't
available, so I fudged by choosing ones that were at least as long.

Thanks for taking a look at the code.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Make sure eDP power is on before using aux channel

[Jesse Barnes]

On Tue, 20 Sep 2011 21:45:54 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Wed, 21 Sep 2011 09:20:01 +0530, Jesse Barnes
> <jbarnes@virtuousgeek.org> wrote:
> 
> > This one mixes up lots of cleanups plus the EDID read with the power
> > changes.
> 
> I think the cleanups are;
> 
>  1) edp checks inside vdd_on and vdd_off to make the other code a bit
>     easier to read.
> 
>  2) Hold VDD on until the end of dp_commit. I think this isn't
> necessary and could be removed -- once the panel is on, we don't need
> to hold vdd up.
> 
>  3) Hold VDD up through the whole dpms sequence. Also probably
>     unnecessary.
> 
>  4) Move intel_dp_i2c_init past the computation of the power
> sequencing delays. Necessary as i2c_init makes an i2c transaction,
> thus powering up the panel.
> 
> I can remove the middle two and split the others out if you like.

Yeah that sounds good.  (2) and (3) are ok cleanups, but it would be
best if they were a separate patch just in case the subtle timing
change breaks the panel power sequencing state machine.

> > I'm worried about the VDD smashing as well; we have lots of
> > bugs in the PPS hardware around VDD vs full PPS.
> 
> Are you worried that we should never have VDD up while running a panel
> power sequence? As far as I can tell from the eDP specs, bringing VDD
> up is part of the normal PPS, and the delay from VDD up to other panel
> sequencing is shorter (T1+T2) than the delay to start using the aux
> channel that I used in the later patch (T1+T2+T3).

No I think:
  1) VDD AUX override on
  2) PPS on
  3) (delay)
  4) VDD AUX override off
is safe, I'm just worried about the timing of step (3).

> One thing I learned for certain -- we don't want a synchronous delay
> between turning the panel off and back on. The required interval
> between these two operations is in units of 100ms; my machine spends
> 600ms doing this; if we end up doing this in the middle of a mode
> set, it's gonna be painful.

Agree, speeding that up would be nice.

> Can we replace any of the current VDD hacks with a full PPS? I can
> easily imagine moving the call to ironlake_edp_panel_on to
> intel_dp_prepare, except that if if the mode_set fails, dp_commit will
> not be called (that looks like a potential source of failure at the
> DRM level to me).

Ah yeah this brings back the memories (or is it PTS?).

To fix both PCH eDP and CPU eDP in the past, I did have a version that
only used full PPS and not VDD AUX override.  So it is possible, but
VDD AUX is a little cleaner since it allows us to keep the registers
locked potentially (theoretically we only actually want to unlock to
handle CPU eDP PLL enable/disable bugs and flicker-free panel fitter
downscaling).

But since we unlock unconditionally now, using full PPS would be ok.

> Getting the panel turned on complete as early as possible seems like a
> good idea, instead of fussing around with the VDD force bits.

Though we will have to shut it down still; PPS on to get AUX data and
EDID, then off while we program the mode and train DP, then PPS on
again.  So I'm not sure it would save much.

> Alternatively, we can eliminate use of the VDD force hack and do a
> full PPS and simply use the delayed work proc to turn that off when
> the screen goes idle.

Yeah, I'm liking your delayed work much better now...  Bring up the
panel early and then just modify the shutdown timeout at various points
to make sure it stays up from module_init all the way through the final
mode set (so an initial timeout of 2s or so would probably be
sufficient).

Another potential optimization is to start trying AUX & i2c
transactions right after we apply VDD AUX override.  The panel will
come up much faster than the T* values imply most of the time (varies
by panel).  And polling the bits can get us into the actual panel
poking code much faster.

But I think the bottom line is to fix the EDID read (make sure the
panel is on) and the i2c stuff.  Everything else is just tasty gravy. :)

Also I think the change to prefer EDID over VBT is correct; afaik eDP
panels are required to have an EDID, so trusting that data over some
potentially untested VBT data is the right way to go.

Thanks,
Jesse

Re: [PATCH 6/9] drm/i915: Make sure eDP power is on before using aux channel

[Jesse Barnes]

On Tue, 20 Sep 2011 21:45:54 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Wed, 21 Sep 2011 09:20:01 +0530, Jesse Barnes
> <jbarnes@virtuousgeek.org> wrote:
> 
> > This one mixes up lots of cleanups plus the EDID read with the power
> > changes.
> 
> I think the cleanups are;
> 
>  1) edp checks inside vdd_on and vdd_off to make the other code a bit
>     easier to read.
> 
>  2) Hold VDD on until the end of dp_commit. I think this isn't
> necessary and could be removed -- once the panel is on, we don't need
> to hold vdd up.
> 
>  3) Hold VDD up through the whole dpms sequence. Also probably
>     unnecessary.
> 
>  4) Move intel_dp_i2c_init past the computation of the power
> sequencing delays. Necessary as i2c_init makes an i2c transaction,
> thus powering up the panel.
> 
> I can remove the middle two and split the others out if you like.

Yeah that sounds good.  (2) and (3) are ok cleanups, but it would be
best if they were a separate patch just in case the subtle timing
change breaks the panel power sequencing state machine.

> > I'm worried about the VDD smashing as well; we have lots of
> > bugs in the PPS hardware around VDD vs full PPS.
> 
> Are you worried that we should never have VDD up while running a panel
> power sequence? As far as I can tell from the eDP specs, bringing VDD
> up is part of the normal PPS, and the delay from VDD up to other panel
> sequencing is shorter (T1+T2) than the delay to start using the aux
> channel that I used in the later patch (T1+T2+T3).

No I think:
  1) VDD AUX override on
  2) PPS on
  3) (delay)
  4) VDD AUX override off
is safe, I'm just worried about the timing of step (3).

> One thing I learned for certain -- we don't want a synchronous delay
> between turning the panel off and back on. The required interval
> between these two operations is in units of 100ms; my machine spends
> 600ms doing this; if we end up doing this in the middle of a mode
> set, it's gonna be painful.

Agree, speeding that up would be nice.

> Can we replace any of the current VDD hacks with a full PPS? I can
> easily imagine moving the call to ironlake_edp_panel_on to
> intel_dp_prepare, except that if if the mode_set fails, dp_commit will
> not be called (that looks like a potential source of failure at the
> DRM level to me).

Ah yeah this brings back the memories (or is it PTS?).

To fix both PCH eDP and CPU eDP in the past, I did have a version that
only used full PPS and not VDD AUX override.  So it is possible, but
VDD AUX is a little cleaner since it allows us to keep the registers
locked potentially (theoretically we only actually want to unlock to
handle CPU eDP PLL enable/disable bugs and flicker-free panel fitter
downscaling).

But since we unlock unconditionally now, using full PPS would be ok.

> Getting the panel turned on complete as early as possible seems like a
> good idea, instead of fussing around with the VDD force bits.

Though we will have to shut it down still; PPS on to get AUX data and
EDID, then off while we program the mode and train DP, then PPS on
again.  So I'm not sure it would save much.

> Alternatively, we can eliminate use of the VDD force hack and do a
> full PPS and simply use the delayed work proc to turn that off when
> the screen goes idle.

Yeah, I'm liking your delayed work much better now...  Bring up the
panel early and then just modify the shutdown timeout at various points
to make sure it stays up from module_init all the way through the final
mode set (so an initial timeout of 2s or so would probably be
sufficient).

Another potential optimization is to start trying AUX & i2c
transactions right after we apply VDD AUX override.  The panel will
come up much faster than the T* values imply most of the time (varies
by panel).  And polling the bits can get us into the actual panel
poking code much faster.

But I think the bottom line is to fix the EDID read (make sure the
panel is on) and the i2c stuff.  Everything else is just tasty gravy. :)

Also I think the change to prefer EDID over VBT is correct; afaik eDP
panels are required to have an EDID, so trusting that data over some
potentially untested VBT data is the right way to go.

Thanks,
Jesse

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Make sure eDP power is on before using aux channel

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 3048 bytes --]

On Fri, 23 Sep 2011 08:25:13 +0530, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Yeah that sounds good.  (2) and (3) are ok cleanups, but it would be
> best if they were a separate patch just in case the subtle timing
> change breaks the panel power sequencing state machine.

Ok, I'll split things up into tiny patches then; easier to review, and
easier to try different combinations.

> No I think:
>   1) VDD AUX override on
>   2) PPS on
>   3) (delay)
>   4) VDD AUX override off
> is safe, I'm just worried about the timing of step (3).

Ok, that seems like a nice plan to me -- just doing the PPS on sequence,
and then setting VDD AUX off after PPS completes. That seems easy, and
avoids leaving VDD AUX once the panel is completely running.

> To fix both PCH eDP and CPU eDP in the past, I did have a version that
> only used full PPS and not VDD AUX override.  So it is possible, but
> VDD AUX is a little cleaner since it allows us to keep the registers
> locked potentially (theoretically we only actually want to unlock to
> handle CPU eDP PLL enable/disable bugs and flicker-free panel fitter
> downscaling).

I really don't care about leaving registers locked; we're not running
DOS anymore. Just doesn't make any sense to me.

> But since we unlock unconditionally now, using full PPS would be ok.

> Though we will have to shut it down still; PPS on to get AUX data and
> EDID, then off while we program the mode and train DP, then PPS on
> again.  So I'm not sure it would save much.

VDD AUX on or PPS has to be done to run the training anyways.

> Yeah, I'm liking your delayed work much better now...  Bring up the
> panel early and then just modify the shutdown timeout at various points
> to make sure it stays up from module_init all the way through the final
> mode set (so an initial timeout of 2s or so would probably be
> sufficient).

The question is whether to do PPS or use VDD AUX on to start
with.

> Another potential optimization is to start trying AUX & i2c
> transactions right after we apply VDD AUX override.  The panel will
> come up much faster than the T* values imply most of the time (varies
> by panel).  And polling the bits can get us into the actual panel
> poking code much faster.

While that's possible, it wasn't true on the MBA; it took almost the
full T3 interval after VDD AUX was on before the EDID came back.

> But I think the bottom line is to fix the EDID read (make sure the
> panel is on) and the i2c stuff.  Everything else is just tasty
> gravy. :)

Right, that is what fixed the MBA for me.

> Also I think the change to prefer EDID over VBT is correct; afaik eDP
> panels are required to have an EDID, so trusting that data over some
> potentially untested VBT data is the right way to go.

Especially when VBT comes from a BIOS emulation mess which just lies.

I'll try to get a new sequence done tomorrow with these
suggestions. Hope your Indian adventures are going well.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Make sure eDP power is on before using aux channel

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 3048 bytes --]

On Fri, 23 Sep 2011 08:25:13 +0530, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Yeah that sounds good.  (2) and (3) are ok cleanups, but it would be
> best if they were a separate patch just in case the subtle timing
> change breaks the panel power sequencing state machine.

Ok, I'll split things up into tiny patches then; easier to review, and
easier to try different combinations.

> No I think:
>   1) VDD AUX override on
>   2) PPS on
>   3) (delay)
>   4) VDD AUX override off
> is safe, I'm just worried about the timing of step (3).

Ok, that seems like a nice plan to me -- just doing the PPS on sequence,
and then setting VDD AUX off after PPS completes. That seems easy, and
avoids leaving VDD AUX once the panel is completely running.

> To fix both PCH eDP and CPU eDP in the past, I did have a version that
> only used full PPS and not VDD AUX override.  So it is possible, but
> VDD AUX is a little cleaner since it allows us to keep the registers
> locked potentially (theoretically we only actually want to unlock to
> handle CPU eDP PLL enable/disable bugs and flicker-free panel fitter
> downscaling).

I really don't care about leaving registers locked; we're not running
DOS anymore. Just doesn't make any sense to me.

> But since we unlock unconditionally now, using full PPS would be ok.

> Though we will have to shut it down still; PPS on to get AUX data and
> EDID, then off while we program the mode and train DP, then PPS on
> again.  So I'm not sure it would save much.

VDD AUX on or PPS has to be done to run the training anyways.

> Yeah, I'm liking your delayed work much better now...  Bring up the
> panel early and then just modify the shutdown timeout at various points
> to make sure it stays up from module_init all the way through the final
> mode set (so an initial timeout of 2s or so would probably be
> sufficient).

The question is whether to do PPS or use VDD AUX on to start
with.

> Another potential optimization is to start trying AUX & i2c
> transactions right after we apply VDD AUX override.  The panel will
> come up much faster than the T* values imply most of the time (varies
> by panel).  And polling the bits can get us into the actual panel
> poking code much faster.

While that's possible, it wasn't true on the MBA; it took almost the
full T3 interval after VDD AUX was on before the EDID came back.

> But I think the bottom line is to fix the EDID read (make sure the
> panel is on) and the i2c stuff.  Everything else is just tasty
> gravy. :)

Right, that is what fixed the MBA for me.

> Also I think the change to prefer EDID over VBT is correct; afaik eDP
> panels are required to have an EDID, so trusting that data over some
> potentially untested VBT data is the right way to go.

Especially when VBT comes from a BIOS emulation mess which just lies.

I'll try to get a new sequence done tomorrow with these
suggestions. Hope your Indian adventures are going well.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/9] drm/i915: Correct eDP panel power sequencing delay computations

[Keith Packard]

Store the panel power sequencing delays in the dp private structure,
rather than the global device structure. Who knows, maybe we'll get
more than one eDP device in the future.

Look at both the current hardware register settings and the VBT
specified panel power sequencing timings. Use the maximum of the two
delays, to make sure things work reliably. If there is no VBT data,
then those values will be initialized to zero, so we'll just use the
values as programmed in the hardware.

This patch computes power-up and power-down delays, rather than using
portions of the appropriate delay values as found in the hardware. The
eDP specified delay between raising VCC and communicating over the aux
channel includes both the power rise time (T1) and the aux channel
communication delay (T3). The eDP specified delay between powering
down the device and powering it back up includes both the power fall
time (T11) and the device idle time (T12).

>From the hardware, I'm taking the T3 value from the PP_OFF_DELAYS
Power_Down_delay value, which is actually documented to be the 'T3
time sequence' value used 'during power up'. There aren't separate T1
and T2 values, but there is a combined T1+T2 value in the PP_ON_DELAYS
register, so I use that instead.

VBT doesn't provide any values for T1 or T2, so we'll always just use
the hardware value for that.

The panel power up delay is thus T1 + T2 + T3, which should be
sufficient in all cases.

The panel power down delay is T1 + T2 + T12, using T1+T2 as a proxy
for T11, which isn't available anywhere.

On the macbook air I'm testing with, this yields a power-up delay of
over 200ms and a power-down delay of over 600ms. It all works now, but
we're frobbing these power controls several times during mode setting,
making the whole process take an awfully long time.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 -
 drivers/gpu/drm/i915/intel_dp.c |   56 ++++++++++++++++++++++++++++----------
 2 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7916bd9..bcdf58b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -672,7 +672,6 @@ typedef struct drm_i915_private {
 	unsigned int lvds_border_bits;
 	/* Panel fitter placement and size for Ironlake+ */
 	u32 pch_pf_pos, pch_pf_size;
-	int panel_t3, panel_t12;
 
 	struct drm_crtc *plane_to_crtc_mapping[2];
 	struct drm_crtc *pipe_to_crtc_mapping[2];
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 41b1e05..f1d6105 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -59,6 +59,8 @@ struct intel_dp {
 	bool is_pch_edp;
 	uint8_t	train_set[4];
 	uint8_t link_status[DP_LINK_STATUS_SIZE];
+	int panel_power_up_delay;
+	int panel_power_down_delay;
 };
 
 /**
@@ -848,10 +850,6 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	 * active PP sequence before enabling AUX VDD.
 	 */
 	pp_status = I915_READ(PCH_PP_STATUS);
-	if (!(pp_status & PP_ON)) {
-		DRM_DEBUG_KMS("eDP VDD was not on\n");
-		msleep(dev_priv->panel_t3);
-	}
 
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
@@ -861,7 +859,10 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	POSTING_READ(PCH_PP_CONTROL);
 	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
 		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
-	msleep(1000);
+	if (!(pp_status & PP_ON)) {
+		msleep(intel_dp->panel_power_up_delay);
+		DRM_DEBUG_KMS("eDP VDD was not on\n");
+	}
 }
 
 static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
@@ -881,10 +882,9 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 	POSTING_READ(PCH_PP_CONTROL);
 
 	/* Make sure sequencer is idle before allowing subsequent activity */
-	msleep(dev_priv->panel_t12);
 	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
 		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
-	msleep(1000);
+	msleep(intel_dp->panel_power_down_delay);
 }
 
 /* Returns true if the panel was already on when called */
@@ -922,8 +922,10 @@ static bool ironlake_edp_panel_on (struct intel_dp *intel_dp)
 	return false;
 }
 
-static void ironlake_edp_panel_off (struct drm_device *dev)
+static void ironlake_edp_panel_off(struct drm_encoder *encoder)
 {
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp, idle_off_mask = PP_ON | PP_SEQUENCE_MASK |
 		PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK;
@@ -940,6 +942,7 @@ static void ironlake_edp_panel_off (struct drm_device *dev)
 	pp &= ~POWER_TARGET_ON;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
+	msleep(intel_dp->panel_power_down_delay);
 
 	if (wait_for((I915_READ(PCH_PP_STATUS) & idle_off_mask) == 0, 5000))
 		DRM_ERROR("panel off wait timed out: 0x%08x\n",
@@ -1045,7 +1048,7 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
 
 	if (is_edp(intel_dp)) {
 		ironlake_edp_backlight_off(dev);
-		ironlake_edp_panel_off(dev);
+		ironlake_edp_panel_off(encoder);
 		if (!is_pch_edp(intel_dp))
 			ironlake_edp_pll_on(encoder);
 		else
@@ -1088,7 +1091,7 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 		intel_dp_sink_dpms(intel_dp, mode);
 		intel_dp_link_down(intel_dp);
 		if (is_edp(intel_dp))
-			ironlake_edp_panel_off(dev);
+			ironlake_edp_panel_off(encoder);
 		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
 			ironlake_edp_pll_off(encoder);
 	} else {
@@ -2115,16 +2118,39 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 	/* Cache some DPCD data in the eDP case */
 	if (is_edp(intel_dp)) {
 		bool ret;
-		u32 pp_on, pp_div;
+		u32 pp_on, pp_off, pp_div;
+		int current_t1_2;
+		int current_t3;
+		int current_t12;
+		int vbt_t3;
+		int vbt_t12;
 
 		pp_on = I915_READ(PCH_PP_ON_DELAYS);
+		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
 		pp_div = I915_READ(PCH_PP_DIVISOR);
 
 		/* Get T3 & T12 values (note: VESA not bspec terminology) */
-		dev_priv->panel_t3 = (pp_on & 0x1fff0000) >> 16;
-		dev_priv->panel_t3 /= 10; /* t3 in 100us units */
-		dev_priv->panel_t12 = pp_div & 0xf;
-		dev_priv->panel_t12 *= 100; /* t12 in 100ms units */
+
+		current_t1_2 = (pp_on & 0x1fff0000) >> 16;
+		current_t1_2 = (current_t1_2 + 9) / 10; /* t1+t2 in 100us units */
+
+		current_t3 = (pp_off & 0x1fff0000) >> 16;
+		current_t3 = (current_t3 + 9) / 10;	/* t3 in 100us units */
+
+		current_t12 = pp_div & 0xf;
+		current_t12 *= 100; /* t12 in 100ms units */
+
+		vbt_t3 = (dev_priv->edp.pps.t3 + 9) / 10;
+		vbt_t12 = (dev_priv->edp.pps.t12 + 9) / 10;
+		DRM_DEBUG_KMS("current t3 %d t12 %d\n",
+			      current_t3, current_t12);
+		DRM_DEBUG_KMS("VBT t3 %d t12 %d\n",
+			      vbt_t3, vbt_t12);
+
+		intel_dp->panel_power_up_delay = current_t1_2 + max(current_t3, vbt_t3);
+		intel_dp->panel_power_down_delay = current_t1_2 + max(current_t12, vbt_t12);
+		DRM_DEBUG_KMS("panel power up delay %d, power down delay %d\n",
+			      intel_dp->panel_power_up_delay, intel_dp->panel_power_down_delay);
 
 		ironlake_edp_panel_vdd_on(intel_dp);
 		ret = intel_dp_get_dpcd(intel_dp);
-- 
1.7.6.3

[PATCH 8/9] drm/i915: Move eDP panel fixed mode from dev_priv to intel_dp

[Keith Packard]

This value doesn't come directly from the VBT, and so is rather
specific to the particular DP output.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 -
 drivers/gpu/drm/i915/intel_dp.c |   35 ++++++++++++++++-------------------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bcdf58b..e6dd19e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -347,7 +347,6 @@ typedef struct drm_i915_private {
 	/* LVDS info */
 	int backlight_level;  /* restore backlight to this value */
 	bool backlight_enabled;
-	struct drm_display_mode *panel_fixed_mode;
 	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
 	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f1d6105..5bc30f9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -61,6 +61,7 @@ struct intel_dp {
 	uint8_t link_status[DP_LINK_STATUS_SIZE];
 	int panel_power_up_delay;
 	int panel_power_down_delay;
+	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
 };
 
 /**
@@ -202,16 +203,14 @@ intel_dp_mode_valid(struct drm_connector *connector,
 		    struct drm_display_mode *mode)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp));
 	int max_lanes = intel_dp_max_lane_count(intel_dp);
 
-	if (is_edp(intel_dp) && dev_priv->panel_fixed_mode) {
-		if (mode->hdisplay > dev_priv->panel_fixed_mode->hdisplay)
+	if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) {
+		if (mode->hdisplay > intel_dp->panel_fixed_mode->hdisplay)
 			return MODE_PANEL;
 
-		if (mode->vdisplay > dev_priv->panel_fixed_mode->vdisplay)
+		if (mode->vdisplay > intel_dp->panel_fixed_mode->vdisplay)
 			return MODE_PANEL;
 	}
 
@@ -630,22 +629,21 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		    struct drm_display_mode *adjusted_mode)
 {
 	struct drm_device *dev = encoder->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	int lane_count, clock;
 	int max_lane_count = intel_dp_max_lane_count(intel_dp);
 	int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
 	static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
 
-	if (is_edp(intel_dp) && dev_priv->panel_fixed_mode) {
-		intel_fixed_panel_mode(dev_priv->panel_fixed_mode, adjusted_mode);
+	if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) {
+		intel_fixed_panel_mode(intel_dp->panel_fixed_mode, adjusted_mode);
 		intel_pch_panel_fitting(dev, DRM_MODE_SCALE_FULLSCREEN,
 					mode, adjusted_mode);
 		/*
 		 * the mode->clock is used to calculate the Data&Link M/N
 		 * of the pipe. For the eDP the fixed clock should be used.
 		 */
-		mode->clock = dev_priv->panel_fixed_mode->clock;
+		mode->clock = intel_dp->panel_fixed_mode->clock;
 	}
 
 	for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
@@ -1812,35 +1810,34 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 
 	ret = intel_dp_get_edid_modes(connector, &intel_dp->adapter);
 	if (ret) {
-		if (is_edp(intel_dp) && !dev_priv->panel_fixed_mode) {
+		if (is_edp(intel_dp) && !intel_dp->panel_fixed_mode) {
 			struct drm_display_mode *newmode;
 			list_for_each_entry(newmode, &connector->probed_modes,
 					    head) {
-				if (newmode->type & DRM_MODE_TYPE_PREFERRED) {
-					dev_priv->panel_fixed_mode =
+				if ((newmode->type & DRM_MODE_TYPE_PREFERRED)) {
+					intel_dp->panel_fixed_mode =
 						drm_mode_duplicate(dev, newmode);
 					break;
 				}
 			}
 		}
-
 		return ret;
 	}
 
 	/* if eDP has no EDID, try to use fixed panel mode from VBT */
 	if (is_edp(intel_dp)) {
 		/* initialize panel mode from VBT if available for eDP */
-		if (dev_priv->panel_fixed_mode == NULL && dev_priv->lfp_lvds_vbt_mode != NULL) {
-			dev_priv->panel_fixed_mode =
+		if (intel_dp->panel_fixed_mode == NULL && dev_priv->lfp_lvds_vbt_mode != NULL) {
+			intel_dp->panel_fixed_mode =
 				drm_mode_duplicate(dev, dev_priv->lfp_lvds_vbt_mode);
-			if (dev_priv->panel_fixed_mode) {
-				dev_priv->panel_fixed_mode->type |=
+			if (intel_dp->panel_fixed_mode) {
+				intel_dp->panel_fixed_mode->type |=
 					DRM_MODE_TYPE_PREFERRED;
 			}
 		}
-		if (dev_priv->panel_fixed_mode) {
+		if (intel_dp->panel_fixed_mode) {
 			struct drm_display_mode *mode;
-			mode = drm_mode_duplicate(dev, dev_priv->panel_fixed_mode);
+			mode = drm_mode_duplicate(dev, intel_dp->panel_fixed_mode);
 			drm_mode_probed_add(connector, mode);
 			return 1;
 		}
-- 
1.7.6.3

[PATCH 9/9] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Keith Packard]

There's no good reason to turn off the eDP force VDD bit synchronously
while probing devices; that just sticks a huge delay into all mode
setting paths. Instead, queue a delayed work proc to disable the VDD
force bit and then remember when that fires to ensure that the
appropriate delay is respected before trying to turn it back on.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   93 +++++++++++++++++++++++++++++++++-----
 1 files changed, 80 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5bc30f9..8130e82 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -62,6 +62,9 @@ struct intel_dp {
 	int panel_power_up_delay;
 	int panel_power_down_delay;
 	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
+	struct delayed_work panel_vdd_work;
+	bool panel_vdd_force;
+	unsigned long panel_off_jiffies;
 };
 
 /**
@@ -834,6 +837,23 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	}
 }
 
+static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
+{
+	unsigned long	off_time;
+	unsigned long	delay;
+	DRM_DEBUG_KMS("Wait for panel power off time\n");
+	off_time = intel_dp->panel_off_jiffies + msecs_to_jiffies(intel_dp->panel_power_down_delay);
+	if (time_after(jiffies, off_time)) {
+		DRM_DEBUG_KMS("Time already passed");
+		return;
+	}
+	delay = jiffies_to_msecs(off_time - jiffies);
+	if (delay > intel_dp->panel_power_down_delay)
+		delay = intel_dp->panel_power_down_delay;
+	DRM_DEBUG_KMS("Waiting an additional %ld ms\n", delay);
+	msleep(delay);
+}
+
 static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
@@ -843,13 +863,18 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	if (!is_edp(intel_dp))
 		return;
 	DRM_DEBUG_KMS("Turn eDP VDD on\n");
-	/*
-	 * If the panel wasn't on, make sure there's not a currently
-	 * active PP sequence before enabling AUX VDD.
-	 */
-	pp_status = I915_READ(PCH_PP_STATUS);
 
+	WARN(intel_dp->panel_vdd_force,
+	     "eDP VDD already forced on\n");
+
+	intel_dp->panel_vdd_force = true;
 	pp = I915_READ(PCH_PP_CONTROL);
+	if (pp & EDP_FORCE_VDD) {
+		DRM_DEBUG_KMS("eDP VDD already on\n");
+		return;
+	}
+
+	ironlake_wait_panel_off(intel_dp);
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
 	pp |= EDP_FORCE_VDD;
@@ -857,21 +882,23 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	POSTING_READ(PCH_PP_CONTROL);
 	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
 		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
+
+	/*
+	 * If the panel wasn't on, delay before accessing aux channel
+	 */
+	pp_status = I915_READ(PCH_PP_STATUS);
 	if (!(pp_status & PP_ON)) {
+		DRM_DEBUG_KMS("eDP was not running\n");
 		msleep(intel_dp->panel_power_up_delay);
-		DRM_DEBUG_KMS("eDP VDD was not on\n");
 	}
 }
 
-static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
+static void ironlake_panel_vdd_delayed_off(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp;
 
-	if (!is_edp(intel_dp))
-		return;
-	DRM_DEBUG_KMS("Turn eDP VDD off\n");
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
@@ -882,7 +909,37 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 	/* Make sure sequencer is idle before allowing subsequent activity */
 	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
 		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
-	msleep(intel_dp->panel_power_down_delay);
+	intel_dp->panel_off_jiffies = jiffies;
+}
+
+static void ironlake_panel_vdd_work(struct work_struct *__work)
+{
+	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
+						 struct intel_dp, panel_vdd_work);
+	struct drm_device *dev = intel_dp->base.base.dev;
+
+	mutex_lock(&dev->struct_mutex);
+	if (!intel_dp->panel_vdd_force)
+		ironlake_panel_vdd_delayed_off(intel_dp);
+	mutex_unlock(&dev->struct_mutex);
+}
+
+static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
+{
+	if (!is_edp(intel_dp))
+		return;
+
+	DRM_DEBUG_KMS("Turn eDP VDD off %d\n", intel_dp->panel_vdd_force);
+	WARN(!intel_dp->panel_vdd_force, "eDP VDD not forced on");
+	
+	intel_dp->panel_vdd_force = false;
+	/*
+	 * Queue the timer to fire a long
+	 * time from now (relative to the power down delay)
+	 * to keep the panel power up across a sequence of operations
+	 */
+	schedule_delayed_work(&intel_dp->panel_vdd_work,
+			      msecs_to_jiffies(intel_dp->panel_power_down_delay * 5));
 }
 
 /* Returns true if the panel was already on when called */
@@ -895,6 +952,7 @@ static bool ironlake_edp_panel_on (struct intel_dp *intel_dp)
 	if (I915_READ(PCH_PP_STATUS) & PP_ON)
 		return true;
 
+	ironlake_wait_panel_off(intel_dp);
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
@@ -940,7 +998,6 @@ static void ironlake_edp_panel_off(struct drm_encoder *encoder)
 	pp &= ~POWER_TARGET_ON;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
-	msleep(intel_dp->panel_power_down_delay);
 
 	if (wait_for((I915_READ(PCH_PP_STATUS) & idle_off_mask) == 0, 5000))
 		DRM_ERROR("panel off wait timed out: 0x%08x\n",
@@ -949,6 +1006,7 @@ static void ironlake_edp_panel_off(struct drm_encoder *encoder)
 	pp |= PANEL_POWER_RESET; /* restore panel reset bit */
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
+	intel_dp->panel_off_jiffies = jiffies;
 }
 
 static void ironlake_edp_backlight_on (struct drm_device *dev)
@@ -1937,6 +1995,10 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 
 	i2c_del_adapter(&intel_dp->adapter);
 	drm_encoder_cleanup(encoder);
+	if (is_edp(intel_dp)) {
+		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
+		ironlake_panel_vdd_delayed_off(intel_dp);
+	}
 	kfree(intel_dp);
 }
 
@@ -2073,8 +2135,11 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 	else if (output_reg == DP_D || output_reg == PCH_DP_D)
 		intel_encoder->clone_mask = (1 << INTEL_DP_D_CLONE_BIT);
 
-	if (is_edp(intel_dp))
+	if (is_edp(intel_dp)) {
 		intel_encoder->clone_mask = (1 << INTEL_EDP_CLONE_BIT);
+		INIT_DELAYED_WORK(&intel_dp->panel_vdd_work,
+				  ironlake_panel_vdd_work);
+	}
 
 	intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
 	connector->interlace_allowed = true;
@@ -2149,6 +2214,8 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 		DRM_DEBUG_KMS("panel power up delay %d, power down delay %d\n",
 			      intel_dp->panel_power_up_delay, intel_dp->panel_power_down_delay);
 
+		intel_dp->panel_off_jiffies = jiffies - intel_dp->panel_power_down_delay;
+
 		ironlake_edp_panel_vdd_on(intel_dp);
 		ret = intel_dp_get_dpcd(intel_dp);
 		ironlake_edp_panel_vdd_off(intel_dp);
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 9/9] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Jesse Barnes]

On Mon, 19 Sep 2011 15:22:03 -0700
Keith Packard <keithp@keithp.com> wrote:

> There's no good reason to turn off the eDP force VDD bit synchronously
> while probing devices; that just sticks a huge delay into all mode
> setting paths. Instead, queue a delayed work proc to disable the VDD
> force bit and then remember when that fires to ensure that the
> appropriate delay is respected before trying to turn it back on.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---

I'm worried this makes our PPS even more complex and hard to follow.
I'd rather see VDD AUX applied only when we need it (dpms, mode set and
detect; for hotplug we can assume the panel is alive) and that we
carefully disable it after waiting for a time after a full PPS on
sequence when doing a mode set or dpms on command.

Having all the power stuff at the highest levels would be clearest I
think.

Jesse

Re: [Intel-gfx] [PATCH 9/9] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1388 bytes --]

On Wed, 21 Sep 2011 09:47:59 +0530, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> I'm worried this makes our PPS even more complex and hard to follow.
> I'd rather see VDD AUX applied only when we need it (dpms, mode set and
> detect; for hotplug we can assume the panel is alive) and that we
> carefully disable it after waiting for a time after a full PPS on
> sequence when doing a mode set or dpms on command.

You'll see the fairly long list of places where VDD AUX is needed in my
other reply; a couple of them could probably be fixed by moving the PPS
calls around.

> Having all the power stuff at the highest levels would be clearest I
> think.

Yes, making it cleaner would help a ton. There are some basic problems
with the DRM API that make this hard though -- intel_dp_prepare may not
ever be followed by a call to intel_dp_commit. That's why I had the VDD
AUX stuff get turned off by a delayed work proc instead.

Also, leaving VDD AUX high after EDID is fetched means that we can start
the mode setting immediately, rather than having to wait for the
power-off/power-on delay (which is really long).

What we could do is force VDD AUX off after the panel gets turned on;
that would ensure that turning the panel off would actually turn the
power off, rather than having VDD stay high for some time after that.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Intel-gfx] [PATCH 9/9] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Jesse Barnes]

On Tue, 20 Sep 2011 21:51:33 -0700
Keith Packard <keithp@keithp.com> wrote:
> Yes, making it cleaner would help a ton. There are some basic problems
> with the DRM API that make this hard though -- intel_dp_prepare may
> not ever be followed by a call to intel_dp_commit. That's why I had
> the VDD AUX stuff get turned off by a delayed work proc instead.
> 
> Also, leaving VDD AUX high after EDID is fetched means that we can
> start the mode setting immediately, rather than having to wait for the
> power-off/power-on delay (which is really long).

Yeah agreed.  

> What we could do is force VDD AUX off after the panel gets turned on;
> that would ensure that turning the panel off would actually turn the
> power off, rather than having VDD stay high for some time after that.

Yep, that sounds great.

Thanks,
Jesse

Re: [PATCH 9/9] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Jesse Barnes]

On Tue, 20 Sep 2011 21:51:33 -0700
Keith Packard <keithp@keithp.com> wrote:
> Yes, making it cleaner would help a ton. There are some basic problems
> with the DRM API that make this hard though -- intel_dp_prepare may
> not ever be followed by a call to intel_dp_commit. That's why I had
> the VDD AUX stuff get turned off by a delayed work proc instead.
> 
> Also, leaving VDD AUX high after EDID is fetched means that we can
> start the mode setting immediately, rather than having to wait for the
> power-off/power-on delay (which is really long).

Yeah agreed.  

> What we could do is force VDD AUX off after the panel gets turned on;
> that would ensure that turning the panel off would actually turn the
> power off, rather than having VDD stay high for some time after that.

Yep, that sounds great.

Thanks,
Jesse

[PATCH 00/24] MacBook Air patch sequence (v2)

[Keith Packard]

Ok, so I've split all of the changes into bite-sized pieces so that
they should make sense individually now. I've also added the same
asynchronous power control to the panel power, this reduces the
module load time down to about 700ms on my MacBook Air, which is
pretty nice.

Given the length of the series, I don't think this should all land in
3.1, however I'd like opinions on whether I should push this subset,
which makes the MacBook Air work, into drm-intel-fixes and send that
along.

 [PATCH 01/21] drm/i915: Enable digital port hotplug on PCH systems
 [PATCH 02/21] drm/i915: Shut down PCH interrupts during
 [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode
 [PATCH 04/21] drm/i915: Only use VBT panel mode on eDP if no EDID is
 [PATCH 05/21] drm/i915: Check eDP power when doing aux channel
 [PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always
 [PATCH 07/21] drm/i915: Check for eDP inside
 [PATCH 08/21] drm/i915: Turn force VDD back off when panel running
 [PATCH 09/21] drm/i915: Delay DP i2c initialization until panel
 [PATCH 10/21] drm/i915: Wrap DP EDID fetch functions to enable eDP
 [PATCH 11/21] drm/i915: Enable eDP panel power during I2C
 [PATCH 12/21] drm/i915: Ensure eDP powered up during DP_SET_POWER
 [PATCH 13/21] drm/i915: Place long delays after each eDP VDD
 [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay

The diffstat for that sequence is:

 i915_drv.h |    1 
 i915_irq.c |   28 ++++++++
 i915_reg.h |    6 +
 intel_dp.c |  194 ++++++++++++++++++++++++++++++++++++++++++++++---------------
 4 files changed, 180 insertions(+), 49 deletions(-)

This might be too big at this point in the 3.1 release, so perhaps
just marking these with a Cc: to stable would be more appropriate.

-keith

[PATCH 01/21] drm/i915: Enable digital port hotplug on PCH systems

[Keith Packard]

We were relying on the BIOS to set these bits, which doesn't always
happen.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_irq.c |   24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h |    5 ++++-
 2 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9cbb0cd..c22823b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1777,6 +1777,26 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	POSTING_READ(SDEIER);
 }
 
+/*
+ * Enable digital hotplug on the PCH, and configure the DP short pulse
+ * duration to 2ms (which is the minimum in the Display Port spec)
+ *
+ * This register is the same on all known PCH chips.
+ */
+
+static void ironlake_enable_pch_hotplug(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	u32	hotplug;
+
+	hotplug = I915_READ(PCH_PORT_HOTPLUG);
+	hotplug &= ~(PORTD_PULSE_DURATION_MASK|PORTC_PULSE_DURATION_MASK|PORTB_PULSE_DURATION_MASK);
+	hotplug |= PORTD_HOTPLUG_ENABLE | PORTD_PULSE_DURATION_2ms;
+	hotplug |= PORTC_HOTPLUG_ENABLE | PORTC_PULSE_DURATION_2ms;
+	hotplug |= PORTB_HOTPLUG_ENABLE | PORTB_PULSE_DURATION_2ms;
+	I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
+}
+
 static int ironlake_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -1839,6 +1859,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(SDEIER, hotplug_mask);
 	POSTING_READ(SDEIER);
 
+	ironlake_enable_pch_hotplug(dev);
+
 	if (IS_IRONLAKE_M(dev)) {
 		/* Clear & enable PCU event interrupts */
 		I915_WRITE(DEIIR, DE_PCU_EVENT);
@@ -1896,6 +1918,8 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(SDEIER, hotplug_mask);
 	POSTING_READ(SDEIER);
 
+	ironlake_enable_pch_hotplug(dev);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 542453f..b7fbb74 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2903,12 +2903,13 @@
 #define SDEIER  0xc400c
 
 /* digital port hotplug */
-#define PCH_PORT_HOTPLUG        0xc4030
+#define PCH_PORT_HOTPLUG        0xc4030		/* SHOTPLUG_CTL */
 #define PORTD_HOTPLUG_ENABLE            (1 << 20)
 #define PORTD_PULSE_DURATION_2ms        (0)
 #define PORTD_PULSE_DURATION_4_5ms      (1 << 18)
 #define PORTD_PULSE_DURATION_6ms        (2 << 18)
 #define PORTD_PULSE_DURATION_100ms      (3 << 18)
+#define PORTD_PULSE_DURATION_MASK	(3 << 18)
 #define PORTD_HOTPLUG_NO_DETECT         (0)
 #define PORTD_HOTPLUG_SHORT_DETECT      (1 << 16)
 #define PORTD_HOTPLUG_LONG_DETECT       (1 << 17)
@@ -2917,6 +2918,7 @@
 #define PORTC_PULSE_DURATION_4_5ms      (1 << 10)
 #define PORTC_PULSE_DURATION_6ms        (2 << 10)
 #define PORTC_PULSE_DURATION_100ms      (3 << 10)
+#define PORTC_PULSE_DURATION_MASK	(3 << 10)
 #define PORTC_HOTPLUG_NO_DETECT         (0)
 #define PORTC_HOTPLUG_SHORT_DETECT      (1 << 8)
 #define PORTC_HOTPLUG_LONG_DETECT       (1 << 9)
@@ -2925,6 +2927,7 @@
 #define PORTB_PULSE_DURATION_4_5ms      (1 << 2)
 #define PORTB_PULSE_DURATION_6ms        (2 << 2)
 #define PORTB_PULSE_DURATION_100ms      (3 << 2)
+#define PORTB_PULSE_DURATION_MASK	(3 << 2)
 #define PORTB_HOTPLUG_NO_DETECT         (0)
 #define PORTB_HOTPLUG_SHORT_DETECT      (1 << 0)
 #define PORTB_HOTPLUG_LONG_DETECT       (1 << 1)
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 01/21] drm/i915: Enable digital port hotplug on PCH systems

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:33PM -0700, Keith Packard wrote:
> We were relying on the BIOS to set these bits, which doesn't always
> happen.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Ok, I'll try to increase our review-bandwidth for modesetting stuff by
jumping into this maze myself. For the moment I'll shortly explain what
I've actually checked so you know how much weight/little to attach to my
r-b's.

Checked with docs, noticed that the public one's lack register addresses
for PCH regs ...
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 01/21] drm/i915: Enable digital port hotplug on PCH systems

[Jesse Barnes]

On Fri, 30 Sep 2011 18:17:32 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Sep 29, 2011 at 06:09:33PM -0700, Keith Packard wrote:
> > We were relying on the BIOS to set these bits, which doesn't always
> > happen.
> > 
> > Signed-off-by: Keith Packard <keithp@keithp.com>
> 
> Ok, I'll try to increase our review-bandwidth for modesetting stuff by
> jumping into this maze myself. For the moment I'll shortly explain what
> I've actually checked so you know how much weight/little to attach to my
> r-b's.
> 
> Checked with docs, noticed that the public one's lack register addresses
> for PCH regs ...
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Yep, I like it too:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 02/21] drm/i915: Shut down PCH interrupts during irq_uninstall

[Keith Packard]

This masks out all interrupts and ack's any pending ones at IRQ
uninstall time to make sure we don't receive any unexpected interrupts
later on.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_irq.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c22823b..adeab2a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2044,6 +2044,10 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(GTIMR, 0xffffffff);
 	I915_WRITE(GTIER, 0x0);
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
+
+	I915_WRITE(SDEIMR, 0xffffffff);
+	I915_WRITE(SDEIER, 0x0);
+	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
 }
 
 static void i915_driver_irq_uninstall(struct drm_device * dev)
-- 
1.7.6.3

Re: [PATCH 02/21] drm/i915: Shut down PCH interrupts during irq_uninstall

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:34PM -0700, Keith Packard wrote:
> This masks out all interrupts and ack's any pending ones at IRQ
> uninstall time to make sure we don't receive any unexpected interrupts
> later on.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index c22823b..adeab2a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2044,6 +2044,10 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  	I915_WRITE(GTIMR, 0xffffffff);
>  	I915_WRITE(GTIER, 0x0);
>  	I915_WRITE(GTIIR, I915_READ(GTIIR));
> +
> +	I915_WRITE(SDEIMR, 0xffffffff);
> +	I915_WRITE(SDEIER, 0x0);
> +	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
>  }

Shouldn't we mask/ack south DE irqs before before we mask DE irqs to avoid
races, i.e.  move this new code up?
-Daniel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 02/21] drm/i915: Shut down PCH interrupts during irq_uninstall

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

On Fri, 30 Sep 2011 18:20:48 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> Shouldn't we mask/ack south DE irqs before before we mask DE irqs to avoid
> races, i.e.  move this new code up?

I don't know. What about the GT interrupts? I just stuck stuff at the
bottom, figuring it would do the least harm...

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 02/21] drm/i915: Shut down PCH interrupts during irq_uninstall

[Daniel Vetter]

On Fri, Sep 30, 2011 at 10:44:22AM -0700, Keith Packard wrote:
> On Fri, 30 Sep 2011 18:20:48 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > Shouldn't we mask/ack south DE irqs before before we mask DE irqs to avoid
> > races, i.e.  move this new code up?
> 
> I don't know. What about the GT interrupts? I just stuck stuff at the
> bottom, figuring it would do the least harm...

Thinking this through again I don't see the race anymore. I've forgotten
that masking things in IMR also prevents stuff from showing up in IIR and
hence a SDE irq after clearing DE irqs can't leave garbage behind in
DEIIR. So no need to clear things in the order the irqs propagate, i.e.
SDE->DE->GT. For the patch:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting

[Keith Packard]

There's no reason to enforce a 300ms delay during eDP mode setting.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 44fef5e..f0cfb6b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -903,13 +903,6 @@ static void ironlake_edp_backlight_on (struct drm_device *dev)
 	u32 pp;
 
 	DRM_DEBUG_KMS("\n");
-	/*
-	 * If we enable the backlight right away following a panel power
-	 * on, we may see slight flicker as the panel syncs with the eDP
-	 * link.  So delay a bit to make sure the image is solid before
-	 * allowing it to appear.
-	 */
-	msleep(300);
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp |= EDP_BLC_ENABLE;
 	I915_WRITE(PCH_PP_CONTROL, pp);
-- 
1.7.6.3

[PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting

[Keith Packard]

There's no reason to enforce a 300ms delay during eDP mode setting.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 44fef5e..f0cfb6b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -903,13 +903,6 @@ static void ironlake_edp_backlight_on (struct drm_device *dev)
 	u32 pp;
 
 	DRM_DEBUG_KMS("\n");
-	/*
-	 * If we enable the backlight right away following a panel power
-	 * on, we may see slight flicker as the panel syncs with the eDP
-	 * link.  So delay a bit to make sure the image is solid before
-	 * allowing it to appear.
-	 */
-	msleep(300);
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp |= EDP_BLC_ENABLE;
 	I915_WRITE(PCH_PP_CONTROL, pp);
-- 
1.7.6.3

Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:35PM -0700, Keith Packard wrote:
> There's no reason to enforce a 300ms delay during eDP mode setting.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Can you elaborate a bit why this is no longer needed? Jesse seems to have
introduced this spefically for edp panels in d209848d61794968. If this
becomes rendundant due to your panel power sequencing fixes, maybe move it
down the series?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

On Fri, 30 Sep 2011 18:27:28 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> Can you elaborate a bit why this is no longer needed? Jesse seems to have
> introduced this spefically for edp panels in d209848d61794968. If this
> becomes rendundant due to your panel power sequencing fixes, maybe move it
> down the series?

The only reason for this delay is to avoid some potential noise on the
screen during mode setting, all of the power sequencing delays are
already correctly managed by the driver. As far as I can tell, we're
trading off the small potential for noise on the screen with an extra
300ms delay at boot time.

In the eDP spec, there's a delay listed (T8) between the start of video
data and turning on the backlight. There are no values presented and
the instructions are simply that the source is supposed to wait until
the display is stable. I'm not sure how we'd ever know that the display
is stable.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

On Fri, 30 Sep 2011 18:27:28 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> Can you elaborate a bit why this is no longer needed? Jesse seems to have
> introduced this spefically for edp panels in d209848d61794968. If this
> becomes rendundant due to your panel power sequencing fixes, maybe move it
> down the series?

The only reason for this delay is to avoid some potential noise on the
screen during mode setting, all of the power sequencing delays are
already correctly managed by the driver. As far as I can tell, we're
trading off the small potential for noise on the screen with an extra
300ms delay at boot time.

In the eDP spec, there's a delay listed (T8) between the start of video
data and turning on the backlight. There are no values presented and
the instructions are simply that the source is supposed to wait until
the display is stable. I'm not sure how we'd ever know that the display
is stable.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting

[Daniel Vetter]

On Fri, Sep 30, 2011 at 10:50:10AM -0700, Keith Packard wrote:
> On Fri, 30 Sep 2011 18:27:28 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > Can you elaborate a bit why this is no longer needed? Jesse seems to have
> > introduced this spefically for edp panels in d209848d61794968. If this
> > becomes rendundant due to your panel power sequencing fixes, maybe move it
> > down the series?
> 
> The only reason for this delay is to avoid some potential noise on the
> screen during mode setting, all of the power sequencing delays are
> already correctly managed by the driver. As far as I can tell, we're
> trading off the small potential for noise on the screen with an extra
> 300ms delay at boot time.
> 
> In the eDP spec, there's a delay listed (T8) between the start of video
> data and turning on the backlight. There are no values presented and
> the instructions are simply that the source is supposed to wait until
> the display is stable. I'm not sure how we'd ever know that the display
> is stable.

Ok, maybe add the above to paragraphs to the commit msg. Then
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting

[Daniel Vetter]

On Fri, Sep 30, 2011 at 07:58:21PM +0200, Daniel Vetter wrote:
> On Fri, Sep 30, 2011 at 10:50:10AM -0700, Keith Packard wrote:
> > On Fri, 30 Sep 2011 18:27:28 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > Can you elaborate a bit why this is no longer needed? Jesse seems to have
> > > introduced this spefically for edp panels in d209848d61794968. If this
> > > becomes rendundant due to your panel power sequencing fixes, maybe move it
> > > down the series?
> > 
> > The only reason for this delay is to avoid some potential noise on the
> > screen during mode setting, all of the power sequencing delays are
> > already correctly managed by the driver. As far as I can tell, we're
> > trading off the small potential for noise on the screen with an extra
> > 300ms delay at boot time.
> > 
> > In the eDP spec, there's a delay listed (T8) between the start of video
> > data and turning on the backlight. There are no values presented and
> > the instructions are simply that the source is supposed to wait until
> > the display is stable. I'm not sure how we'd ever know that the display
> > is stable.
> 
> Ok, maybe add the above to paragraphs to the commit msg. Then
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

On further strolling through bspec to review later patches I've noticed
that PCH_PP_ON_DELAYS and PCH_PP_OFF_DELAYS seem to have values for
power on->backlight on and backlight off->panel off delays. Maybe we
should use these?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting

[Daniel Vetter]

On Fri, Sep 30, 2011 at 07:58:21PM +0200, Daniel Vetter wrote:
> On Fri, Sep 30, 2011 at 10:50:10AM -0700, Keith Packard wrote:
> > On Fri, 30 Sep 2011 18:27:28 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > Can you elaborate a bit why this is no longer needed? Jesse seems to have
> > > introduced this spefically for edp panels in d209848d61794968. If this
> > > becomes rendundant due to your panel power sequencing fixes, maybe move it
> > > down the series?
> > 
> > The only reason for this delay is to avoid some potential noise on the
> > screen during mode setting, all of the power sequencing delays are
> > already correctly managed by the driver. As far as I can tell, we're
> > trading off the small potential for noise on the screen with an extra
> > 300ms delay at boot time.
> > 
> > In the eDP spec, there's a delay listed (T8) between the start of video
> > data and turning on the backlight. There are no values presented and
> > the instructions are simply that the source is supposed to wait until
> > the display is stable. I'm not sure how we'd ever know that the display
> > is stable.
> 
> Ok, maybe add the above to paragraphs to the commit msg. Then
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

On further strolling through bspec to review later patches I've noticed
that PCH_PP_ON_DELAYS and PCH_PP_OFF_DELAYS seem to have values for
power on->backlight on and backlight off->panel off delays. Maybe we
should use these?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

On Fri, 30 Sep 2011 20:09:59 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> On further strolling through bspec to review later patches I've noticed
> that PCH_PP_ON_DELAYS and PCH_PP_OFF_DELAYS seem to have values for
> power on->backlight on and backlight off->panel off delays. Maybe we
> should use these?

Yeah, I must have missed those. For backlight on, I think I'll make it
asynchronous with a delayed work proc so that the rest of the boot/dpms
process doesn't block behind turning the light on.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

On Fri, 30 Sep 2011 20:09:59 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> On further strolling through bspec to review later patches I've noticed
> that PCH_PP_ON_DELAYS and PCH_PP_OFF_DELAYS seem to have values for
> power on->backlight on and backlight off->panel off delays. Maybe we
> should use these?

Yeah, I must have missed those. For backlight on, I think I'll make it
asynchronous with a delayed work proc so that the rest of the boot/dpms
process doesn't block behind turning the light on.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 04/21] drm/i915: Only use VBT panel mode on eDP if no EDID is found

[Keith Packard]

We're going to assume that EDID is more reliable than the VBT tables
for eDP panels, which is notably true on MacBook machines where the
VBT contains completely bogus data.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f0cfb6b..8ab2a88 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1748,7 +1748,16 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 
 	/* if eDP has no EDID, try to use fixed panel mode from VBT */
 	if (is_edp(intel_dp)) {
-		if (dev_priv->panel_fixed_mode != NULL) {
+		/* initialize panel mode from VBT if available for eDP */
+		if (dev_priv->panel_fixed_mode == NULL && dev_priv->lfp_lvds_vbt_mode != NULL) {
+			dev_priv->panel_fixed_mode =
+				drm_mode_duplicate(dev, dev_priv->lfp_lvds_vbt_mode);
+			if (dev_priv->panel_fixed_mode) {
+				dev_priv->panel_fixed_mode->type |=
+					DRM_MODE_TYPE_PREFERRED;
+			}
+		}
+		if (dev_priv->panel_fixed_mode) {
 			struct drm_display_mode *mode;
 			mode = drm_mode_duplicate(dev, dev_priv->panel_fixed_mode);
 			drm_mode_probed_add(connector, mode);
@@ -2061,15 +2070,6 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
 	if (is_edp(intel_dp)) {
-		/* initialize panel mode from VBT if available for eDP */
-		if (dev_priv->lfp_lvds_vbt_mode) {
-			dev_priv->panel_fixed_mode =
-				drm_mode_duplicate(dev, dev_priv->lfp_lvds_vbt_mode);
-			if (dev_priv->panel_fixed_mode) {
-				dev_priv->panel_fixed_mode->type |=
-					DRM_MODE_TYPE_PREFERRED;
-			}
-		}
 		dev_priv->int_edp_connector = connector;
 		intel_panel_setup_backlight(dev);
 	}
-- 
1.7.6.3

[PATCH 04/21] drm/i915: Only use VBT panel mode on eDP if no EDID is found

[Keith Packard]

We're going to assume that EDID is more reliable than the VBT tables
for eDP panels, which is notably true on MacBook machines where the
VBT contains completely bogus data.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f0cfb6b..8ab2a88 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1748,7 +1748,16 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 
 	/* if eDP has no EDID, try to use fixed panel mode from VBT */
 	if (is_edp(intel_dp)) {
-		if (dev_priv->panel_fixed_mode != NULL) {
+		/* initialize panel mode from VBT if available for eDP */
+		if (dev_priv->panel_fixed_mode == NULL && dev_priv->lfp_lvds_vbt_mode != NULL) {
+			dev_priv->panel_fixed_mode =
+				drm_mode_duplicate(dev, dev_priv->lfp_lvds_vbt_mode);
+			if (dev_priv->panel_fixed_mode) {
+				dev_priv->panel_fixed_mode->type |=
+					DRM_MODE_TYPE_PREFERRED;
+			}
+		}
+		if (dev_priv->panel_fixed_mode) {
 			struct drm_display_mode *mode;
 			mode = drm_mode_duplicate(dev, dev_priv->panel_fixed_mode);
 			drm_mode_probed_add(connector, mode);
@@ -2061,15 +2070,6 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
 	if (is_edp(intel_dp)) {
-		/* initialize panel mode from VBT if available for eDP */
-		if (dev_priv->lfp_lvds_vbt_mode) {
-			dev_priv->panel_fixed_mode =
-				drm_mode_duplicate(dev, dev_priv->lfp_lvds_vbt_mode);
-			if (dev_priv->panel_fixed_mode) {
-				dev_priv->panel_fixed_mode->type |=
-					DRM_MODE_TYPE_PREFERRED;
-			}
-		}
 		dev_priv->int_edp_connector = connector;
 		intel_panel_setup_backlight(dev);
 	}
-- 
1.7.6.3

Re: [PATCH 04/21] drm/i915: Only use VBT panel mode on eDP if no EDID is found

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:36PM -0700, Keith Packard wrote:
> We're going to assume that EDID is more reliable than the VBT tables
> for eDP panels, which is notably true on MacBook machines where the
> VBT contains completely bogus data.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Ok, this is way over my head, just checked whether the patch does what it
claims to - nice exercise in reading dp modeset code ;-)
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 04/21] drm/i915: Only use VBT panel mode on eDP if no EDID is found

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]

On Fri, 30 Sep 2011 18:32:35 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> Ok, this is way over my head, just checked whether the patch does what it
> claims to - nice exercise in reading dp modeset code ;-)

Yeah, it's purely heuristic -- the VBT contains a mode which was
originally for the LVDS. It's unclear whether it should ever be applied
to an eDP panel, but absent other information, it seems like we should
at least consider it.

Many LVDS panels don't bother to include an EDID rom, or the vendor
didn't bother to hook up the DDC wire; presumably it's cheaper for them
to stick more data in the VBIOS than add hardware.  However, there are
some LVDS panels with EDID roms which contain *incorrect* mode data for
the panel (amazing, I know), and so the driver prefers to use the VBT
data when both are present.

eDP, on the other hand, doesn't require any additional wiring (at least)
to connect up the DDC channel, and eDP panels are required to provide
EDID data. So far, in my (very small) sample set, I've got one machine
which provides accurate VBT *and* EDID data (an HP 2540p) and one
machine which provides inaccurate VBT data but accurate EDID data (a
MacBook air). So, I just decided to prefer the EDID data.

One option would be to extract the current mode from the hardware when
the driver starts and use that if present. But, that might mean that
you'd get different modes depending on whether the machine booted with
the lid closed or open, which seems like a bad plan.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Intel-gfx] [PATCH 04/21] drm/i915: Only use VBT panel mode on eDP if no EDID is found

[Jesse Barnes]

On Fri, 30 Sep 2011 10:58:26 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Fri, 30 Sep 2011 18:32:35 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > Ok, this is way over my head, just checked whether the patch does what it
> > claims to - nice exercise in reading dp modeset code ;-)
> 
> Yeah, it's purely heuristic -- the VBT contains a mode which was
> originally for the LVDS. It's unclear whether it should ever be applied
> to an eDP panel, but absent other information, it seems like we should
> at least consider it.
> 
> Many LVDS panels don't bother to include an EDID rom, or the vendor
> didn't bother to hook up the DDC wire; presumably it's cheaper for them
> to stick more data in the VBIOS than add hardware.  However, there are
> some LVDS panels with EDID roms which contain *incorrect* mode data for
> the panel (amazing, I know), and so the driver prefers to use the VBT
> data when both are present.
> 
> eDP, on the other hand, doesn't require any additional wiring (at least)
> to connect up the DDC channel, and eDP panels are required to provide
> EDID data. So far, in my (very small) sample set, I've got one machine
> which provides accurate VBT *and* EDID data (an HP 2540p) and one
> machine which provides inaccurate VBT data but accurate EDID data (a
> MacBook air). So, I just decided to prefer the EDID data.
> 
> One option would be to extract the current mode from the hardware when
> the driver starts and use that if present. But, that might mean that
> you'd get different modes depending on whether the machine booted with
> the lid closed or open, which seems like a bad plan.

More than that, I think eDP *requires* an EDID, and it sounds like even
the Air has one (and if any machine didn't, you know it would be an
Apple).

So I'm definitely in favor of this change.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications

[Keith Packard]

Verify that the eDP VDD is on, either with the panel being on or with
the VDD force-on bit being set.

This demonstrates that in many instances, VDD is not on when needed,
which leads to failed EDID communications.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8ab2a88..3b29a6f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -279,6 +279,24 @@ intel_hrawclk(struct drm_device *dev)
 	}
 }
 
+static void
+intel_dp_check_edp(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 pp_status, pp_control;
+	if (!is_edp(intel_dp))
+		return;
+	pp_status = I915_READ(PCH_PP_STATUS);
+	pp_control = I915_READ(PCH_PP_CONTROL);
+	if ((pp_status & PP_ON) == 0 && (pp_control & EDP_FORCE_VDD) == 0) {
+		WARN(1, "eDP powered off while attempting aux channel communication.\n");
+		DRM_DEBUG_KMS("Status 0x%08x Control 0x%08x\n",
+			      pp_status,
+			      I915_READ(PCH_PP_CONTROL));
+	}
+}
+
 static int
 intel_dp_aux_ch(struct intel_dp *intel_dp,
 		uint8_t *send, int send_bytes,
@@ -295,6 +313,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	uint32_t aux_clock_divider;
 	int try, precharge;
 
+	intel_dp_check_edp(intel_dp);
 	/* The clock divider is based off the hrawclk,
 	 * and would like to run at 2MHz. So, take the
 	 * hrawclk value and divide by 2 and use that
@@ -408,6 +427,7 @@ intel_dp_aux_native_write(struct intel_dp *intel_dp,
 	int msg_bytes;
 	uint8_t	ack;
 
+	intel_dp_check_edp(intel_dp);
 	if (send_bytes > 16)
 		return -1;
 	msg[0] = AUX_NATIVE_WRITE << 4;
@@ -450,6 +470,7 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 	uint8_t ack;
 	int ret;
 
+	intel_dp_check_edp(intel_dp);
 	msg[0] = AUX_NATIVE_READ << 4;
 	msg[1] = address >> 8;
 	msg[2] = address & 0xff;
@@ -493,6 +514,7 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 	int reply_bytes;
 	int ret;
 
+	intel_dp_check_edp(intel_dp);
 	/* Set up the command byte */
 	if (mode & MODE_I2C_READ)
 		msg[0] = AUX_I2C_READ << 4;
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:37PM -0700, Keith Packard wrote:
> Verify that the eDP VDD is on, either with the panel being on or with
> the VDD force-on bit being set.
> 
> This demonstrates that in many instances, VDD is not on when needed,
> which leads to failed EDID communications.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8ab2a88..3b29a6f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -279,6 +279,24 @@ intel_hrawclk(struct drm_device *dev)
>  	}
>  }
>  
> +static void
> +intel_dp_check_edp(struct intel_dp *intel_dp)
> +{
> +	struct drm_device *dev = intel_dp->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 pp_status, pp_control;
> +	if (!is_edp(intel_dp))
> +		return;
> +	pp_status = I915_READ(PCH_PP_STATUS);
> +	pp_control = I915_READ(PCH_PP_CONTROL);
> +	if ((pp_status & PP_ON) == 0 && (pp_control & EDP_FORCE_VDD) == 0) {
> +		WARN(1, "eDP powered off while attempting aux channel communication.\n");
> +		DRM_DEBUG_KMS("Status 0x%08x Control 0x%08x\n",
> +			      pp_status,
> +			      I915_READ(PCH_PP_CONTROL));

Use pp_control instead of re-reading?

> +	}
> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		uint8_t *send, int send_bytes,
> @@ -295,6 +313,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	uint32_t aux_clock_divider;
>  	int try, precharge;
>  
> +	intel_dp_check_edp(intel_dp);
>  	/* The clock divider is based off the hrawclk,
>  	 * and would like to run at 2MHz. So, take the
>  	 * hrawclk value and divide by 2 and use that

dp_aux_ch does the low-level io for the below, so either this one or the
below three hunks look a bit redundant.

> @@ -408,6 +427,7 @@ intel_dp_aux_native_write(struct intel_dp *intel_dp,
>  	int msg_bytes;
>  	uint8_t	ack;
>  
> +	intel_dp_check_edp(intel_dp);
>  	if (send_bytes > 16)
>  		return -1;
>  	msg[0] = AUX_NATIVE_WRITE << 4;
> @@ -450,6 +470,7 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
>  	uint8_t ack;
>  	int ret;
>  
> +	intel_dp_check_edp(intel_dp);
>  	msg[0] = AUX_NATIVE_READ << 4;
>  	msg[1] = address >> 8;
>  	msg[2] = address & 0xff;
> @@ -493,6 +514,7 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
>  	int reply_bytes;
>  	int ret;
>  
> +	intel_dp_check_edp(intel_dp);
>  	/* Set up the command byte */
>  	if (mode & MODE_I2C_READ)
>  		msg[0] = AUX_I2C_READ << 4;
> -- 
> 1.7.6.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

On Fri, 30 Sep 2011 19:02:42 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> Use pp_control instead of re-reading?

Could, but you'll note a later patch eliminates both pp_status and
pp_control local variables, so I didn't bother to clean this up when
refactoring.

> dp_aux_ch does the low-level io for the below, so either this one or the
> below three hunks look a bit redundant.

Yeah, probably not necessary. I just added checks everywhere I could
think of to try and figure out where power was not being applied when
needed.

Should I bother to include this patch in the series at all? It's purely
for diagnostics to make sure the panel is powered during all aux channel
transactions.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Intel-gfx] [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications

[Daniel Vetter]

On Fri, Sep 30, 2011 at 11:01:06AM -0700, Keith Packard wrote:
> Should I bother to include this patch in the series at all? It's purely
> for diagnostics to make sure the panel is powered during all aux channel
> transactions.

I'd say yes, imo paranoia in modesetting code is justified ;-)
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications

[Chris Wilson]

On Fri, 30 Sep 2011 11:01:06 -0700, Keith Packard <keithp@keithp.com> wrote:
Non-text part: multipart/signed
> On Fri, 30 Sep 2011 19:02:42 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > Use pp_control instead of re-reading?
> 
> Could, but you'll note a later patch eliminates both pp_status and
> pp_control local variables, so I didn't bother to clean this up when
> refactoring.
> 
> > dp_aux_ch does the low-level io for the below, so either this one or the
> > below three hunks look a bit redundant.
> 
> Yeah, probably not necessary. I just added checks everywhere I could
> think of to try and figure out where power was not being applied when
> needed.
> 
> Should I bother to include this patch in the series at all? It's purely
> for diagnostics to make sure the panel is powered during all aux channel
> transactions.

Yes, adding the extra checks made my day. :)

Modesetting bugs are the hardest to reproduce so every additional check
(with a reasonable false positive rate) is worth their weight in gold when
it comes to diagnosing misbehaving hardware. And the checks improve the
maintainability of the code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications

[Jesse Barnes]

On Thu, 29 Sep 2011 18:09:37 -0700
Keith Packard <keithp@keithp.com> wrote:

> Verify that the eDP VDD is on, either with the panel being on or with
> the VDD force-on bit being set.
> 
> This demonstrates that in many instances, VDD is not on when needed,
> which leads to failed EDID communications.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8ab2a88..3b29a6f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -279,6 +279,24 @@ intel_hrawclk(struct drm_device *dev)
>  	}
>  }
>  
> +static void
> +intel_dp_check_edp(struct intel_dp *intel_dp)
> +{
> +	struct drm_device *dev = intel_dp->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 pp_status, pp_control;
> +	if (!is_edp(intel_dp))
> +		return;
> +	pp_status = I915_READ(PCH_PP_STATUS);
> +	pp_control = I915_READ(PCH_PP_CONTROL);
> +	if ((pp_status & PP_ON) == 0 && (pp_control & EDP_FORCE_VDD) == 0) {
> +		WARN(1, "eDP powered off while attempting aux channel communication.\n");
> +		DRM_DEBUG_KMS("Status 0x%08x Control 0x%08x\n",
> +			      pp_status,
> +			      I915_READ(PCH_PP_CONTROL));
> +	}
> +}

I'd call it "intel_dp_assert_dp_power" or something more descriptive
(or just assert_panel_power to match the other asserts in
intel_display.c), but other than that this is a nice sanity check.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications

[Jesse Barnes]

On Thu, 29 Sep 2011 18:09:37 -0700
Keith Packard <keithp@keithp.com> wrote:

> Verify that the eDP VDD is on, either with the panel being on or with
> the VDD force-on bit being set.
> 
> This demonstrates that in many instances, VDD is not on when needed,
> which leads to failed EDID communications.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8ab2a88..3b29a6f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -279,6 +279,24 @@ intel_hrawclk(struct drm_device *dev)
>  	}
>  }
>  
> +static void
> +intel_dp_check_edp(struct intel_dp *intel_dp)
> +{
> +	struct drm_device *dev = intel_dp->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 pp_status, pp_control;
> +	if (!is_edp(intel_dp))
> +		return;
> +	pp_status = I915_READ(PCH_PP_STATUS);
> +	pp_control = I915_READ(PCH_PP_CONTROL);
> +	if ((pp_status & PP_ON) == 0 && (pp_control & EDP_FORCE_VDD) == 0) {
> +		WARN(1, "eDP powered off while attempting aux channel communication.\n");
> +		DRM_DEBUG_KMS("Status 0x%08x Control 0x%08x\n",
> +			      pp_status,
> +			      I915_READ(PCH_PP_CONTROL));
> +	}
> +}

I'd call it "intel_dp_assert_dp_power" or something more descriptive
(or just assert_panel_power to match the other asserts in
intel_display.c), but other than that this is a nice sanity check.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always

[Keith Packard]

Avoid any question about locked registers by just writing the unlock
pattern with every write to the register.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_reg.h |    1 +
 drivers/gpu/drm/i915/intel_dp.c |   14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b7fbb74..5596e8e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3311,6 +3311,7 @@
 #define PCH_PP_STATUS		0xc7200
 #define PCH_PP_CONTROL		0xc7204
 #define  PANEL_UNLOCK_REGS	(0xabcd << 16)
+#define  PANEL_UNLOCK_MASK	(0xffff << 16)
 #define  EDP_FORCE_VDD		(1 << 3)
 #define  EDP_BLC_ENABLE		(1 << 2)
 #define  PANEL_POWER_RESET	(1 << 1)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3b29a6f..a983d0f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -840,6 +840,8 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 		msleep(dev_priv->panel_t3);
 
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 	pp |= EDP_FORCE_VDD;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
@@ -852,6 +854,8 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 	u32 pp;
 
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 	pp &= ~EDP_FORCE_VDD;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
@@ -871,13 +875,15 @@ static bool ironlake_edp_panel_on (struct intel_dp *intel_dp)
 		return true;
 
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 
 	/* ILK workaround: disable reset around power sequence */
 	pp &= ~PANEL_POWER_RESET;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
 
-	pp |= PANEL_UNLOCK_REGS | POWER_TARGET_ON;
+	pp |= POWER_TARGET_ON;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
 
@@ -900,6 +906,8 @@ static void ironlake_edp_panel_off (struct drm_device *dev)
 		PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK;
 
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 
 	/* ILK workaround: disable reset around power sequence */
 	pp &= ~PANEL_POWER_RESET;
@@ -926,6 +934,8 @@ static void ironlake_edp_backlight_on (struct drm_device *dev)
 
 	DRM_DEBUG_KMS("\n");
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 	pp |= EDP_BLC_ENABLE;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 }
@@ -937,6 +947,8 @@ static void ironlake_edp_backlight_off (struct drm_device *dev)
 
 	DRM_DEBUG_KMS("\n");
 	pp = I915_READ(PCH_PP_CONTROL);
+	pp &= ~PANEL_UNLOCK_MASK;
+	pp |= PANEL_UNLOCK_REGS;
 	pp &= ~EDP_BLC_ENABLE;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 }
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:38PM -0700, Keith Packard wrote:
> Avoid any question about locked registers by just writing the unlock
> pattern with every write to the register.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

grep shows that we also write to PCH_PP_CONTROL in intel_lvds.c in the
dpms functions - any reasons these two writes are left out?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

On Fri, 30 Sep 2011 19:09:46 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> grep shows that we also write to PCH_PP_CONTROL in intel_lvds.c in the
> dpms functions - any reasons these two writes are left out?

Nope, I just didn't look there. I'll fix that. Locking registers with
magic patterns is just crazy these days.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Intel-gfx] [PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

On Fri, 30 Sep 2011 19:09:46 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> grep shows that we also write to PCH_PP_CONTROL in intel_lvds.c in the
> dpms functions - any reasons these two writes are left out?

Upon a bit of review:

  The bspec makes it clear that this write protect key only needs to
  be written for eDP on DPA -- it's a work-around for a bug where panel
  power sequencing wouldn't work right.

  The LVDS code does disable write protect in the _init function, which
  seems global enough, but misses the resume case. We shouldn't ever
  need to set this field though; it write protects registers only
  when the panel is running. We could presumably remove the
  write protect disable entirely in the LVDS code.

So, I think the patch as written is correct.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Intel-gfx] [PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always

[Daniel Vetter]

On Fri, Sep 30, 2011 at 04:14:40PM -0700, Keith Packard wrote:
> On Fri, 30 Sep 2011 19:09:46 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > grep shows that we also write to PCH_PP_CONTROL in intel_lvds.c in the
> > dpms functions - any reasons these two writes are left out?
> 
> Upon a bit of review:
> 
>   The bspec makes it clear that this write protect key only needs to
>   be written for eDP on DPA -- it's a work-around for a bug where panel
>   power sequencing wouldn't work right.
> 
>   The LVDS code does disable write protect in the _init function, which
>   seems global enough, but misses the resume case. We shouldn't ever
>   need to set this field though; it write protects registers only
>   when the panel is running. We could presumably remove the
>   write protect disable entirely in the LVDS code.
> 
> So, I think the patch as written is correct.

Convinced.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 07/21] drm/i915: Check for eDP inside intel_edp_panel_vdd_on/off

[Keith Packard]

Cleans up code dealing with eDP VDD a bit.  Remove redundant checks in
callers

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a983d0f..da725d8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -832,6 +832,8 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp;
 
+	if (!is_edp(intel_dp))
+		return;
 	/*
 	 * If the panel wasn't on, make sure there's not a currently
 	 * active PP sequence before enabling AUX VDD.
@@ -853,6 +855,8 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp;
 
+	if (!is_edp(intel_dp))
+		return;
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
@@ -1034,15 +1038,13 @@ static void intel_dp_commit(struct drm_encoder *encoder)
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	struct drm_device *dev = encoder->dev;
 
-	if (is_edp(intel_dp))
-		ironlake_edp_panel_vdd_on(intel_dp);
+	ironlake_edp_panel_vdd_on(intel_dp);
 
 	intel_dp_start_link_train(intel_dp);
 
-	if (is_edp(intel_dp)) {
+	if (is_edp(intel_dp))
 		ironlake_edp_panel_on(intel_dp);
-		ironlake_edp_panel_vdd_off(intel_dp);
-	}
+	ironlake_edp_panel_vdd_off(intel_dp);
 
 	intel_dp_complete_link_train(intel_dp);
 
@@ -1070,15 +1072,13 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
 			ironlake_edp_pll_off(encoder);
 	} else {
-		if (is_edp(intel_dp))
-			ironlake_edp_panel_vdd_on(intel_dp);
+		ironlake_edp_panel_vdd_on(intel_dp);
 		intel_dp_sink_dpms(intel_dp, mode);
 		if (!(dp_reg & DP_PORT_EN)) {
 			intel_dp_start_link_train(intel_dp);
-			if (is_edp(intel_dp)) {
+			if (is_edp(intel_dp))
 				ironlake_edp_panel_on(intel_dp);
-				ironlake_edp_panel_vdd_off(intel_dp);
-			}
+			ironlake_edp_panel_vdd_off(intel_dp);
 			intel_dp_complete_link_train(intel_dp);
 		}
 		if (is_edp(intel_dp))
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 07/21] drm/i915: Check for eDP inside intel_edp_panel_vdd_on/off

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:39PM -0700, Keith Packard wrote:
> Cleans up code dealing with eDP VDD a bit.  Remove redundant checks in
> callers
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++----------
>  1 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a983d0f..da725d8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -832,6 +832,8 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 pp;
>  
> +	if (!is_edp(intel_dp))
> +		return;
>  	/*
>  	 * If the panel wasn't on, make sure there's not a currently
>  	 * active PP sequence before enabling AUX VDD.
> @@ -853,6 +855,8 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 pp;
>  
> +	if (!is_edp(intel_dp))
> +		return;
>  	pp = I915_READ(PCH_PP_CONTROL);
>  	pp &= ~PANEL_UNLOCK_MASK;
>  	pp |= PANEL_UNLOCK_REGS;
> @@ -1034,15 +1038,13 @@ static void intel_dp_commit(struct drm_encoder *encoder)
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	struct drm_device *dev = encoder->dev;
>  
> -	if (is_edp(intel_dp))
> -		ironlake_edp_panel_vdd_on(intel_dp);
> +	ironlake_edp_panel_vdd_on(intel_dp);
>  
>  	intel_dp_start_link_train(intel_dp);
>  
> -	if (is_edp(intel_dp)) {
> +	if (is_edp(intel_dp))
>  		ironlake_edp_panel_on(intel_dp);

Why not also move the id_edp check into edp_panel_on|off like for the vdd
functions? This way it looks a bit inconsistent ...
-Daniel

> -		ironlake_edp_panel_vdd_off(intel_dp);
> -	}
> +	ironlake_edp_panel_vdd_off(intel_dp);
>  
>  	intel_dp_complete_link_train(intel_dp);
>  
> @@ -1070,15 +1072,13 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
>  		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
>  			ironlake_edp_pll_off(encoder);
>  	} else {
> -		if (is_edp(intel_dp))
> -			ironlake_edp_panel_vdd_on(intel_dp);
> +		ironlake_edp_panel_vdd_on(intel_dp);
>  		intel_dp_sink_dpms(intel_dp, mode);
>  		if (!(dp_reg & DP_PORT_EN)) {
>  			intel_dp_start_link_train(intel_dp);
> -			if (is_edp(intel_dp)) {
> +			if (is_edp(intel_dp))
>  				ironlake_edp_panel_on(intel_dp);
> -				ironlake_edp_panel_vdd_off(intel_dp);
> -			}
> +			ironlake_edp_panel_vdd_off(intel_dp);
>  			intel_dp_complete_link_train(intel_dp);
>  		}
>  		if (is_edp(intel_dp))
> -- 
> 1.7.6.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 07/21] drm/i915: Check for eDP inside intel_edp_panel_vdd_on/off

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]

On Fri, 30 Sep 2011 19:13:55 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> Why not also move the id_edp check into edp_panel_on|off like for the vdd
> functions? This way it looks a bit inconsistent ...

Yeah, I can do that. May mean a few redundant checks, but they're cheap.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 08/21] drm/i915: Turn force VDD back off when panel running in intel_dp_dpms

[Keith Packard]

The VDD force bit is turned on before touching the panel, but if it
was enabled, there was no call to turn it back off. Add a call.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index da725d8..b915fc3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1080,7 +1080,8 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 				ironlake_edp_panel_on(intel_dp);
 			ironlake_edp_panel_vdd_off(intel_dp);
 			intel_dp_complete_link_train(intel_dp);
-		}
+		} else
+			ironlake_edp_panel_vdd_off(intel_dp);
 		if (is_edp(intel_dp))
 			ironlake_edp_backlight_on(dev);
 	}
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 08/21] drm/i915: Turn force VDD back off when panel running in intel_dp_dpms

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:40PM -0700, Keith Packard wrote:
> The VDD force bit is turned on before touching the panel, but if it
> was enabled, there was no call to turn it back off. Add a call.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Nice catch of in inbalanced on/off.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 09/21] drm/i915: Delay DP i2c initialization until panel power timings are computed

[Keith Packard]

On eDP, DDC requires panel power, but turning that on uses the panel
power sequencing timing values fetch from the DPCD data.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b915fc3..0b462f9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2069,8 +2069,6 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			break;
 	}
 
-	intel_dp_i2c_init(intel_dp, intel_connector, name);
-
 	/* Cache some DPCD data in the eDP case */
 	if (is_edp(intel_dp)) {
 		bool ret;
@@ -2102,6 +2100,8 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 		}
 	}
 
+	intel_dp_i2c_init(intel_dp, intel_connector, name);
+
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
 	if (is_edp(intel_dp)) {
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 09/21] drm/i915: Delay DP i2c initialization until panel power timings are computed

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:41PM -0700, Keith Packard wrote:
> On eDP, DDC requires panel power, but turning that on uses the panel
> power sequencing timing values fetch from the DPCD data.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Can't really check more than what the patch does what it claims to do, so
a weak:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

And a small question from the clueless: With your patch
intel_dp_i2c_auch_ch checks whether panel or vdd is on, but I don't see
where we turn that on and the drm dp i2c core seems to do a few
transactions on setup time. Should we splatter a vdd_on/off around the
i2c_init or is the panel guaranteed to have power at init time?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 10/21] drm/i915: Wrap DP EDID fetch functions to enable eDP panel power

[Keith Packard]

Talking to the eDP DDC channel requires that the panel be powered
up. Wrap both the EDID and modes fetch code with calls to turn the vdd
power on and back off.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0b462f9..9829581 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1711,6 +1711,31 @@ g4x_dp_detect(struct intel_dp *intel_dp)
 	return intel_dp_detect_dpcd(intel_dp);
 }
 
+static struct edid *
+intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct edid	*edid;
+
+	ironlake_edp_panel_vdd_on(intel_dp);
+	edid = drm_get_edid(connector, adapter);
+	ironlake_edp_panel_vdd_off(intel_dp);
+	return edid;
+}
+
+static int
+intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *adapter)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	int	ret;
+
+	ironlake_edp_panel_vdd_on(intel_dp);
+	ret = intel_ddc_get_modes(connector, adapter);
+	ironlake_edp_panel_vdd_off(intel_dp);
+	return ret;
+}
+
+
 /**
  * Uses CRT_HOTPLUG_EN and CRT_HOTPLUG_STAT to detect DP connection.
  *
@@ -1743,7 +1768,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	if (intel_dp->force_audio) {
 		intel_dp->has_audio = intel_dp->force_audio > 0;
 	} else {
-		edid = drm_get_edid(connector, &intel_dp->adapter);
+		edid = intel_dp_get_edid(connector, &intel_dp->adapter);
 		if (edid) {
 			intel_dp->has_audio = drm_detect_monitor_audio(edid);
 			connector->display_info.raw_edid = NULL;
@@ -1764,7 +1789,7 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 	/* We should parse the EDID data and find out if it has an audio sink
 	 */
 
-	ret = intel_ddc_get_modes(connector, &intel_dp->adapter);
+	ret = intel_dp_get_edid_modes(connector, &intel_dp->adapter);
 	if (ret) {
 		if (is_edp(intel_dp) && !dev_priv->panel_fixed_mode) {
 			struct drm_display_mode *newmode;
@@ -1809,7 +1834,7 @@ intel_dp_detect_audio(struct drm_connector *connector)
 	struct edid *edid;
 	bool has_audio = false;
 
-	edid = drm_get_edid(connector, &intel_dp->adapter);
+	edid = intel_dp_get_edid(connector, &intel_dp->adapter);
 	if (edid) {
 		has_audio = drm_detect_monitor_audio(edid);
 
-- 
1.7.6.3

Re: [PATCH 10/21] drm/i915: Wrap DP EDID fetch functions to enable eDP panel power

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:42PM -0700, Keith Packard wrote:
> Talking to the eDP DDC channel requires that the panel be powered
> up. Wrap both the EDID and modes fetch code with calls to turn the vdd
> power on and back off.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 10/21] drm/i915: Wrap DP EDID fetch functions to enable eDP panel power

[Jesse Barnes]

On Thu, 29 Sep 2011 18:09:42 -0700
Keith Packard <keithp@keithp.com> wrote:

> Talking to the eDP DDC channel requires that the panel be powered
> up. Wrap both the EDID and modes fetch code with calls to turn the vdd
> power on and back off.
> 

These VDD AUX changes look good, ack on all of them.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 11/21] drm/i915: Enable eDP panel power during I2C initialization sequence

[Keith Packard]

The DP i2c initialization code does a couple of i2c transactions,
which means that an eDP panel must be powered up.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9829581..414aef7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -595,10 +595,15 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 	return -EREMOTEIO;
 }
 
+static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp);
+static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp);
+
 static int
 intel_dp_i2c_init(struct intel_dp *intel_dp,
 		  struct intel_connector *intel_connector, const char *name)
 {
+	int	ret;
+
 	DRM_DEBUG_KMS("i2c_init %s\n", name);
 	intel_dp->algo.running = false;
 	intel_dp->algo.address = 0;
@@ -612,7 +617,10 @@ intel_dp_i2c_init(struct intel_dp *intel_dp,
 	intel_dp->adapter.algo_data = &intel_dp->algo;
 	intel_dp->adapter.dev.parent = &intel_connector->base.kdev;
 
-	return i2c_dp_aux_add_bus(&intel_dp->adapter);
+	ironlake_edp_panel_vdd_on(intel_dp);
+	ret = i2c_dp_aux_add_bus(&intel_dp->adapter);
+	ironlake_edp_panel_vdd_off(intel_dp);
+	return ret;
 }
 
 static bool
-- 
1.7.6.3

Re: [PATCH 11/21] drm/i915: Enable eDP panel power during I2C initialization sequence

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:43PM -0700, Keith Packard wrote:
> The DP i2c initialization code does a couple of i2c transactions,
> which means that an eDP panel must be powered up.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Ah, here's the code I've been looking for when reviewing patch 9.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 12/21] drm/i915: Ensure eDP powered up during DP_SET_POWER operation in dp_prepare

[Keith Packard]

Any call to intel_dp_sink_dpms must ensure that the panel has power so
that the DP_SET_POWER operation will be correctly received. The only
one missing this was in intel_dp_prepare.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 414aef7..a091a18 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1028,7 +1028,9 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
 	struct drm_device *dev = encoder->dev;
 
 	/* Wake up the sink first */
+	ironlake_edp_panel_vdd_on(intel_dp);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
+	ironlake_edp_panel_vdd_off(intel_dp);
 
 	if (is_edp(intel_dp)) {
 		ironlake_edp_backlight_off(dev);
-- 
1.7.6.3

Re: [PATCH 12/21] drm/i915: Ensure eDP powered up during DP_SET_POWER operation in dp_prepare

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:44PM -0700, Keith Packard wrote:
> Any call to intel_dp_sink_dpms must ensure that the panel has power so
> that the DP_SET_POWER operation will be correctly received. The only
> one missing this was in intel_dp_prepare.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
If I understand things correctly we don't need the vdd_on/off on dpms off
because the panel is running and has power. Assuming my understanding is
correct:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 12/21] drm/i915: Ensure eDP powered up during DP_SET_POWER operation in dp_prepare

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

On Fri, 30 Sep 2011 19:45:02 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> If I understand things correctly we don't need the vdd_on/off on dpms off
> because the panel is running and has power.

Yes, that's correct; the aux channel works with either full power or
with the force VDD bit turned on.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 13/21] drm/i915: Place long delays after each eDP VDD operation

[Keith Packard]

This ensures that the panel power sequencing is complete before
attempting to communicate over the aux channel.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a091a18..fbc19e4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -842,12 +842,15 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 
 	if (!is_edp(intel_dp))
 		return;
+	DRM_DEBUG_KMS("Turn eDP VDD on\n");
 	/*
 	 * If the panel wasn't on, make sure there's not a currently
 	 * active PP sequence before enabling AUX VDD.
 	 */
-	if (!(I915_READ(PCH_PP_STATUS) & PP_ON))
+	if (!(I915_READ(PCH_PP_STATUS) & PP_ON)) {
+		DRM_DEBUG_KMS("eDP VDD was not on\n");
 		msleep(dev_priv->panel_t3);
+	}
 
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
@@ -855,6 +858,9 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	pp |= EDP_FORCE_VDD;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
+	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
+		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
+	msleep(1000);
 }
 
 static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
@@ -865,6 +871,7 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 
 	if (!is_edp(intel_dp))
 		return;
+	DRM_DEBUG_KMS("Turn eDP VDD off\n");
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
@@ -874,6 +881,9 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 
 	/* Make sure sequencer is idle before allowing subsequent activity */
 	msleep(dev_priv->panel_t12);
+	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
+		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
+	msleep(1000);
 }
 
 /* Returns true if the panel was already on when called */
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 13/21] drm/i915: Place long delays after each eDP VDD operation

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:45PM -0700, Keith Packard wrote:
> This ensures that the panel power sequencing is complete before
> attempting to communicate over the aux channel.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Looks like a patch useful for fixing up this mess, but I don't quite see
the point of merging this given that you fix things for real in the next
one ... 
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 13/21] drm/i915: Place long delays after each eDP VDD operation

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]

On Fri, 30 Sep 2011 20:01:11 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> Looks like a patch useful for fixing up this mess, but I don't quite see
> the point of merging this given that you fix things for real in the next
> one ...

Good point. In the development process, this was the patch which made
things work, so it's purely an artifact of that. I'll merge this with
the subsequent patch.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations

[Keith Packard]

Store the panel power sequencing delays in the dp private structure,
rather than the global device structure. Who knows, maybe we'll get
more than one eDP device in the future.

Look at both the current hardware register settings and the VBT
specified panel power sequencing timings. Use the maximum of the two
delays, to make sure things work reliably. If there is no VBT data,
then those values will be initialized to zero, so we'll just use the
values as programmed in the hardware.

This patch computes power-up and power-down delays, rather than using
portions of the appropriate delay values as found in the hardware. The
eDP specified delay between raising VCC and communicating over the aux
channel includes both the power rise time (T1) and the aux channel
communication delay (T3). The eDP specified delay between powering
down the device and powering it back up includes both the power fall
time (T11) and the device idle time (T12).

>From the hardware, I'm taking the T3 value from the PP_OFF_DELAYS
Power_Down_delay value, which is actually documented to be the 'T3
time sequence' value used 'during power up'. There aren't separate T1
and T2 values, but there is a combined T1+T2 value in the PP_ON_DELAYS
register, so I use that instead.

VBT doesn't provide any values for T1 or T2, so we'll always just use
the hardware value for that.

The panel power up delay is thus T1 + T2 + T3, which should be
sufficient in all cases.

The panel power down delay is T1 + T2 + T12, using T1+T2 as a proxy
for T11, which isn't available anywhere.

On the macbook air I'm testing with, this yields a power-up delay of
over 200ms and a power-down delay of over 600ms. It all works now, but
we're frobbing these power controls several times during mode setting,
making the whole process take an awfully long time.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 -
 drivers/gpu/drm/i915/intel_dp.c |   59 ++++++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7916bd9..bcdf58b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -672,7 +672,6 @@ typedef struct drm_i915_private {
 	unsigned int lvds_border_bits;
 	/* Panel fitter placement and size for Ironlake+ */
 	u32 pch_pf_pos, pch_pf_size;
-	int panel_t3, panel_t12;
 
 	struct drm_crtc *plane_to_crtc_mapping[2];
 	struct drm_crtc *pipe_to_crtc_mapping[2];
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fbc19e4..7ebbdff 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -59,6 +59,8 @@ struct intel_dp {
 	bool is_pch_edp;
 	uint8_t	train_set[4];
 	uint8_t link_status[DP_LINK_STATUS_SIZE];
+	int panel_power_up_delay;
+	int panel_power_down_delay;
 };
 
 /**
@@ -838,7 +840,7 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 pp;
+	u32 pp, pp_status;
 
 	if (!is_edp(intel_dp))
 		return;
@@ -847,10 +849,7 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	 * If the panel wasn't on, make sure there's not a currently
 	 * active PP sequence before enabling AUX VDD.
 	 */
-	if (!(I915_READ(PCH_PP_STATUS) & PP_ON)) {
-		DRM_DEBUG_KMS("eDP VDD was not on\n");
-		msleep(dev_priv->panel_t3);
-	}
+	pp_status = I915_READ(PCH_PP_STATUS);
 
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
@@ -860,7 +859,10 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	POSTING_READ(PCH_PP_CONTROL);
 	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
 		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
-	msleep(1000);
+	if (!(pp_status & PP_ON)) {
+		msleep(intel_dp->panel_power_up_delay);
+		DRM_DEBUG_KMS("eDP VDD was not on\n");
+	}
 }
 
 static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
@@ -880,10 +882,9 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 	POSTING_READ(PCH_PP_CONTROL);
 
 	/* Make sure sequencer is idle before allowing subsequent activity */
-	msleep(dev_priv->panel_t12);
 	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
 		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
-	msleep(1000);
+	msleep(intel_dp->panel_power_down_delay);
 }
 
 /* Returns true if the panel was already on when called */
@@ -921,8 +922,10 @@ static bool ironlake_edp_panel_on (struct intel_dp *intel_dp)
 	return false;
 }
 
-static void ironlake_edp_panel_off (struct drm_device *dev)
+static void ironlake_edp_panel_off(struct drm_encoder *encoder)
 {
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp, idle_off_mask = PP_ON | PP_SEQUENCE_MASK |
 		PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK;
@@ -939,6 +942,7 @@ static void ironlake_edp_panel_off (struct drm_device *dev)
 	pp &= ~POWER_TARGET_ON;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
+	msleep(intel_dp->panel_power_down_delay);
 
 	if (wait_for((I915_READ(PCH_PP_STATUS) & idle_off_mask) == 0, 5000))
 		DRM_ERROR("panel off wait timed out: 0x%08x\n",
@@ -1044,7 +1048,7 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
 
 	if (is_edp(intel_dp)) {
 		ironlake_edp_backlight_off(dev);
-		ironlake_edp_panel_off(dev);
+		ironlake_edp_panel_off(encoder);
 		if (!is_pch_edp(intel_dp))
 			ironlake_edp_pll_on(encoder);
 		else
@@ -1088,7 +1092,7 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 		intel_dp_sink_dpms(intel_dp, mode);
 		intel_dp_link_down(intel_dp);
 		if (is_edp(intel_dp))
-			ironlake_edp_panel_off(dev);
+			ironlake_edp_panel_off(encoder);
 		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
 			ironlake_edp_pll_off(encoder);
 	} else {
@@ -2117,16 +2121,39 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 	/* Cache some DPCD data in the eDP case */
 	if (is_edp(intel_dp)) {
 		bool ret;
-		u32 pp_on, pp_div;
+		u32 pp_on, pp_off, pp_div;
+		int current_t1_2;
+		int current_t3;
+		int current_t12;
+		int vbt_t3;
+		int vbt_t12;
 
 		pp_on = I915_READ(PCH_PP_ON_DELAYS);
+		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
 		pp_div = I915_READ(PCH_PP_DIVISOR);
 
 		/* Get T3 & T12 values (note: VESA not bspec terminology) */
-		dev_priv->panel_t3 = (pp_on & 0x1fff0000) >> 16;
-		dev_priv->panel_t3 /= 10; /* t3 in 100us units */
-		dev_priv->panel_t12 = pp_div & 0xf;
-		dev_priv->panel_t12 *= 100; /* t12 in 100ms units */
+
+		current_t1_2 = (pp_on & 0x1fff0000) >> 16;
+		current_t1_2 = (current_t1_2 + 9) / 10; /* t1+t2 in 100us units */
+
+		current_t3 = (pp_off & 0x1fff0000) >> 16;
+		current_t3 = (current_t3 + 9) / 10;	/* t3 in 100us units */
+
+		current_t12 = pp_div & 0xf;
+		current_t12 *= 100; /* t12 in 100ms units */
+
+		vbt_t3 = (dev_priv->edp.pps.t3 + 9) / 10;
+		vbt_t12 = (dev_priv->edp.pps.t12 + 9) / 10;
+		DRM_DEBUG_KMS("current t3 %d t12 %d\n",
+			      current_t3, current_t12);
+		DRM_DEBUG_KMS("VBT t3 %d t12 %d\n",
+			      vbt_t3, vbt_t12);
+
+		intel_dp->panel_power_up_delay = current_t1_2 + max(current_t3, vbt_t3);
+		intel_dp->panel_power_down_delay = current_t1_2 + max(current_t12, vbt_t12);
+		DRM_DEBUG_KMS("panel power up delay %d, power down delay %d\n",
+			      intel_dp->panel_power_up_delay, intel_dp->panel_power_down_delay);
 
 		ironlake_edp_panel_vdd_on(intel_dp);
 		ret = intel_dp_get_dpcd(intel_dp);
-- 
1.7.6.3

Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:46PM -0700, Keith Packard wrote:
> Store the panel power sequencing delays in the dp private structure,
> rather than the global device structure. Who knows, maybe we'll get
> more than one eDP device in the future.
> 
> Look at both the current hardware register settings and the VBT
> specified panel power sequencing timings. Use the maximum of the two
> delays, to make sure things work reliably. If there is no VBT data,
> then those values will be initialized to zero, so we'll just use the
> values as programmed in the hardware.
> 
> This patch computes power-up and power-down delays, rather than using
> portions of the appropriate delay values as found in the hardware. The
> eDP specified delay between raising VCC and communicating over the aux
> channel includes both the power rise time (T1) and the aux channel
> communication delay (T3). The eDP specified delay between powering
> down the device and powering it back up includes both the power fall
> time (T11) and the device idle time (T12).
> 
> From the hardware, I'm taking the T3 value from the PP_OFF_DELAYS
> Power_Down_delay value, which is actually documented to be the 'T3
> time sequence' value used 'during power up'. There aren't separate T1
> and T2 values, but there is a combined T1+T2 value in the PP_ON_DELAYS
> register, so I use that instead.
> 
> VBT doesn't provide any values for T1 or T2, so we'll always just use
> the hardware value for that.
> 
> The panel power up delay is thus T1 + T2 + T3, which should be
> sufficient in all cases.
> 
> The panel power down delay is T1 + T2 + T12, using T1+T2 as a proxy
> for T11, which isn't available anywhere.
> 
> On the macbook air I'm testing with, this yields a power-up delay of
> over 200ms and a power-down delay of over 600ms. It all works now, but
> we're frobbing these power controls several times during mode setting,
> making the whole process take an awfully long time.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Awesome patch description and the code agrees with what I've cross-checked
on bspec. The only fear I have is that we currently ignore the backlight
on/off timings and some panel probably relies on use waiting for backlight
on/off + panel on/off in total. But that's material for another patch.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

On Fri, 30 Sep 2011 20:16:44 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> Awesome patch description and the code agrees with what I've cross-checked
> on bspec. The only fear I have is that we currently ignore the backlight
> on/off timings and some panel probably relies on use waiting for backlight
> on/off + panel on/off in total. But that's material for another patch.

Thanks for all of your review. I'm off to lunch and some meetings, but
I should get the changes suggested merged into an updated patch sequence
this afternoon.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 2791 bytes --]

On Fri, 30 Sep 2011 20:16:44 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> Awesome patch description and the code agrees with what I've cross-checked
> on bspec. The only fear I have is that we currently ignore the backlight
> on/off timings and some panel probably relies on use waiting for backlight
> on/off + panel on/off in total. But that's material for another patch.

Ok, I've just read through the VBIOS source code to figure out how it
converts the edp_power_seq table into the panel delay register settings.

intel_bios.h says:

        struct edp_power_seq {
        	u16 t3;
        	u16 t7;
        	u16 t9;
        	u16 t10;
        	u16 t12;
        } __attribute__ ((packed));

The VBIOS source code says that these five words are:
                            Data        Code               
        DW      xxxx        T1+T2       T1_T2   (Power Up Delay)
        DW      yyyy        T3          T5      (Power-On to backlight On)
        DW      zzzz        T4          T6      (Backlight off to power off)
        DW      wwww        T5          T3      (power down delay)
        DW      tttt        T7          T4      (power cycle delay)

('Data' is how the initialized data is labeled, 'Code' is how the
struct is defined and used. Yes, they appear to be different).

And, reading the code that programs the eDP registers

        PP_ON_DELAYS = (T1_T2 << 16) | T5
        PP_OFF_DELAYS = (T3 << 16) | T6
        PP_DIVISOR = T4 / 1000 + 1

The BSPEC/BIOS names don't agree with the eDP spec. Here's my plan:

Data    Code            eDP spec        intel_dp name           What I'm using this for:

T1+T2   T1_T2           T1+T3           panel_power_up_delay    VCC on to AUX channel
T3      T5              T6+T8           backlight_on_delay      Link training to backlight on
T4      T6              T9              backlight_off_delay     Backlight off to video off
T5      T3              T10             panel_power_down_delay  Video off to VCC off
T7      T4              T11+T12         panel_power_cycle_delay VCC off to VCC on

It's trivial to pull the values back out of the PP registers, so I'll do
that and use the maximum of the two instead of preferring one or the
other. Waiting too long shouldn't ever hurt.

As far as the edp_power_seq register goes, I'd love to know where those
names came from. They don't quite match my interpretation, in
particular, the 't7' is almost certainly not the eDP T7 value, which is
the delay from valid video data on the link to a correct display,
which isn't a useful value as it doesn't impact the source at all.

Ok, so a bit more whacking of code and I'll post an updated patch that
uses all of these values as above.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations

[Daniel Vetter]

On Fri, Sep 30, 2011 at 08:31:13PM -0700, Keith Packard wrote:
> On Fri, 30 Sep 2011 20:16:44 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > Awesome patch description and the code agrees with what I've cross-checked
> > on bspec. The only fear I have is that we currently ignore the backlight
> > on/off timings and some panel probably relies on use waiting for backlight
> > on/off + panel on/off in total. But that's material for another patch.
> 
> Ok, I've just read through the VBIOS source code to figure out how it
> converts the edp_power_seq table into the panel delay register settings.
> 
> intel_bios.h says:
> 
>         struct edp_power_seq {
>         	u16 t3;
>         	u16 t7;
>         	u16 t9;
>         	u16 t10;
>         	u16 t12;
>         } __attribute__ ((packed));
> 
> The VBIOS source code says that these five words are:
>                             Data        Code               
>         DW      xxxx        T1+T2       T1_T2   (Power Up Delay)
>         DW      yyyy        T3          T5      (Power-On to backlight On)
>         DW      zzzz        T4          T6      (Backlight off to power off)
>         DW      wwww        T5          T3      (power down delay)
>         DW      tttt        T7          T4      (power cycle delay)
> 
> ('Data' is how the initialized data is labeled, 'Code' is how the
> struct is defined and used. Yes, they appear to be different).
> 
> And, reading the code that programs the eDP registers
> 
>         PP_ON_DELAYS = (T1_T2 << 16) | T5
>         PP_OFF_DELAYS = (T3 << 16) | T6
>         PP_DIVISOR = T4 / 1000 + 1
> 
> The BSPEC/BIOS names don't agree with the eDP spec. Here's my plan:
> 
> Data    Code            eDP spec        intel_dp name           What I'm using this for:
> 
> T1+T2   T1_T2           T1+T3           panel_power_up_delay    VCC on to AUX channel
> T3      T5              T6+T8           backlight_on_delay      Link training to backlight on
> T4      T6              T9              backlight_off_delay     Backlight off to video off
> T5      T3              T10             panel_power_down_delay  Video off to VCC off
> T7      T4              T11+T12         panel_power_cycle_delay VCC off to VCC on
> 
> It's trivial to pull the values back out of the PP registers, so I'll do
> that and use the maximum of the two instead of preferring one or the
> other. Waiting too long shouldn't ever hurt.
> 
> As far as the edp_power_seq register goes, I'd love to know where those
> names came from. They don't quite match my interpretation, in
> particular, the 't7' is almost certainly not the eDP T7 value, which is
> the delay from valid video data on the link to a correct display,
> which isn't a useful value as it doesn't impact the source at all.
> 
> Ok, so a bit more whacking of code and I'll post an updated patch that
> uses all of these values as above.

Sounds like a plan. On further bspec review I've noticed that the
backlight power sequencing values exist since the introduction of on-chip
panel power/lvds output control for the i855gm. Currently we ignore them
for lvds panels, too. I'm hoping a bit that this causes the occasional
"backlight doesn't light up after dpms on/out of resume" issue I sometimes
see on about all my laptops here.

So while you bang your head against this, can you add the backlight power
sequencing delays for lvds panels, too? The lvds panel power sequencing
should be imo safe - we just msleep-spin with wait_for until the power
sequencer has completed the request.

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Sat, 1 Oct 2011 11:59:09 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> So while you bang your head against this, can you add the backlight power
> sequencing delays for lvds panels, too? The lvds panel power sequencing
> should be imo safe - we just msleep-spin with wait_for until the power
> sequencer has completed the request.

That should be a separate patch set, probably by someone with a
reasonable range of hardware to actually test it. Somehow I've managed
to get rid of all of my non-PCH hardware...

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Sat, 1 Oct 2011 11:59:09 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> So while you bang your head against this, can you add the backlight power
> sequencing delays for lvds panels, too? The lvds panel power sequencing
> should be imo safe - we just msleep-spin with wait_for until the power
> sequencer has completed the request.

That should be a separate patch set, probably by someone with a
reasonable range of hardware to actually test it. Somehow I've managed
to get rid of all of my non-PCH hardware...

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations

[Daniel Vetter]

On Sat, Oct 1, 2011 at 21:01, Keith Packard <keithp@keithp.com> wrote:
> On Sat, 1 Oct 2011 11:59:09 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> So while you bang your head against this, can you add the backlight power
>> sequencing delays for lvds panels, too? The lvds panel power sequencing
>> should be imo safe - we just msleep-spin with wait_for until the power
>> sequencer has completed the request.
>
> That should be a separate patch set, probably by someone with a
> reasonable range of hardware to actually test it. Somehow I've managed
> to get rid of all of my non-PCH hardware...

Agreed, I've looked a bit at the lvds code and it's quite different,
backlight-wise. Btw, I also see backlight-not-lighting-up issues on
snb, so not only pre-pch-split. I'll look into this when your edp
fixes have settled.
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

[PATCH 15/21] drm/i915: Move eDP panel fixed mode from dev_priv to intel_dp

[Keith Packard]

This value doesn't come directly from the VBT, and so is rather
specific to the particular DP output.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 -
 drivers/gpu/drm/i915/intel_dp.c |   35 ++++++++++++++++-------------------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bcdf58b..e6dd19e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -347,7 +347,6 @@ typedef struct drm_i915_private {
 	/* LVDS info */
 	int backlight_level;  /* restore backlight to this value */
 	bool backlight_enabled;
-	struct drm_display_mode *panel_fixed_mode;
 	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
 	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7ebbdff..02b5162 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -61,6 +61,7 @@ struct intel_dp {
 	uint8_t link_status[DP_LINK_STATUS_SIZE];
 	int panel_power_up_delay;
 	int panel_power_down_delay;
+	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
 };
 
 /**
@@ -202,16 +203,14 @@ intel_dp_mode_valid(struct drm_connector *connector,
 		    struct drm_display_mode *mode)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp));
 	int max_lanes = intel_dp_max_lane_count(intel_dp);
 
-	if (is_edp(intel_dp) && dev_priv->panel_fixed_mode) {
-		if (mode->hdisplay > dev_priv->panel_fixed_mode->hdisplay)
+	if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) {
+		if (mode->hdisplay > intel_dp->panel_fixed_mode->hdisplay)
 			return MODE_PANEL;
 
-		if (mode->vdisplay > dev_priv->panel_fixed_mode->vdisplay)
+		if (mode->vdisplay > intel_dp->panel_fixed_mode->vdisplay)
 			return MODE_PANEL;
 	}
 
@@ -630,22 +629,21 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		    struct drm_display_mode *adjusted_mode)
 {
 	struct drm_device *dev = encoder->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	int lane_count, clock;
 	int max_lane_count = intel_dp_max_lane_count(intel_dp);
 	int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
 	static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
 
-	if (is_edp(intel_dp) && dev_priv->panel_fixed_mode) {
-		intel_fixed_panel_mode(dev_priv->panel_fixed_mode, adjusted_mode);
+	if (is_edp(intel_dp) && intel_dp->panel_fixed_mode) {
+		intel_fixed_panel_mode(intel_dp->panel_fixed_mode, adjusted_mode);
 		intel_pch_panel_fitting(dev, DRM_MODE_SCALE_FULLSCREEN,
 					mode, adjusted_mode);
 		/*
 		 * the mode->clock is used to calculate the Data&Link M/N
 		 * of the pipe. For the eDP the fixed clock should be used.
 		 */
-		mode->clock = dev_priv->panel_fixed_mode->clock;
+		mode->clock = intel_dp->panel_fixed_mode->clock;
 	}
 
 	for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
@@ -1815,35 +1813,34 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 
 	ret = intel_dp_get_edid_modes(connector, &intel_dp->adapter);
 	if (ret) {
-		if (is_edp(intel_dp) && !dev_priv->panel_fixed_mode) {
+		if (is_edp(intel_dp) && !intel_dp->panel_fixed_mode) {
 			struct drm_display_mode *newmode;
 			list_for_each_entry(newmode, &connector->probed_modes,
 					    head) {
-				if (newmode->type & DRM_MODE_TYPE_PREFERRED) {
-					dev_priv->panel_fixed_mode =
+				if ((newmode->type & DRM_MODE_TYPE_PREFERRED)) {
+					intel_dp->panel_fixed_mode =
 						drm_mode_duplicate(dev, newmode);
 					break;
 				}
 			}
 		}
-
 		return ret;
 	}
 
 	/* if eDP has no EDID, try to use fixed panel mode from VBT */
 	if (is_edp(intel_dp)) {
 		/* initialize panel mode from VBT if available for eDP */
-		if (dev_priv->panel_fixed_mode == NULL && dev_priv->lfp_lvds_vbt_mode != NULL) {
-			dev_priv->panel_fixed_mode =
+		if (intel_dp->panel_fixed_mode == NULL && dev_priv->lfp_lvds_vbt_mode != NULL) {
+			intel_dp->panel_fixed_mode =
 				drm_mode_duplicate(dev, dev_priv->lfp_lvds_vbt_mode);
-			if (dev_priv->panel_fixed_mode) {
-				dev_priv->panel_fixed_mode->type |=
+			if (intel_dp->panel_fixed_mode) {
+				intel_dp->panel_fixed_mode->type |=
 					DRM_MODE_TYPE_PREFERRED;
 			}
 		}
-		if (dev_priv->panel_fixed_mode) {
+		if (intel_dp->panel_fixed_mode) {
 			struct drm_display_mode *mode;
-			mode = drm_mode_duplicate(dev, dev_priv->panel_fixed_mode);
+			mode = drm_mode_duplicate(dev, intel_dp->panel_fixed_mode);
 			drm_mode_probed_add(connector, mode);
 			return 1;
 		}
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 15/21] drm/i915: Move eDP panel fixed mode from dev_priv to intel_dp

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:47PM -0700, Keith Packard wrote:
> This value doesn't come directly from the VBT, and so is rather
> specific to the particular DP output.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 16/21] drm/i915: edp_panel_on does not need to return a bool

[Keith Packard]

The return value was unused, so just stop doing that.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 02b5162..56146a2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -886,14 +886,14 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
 }
 
 /* Returns true if the panel was already on when called */
-static bool ironlake_edp_panel_on (struct intel_dp *intel_dp)
+static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp, idle_on_mask = PP_ON | PP_SEQUENCE_STATE_ON_IDLE;
 
 	if (I915_READ(PCH_PP_STATUS) & PP_ON)
-		return true;
+		return;
 
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
@@ -916,8 +916,6 @@ static bool ironlake_edp_panel_on (struct intel_dp *intel_dp)
 	pp |= PANEL_POWER_RESET; /* restore panel reset bit */
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
-
-	return false;
 }
 
 static void ironlake_edp_panel_off(struct drm_encoder *encoder)
-- 
1.7.6.3

Re: [PATCH 16/21] drm/i915: edp_panel_on does not need to return a bool

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:48PM -0700, Keith Packard wrote:
> The return value was unused, so just stop doing that.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 16/21] drm/i915: edp_panel_on does not need to return a bool

[Jesse Barnes]

On Thu, 29 Sep 2011 18:09:48 -0700
Keith Packard <keithp@keithp.com> wrote:

> The return value was unused, so just stop doing that.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 02b5162..56146a2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -886,14 +886,14 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
>  }
>  
>  /* Returns true if the panel was already on when called */
> -static bool ironlake_edp_panel_on (struct intel_dp *intel_dp)
> +static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
>  {

Remove the comment too?

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 17/21] drm/i915: Create helper functions to determine eDP power state

[Keith Packard]

We need to check eDP VDD force and panel on in several places, so
create some simple helper functions to avoid duplicating code.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   39 ++++++++++++++++++++++++++-------------
 1 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 56146a2..c7ccb46 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -280,20 +280,34 @@ intel_hrawclk(struct drm_device *dev)
 	}
 }
 
+static bool ironlake_edp_have_panel_power(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	return (I915_READ(PCH_PP_STATUS) & PP_ON) != 0;
+}
+
+static bool ironlake_edp_have_panel_vdd(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	return (I915_READ(PCH_PP_CONTROL) & EDP_FORCE_VDD) != 0;
+}
+
 static void
 intel_dp_check_edp(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 pp_status, pp_control;
+
 	if (!is_edp(intel_dp))
 		return;
-	pp_status = I915_READ(PCH_PP_STATUS);
-	pp_control = I915_READ(PCH_PP_CONTROL);
-	if ((pp_status & PP_ON) == 0 && (pp_control & EDP_FORCE_VDD) == 0) {
+	if (!ironlake_edp_have_panel_power(intel_dp) && !ironlake_edp_have_panel_vdd(intel_dp)) {
 		WARN(1, "eDP powered off while attempting aux channel communication.\n");
 		DRM_DEBUG_KMS("Status 0x%08x Control 0x%08x\n",
-			      pp_status,
+			      I915_READ(PCH_PP_STATUS),
 			      I915_READ(PCH_PP_CONTROL));
 	}
 }
@@ -838,16 +852,11 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 pp, pp_status;
+	u32 pp;
 
 	if (!is_edp(intel_dp))
 		return;
 	DRM_DEBUG_KMS("Turn eDP VDD on\n");
-	/*
-	 * If the panel wasn't on, make sure there's not a currently
-	 * active PP sequence before enabling AUX VDD.
-	 */
-	pp_status = I915_READ(PCH_PP_STATUS);
 
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
@@ -857,7 +866,11 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	POSTING_READ(PCH_PP_CONTROL);
 	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
 		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
-	if (!(pp_status & PP_ON)) {
+
+	/*
+	 * If the panel wasn't on, delay before accessing aux channel
+	 */
+	if (!ironlake_edp_have_panel_power(intel_dp)) {
 		msleep(intel_dp->panel_power_up_delay);
 		DRM_DEBUG_KMS("eDP VDD was not on\n");
 	}
@@ -892,7 +905,7 @@ static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp, idle_on_mask = PP_ON | PP_SEQUENCE_STATE_ON_IDLE;
 
-	if (I915_READ(PCH_PP_STATUS) & PP_ON)
+	if (ironlake_edp_have_panel_power(intel_dp))
 		return;
 
 	pp = I915_READ(PCH_PP_CONTROL);
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 17/21] drm/i915: Create helper functions to determine eDP power state

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:49PM -0700, Keith Packard wrote:
> We need to check eDP VDD force and panel on in several places, so
> create some simple helper functions to avoid duplicating code.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Ah, here's the code that kills my "... instead of re-reading PP_CONTROL"
comment ;-)
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Keith Packard]

There's no good reason to turn off the eDP force VDD bit synchronously
while probing devices; that just sticks a huge delay into all mode
setting paths. Instead, queue a delayed work proc to disable the VDD
force bit and then remember when that fires to ensure that the
appropriate delay is respected before trying to turn it back on.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |  121 +++++++++++++++++++++++++++++++--------
 1 files changed, 97 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c7ccb46..08817b0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -62,6 +62,9 @@ struct intel_dp {
 	int panel_power_up_delay;
 	int panel_power_down_delay;
 	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
+	struct delayed_work panel_vdd_work;
+	bool want_panel_vdd;
+	unsigned long panel_off_jiffies;
 };
 
 /**
@@ -611,7 +614,7 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 }
 
 static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp);
-static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp);
+static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 
 static int
 intel_dp_i2c_init(struct intel_dp *intel_dp,
@@ -634,7 +637,7 @@ intel_dp_i2c_init(struct intel_dp *intel_dp,
 
 	ironlake_edp_panel_vdd_on(intel_dp);
 	ret = i2c_dp_aux_add_bus(&intel_dp->adapter);
-	ironlake_edp_panel_vdd_off(intel_dp);
+	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return ret;
 }
 
@@ -848,6 +851,23 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	}
 }
 
+static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
+{
+	unsigned long	off_time;
+	unsigned long	delay;
+	DRM_DEBUG_KMS("Wait for panel power off time\n");
+	off_time = intel_dp->panel_off_jiffies + msecs_to_jiffies(intel_dp->panel_power_down_delay);
+	if (time_after(jiffies, off_time)) {
+		DRM_DEBUG_KMS("Time already passed");
+		return;
+	}
+	delay = jiffies_to_msecs(off_time - jiffies);
+	if (delay > intel_dp->panel_power_down_delay)
+		delay = intel_dp->panel_power_down_delay;
+	DRM_DEBUG_KMS("Waiting an additional %ld ms\n", delay);
+	msleep(delay);
+}
+
 static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
@@ -858,6 +878,16 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 		return;
 	DRM_DEBUG_KMS("Turn eDP VDD on\n");
 
+	WARN(intel_dp->want_panel_vdd,
+	     "eDP VDD already requested on\n");
+
+	intel_dp->want_panel_vdd = true;
+	if (ironlake_edp_have_panel_vdd(intel_dp)) {
+		DRM_DEBUG_KMS("eDP VDD already on\n");
+		return;
+	}
+
+	ironlake_wait_panel_off(intel_dp);
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
@@ -871,31 +901,64 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	 * If the panel wasn't on, delay before accessing aux channel
 	 */
 	if (!ironlake_edp_have_panel_power(intel_dp)) {
+		DRM_DEBUG_KMS("eDP was not running\n");
 		msleep(intel_dp->panel_power_up_delay);
-		DRM_DEBUG_KMS("eDP VDD was not on\n");
 	}
 }
 
-static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
+static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp;
 
+	if (!intel_dp->want_panel_vdd && ironlake_edp_have_panel_vdd(intel_dp)) {
+		pp = I915_READ(PCH_PP_CONTROL);
+		pp &= ~PANEL_UNLOCK_MASK;
+		pp |= PANEL_UNLOCK_REGS;
+		pp &= ~EDP_FORCE_VDD;
+		I915_WRITE(PCH_PP_CONTROL, pp);
+		POSTING_READ(PCH_PP_CONTROL);
+
+		/* Make sure sequencer is idle before allowing subsequent activity */
+		DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
+			      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
+		intel_dp->panel_off_jiffies = jiffies;
+	}
+}
+
+static void ironlake_panel_vdd_work(struct work_struct *__work)
+{
+	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
+						 struct intel_dp, panel_vdd_work);
+	struct drm_device *dev = intel_dp->base.base.dev;
+
+	mutex_lock(&dev->struct_mutex);
+	ironlake_panel_vdd_off_sync(intel_dp);
+	mutex_unlock(&dev->struct_mutex);
+}
+
+static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
+{
 	if (!is_edp(intel_dp))
 		return;
-	DRM_DEBUG_KMS("Turn eDP VDD off\n");
-	pp = I915_READ(PCH_PP_CONTROL);
-	pp &= ~PANEL_UNLOCK_MASK;
-	pp |= PANEL_UNLOCK_REGS;
-	pp &= ~EDP_FORCE_VDD;
-	I915_WRITE(PCH_PP_CONTROL, pp);
-	POSTING_READ(PCH_PP_CONTROL);
 
-	/* Make sure sequencer is idle before allowing subsequent activity */
-	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
-		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
-	msleep(intel_dp->panel_power_down_delay);
+	DRM_DEBUG_KMS("Turn eDP VDD off %d\n", intel_dp->want_panel_vdd);
+	WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
+	
+	intel_dp->want_panel_vdd = false;
+
+	if (sync) {
+		ironlake_panel_vdd_off_sync(intel_dp);
+	} else {
+		/*
+		 * Queue the timer to fire a long
+		 * time from now (relative to the power down delay)
+		 * to keep the panel power up across a sequence of operations
+		 */
+		schedule_delayed_work(&intel_dp->panel_vdd_work,
+				      msecs_to_jiffies(intel_dp->panel_power_down_delay * 5));
+	}
 }
 
 /* Returns true if the panel was already on when called */
@@ -908,6 +971,7 @@ static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
 	if (ironlake_edp_have_panel_power(intel_dp))
 		return;
 
+	ironlake_wait_panel_off(intel_dp);
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
@@ -951,7 +1015,6 @@ static void ironlake_edp_panel_off(struct drm_encoder *encoder)
 	pp &= ~POWER_TARGET_ON;
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
-	msleep(intel_dp->panel_power_down_delay);
 
 	if (wait_for((I915_READ(PCH_PP_STATUS) & idle_off_mask) == 0, 5000))
 		DRM_ERROR("panel off wait timed out: 0x%08x\n",
@@ -960,6 +1023,7 @@ static void ironlake_edp_panel_off(struct drm_encoder *encoder)
 	pp |= PANEL_POWER_RESET; /* restore panel reset bit */
 	I915_WRITE(PCH_PP_CONTROL, pp);
 	POSTING_READ(PCH_PP_CONTROL);
+	intel_dp->panel_off_jiffies = jiffies;
 }
 
 static void ironlake_edp_backlight_on (struct drm_device *dev)
@@ -1053,7 +1117,7 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
 	/* Wake up the sink first */
 	ironlake_edp_panel_vdd_on(intel_dp);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
-	ironlake_edp_panel_vdd_off(intel_dp);
+	ironlake_edp_panel_vdd_off(intel_dp, false);
 
 	if (is_edp(intel_dp)) {
 		ironlake_edp_backlight_off(dev);
@@ -1077,7 +1141,7 @@ static void intel_dp_commit(struct drm_encoder *encoder)
 
 	if (is_edp(intel_dp))
 		ironlake_edp_panel_on(intel_dp);
-	ironlake_edp_panel_vdd_off(intel_dp);
+	ironlake_edp_panel_vdd_off(intel_dp, true);
 
 	intel_dp_complete_link_train(intel_dp);
 
@@ -1111,10 +1175,10 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 			intel_dp_start_link_train(intel_dp);
 			if (is_edp(intel_dp))
 				ironlake_edp_panel_on(intel_dp);
-			ironlake_edp_panel_vdd_off(intel_dp);
+			ironlake_edp_panel_vdd_off(intel_dp, true);
 			intel_dp_complete_link_train(intel_dp);
 		} else
-			ironlake_edp_panel_vdd_off(intel_dp);
+			ironlake_edp_panel_vdd_off(intel_dp, false);
 		if (is_edp(intel_dp))
 			ironlake_edp_backlight_on(dev);
 	}
@@ -1752,7 +1816,7 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 	ironlake_edp_panel_vdd_on(intel_dp);
 	edid = drm_get_edid(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp);
+	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return edid;
 }
 
@@ -1764,7 +1828,7 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
 
 	ironlake_edp_panel_vdd_on(intel_dp);
 	ret = intel_ddc_get_modes(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp);
+	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return ret;
 }
 
@@ -1951,6 +2015,10 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 
 	i2c_del_adapter(&intel_dp->adapter);
 	drm_encoder_cleanup(encoder);
+	if (is_edp(intel_dp)) {
+		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
+		ironlake_panel_vdd_off_sync(intel_dp);
+	}
 	kfree(intel_dp);
 }
 
@@ -2087,8 +2155,11 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 	else if (output_reg == DP_D || output_reg == PCH_DP_D)
 		intel_encoder->clone_mask = (1 << INTEL_DP_D_CLONE_BIT);
 
-	if (is_edp(intel_dp))
+	if (is_edp(intel_dp)) {
 		intel_encoder->clone_mask = (1 << INTEL_EDP_CLONE_BIT);
+		INIT_DELAYED_WORK(&intel_dp->panel_vdd_work,
+				  ironlake_panel_vdd_work);
+	}
 
 	intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
 	connector->interlace_allowed = true;
@@ -2163,9 +2234,11 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 		DRM_DEBUG_KMS("panel power up delay %d, power down delay %d\n",
 			      intel_dp->panel_power_up_delay, intel_dp->panel_power_down_delay);
 
+		intel_dp->panel_off_jiffies = jiffies - intel_dp->panel_power_down_delay;
+
 		ironlake_edp_panel_vdd_on(intel_dp);
 		ret = intel_dp_get_dpcd(intel_dp);
-		ironlake_edp_panel_vdd_off(intel_dp);
+		ironlake_edp_panel_vdd_off(intel_dp, false);
 		if (ret) {
 			if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
 				dev_priv->no_aux_handshake =
-- 
1.7.6.3

Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Chris Wilson]

On Thu, 29 Sep 2011 18:09:50 -0700, Keith Packard <keithp@keithp.com> wrote:
> +static void ironlake_panel_vdd_work(struct work_struct *__work)
> +{
> +	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
> +						 struct intel_dp, panel_vdd_work);
> +	struct drm_device *dev = intel_dp->base.base.dev;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	ironlake_panel_vdd_off_sync(intel_dp);
> +	mutex_unlock(&dev->struct_mutex);

I think you want the dev->mode_config.mutex here.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Chris Wilson]

On Thu, 29 Sep 2011 18:09:50 -0700, Keith Packard <keithp@keithp.com> wrote:
> +static void ironlake_panel_vdd_work(struct work_struct *__work)
> +{
> +	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
> +						 struct intel_dp, panel_vdd_work);
> +	struct drm_device *dev = intel_dp->base.base.dev;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	ironlake_panel_vdd_off_sync(intel_dp);
> +	mutex_unlock(&dev->struct_mutex);

I think you want the dev->mode_config.mutex here.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 192 bytes --]

On Fri, 30 Sep 2011 11:31:45 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> I think you want the dev->mode_config.mutex here.

oops. good catch.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:50PM -0700, Keith Packard wrote:
> There's no good reason to turn off the eDP force VDD bit synchronously
> while probing devices; that just sticks a huge delay into all mode
> setting paths. Instead, queue a delayed work proc to disable the VDD
> force bit and then remember when that fires to ensure that the
> appropriate delay is respected before trying to turn it back on.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |  121 +++++++++++++++++++++++++++++++--------
>  1 files changed, 97 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c7ccb46..08817b0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -62,6 +62,9 @@ struct intel_dp {
>  	int panel_power_up_delay;
>  	int panel_power_down_delay;
>  	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
> +	struct delayed_work panel_vdd_work;
> +	bool want_panel_vdd;
> +	unsigned long panel_off_jiffies;
>  };
>  
>  /**
> @@ -611,7 +614,7 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
>  }
>  
>  static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp);
> -static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp);
> +static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>  
>  static int
>  intel_dp_i2c_init(struct intel_dp *intel_dp,
> @@ -634,7 +637,7 @@ intel_dp_i2c_init(struct intel_dp *intel_dp,
>  
>  	ironlake_edp_panel_vdd_on(intel_dp);
>  	ret = i2c_dp_aux_add_bus(&intel_dp->adapter);
> -	ironlake_edp_panel_vdd_off(intel_dp);
> +	ironlake_edp_panel_vdd_off(intel_dp, false);
>  	return ret;
>  }
>  
> @@ -848,6 +851,23 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
>  	}
>  }
>  
> +static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
> +{
> +	unsigned long	off_time;
> +	unsigned long	delay;
> +	DRM_DEBUG_KMS("Wait for panel power off time\n");
> +	off_time = intel_dp->panel_off_jiffies + msecs_to_jiffies(intel_dp->panel_power_down_delay);
> +	if (time_after(jiffies, off_time)) {
> +		DRM_DEBUG_KMS("Time already passed");
> +		return;
> +	}
> +	delay = jiffies_to_msecs(off_time - jiffies);
> +	if (delay > intel_dp->panel_power_down_delay)
> +		delay = intel_dp->panel_power_down_delay;
> +	DRM_DEBUG_KMS("Waiting an additional %ld ms\n", delay);
> +	msleep(delay);
> +}

A cancel_work might be good here, not point in waking up the cpu for no
reason. And can you add "panel off timer: " to the deboug output?

> +
>  static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp->base.base.dev;
> @@ -858,6 +878,16 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
>  		return;
>  	DRM_DEBUG_KMS("Turn eDP VDD on\n");
>  
> +	WARN(intel_dp->want_panel_vdd,
> +	     "eDP VDD already requested on\n");
> +
> +	intel_dp->want_panel_vdd = true;
> +	if (ironlake_edp_have_panel_vdd(intel_dp)) {
> +		DRM_DEBUG_KMS("eDP VDD already on\n");
> +		return;
> +	}
> +
> +	ironlake_wait_panel_off(intel_dp);
>  	pp = I915_READ(PCH_PP_CONTROL);
>  	pp &= ~PANEL_UNLOCK_MASK;
>  	pp |= PANEL_UNLOCK_REGS;
> @@ -871,31 +901,64 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
>  	 * If the panel wasn't on, delay before accessing aux channel
>  	 */
>  	if (!ironlake_edp_have_panel_power(intel_dp)) {
> +		DRM_DEBUG_KMS("eDP was not running\n");
>  		msleep(intel_dp->panel_power_up_delay);
> -		DRM_DEBUG_KMS("eDP VDD was not on\n");
>  	}
>  }
>  
> -static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp)
> +static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 pp;
>  
> +	if (!intel_dp->want_panel_vdd && ironlake_edp_have_panel_vdd(intel_dp)) {
> +		pp = I915_READ(PCH_PP_CONTROL);
> +		pp &= ~PANEL_UNLOCK_MASK;
> +		pp |= PANEL_UNLOCK_REGS;
> +		pp &= ~EDP_FORCE_VDD;
> +		I915_WRITE(PCH_PP_CONTROL, pp);
> +		POSTING_READ(PCH_PP_CONTROL);
> +
> +		/* Make sure sequencer is idle before allowing subsequent activity */
> +		DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
> +			      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
> +		intel_dp->panel_off_jiffies = jiffies;
> +	}
> +}
> +
> +static void ironlake_panel_vdd_work(struct work_struct *__work)
> +{
> +	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
> +						 struct intel_dp, panel_vdd_work);
> +	struct drm_device *dev = intel_dp->base.base.dev;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	ironlake_panel_vdd_off_sync(intel_dp);
> +	mutex_unlock(&dev->struct_mutex);
> +}

Like Chris already mentioned, s/struct_mutex/mode_config.mutex/ Maybe also
move the intel_dp->want_panel_vdd check in here - we need it only here to
avoid a race with vdd_on. I've almost overlooked it and already started to
write the complaint about the race ;-)

> +
> +static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
> +{
>  	if (!is_edp(intel_dp))
>  		return;
> -	DRM_DEBUG_KMS("Turn eDP VDD off\n");
> -	pp = I915_READ(PCH_PP_CONTROL);
> -	pp &= ~PANEL_UNLOCK_MASK;
> -	pp |= PANEL_UNLOCK_REGS;
> -	pp &= ~EDP_FORCE_VDD;
> -	I915_WRITE(PCH_PP_CONTROL, pp);
> -	POSTING_READ(PCH_PP_CONTROL);
>  
> -	/* Make sure sequencer is idle before allowing subsequent activity */
> -	DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
> -		      I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
> -	msleep(intel_dp->panel_power_down_delay);
> +	DRM_DEBUG_KMS("Turn eDP VDD off %d\n", intel_dp->want_panel_vdd);
> +	WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
> +	
> +	intel_dp->want_panel_vdd = false;
> +
> +	if (sync) {
> +		ironlake_panel_vdd_off_sync(intel_dp);
> +	} else {
> +		/*
> +		 * Queue the timer to fire a long
> +		 * time from now (relative to the power down delay)
> +		 * to keep the panel power up across a sequence of operations
> +		 */
> +		schedule_delayed_work(&intel_dp->panel_vdd_work,
> +				      msecs_to_jiffies(intel_dp->panel_power_down_delay * 5));
> +	}
>  }
>  
>  /* Returns true if the panel was already on when called */
> @@ -908,6 +971,7 @@ static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
>  	if (ironlake_edp_have_panel_power(intel_dp))
>  		return;
>  
> +	ironlake_wait_panel_off(intel_dp);
>  	pp = I915_READ(PCH_PP_CONTROL);
>  	pp &= ~PANEL_UNLOCK_MASK;
>  	pp |= PANEL_UNLOCK_REGS;
> @@ -951,7 +1015,6 @@ static void ironlake_edp_panel_off(struct drm_encoder *encoder)
>  	pp &= ~POWER_TARGET_ON;
>  	I915_WRITE(PCH_PP_CONTROL, pp);
>  	POSTING_READ(PCH_PP_CONTROL);
> -	msleep(intel_dp->panel_power_down_delay);
>  
>  	if (wait_for((I915_READ(PCH_PP_STATUS) & idle_off_mask) == 0, 5000))
>  		DRM_ERROR("panel off wait timed out: 0x%08x\n",
> @@ -960,6 +1023,7 @@ static void ironlake_edp_panel_off(struct drm_encoder *encoder)
>  	pp |= PANEL_POWER_RESET; /* restore panel reset bit */
>  	I915_WRITE(PCH_PP_CONTROL, pp);
>  	POSTING_READ(PCH_PP_CONTROL);
> +	intel_dp->panel_off_jiffies = jiffies;
>  }
>  
>  static void ironlake_edp_backlight_on (struct drm_device *dev)
> @@ -1053,7 +1117,7 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
>  	/* Wake up the sink first */
>  	ironlake_edp_panel_vdd_on(intel_dp);
>  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> -	ironlake_edp_panel_vdd_off(intel_dp);
> +	ironlake_edp_panel_vdd_off(intel_dp, false);
>  
>  	if (is_edp(intel_dp)) {
>  		ironlake_edp_backlight_off(dev);
> @@ -1077,7 +1141,7 @@ static void intel_dp_commit(struct drm_encoder *encoder)
>  
>  	if (is_edp(intel_dp))
>  		ironlake_edp_panel_on(intel_dp);
> -	ironlake_edp_panel_vdd_off(intel_dp);
> +	ironlake_edp_panel_vdd_off(intel_dp, true);
>  
>  	intel_dp_complete_link_train(intel_dp);
>  
> @@ -1111,10 +1175,10 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
>  			intel_dp_start_link_train(intel_dp);
>  			if (is_edp(intel_dp))
>  				ironlake_edp_panel_on(intel_dp);
> -			ironlake_edp_panel_vdd_off(intel_dp);
> +			ironlake_edp_panel_vdd_off(intel_dp, true);
>  			intel_dp_complete_link_train(intel_dp);
>  		} else
> -			ironlake_edp_panel_vdd_off(intel_dp);
> +			ironlake_edp_panel_vdd_off(intel_dp, false);

Any reason why one vdd_off is sync while the other isn't?

>  		if (is_edp(intel_dp))
>  			ironlake_edp_backlight_on(dev);
>  	}
> @@ -1752,7 +1816,7 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  
>  	ironlake_edp_panel_vdd_on(intel_dp);
>  	edid = drm_get_edid(connector, adapter);
> -	ironlake_edp_panel_vdd_off(intel_dp);
> +	ironlake_edp_panel_vdd_off(intel_dp, false);
>  	return edid;
>  }
>  
> @@ -1764,7 +1828,7 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
>  
>  	ironlake_edp_panel_vdd_on(intel_dp);
>  	ret = intel_ddc_get_modes(connector, adapter);
> -	ironlake_edp_panel_vdd_off(intel_dp);
> +	ironlake_edp_panel_vdd_off(intel_dp, false);
>  	return ret;
>  }
>  
> @@ -1951,6 +2015,10 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  
>  	i2c_del_adapter(&intel_dp->adapter);
>  	drm_encoder_cleanup(encoder);
> +	if (is_edp(intel_dp)) {
> +		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> +		ironlake_panel_vdd_off_sync(intel_dp);
> +	}
>  	kfree(intel_dp);
>  }
>  
> @@ -2087,8 +2155,11 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  	else if (output_reg == DP_D || output_reg == PCH_DP_D)
>  		intel_encoder->clone_mask = (1 << INTEL_DP_D_CLONE_BIT);
>  
> -	if (is_edp(intel_dp))
> +	if (is_edp(intel_dp)) {
>  		intel_encoder->clone_mask = (1 << INTEL_EDP_CLONE_BIT);
> +		INIT_DELAYED_WORK(&intel_dp->panel_vdd_work,
> +				  ironlake_panel_vdd_work);
> +	}
>  
>  	intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
>  	connector->interlace_allowed = true;
> @@ -2163,9 +2234,11 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  		DRM_DEBUG_KMS("panel power up delay %d, power down delay %d\n",
>  			      intel_dp->panel_power_up_delay, intel_dp->panel_power_down_delay);
>  
> +		intel_dp->panel_off_jiffies = jiffies - intel_dp->panel_power_down_delay;
> +
>  		ironlake_edp_panel_vdd_on(intel_dp);
>  		ret = intel_dp_get_dpcd(intel_dp);
> -		ironlake_edp_panel_vdd_off(intel_dp);
> +		ironlake_edp_panel_vdd_off(intel_dp, false);
>  		if (ret) {
>  			if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
>  				dev_priv->no_aux_handshake =
> -- 
> 1.7.6.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]

On Fri, 30 Sep 2011 20:47:00 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> A cancel_work might be good here, not point in waking up the cpu for no
> reason. And can you add "panel off timer: " to the deboug output?

No, that's not correct, this is run before turning the panel back on and
must check that enough time has elapsed since it was turned off, which
may have happened in the work proc.

However, you are right in saying that I could call the cancel work
function when the panel is forced off synchronously. I'll add that.

> Like Chris already mentioned, s/struct_mutex/mode_config.mutex/ Maybe also
> move the intel_dp->want_panel_vdd check in here - we need it only here to
> avoid a race with vdd_on. I've almost overlooked it and already started to
> write the complaint about the race ;-)

With the right mutex held, there won't be a race, we just need to check the
value somewhere. Do you still see a race condition that needs to be avoided?

> > @@ -1111,10 +1175,10 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
> >  			intel_dp_start_link_train(intel_dp);
> >  			if (is_edp(intel_dp))
> >  				ironlake_edp_panel_on(intel_dp);
> > -			ironlake_edp_panel_vdd_off(intel_dp);
> > +			ironlake_edp_panel_vdd_off(intel_dp, true);
> >  			intel_dp_complete_link_train(intel_dp);
> >  		} else
> > -			ironlake_edp_panel_vdd_off(intel_dp);
> > +			ironlake_edp_panel_vdd_off(intel_dp, false);
> 
> Any reason why one vdd_off is sync while the other isn't?

After the panel is turned on, I want to synchronously turn off the VDD
AUX bits, as that's what the driver did before. That's 'free' because
the panel is on. In the other (failure) path, I don't want to impose a
delay if the application wants to try the mode set again.



-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

[Daniel Vetter]

On Fri, Sep 30, 2011 at 01:56:09PM -0700, Keith Packard wrote:
> On Fri, 30 Sep 2011 20:47:00 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > A cancel_work might be good here, not point in waking up the cpu for no
> > reason. And can you add "panel off timer: " to the deboug output?
> 
> No, that's not correct, this is run before turning the panel back on and
> must check that enough time has elapsed since it was turned off, which
> may have happened in the work proc.

You're right, I've got a bit confused between wait_panel_off and
vdd_panel_off.

> However, you are right in saying that I could call the cancel work
> function when the panel is forced off synchronously. I'll add that.

Sounds good.

> > Like Chris already mentioned, s/struct_mutex/mode_config.mutex/ Maybe also
> > move the intel_dp->want_panel_vdd check in here - we need it only here to
> > avoid a race with vdd_on. I've almost overlooked it and already started to
> > write the complaint about the race ;-)
> 
> With the right mutex held, there won't be a race, we just need to check the
> value somewhere. Do you still see a race condition that needs to be avoided?

Nope, with the right mutex protection the race is gone. And judging by
your other response my 2nd trial at formulating the issue I see seems to
have come out much better ;-)

> > > @@ -1111,10 +1175,10 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
> > >  			intel_dp_start_link_train(intel_dp);
> > >  			if (is_edp(intel_dp))
> > >  				ironlake_edp_panel_on(intel_dp);
> > > -			ironlake_edp_panel_vdd_off(intel_dp);
> > > +			ironlake_edp_panel_vdd_off(intel_dp, true);
> > >  			intel_dp_complete_link_train(intel_dp);
> > >  		} else
> > > -			ironlake_edp_panel_vdd_off(intel_dp);
> > > +			ironlake_edp_panel_vdd_off(intel_dp, false);
> > 
> > Any reason why one vdd_off is sync while the other isn't?
> 
> After the panel is turned on, I want to synchronously turn off the VDD
> AUX bits, as that's what the driver did before. That's 'free' because
> the panel is on. In the other (failure) path, I don't want to impose a
> delay if the application wants to try the mode set again.

Ok, makes sense. Maybe add a comment explaining why the sync case is free
for dummies like me?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 19/21] drm/i915: Asynchronous eDP panel power off

[Keith Packard]

Using the same basic plan as the VDD force delayed power off, make
turning the panel power off asynchronous.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   71 +++++++++++++++++++++++++++++----------
 1 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 08817b0..7120ba7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -62,8 +62,9 @@ struct intel_dp {
 	int panel_power_up_delay;
 	int panel_power_down_delay;
 	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
-	struct delayed_work panel_vdd_work;
+	struct delayed_work panel_work;
 	bool want_panel_vdd;
+	bool want_panel_power;
 	unsigned long panel_off_jiffies;
 };
 
@@ -906,7 +907,9 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
 	}
 }
 
-static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
+static void ironlake_edp_panel_off_sync(struct intel_dp *intel_dp);
+
+static void ironlake_edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -927,14 +930,15 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
 	}
 }
 
-static void ironlake_panel_vdd_work(struct work_struct *__work)
+static void ironlake_panel_work(struct work_struct *__work)
 {
 	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
-						 struct intel_dp, panel_vdd_work);
+						 struct intel_dp, panel_work);
 	struct drm_device *dev = intel_dp->base.base.dev;
 
 	mutex_lock(&dev->struct_mutex);
-	ironlake_panel_vdd_off_sync(intel_dp);
+	ironlake_edp_panel_vdd_off_sync(intel_dp);
+	ironlake_edp_panel_off_sync(intel_dp);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -943,20 +947,20 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
 	if (!is_edp(intel_dp))
 		return;
 
-	DRM_DEBUG_KMS("Turn eDP VDD off %d\n", intel_dp->want_panel_vdd);
+	DRM_DEBUG_KMS("Turn eDP VDD off %d\n", sync);
 	WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
 	
 	intel_dp->want_panel_vdd = false;
 
 	if (sync) {
-		ironlake_panel_vdd_off_sync(intel_dp);
+		ironlake_edp_panel_vdd_off_sync(intel_dp);
 	} else {
 		/*
 		 * Queue the timer to fire a long
 		 * time from now (relative to the power down delay)
 		 * to keep the panel power up across a sequence of operations
 		 */
-		schedule_delayed_work(&intel_dp->panel_vdd_work,
+		schedule_delayed_work(&intel_dp->panel_work,
 				      msecs_to_jiffies(intel_dp->panel_power_down_delay * 5));
 	}
 }
@@ -968,10 +972,16 @@ static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp, idle_on_mask = PP_ON | PP_SEQUENCE_STATE_ON_IDLE;
 
-	if (ironlake_edp_have_panel_power(intel_dp))
+	DRM_DEBUG_KMS("Turn eDP panel on\n");
+	if (ironlake_edp_have_panel_power(intel_dp)) {
+		DRM_DEBUG_KMS("eDP panel already on\n");
 		return;
+	}
 
 	ironlake_wait_panel_off(intel_dp);
+
+	intel_dp->want_panel_power = true;
+
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
@@ -995,14 +1005,16 @@ static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
 	POSTING_READ(PCH_PP_CONTROL);
 }
 
-static void ironlake_edp_panel_off(struct drm_encoder *encoder)
+static void ironlake_edp_panel_off_sync(struct intel_dp *intel_dp)
 {
-	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
-	struct drm_device *dev = encoder->dev;
+	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp, idle_off_mask = PP_ON | PP_SEQUENCE_MASK |
 		PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK;
 
+	if (intel_dp->want_panel_power || !ironlake_edp_have_panel_power(intel_dp))
+		return;
+
 	pp = I915_READ(PCH_PP_CONTROL);
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
@@ -1026,6 +1038,28 @@ static void ironlake_edp_panel_off(struct drm_encoder *encoder)
 	intel_dp->panel_off_jiffies = jiffies;
 }
 
+static void ironlake_edp_panel_off(struct drm_encoder *encoder, bool sync)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+
+	DRM_DEBUG_KMS("Turn eDP panel off %d\n", sync);
+
+	intel_dp->want_panel_power = false;
+
+	if (sync)
+		ironlake_edp_panel_off_sync(intel_dp);
+	else {
+		/*
+		 * Queue the timer to fire a long
+		 * time from now (relative to the power down delay)
+		 * to keep the panel power up across a sequence of operations
+		 */
+		schedule_delayed_work(&intel_dp->panel_work,
+				      msecs_to_jiffies(intel_dp->panel_power_down_delay * 5));
+	}
+		
+}
+
 static void ironlake_edp_backlight_on (struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1121,7 +1155,7 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
 
 	if (is_edp(intel_dp)) {
 		ironlake_edp_backlight_off(dev);
-		ironlake_edp_panel_off(encoder);
+		ironlake_edp_panel_off(encoder, false);
 		if (!is_pch_edp(intel_dp))
 			ironlake_edp_pll_on(encoder);
 		else
@@ -1165,7 +1199,7 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
 		intel_dp_sink_dpms(intel_dp, mode);
 		intel_dp_link_down(intel_dp);
 		if (is_edp(intel_dp))
-			ironlake_edp_panel_off(encoder);
+			ironlake_edp_panel_off(encoder, true);
 		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
 			ironlake_edp_pll_off(encoder);
 	} else {
@@ -2016,8 +2050,9 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	i2c_del_adapter(&intel_dp->adapter);
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
-		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
-		ironlake_panel_vdd_off_sync(intel_dp);
+		cancel_delayed_work_sync(&intel_dp->panel_work);
+		ironlake_edp_panel_vdd_off_sync(intel_dp);
+		ironlake_edp_panel_off_sync(intel_dp);
 	}
 	kfree(intel_dp);
 }
@@ -2157,8 +2192,8 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 
 	if (is_edp(intel_dp)) {
 		intel_encoder->clone_mask = (1 << INTEL_EDP_CLONE_BIT);
-		INIT_DELAYED_WORK(&intel_dp->panel_vdd_work,
-				  ironlake_panel_vdd_work);
+		INIT_DELAYED_WORK(&intel_dp->panel_work,
+				  ironlake_panel_work);
 	}
 
 	intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
-- 
1.7.6.3

Re: [PATCH 19/21] drm/i915: Asynchronous eDP panel power off

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:51PM -0700, Keith Packard wrote:
> Using the same basic plan as the VDD force delayed power off, make
> turning the panel power off asynchronous.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   71 +++++++++++++++++++++++++++++----------
>  1 files changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 08817b0..7120ba7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -62,8 +62,9 @@ struct intel_dp {
>  	int panel_power_up_delay;
>  	int panel_power_down_delay;
>  	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
> -	struct delayed_work panel_vdd_work;
> +	struct delayed_work panel_work;
>  	bool want_panel_vdd;
> +	bool want_panel_power;
>  	unsigned long panel_off_jiffies;
>  };
>  
> @@ -906,7 +907,9 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
>  	}
>  }
>  
> -static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
> +static void ironlake_edp_panel_off_sync(struct intel_dp *intel_dp);
> +
> +static void ironlake_edp_panel_vdd_off_sync(struct intel_dp *intel_dp)

If it's not too annoying to do, can you move this to the previous patch?
Dito for the s/panel_vdd_work/panel_work/.

>  {
>  	struct drm_device *dev = intel_dp->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -927,14 +930,15 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
>  	}
>  }
>  
> -static void ironlake_panel_vdd_work(struct work_struct *__work)
> +static void ironlake_panel_work(struct work_struct *__work)
>  {
>  	struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
> -						 struct intel_dp, panel_vdd_work);
> +						 struct intel_dp, panel_work);
>  	struct drm_device *dev = intel_dp->base.base.dev;
>  
>  	mutex_lock(&dev->struct_mutex);
> -	ironlake_panel_vdd_off_sync(intel_dp);
> +	ironlake_edp_panel_vdd_off_sync(intel_dp);
> +	ironlake_edp_panel_off_sync(intel_dp);
>  	mutex_unlock(&dev->struct_mutex);
>  }

Same comment as in the previous patch: I think the
intel_dp->want_panel_power check belongs into the work queue. We don't
want to hide the fact that we properly handle that race ;-)

>  
> @@ -943,20 +947,20 @@ static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  	if (!is_edp(intel_dp))
>  		return;
>  
> -	DRM_DEBUG_KMS("Turn eDP VDD off %d\n", intel_dp->want_panel_vdd);
> +	DRM_DEBUG_KMS("Turn eDP VDD off %d\n", sync);
>  	WARN(!intel_dp->want_panel_vdd, "eDP VDD not forced on");
>  	
>  	intel_dp->want_panel_vdd = false;
>  
>  	if (sync) {
> -		ironlake_panel_vdd_off_sync(intel_dp);
> +		ironlake_edp_panel_vdd_off_sync(intel_dp);
>  	} else {
>  		/*
>  		 * Queue the timer to fire a long
>  		 * time from now (relative to the power down delay)
>  		 * to keep the panel power up across a sequence of operations
>  		 */
> -		schedule_delayed_work(&intel_dp->panel_vdd_work,
> +		schedule_delayed_work(&intel_dp->panel_work,
>  				      msecs_to_jiffies(intel_dp->panel_power_down_delay * 5));
>  	}
>  }
> @@ -968,10 +972,16 @@ static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 pp, idle_on_mask = PP_ON | PP_SEQUENCE_STATE_ON_IDLE;
>  
> -	if (ironlake_edp_have_panel_power(intel_dp))
> +	DRM_DEBUG_KMS("Turn eDP panel on\n");
> +	if (ironlake_edp_have_panel_power(intel_dp)) {
> +		DRM_DEBUG_KMS("eDP panel already on\n");
>  		return;
> +	}
>  
>  	ironlake_wait_panel_off(intel_dp);
> +
> +	intel_dp->want_panel_power = true;
> +
>  	pp = I915_READ(PCH_PP_CONTROL);
>  	pp &= ~PANEL_UNLOCK_MASK;
>  	pp |= PANEL_UNLOCK_REGS;
> @@ -995,14 +1005,16 @@ static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
>  	POSTING_READ(PCH_PP_CONTROL);
>  }
>  
> -static void ironlake_edp_panel_off(struct drm_encoder *encoder)
> +static void ironlake_edp_panel_off_sync(struct intel_dp *intel_dp)
>  {
> -	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> -	struct drm_device *dev = encoder->dev;
> +	struct drm_device *dev = intel_dp->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 pp, idle_off_mask = PP_ON | PP_SEQUENCE_MASK |
>  		PP_CYCLE_DELAY_ACTIVE | PP_SEQUENCE_STATE_MASK;
>  
> +	if (intel_dp->want_panel_power || !ironlake_edp_have_panel_power(intel_dp))
> +		return;
> +
>  	pp = I915_READ(PCH_PP_CONTROL);
>  	pp &= ~PANEL_UNLOCK_MASK;
>  	pp |= PANEL_UNLOCK_REGS;
> @@ -1026,6 +1038,28 @@ static void ironlake_edp_panel_off(struct drm_encoder *encoder)
>  	intel_dp->panel_off_jiffies = jiffies;
>  }
>  
> +static void ironlake_edp_panel_off(struct drm_encoder *encoder, bool sync)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> +	DRM_DEBUG_KMS("Turn eDP panel off %d\n", sync);
> +
> +	intel_dp->want_panel_power = false;
> +
> +	if (sync)
> +		ironlake_edp_panel_off_sync(intel_dp);
> +	else {
> +		/*
> +		 * Queue the timer to fire a long
> +		 * time from now (relative to the power down delay)
> +		 * to keep the panel power up across a sequence of operations
> +		 */
> +		schedule_delayed_work(&intel_dp->panel_work,
> +				      msecs_to_jiffies(intel_dp->panel_power_down_delay * 5));
> +	}
> +		
> +}
> +
>  static void ironlake_edp_backlight_on (struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1121,7 +1155,7 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
>  
>  	if (is_edp(intel_dp)) {
>  		ironlake_edp_backlight_off(dev);
> -		ironlake_edp_panel_off(encoder);
> +		ironlake_edp_panel_off(encoder, false);
>  		if (!is_pch_edp(intel_dp))
>  			ironlake_edp_pll_on(encoder);
>  		else
> @@ -1165,7 +1199,7 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
>  		intel_dp_sink_dpms(intel_dp, mode);
>  		intel_dp_link_down(intel_dp);
>  		if (is_edp(intel_dp))
> -			ironlake_edp_panel_off(encoder);
> +			ironlake_edp_panel_off(encoder, true);
>  		if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
>  			ironlake_edp_pll_off(encoder);
>  	} else {
> @@ -2016,8 +2050,9 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	i2c_del_adapter(&intel_dp->adapter);
>  	drm_encoder_cleanup(encoder);
>  	if (is_edp(intel_dp)) {
> -		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> -		ironlake_panel_vdd_off_sync(intel_dp);
> +		cancel_delayed_work_sync(&intel_dp->panel_work);
> +		ironlake_edp_panel_vdd_off_sync(intel_dp);
> +		ironlake_edp_panel_off_sync(intel_dp);
>  	}
>  	kfree(intel_dp);
>  }
> @@ -2157,8 +2192,8 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  
>  	if (is_edp(intel_dp)) {
>  		intel_encoder->clone_mask = (1 << INTEL_EDP_CLONE_BIT);
> -		INIT_DELAYED_WORK(&intel_dp->panel_vdd_work,
> -				  ironlake_panel_vdd_work);
> +		INIT_DELAYED_WORK(&intel_dp->panel_work,
> +				  ironlake_panel_work);
>  	}
>  
>  	intel_encoder->crtc_mask = (1 << 0) | (1 << 1);
> -- 
> 1.7.6.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 19/21] drm/i915: Asynchronous eDP panel power off

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Fri, 30 Sep 2011 20:55:24 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> If it's not too annoying to do, can you move this to the previous patch?
> Dito for the s/panel_vdd_work/panel_work/.

Yeah, that was a bit of sloppy patch mangling.

> Same comment as in the previous patch: I think the
> intel_dp->want_panel_power check belongs into the work queue. We don't
> want to hide the fact that we properly handle that race ;-)

Oh, I see your point. Yeah, either the check needs to be moved, or the
function name changed. I'll move the check.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 19/21] drm/i915: Asynchronous eDP panel power off

[Dave Airlie]

> Using the same basic plan as the VDD force delayed power off, make
> turning the panel power off asynchronous.

NAK, tested on my 2540p, up to this patch in macbook-air branch stuff
worked, after this I just get black screen on resume.

Dave.

Re: [PATCH 19/21] drm/i915: Asynchronous eDP panel power off

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On Wed, 12 Oct 2011 15:41:11 +0100, Dave Airlie <airlied@gmail.com> wrote:

> > Using the same basic plan as the VDD force delayed power off, make
> > turning the panel power off asynchronous.
> 
> NAK, tested on my 2540p, up to this patch in macbook-air branch stuff
> worked, after this I just get black screen on resume.

Thanks for testing. I've created a new edp-training-fixes branch that
removes the async panel power off and leaves the rest of the branch.

I'll test on a 2540p that I've got access to today, and on the MBA when
I get home this evening.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

[PATCH 20/21] drm/i915: Restrict ILK-specific eDP power hack to ILK

[Keith Packard]

This eliminates a fairly long delay when power sequencing newer
hardware

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   51 +++++++++++++++++++++++----------------
 1 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7120ba7..f223504 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -986,10 +986,12 @@ static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
 
-	/* ILK workaround: disable reset around power sequence */
-	pp &= ~PANEL_POWER_RESET;
-	I915_WRITE(PCH_PP_CONTROL, pp);
-	POSTING_READ(PCH_PP_CONTROL);
+	if (IS_GEN5(dev)) {
+		/* ILK workaround: disable reset around power sequence */
+		pp &= ~PANEL_POWER_RESET;
+		I915_WRITE(PCH_PP_CONTROL, pp);
+		POSTING_READ(PCH_PP_CONTROL);
+	}
 
 	pp |= POWER_TARGET_ON;
 	I915_WRITE(PCH_PP_CONTROL, pp);
@@ -1000,9 +1002,11 @@ static void ironlake_edp_panel_on (struct intel_dp *intel_dp)
 		DRM_ERROR("panel on wait timed out: 0x%08x\n",
 			  I915_READ(PCH_PP_STATUS));
 
-	pp |= PANEL_POWER_RESET; /* restore panel reset bit */
-	I915_WRITE(PCH_PP_CONTROL, pp);
-	POSTING_READ(PCH_PP_CONTROL);
+	if (IS_GEN5(dev)) {
+		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
+		I915_WRITE(PCH_PP_CONTROL, pp);
+		POSTING_READ(PCH_PP_CONTROL);
+	}
 }
 
 static void ironlake_edp_panel_off_sync(struct intel_dp *intel_dp)
@@ -1019,23 +1023,28 @@ static void ironlake_edp_panel_off_sync(struct intel_dp *intel_dp)
 	pp &= ~PANEL_UNLOCK_MASK;
 	pp |= PANEL_UNLOCK_REGS;
 
-	/* ILK workaround: disable reset around power sequence */
-	pp &= ~PANEL_POWER_RESET;
-	I915_WRITE(PCH_PP_CONTROL, pp);
-	POSTING_READ(PCH_PP_CONTROL);
+	if (IS_GEN5(dev)) {
+		/* ILK workaround: disable reset around power sequence */
+		pp &= ~PANEL_POWER_RESET;
+		I915_WRITE(PCH_PP_CONTROL, pp);
+		POSTING_READ(PCH_PP_CONTROL);
+	}
 
-	pp &= ~POWER_TARGET_ON;
-	I915_WRITE(PCH_PP_CONTROL, pp);
-	POSTING_READ(PCH_PP_CONTROL);
+	intel_dp->panel_off_jiffies = jiffies;
 
-	if (wait_for((I915_READ(PCH_PP_STATUS) & idle_off_mask) == 0, 5000))
-		DRM_ERROR("panel off wait timed out: 0x%08x\n",
-			  I915_READ(PCH_PP_STATUS));
+	if (IS_GEN5(dev)) {
+		pp &= ~POWER_TARGET_ON;
+		I915_WRITE(PCH_PP_CONTROL, pp);
+		POSTING_READ(PCH_PP_CONTROL);
 
-	pp |= PANEL_POWER_RESET; /* restore panel reset bit */
-	I915_WRITE(PCH_PP_CONTROL, pp);
-	POSTING_READ(PCH_PP_CONTROL);
-	intel_dp->panel_off_jiffies = jiffies;
+		if (wait_for((I915_READ(PCH_PP_STATUS) & idle_off_mask) == 0, 5000))
+			DRM_ERROR("panel off wait timed out: 0x%08x\n",
+				  I915_READ(PCH_PP_STATUS));
+
+		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
+		I915_WRITE(PCH_PP_CONTROL, pp);
+		POSTING_READ(PCH_PP_CONTROL);
+	}
 }
 
 static void ironlake_edp_panel_off(struct drm_encoder *encoder, bool sync)
-- 
1.7.6.3

Re: [PATCH 20/21] drm/i915: Restrict ILK-specific eDP power hack to ILK

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:52PM -0700, Keith Packard wrote:
> This eliminates a fairly long delay when power sequencing newer
> hardware
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 21/21] drm/i915: No need to wait for eDP power off delay if panel is on

[Keith Packard]

If the panel is powered up, there's no need to delay for the 'off'
interval when turning the panel on.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f223504..a01cc2a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -856,7 +856,16 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
 {
 	unsigned long	off_time;
 	unsigned long	delay;
+
 	DRM_DEBUG_KMS("Wait for panel power off time\n");
+
+	if (ironlake_edp_have_panel_power(intel_dp) ||
+	    ironlake_edp_have_panel_vdd(intel_dp))
+	{
+		DRM_DEBUG_KMS("Panel still on, no delay needed\n");
+		return;
+	}
+
 	off_time = intel_dp->panel_off_jiffies + msecs_to_jiffies(intel_dp->panel_power_down_delay);
 	if (time_after(jiffies, off_time)) {
 		DRM_DEBUG_KMS("Time already passed");
-- 
1.7.6.3

Re: [PATCH 21/21] drm/i915: No need to wait for eDP power off delay if panel is on

[Daniel Vetter]

On Thu, Sep 29, 2011 at 06:09:53PM -0700, Keith Packard wrote:
> If the panel is powered up, there's no need to delay for the 'off'
> interval when turning the panel on.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

I'll hold off carefully checking this one until the previous patches
settle. Especially when we want to implement the backlight power
sequencing delays, a few things might move around still. Idea sounds good,
though.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 00/24] MacBook Air patch sequence (v2)

[Greg KH]

On Thu, Sep 29, 2011 at 06:09:32PM -0700, Keith Packard wrote:
> Ok, so I've split all of the changes into bite-sized pieces so that
> they should make sense individually now. I've also added the same
> asynchronous power control to the panel power, this reduces the
> module load time down to about 700ms on my MacBook Air, which is
> pretty nice.
> 
> Given the length of the series, I don't think this should all land in
> 3.1, however I'd like opinions on whether I should push this subset,
> which makes the MacBook Air work, into drm-intel-fixes and send that
> along.
> 
>  [PATCH 01/21] drm/i915: Enable digital port hotplug on PCH systems
>  [PATCH 02/21] drm/i915: Shut down PCH interrupts during
>  [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode
>  [PATCH 04/21] drm/i915: Only use VBT panel mode on eDP if no EDID is
>  [PATCH 05/21] drm/i915: Check eDP power when doing aux channel
>  [PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always
>  [PATCH 07/21] drm/i915: Check for eDP inside
>  [PATCH 08/21] drm/i915: Turn force VDD back off when panel running
>  [PATCH 09/21] drm/i915: Delay DP i2c initialization until panel
>  [PATCH 10/21] drm/i915: Wrap DP EDID fetch functions to enable eDP
>  [PATCH 11/21] drm/i915: Enable eDP panel power during I2C
>  [PATCH 12/21] drm/i915: Ensure eDP powered up during DP_SET_POWER
>  [PATCH 13/21] drm/i915: Place long delays after each eDP VDD
>  [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay
> 
> The diffstat for that sequence is:
> 
>  i915_drv.h |    1 
>  i915_irq.c |   28 ++++++++
>  i915_reg.h |    6 +
>  intel_dp.c |  194 ++++++++++++++++++++++++++++++++++++++++++++++---------------
>  4 files changed, 180 insertions(+), 49 deletions(-)
> 
> This might be too big at this point in the 3.1 release, so perhaps
> just marking these with a Cc: to stable would be more appropriate.

Are these really all -stable material?

I'm all for enabling new hardware like this, and overall, the patches
aren't that bad, just want to verify this.

And, I do have to tell you, "curses, now I have no excuse to not buy
that laptop!"

greg k-h

Re: [PATCH 00/24] MacBook Air patch sequence (v2)

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On Thu, 29 Sep 2011 20:33:56 -0700, Greg KH <greg@kroah.com> wrote:

> Are these really all -stable material?

I think just the sequence that actually makes the machine work; the
scarier patches are those which reduce the mode setting time from 5-10s
down to .7s.

Is this stretching the bounds of what is acceptable for -stable? Would
it look better as a single patch, instead of 14 separate ones?

> I'm all for enabling new hardware like this, and overall, the patches
> aren't that bad, just want to verify this.

Let me know what you think; they'll be queued for 3.2 once they've
gotten review and (I hope) more testing. It's Jesse's fault there are
so many little patches; he asked me to split things up into separate
functional changes. It's either that, or I'm just looking to increase
the number of patches I have in the kernel.

> And, I do have to tell you, "curses, now I have no excuse to not buy
> that laptop!"

I'd rather have a 'regular' PC; getting Debian installed on this machine
was no picnic. But, I haven't seen anything else in this form factor
that includes a display port connector.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 00/24] MacBook Air patch sequence (v2)

[Greg KH]

On Fri, Sep 30, 2011 at 01:58:29AM -0700, Keith Packard wrote:
> On Thu, 29 Sep 2011 20:33:56 -0700, Greg KH <greg@kroah.com> wrote:
> 
> > Are these really all -stable material?
> 
> I think just the sequence that actually makes the machine work; the
> scarier patches are those which reduce the mode setting time from 5-10s
> down to .7s.
> 
> Is this stretching the bounds of what is acceptable for -stable? Would
> it look better as a single patch, instead of 14 separate ones?

No, actually this makes it easier for -stable as each individual patch
fixes a problem, so in re-reading them, I have no objection for them to
go into -stable.

Would these also work on 3.0?

> > I'm all for enabling new hardware like this, and overall, the patches
> > aren't that bad, just want to verify this.
> 
> Let me know what you think; they'll be queued for 3.2 once they've
> gotten review and (I hope) more testing. It's Jesse's fault there are
> so many little patches; he asked me to split things up into separate
> functional changes. It's either that, or I'm just looking to increase
> the number of patches I have in the kernel.
> 
> > And, I do have to tell you, "curses, now I have no excuse to not buy
> > that laptop!"
> 
> I'd rather have a 'regular' PC; getting Debian installed on this machine
> was no picnic.

Due to UEFI stuff?  Or something else?

> But, I haven't seen anything else in this form factor that includes a
> display port connector.

I agree, it is a very nice hardware form factor that no other
manufacturer can seem to duplicate for the same (well, any) price these
days.

greg k-h

Re: [PATCH 00/24] MacBook Air patch sequence (v2)

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

On Fri, 30 Sep 2011 06:57:05 -0700, Greg KH <greg@kroah.com> wrote:

> No, actually this makes it easier for -stable as each individual patch
> fixes a problem, so in re-reading them, I have no objection for them to
> go into -stable.

Ok, cool. Splitting these up was a pain.

> Would these also work on 3.0?

They won't apply cleanly, but yes, they should be useful there.

> > I'd rather have a 'regular' PC; getting Debian installed on this machine
> > was no picnic.
> 
> Due to UEFI stuff?  Or something else?

UEFI, the lack of CD booting, and the general 'Steve knows best'
attitude of the hardware.

> I agree, it is a very nice hardware form factor that no other
> manufacturer can seem to duplicate for the same (well, any) price these
> days.

Yeah, PC vendors clearly aren't making hardware for us anymore.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 00/24] MacBook Air patch sequence (v2)

[Ted Ts'o]

On Thu, Sep 29, 2011 at 08:33:56PM -0700, Greg KH wrote:
> I'm all for enabling new hardware like this, and overall, the patches
> aren't that bad, just want to verify this.
> 
> And, I do have to tell you, "curses, now I have no excuse to not buy
> that laptop!"

What kind of battery life do you get with these patches applied, out
of curiosity?

Darn it, I have a Macbook Air, but I use it for media consumption
purposes.  This would be a great temption to get a second one as a
Linux box...

						- Ted

Re: [PATCH 00/24] MacBook Air patch sequence (v2)

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

On Fri, 30 Sep 2011 09:20:55 -0400, Ted Ts'o <tytso@mit.edu> wrote:

> What kind of battery life do you get with these patches applied, out
> of curiosity?

4-5 hours when doing the usual web-surfing, etc. Seems pretty much the
same as people are getting under Mac OS.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Intel-gfx] [PATCH 00/24] MacBook Air patch sequence (v2)

[Jesse Barnes]

On Fri, 30 Sep 2011 11:17:38 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Fri, 30 Sep 2011 09:20:55 -0400, Ted Ts'o <tytso@mit.edu> wrote:
> 
> > What kind of battery life do you get with these patches applied, out
> > of curiosity?
> 
> 4-5 hours when doing the usual web-surfing, etc. Seems pretty much the
> same as people are getting under Mac OS.

As long as you enable RC6. :)

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 00/24] MacBook Air patch sequence (v2)

[Chris Wilson]

On Thu, 29 Sep 2011 18:09:32 -0700, Keith Packard <keithp@keithp.com> wrote:
> Ok, so I've split all of the changes into bite-sized pieces so that
> they should make sense individually now. I've also added the same
> asynchronous power control to the panel power, this reduces the
> module load time down to about 700ms on my MacBook Air, which is
> pretty nice.
> 
> Given the length of the series, I don't think this should all land in
> 3.1, however I'd like opinions on whether I should push this subset,
> which makes the MacBook Air work, into drm-intel-fixes and send that
> along.

I can confirm that these indeed make the MBA (this one at least!) work,
but I don't have any other eDP device with which to cross-check.

Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre