[PATCH 0/4] drm/i915: backlight fixes and cleanup

[PATCH 0/4] drm/i915: backlight fixes and cleanup

[Jani Nikula]

Hi all, this should fix some backlight issues, and do some cleanups
while at it. Please test.

BR,
Jani.


Daniel Vetter (1):
  drm/i915: save/restore the legacy backlight control

Jani Nikula (3):
  drm/i915: remove combination mode for backlight control, again
  drm/i915: remove brightness inversion quirk for acer aspire 5734z
  drm/i915: remove module parameter and quirk for inverting brightness

 Documentation/kernel-parameters.txt  |   14 --------
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/i915_reg.h      |    3 ++
 drivers/gpu/drm/i915/i915_suspend.c  |    8 +++++
 drivers/gpu/drm/i915/intel_display.c |   14 --------
 drivers/gpu/drm/i915/intel_panel.c   |   58 ----------------------------------
 6 files changed, 12 insertions(+), 87 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] drm/i915: save/restore the legacy backlight control

[Jani Nikula]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

This is a prep patch to stop drm/i915 from changing the LBPC registers
itself - but we still need to properly save/restore it on
suspend/resume.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |    1 +
 drivers/gpu/drm/i915/i915_reg.h     |    3 +++
 drivers/gpu/drm/i915/i915_suspend.c |    8 ++++++++
 drivers/gpu/drm/i915/intel_panel.c  |    2 --
 4 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 58b43db..af1701c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -606,6 +606,7 @@ typedef struct drm_i915_private {
 	u32 savePP_CONTROL;
 	u32 savePP_DIVISOR;
 	u32 savePFIT_CONTROL;
+	u8 saveLBPC;
 	u32 save_palette_a[256];
 	u32 save_palette_b[256];
 	u32 saveDPFC_CB_BASE;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index d0b60f2..3303c18 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1889,6 +1889,9 @@
 
 #define PFIT_AUTO_RATIOS 0x61238
 
+/* legacy/combination backlight modes in pci config space. */
+#define PCI_LBPC 0xf4
+
 /* Backlight control */
 #define BLC_PWM_CTL2		0x61250 /* 965+ only */
 #define   BLM_PWM_ENABLE		(1 << 31)
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 4776ccf..05daff7 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -639,6 +639,10 @@ static void i915_save_display(struct drm_device *dev)
 			dev_priv->saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_CTL2);
 		if (IS_MOBILE(dev) && !IS_I830(dev))
 			dev_priv->saveLVDS = I915_READ(LVDS);
+
+		if (IS_GEN2(dev) || IS_GEN4(dev))
+			pci_read_config_byte(dev->pdev, PCI_LBPC,
+					     &dev_priv->saveLBPC);
 	}
 
 	if (!IS_I830(dev) && !IS_845G(dev) && !HAS_PCH_SPLIT(dev))
@@ -759,6 +763,10 @@ static void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(PP_OFF_DELAYS, dev_priv->savePP_OFF_DELAYS);
 		I915_WRITE(PP_DIVISOR, dev_priv->savePP_DIVISOR);
 		I915_WRITE(PP_CONTROL, dev_priv->savePP_CONTROL);
+
+		if (IS_GEN2(dev) || IS_GEN4(dev))
+			pci_write_config_byte(dev->pdev, PCI_LBPC,
+					      dev_priv->saveLBPC);
 	}
 
 	/* Display Port state */
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 3df4f5f..9546f97 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -33,8 +33,6 @@
 #include <linux/moduleparam.h>
 #include "intel_drv.h"
 
-#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
-
 void
 intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 		       struct drm_display_mode *adjusted_mode)
-- 
1.7.9.5

[PATCH 2/4] drm/i915: remove combination mode for backlight control, again

[Jani Nikula]

The combination/legacy mode was first removed in

commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
Author: Indan Zupancic <indan@nul.nu>
Date:   Thu Feb 17 02:41:49 2011 +0100

    drm/i915: Do not handle backlight combination mode specially

which was subsequently reverted due to regression in

commit ba3820ade317ee36e496b9b40d2ec3987dd4aef0
Author: Takashi Iwai <tiwai@suse.de>
Date:   Thu Mar 10 14:02:12 2011 +0100

    drm/i915: Revive combination mode for backlight control

It seems that on some machines the combination mode was necessary because
it reset the LBPC register value on resume. Since the driver now saves and
restores the register over suspend, the combination mode no longer needs to
be treated specially, and the driver doesn't need to modify the LBPC
register at all.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53153
CC: Indan Zupancic <indan@nul.nu>
CC: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c |   32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 9546f97..0a7f47a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -115,19 +115,6 @@ done:
 	dev_priv->pch_pf_size = (width << 16) | height;
 }
 
-static int is_backlight_combination_mode(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (INTEL_INFO(dev)->gen >= 4)
-		return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
-
-	if (IS_GEN2(dev))
-		return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
-
-	return 0;
-}
-
 static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
 {
 	u32 val;
@@ -181,9 +168,6 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
 			max >>= 17;
 		else
 			max >>= 16;
-
-		if (is_backlight_combination_mode(dev))
-			max *= 0xff;
 	}
 
 	DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -222,13 +206,6 @@ static u32 intel_panel_get_backlight(struct drm_device *dev)
 		val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
 		if (INTEL_INFO(dev)->gen < 4)
 			val >>= 1;
-
-		if (is_backlight_combination_mode(dev)) {
-			u8 lbpc;
-
-			pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
-			val *= lbpc;
-		}
 	}
 
 	val = intel_panel_compute_brightness(dev, val);
@@ -254,15 +231,6 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level
 	if (HAS_PCH_SPLIT(dev))
 		return intel_pch_panel_set_backlight(dev, level);
 
-	if (is_backlight_combination_mode(dev)) {
-		u32 max = intel_panel_get_max_backlight(dev);
-		u8 lbpc;
-
-		lbpc = level * 0xfe / max + 1;
-		level /= lbpc;
-		pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc);
-	}
-
 	tmp = I915_READ(BLC_PWM_CTL);
 	if (INTEL_INFO(dev)->gen < 4) 
 		level <<= 1;
-- 
1.7.9.5

[PATCH 3/4] drm/i915: remove brightness inversion quirk for acer aspire 5734z

[Jani Nikula]

This reverts

commit 5a15ab5b93e4a3ebcd4fa6c76cf646a45e9cf806
Author: Carsten Emde <C.Emde@osadl.org>
Date:   Thu Mar 15 15:56:27 2012 +0100

    drm/i915: panel: invert brightness acer aspire 5734z

In preparation of the whole brightness inversion module parameter and quirk
removal.

CC: Carsten Emde <C.Emde@osadl.org>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 778cbb8..331c817 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7085,8 +7085,7 @@ static void quirk_ssc_force_disable(struct drm_device *dev)
 }
 
 /*
- * A machine (e.g. Acer Aspire 5734Z) may need to invert the panel backlight
- * brightness value
+ * A machine may need to invert the panel backlight brightness value
  */
 static void quirk_invert_brightness(struct drm_device *dev)
 {
@@ -7122,9 +7121,6 @@ static struct intel_quirk intel_quirks[] = {
 
 	/* Sony Vaio Y cannot use SSC on LVDS */
 	{ 0x0046, 0x104d, 0x9076, quirk_ssc_force_disable },
-
-	/* Acer Aspire 5734Z must invert backlight brightness */
-	{ 0x2a42, 0x1025, 0x0459, quirk_invert_brightness },
 };
 
 static void intel_init_quirks(struct drm_device *dev)
-- 
1.7.9.5

[PATCH 4/4] drm/i915: remove module parameter and quirk for inverting brightness

[Jani Nikula]

This is effectively a revert of

commit 4dca20efb1a9c2efefc28ad2867e5d6c3f5e1955
Author: Carsten Emde <C.Emde@osadl.org>
Date:   Thu Mar 15 15:56:26 2012 +0100

    drm/i915: panel: invert brightness via quirk

and

commit 7bd90909bbf9ce7c40e1da3d72b97b93839c188a
Author: Carsten Emde <C.Emde@osadl.org>
Date:   Thu Mar 15 15:56:25 2012 +0100

    drm/i915: panel: invert brightness via parameter

They were hacky solutions to problems caused by the combination mode, which
has now been removed. Thus clean up the code.

CC: Carsten Emde <C.Emde@osadl.org>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 Documentation/kernel-parameters.txt  |   14 --------------
 drivers/gpu/drm/i915/i915_drv.h      |    1 -
 drivers/gpu/drm/i915/intel_display.c |   10 ----------
 drivers/gpu/drm/i915/intel_panel.c   |   24 ------------------------
 4 files changed, 49 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index ad7e2e5..1dd1463 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1000,20 +1000,6 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	i8k.restricted	[HW] Allow controlling fans only if SYS_ADMIN
 			capability is set.
 
-	i915.invert_brightness=
-			[DRM] Invert the sense of the variable that is used to
-			set the brightness of the panel backlight. Normally a
-			brightness value of 0 indicates backlight switched off,
-			and the maximum of the brightness value sets the backlight
-			to maximum brightness. If this parameter is set to 0
-			(default) and the machine requires it, or this parameter
-			is set to 1, a brightness value of 0 sets the backlight
-			to maximum brightness, and the maximum of the brightness
-			value switches the backlight off.
-			-1 -- never invert brightness
-			 0 -- machine default
-			 1 -- force brightness inversion
-
 	icn=		[HW,ISDN]
 			Format: <io>[,<membase>[,<icn_id>[,<icn_id2>]]]
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index af1701c..7b34f46 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -375,7 +375,6 @@ enum intel_pch {
 
 #define QUIRK_PIPEA_FORCE (1<<0)
 #define QUIRK_LVDS_SSC_DISABLE (1<<1)
-#define QUIRK_INVERT_BRIGHTNESS (1<<2)
 
 struct intel_fbdev;
 struct intel_fbc_work;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 331c817..40cadea 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7084,16 +7084,6 @@ static void quirk_ssc_force_disable(struct drm_device *dev)
 	DRM_INFO("applying lvds SSC disable quirk\n");
 }
 
-/*
- * A machine may need to invert the panel backlight brightness value
- */
-static void quirk_invert_brightness(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	dev_priv->quirks |= QUIRK_INVERT_BRIGHTNESS;
-	DRM_INFO("applying inverted panel brightness quirk\n");
-}
-
 struct intel_quirk {
 	int device;
 	int subsystem_vendor;
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 0a7f47a..cfacac4 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -30,7 +30,6 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/moduleparam.h>
 #include "intel_drv.h"
 
 void
@@ -174,27 +173,6 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
 	return max;
 }
 
-static int i915_panel_invert_brightness;
-MODULE_PARM_DESC(invert_brightness, "Invert backlight brightness "
-	"(-1 force normal, 0 machine defaults, 1 force inversion), please "
-	"report PCI device ID, subsystem vendor and subsystem device ID "
-	"to dri-devel@lists.freedesktop.org, if your machine needs it. "
-	"It will then be included in an upcoming module version.");
-module_param_named(invert_brightness, i915_panel_invert_brightness, int, 0600);
-static u32 intel_panel_compute_brightness(struct drm_device *dev, u32 val)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (i915_panel_invert_brightness < 0)
-		return val;
-
-	if (i915_panel_invert_brightness > 0 ||
-	    dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS)
-		return intel_panel_get_max_backlight(dev) - val;
-
-	return val;
-}
-
 static u32 intel_panel_get_backlight(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -208,7 +186,6 @@ static u32 intel_panel_get_backlight(struct drm_device *dev)
 			val >>= 1;
 	}
 
-	val = intel_panel_compute_brightness(dev, val);
 	DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
 	return val;
 }
@@ -226,7 +203,6 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level
 	u32 tmp;
 
 	DRM_DEBUG_DRIVER("set backlight PWM = %d\n", level);
-	level = intel_panel_compute_brightness(dev, level);
 
 	if (HAS_PCH_SPLIT(dev))
 		return intel_pch_panel_set_backlight(dev, level);
-- 
1.7.9.5

Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control

[Chris Wilson]

On Tue, 28 Aug 2012 09:53:36 +0300, Jani Nikula <jani.nikula@intel.com> wrote:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> This is a prep patch to stop drm/i915 from changing the LBPC registers
> itself - but we still need to properly save/restore it on
> suspend/resume.

My presumption was that there were BIOSes out there that only adjusted
the LBPC values, and so any device setting that flag would likely only
with it through the BIOS hotkeys. I couldn't see any other rational
reason for the hardware maintaining multiple interfaces...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control

[Daniel Vetter]

On Tue, Aug 28, 2012 at 08:16:54AM +0100, Chris Wilson wrote:
> On Tue, 28 Aug 2012 09:53:36 +0300, Jani Nikula <jani.nikula@intel.com> wrote:
> > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > This is a prep patch to stop drm/i915 from changing the LBPC registers
> > itself - but we still need to properly save/restore it on
> > suspend/resume.
> 
> My presumption was that there were BIOSes out there that only adjusted
> the LBPC values, and so any device setting that flag would likely only
> with it through the BIOS hotkeys. I couldn't see any other rational
> reason for the hardware maintaining multiple interfaces...

I've run a hackish version of this through a few bug reports and they
reported that there's no more black screen at boot-up, but also that the
backlight doesn't work. My conclusion was:
- there are machines that are only controlled through the lbpc reg
- some of those use lbpc in an inverted sense
- we have now idea how to figure out the sense of lbpc

Hence my thinking is that we should avoid to touch lbpc (but just restore
it across suspend/resume since some machines need that) and rely on any
firmware madness to properly adjust the backlight. I also realize that
this will regress machines without working firmware, but for which the
current lbpc-adjusting mess seems to do the right thing.

But if the only regression this causes is some non-working backlight
adjusting I'll ignore that, since it looks like we fundamentally can't do
the right thing with lbpc, and this series here should kill a few black
screen bugs.

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

Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control

[Indan Zupancic]

Hello,

On Tue, August 28, 2012 08:53, Jani Nikula wrote:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> This is a prep patch to stop drm/i915 from changing the LBPC registers
> itself - but we still need to properly save/restore it on
> suspend/resume.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |    1 +
>  drivers/gpu/drm/i915/i915_reg.h     |    3 +++
>  drivers/gpu/drm/i915/i915_suspend.c |    8 ++++++++
>  drivers/gpu/drm/i915/intel_panel.c  |    2 --
>  4 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 58b43db..af1701c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -606,6 +606,7 @@ typedef struct drm_i915_private {
>  	u32 savePP_CONTROL;
>  	u32 savePP_DIVISOR;
>  	u32 savePFIT_CONTROL;
> +	u8 saveLBPC;
>  	u32 save_palette_a[256];
>  	u32 save_palette_b[256];
>  	u32 saveDPFC_CB_BASE;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index d0b60f2..3303c18 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1889,6 +1889,9 @@
>
>  #define PFIT_AUTO_RATIOS 0x61238
>
> +/* legacy/combination backlight modes in pci config space. */
> +#define PCI_LBPC 0xf4
> +
>  /* Backlight control */
>  #define BLC_PWM_CTL2		0x61250 /* 965+ only */
>  #define   BLM_PWM_ENABLE		(1 << 31)
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 4776ccf..05daff7 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -639,6 +639,10 @@ static void i915_save_display(struct drm_device *dev)
>  			dev_priv->saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_CTL2);
>  		if (IS_MOBILE(dev) && !IS_I830(dev))
>  			dev_priv->saveLVDS = I915_READ(LVDS);
> +
> +		if (IS_GEN2(dev) || IS_GEN4(dev))
> +			pci_read_config_byte(dev->pdev, PCI_LBPC,
> +					     &dev_priv->saveLBPC);

