Signed-off-by: Alex Riesen raa.lkml@gmail.com
--- Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
Most of the changes are pretty spread out and small, with drivers/gpu (radeon and i915) somewhat standing out from the pack. ...
The backlight level on this Dell XPS M1330 reduces every time I reopen the lid, and BIOS does not seem to know anything about that (the keyboard shortcuts to set backlight brightness cause it to jump to the level next to the one set before closing the lid).
Maybe i915 code for LVDS panels have lost some parts of the backlight enable/disable balancing patch by Chris Wilson? Or maybe it just got broken along the way...
This part of the patch by Chris helped here, but I afraid it might be not complete or just wrong (for instance, the original patch didn't have to remove the i915_read_blc_pwm_ctl function).
drivers/gpu/drm/i915/intel_panel.c | 65 ++++++++++-------------------------- 1 files changed, 18 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index c65992d..c4b1ca4 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -125,55 +125,15 @@ static int is_backlight_combination_mode(struct drm_device *dev) return 0; }
-static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv) -{ - u32 val; - - /* Restore the CTL value if it lost, e.g. GPU reset */ - - if (HAS_PCH_SPLIT(dev_priv->dev)) { - val = I915_READ(BLC_PWM_PCH_CTL2); - if (dev_priv->saveBLC_PWM_CTL2 == 0) { - dev_priv->saveBLC_PWM_CTL2 = val; - } else if (val == 0) { - I915_WRITE(BLC_PWM_PCH_CTL2, - dev_priv->saveBLC_PWM_CTL); - val = dev_priv->saveBLC_PWM_CTL; - } - } else { - val = I915_READ(BLC_PWM_CTL); - if (dev_priv->saveBLC_PWM_CTL == 0) { - dev_priv->saveBLC_PWM_CTL = val; - dev_priv->saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_CTL2); - } else if (val == 0) { - I915_WRITE(BLC_PWM_CTL, - dev_priv->saveBLC_PWM_CTL); - I915_WRITE(BLC_PWM_CTL2, - dev_priv->saveBLC_PWM_CTL2); - val = dev_priv->saveBLC_PWM_CTL; - } - } - - return val; -} - u32 intel_panel_get_max_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; u32 max;
- max = i915_read_blc_pwm_ctl(dev_priv); - if (max == 0) { - /* XXX add code here to query mode clock or hardware clock - * and program max PWM appropriately. - */ - printk_once(KERN_WARNING "fixme: max PWM is zero.\n"); - return 1; - } - if (HAS_PCH_SPLIT(dev)) { - max >>= 16; + max = I915_READ(BLC_PWM_PCH_CTL2) >> 16; } else { + max = I915_READ(BLC_PWM_CTL); if (IS_PINEVIEW(dev)) { max >>= 17; } else { @@ -186,6 +146,14 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) max *= 0xff; }
+ if (max == 0) { + /* XXX add code here to query mode clock or hardware clock + * and program max PWM appropriately. + */ + DRM_ERROR("fixme: max PWM is zero.\n"); + max = 1; + } + DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max); return max; } @@ -255,11 +223,11 @@ void intel_panel_disable_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;
- if (dev_priv->backlight_enabled) { - dev_priv->backlight_level = intel_panel_get_backlight(dev); - dev_priv->backlight_enabled = false; - } + if (!dev_priv->backlight_enabled) + return;
+ dev_priv->backlight_enabled = false; + dev_priv->backlight_level = intel_panel_get_backlight(dev); intel_panel_set_backlight(dev, 0); }
@@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;
+ if (dev_priv->backlight_enabled) + return; + if (dev_priv->backlight_level == 0) dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
@@ -278,6 +249,6 @@ void intel_panel_setup_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;
- dev_priv->backlight_level = intel_panel_get_backlight(dev); + dev_priv->backlight_level = intel_panel_get_max_backlight(dev); dev_priv->backlight_enabled = dev_priv->backlight_level != 0; }
On Wed, Feb 16, 2011 at 20:26, Alex Riesen raa.lkml@gmail.com wrote:
Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
Most of the changes are pretty spread out and small, with drivers/gpu (radeon and i915) somewhat standing out from the pack. ...
The backlight level on this Dell XPS M1330 reduces every time I reopen the lid, and BIOS does not seem to know anything about that (the keyboard shortcuts to set backlight brightness cause it to jump to the level next to the one set before closing the lid).
It is this bug, I believe:
https://bugzilla.kernel.org/show_bug.cgi?id=25072
I somehow missed it at first, and only noticed after sending the patch.
On Wed, 16 Feb 2011 20:46:45 +0100 Alex Riesen raa.lkml@gmail.com wrote:
On Wed, Feb 16, 2011 at 20:26, Alex Riesen raa.lkml@gmail.com wrote:
Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
Most of the changes are pretty spread out and small, with drivers/gpu (radeon and i915) somewhat standing out from the pack. ...
The backlight level on this Dell XPS M1330 reduces every time I reopen the lid, and BIOS does not seem to know anything about that (the keyboard shortcuts to set backlight brightness cause it to jump to the level next to the one set before closing the lid).
It is this bug, I believe:
https://bugzilla.kernel.org/show_bug.cgi?id=25072
I somehow missed it at first, and only noticed after sending the patch.
There's also this patch: https://patchwork.kernel.org/patch/499221/. It got things working on my E6510 again at least.
On Wed, Feb 16, 2011 at 20:54, Jesse Barnes jbarnes@virtuousgeek.org wrote:
On Wed, 16 Feb 2011 20:46:45 +0100 Alex Riesen raa.lkml@gmail.com wrote:
The backlight level on this Dell XPS M1330 reduces every time I reopen the lid, and BIOS does not seem to know anything about that (the keyboard shortcuts to set backlight brightness cause it to jump to the level next to the one set before closing the lid).
It is this bug, I believe:
https://bugzilla.kernel.org/show_bug.cgi?id=25072
I somehow missed it at first, and only noticed after sending the patch.
There's also this patch: https://patchwork.kernel.org/patch/499221/. It got things working on my E6510 again at least.
I don't think it is related. I don't have problems switching the outputs (frankly, didn't try) and I do have problems restoring backlight, very similar to what I had earlier in 2.6.37.
On Wed, 16 Feb 2011 20:59:35 +0100 Alex Riesen raa.lkml@gmail.com wrote:
On Wed, Feb 16, 2011 at 20:54, Jesse Barnes jbarnes@virtuousgeek.org wrote:
On Wed, 16 Feb 2011 20:46:45 +0100 Alex Riesen raa.lkml@gmail.com wrote:
The backlight level on this Dell XPS M1330 reduces every time I reopen the lid, and BIOS does not seem to know anything about that (the keyboard shortcuts to set backlight brightness cause it to jump to the level next to the one set before closing the lid).
It is this bug, I believe:
https://bugzilla.kernel.org/show_bug.cgi?id=25072
I somehow missed it at first, and only noticed after sending the patch.
There's also this patch: https://patchwork.kernel.org/patch/499221/. It got things working on my E6510 again at least.
I don't think it is related. I don't have problems switching the outputs (frankly, didn't try) and I do have problems restoring backlight, very similar to what I had earlier in 2.6.37.
Right, but it affects the registration of the backlight and ACPI video interface as well, so can affect backlight restore on resume. In my case, without the above patch my backlight wouldn't be restored on resume, so I'd have to manually echo a value into /sys/class/backlight/<foo> to get my display back.
On Wed, Feb 16, 2011 at 21:05, Jesse Barnes jbarnes@virtuousgeek.org wrote:
On Wed, 16 Feb 2011 20:59:35 +0100 Alex Riesen raa.lkml@gmail.com wrote:
I don't think it is related. I don't have problems switching the outputs (frankly, didn't try) and I do have problems restoring backlight, very similar to what I had earlier in 2.6.37.
Right, but it affects the registration of the backlight and ACPI video interface as well, so can affect backlight restore on resume. In my case, without the above patch my backlight wouldn't be restored on resume, so I'd have to manually echo a value into /sys/class/backlight/<foo> to get my display back.
Ok, I tried, but only to find out that my tree already has the patch (it is in mainline since today). So it definitely does not help.
Thanks anyway
drm/i915: Do not handle backlight combination mode specially.
The current code does not follow Intel documentation: It misses some things and does other, undocumented things. This causes wrong backlight values in certain conditions. Instead of adding tricky code handling badly documented and rare corner cases, don't handle combination mode specially at all. This way PCI_LBPC is never touched and weird things shouldn't happen.
If combination mode is enabled, then the only downside is that changing the brightness has a greater granularity (the LBPC value), but LBPC is at most 254 and the maximum is in the thousands, so this is no real functional loss.
A potential problem with not handling combined mode is that a brightness of max * PCI_LBPC is not bright enough. However, this is very unlikely because from the documentation LBPC seems to act as a scaling factor and doesn't look like it's supposed to be changed after boot. The value at boot should always result in a bright enough screen.
IMPORTANT: However, although usually the above is true, it may not be when people ran an older (2.6.37) kernel which messed up the LBPC register, and they are unlucky enough to have a BIOS that saves and restores the LBPC value. Then a good kernel may seem to not work: Max brightness isn't bright enough. If this happens people should boot back into the old kernel, set brightness to the maximum, and then reboot. After that everything should be fine.
For more information see the below links. This fixes bugs:
http://bugzilla.kernel.org/show_bug.cgi?id=23472 http://bugzilla.kernel.org/show_bug.cgi?id=25072
Signed-off-by: Indan Zupancic indan@nul.nu
---
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 5cfc689..af2fc32 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1551,17 +1551,7 @@
/* Backlight control */ #define BLC_PWM_CTL 0x61254 -#define BACKLIGHT_MODULATION_FREQ_SHIFT (17) #define BLC_PWM_CTL2 0x61250 /* 965+ only */ -#define BLM_COMBINATION_MODE (1 << 30) -/* - * This is the most significant 15 bits of the number of backlight cycles in a - * complete cycle of the modulated backlight control. - * - * The actual value is this field multiplied by two. - */ -#define BACKLIGHT_MODULATION_FREQ_MASK (0x7fff << 17) -#define BLM_LEGACY_MODE (1 << 16) /* * This is the number of cycles out of the backlight modulation cycle for which * the backlight is on. diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index c65992d..d860abe 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,8 +30,6 @@
#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) @@ -112,19 +110,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 +166,6 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) if (INTEL_INFO(dev)->gen < 4) max &= ~1; } - - if (is_backlight_combination_mode(dev)) - max *= 0xff; }
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max); @@ -201,15 +183,6 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1; - - if (is_backlight_combination_mode(dev)){ - u8 lbpc; - - val &= ~1; - pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); - val *= lbpc; - val >>= 1; - } }
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val); @@ -232,16 +205,6 @@ void intel_panel_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 lpbc; - - lpbc = level * 0xfe / max + 1; - level /= lpbc; - pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc); - } - tmp = I915_READ(BLC_PWM_CTL); if (IS_PINEVIEW(dev)) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
On Wed, Feb 16, 2011 at 20:26:58 +0100, Alex Riesen wrote:
Signed-off-by: Alex Riesen raa.lkml@gmail.com
Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
Most of the changes are pretty spread out and small, with drivers/gpu (radeon and i915) somewhat standing out from the pack. ...
The backlight level on this Dell XPS M1330 reduces every time I reopen the lid, and BIOS does not seem to know anything about that (the keyboard shortcuts to set backlight brightness cause it to jump to the level next to the one set before closing the lid).
Hi,
with kernel 2.6.37, the display brightness of my ThinkPad X61s was always reduced after lid open, resume from suspend etc. With this patch on top of 2.6.38-rc5, the problem is gone. Thanks.
Regards, Tino
On Thu, February 17, 2011 23:13, Tino Keitel wrote:
with kernel 2.6.37, the display brightness of my ThinkPad X61s was always reduced after lid open, resume from suspend etc. With this patch on top of 2.6.38-rc5, the problem is gone. Thanks.
Tino, I think Alex's patch only hides the problem and doesn't properly solve the real bug. Can you confirm that this is the bit that fixes it for you?
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index c65992d..c4b1ca4 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;
+ if (dev_priv->backlight_enabled) + return; + if (dev_priv->backlight_level == 0) dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
(Alex's patch edited by hand, offsets might be wrong.)
The other bits either don't change the logic, or should be harmless, or are plain wrong, like setting the brightness to maximum at bootup.
If the above bit "fixes" it then it's because intel_panel_set_backlight() is called less often, as that's the buggy function the problem doesn't show up (or is less clear). Calling intel_panel_set_backlight() with the same value should keep the brightness the same. Because of the buggy combination code it doesn't always.
Also, try suspending/resuming or "xset dpms force off/on" often in a loop with both highest and lowest brightness and check if it works correctly with just Alex's patch.
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes it for you too? (Make sure you're at max brightness before rebooting.)
That said, the above bit of Alex's patch should be fine to apply, because it avoids unnecessary register fiddling either way.
Greetings,
Indan
Sorry for this late answer. I only get a very little time for this.
On Fri, Feb 18, 2011 at 05:57, Indan Zupancic indan@nul.nu wrote:
On Thu, February 17, 2011 23:13, Tino Keitel wrote:
with kernel 2.6.37, the display brightness of my ThinkPad X61s was always reduced after lid open, resume from suspend etc. With this patch on top of 2.6.38-rc5, the problem is gone. Thanks.
Tino, I think Alex's patch only hides the problem and doesn't properly solve
Could well be. I don't understand what the code is supposed to do. The patch was created just be looking at diffs.
the real bug. Can you confirm that this is the bit that fixes it for you?
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index c65992d..c4b1ca4 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;
- if (dev_priv->backlight_enabled)
- return;
if (dev_priv->backlight_level == 0) dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
(Alex's patch edited by hand, offsets might be wrong.)
It is not enough, at least for me.
The other bits either don't change the logic, or should be harmless, or are plain wrong, like setting the brightness to maximum at bootup.
I am not absolutely sure, but I don't think this is what happens on this laptop.
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes it for you too? (Make sure you're at max brightness before rebooting.)
I'll try it now.
On Sat, Feb 19, 2011 at 13:11, Alex Riesen raa.lkml@gmail.com wrote:
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes it for you too? (Make sure you're at max brightness before rebooting.)
I'll try it now.
I can confirm that it does fix backlight in my case (Dell XPS 1330, LVDS panel, GM965/GL960).
Tested-by: Alex Riesen raa.lkml@gmail.com
On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen raa.lkml@gmail.com wrote:
On Sat, Feb 19, 2011 at 13:11, Alex Riesen raa.lkml@gmail.com wrote:
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes it for you too? (Make sure you're at max brightness before rebooting.)
I'll try it now.
I can confirm that it does fix backlight in my case (Dell XPS 1330, LVDS panel, GM965/GL960).
Tested-by: Alex Riesen raa.lkml@gmail.com
Guys, should I apply this, or will I get it through somebody's pull?
Linus
On Sat, 19 Feb 2011 15:07:49 -0800 Linus Torvalds torvalds@linux-foundation.org wrote:
On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen raa.lkml@gmail.com wrote:
On Sat, Feb 19, 2011 at 13:11, Alex Riesen raa.lkml@gmail.com wrote:
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes it for you too? (Make sure you're at max brightness before rebooting.)
I'll try it now.
I can confirm that it does fix backlight in my case (Dell XPS 1330, LVDS panel, GM965/GL960).
Tested-by: Alex Riesen raa.lkml@gmail.com
Guys, should I apply this, or will I get it through somebody's pull?
I'm worried that removing combo mode will break some working setups, but if it's seen a lot of testing and is ok, then I'm fine with it, as it definitely simplifies things.
On Tue, Feb 22, 2011 at 13:04:40 -0800, Jesse Barnes wrote:
On Sat, 19 Feb 2011 15:07:49 -0800 Linus Torvalds torvalds@linux-foundation.org wrote:
On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen raa.lkml@gmail.com wrote:
On Sat, Feb 19, 2011 at 13:11, Alex Riesen raa.lkml@gmail.com wrote:
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes it for you too? (Make sure you're at max brightness before rebooting.)
I'll try it now.
I can confirm that it does fix backlight in my case (Dell XPS 1330, LVDS panel, GM965/GL960).
Tested-by: Alex Riesen raa.lkml@gmail.com
Guys, should I apply this, or will I get it through somebody's pull?
I'm worried that removing combo mode will break some working setups, but if it's seen a lot of testing and is ok, then I'm fine with it, as it definitely simplifies things.
Hi,
I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM related patches, and my backlight issue is gone.
Regards, Tino
On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel tino.keitel@tikei.de wrote:
I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM related patches, and my backlight issue is gone.
I applied Indan's fix in -rc6 (commit 951f3512dba5), since it had several testers and seemed to simplify the code nicely too.
Linus
Hello,
On Wed, February 23, 2011 02:09, Linus Torvalds wrote:
On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel tino.keitel@tikei.de wrote:
I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM related patches, and my backlight issue is gone.
I applied Indan's fix in -rc6 (commit 951f3512dba5), since it had several testers and seemed to simplify the code nicely too.
Sadly, as so often in life, it's not correct. At this point I'm not sure if it's better to revert that patch and add a correct one, or to just fix it up. The end result is the same I suppose. I've also found more documentation, namely: ACPI_IGD_OpRegion_Spec.pdf, which has the ASLE stuff in intel_opregion.c, and VOL_1_graphics_core.pdf, which mentions LBPC (I was looking at 3 before). Apparently the undocumented stuff the old code did was correct. What I don't understand is how BIOS makers could know about those bits.
The good side is that that big warning in my patch description is invalid, something else was going on: The BIOS used LBPC to set and restore brightness, while the driver only used BLC_PWM_CTL after my patch.
All credits to Intel for making something simple as backlight control as stupid and complex as possible:
- It has two registers to control brightness, sometimes one is used, sometimes the other, sometimes both, and it's unknown what the BIOS uses, and it's undefined what registers are restored by the BIOS after reboot/resume.
- When using ACPI and ASLE, the kernel requests a brightness change via a standard ACPI method, which in turns lets the BIOS generate an ASLE interrupt, which is handled by the driver. The brightness to set is between 0 and 255, and the driver is supposed to store the current brightness in another register. That register stores the brightness in percentages, which is used by the BIOS to restore brightness. How it does that is undefined, so it can use either register. So the BIOS obviously knows how to change the brightness, and it's still seemed like a good idea to bother the driver with it. The ASLE interface is a mess.
All in all, after my patch, systems using ASLE and a BIOS storing the brightness in LBPC stopped working. The reason it works without ASLE is because the brightness is never changed by the driver, the backlight is only enabled or disabled.
I'd love to clean up the whole backlight mess, but it's too late in the RC cycle to do that.
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
From f6b8a45b9544072e6ddbb944a4c03a9ec8cbca3a Mon Sep 17 00:00:00 2001 From: Takashi
Iwai tiwai@suse.de Date: Mon, 21 Feb 2011 14:19:27 +0100 Subject: [PATCH] drm/i915: Fix calculation of backlight value in combined mode
The commit a95735569312f2ab0c80425e2cd1e5cb0b4e1870 drm/i915: Refactor panel backlight controls causes a regression for GM45 that is using the combined mode for controlling the backlight brightness. The commit introduced a wrong bit shift for computing the current backlight level.
Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=672946 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524
Signed-off-by: Takashi Iwai tiwai@suse.de Cc: stable@kernel.org --- drivers/gpu/drm/i915/intel_panel.c | 1 - 1 file changed, 1 deletion(-)
--- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -176,7 +176,6 @@ val &= ~1; pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); val *= lbpc; - val >>= 1; } }
Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?
Linus
On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?
LinusOn Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
For what it's worth, doing the above does prevent the regression for the ASLE case, but for me with gen 2 hardware the brightness isn't quite right after suspend/resume, while it is with my patch applied. So assuming that there are no gen 2 systems with ASLE out there, the best solution may be the remove the combination mode check for gen 2 hardware and leave it for gen >=4. Would be nice if Jesse or Chris could confirm that there are no gen 2 ASLE systems out there though.
I'm going camping, I'll send a patch next week.
Greetings,
Indan
On Fri, Mar 4, 2011 at 19:47, Linus Torvalds torvalds@linux-foundation.org wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?
I afraid mine is a different case, because backlight in this system works properly even with Indan's patch reverted.
Sorry for the late reply...
On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
Linus
Hello,
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?
LinusOn Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
I found another backlight bug:
When suspending intel_panel_disable_backlight() is never called, but intel_panel_enable_backlight() is called at resume. With the effect that if the brightness was ever changed after screen blanking, the wrong brightness gets restored.
This explains the weird behaviour I've seen. I didn't see it with combination mode, because then the brightness is always the same (zero or the maximum, the BIOS only uses LBPC on my system.) I'll send a patch in a moment.
Alternative for reverting the combination mode removal (I can also redo the patch against the revert and Takashi's patch, if that's preferred):
--
drm/i915: Do handle backlight combination mode specially
Add back the combination mode check, but with slightly cleaner code and the weirdness removed: No val >>= 1, but also no val &= ~1. The old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16. The other change is clearer calculations: Just check for zero level explicitly instead of avoiding the divide-by-zero.
Signed-off-by: Indan Zupancic indan@nul.nu
---
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..b05631a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,10 @@
#include "intel_drv.h"
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */ +#define BLM_COMBINATION_MODE (1 << 30) +#define BLM_LEGACY_MODE (1 << 16) + void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -110,6 +114,22 @@ done: dev_priv->pch_pf_size = (width << 16) | height; }
+/* + * What about gen 3? If there are no gen 3 systems with ASLE, + * then it doesn't matter, as we don't need to change the + * brightness. But then the gen 2 check can be removed too. + */ +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; @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) max >>= 17; } else { max >>= 16; + /* Ignore BLM_LEGACY_MODE bit */ if (INTEL_INFO(dev)->gen < 4) max &= ~1; } + if (is_backlight_combination_mode(dev)) + max *= 0xff; }
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max); @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1; + if (is_backlight_combination_mode(dev)){ + u8 lbpc; + + pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); + val *= lbpc; + } }
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val); @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level); + + if (level && is_backlight_combination_mode(dev)){ + u32 max = intel_panel_get_max_backlight(dev); + u8 lpbc; + + lpbc = level * 0xff / max; + level /= lpbc; + pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc); + } tmp = I915_READ(BLC_PWM_CTL); if (IS_PINEVIEW(dev)) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
drm/i915: Fix DPMS and suspend interaction for intel_panel.c
When suspending intel_panel_disable_backlight() is never called, but intel_panel_enable_backlight() is called at resume. With the effect that if the brightness was ever changed after screen blanking, the wrong brightness gets restored at resume time.
Nothing guarantees that those calls will be balanced, so having backlight_enabled makes no sense, as the real state can change without the panel code noticing. So keep things as stateless as possible.
Signed-off-by: Indan Zupancic indan@nul.nu
---
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 456f404..4a3d9ed 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -333,7 +333,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 */ @@ -1220,10 +1219,6 @@ void i915_debugfs_cleanup(struct drm_minor *minor); extern int i915_save_state(struct drm_device *dev); extern int i915_restore_state(struct drm_device *dev);
-/* i915_suspend.c */ -extern int i915_save_state(struct drm_device *dev); -extern int i915_restore_state(struct drm_device *dev); - /* intel_i2c.c */ extern int intel_setup_gmbus(struct drm_device *dev); extern void intel_teardown_gmbus(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 49fb54f..1b5a32d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5924,8 +5924,6 @@ static void intel_setup_outputs(struct drm_device *dev) encoder->base.possible_clones = intel_encoder_clones(dev, encoder->clone_mask); } - - intel_panel_setup_backlight(dev); }
static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2c43104..70e8b82 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -257,7 +257,6 @@ extern void intel_pch_panel_fitting(struct drm_device *dev, extern u32 intel_panel_get_max_backlight(struct drm_device *dev); extern u32 intel_panel_get_backlight(struct drm_device *dev); extern void intel_panel_set_backlight(struct drm_device *dev, u32 level); -extern void intel_panel_setup_backlight(struct drm_device *dev); extern void intel_panel_enable_backlight(struct drm_device *dev); extern void intel_panel_disable_backlight(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..b05631a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -217,12 +255,11 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level) void intel_panel_disable_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + u32 level = intel_panel_get_backlight(dev);
- if (dev_priv->backlight_enabled) { - dev_priv->backlight_level = intel_panel_get_backlight(dev); - dev_priv->backlight_enabled = false; - } - + if (level == 0) + return; + dev_priv->backlight_level = level; intel_panel_set_backlight(dev, 0); }
@@ -230,17 +267,9 @@ void intel_panel_enable_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;
+ if (intel_panel_get_backlight(dev)) + return; if (dev_priv->backlight_level == 0) dev_priv->backlight_level = intel_panel_get_max_backlight(dev); - intel_panel_set_backlight(dev, dev_priv->backlight_level); - dev_priv->backlight_enabled = true; -} - -void intel_panel_setup_backlight(struct drm_device *dev) -{ - struct drm_i915_private *dev_priv = dev->dev_private; - - dev_priv->backlight_level = intel_panel_get_backlight(dev); - dev_priv->backlight_enabled = dev_priv->backlight_level != 0; }
At Thu, 10 Mar 2011 06:50:09 +0100 (CET), Indan Zupancic wrote:
Hello,
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?
LinusOn Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
I found another backlight bug:
When suspending intel_panel_disable_backlight() is never called, but intel_panel_enable_backlight() is called at resume. With the effect that if the brightness was ever changed after screen blanking, the wrong brightness gets restored.
This explains the weird behaviour I've seen. I didn't see it with combination mode, because then the brightness is always the same (zero or the maximum, the BIOS only uses LBPC on my system.) I'll send a patch in a moment.
Alternative for reverting the combination mode removal (I can also redo the patch against the revert and Takashi's patch, if that's preferred):
--
drm/i915: Do handle backlight combination mode specially
Add back the combination mode check, but with slightly cleaner code and the weirdness removed: No val >>= 1, but also no val &= ~1. The old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16. The other change is clearer calculations: Just check for zero level explicitly instead of avoiding the divide-by-zero.
Signed-off-by: Indan Zupancic indan@nul.nu
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..b05631a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,10 @@
#include "intel_drv.h"
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */ +#define BLM_COMBINATION_MODE (1 << 30) +#define BLM_LEGACY_MODE (1 << 16)
void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -110,6 +114,22 @@ done: dev_priv->pch_pf_size = (width << 16) | height; }
+/*
- What about gen 3? If there are no gen 3 systems with ASLE,
- then it doesn't matter, as we don't need to change the
- brightness. But then the gen 2 check can be removed too.
- */
+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; @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) max >>= 17; } else { max >>= 16;
/* Ignore BLM_LEGACY_MODE bit */ if (INTEL_INFO(dev)->gen < 4) max &= ~1;}
if (is_backlight_combination_mode(dev))max *= 0xff;}
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1;
if (is_backlight_combination_mode(dev)){u8 lbpc;pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);val *= lbpc;}}
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level);
- if (level && is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);u8 lpbc;lpbc = level * 0xff / max;level /= lpbc;
Hmm, I don't think this calculation is correct. This would result in level of opregion over its limit. For example, assume the level max = 100, so total max = 25500. Passing level=150 here will be:
lbpc = 150 * 0xff / 25500 = 1.5 = 1 level = 150 / 1 = 150, which is over limit.
More worse, lbpc can be zero when level is below 100 in the case above...
thanks,
Takashi
At Thu, 10 Mar 2011 08:49:37 +0100, Takashi Iwai wrote:
At Thu, 10 Mar 2011 06:50:09 +0100 (CET), Indan Zupancic wrote:
Hello,
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?
LinusOn Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
I found another backlight bug:
When suspending intel_panel_disable_backlight() is never called, but intel_panel_enable_backlight() is called at resume. With the effect that if the brightness was ever changed after screen blanking, the wrong brightness gets restored.
This explains the weird behaviour I've seen. I didn't see it with combination mode, because then the brightness is always the same (zero or the maximum, the BIOS only uses LBPC on my system.) I'll send a patch in a moment.
Alternative for reverting the combination mode removal (I can also redo the patch against the revert and Takashi's patch, if that's preferred):
--
drm/i915: Do handle backlight combination mode specially
Add back the combination mode check, but with slightly cleaner code and the weirdness removed: No val >>= 1, but also no val &= ~1. The old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16. The other change is clearer calculations: Just check for zero level explicitly instead of avoiding the divide-by-zero.
Signed-off-by: Indan Zupancic indan@nul.nu
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..b05631a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,10 @@
#include "intel_drv.h"
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */ +#define BLM_COMBINATION_MODE (1 << 30) +#define BLM_LEGACY_MODE (1 << 16)
void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -110,6 +114,22 @@ done: dev_priv->pch_pf_size = (width << 16) | height; }
+/*
- What about gen 3? If there are no gen 3 systems with ASLE,
- then it doesn't matter, as we don't need to change the
- brightness. But then the gen 2 check can be removed too.
- */
+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; @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) max >>= 17; } else { max >>= 16;
/* Ignore BLM_LEGACY_MODE bit */ if (INTEL_INFO(dev)->gen < 4) max &= ~1;}
if (is_backlight_combination_mode(dev))max *= 0xff;}
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1;
if (is_backlight_combination_mode(dev)){u8 lbpc;pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);val *= lbpc;}}
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level);
- if (level && is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);u8 lpbc;lpbc = level * 0xff / max;level /= lpbc;Hmm, I don't think this calculation is correct. This would result in level of opregion over its limit. For example, assume the level max = 100, so total max = 25500. Passing level=150 here will be:
lbpc = 150 * 0xff / 25500 = 1.5 = 1 level = 150 / 1 = 150, which is over limit.
More worse, lbpc can be zero when level is below 100 in the case above...
That is, Chris' original code in that portion was correct:
if (is_backlight_combination_mode(dev)){ u32 max = intel_panel_get_max_backlight(dev); u8 lpbc;
lpbc = level * 0xfe / max + 1; level /= lpbc; pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc); }
This will fit within the right range. Though, changing like below will give a bit better calculation, closer to the real level.
lpbc = level * 0xfe / max + 1; level = (level + lpbc / 2) / lpbc;
Takashi
On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
At Thu, 10 Mar 2011 08:49:37 +0100, Takashi Iwai wrote:
At Thu, 10 Mar 2011 06:50:09 +0100 (CET), Indan Zupancic wrote:
Hello,
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?
LinusOn Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
I found another backlight bug:
When suspending intel_panel_disable_backlight() is never called, but intel_panel_enable_backlight() is called at resume. With the effect that if the brightness was ever changed after screen blanking, the wrong brightness gets restored.
This explains the weird behaviour I've seen. I didn't see it with combination mode, because then the brightness is always the same (zero or the maximum, the BIOS only uses LBPC on my system.) I'll send a patch in a moment.
Alternative for reverting the combination mode removal (I can also redo the patch against the revert and Takashi's patch, if that's preferred):
--
drm/i915: Do handle backlight combination mode specially
Add back the combination mode check, but with slightly cleaner code and the weirdness removed: No val >>= 1, but also no val &= ~1. The old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16. The other change is clearer calculations: Just check for zero level explicitly instead of avoiding the divide-by-zero.
Signed-off-by: Indan Zupancic indan@nul.nu
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..b05631a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,10 @@
#include "intel_drv.h"
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */ +#define BLM_COMBINATION_MODE (1 << 30) +#define BLM_LEGACY_MODE (1 << 16)
void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -110,6 +114,22 @@ done: dev_priv->pch_pf_size = (width << 16) | height; }
+/*
- What about gen 3? If there are no gen 3 systems with ASLE,
- then it doesn't matter, as we don't need to change the
- brightness. But then the gen 2 check can be removed too.
- */
+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; @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) max >>= 17; } else { max >>= 16;
/* Ignore BLM_LEGACY_MODE bit */ if (INTEL_INFO(dev)->gen < 4) max &= ~1;}
if (is_backlight_combination_mode(dev))max *= 0xff;}
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1;
if (is_backlight_combination_mode(dev)){u8 lbpc;pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);val *= lbpc;}}
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level);
- if (level && is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);u8 lpbc;lpbc = level * 0xff / max;level /= lpbc;Hmm, I don't think this calculation is correct. This would result in level of opregion over its limit. For example, assume the level max = 100, so total max = 25500. Passing level=150 here will be:
lbpc = 150 * 0xff / 25500 = 1.5 = 1 level = 150 / 1 = 150, which is over limit.
More worse, lbpc can be zero when level is below 100 in the case above...
That is, Chris' original code in that portion was correct:
if (is_backlight_combination_mode(dev)){ u32 max = intel_panel_get_max_backlight(dev); u8 lpbc;
lpbc = level * 0xfe / max + 1; level /= lpbc; pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc);}
This will fit within the right range. Though, changing like below will give a bit better calculation, closer to the real level.
lpbc = level * 0xfe / max + 1; level = (level + lpbc / 2) / lpbc;
Indeed, though I don't think it makes much difference in practise.
All in all it seems best to just revert my patch and apply your fix. Any "improvements" I may have are either buggy or can be added later.
Care to make a new patch with the above improvement added? You can add my acked-by, for what it's worth.
At this point I don't even dare removing that "obviously" bogus val &= ~1; I bet it's an undocumented bit having some obscure secret function on not well tested systems.
Greetings,
Indan
At Thu, 10 Mar 2011 11:06:28 +0100 (CET), Indan Zupancic wrote:
On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
At Thu, 10 Mar 2011 08:49:37 +0100, Takashi Iwai wrote:
At Thu, 10 Mar 2011 06:50:09 +0100 (CET), Indan Zupancic wrote:
Hello,
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?
LinusOn Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
I found another backlight bug:
When suspending intel_panel_disable_backlight() is never called, but intel_panel_enable_backlight() is called at resume. With the effect that if the brightness was ever changed after screen blanking, the wrong brightness gets restored.
This explains the weird behaviour I've seen. I didn't see it with combination mode, because then the brightness is always the same (zero or the maximum, the BIOS only uses LBPC on my system.) I'll send a patch in a moment.
Alternative for reverting the combination mode removal (I can also redo the patch against the revert and Takashi's patch, if that's preferred):
--
drm/i915: Do handle backlight combination mode specially
Add back the combination mode check, but with slightly cleaner code and the weirdness removed: No val >>= 1, but also no val &= ~1. The old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16. The other change is clearer calculations: Just check for zero level explicitly instead of avoiding the divide-by-zero.
Signed-off-by: Indan Zupancic indan@nul.nu
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..b05631a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,10 @@
#include "intel_drv.h"
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */ +#define BLM_COMBINATION_MODE (1 << 30) +#define BLM_LEGACY_MODE (1 << 16)
void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -110,6 +114,22 @@ done: dev_priv->pch_pf_size = (width << 16) | height; }
+/*
- What about gen 3? If there are no gen 3 systems with ASLE,
- then it doesn't matter, as we don't need to change the
- brightness. But then the gen 2 check can be removed too.
- */
+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; @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) max >>= 17; } else { max >>= 16;
/* Ignore BLM_LEGACY_MODE bit */ if (INTEL_INFO(dev)->gen < 4) max &= ~1;}
if (is_backlight_combination_mode(dev))max *= 0xff;}
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1;
if (is_backlight_combination_mode(dev)){u8 lbpc;pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);val *= lbpc;}}
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level);
- if (level && is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);u8 lpbc;lpbc = level * 0xff / max;level /= lpbc;Hmm, I don't think this calculation is correct. This would result in level of opregion over its limit. For example, assume the level max = 100, so total max = 25500. Passing level=150 here will be:
lbpc = 150 * 0xff / 25500 = 1.5 = 1 level = 150 / 1 = 150, which is over limit.
More worse, lbpc can be zero when level is below 100 in the case above...
That is, Chris' original code in that portion was correct:
if (is_backlight_combination_mode(dev)){ u32 max = intel_panel_get_max_backlight(dev); u8 lpbc;
lpbc = level * 0xfe / max + 1; level /= lpbc; pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc);}
This will fit within the right range. Though, changing like below will give a bit better calculation, closer to the real level.
lpbc = level * 0xfe / max + 1; level = (level + lpbc / 2) / lpbc;Indeed, though I don't think it makes much difference in practise.
All in all it seems best to just revert my patch and apply your fix. Any "improvements" I may have are either buggy or can be added later.
Agreed. We should take a safer way.
Care to make a new patch with the above improvement added? You can add my acked-by, for what it's worth.
OK, I'm going to send it now.
At this point I don't even dare removing that "obviously" bogus val &= ~1; I bet it's an undocumented bit having some obscure secret function on not well tested systems.
Yes, I also left it...
Takashi
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.
In addition to the revert, this patch includes a fix for the original issue (weird backlight levels) by removing the wrong bit shift for computing the current backlight level. Also, including typo fixes (lpbc -> lbpc).
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524 Acked-by: Indan Zupancic indan@nul.nu Cc: stable@kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/i915/i915_reg.h | 10 ++++++++++ drivers/gpu/drm/i915/intel_panel.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 3e6f486..2abe240 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1553,7 +1553,17 @@
/* Backlight control */ #define BLC_PWM_CTL 0x61254 +#define BACKLIGHT_MODULATION_FREQ_SHIFT (17) #define BLC_PWM_CTL2 0x61250 /* 965+ only */ +#define BLM_COMBINATION_MODE (1 << 30) +/* + * This is the most significant 15 bits of the number of backlight cycles in a + * complete cycle of the modulated backlight control. + * + * The actual value is this field multiplied by two. + */ +#define BACKLIGHT_MODULATION_FREQ_MASK (0x7fff << 17) +#define BLM_LEGACY_MODE (1 << 16) /* * This is the number of cycles out of the backlight modulation cycle for which * the backlight is on. diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..f8f86e5 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,8 @@
#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) @@ -110,6 +112,19 @@ 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; @@ -166,6 +181,9 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) if (INTEL_INFO(dev)->gen < 4) max &= ~1; } + + if (is_backlight_combination_mode(dev)) + max *= 0xff; }
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max); @@ -183,6 +201,14 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1; + + if (is_backlight_combination_mode(dev)){ + u8 lbpc; + + val &= ~1; + pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); + val *= lbpc; + } }
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val); @@ -205,6 +231,16 @@ void intel_panel_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 (IS_PINEVIEW(dev)) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
On Thu, 10 Mar 2011 14:02:12 +0100, Takashi Iwai tiwai@suse.de wrote:
This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
drm/i915: Do not handle backlight combination mode speciallysince this commit introduced other regressions due to untouched LBPC register, e.g. the backlight dimmed after resume.
In addition to the revert, this patch includes a fix for the original issue (weird backlight levels) by removing the wrong bit shift for computing the current backlight level. Also, including typo fixes (lpbc -> lbpc).
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524 Acked-by: Indan Zupancic indan@nul.nu Cc: stable@kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de
This looks good, and ensures that LBPC ranges from 0 - 0xfe as required.
The one thing we want is a comment that explains
val &= ~1;
The reason for this is that some ancient platforms wire this bit to be "go to max backlight", or at least that's why this was in the 2D driver from which this code was derived.
Reviewed-by: Keith Packard keithp@keithp.com Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
On Thu, March 10, 2011 20:36, Keith Packard wrote:
On Thu, 10 Mar 2011 14:02:12 +0100, Takashi Iwai tiwai@suse.de wrote:
val &= ~1;The reason for this is that some ancient platforms wire this bit to be "go to max backlight", or at least that's why this was in the 2D driver from which this code was derived.
If that is the case, then shouldn't it be at the end, after val *= lbpc? I know it doesn't make much difference, as multiplying with an even number always gives an even result, but it would make the intention clearer and give a more precise result.
What about gen 3? Does it support combination mode too?
If you can confirm that there are no gen 2 systems with ASLE support, we can remove the gen 2 check and only touch the PWM register, as the ASLE code is the only one that changes the brightness. The panel code only saves and restores it, and LBPC is saved and restored by the BIOS already. Then those weird gen 2 quirks can be removed.
(Something for 2.6.39 perhaps.)
It's sad that something as simple as backlight control needs so much code.
Greetings,
Indan
Hi,
Some nitpicks below. I know you're just restoring the original code, but if we improve it we can as well do it now.
On Thu, March 10, 2011 14:02, Takashi Iwai wrote:
This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
drm/i915: Do not handle backlight combination mode speciallysince 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.
In addition to the revert, this patch includes a fix for the original issue (weird backlight levels) by removing the wrong bit shift for computing the current backlight level. Also, including typo fixes (lpbc -> lbpc).
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524 Acked-by: Indan Zupancic indan@nul.nu Cc: stable@kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/i915/i915_reg.h | 10 ++++++++++ drivers/gpu/drm/i915/intel_panel.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 3e6f486..2abe240 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1553,7 +1553,17 @@
/* Backlight control */ #define BLC_PWM_CTL 0x61254 +#define BACKLIGHT_MODULATION_FREQ_SHIFT (17)
This one isn't used anywhere.
#define BLC_PWM_CTL2 0x61250 /* 965+ only */ +#define BLM_COMBINATION_MODE (1 << 30) +/*
- This is the most significant 15 bits of the number of backlight cycles in a
- complete cycle of the modulated backlight control.
- The actual value is this field multiplied by two.
- */
+#define BACKLIGHT_MODULATION_FREQ_MASK (0x7fff << 17)
This one isn't used and the comment is misleading, I think.
+#define BLM_LEGACY_MODE (1 << 16) /*
- This is the number of cycles out of the backlight modulation cycle for which
- the backlight is on.
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..f8f86e5 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,8 @@
#include "intel_drv.h"
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
I'd either put all the defines in i915_reg.h, or have all combination mode specific defines here. Though I guess LBPC is an odd one.
void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -110,6 +112,19 @@ 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; @@ -166,6 +181,9 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) if (INTEL_INFO(dev)->gen < 4) max &= ~1;
I'd add a comment that this is to clear the BLM_LEGACY_MODE bit.
}
if (is_backlight_combination_mode(dev))max *= 0xff;}
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -183,6 +201,14 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1;
if (is_backlight_combination_mode(dev)){u8 lbpc;val &= ~1;pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);val *= lbpc;}}
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +231,16 @@ void intel_panel_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 (IS_PINEVIEW(dev)) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
After this patch, combined with my new patch
"drm/i915: Fix DPMS and suspend interaction for intel_panel.c"
all known backlight problems should be fixed.
Greetings,
Indan
On Thu, Mar 10, 2011 at 5:23 PM, Indan Zupancic indan@nul.nu wrote:
After this patch, combined with my new patch
"drm/i915: Fix DPMS and suspend interaction for intel_panel.c"
all known backlight problems should be fixed.
Btw, can you repost that one as a new email (and cc keithp too)? I think it got hidden in the other thread by different subject line.
Jesse, did you take a look at that one? It was in the thread with a subject like "[PATCH] fix backlight brightness on intel LVDS panel after reopening lid".
Search for
When suspending intel_panel_disable_backlight() is never called, but intel_panel_enable_backlight() is called at resume. With the effect that if the brightness was ever changed after screen blanking, the wrong brightness gets restored at resume time.
in your mailbox.
Linus
At Fri, 11 Mar 2011 02:23:08 +0100 (CET), Indan Zupancic wrote:
Hi,
Some nitpicks below. I know you're just restoring the original code, but if we improve it we can as well do it now.
Well, Linus already merged my patch quickly. So, we can improve the readability as an additional patch, but I think it's likely a 2.6.39 material.
All you comments below look reasonable. Could you send a new patch?
thanks,
Takashi
On Thu, March 10, 2011 14:02, Takashi Iwai wrote:
This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
drm/i915: Do not handle backlight combination mode speciallysince 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.
In addition to the revert, this patch includes a fix for the original issue (weird backlight levels) by removing the wrong bit shift for computing the current backlight level. Also, including typo fixes (lpbc -> lbpc).
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524 Acked-by: Indan Zupancic indan@nul.nu Cc: stable@kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/i915/i915_reg.h | 10 ++++++++++ drivers/gpu/drm/i915/intel_panel.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 3e6f486..2abe240 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1553,7 +1553,17 @@
/* Backlight control */ #define BLC_PWM_CTL 0x61254 +#define BACKLIGHT_MODULATION_FREQ_SHIFT (17)
This one isn't used anywhere.
#define BLC_PWM_CTL2 0x61250 /* 965+ only */ +#define BLM_COMBINATION_MODE (1 << 30) +/*
- This is the most significant 15 bits of the number of backlight cycles in a
- complete cycle of the modulated backlight control.
- The actual value is this field multiplied by two.
- */
+#define BACKLIGHT_MODULATION_FREQ_MASK (0x7fff << 17)
This one isn't used and the comment is misleading, I think.
+#define BLM_LEGACY_MODE (1 << 16) /*
- This is the number of cycles out of the backlight modulation cycle for which
- the backlight is on.
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..f8f86e5 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,8 @@
#include "intel_drv.h"
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
I'd either put all the defines in i915_reg.h, or have all combination mode specific defines here. Though I guess LBPC is an odd one.
void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -110,6 +112,19 @@ 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; @@ -166,6 +181,9 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) if (INTEL_INFO(dev)->gen < 4) max &= ~1;
I'd add a comment that this is to clear the BLM_LEGACY_MODE bit.
}
if (is_backlight_combination_mode(dev))max *= 0xff;}
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -183,6 +201,14 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1;
if (is_backlight_combination_mode(dev)){u8 lbpc;val &= ~1;pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);val *= lbpc;}}
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +231,16 @@ void intel_panel_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 (IS_PINEVIEW(dev)) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
After this patch, combined with my new patch
"drm/i915: Fix DPMS and suspend interaction for intel_panel.c"
all known backlight problems should be fixed.
Greetings,
Indan
On Fri, March 11, 2011 08:26, Takashi Iwai wrote:
At Fri, 11 Mar 2011 02:23:08 +0100 (CET), Indan Zupancic wrote:
Hi,
Some nitpicks below. I know you're just restoring the original code, but if we improve it we can as well do it now.
Well, Linus already merged my patch quickly. So, we can improve the readability as an additional patch, but I think it's likely a 2.6.39 material.
All you comments below look reasonable. Could you send a new patch?
Well, my main concern was the slightly incorrect commit message, but oh well, too late for that now. I'll probably send a patch after 2.6.38 is released.
Greetings,
Indan
On Fri, 11 Mar 2011 02:23:08 +0100 (CET), "Indan Zupancic" indan@nul.nu wrote:
Some nitpicks below. I know you're just restoring the original code, but if we improve it we can as well do it now.
Let's pend these changes until after 2.6.38; the backlight code is fragile enough without trying to 'clean it up' at this point in the release cycle.
On Thu, March 10, 2011 08:49, Takashi Iwai wrote:
At Thu, 10 Mar 2011 06:50:09 +0100 (CET), Indan Zupancic wrote:
Hello,
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?
LinusOn Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
I found another backlight bug:
When suspending intel_panel_disable_backlight() is never called, but intel_panel_enable_backlight() is called at resume. With the effect that if the brightness was ever changed after screen blanking, the wrong brightness gets restored.
This explains the weird behaviour I've seen. I didn't see it with combination mode, because then the brightness is always the same (zero or the maximum, the BIOS only uses LBPC on my system.) I'll send a patch in a moment.
Alternative for reverting the combination mode removal (I can also redo the patch against the revert and Takashi's patch, if that's preferred):
--
drm/i915: Do handle backlight combination mode specially
Add back the combination mode check, but with slightly cleaner code and the weirdness removed: No val >>= 1, but also no val &= ~1. The old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16. The other change is clearer calculations: Just check for zero level explicitly instead of avoiding the divide-by-zero.
Signed-off-by: Indan Zupancic indan@nul.nu
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..b05631a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,10 @@
#include "intel_drv.h"
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */ +#define BLM_COMBINATION_MODE (1 << 30) +#define BLM_LEGACY_MODE (1 << 16)
void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -110,6 +114,22 @@ done: dev_priv->pch_pf_size = (width << 16) | height; }
+/*
- What about gen 3? If there are no gen 3 systems with ASLE,
- then it doesn't matter, as we don't need to change the
- brightness. But then the gen 2 check can be removed too.
- */
+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; @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) max >>= 17; } else { max >>= 16;
/* Ignore BLM_LEGACY_MODE bit */ if (INTEL_INFO(dev)->gen < 4) max &= ~1;}
if (is_backlight_combination_mode(dev))max *= 0xff;}
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1;
if (is_backlight_combination_mode(dev)){u8 lbpc;pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);val *= lbpc;}}
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level);
- if (level && is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);u8 lpbc;lpbc = level * 0xff / max;level /= lpbc;Hmm, I don't think this calculation is correct. This would result in level of opregion over its limit. For example, assume the level max = 100, so total max = 25500. Passing level=150 here will be:
lbpc = 150 * 0xff / 25500 = 1.5 = 1 level = 150 / 1 = 150, which is over limit.
More worse, lbpc can be zero when level is below 100 in the case above...
Yes, you're right. It seems that any simplification I try to do creates a new bug.
Do you have any bright idea why the old code did val &= ~1; too? It seems obvious it's related to val >>= 1, but...
Thanks,
Indan
At Thu, 10 Mar 2011 09:45:18 +0100 (CET), Indan Zupancic wrote:
On Thu, March 10, 2011 08:49, Takashi Iwai wrote:
At Thu, 10 Mar 2011 06:50:09 +0100 (CET), Indan Zupancic wrote:
Hello,
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?
LinusOn Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:
So please revert my patch and apply Takashi Iwai's, which fixes the most immediate bug without changing anything else. This should go in stable too.
I found another backlight bug:
When suspending intel_panel_disable_backlight() is never called, but intel_panel_enable_backlight() is called at resume. With the effect that if the brightness was ever changed after screen blanking, the wrong brightness gets restored.
This explains the weird behaviour I've seen. I didn't see it with combination mode, because then the brightness is always the same (zero or the maximum, the BIOS only uses LBPC on my system.) I'll send a patch in a moment.
Alternative for reverting the combination mode removal (I can also redo the patch against the revert and Takashi's patch, if that's preferred):
--
drm/i915: Do handle backlight combination mode specially
Add back the combination mode check, but with slightly cleaner code and the weirdness removed: No val >>= 1, but also no val &= ~1. The old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16. The other change is clearer calculations: Just check for zero level explicitly instead of avoiding the divide-by-zero.
Signed-off-by: Indan Zupancic indan@nul.nu
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..b05631a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,10 @@
#include "intel_drv.h"
+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */ +#define BLM_COMBINATION_MODE (1 << 30) +#define BLM_LEGACY_MODE (1 << 16)
void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -110,6 +114,22 @@ done: dev_priv->pch_pf_size = (width << 16) | height; }
+/*
- What about gen 3? If there are no gen 3 systems with ASLE,
- then it doesn't matter, as we don't need to change the
- brightness. But then the gen 2 check can be removed too.
- */
+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; @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) max >>= 17; } else { max >>= 16;
/* Ignore BLM_LEGACY_MODE bit */ if (INTEL_INFO(dev)->gen < 4) max &= ~1;}
if (is_backlight_combination_mode(dev))max *= 0xff;}
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
@@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1;
if (is_backlight_combination_mode(dev)){u8 lbpc;pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);val *= lbpc;}}
DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);
@@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level);
- if (level && is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);u8 lpbc;lpbc = level * 0xff / max;level /= lpbc;Hmm, I don't think this calculation is correct. This would result in level of opregion over its limit. For example, assume the level max = 100, so total max = 25500. Passing level=150 here will be:
lbpc = 150 * 0xff / 25500 = 1.5 = 1 level = 150 / 1 = 150, which is over limit.
More worse, lbpc can be zero when level is below 100 in the case above...
Yes, you're right. It seems that any simplification I try to do creates a new bug.
Do you have any bright idea why the old code did val &= ~1; too? It seems obvious it's related to val >>= 1, but...
I guess it's for the case GEN < 4. But, no certain idea.
In my patch, I left it since this is relatively harmless, even if it's not correct.
Takashi
On Tue, February 22, 2011 22:04, Jesse Barnes wrote:
On Sat, 19 Feb 2011 15:07:49 -0800 Linus Torvalds torvalds@linux-foundation.org wrote:
On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen raa.lkml@gmail.com wrote:
On Sat, Feb 19, 2011 at 13:11, Alex Riesen raa.lkml@gmail.com wrote:
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447
fixes
it for you too? (Make sure you're at max brightness before rebooting.)
I'll try it now.
I can confirm that it does fix backlight in my case (Dell XPS 1330, LVDS panel, GM965/GL960).
Tested-by: Alex Riesen raa.lkml@gmail.com
Guys, should I apply this, or will I get it through somebody's pull?
I'm worried that removing combo mode will break some working setups, but if it's seen a lot of testing and is ok, then I'm fine with it, as it definitely simplifies things.
This all seems new code added in 2.6.37, it wasn't there before. The working setups stopped working when that code was added. The only reason it may work for some gen 2 and gen 3 hardware is because of a random value of the max brightness (bit 16). The buggy code seems to be written in a haste without any testing done. It's so off from the official documentation that I suspect that the windows driver was used as reference, but its code was misinterpreted.
I grepped the userspace driver source, and LBPC is defined there for 810, but not used anywhere either.
This patch should be added to stable kernel 2.6.37.2, because it messes up the LPBC register, which some laptops store between boots.
Quoting https://bugzilla.kernel.org/show_bug.cgi?id=23472#c57
- Checking bit 16 in BLC_PWM_CTL is wrong, it has no special meaning.
- If LBPC == 0xff, it should be ignored and it's not in combination mode. (This is for gen 3).
- Gen 2 documentation doesn't mention LBPC or combination mode at all. Gen 3 does, but doesn't tell what the register value is or how to use it, it just mentions it.
- The calculations are rubbish, resulting in bogus LBPC values, and depending on how lucky you are, it writes different values for the registers after a restore.
All this code is new and causes problems, while everything worked before just fine, when the driver didn't do anything special.
So it seems a bit like voodoo programming, because nothing the driver did followed the official Intel documentation.
Now, there may be real reasons for some of the code, but I propose we add them one at a time when people show up with problems without the weird code added. That way the reason for any weirdness is also documented.
Greetings,
Indan
On Wed, Feb 16, 2011 at 20:26:58 +0100, Alex Riesen wrote:
Signed-off-by: Alex Riesen raa.lkml@gmail.com
Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
Most of the changes are pretty spread out and small, with drivers/gpu (radeon and i915) somewhat standing out from the pack. ...
The backlight level on this Dell XPS M1330 reduces every time I reopen the lid, and BIOS does not seem to know anything about that (the keyboard shortcuts to set backlight brightness cause it to jump to the level next to the one set before closing the lid).
Hi,
with kernel 2.6.37, the display brightness of my ThinkPad X61s was always reduced after lid open, resume from suspend etc. With this patch on top of 2.6.38-rc5, the problem is gone. Thanks.
Regards, Tino
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
dri-devel@lists.freedesktop.org
-
Alex Riesen -
Indan Zupancic -
Jesse Barnes -
Keith Packard -
Linus Torvalds -
Peter Stuge -
Takashi Iwai -
Tino Keitel -
Tino Keitel