drm/i915: backlight fixes and cleanup
Subsystems (3)
cover drm/i915: backlight fixes and cleanup
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(-)
Signature · 1 line
1.7.9.5
PATCH 1/4 drm/i915: save/restore the legacy backlight control
From: Daniel Vetter <[email protected]>
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.
--- 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(-)
drivers/gpu/drm/i915/i915_drv.h +1 -0
@@ -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;
drivers/gpu/drm/i915/i915_reg.h +3 -0
@@ -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)
drivers/gpu/drm/i915/i915_suspend.c +8 -0
@@ -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 */
drivers/gpu/drm/i915/intel_panel.c +0 -2
@@ -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)
Signature · 1 line
1.7.9.5
Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control
Quoted 5 lines · Jani Nikula
From: Daniel Vetter <[email protected]>
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
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control
Quoted 11 lines · Chris Wilson
Quoted 5 lines · Jani Nikula
From: Daniel Vetter <[email protected]>
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control
In reply to Jani NikulaHello,
Quoted 46 lines · Jani Nikula
From: Daniel Vetter <[email protected]>
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 <[email protected]>Signed-off-by Jani Nikula <[email protected]>--- 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(-)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -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;drivers/gpu/drm/i915/i915_reg.h +3
-0@@ -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)drivers/gpu/drm/i915/i915_suspend.c +0
-0
drivers/gpu/drm/i915/i915_suspend.c static void i915_save_display(struct drm_device *dev) · line 645
Quoted 7 lines
drivers/gpu/drm/i915/i915_suspend.c +4
-0@@ -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
Quoted 59 lines · Indan Zupancic
Hello,
Quoted 46 lines · Jani Nikula
From: Daniel Vetter <[email protected]>
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 <[email protected]>Signed-off-by Jani Nikula <[email protected]>--- 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(-)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -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;drivers/gpu/drm/i915/i915_reg.h +3
-0@@ -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)drivers/gpu/drm/i915/i915_suspend.c +0
-0drivers/gpu/drm/i915/i915_suspend.c static void i915_save_display(struct drm_device *dev) · line 645
Quoted 7 lines
drivers/gpu/drm/i915/i915_suspend.c +4
-0@@ -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?
Quoted 2 lines
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control
Quoted 13 lines · Daniel Vetter
Quoted 12 lines · Indan Zupancic
Hello,
Quoted 9 lines · Jani Nikula
From: Daniel Vetter <[email protected]>
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 <[email protected]>Signed-off-by Jani Nikula <[email protected]>---
[...]
Quoted 5 lines
Quoted 2 lines
It seems weird that LBPC wouldn't be restored during resume by some BIOSes, is this really necessary?
ba3820ade317ee36e496b9b40d2ec3987dd4aef0claims 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 1/4] drm/i915: save/restore the legacy backlight control
Quoted 21 lines · Indan Zupancic
Quoted 13 lines · Daniel Vetter
Quoted 12 lines · Indan Zupancic
Hello,
Quoted 9 lines · Jani Nikula
From: Daniel Vetter <[email protected]>
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 <[email protected]>Signed-off-by Jani Nikula <[email protected]>---
[...]
Quoted 5 lines
Quoted 2 lines
It seems weird that LBPC wouldn't be restored during resume by some BIOSes, is this really necessary?
ba3820ade317ee36e496b9b40d2ec3987dd4aef0claims so. But that commit managed to put too many things into the same thing unfortunately.
Quoted 6 lines
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.
Quoted 3 lines
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control
Quoted 10 lines · Daniel Vetter
Quoted 3 lines · Indan Zupancic
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
Quoted 33 lines · Indan Zupancic
Quoted 10 lines · Daniel Vetter
Quoted 3 lines · Indan Zupancic
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.
Quoted 4 lines
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).
Quoted 2 lines
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
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
PATCH 2/4 drm/i915: remove combination mode for backlight control, again
In reply to Jani NikulaThe combination/legacy mode was first removed in
commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
Author: Indan Zupancic <[email protected]> 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 <[email protected]> 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
--- drivers/gpu/drm/i915/intel_panel.c | 32 -------------------------------- 1 file changed, 32 deletions(-)
drivers/gpu/drm/i915/intel_panel.c +0 -32
@@ -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 levelif (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 ;
Signature · 1 line
1.7.9.5
Re: [PATCH 2/4] drm/i915: remove combination mode for backlight control, again
Hello,
You seem to be making exactly the same mistake I made.
Quoted 10 lines · Jani Nikula
The combination/legacy mode was first removed in
commit
951f3512dba5bd44cda3e5ee22b4b522e4bb09fbAuthor: Indan Zupancic <[email protected]> Date: Thu Feb 17 02:41:49 2011 +0100drm/i915: Do not handle backlight combination mode specially
which was subsequently reverted due to regression in
Quoted 8 lines
commit
ba3820ade317ee36e496b9b40d2ec3987dd4aef0Author: Takashi Iwai <[email protected]> Date: Thu Mar 10 14:02:12 2011 +0100drm/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:
Quoted 6 lines · Takashi Iwai
This reverts commit
951f3512dba5bd44cda3e5ee22b4b522e4bb09fbdrm/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
Quoted 4 lines
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.
Quoted 73 lines
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53153
CC Indan Zupancic <[email protected]>CC Takashi Iwai <[email protected]>Signed-off-by Jani Nikula <[email protected]>--- drivers/gpu/drm/i915/intel_panel.c | 32 -------------------------------- 1 file changed, 32 deletions(-)drivers/gpu/drm/i915/intel_panel.c +0
-32@@ -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 )
Quoted 5 lines
level <<= 1;
--
1.7.9.5
Greetings,
Indan
Re: [PATCH 2/4] drm/i915: remove combination mode for backlight control, again
Quoted 41 lines · Indan Zupancic
Hello,
You seem to be making exactly the same mistake I made.
Quoted 10 lines · Jani Nikula
The combination/legacy mode was first removed in
commit
951f3512dba5bd44cda3e5ee22b4b522e4bb09fbAuthor: Indan Zupancic <[email protected]> Date: Thu Feb 17 02:41:49 2011 +0100drm/i915: Do not handle backlight combination mode specially
which was subsequently reverted due to regression in
Quoted 8 lines
commit
ba3820ade317ee36e496b9b40d2ec3987dd4aef0Author: Takashi Iwai <[email protected]> Date: Thu Mar 10 14:02:12 2011 +0100drm/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:
Quoted 6 lines · Takashi Iwai
This reverts commit
951f3512dba5bd44cda3e5ee22b4b522e4bb09fbdrm/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.
Quoted 8 lines
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."
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.
Quoted 14 lines
Quoted 4 lines
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.
Quoted 5 lines
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
Quoted 85 lines
Quoted 74 lines
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53153
CC Indan Zupancic <[email protected]>CC Takashi Iwai <[email protected]>Signed-off-by Jani Nikula <[email protected]>--- drivers/gpu/drm/i915/intel_panel.c | 32 -------------------------------- 1 file changed, 32 deletions(-)drivers/gpu/drm/i915/intel_panel.c +0
-32@@ -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 )Quoted 5 lines
level <<= 1;
--
1.7.9.5Greetings,
Indan
Signature · 3 lines
Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48
Re: [PATCH 2/4] drm/i915: remove combination mode for backlight control, again
Quoted 7 lines · Daniel Vetter
Quoted 5 lines · Indan Zupancic
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.
Quoted 6 lines
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_backlightfunctions. Would it be correct in your opinion to simply ditch the call tointel_panel_actually_set_backlightfor gen4+ unconditionally? Same forintel_panel_disable_backlightobviously?
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
Quoted 26 lines · On Thu, 30 Aug 2012 11:29:11 +0200 "Indan Zupancic" <[email protected]> wrote:
Quoted 7 lines · Daniel Vetter
Quoted 5 lines · Indan Zupancic
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.Quoted 6 lines
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_backlightfunctions. Would it be correct in your opinion to simply ditch the call tointel_panel_actually_set_backlightfor gen4+ unconditionally? Same forintel_panel_disable_backlightobviously?Yes, that seems the whole point of having the PWM disable bit.
I would also ditch the
backlight_enabledstate 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.cafter all.
Quoted 5 lines
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...
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 3/4 drm/i915: remove brightness inversion quirk for acer aspire 5734z
In reply to Jani NikulaThis reverts
commit 5a15ab5b93e4a3ebcd4fa6c76cf646a45e9cf806
Author: Carsten Emde <[email protected]> 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.
--- drivers/gpu/drm/i915/intel_display.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/gpu/drm/i915/intel_display.c +1 -5
@@ -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)
Signature · 1 line
1.7.9.5
PATCH 4/4 drm/i915: remove module parameter and quirk for inverting brightness
In reply to Jani NikulaThis is effectively a revert of
commit 4dca20efb1a9c2efefc28ad2867e5d6c3f5e1955
Author: Carsten Emde <[email protected]> Date: Thu Mar 15 15:56:26 2012 +0100
drm/i915: panel: invert brightness via quirk
and
commit 7bd90909bbf9ce7c40e1da3d72b97b93839c188a
Author: Carsten Emde <[email protected]> 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.
--- 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(-)
Documentation/kernel-parameters.txt +0 -14
@@ -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>]]]
drivers/gpu/drm/i915/i915_drv.h +0 -1
@@ -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 ;
drivers/gpu/drm/i915/intel_display.c +0 -10
@@ -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;
drivers/gpu/drm/i915/intel_panel.c +0 -24
@@ -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 [email protected], 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 levelu32 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);
Signature · 1 line
1.7.9.5