What about GEN3?

It seems weird that LBPC wouldn't be restored during resume by some BIOSes,
is this really necessary?

Greetings,

Indan

Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control

[Daniel Vetter]

On Tue, Aug 28, 2012 at 03:56:31PM +0200, Indan Zupancic wrote:
> Hello,
> 
> On Tue, August 28, 2012 08:53, Jani Nikula wrote:
> > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > This is a prep patch to stop drm/i915 from changing the LBPC registers
> > itself - but we still need to properly save/restore it on
> > suspend/resume.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h     |    1 +
> >  drivers/gpu/drm/i915/i915_reg.h     |    3 +++
> >  drivers/gpu/drm/i915/i915_suspend.c |    8 ++++++++
> >  drivers/gpu/drm/i915/intel_panel.c  |    2 --
> >  4 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 58b43db..af1701c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -606,6 +606,7 @@ typedef struct drm_i915_private {
> >  	u32 savePP_CONTROL;
> >  	u32 savePP_DIVISOR;
> >  	u32 savePFIT_CONTROL;
> > +	u8 saveLBPC;
> >  	u32 save_palette_a[256];
> >  	u32 save_palette_b[256];
> >  	u32 saveDPFC_CB_BASE;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index d0b60f2..3303c18 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1889,6 +1889,9 @@
> >
> >  #define PFIT_AUTO_RATIOS 0x61238
> >
> > +/* legacy/combination backlight modes in pci config space. */
> > +#define PCI_LBPC 0xf4
> > +
> >  /* Backlight control */
> >  #define BLC_PWM_CTL2		0x61250 /* 965+ only */
> >  #define   BLM_PWM_ENABLE		(1 << 31)
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > index 4776ccf..05daff7 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -639,6 +639,10 @@ static void i915_save_display(struct drm_device *dev)
> >  			dev_priv->saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_CTL2);
> >  		if (IS_MOBILE(dev) && !IS_I830(dev))
> >  			dev_priv->saveLVDS = I915_READ(LVDS);
> > +
> > +		if (IS_GEN2(dev) || IS_GEN4(dev))
> > +			pci_read_config_byte(dev->pdev, PCI_LBPC,
> > +					     &dev_priv->saveLBPC);
> 
> What about GEN3?
> 
> It seems weird that LBPC wouldn't be restored during resume by some BIOSes,
> is this really necessary?

ba3820ade317ee36e496b9b40d2ec3987dd4aef0 claims so. But that commit
managed to put too many things into the same thing unfortunately.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/4] drm/i915: remove combination mode for backlight control, again

[Indan Zupancic]

Hello,

You seem to be making exactly the same mistake I made.

On Tue, August 28, 2012 08:53, Jani Nikula wrote:
> The combination/legacy mode was first removed in
>
> commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
> Author: Indan Zupancic <indan@nul.nu>
> Date:   Thu Feb 17 02:41:49 2011 +0100
>
>     drm/i915: Do not handle backlight combination mode specially
>
> which was subsequently reverted due to regression in
>
> commit ba3820ade317ee36e496b9b40d2ec3987dd4aef0
> Author: Takashi Iwai <tiwai@suse.de>
> Date:   Thu Mar 10 14:02:12 2011 +0100
>
>     drm/i915: Revive combination mode for backlight control
>
> It seems that on some machines the combination mode was necessary because
> it reset the LBPC register value on resume.

No, it was bad interaction with systems using ASLE to control backlight.

To quote my old email:

  "On Thu, March 10, 2011 14:02, Takashi Iwai wrote:
  > This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
  >
  >     drm/i915: Do not handle backlight combination mode specially
  >
  > since this commit introduced other regressions due to untouched LBPC
  > register, e.g. the backlight dimmed after resume.

  The regression was that, if ALSE was used, the maximum brightness
  would be the brightness at boot, because LBPC isn't touched and
  the BIOS restores it at boot. So the sympom would be less brightness
  levels or lower maximum brightness.

  Systems with no ASLE support work fine because there the code is only
  used to save and restore the PWM register. LBPC is saved and restored
  by the BIOS.

  The backlight dimming after resume/blanking was the original bug
  caused by the bogus val <<=1 shift."

See http://marc.info/?l=dri-devel&m=129980668309522&w=2

