drm/i915: eDP cleanup patch series -- fixes SNB MacBook Air
Subsystems (2)
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
We were relying on the BIOS to set these bits, which doesn't always happen.
--- drivers/gpu/drm/i915/i915_reg.h | 5 ++++- drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/i915_reg.h +4 -1
@@ -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)
drivers/gpu/drm/i915/intel_display.c +12 -0
@@ -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);
Signature · 1 line
1.7.6.3
PATCH 2/9 drm/i915: Remove extra 300ms delay during eDP mode setting
In reply to Keith PackardThere's no reason to enforce a 300ms delay during eDP mode setting.
--- drivers/gpu/drm/i915/intel_dp.c | 7 ------- 1 files changed, 0 insertions(+), 7 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +0 -7
@@ -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);
Signature · 1 line
1.7.6.3
PATCH 3/9 drm/i915: Only use VBT panel mode on eDP if no EDID is found
In reply to Keith PackardWe'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.
--- drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +10 -10
@@ -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); }
Signature · 1 line
1.7.6.3
PATCH 4/9 drm/i915: Check eDP power when doing aux channel communications
In reply to Keith PackardVerify 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.
--- drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +22 -0
@@ -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 ;
Signature · 1 line
1.7.6.3
PATCH 5/9 drm/i915: Unlock PCH_PP_CONTROL always
In reply to Keith PackardAvoid any question about locked registers by just writing the unlock pattern with every write to the register.
--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_dp.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/i915_reg.h +1 -0
@@ -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)
drivers/gpu/drm/i915/intel_dp.c +13 -1
@@ -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); }
Signature · 1 line
1.7.6.3
PATCH 6/9 drm/i915: Make sure eDP power is on before using aux channel
In reply to Keith PackardThe 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.
--- drivers/gpu/drm/i915/intel_dp.c | 84 +++++++++++++++++++++++++++++--------- 1 files changed, 64 insertions(+), 20 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +64 -20
@@ -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)) {
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Make sure eDP power is on before using aux channel
Quoted 9 lines · On Mon, 19 Sep 2011 15:22:00 -0700 Keith Packard <[email protected]> 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 <[email protected]>
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
Quoted 2 lines · Jesse Barnes
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.
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 2 lines
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).
Quoted 1 line
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.
Signature · 1 line
[email protected]
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Make sure eDP power is on before using aux channel
Quoted 15 lines · On Tue, 20 Sep 2011 21:45:54 -0700 Keith Packard <[email protected]> wrote:
Quoted 2 lines · Jesse Barnes
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_onandvdd_offto 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.
Quoted 8 lines
3) Hold VDD up through the whole dpms sequence. Also probably unnecessary.
4) Move
intel_dp_i2c_initpast the computation of the power
sequencing delays. Necessary asi2c_initmakes 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.
Quoted 8 lines
Quoted 2 lines
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).
Quoted 5 lines
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.
Quoted 5 lines
Can we replace any of the current VDD hacks with a full PPS? I can easily imagine moving the call to
ironlake_edp_panel_ontointel_dp_prepare, except that if if themode_setfails,dp_commitwill 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.
Quoted 2 lines
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.
Quoted 3 lines
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
Quoted 3 lines · Jesse Barnes
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.
Quoted 6 lines
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.
Quoted 6 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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_initall 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.
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 3 lines
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.
Signature · 1 line
[email protected]
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 7/9 drm/i915: Correct eDP panel power sequencing delay computations
In reply to Keith PackardStore 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).
Quoted 1 line
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.
--- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++---------- 2 files changed, 41 insertions(+), 16 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +0 -1
@@ -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 ];
drivers/gpu/drm/i915/intel_dp.c +41 -15
118 lines not on this page · raw message
Signature · 1 line
1.7.6.3
PATCH 8/9 drm/i915: Move eDP panel fixed mode from dev_priv to intel_dp
In reply to Keith PackardThis value doesn't come directly from the VBT, and so is rather specific to the particular DP output.
--- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_dp.c | 35 ++++++++++++++++------------------- 2 files changed, 16 insertions(+), 20 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +0 -1
@@ -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 */
drivers/gpu/drm/i915/intel_dp.c +16 -19
@@ -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 ; }
Signature · 1 line
1.7.6.3
PATCH 9/9 drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously
In reply to Keith PackardThere'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.
--- drivers/gpu/drm/i915/intel_dp.c | 93 +++++++++++++++++++++++++++++++++----- 1 files changed, 80 insertions(+), 13 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +80 -13
@@ -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);
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 9/9] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously
Quoted 8 lines · On Mon, 19 Sep 2011 15:22:03 -0700 Keith Packard <[email protected]> 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 <[email protected]>---
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
Quoted 5 lines · Jesse Barnes
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.
Quoted 2 lines
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.
Signature · 1 line
[email protected]
Re: [Intel-gfx] [PATCH 9/9] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously
Quoted 8 lines · On Tue, 20 Sep 2011 21:51:33 -0700 Keith Packard <[email protected]> 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_preparemay not ever be followed by a call tointel_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.
Quoted 3 lines
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
cover MacBook Air patch sequence (v2)
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 1/21 drm/i915: Enable digital port hotplug on PCH systems
We were relying on the BIOS to set these bits, which doesn't always happen.
--- drivers/gpu/drm/i915/i915_irq.c | 24 ++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_reg.h | 5 ++++- 2 files changed, 28 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +24 -0
@@ -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 ; }
drivers/gpu/drm/i915/i915_reg.h +4 -1
@@ -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)
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 01/21] drm/i915: Enable digital port hotplug on PCH systems
Quoted 4 lines · Keith Packard
We were relying on the BIOS to set these bits, which doesn't always happen.
Signed-off-by Keith Packard <[email protected]>
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 ...
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Jesse Barnes
Reviewed-byRe: [Intel-gfx] [PATCH 01/21] drm/i915: Enable digital port hotplug on PCH systems
Quoted 6 lines · On Fri, 30 Sep 2011 18:17:32 +0200 Daniel Vetter <[email protected]> wrote:
Quoted 4 lines · Keith Packard
We were relying on the BIOS to set these bits, which doesn't always happen.
Signed-off-by Keith Packard <[email protected]>
Quoted 8 lines
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 <[email protected]>
Yep, I like it too:
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 2/21 drm/i915: Shut down PCH interrupts during irq_uninstall
In reply to Keith PackardThis 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.
--- drivers/gpu/drm/i915/i915_irq.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +4 -0
@@ -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)
Signature · 1 line
1.7.6.3
Re: [PATCH 02/21] drm/i915: Shut down PCH interrupts during irq_uninstall
Quoted 14 lines · 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 <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)drivers/gpu/drm/i915/i915_irq.c +0
-0
drivers/gpu/drm/i915/i915_irq.c static void ironlake_irq_uninstall(struct drm_device *dev) · line 2051
Quoted 8 lines
drivers/gpu/drm/i915/i915_irq.c +4
-0@@ -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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 02/21] drm/i915: Shut down PCH interrupts during irq_uninstall
Quoted 2 lines · Daniel Vetter
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...
Signature · 1 line
[email protected]
Daniel Vetter
Reviewed-byRe: [PATCH 02/21] drm/i915: Shut down PCH interrupts during irq_uninstall
Quoted 7 lines · Keith Packard
Quoted 2 lines · Daniel Vetter
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:
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
PATCH 3/21 drm/i915: Remove extra 300ms delay during eDP mode setting
In reply to Keith PackardThere's no reason to enforce a 300ms delay during eDP mode setting.
--- drivers/gpu/drm/i915/intel_dp.c | 7 ------- 1 files changed, 0 insertions(+), 7 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +0 -7
@@ -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);
Signature · 1 line
1.7.6.3
Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting
Quoted 3 lines · Keith Packard
There's no reason to enforce a 300ms delay during eDP mode setting.
Signed-off-by Keith Packard <[email protected]>
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting
Quoted 4 lines · Daniel Vetter
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.
Signature · 1 line
[email protected]
Daniel Vetter
Reviewed-byRe: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting
Quoted 13 lines · Keith Packard
Quoted 4 lines · Daniel Vetter
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.
Quoted 5 lines
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting
Quoted 14 lines · Daniel Vetter
Quoted 13 lines · Keith Packard
Quoted 4 lines · Daniel Vetter
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.
Quoted 8 lines
Quoted 5 lines
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 <[email protected]>
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 03/21] drm/i915: Remove extra 300ms delay during eDP mode setting
Quoted 4 lines · Daniel Vetter
On further strolling through bspec to review later patches I've noticed that
PCH_PP_ON_DELAYSandPCH_PP_OFF_DELAYSseem to have values for poweron->backlighton and backlightoff->paneloff 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.
Signature · 1 line
[email protected]
PATCH 4/21 drm/i915: Only use VBT panel mode on eDP if no EDID is found
In reply to Keith PackardWe'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.
--- drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +10 -10
@@ -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); }
Signature · 1 line
1.7.6.3
Re: [PATCH 04/21] drm/i915: Only use VBT panel mode on eDP if no EDID is found
Quoted 5 lines · 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 <[email protected]>
Ok, this is way over my head, just checked whether the patch does what it claims to - nice exercise in reading dp modeset code ;-)
Signature · 3 lines
Daniel Vetter Mail: [email protected] 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
Quoted 2 lines · Daniel Vetter
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.
Signature · 1 line
[email protected]
Re: [Intel-gfx] [PATCH 04/21] drm/i915: Only use VBT panel mode on eDP if no EDID is found
Quoted 24 lines · On Fri, 30 Sep 2011 10:58:26 -0700 Keith Packard <[email protected]> wrote:
Quoted 2 lines · Daniel Vetter
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.
Quoted 4 lines
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.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 5/21 drm/i915: Check eDP power when doing aux channel communications
In reply to Keith PackardVerify 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.
--- drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +22 -0
@@ -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 ;
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications
Quoted 24 lines · 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 <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-)drivers/gpu/drm/i915/intel_dp.c +5
-0@@ -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;
drivers/gpu/drm/i915/intel_dp.c intel_hrawclk(struct drm_device *dev) · line 296
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+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?
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +3
-0+ } +} + static int intel_dp_aux_ch(struct intel_dp *intel_dp, uint8_t *send, int send_bytes,
drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 319
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +1
-0@@ -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.
Quoted 31 lines
drivers/gpu/drm/i915/intel_dp.c +3
-0@@ -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 ;Signature · 6 lines
1.7.6.3 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [Intel-gfx] [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications
Quoted 1 line · Daniel Vetter
Use
pp_controlinstead 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.
Quoted 2 lines
dp_aux_chdoes 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.
Signature · 1 line
[email protected]
Re: [Intel-gfx] [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications
Quoted 3 lines · Keith Packard
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [Intel-gfx] [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications
In reply to Keith PackardOn Fri, 30 Sep 2011 11:01:06 -0700, Keith Packard <[email protected]> wrote:
Non-text part: multipart/signed
Quoted 11 lines · Daniel Vetter
Quoted 2 lines
Use
pp_controlinstead of re-reading?Could, but you'll note a later patch eliminates both
pp_statusandpp_controllocal variables, so I didn't bother to clean this up when refactoring.Quoted 2 lines
dp_aux_chdoes the low-level io for the below, so either this one or the below three hunks look a bit redundant.
Quoted 7 lines
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
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [Intel-gfx] [PATCH 05/21] drm/i915: Check eDP power when doing aux channel communications
In reply to Keith PackardQuoted 26 lines · On Thu, 29 Sep 2011 18:09:37 -0700 Keith Packard <[email protected]> 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 <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-)drivers/gpu/drm/i915/intel_dp.c +7
-0@@ -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))
drivers/gpu/drm/i915/intel_dp.c intel_hrawclk(struct drm_device *dev) · line 298
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+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.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 6/21 drm/i915: Unlock PCH_PP_CONTROL always
In reply to Keith PackardAvoid any question about locked registers by just writing the unlock pattern with every write to the register.
--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_dp.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/i915_reg.h +1 -0
@@ -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)
drivers/gpu/drm/i915/intel_dp.c +13 -1
@@ -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); }
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always
Quoted 4 lines · 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 <[email protected]>
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [Intel-gfx] [PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always
Quoted 2 lines · Daniel Vetter
grep shows that we also write to
PCH_PP_CONTROLinintel_lvds.cin 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.
Signature · 1 line
[email protected]
Re: [Intel-gfx] [PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always
In reply to Daniel VetterQuoted 2 lines · Daniel Vetter
grep shows that we also write to
PCH_PP_CONTROLinintel_lvds.cin 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.
Signature · 1 line
[email protected]
Daniel Vetter
Reviewed-byRe: [Intel-gfx] [PATCH 06/21] drm/i915: Unlock PCH_PP_CONTROL always
Quoted 11 lines · Keith Packard
Quoted 2 lines · Daniel Vetter
grep shows that we also write to
PCH_PP_CONTROLinintel_lvds.cin 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.
Quoted 7 lines
The LVDS code does disable write protect in the
_initfunction, 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.
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
PATCH 7/21 drm/i915: Check for eDP inside intel_edp_panel_vdd_on/off
In reply to Keith PackardCleans up code dealing with eDP VDD a bit. Remove redundant checks in callers
--- drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +10 -10
@@ -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))
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 07/21] drm/i915: Check for eDP inside intel_edp_panel_vdd_on/off
Quoted 33 lines · Keith Packard
Cleans up code dealing with eDP VDD a bit. Remove redundant checks in callers
Signed-off-by Keith Packard <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)drivers/gpu/drm/i915/intel_dp.c +4
-0@@ -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;
drivers/gpu/drm/i915/intel_dp.c static void intel_dp_commit(struct drm_encoder *encoder) · line 1046
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +2
-3-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
Quoted 32 lines
drivers/gpu/drm/i915/intel_dp.c +4
-7-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))Signature · 6 lines
1.7.6.3 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Mail: [email protected] 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
Quoted 2 lines · Daniel Vetter
Why not also move the
id_edpcheck 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.
Signature · 1 line
[email protected]
PATCH 8/21 drm/i915: Turn force VDD back off when panel running in intel_dp_dpms
In reply to Keith PackardThe 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.
--- drivers/gpu/drm/i915/intel_dp.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +2 -1
@@ -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); }
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 08/21] drm/i915: Turn force VDD back off when panel running in intel_dp_dpms
Quoted 4 lines · 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 <[email protected]>
Nice catch of in inbalanced on/off.
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
PATCH 9/21 drm/i915: Delay DP i2c initialization until panel power timings are computed
In reply to Keith PackardOn eDP, DDC requires panel power, but turning that on uses the panel power sequencing timing values fetch from the DPCD data.
--- drivers/gpu/drm/i915/intel_dp.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +2 -2
@@ -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)) {
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 09/21] drm/i915: Delay DP i2c initialization until panel power timings are computed
Quoted 4 lines · 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 <[email protected]>
Can't really check more than what the patch does what it claims to do, so a weak:
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
PATCH 10/21 drm/i915: Wrap DP EDID fetch functions to enable eDP panel power
In reply to Keith PackardTalking 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.
--- drivers/gpu/drm/i915/intel_dp.c | 31 ++++++++++++++++++++++++++++--- 1 files changed, 28 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +28 -3
@@ -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);
Signature · 1 line
1.7.6.3
Re: [PATCH 10/21] drm/i915: Wrap DP EDID fetch functions to enable eDP panel power
Quoted 5 lines · 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 <[email protected]>
Signature · 3 lines
Daniel Vetter Mail: [email protected] 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
In reply to Keith PackardQuoted 4 lines · On Thu, 29 Sep 2011 18:09:42 -0700 Keith Packard <[email protected]> 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.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 11/21 drm/i915: Enable eDP panel power during I2C initialization sequence
In reply to Keith PackardThe DP i2c initialization code does a couple of i2c transactions, which means that an eDP panel must be powered up.
--- drivers/gpu/drm/i915/intel_dp.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +9 -1
@@ -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
Signature · 1 line
1.7.6.3
Re: [PATCH 11/21] drm/i915: Enable eDP panel power during I2C initialization sequence
Quoted 4 lines · 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 <[email protected]>
Ah, here's the code I've been looking for when reviewing patch 9.
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
PATCH 12/21 drm/i915: Ensure eDP powered up during DP_SET_POWER operation in dp_prepare
In reply to Keith PackardAny 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.
--- drivers/gpu/drm/i915/intel_dp.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +2 -0
@@ -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);
Signature · 1 line
1.7.6.3
Re: [PATCH 12/21] drm/i915: Ensure eDP powered up during DP_SET_POWER operation in dp_prepare
Quoted 5 lines · Keith Packard
Any call to
intel_dp_sink_dpmsmust ensure that the panel has power so that theDP_SET_POWERoperation will be correctly received. The only one missing this was inintel_dp_prepare.Signed-off-by Keith Packard <[email protected]>
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:
Signature · 3 lines
Daniel Vetter Mail: [email protected] 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
Quoted 2 lines · Daniel Vetter
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.
Signature · 1 line
[email protected]
PATCH 13/21 drm/i915: Place long delays after each eDP VDD operation
In reply to Keith PackardThis ensures that the panel power sequencing is complete before attempting to communicate over the aux channel.
--- drivers/gpu/drm/i915/intel_dp.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +11 -1
@@ -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 */
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 13/21] drm/i915: Place long delays after each eDP VDD operation
Quoted 4 lines · Keith Packard
This ensures that the panel power sequencing is complete before attempting to communicate over the aux channel.
Signed-off-by Keith Packard <[email protected]>
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [Intel-gfx] [PATCH 13/21] drm/i915: Place long delays after each eDP VDD operation
Quoted 3 lines · Daniel Vetter
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.
Signature · 1 line
[email protected]
PATCH 14/21 drm/i915: Correct eDP panel power sequencing delay computations
In reply to Keith PackardStore 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).
Quoted 1 line
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.
--- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++++---------- 2 files changed, 43 insertions(+), 17 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +0 -1
@@ -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 ];
drivers/gpu/drm/i915/intel_dp.c +43 -16
127 lines not on this page · raw message
Signature · 1 line
1.7.6.3
Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations
Quoted 33 lines · 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_DELAYSPower_Down_delayvalue, 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 thePP_ON_DELAYSregister, 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.
Quoted 6 lines
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 <[email protected]>
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.
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations
Quoted 4 lines · Daniel Vetter
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.
Signature · 1 line
[email protected]
Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations
In reply to Daniel VetterQuoted 4 lines · Daniel Vetter
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 + 1The 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.
Signature · 1 line
[email protected]
Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations
Quoted 51 lines · Keith Packard
Quoted 4 lines · Daniel Vetter
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_seqtable into the panel delay register settings.
intel_bios.hsays: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 + 1The BSPEC/BIOS names don't agree with the eDP spec. Here's my plan:
Data Code eDP spec
intel_dpname 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 onIt'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.
Quoted 8 lines
As far as the
edp_power_seqregister 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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations
Quoted 4 lines · Daniel Vetter
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_foruntil 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...
Signature · 1 line
[email protected]
Re: [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delay computations
Quoted 10 lines · Keith Packard
Quoted 4 lines · Daniel Vetter
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_foruntil 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
Signature · 2 lines
Daniel Vetter [email protected] - +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
In reply to Keith PackardThis value doesn't come directly from the VBT, and so is rather specific to the particular DP output.
--- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_dp.c | 35 ++++++++++++++++------------------- 2 files changed, 16 insertions(+), 20 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +0 -1
@@ -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 */
drivers/gpu/drm/i915/intel_dp.c +16 -19
@@ -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 ; }
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 15/21] drm/i915: Move eDP panel fixed mode from dev_priv to intel_dp
Quoted 4 lines · 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 <[email protected]>
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
PATCH 16/21 drm/i915: edp_panel_on does not need to return a bool
In reply to Keith PackardThe return value was unused, so just stop doing that.
--- drivers/gpu/drm/i915/intel_dp.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +2 -4
@@ -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)
Signature · 1 line
1.7.6.3
Re: [PATCH 16/21] drm/i915: edp_panel_on does not need to return a bool
Quoted 3 lines · Keith Packard
The return value was unused, so just stop doing that.
Signed-off-by Keith Packard <[email protected]>
Signature · 3 lines
Daniel Vetter Mail: [email protected] 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
In reply to Keith PackardQuoted 12 lines · On Thu, 29 Sep 2011 18:09:48 -0700 Keith Packard <[email protected]> wrote:
The return value was unused, so just stop doing that.
Signed-off-by Keith Packard <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-0
drivers/gpu/drm/i915/intel_dp.c static void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp) · line 890
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c +1
-1@@ -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?
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 17/21 drm/i915: Create helper functions to determine eDP power state
In reply to Keith PackardWe need to check eDP VDD force and panel on in several places, so create some simple helper functions to avoid duplicating code.
--- drivers/gpu/drm/i915/intel_dp.c | 39 ++++++++++++++++++++++++++------------- 1 files changed, 26 insertions(+), 13 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +26 -13
@@ -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 );
Signature · 1 line
1.7.6.3
Re: [Intel-gfx] [PATCH 17/21] drm/i915: Create helper functions to determine eDP power state
Quoted 4 lines · 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 <[email protected]>
Ah, here's the code that kills my "... instead of re-reading PP_CONTROL" comment ;-)
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
PATCH 18/21 drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously
In reply to Keith PackardThere'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.
--- drivers/gpu/drm/i915/intel_dp.c | 121 +++++++++++++++++++++++++++++++-------- 1 files changed, 97 insertions(+), 24 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +97 -24
@@ -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 *adaironlake_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 =
Signature · 1 line
1.7.6.3
Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously
drivers/gpu/drm/i915/intel_dp.c static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp) · line 938
Quoted 9 lines · Keith Packard
drivers/gpu/drm/i915/intel_dp.c +9
-0+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
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously
Quoted 1 line · Chris Wilson
I think you want the
dev->mode_config.mutexhere.
oops. good catch.
Signature · 1 line
[email protected]
Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously
In reply to Keith PackardQuoted 53 lines · 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 <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 121 +++++++++++++++++++++++++++++++-------- 1 files changed, 97 insertions(+), 24 deletions(-)drivers/gpu/drm/i915/intel_dp.c +11
-2@@ -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);
drivers/gpu/drm/i915/intel_dp.c intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode, · line 869
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+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?
Quoted 53 lines
drivers/gpu/drm/i915/intel_dp.c +28
-2+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; + } +} +
drivers/gpu/drm/i915/intel_dp.c static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp) · line 939
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+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 ;-)
Quoted 79 lines
drivers/gpu/drm/i915/intel_dp.c +23
-14+ +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);
drivers/gpu/drm/i915/intel_dp.c intel_dp_dpms(struct drm_encoder *encoder, int mode) · line 1181
Quoted 9 lines
drivers/gpu/drm/i915/intel_dp.c +2
-2@@ -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?
Quoted 65 lines
drivers/gpu/drm/i915/intel_dp.c +13
-4if (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 *adaironlake_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 =Signature · 6 lines
1.7.6.3 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously
Quoted 2 lines · Daniel Vetter
A
cancel_workmight 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.
Quoted 4 lines
Like Chris already mentioned, s/struct_mutex/mode_config.mutex/ Maybe also move the
intel_dp->want_panel_vddcheck in here - we need it only here to avoid a race withvdd_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?
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -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);
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_dpms(struct drm_encoder *encoder, int mode) · line 1181
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c +2
-2-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_offis 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.
Signature · 1 line
[email protected]
Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously
Quoted 8 lines · Keith Packard
Quoted 2 lines · Daniel Vetter
A
cancel_workmight 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.
Quoted 2 lines
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.
Quoted 7 lines
Quoted 4 lines
Like Chris already mentioned, s/struct_mutex/mode_config.mutex/ Maybe also move the
intel_dp->want_panel_vddcheck in here - we need it only here to avoid a race withvdd_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 ;-)
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -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);Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_dpms(struct drm_encoder *encoder, int mode) · line 1181
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c +2
-2-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_offis sync while the other isn't?
Quoted 4 lines
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
PATCH 19/21 drm/i915: Asynchronous eDP panel power off
In reply to Keith PackardUsing the same basic plan as the VDD force delayed power off, make turning the panel power off asynchronous.
--- drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++++++---------- 1 files changed, 53 insertions(+), 18 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +53 -18
@@ -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 );
Signature · 1 line
1.7.6.3
Re: [PATCH 19/21] drm/i915: Asynchronous eDP panel power off
Quoted 24 lines · 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 <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++++++---------- 1 files changed, 53 insertions(+), 18 deletions(-)drivers/gpu/drm/i915/intel_dp.c +2
-1@@ -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; };
drivers/gpu/drm/i915/intel_dp.c static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp) · line 912
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +3
-1@@ -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/.
Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c +1
-1{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),
drivers/gpu/drm/i915/intel_dp.c static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp) · line 943
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +3
-2- 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 ;-)
Quoted 140 lines
drivers/gpu/drm/i915/intel_dp.c +44
-13@@ -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 );Signature · 6 lines
1.7.6.3 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 19/21] drm/i915: Asynchronous eDP panel power off
Quoted 2 lines · Daniel Vetter
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.
Quoted 3 lines
Same comment as in the previous patch: I think the
intel_dp->want_panel_powercheck 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.
Signature · 1 line
[email protected]
Re: [PATCH 19/21] drm/i915: Asynchronous eDP panel power off
In reply to Keith PackardQuoted 2 lines
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
Quoted 5 lines · Dave Airlie
Quoted 2 lines
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.
Signature · 1 line
[email protected]
PATCH 20/21 drm/i915: Restrict ILK-specific eDP power hack to ILK
In reply to Keith PackardThis eliminates a fairly long delay when power sequencing newer hardware
--- drivers/gpu/drm/i915/intel_dp.c | 51 +++++++++++++++++++++++---------------- 1 files changed, 30 insertions(+), 21 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +30 -21
@@ -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)
Signature · 1 line
1.7.6.3
Re: [PATCH 20/21] drm/i915: Restrict ILK-specific eDP power hack to ILK
Quoted 4 lines · Keith Packard
This eliminates a fairly long delay when power sequencing newer hardware
Signed-off-by Keith Packard <[email protected]>
Signature · 3 lines
Daniel Vetter Mail: [email protected] 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
In reply to Keith PackardIf the panel is powered up, there's no need to delay for the 'off' interval when turning the panel on.
--- drivers/gpu/drm/i915/intel_dp.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +9 -0
@@ -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" );
Signature · 1 line
1.7.6.3
Re: [PATCH 21/21] drm/i915: No need to wait for eDP power off delay if panel is on
Quoted 4 lines · 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 <[email protected]>
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 00/24] MacBook Air patch sequence (v2)
In reply to Keith PackardQuoted 28 lines · 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_CONTROLalways [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 duringDP_SET_POWER[PATCH 13/21] drm/i915: Place long delays after each eDP VDD [PATCH 14/21] drm/i915: Correct eDP panel power sequencing delayThe diffstat for that sequence is:
Quoted 8 lines
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)
Quoted 1 line · Greg KH
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?
Quoted 2 lines
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.
Quoted 2 lines
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.
Signature · 1 line
[email protected]
Re: [PATCH 00/24] MacBook Air patch sequence (v2)
Quoted 10 lines · Keith Packard
Quoted 1 line · Greg KH
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?
Quoted 9 lines
Quoted 2 lines
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.
Quoted 5 lines
Quoted 2 lines
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?
Quoted 2 lines
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)
Quoted 3 lines · Greg KH
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.
Quoted 1 line
Would these also work on 3.0?
They won't apply cleanly, but yes, they should be useful there.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Signature · 1 line
[email protected]
Re: [PATCH 00/24] MacBook Air patch sequence (v2)
In reply to Greg KHQuoted 5 lines · Greg KH
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)
Quoted 2 lines · Ted Ts'o
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.
Signature · 1 line
[email protected]
Re: [Intel-gfx] [PATCH 00/24] MacBook Air patch sequence (v2)
Quoted 7 lines · On Fri, 30 Sep 2011 11:17:38 -0700 Keith Packard <[email protected]> wrote:
Quoted 2 lines · Ted Ts'o
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. :)
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Re: [PATCH 00/24] MacBook Air patch sequence (v2)
In reply to Keith PackardQuoted 10 lines · 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.
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.
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre