diff mbox

[Intel-gfx,2/4] drm/i915: Leave LVDS registers unlocked

Message ID yunobzz1xz8.fsf@aiko.keithp.com
State New, archived
Headers show

Commit Message

Keith Packard Aug. 8, 2011, 7:53 p.m. UTC
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(-)

Comments

Jesse Barnes Aug. 8, 2011, 8:01 p.m. UTC | #1
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.
Jesse Barnes Aug. 8, 2011, 8:25 p.m. UTC | #2
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>
diff mbox

Patch

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)) {