> Since the driver now saves and
> restores the register over suspend, the combination mode no longer needs to
> be treated specially, and the driver doesn't need to modify the LBPC
> register at all.

That's what I thought when I wrote my original patch. But that only seems
to be true for GEN2 hardware which has no ASLE. It seems GEN4 with ASLE
needs the combination mode check because there the BIOS might use LBPC to
set the brightness at system boot, even though the driver only uses the
PWM registers to set it. (If the BIOS does something similar at resume
time then it's really messed up.) But assuming it only happens at bootup,
you could check for it once at module init and turn it off if it's enabled.
But it seems you can't totally ignore legacy/combination mode.

Some backlight problems on GEN4 can be solved by not fiddling with the
backlight. The current code sets the backlight to 0 to disable the panel
(last year anyway, maybe the code changed), but on gen >= 4 it can do the
same by clearing bit 31 in BLC_PWM_CLT2. That way the original backlight
value doesn't need to be saved anywhere.

>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53153
> CC: Indan Zupancic <indan@nul.nu>
> CC: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c |   32 --------------------------------
>  1 file changed, 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 9546f97..0a7f47a 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -115,19 +115,6 @@ done:
>  	dev_priv->pch_pf_size = (width << 16) | height;
>  }
>
> -static int is_backlight_combination_mode(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	if (INTEL_INFO(dev)->gen >= 4)
> -		return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> -
> -	if (IS_GEN2(dev))
> -		return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> -
> -	return 0;
> -}
> -
>  static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
>  {
>  	u32 val;
> @@ -181,9 +168,6 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
>  			max >>= 17;
>  		else
>  			max >>= 16;
> -
> -		if (is_backlight_combination_mode(dev))
> -			max *= 0xff;
>  	}
>
>  	DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> @@ -222,13 +206,6 @@ static u32 intel_panel_get_backlight(struct drm_device *dev)
>  		val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
>  		if (INTEL_INFO(dev)->gen < 4)
>  			val >>= 1;
> -
> -		if (is_backlight_combination_mode(dev)) {
> -			u8 lbpc;
> -
> -			pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
> -			val *= lbpc;
> -		}
>  	}
>
>  	val = intel_panel_compute_brightness(dev, val);
> @@ -254,15 +231,6 @@ static void intel_panel_actually_set_backlight(struct drm_device
> *dev, u32 level
>  	if (HAS_PCH_SPLIT(dev))
>  		return intel_pch_panel_set_backlight(dev, level);
>
> -	if (is_backlight_combination_mode(dev)) {
> -		u32 max = intel_panel_get_max_backlight(dev);
> -		u8 lbpc;
> -
> -		lbpc = level * 0xfe / max + 1;
> -		level /= lbpc;
> -		pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc);
> -	}
> -
>  	tmp = I915_READ(BLC_PWM_CTL);
>  	if (INTEL_INFO(dev)->gen < 4)
>  		level <<= 1;
> --
> 1.7.9.5
>
>

Greetings,

Indan

Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control

[Indan Zupancic]

On Tue, August 28, 2012 16:14, Daniel Vetter wrote:
> On Tue, Aug 28, 2012 at 03:56:31PM +0200, Indan Zupancic wrote:
>> Hello,
>>
>> On Tue, August 28, 2012 08:53, Jani Nikula wrote:
>> > From: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >
>> > This is a prep patch to stop drm/i915 from changing the LBPC registers
>> > itself - but we still need to properly save/restore it on
>> > suspend/resume.
>> >
>> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > ---
[...]
>> It seems weird that LBPC wouldn't be restored during resume by some BIOSes,
>> is this really necessary?
>
> ba3820ade317ee36e496b9b40d2ec3987dd4aef0 claims so. But that commit
> managed to put too many things into the same thing unfortunately.

Is that the right SHA? Because that just reverts my combination mode
removal patch. Assuming it is the right commit, then I think it's
incorrect in saying that it caused backlight dimming problems after
resume. That particular problem was caused by a bogus shift. The
problems caused by removing the mode was a lower max brightness
and/or less brightness levels.

By the way, saving LBPC only makes sense if it's done before it was
set to 0 to disable the panel. I don't know if the current code does
the right thing, I haven't looked at it for a while.

Greetings,

Indan

Re: [PATCH 2/4] drm/i915: remove combination mode for backlight control, again

[Daniel Vetter]

