Here's a fun sequence:
$ xrandr --output LVDS1 --auto --crtc 1 $ sudo chvt 1
(then switch back to X)
The LVDS goes black. Why? The LVDS is *disabled*. Turns out that the cause was that after the mode was all nicely set, intel_disable_pch_ports went and turned it back off.
Well, a couple of weeks ago, we found that intel_disable_pch_ports was doing the same thing to DP ports, and the cause was that the way to tell which port a DP output was running from is different on CPT that on every other chip. We fixed that, but didn't (foolishly) review the non-DP code.
Oddly, the non-DP code had essentially the same bug:
[PATCH 3/4] drm/i915: Fix PCH port pipe select in CPT disable paths
Also in this sequence is some code that validates the LVDS panel power sequencing which discovered that the LVDS panel wasn't being disabled correctly because the LVDS registers were locked in the DPMS function. Instead of trying to get the LVDS locking right, I've just unlocked the registers in the lvds initialization code and left them unlocked after that:
[PATCH 1/4] drm/i915: Wait for LVDS panel power sequence [PATCH 2/4] drm/i915: Leave LVDS registers unlocked
Finally, I removed an unused argument to dp_pipe_enable while I was poking around in that code:
[PATCH 4/4] drm/i915: Remove unused 'reg' argument to dp_pipe_enable
-- keith.packard@intel.com
During mode setting, check to make sure the panel power sequencing has completed before doing further operations on the device. This uncovered errors with DPMS not turning the device off as it was left locked.
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_lvds.c | 13 +++++++++++-- 1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 2e8ddfc..6985e42 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -72,14 +72,16 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds) { struct drm_device *dev = intel_lvds->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - u32 ctl_reg, lvds_reg; + u32 ctl_reg, lvds_reg, stat_reg;
if (HAS_PCH_SPLIT(dev)) { ctl_reg = PCH_PP_CONTROL; lvds_reg = PCH_LVDS; + stat_reg = PCH_PP_STATUS; } else { ctl_reg = PP_CONTROL; lvds_reg = LVDS; + stat_reg = PP_STATUS; }
I915_WRITE(lvds_reg, I915_READ(lvds_reg) | LVDS_PORT_EN); @@ -105,6 +107,8 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds)
I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON); POSTING_READ(lvds_reg); + if (wait_for((I915_READ(stat_reg) & PP_ON) != 0, 1000)) + DRM_ERROR("timed out waiting for panel to power on\n");
intel_panel_enable_backlight(dev); } @@ -113,19 +117,24 @@ static void intel_lvds_disable(struct intel_lvds *intel_lvds) { struct drm_device *dev = intel_lvds->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - u32 ctl_reg, lvds_reg; + u32 ctl_reg, lvds_reg, stat_reg;
if (HAS_PCH_SPLIT(dev)) { ctl_reg = PCH_PP_CONTROL; lvds_reg = PCH_LVDS; + stat_reg = PCH_PP_STATUS; } else { ctl_reg = PP_CONTROL; lvds_reg = LVDS; + stat_reg = PP_STATUS; }
intel_panel_disable_backlight(dev);
I915_WRITE(ctl_reg, I915_READ(ctl_reg) & ~POWER_TARGET_ON); + if (wait_for((I915_READ(stat_reg) & PP_ON) == 0, 1000)) + DRM_ERROR("timed out waiting for panel to power off status 0x%08x control 0x%08x\n", + I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
if (intel_lvds->pfit_control) { if (wait_for((I915_READ(PP_STATUS) & PP_ON) == 0, 1000))
On Sat, 6 Aug 2011 10:54:05 -0700 Keith Packard keithp@keithp.com wrote:
During mode setting, check to make sure the panel power sequencing has completed before doing further operations on the device. This uncovered errors with DPMS not turning the device off as it was left locked.
Signed-off-by: Keith Packard keithp@keithp.com
drivers/gpu/drm/i915/intel_lvds.c | 13 +++++++++++-- 1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 2e8ddfc..6985e42 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -72,14 +72,16 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds) { struct drm_device *dev = intel_lvds->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private;
- u32 ctl_reg, lvds_reg;
u32 ctl_reg, lvds_reg, stat_reg;
if (HAS_PCH_SPLIT(dev)) { ctl_reg = PCH_PP_CONTROL; lvds_reg = PCH_LVDS;
stat_reg = PCH_PP_STATUS;} else { ctl_reg = PP_CONTROL; lvds_reg = LVDS;
stat_reg = PP_STATUS;}
I915_WRITE(lvds_reg, I915_READ(lvds_reg) | LVDS_PORT_EN);
@@ -105,6 +107,8 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds)
I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON); POSTING_READ(lvds_reg);
- if (wait_for((I915_READ(stat_reg) & PP_ON) != 0, 1000))
DRM_ERROR("timed out waiting for panel to power on\n");
Looks like maybe we want a small function for this...
I915_WRITE(ctl_reg, I915_READ(ctl_reg) & ~POWER_TARGET_ON);
- if (wait_for((I915_READ(stat_reg) & PP_ON) == 0, 1000))
DRM_ERROR("timed out waiting for panel to power off status 0x%08x control 0x%08x\n",I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
...to catch places like this where the wrong register gets used. :)
On Mon, 8 Aug 2011 09:27:19 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
...to catch places like this where the wrong register gets used. :)
Ouch! There are only two places we *should* have these loops, one when turning it off, another when turning it on. There's a couple of loops which just need to be removed.
Here's a replacement patch:
From 436f2b19cf17c43e4d5ad55b47aeb3660c2af9b9 Mon Sep 17 00:00:00 2001 From: Keith Packard keithp@keithp.com Date: Sat, 6 Aug 2011 10:30:45 -0700 Subject: [PATCH] drm/i915: Wait for LVDS panel power sequence
During mode setting, check to make sure the panel power sequencing has completed before doing further operations on the device. This uncovered errors with DPMS not turning the device off as it was left locked.
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_lvds.c | 26 ++++++++++++++------------ 1 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 2e8ddfc..6318828 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -72,14 +72,16 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds) { struct drm_device *dev = intel_lvds->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - u32 ctl_reg, lvds_reg; + u32 ctl_reg, lvds_reg, stat_reg;
if (HAS_PCH_SPLIT(dev)) { ctl_reg = PCH_PP_CONTROL; lvds_reg = PCH_LVDS; + stat_reg = PCH_PP_STATUS; } else { ctl_reg = PP_CONTROL; lvds_reg = LVDS; + stat_reg = PP_STATUS; }
I915_WRITE(lvds_reg, I915_READ(lvds_reg) | LVDS_PORT_EN); @@ -94,17 +96,16 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds) DRM_DEBUG_KMS("applying panel-fitter: %x, %x\n", intel_lvds->pfit_control, intel_lvds->pfit_pgm_ratios); - if (wait_for((I915_READ(PP_STATUS) & PP_ON) == 0, 1000)) { - DRM_ERROR("timed out waiting for panel to power off\n"); - } else { - I915_WRITE(PFIT_PGM_RATIOS, intel_lvds->pfit_pgm_ratios); - I915_WRITE(PFIT_CONTROL, intel_lvds->pfit_control); - intel_lvds->pfit_dirty = false; - } + + I915_WRITE(PFIT_PGM_RATIOS, intel_lvds->pfit_pgm_ratios); + I915_WRITE(PFIT_CONTROL, intel_lvds->pfit_control); + intel_lvds->pfit_dirty = false; }
I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON); POSTING_READ(lvds_reg); + if (wait_for((I915_READ(stat_reg) & PP_ON) != 0, 1000)) + DRM_ERROR("timed out waiting for panel to power on\n");
intel_panel_enable_backlight(dev); } @@ -113,24 +114,25 @@ static void intel_lvds_disable(struct intel_lvds *intel_lvds) { struct drm_device *dev = intel_lvds->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - u32 ctl_reg, lvds_reg; + u32 ctl_reg, lvds_reg, stat_reg;
if (HAS_PCH_SPLIT(dev)) { ctl_reg = PCH_PP_CONTROL; lvds_reg = PCH_LVDS; + stat_reg = PCH_PP_STATUS; } else { ctl_reg = PP_CONTROL; lvds_reg = LVDS; + stat_reg = PP_STATUS; }
intel_panel_disable_backlight(dev);
I915_WRITE(ctl_reg, I915_READ(ctl_reg) & ~POWER_TARGET_ON); + if (wait_for((I915_READ(stat_reg) & PP_ON) == 0, 1000)) + DRM_ERROR("timed out waiting for panel to power off\n");
if (intel_lvds->pfit_control) { - if (wait_for((I915_READ(PP_STATUS) & PP_ON) == 0, 1000)) - DRM_ERROR("timed out waiting for panel to power off\n"); - I915_WRITE(PFIT_CONTROL, 0); intel_lvds->pfit_dirty = true; }
On Mon, 08 Aug 2011 11:40:06 -0700 Keith Packard keithp@keithp.com wrote:
On Mon, 8 Aug 2011 09:27:19 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
...to catch places like this where the wrong register gets used. :)
Ouch! There are only two places we *should* have these loops, one when turning it off, another when turning it on. There's a couple of loops which just need to be removed.
Yeah looks better.
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
There's no reason to relock them; it just makes operations more complex. This fixes DPMS where the panel registers were locked making the disable not work.
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_lvds.c | 46 +++++++++++++++--------------------- 1 files changed, 19 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 6985e42..cc85618 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -414,46 +414,32 @@ static void intel_lvds_prepare(struct drm_encoder *encoder) /* We try to do the minimum that is necessary in order to unlock * the registers for mode setting. * - * On Ironlake, this is quite simple as we just set the unlock key - * and ignore all subtleties. (This may cause some issues...) + * On Ironlake, this is quite simple as we just set the unlock + * key in the driver init code and ignore all subtleties. + * (This may cause some issues...) * * Prior to Ironlake, we must disable the pipe if we want to adjust * the panel fitter. However at all other times we can just reset * the registers regardless. */
- if (HAS_PCH_SPLIT(dev)) { - I915_WRITE(PCH_PP_CONTROL, - I915_READ(PCH_PP_CONTROL) | PANEL_UNLOCK_REGS); - } else if (intel_lvds->pfit_dirty) { - I915_WRITE(PP_CONTROL, - (I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS) - & ~POWER_TARGET_ON); - } else { - I915_WRITE(PP_CONTROL, - I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS); + if (!HAS_PCH_SPLIT(dev)) { + if (intel_lvds->pfit_dirty) { + I915_WRITE(PP_CONTROL, + (I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS) + & ~POWER_TARGET_ON); + } else { + I915_WRITE(PP_CONTROL, + I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS); + } } + intel_lvds_disable(intel_lvds); }
static void intel_lvds_commit(struct drm_encoder *encoder) { - struct drm_device *dev = encoder->dev; - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_lvds *intel_lvds = to_intel_lvds(encoder);
- /* Undo any unlocking done in prepare to prevent accidental - * adjustment of the registers. - */ - if (HAS_PCH_SPLIT(dev)) { - u32 val = I915_READ(PCH_PP_CONTROL); - if ((val & PANEL_UNLOCK_REGS) == PANEL_UNLOCK_REGS) - I915_WRITE(PCH_PP_CONTROL, val & 0x3); - } else { - u32 val = I915_READ(PP_CONTROL); - if ((val & PANEL_UNLOCK_REGS) == PANEL_UNLOCK_REGS) - I915_WRITE(PP_CONTROL, val & 0x3); - } - /* Always do a full power on as we do not know what state * we were left in. */ @@ -1049,6 +1035,12 @@ out: pwm = I915_READ(BLC_PWM_PCH_CTL1); pwm |= PWM_PCH_ENABLE; I915_WRITE(BLC_PWM_PCH_CTL1, pwm); + /* + * Unlock registers and just + * leave them unlocked + */ + I915_WRITE(PCH_PP_CONTROL, + I915_READ(PCH_PP_CONTROL) | PANEL_UNLOCK_REGS); } dev_priv->lid_notifier.notifier_call = intel_lid_notify; if (acpi_lid_notifier_register(&dev_priv->lid_notifier)) {
On Sat, 6 Aug 2011 10:54:06 -0700 Keith Packard keithp@keithp.com wrote:
There's no reason to relock them; it just makes operations more complex. This fixes DPMS where the panel registers were locked making the disable not work.
Signed-off-by: Keith Packard keithp@keithp.com
Yep, looks fine. The only think we might want to sprinkle about are checks for panel off so we can avoid visible corruption if we whack timing or fb stuff while the panel is on.
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
On Mon, 8 Aug 2011 09:30:10 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Yep, looks fine. The only think we might want to sprinkle about are checks for panel off so we can avoid visible corruption if we whack timing or fb stuff while the panel is on.
Yeah, could do. Would be nice to somehow get the LVDS code ripped out of the middle of intel_display.c; everything in intel_lvds.c is nicely bracketed by prepare/commit, which turn the panel off and back on.
On Mon, 8 Aug 2011 09:30:10 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Yep, looks fine. The only think we might want to sprinkle about are checks for panel off so we can avoid visible corruption if we whack timing or fb stuff while the panel is on.
So, I'd like to know if we could unlock the panel registers on pre-PCH hardware as well at init time; that way I could remove the unlock code From intel_lvds_prepare too. Should it be possible to set the PANEL_UNLOCK_REGS bits for pre-PCH hardware without having the target off?
On Mon, 08 Aug 2011 11:42:56 -0700 Keith Packard keithp@keithp.com wrote:
On Mon, 8 Aug 2011 09:30:10 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Yep, looks fine. The only think we might want to sprinkle about are checks for panel off so we can avoid visible corruption if we whack timing or fb stuff while the panel is on.
So, I'd like to know if we could unlock the panel registers on pre-PCH hardware as well at init time; that way I could remove the unlock code From intel_lvds_prepare too. Should it be possible to set the PANEL_UNLOCK_REGS bits for pre-PCH hardware without having the target off?
Yep, it's safe and possible to do on pre-PCH as well. For panel fitting we do need to do an actual power cycle when going from non-native back to native iirc, but we can still leave them unlocked so we don't have to worry about the lock/unlock sequence everywhere.
On Mon, 8 Aug 2011 11:49:54 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Yep, it's safe and possible to do on pre-PCH as well. For panel fitting we do need to do an actual power cycle when going from non-native back to native iirc, but we can still leave them unlocked so we don't have to worry about the lock/unlock sequence everywhere.
Hidden in the unlock patch was a call to intel_lvds_disable from intel_lvds_prepare -- that *always* turns off the LVDS for mode setting. Do we care enough about LVDS mode setting performance that we should try leave the optimization in place that doesn't turn off the backlight when switching between modes?
Here's a replacement which unlocks the regs at init time for all generations. This also includes the unconditional call to intel_lvds_disable in the _prepare function. I could back that out if you like.
From c0f946bf41e49d9a10bcc0e4ae18a481bb8cdab3 Mon Sep 17 00:00:00 2001 From: Keith Packard keithp@keithp.com Date: Sat, 6 Aug 2011 10:33:12 -0700 Subject: [PATCH 2/5] drm/i915: Leave LVDS registers unlocked
There's no reason to relock them; it just makes operations more complex. This fixes DPMS where the panel registers were locked making the disable not work.
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_lvds.c | 53 ++++++++++-------------------------- 1 files changed, 15 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 6318828..03a9e6d 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -400,53 +400,17 @@ out:
static void intel_lvds_prepare(struct drm_encoder *encoder) { - struct drm_device *dev = encoder->dev; - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_lvds *intel_lvds = to_intel_lvds(encoder);
- /* We try to do the minimum that is necessary in order to unlock - * the registers for mode setting. - * - * On Ironlake, this is quite simple as we just set the unlock key - * and ignore all subtleties. (This may cause some issues...) - * - * Prior to Ironlake, we must disable the pipe if we want to adjust - * the panel fitter. However at all other times we can just reset - * the registers regardless. + /* Safest to just always power off for mode setting. */ - - if (HAS_PCH_SPLIT(dev)) { - I915_WRITE(PCH_PP_CONTROL, - I915_READ(PCH_PP_CONTROL) | PANEL_UNLOCK_REGS); - } else if (intel_lvds->pfit_dirty) { - I915_WRITE(PP_CONTROL, - (I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS) - & ~POWER_TARGET_ON); - } else { - I915_WRITE(PP_CONTROL, - I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS); - } + intel_lvds_disable(intel_lvds); }
static void intel_lvds_commit(struct drm_encoder *encoder) { - struct drm_device *dev = encoder->dev; - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_lvds *intel_lvds = to_intel_lvds(encoder);
- /* Undo any unlocking done in prepare to prevent accidental - * adjustment of the registers. - */ - if (HAS_PCH_SPLIT(dev)) { - u32 val = I915_READ(PCH_PP_CONTROL); - if ((val & PANEL_UNLOCK_REGS) == PANEL_UNLOCK_REGS) - I915_WRITE(PCH_PP_CONTROL, val & 0x3); - } else { - u32 val = I915_READ(PP_CONTROL); - if ((val & PANEL_UNLOCK_REGS) == PANEL_UNLOCK_REGS) - I915_WRITE(PP_CONTROL, val & 0x3); - } - /* Always do a full power on as we do not know what state * we were left in. */ @@ -1042,6 +1006,19 @@ out: pwm = I915_READ(BLC_PWM_PCH_CTL1); pwm |= PWM_PCH_ENABLE; I915_WRITE(BLC_PWM_PCH_CTL1, pwm); + /* + * Unlock registers and just + * leave them unlocked + */ + I915_WRITE(PCH_PP_CONTROL, + I915_READ(PCH_PP_CONTROL) | PANEL_UNLOCK_REGS); + } else { + /* + * Unlock registers and just + * leave them unlocked + */ + I915_WRITE(PP_CONTROL, + I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS); } dev_priv->lid_notifier.notifier_call = intel_lid_notify; if (acpi_lid_notifier_register(&dev_priv->lid_notifier)) {
On Mon, 08 Aug 2011 12:53:31 -0700 Keith Packard keithp@keithp.com wrote:
On Mon, 8 Aug 2011 11:49:54 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Yep, it's safe and possible to do on pre-PCH as well. For panel fitting we do need to do an actual power cycle when going from non-native back to native iirc, but we can still leave them unlocked so we don't have to worry about the lock/unlock sequence everywhere.
Hidden in the unlock patch was a call to intel_lvds_disable from intel_lvds_prepare -- that *always* turns off the LVDS for mode setting. Do we care enough about LVDS mode setting performance that we should try leave the optimization in place that doesn't turn off the backlight when switching between modes?
We hate flicker right? But generally yes it's safer to just turn it off all the time.
On Mon, 8 Aug 2011 13:01:28 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
On Mon, 08 Aug 2011 12:53:31 -0700 Keith Packard keithp@keithp.com wrote:
On Mon, 8 Aug 2011 11:49:54 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Yep, it's safe and possible to do on pre-PCH as well. For panel fitting we do need to do an actual power cycle when going from non-native back to native iirc, but we can still leave them unlocked so we don't have to worry about the lock/unlock sequence everywhere.
Hidden in the unlock patch was a call to intel_lvds_disable from intel_lvds_prepare -- that *always* turns off the LVDS for mode setting. Do we care enough about LVDS mode setting performance that we should try leave the optimization in place that doesn't turn off the backlight when switching between modes?
We hate flicker right? But generally yes it's safer to just turn it off all the time.
I'll leave the optimization in place then; it's been there for a while so at least it shouldn't cause any regressions.
How about this? Has the advantage of not lying in the commit message anymore.
From 092719152aa5a235d6678798a34dc784d5cec2ad Mon Sep 17 00:00:00 2001 From: Keith Packard keithp@keithp.com Date: Sat, 6 Aug 2011 10:33:12 -0700 Subject: [PATCH 2/5] drm/i915: Leave LVDS registers unlocked
There's no reason to relock them; it just makes operations more complex. This fixes DPMS where the panel registers were locked making the disable not work.
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_lvds.c | 51 +++++++++++------------------------- 1 files changed, 16 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 6318828..8b521a2 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -400,53 +400,21 @@ out:
static void intel_lvds_prepare(struct drm_encoder *encoder) { - struct drm_device *dev = encoder->dev; - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_lvds *intel_lvds = to_intel_lvds(encoder);
- /* We try to do the minimum that is necessary in order to unlock - * the registers for mode setting. - * - * On Ironlake, this is quite simple as we just set the unlock key - * and ignore all subtleties. (This may cause some issues...) - * + /* * Prior to Ironlake, we must disable the pipe if we want to adjust * the panel fitter. However at all other times we can just reset * the registers regardless. */ - - if (HAS_PCH_SPLIT(dev)) { - I915_WRITE(PCH_PP_CONTROL, - I915_READ(PCH_PP_CONTROL) | PANEL_UNLOCK_REGS); - } else if (intel_lvds->pfit_dirty) { - I915_WRITE(PP_CONTROL, - (I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS) - & ~POWER_TARGET_ON); - } else { - I915_WRITE(PP_CONTROL, - I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS); - } + if (!HAS_PCH_SPLIT(encoder->dev) && intel_lvds->pfit_dirty) + intel_lvds_disable(intel_lvds); }
static void intel_lvds_commit(struct drm_encoder *encoder) { - struct drm_device *dev = encoder->dev; - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_lvds *intel_lvds = to_intel_lvds(encoder);
- /* Undo any unlocking done in prepare to prevent accidental - * adjustment of the registers. - */ - if (HAS_PCH_SPLIT(dev)) { - u32 val = I915_READ(PCH_PP_CONTROL); - if ((val & PANEL_UNLOCK_REGS) == PANEL_UNLOCK_REGS) - I915_WRITE(PCH_PP_CONTROL, val & 0x3); - } else { - u32 val = I915_READ(PP_CONTROL); - if ((val & PANEL_UNLOCK_REGS) == PANEL_UNLOCK_REGS) - I915_WRITE(PP_CONTROL, val & 0x3); - } - /* Always do a full power on as we do not know what state * we were left in. */ @@ -1042,6 +1010,19 @@ out: pwm = I915_READ(BLC_PWM_PCH_CTL1); pwm |= PWM_PCH_ENABLE; I915_WRITE(BLC_PWM_PCH_CTL1, pwm); + /* + * Unlock registers and just + * leave them unlocked + */ + I915_WRITE(PCH_PP_CONTROL, + I915_READ(PCH_PP_CONTROL) | PANEL_UNLOCK_REGS); + } else { + /* + * Unlock registers and just + * leave them unlocked + */ + I915_WRITE(PP_CONTROL, + I915_READ(PP_CONTROL) | PANEL_UNLOCK_REGS); } dev_priv->lid_notifier.notifier_call = intel_lid_notify; if (acpi_lid_notifier_register(&dev_priv->lid_notifier)) {
On Mon, 08 Aug 2011 13:24:12 -0700 Keith Packard keithp@keithp.com wrote:
On Mon, 8 Aug 2011 13:01:28 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
On Mon, 08 Aug 2011 12:53:31 -0700 Keith Packard keithp@keithp.com wrote:
On Mon, 8 Aug 2011 11:49:54 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Yep, it's safe and possible to do on pre-PCH as well. For panel fitting we do need to do an actual power cycle when going from non-native back to native iirc, but we can still leave them unlocked so we don't have to worry about the lock/unlock sequence everywhere.
Hidden in the unlock patch was a call to intel_lvds_disable from intel_lvds_prepare -- that *always* turns off the LVDS for mode setting. Do we care enough about LVDS mode setting performance that we should try leave the optimization in place that doesn't turn off the backlight when switching between modes?
We hate flicker right? But generally yes it's safer to just turn it off all the time.
I'll leave the optimization in place then; it's been there for a while so at least it shouldn't cause any regressions.
How about this? Has the advantage of not lying in the commit message anymore.
From 092719152aa5a235d6678798a34dc784d5cec2ad Mon Sep 17 00:00:00 2001 From: Keith Packard keithp@keithp.com Date: Sat, 6 Aug 2011 10:33:12 -0700 Subject: [PATCH 2/5] drm/i915: Leave LVDS registers unlocked
There's no reason to relock them; it just makes operations more complex. This fixes DPMS where the panel registers were locked making the disable not work.
Signed-off-by: Keith Packard keithp@keithp.com
Yeah looks like a nice improvement.
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
CPT pipe select is different from previous generations (using two bits instead of one). All of the paths from intel_disable_pch_ports were not making this distinction.
Mode setting with pipe A turned off would then also force all outputs on pipe B to get turned off as the disable code would mistakenly decide that all of these outputs were on pipe A and turn them off.
This is an extension of the CPT DP disable fix (why didn't I fix this then?)
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/i915_reg.h | 13 ++----- drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++++--- 2 files changed, 58 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index d1331f7..5baaef4 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1318,6 +1318,7 @@ #define ADPA_PIPE_SELECT_MASK (1<<30) #define ADPA_PIPE_A_SELECT 0 #define ADPA_PIPE_B_SELECT (1<<30) +#define ADPA_PIPE_SELECT(pipe) ((pipe) << 30) #define ADPA_USE_VGA_HVPOLARITY (1<<15) #define ADPA_SETS_HVPOLARITY 0 #define ADPA_VSYNC_CNTL_DISABLE (1<<11) @@ -1460,6 +1461,7 @@ /* Selects pipe B for LVDS data. Must be set on pre-965. */ #define LVDS_PIPEB_SELECT (1 << 30) #define LVDS_PIPE_MASK (1 << 30) +#define LVDS_PIPE(pipe) ((pipe) << 30) /* LVDS dithering flag on 965/g4x platform */ #define LVDS_ENABLE_DITHER (1 << 25) /* LVDS sync polarity flags. Set to invert (i.e. negative) */ @@ -1499,9 +1501,6 @@ #define LVDS_B0B3_POWER_DOWN (0 << 2) #define LVDS_B0B3_POWER_UP (3 << 2)
-#define LVDS_PIPE_ENABLED(V, P) \ - (((V) & (LVDS_PIPE_MASK | LVDS_PORT_EN)) == ((P) << 30 | LVDS_PORT_EN)) - /* Video Data Island Packet control */ #define VIDEO_DIP_DATA 0x61178 #define VIDEO_DIP_CTL 0x61170 @@ -3256,14 +3255,12 @@ #define ADPA_CRT_HOTPLUG_VOLREF_475MV (1<<17) #define ADPA_CRT_HOTPLUG_FORCE_TRIGGER (1<<16)
-#define ADPA_PIPE_ENABLED(V, P) \ - (((V) & (ADPA_TRANS_SELECT_MASK | ADPA_DAC_ENABLE)) == ((P) << 30 | ADPA_DAC_ENABLE)) - /* or SDVOB */ #define HDMIB 0xe1140 #define PORT_ENABLE (1 << 31) #define TRANSCODER_A (0) #define TRANSCODER_B (1 << 30) +#define TRANSCODER(pipe) ((pipe) << 30) #define TRANSCODER_MASK (1 << 30) #define COLOR_FORMAT_8bpc (0) #define COLOR_FORMAT_12bpc (3 << 26) @@ -3280,9 +3277,6 @@ #define HSYNC_ACTIVE_HIGH (1 << 3) #define PORT_DETECTED (1 << 2)
-#define HDMI_PIPE_ENABLED(V, P) \ - (((V) & (TRANSCODER_MASK | PORT_ENABLE)) == ((P) << 30 | PORT_ENABLE)) - /* PCH SDVOB multiplex with HDMIB */ #define PCH_SDVOB HDMIB
@@ -3349,6 +3343,7 @@ #define PORT_TRANS_B_SEL_CPT (1<<29) #define PORT_TRANS_C_SEL_CPT (2<<29) #define PORT_TRANS_SEL_MASK (3<<29) +#define PORT_TRANS_SEL_CPT(pipe) ((pipe) << 29)
#define TRANS_DP_CTL_A 0xe0300 #define TRANS_DP_CTL_B 0xe1300 diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 35364e6..4c4c903 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -998,6 +998,53 @@ static bool dp_pipe_enabled(struct drm_i915_private *dev_priv, enum pipe pipe, return true; }
+static bool hdmi_pipe_enabled(struct drm_i915_private *dev_priv, + enum pipe pipe, u32 val) +{ + if ((val & PORT_ENABLE) == 0) + return false; + + if (HAS_PCH_CPT(dev_priv->dev)) { + if ((val & PORT_TRANS_SEL_MASK) != PORT_TRANS_SEL_CPT(pipe)) + return false; + } else { + if ((val & TRANSCODER_MASK) != TRANSCODER(pipe)) + return false; + } + return true; +} + +static bool lvds_pipe_enabled(struct drm_i915_private *dev_priv, + enum pipe pipe, u32 val) +{ + if ((val & LVDS_PORT_EN) == 0) + return false; + + if (HAS_PCH_CPT(dev_priv->dev)) { + if ((val & PORT_TRANS_SEL_MASK) != PORT_TRANS_SEL_CPT(pipe)) + return false; + } else { + if ((val & LVDS_PIPE_MASK) != LVDS_PIPE(pipe)) + return false; + } + return true; +} + +static bool adpa_pipe_enabled(struct drm_i915_private *dev_priv, + enum pipe pipe, u32 val) +{ + if ((val & ADPA_DAC_ENABLE) == 0) + return false; + if (HAS_PCH_CPT(dev_priv->dev)) { + if ((val & PORT_TRANS_SEL_MASK) != PORT_TRANS_SEL_CPT(pipe)) + return false; + } else { + if ((val & ADPA_PIPE_SELECT_MASK) != ADPA_PIPE_SELECT(pipe)) + return false; + } + return true; +} + static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv, enum pipe pipe, int reg, u32 port_sel) { @@ -1011,7 +1058,7 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv, enum pipe pipe, int reg) { u32 val = I915_READ(reg); - WARN(HDMI_PIPE_ENABLED(val, pipe), + WARN(hdmi_pipe_enabled(dev_priv, val, pipe), "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n", reg, pipe_name(pipe)); } @@ -1028,13 +1075,13 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
reg = PCH_ADPA; val = I915_READ(reg); - WARN(ADPA_PIPE_ENABLED(val, pipe), + WARN(adpa_pipe_enabled(dev_priv, val, pipe), "PCH VGA enabled on transcoder %c, should be disabled\n", pipe_name(pipe));
reg = PCH_LVDS; val = I915_READ(reg); - WARN(LVDS_PIPE_ENABLED(val, pipe), + WARN(lvds_pipe_enabled(dev_priv, val, pipe), "PCH LVDS enabled on transcoder %c, should be disabled\n", pipe_name(pipe));
@@ -1370,7 +1417,7 @@ static void disable_pch_hdmi(struct drm_i915_private *dev_priv, enum pipe pipe, int reg) { u32 val = I915_READ(reg); - if (HDMI_PIPE_ENABLED(val, pipe)) { + if (hdmi_pipe_enabled(dev_priv, val, pipe)) { DRM_DEBUG_KMS("Disabling pch HDMI %x on pipe %d\n", reg, pipe); I915_WRITE(reg, val & ~PORT_ENABLE); @@ -1392,12 +1439,13 @@ static void intel_disable_pch_ports(struct drm_i915_private *dev_priv,
reg = PCH_ADPA; val = I915_READ(reg); - if (ADPA_PIPE_ENABLED(val, pipe)) + if (adpa_pipe_enabled(dev_priv, val, pipe)) I915_WRITE(reg, val & ~ADPA_DAC_ENABLE);
reg = PCH_LVDS; val = I915_READ(reg); - if (LVDS_PIPE_ENABLED(val, pipe)) { + if (lvds_pipe_enabled(dev_priv, val, pipe)) { + DRM_DEBUG_KMS("disable lvds on pipe %d val 0x%08x\n", pipe, val); I915_WRITE(reg, val & ~LVDS_PORT_EN); POSTING_READ(reg); udelay(100);
On Sat, 6 Aug 2011 10:54:07 -0700 Keith Packard keithp@keithp.com wrote:
CPT pipe select is different from previous generations (using two bits instead of one). All of the paths from intel_disable_pch_ports were not making this distinction.
Mode setting with pipe A turned off would then also force all outputs on pipe B to get turned off as the disable code would mistakenly decide that all of these outputs were on pipe A and turn them off.
This is an extension of the CPT DP disable fix (why didn't I fix this then?)
Signed-off-by: Keith Packard keithp@keithp.com
drivers/gpu/drm/i915/i915_reg.h | 13 ++----- drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++++--- 2 files changed, 58 insertions(+), 15 deletions(-)
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
Just an extra parameter which isn't actually needed.
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_display.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4c4c903..f6f18c7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -980,8 +980,8 @@ static void assert_transcoder_disabled(struct drm_i915_private *dev_priv, pipe_name(pipe)); }
-static bool dp_pipe_enabled(struct drm_i915_private *dev_priv, enum pipe pipe, - int reg, u32 port_sel, u32 val) +static bool dp_pipe_enabled(struct drm_i915_private *dev_priv, + enum pipe pipe, u32 port_sel, u32 val) { if ((val & DP_PORT_EN) == 0) return false; @@ -1049,7 +1049,7 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv, enum pipe pipe, int reg, u32 port_sel) { u32 val = I915_READ(reg); - WARN(dp_pipe_enabled(dev_priv, pipe, reg, port_sel, val), + WARN(dp_pipe_enabled(dev_priv, pipe, port_sel, val), "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n", reg, pipe_name(pipe)); } @@ -1407,7 +1407,7 @@ static void disable_pch_dp(struct drm_i915_private *dev_priv, enum pipe pipe, int reg, u32 port_sel) { u32 val = I915_READ(reg); - if (dp_pipe_enabled(dev_priv, pipe, reg, port_sel, val)) { + if (dp_pipe_enabled(dev_priv, pipe, port_sel, val)) { DRM_DEBUG_KMS("Disabling pch dp %x on pipe %d\n", reg, pipe); I915_WRITE(reg, val & ~DP_PORT_EN); }
On Sat, 6 Aug 2011 10:54:08 -0700 Keith Packard keithp@keithp.com wrote:
Just an extra parameter which isn't actually needed.
Signed-off-by: Keith Packard keithp@keithp.com
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
dri-devel@lists.freedesktop.org
-
Jesse Barnes -
Keith Packard