On Tue, Aug 28, 2012 at 04:39:34PM +0200, Indan Zupancic wrote:
> Hello,
> 
> You seem to be making exactly the same mistake I made.
> 
> On Tue, August 28, 2012 08:53, Jani Nikula wrote:
> > The combination/legacy mode was first removed in
> >
> > commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
> > Author: Indan Zupancic <indan@nul.nu>
> > Date:   Thu Feb 17 02:41:49 2011 +0100
> >
> >     drm/i915: Do not handle backlight combination mode specially
> >
> > which was subsequently reverted due to regression in
> >
> > commit ba3820ade317ee36e496b9b40d2ec3987dd4aef0
> > Author: Takashi Iwai <tiwai@suse.de>
> > Date:   Thu Mar 10 14:02:12 2011 +0100
> >
> >     drm/i915: Revive combination mode for backlight control
> >
> > It seems that on some machines the combination mode was necessary because
> > it reset the LBPC register value on resume.
> 
> No, it was bad interaction with systems using ASLE to control backlight.
> 
> To quote my old email:
> 
>   "On Thu, March 10, 2011 14:02, Takashi Iwai wrote:
>   > This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
>   >
>   >     drm/i915: Do not handle backlight combination mode specially
>   >
>   > since this commit introduced other regressions due to untouched LBPC
>   > register, e.g. the backlight dimmed after resume.
> 
>   The regression was that, if ALSE was used, the maximum brightness
>   would be the brightness at boot, because LBPC isn't touched and
>   the BIOS restores it at boot. So the sympom would be less brightness
>   levels or lower maximum brightness.
> 
>   Systems with no ASLE support work fine because there the code is only
>   used to save and restore the PWM register. LBPC is saved and restored
>   by the BIOS.
> 
>   The backlight dimming after resume/blanking was the original bug
>   caused by the bogus val <<=1 shift."
> 
> See http://marc.info/?l=dri-devel&m=129980668309522&w=2

Meh, I've totally failed to dig this out. Makes quite a bit more sense and
clarifies the confusion revert commit message quite a bit. Thanks for
digging this out again, I guess we should include it.

> > Since the driver now saves and
> > restores the register over suspend, the combination mode no longer needs to
> > be treated specially, and the driver doesn't need to modify the LBPC
> > register at all.
> 
> That's what I thought when I wrote my original patch. But that only seems
> to be true for GEN2 hardware which has no ASLE. It seems GEN4 with ASLE
> needs the combination mode check because there the BIOS might use LBPC to
> set the brightness at system boot, even though the driver only uses the
> PWM registers to set it. (If the BIOS does something similar at resume
> time then it's really messed up.) But assuming it only happens at bootup,
> you could check for it once at module init and turn it off if it's enabled.
> But it seems you can't totally ignore legacy/combination mode.
> 
> Some backlight problems on GEN4 can be solved by not fiddling with the
> backlight. The current code sets the backlight to 0 to disable the panel
> (last year anyway, maybe the code changed), but on gen >= 4 it can do the
> same by clearing bit 31 in BLC_PWM_CLT2. That way the original backlight
> value doesn't need to be saved anywhere.

A while back I've improved the backlight code to properly switch the pipe
that controls the pwm and also to properly toggle the enable bit for
gen4+, see the new intel_panel_enable_backlight functions. Would it be
correct in your opinion to simply ditch the call to
intel_panel_actually_set_backlight for gen4+ unconditionally? Same for
intel_panel_disable_backlight obviously?

Thanks, Daniel

> 
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53153
> > CC: Indan Zupancic <indan@nul.nu>
> > CC: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c |   32 --------------------------------
> >  1 file changed, 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 9546f97..0a7f47a 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -115,19 +115,6 @@ done:
> >  	dev_priv->pch_pf_size = (width << 16) | height;
> >  }
> >
> > -static int is_backlight_combination_mode(struct drm_device *dev)
> > -{
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> > -	if (INTEL_INFO(dev)->gen >= 4)
> > -		return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> > -
> > -	if (IS_GEN2(dev))
> > -		return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> > -
> > -	return 0;
> > -}
> > -
> >  static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 val;
> > @@ -181,9 +168,6 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
> >  			max >>= 17;
> >  		else
> >  			max >>= 16;
> > -
> > -		if (is_backlight_combination_mode(dev))
> > -			max *= 0xff;
> >  	}
> >
> >  	DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
> > @@ -222,13 +206,6 @@ static u32 intel_panel_get_backlight(struct drm_device *dev)
> >  		val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
> >  		if (INTEL_INFO(dev)->gen < 4)
> >  			val >>= 1;
> > -
> > -		if (is_backlight_combination_mode(dev)) {
> > -			u8 lbpc;
> > -
> > -			pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
> > -			val *= lbpc;
> > -		}
> >  	}
> >
> >  	val = intel_panel_compute_brightness(dev, val);
> > @@ -254,15 +231,6 @@ static void intel_panel_actually_set_backlight(struct drm_device
> > *dev, u32 level
> >  	if (HAS_PCH_SPLIT(dev))
> >  		return intel_pch_panel_set_backlight(dev, level);
> >
> > -	if (is_backlight_combination_mode(dev)) {
> > -		u32 max = intel_panel_get_max_backlight(dev);
> > -		u8 lbpc;
> > -
> > -		lbpc = level * 0xfe / max + 1;
> > -		level /= lbpc;
> > -		pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc);
> > -	}
> > -
> >  	tmp = I915_READ(BLC_PWM_CTL);
> >  	if (INTEL_INFO(dev)->gen < 4)
> >  		level <<= 1;
> > --
> > 1.7.9.5
> >
> >
> 
> Greetings,
> 
> Indan
> 
> 

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

Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control

[Daniel Vetter]

On Tue, Aug 28, 2012 at 04:49:15PM +0200, Indan Zupancic wrote:
> On Tue, August 28, 2012 16:14, Daniel Vetter wrote:
> > On Tue, Aug 28, 2012 at 03:56:31PM +0200, Indan Zupancic wrote:
> >> Hello,
> >>
> >> On Tue, August 28, 2012 08:53, Jani Nikula wrote:
> >> > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> >
> >> > This is a prep patch to stop drm/i915 from changing the LBPC registers
> >> > itself - but we still need to properly save/restore it on
> >> > suspend/resume.
> >> >
> >> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> > ---
> [...]
> >> It seems weird that LBPC wouldn't be restored during resume by some BIOSes,
> >> is this really necessary?
> >
> > ba3820ade317ee36e496b9b40d2ec3987dd4aef0 claims so. But that commit
> > managed to put too many things into the same thing unfortunately.
> 
> Is that the right SHA? Because that just reverts my combination mode
> removal patch. Assuming it is the right commit, then I think it's
> incorrect in saying that it caused backlight dimming problems after
> resume. That particular problem was caused by a bogus shift. The
> problems caused by removing the mode was a lower max brightness
> and/or less brightness levels.

Yeah, right commit but imo with sub-par commit message. Your other mail
clarified things, thanks.

> By the way, saving LBPC only makes sense if it's done before it was
> set to 0 to disable the panel. I don't know if the current code does
> the right thing, I haven't looked at it for a while.

I think we can coax it into doing the right thing, see my other mail. If
your completely sure that lbpc /should/ be handled by the bios across s/r
I think we can drop this. But tbh I have no idea how this really is
supposed to work, and unfortunately we're not allowed to cross-check with
the windows driver codebase :(

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

Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control

[Indan Zupancic]

On Tue, August 28, 2012 17:15, Daniel Vetter wrote:
> On Tue, Aug 28, 2012 at 04:49:15PM +0200, Indan Zupancic wrote:
>> By the way, saving LBPC only makes sense if it's done before it was
>> set to 0 to disable the panel. I don't know if the current code does
>> the right thing, I haven't looked at it for a while.
>
> I think we can coax it into doing the right thing, see my other mail. If
> your completely sure that lbpc /should/ be handled by the bios across s/r
> I think we can drop this. But tbh I have no idea how this really is
> supposed to work, and unfortunately we're not allowed to cross-check with
> the windows driver codebase :(

Of course I'm not completely sure. But I agree with Chris' reasoning,
why else have two ways of controlling the backlight? To me LBPC gives
the impression of being a system specific thing the OS shouldn't need
to worry about. Partly because it wasn't documented in the old docs at
all, and its address isn't mentioned in the 965 vol3 manual, but also
because it's from around the time that the BIOS used to do a lot more
directly (APM versus ACPI etc.) It also gives a way to lower the max
output voltage (via PWM) to the backlight hardware.

These kind of problems are caused by us not having exactly the same
info as the BIOS/machine makers got. Because that's what guided them
into making the stuff as it is, not how it's supposed to work.

The code could check if LBPC has a reasonable value after resume: Do
nothing if it has and restore the last known good value if it doesn't.

Writing 0 to LBPC before suspend seems unnecessary because the hardware
is going into sleep mode anyway. So the PWM signal should become 0 anyway,
or if it doesn't, the BIOS has a chance to disable the backlight. But I'm
not sure if that's really true.

So to get back to this patch, saving the LBPC value only makes sense if
it's done before it's set to 0. I think saving it at i915_save_display()
time is too late if the panel got disabled. Then the code is always saving
and restoring zeros.

BTW, inverted sense of the PWM register could be related to the polarity
of the PWM signal. That should be controlled with bit 28 in BLC_PWM_CTL2.

Greetings,

Indan

Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control

[Daniel Vetter]

On Thu, Aug 30, 2012 at 10:32:48AM +0200, Indan Zupancic wrote:
> On Tue, August 28, 2012 17:15, Daniel Vetter wrote:
> > On Tue, Aug 28, 2012 at 04:49:15PM +0200, Indan Zupancic wrote:
> >> By the way, saving LBPC only makes sense if it's done before it was
> >> set to 0 to disable the panel. I don't know if the current code does
> >> the right thing, I haven't looked at it for a while.
> >
> > I think we can coax it into doing the right thing, see my other mail. If
> > your completely sure that lbpc /should/ be handled by the bios across s/r
> > I think we can drop this. But tbh I have no idea how this really is
> > supposed to work, and unfortunately we're not allowed to cross-check with
> > the windows driver codebase :(
> 
> Of course I'm not completely sure. But I agree with Chris' reasoning,
> why else have two ways of controlling the backlight? To me LBPC gives
> the impression of being a system specific thing the OS shouldn't need
> to worry about. Partly because it wasn't documented in the old docs at
> all, and its address isn't mentioned in the 965 vol3 manual, but also
> because it's from around the time that the BIOS used to do a lot more
> directly (APM versus ACPI etc.) It also gives a way to lower the max
> output voltage (via PWM) to the backlight hardware.
> 
> These kind of problems are caused by us not having exactly the same
> info as the BIOS/machine makers got. Because that's what guided them
> into making the stuff as it is, not how it's supposed to work.
> 
> The code could check if LBPC has a reasonable value after resume: Do
> nothing if it has and restore the last known good value if it doesn't.
> 
> Writing 0 to LBPC before suspend seems unnecessary because the hardware
> is going into sleep mode anyway. So the PWM signal should become 0 anyway,
> or if it doesn't, the BIOS has a chance to disable the backlight. But I'm
> not sure if that's really true.
> 
> So to get back to this patch, saving the LBPC value only makes sense if
> it's done before it's set to 0. I think saving it at i915_save_display()
> time is too late if the panel got disabled. Then the code is always saving
> and restoring zeros.

Well, if we use that backlight enable bit for panel on/off, we should no
longer clear neither lbpc nor the backlight registers. Hence I think this
might just work, without us saving lbpc. But we can easily add that later
on I think (if it's required on some kind of crazy machine).

> BTW, inverted sense of the PWM register could be related to the polarity
> of the PWM signal. That should be controlled with bit 28 in BLC_PWM_CTL2.

Well, I've tried that and it didn't work. I suspect that this entire
inverted backlight quirk mess we currently have is just an ugly hack to
prevent our code from setting the backlight to 0 and doesn't actually work
as advertised. Imo it was bad judgment to merge it ...

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

Re: [PATCH 2/4] drm/i915: remove combination mode for backlight control, again

[Indan Zupancic]

On Tue, August 28, 2012 16:55, Daniel Vetter wrote:
> On Tue, Aug 28, 2012 at 04:39:34PM +0200, Indan Zupancic wrote:
>> Some backlight problems on GEN4 can be solved by not fiddling with the
>> backlight. The current code sets the backlight to 0 to disable the panel
>> (last year anyway, maybe the code changed), but on gen >= 4 it can do the
>> same by clearing bit 31 in BLC_PWM_CLT2. That way the original backlight
>> value doesn't need to be saved anywhere.
>
> A while back I've improved the backlight code to properly switch the pipe
> that controls the pwm and also to properly toggle the enable bit for
> gen4+, see the new intel_panel_enable_backlight functions. Would it be
> correct in your opinion to simply ditch the call to
> intel_panel_actually_set_backlight for gen4+ unconditionally? Same for
> intel_panel_disable_backlight obviously?

Yes, that seems the whole point of having the PWM disable bit.

I would also ditch the backlight_enabled state tracking, as it's
unnecessary and incorrect because the enable/disable calls aren't
balanced.

I'll update my kernel to the latest git code and try out how the
current code works and if not touching LBPC at all works for gen 2
hardware. If it doesn't then LBPC needs to be saved/restored in
i915_suspend.c after all.

It would be nice if backlight control worked with ASLE disabled,
that would get rid of all complexity, including combination mode.
Or alternatively, if disabling combination mode at boot works then
we can get rid of the complicated brightness setting code to cope
with it.

Greetings,

Indan

Re: [PATCH 2/4] drm/i915: remove combination mode for backlight control, again

[Jesse Barnes]

On Thu, 30 Aug 2012 11:29:11 +0200
"Indan Zupancic" <indan@nul.nu> wrote:

> On Tue, August 28, 2012 16:55, Daniel Vetter wrote:
> > On Tue, Aug 28, 2012 at 04:39:34PM +0200, Indan Zupancic wrote:
> >> Some backlight problems on GEN4 can be solved by not fiddling with the
> >> backlight. The current code sets the backlight to 0 to disable the panel
> >> (last year anyway, maybe the code changed), but on gen >= 4 it can do the
> >> same by clearing bit 31 in BLC_PWM_CLT2. That way the original backlight
> >> value doesn't need to be saved anywhere.
> >
> > A while back I've improved the backlight code to properly switch the pipe
> > that controls the pwm and also to properly toggle the enable bit for
> > gen4+, see the new intel_panel_enable_backlight functions. Would it be
> > correct in your opinion to simply ditch the call to
> > intel_panel_actually_set_backlight for gen4+ unconditionally? Same for
> > intel_panel_disable_backlight obviously?
> 
> Yes, that seems the whole point of having the PWM disable bit.
> 
> I would also ditch the backlight_enabled state tracking, as it's
> unnecessary and incorrect because the enable/disable calls aren't
> balanced.
> 
> I'll update my kernel to the latest git code and try out how the
> current code works and if not touching LBPC at all works for gen 2
> hardware. If it doesn't then LBPC needs to be saved/restored in
> i915_suspend.c after all.
> 
> It would be nice if backlight control worked with ASLE disabled,
> that would get rid of all complexity, including combination mode.
> Or alternatively, if disabling combination mode at boot works then
> we can get rid of the complicated brightness setting code to cope
> with it.

Did we bottom out on this discussion?  Reading through the thread I
don't see any firm conclusions, and we definitely still have bugs open
that may be resolved by this incorrect patchset...

-- 
Jesse Barnes, Intel Open Source Technology Center