Bug 23472

This is probably a dupe of bug 22722.

Nack. The backlight problem persists after applying the one-line patch from https://bugzilla.kernel.org/attachment.cgi?id=37442 - with one apparent improvement: backlight dims upon gdm login screen, stays dim after login, but when I make it bright by pressing the brighness keys (which always worked for me), and then I lock the screen and unlock it again, it stays bright. Without that patch, IIRC, it dimmed also after lock screen / unlock.

I have a similar issue. On startup everything is well. The trouble begins when the screen is blanked. After this the back light returns in its lowest brightness level, and more frustrating, does not respond to the 'fn, arrow-up' keys anymore.  

(dell latitude D430, 00:02.1 Display controller: Intel Corporation Mobile 945GM/GMS/GME, 943/940GML Express Integrated Graphics Controller (rev 03))

The problem is that gnome save and restore isn't working for backlight brightness.

Patrick, please try again except this time shutdown from a good kernel and restart in a good kernel.  (In the last test, you shutdown on your bad kernel so the save part of the save -> restore transition was broken).

The reason it worked for the lock and unlock is that that is a patched kernel to patched kernel save and restore.

(In reply to comment #4)

> Patrick, please try again except this time shutdown from a good kernel and
> restart in a good kernel.  (In the last test, you shutdown on your bad kernel
> so the save part of the save -> restore transition was broken).

I'm sorry to say, but that did not help either.

I went through the following motions:

- booted back into 2.6.36. Logged in. Backlight bright. Shut down.
- booted again into 2.6.36. Logged in. Backlight bright. Shut down.
- booted into the patched 2.6.37-rc2. Logged in. Backlight dim. Made it bright again by pushing the appropriate keys (THAT works all the time, for me!). Shut down.
- booted again into the patched 2.6.37-rc2. Logged in. Backlight dim. No luck.
- update this bug tracker entry...

Is there anything (script, config, logfile, debugging switch) I could look at on my system to look into this further? I dont't know all that desktop backend stuff from the inside, but I can follow orders :)

I believe gnome uses this command to set the backlight:

bus-send --print-reply --system --dest=org.freedesktop.Hal \
    /org/freedesktop/Hal/devices/computer_backlight \
    org.freedesktop.Hal.Device.LaptopPanel.SetBrightness int32:<SETTING>

Where <SETTING> is the brightness level.

To view the current setting use:
bus-send --print-reply --system --dest=org.freedesktop.Hal \
    /org/freedesktop/Hal/devices/computer_backlight \
    org.freedesktop.Hal.Device.LaptopPanel.GetBrightness

(I have tested this on Ubuntu 10.4)

You can do this in a more simple way by going to:
/sys/devices/platform/eeepc/backlight/eeepc/

Except if you aren't using an eeepc then explore under your /sys/devices/platform until you find the backlight settings.

cd /sys/devices/platform/eeepc/backlight/eeepc/
cat brightness
echo 4 > brightness
cat max_brightness > brightness

Thanks Dan for the directions.

On my system I have dbus-send, which works with the command line you gave.

The sysfs stuff is under /sys/devices/platform/dell-laptop/brightness/dell-laptop
where I have "max_backlight" (7), "actual_backlight", and writable "backlight" pseudofiles.

writing to the backlight file, has immediately visible, expected outcome.

The dbus-send commands, both reading and writing, are completely consistent with fondling the sysfs knobs by hand.

When I experience the reported backlight dimming, both the sysfs actual_brightness file, and the dbus-send reading, STILL SAY 7!

I ran the sysfs / dbus-send stuff from a text console, first without being logged in (gdm login screen on vt7). Whenever I switch to the gdm screen, the backlight dims. Switching back to the text console keeps it dim. The "actual_brightness" file stays at 7. Writing 7 to "brightness" then, has no effect, probably because the kernel thinks nothing changes. Writing 6 to "brighness" makes the display brighter, then writing 7 makes it maximally bright. Switching back to the gdm login screen again DIMS. This is absolutely repeatable over several tries.

On the other hand, once I log in to X, and switch back and forth between the running X session and the text console, the visible brighness stays high (after fondling "brighness" on the text console once, or using the laptop keys).

So, the dimming-that-is-not-visible-in-sysfs-and-dbus, seems to happen when switching to the gdm login screen.

Hope this helps understanding the problem further.

(In reply to comment #7)
> The sysfs stuff is under
> /sys/devices/platform/dell-laptop/brightness/dell-laptop

Sorry, wrong path, due to typing from mind instead of cut+paste.
It is /sys/devices/platform/dell-laptop/backlight/dell_backlight

As a result of the gained understanding, I now have a personal workaround for the problem, in the form of a script /etc/X11/xinit/xinitrc.d/rise_and_shine

----------------------------------------------------------------------------
#! /bin/sh
(
	sleep 4
	dbus-send --print-reply --system --dest=org.freedesktop.Hal \
	    /org/freedesktop/Hal/devices/computer_backlight \
	    org.freedesktop.Hal.Device.LaptopPanel.SetBrightness int32:6
	dbus-send --print-reply --system --dest=org.freedesktop.Hal \
	    /org/freedesktop/Hal/devices/computer_backlight \
	    org.freedesktop.Hal.Device.LaptopPanel.SetBrightness int32:7
) </dev/null >/dev/null 2>&1 &
----------------------------------------------------------------------------

The sleep is neccessary for timing; without it I see a short brightening flicker after entering my password on the login screen, but when the desktop appears it is dim again. So, it is not only switching TO the gdm login screen that makes the display dim, but also switching away from it (or switching TO the desktop X server).

So to summarize:  

Switching to GDM makes the screen dim and it doesn't affect 
/sys/devices/platform/dell-laptop/backlight/dell_backlight/actual_brightness which still says 7.

Does it affect dell_backlight/brightness?

Writing 7 to dell_backlight/brightness doesn't do anything.

Are you using dbus or the echo 7 > brightness to test this?  Please don't use dbus because if it's a dbus problem that goes to bugzilla.gnome.org.

(In reply to comment #9)
> So to summarize:  
> 
> Switching to GDM makes the screen dim and it doesn't affect 
> /sys/devices/platform/dell-laptop/backlight/dell_backlight/actual_brightness
> which still says 7.

Right.

> Does it affect dell_backlight/brightness?

No, that stays at 7, too.

> Writing 7 to dell_backlight/brightness doesn't do anything.

Yes, probably because actual_brighness still reads 7.

> Are you using dbus or the echo 7 > brightness to test this?  Please don't use
> dbus because if it's a dbus problem that goes to bugzilla.gnome.org.

I tried both, the echo stuff first. later also dbus-send first, made no difference.

dbus-send had identical behaviour in everything I tried (reading staying at 7 although dimmed, writing only making a visible change when first setting to a different value, e.g. 6)

I'm sorry to bother you for more information but could you do a:

find /sys -name brightness

and post the output back here?

My current theory is that there may be more than one file setting the brightness.

It's odd that you have to write the 6 out before you can write the 7.  That used to be the way it worked in 2008 but in Jan 2009 they changed it to always write the setting even if it was the same as the current brightness.  The patch that changed it is: 9be1df98bca "bd->props.brightness doesn't reflect the actual backlight level."

(In reply to comment #11)
> I'm sorry to bother you for more information

No problem. you're welcome. I learn in the process, and hope for an eventual resolution.

> could you do a:
> find /sys -name brightness

lat630:~ # find /sys -name brightness
/sys/devices/platform/dell-laptop/backlight/dell_backlight/brightness

> It's odd that you have to write the 6 out before you can write the 7.

I'm going to instrument dell-laptop.c:dell_send_request to see what actually is read and written in the driver. I'm a bit busy with "real work", but will post results asap.

(In reply to comment #11)

> It's odd that you have to write the 6 out before you can write the 7.  That
> used to be the way it worked in 2008 but in Jan 2009 they changed it to
> always
> write the setting even if it was the same as the current brightness.

It tries to...

I instrumented dell-laptop.c:dell_send_request, to show me the brightness setting and getting activity.

I can see then, that when echoing to the sys knobs, the request is passed on, even when the apparent value does not change.

In my test case, switchint between the GDM login screen and the text console, I do NOT see any calls to that dell_send_request - I.E. THE DIMMING DOES NOT HAPPEN THROUGH THE BRIGHTNESS API.

When on the text console, echoing 7 to the brightness sysfs file, I DO see dell_send_request being called with the expected parameters, but it has no visible effect on the dimness.

The dell_send_request function calls dcdbas.c:dcdbas_smi_request, which in turn and pretty much unconditionally, makes an "outb" SMI request. That dcdbas_smi_request function has a single other caller, in the dcdbas.c module, which I also instrumented, just to find that path to be never called. So the DIMMING DOES NOT HAPPEN THROUGH THE dcdbas SMI API, as far as I an see.

It appears as if the dimming is a side effect of something else (mode switching?) on this platform, and it appears as if the SMI handler of the laptop itself remembers what was set "officially" the last time.

Any idea what to look at, next?


I also instrumented

Part of your comment is missing...  What else did you instrument?

I'm going to CC the maintainers.  Could you first upload your:
1) .config
2) an acpidump file.  (You may need to apt-get install acpidump)
3) the output from "sudo lspci -vv"

There are three ways to set the backlight setting on the computer.  The platform specific way (dell-laptop), using ACPI, and through the GPU.  I thought the only way to access those controls was through sysfs though.

Created attachment 38132 [details]
.config of Dell Latitude D630 build showing unexpected backlight dimming

Created attachment 38142 [details]
lspci -vv of Dell Latitude D630 with backlight dimming problems

Created attachment 38152 [details]
acpidump output of Dell D630 with backlight dimming problems

(In reply to comment #14)
> Part of your comment is missing...  What else did you instrument?

Sorry, that was an editing glitch. I also instrumented the
dcdbas.c:smi_request_store function, which is the second caller of
dcdbas.c:dcdbas_smi_request, but that path turned out to be not taken at all.
So, as far as I can see, the backlight-dimming GDM-to-text-console transition
does not happen through the SMI request interface in the dcdbas.c module.

> I'm going to CC the maintainers.  Could you first upload your:
> 1) .config
> 2) an acpidump file.  (You may need to apt-get install acpidump)
> 3) the output from "sudo lspci -vv"

3 attachments appended.

The acpidump command gave some errors:
Wrong checksum for DSDT!
Wrong checksum for SLIC
Wrong checksum for SSDT
Wrong checksum for DSDT!
Wrong checksum for SLIC!
Wrong checksum for SSDT!
Wrong checksum for DSDT!
Wrong checksum for SLIC!
Wrong checksum for SSDT!

Regarding ACPI, I also tried booting with "noacpi" (running that now) - that
did not change the symptoms.

best regards
  Patrick

For the record, the described problem / phenomenon persists in 2.6.37-rc3

(In reply to comment #3)
> I have a similar issue. On startup everything is well. The trouble begins
> when
> the screen is blanked. After this the back light returns in its lowest
> brightness level, and more frustrating, does not respond to the 'fn,
> arrow-up'
> keys anymore.  
> 
> (dell latitude D430, 00:02.1 Display controller: Intel Corporation Mobile
> 945GM/GMS/GME, 943/940GML Express Integrated Graphics Controller (rev 03))

e9e331a8abeece1565d383510ed985945132ffe3 is the first bad commit
commit e9e331a8abeece1565d383510ed985945132ffe3
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Mon Sep 13 01:16:10 2010 +0100

    drm/i915/lvds: Ensure panel is unlocked for Ironlake or the panel fitter
    
    Commit 77d07fd9d73ef28689737c0952dbd5d6a5017743 introduced a regression
    where by not waiting for the panel to be turned off, left the panel and
    PLL registers locked across the modeset. Thus the panel remaining blank.
    
    As pointed out by Daniel Vetter, when testing LVDS it helps to open the
    laptop and look at the actual panel you are purporting to test.
    
    A second issue with the patch was that in order to modify the panel
    fitter before gen5, the pipe and the panel must have be completely
    powered down. So we wait.
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Created attachment 38462 [details]
intel_panel switching backlight dimmer and dimmer

kernel messages from 2.6.37-rc2 with drm.debug=0x07, grepping for "backlight".

Starting with [  169.869343] I cycled from GDM login screen to text console and back several times, without doing anything else in between.

As can be seen (and could be seen physically on the laptop screen) backlight is set dimmer and dimmer with each cycle, until at [  579.977224] the value underflowed somehow - things got a lot brighter at that point.

Hans, can you verify the results Patrick got in comment #21?
If not, it might be better (so we get a clear picture)  if you open a new bugreport and append your bisect result there and also post a comment with the new bug# here.

Hans, your bug might be already reported as #22672.

(In reply to comment #23)
> Hans, your bug might be already reported as #22672.

Yep exactly as as 22672. closing and opening the lid restores the brightness and restores the funtion of the brightness function key's on my laptop. I will switch bug's.

( #21 does not alter brightness settings )

*** This bug has been marked as a duplicate of bug 22672 ***

I'm not shure that Patrick's Bug is a Duplicate of 22672 as his brighness is changing while closing and opening. 

Patrick, do the patches from Keith:

https://patchwork.kernel.org/patch/359472/
https://patchwork.kernel.org/patch/359502/

help your case?

(In reply to comment #26)
> I'm not shure that Patrick's Bug is a Duplicate of 22672 as his brighness is
> changing while closing and opening.

Yep, seems to be a different problem.

> Patrick, do the patches from Keith:
> https://patchwork.kernel.org/patch/359472/
> https://patchwork.kernel.org/patch/359502/
> help your case?

They don't. Applied to -rc4. Seeing some dimming when GDM comes up, dimming worsens again when cycling between GDM and text console.

There seems to be _some_ effect, though: the cycling appears not to go through 0 and start bright again, but stays low.

Also, when on the text console and echoing 6 then 7 to the brighness file of the dell-laptop module, which previously made the display bright again, it now stays dim.

However, logging in to X (server restart) makes the brighness setting have an effect again; the xinit hack I have installed works.

HTH
  Patrick

(In reply to comment #27)

> Also, when on the text console and echoing 6 then 7 to the brighness file of
> the dell-laptop module, which previously made the display bright again, it
> now
> stays dim.

Funny, I cannot reproduce that now, even after another system reboot - the echoing to the dell-laptop brighness file now has the same visible effect as before.

In any case, the dimming problem is most definitely not fixed by these patches.

Looking forward to trying something else.

The regressions I hit are:

https://bugzilla.kernel.org/show_bug.cgi?id=23472
https://bugzilla.kernel.org/show_bug.cgi?id=22672

This on a Thinkpad X40 with Intel 855GM chipset.

As far as I know, the BIOS is the one controlling the backlight on
this laptop, and everything always worked fine. Now the backlight
gets messed up when resuming, and it doesn't turn on until a s2ram
cycle after a "xset dpms force suspend". So it seems that something
started messing with the backlight control while it didn't before,
and it does it wrong.

Symptoms:

Something changes the brightness to the highest setting when the
kernel starts, instead of leaving it to whatever it was before.

Brightness gets messed up again after resuming from suspend, in
a similar manner.

Both of this also happens without X running.

After turning off the backlight with xset dpms force suspend,
it won't turn on again until a suspend/resume cycle.

I have no userspace crap fiddling with hardware behind my back,
it's a clean and simple setup.

Brightness used to stay the same, even after shutting down and
restarting the laptop. It seems the embedded chip stores and
restores the brightness levels and the kernel just shouldn't
touch it at all, but now started to.

> Patrick, do the patches from Keith:
> 
> https://patchwork.kernel.org/patch/359472/
> https://patchwork.kernel.org/patch/359502/
> 
> help your case?

There is a corresponding userspace driver fix which goes with those 2 kernel patches. It's on the master branch of git://anongit.freedesktop.org/xorg/driver/xf86-video-intel  

commit 33c08882c0d551afb28baef643279901dcc65fa9
Author: Keith Packard <keithp@keithp.com>
Date:   Wed Nov 17 16:37:53 2010 +0800

    Mark outputs as DPMSModeOn and restore backlight at mode set
    
    The kernel always turns monitors on when doing mode setting, and so no
    further DPMS action is required. Note this in the mode setting code by
    marking the updated DPMS mode and restoring any saved backlight level.
    
    Signed-off-by: Keith Packard <keithp@keithp.com>
    Reviewed-by: Eric Anholt <eric@anholt.net>

(In reply to comment #30)

> There is a corresponding userspace driver fix which goes with those 2 kernel
> patches. It's on the master branch of
> git://anongit.freedesktop.org/xorg/driver/xf86-video-intel  

I'm sorry, but the last time I compiled userlevel X servers was in the late 80ies under SunOS. I'm not set up for that, I'm hooked on running the openSUSE 11.3 X server userlevel, and if using 2.6.37 means I need a new X server, I will refrain from upgrading the kernel.

I've hunted this one down and I'm pretty sure I found the cause, at least for me. This doesn't seem to fix Patrick's problem though, as he has a gen 3 chipset (I have a 855GM, a gen 2), but it may be related.

The cause seems a faulty is_backlight_combination_mode(). If I add a return 0 to the beginning everything works as it should. The changed code that works for me + debugging info is:

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 92ff8f3..7d81770 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -116,6 +116,7 @@ static int is_backlight_combination_mode(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	return 0;
 	if (INTEL_INFO(dev)->gen >= 4)
 		return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
 
@@ -194,9 +195,11 @@ static void intel_pch_panel_set_backlight(struct drm_device *dev, u32 level)
 void intel_panel_set_backlight(struct drm_device *dev, u32 level)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 tmp;
+	u32 tmp, tmp0, lvl0;
 
 	DRM_DEBUG_DRIVER("set backlight PWM = %d\n", level);
+	tmp0 = intel_panel_get_backlight(dev);
+	lvl0 = level;
 
 	if (HAS_PCH_SPLIT(dev))
 		return intel_pch_panel_set_backlight(dev, level);
@@ -217,4 +220,8 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
 	} else
 		tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
 	I915_WRITE(BLC_PWM_CTL, tmp | level);
+	
+	tmp = intel_panel_get_backlight(dev);
+	printk(KERN_ERR "intel_panel_set_backlight: %u -> %u (level = %u/%u)\n",
+			tmp0, tmp, lvl0, level);
 }

---

The problem might be introduced by this kernel commit, I recommend Patrick to try with this one reverted (I haven't yet):

commit a6c45cf013a57e32ddae43dd4ac911eb4a3919fd
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Sep 17 00:32:17 2010 +0100

    drm/i915: INTEL_INFO->gen supercedes i8xx, i9xx, i965g
    
    Avoid confusion between i965g meaning broadwater and the gen4+ chipset
    families.

You can also try my changes above, without and with a return 0 or 1 and see if the backlight setting actually works as it should, and if your problem is also related to is_backlight_combination_mode(). I wouldn't bother with changing any userspace stuff, at least I have exactly the same problems without X.

Hurray! Great hunting, Indan!

My backlight problem is still present in released 2.6.37.

With your "return 0" patch applied, IT IS GONE!

Everything stays bright when repeatedly switching between gdm and text console.

The backlight keys on the laptop keyboard continue to work as expected.

Echoing stuff to the sysfs brightness file works as expected.

/me is happy now :)

Now, to me the whole is_backlight_combination_mode() things looks bogus and I'd love to rip it out completely. However, there is probably a reason for it to be there, though I doubt it was tested very well.

Chris, what's going on here? You adding this code.

What we need to find are people whose hardware stops working without is_backlight_combination_mode().

Or is it all a simple mistake and should it have been:

--- drivers/gpu/drm/i915/intel_panel.c	2011-01-11 22:18:44.876246546 +1100
+++ drivers/gpu/drm/i915/intel_panel.c	2011-01-11 22:19:01.000000000 +1100
@@ -121,7 +121,7 @@ static int is_backlight_combination_mode
 		return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
 
 	if (IS_GEN2(dev))
-		return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
+		return !(I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE);
 
 	return 0;
 }

According to the Intel documentation, BLM_COMBINATION_MODE indeed exists and should be checked for at least gen 4, probably higher too. However, the gen 2 documentation does not mention anything like BLM_LEGACY_MODE at all. So my proposed patch is:

Get rid of BLM_LEGACY_MODE, as it causes backlight dimming problems if bit 
BLM_LEGACY_MODE is coincidentally set in the max brightness value. According 
to the Intel documentation BLM_LEGACY_MODE does not exist for gen 2 hardware 
anyway.

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 cb8f434..d6525de 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1440,7 +1440,6 @@
  * 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 92ff8f3..5b4de5e 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -118,10 +118,6 @@ static int is_backlight_combination_mode(struct drm_device *dev)
 
 	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;
 }
 
---

A correct check can always be added when real problems show up when this is missing.

Quote from Vol_3_G45_register.pdf:

"Bit 30   BLM Legacy Mode

0 = PWM Duty Cycle is derived from the Backlight Duty Cycle only
1 = PWM Duty Cycle is a combination of Backlight Duty Cycle and Legacy Backlight Control (LBPC)

Note: `1' implies the Duty Cycle =
 if(BPC[7:0] <> xFF) then BPCR[15:0] * BPC[7:0] Else BPCR[15:0]"

If that's all that is to it, then I suspect that the current code is more complicated than needed.

My problems are also solved by using the patch provided by Chris Wilson here:

https://bugzilla.kernel.org/show_bug.cgi?id=22672#c42
https://bugzilla.kernel.org/attachment.cgi?id=43272

*** This bug has been marked as a duplicate of bug 22672 ***

I tried without my patches and using only Chris' patches. For me the backlight problem is not as bad as it was before, but is still present. So the bug is not a pure duplicate of bug 22672, though it may be related.

Patrick, can you please double check that all dimming problems are gone?

It seems that if is_backlight_combination_mode() ever returns true things go wrong.

Observed behaviour:

Suspend and resume works as expected. But:

When doing "xset dpms force off; sleep 0.1; xset dpms force on" in a loop:

- When starting at highest level, the brightness becomes one notch dimmer. Lowering the brightness one notch makes the screen as bright as it should be for that level. However, interestingly enough, if I increase it back, then it goes too dim again. My guestimation is that the highest level is set to halve the value it should have been, the same bug I fixed with the return 0.

- When started at lowest brightness level, the brightness gets increasingly dimmer with each invocation.

- Doing the loop in any other starting brightness than either the lowest or highest works as it should.

I get the same in different screen resolutions.

For both I see strange behaviour I haven't seen before, but probably was there because you can only see it with a short interval between the off and on:

After turning on, the xterm is show at the bottom of the screen instead of at the top for a short moment, then the screen flickers and everything is back to normal. How much lower the xterm is varies. This is probably not related to this bug though, but might be either a harmless hardware quirk or some suboptimality in the driver.

It seems that the behaviour at bootup is the same as when doing one loop.

So I stand by what I said in comment 35, checking for bit 16 in BLC_PWM_CTL seems wrong for gen 2. The upper 16 bits are the maximum brightness, and the value I have just happens to have bit 16 set. With my debugging patch I saw that the upper bits of BLC_PWM_CTL are always the same.

I'll reboot with drm.debug=0x02 and see if there's anything interesting there.

I still notice one or two small dimming problems:

At some time during boot, before X even turns on, the laptop screen backlight dims down by one step.

Also, after having the X screen locked for some time, after unlocking it, the backlight is also dimmed a single step. This is not reproducible by just locking and immediately unlocking, but I noticed it once yesterday after 30 minutes, and once this morning after having the screen locked for the night.

In both cases, hitting the brightness-up key once returns to full brightness.

I did not notice this effect with Indan's "return 0" patch.

The one-step dimming at boot, still happens with Indan's patch from comment #35.
It seems that I have a "gen 4" device.

It no longer happens when I again make is_backlight_combination_mode return 0 unconditionally.

In addition to the "return 0" I added a printk showing the relevant stuff to is_backlight_combination_mode. This is what it shows me upon booting:

<4>[    3.746796] is_backlight_combination_mode gen 4 IS_GEN2 0 BLC_PWM_CTL2 0xe0000000 - FORCE 0
<4>[    3.750059] is_backlight_combination_mode gen 4 IS_GEN2 0 BLC_PWM_CTL2 0xe0000000 - FORCE 0
<4>[    3.751072] is_backlight_combination_mode gen 4 IS_GEN2 0 BLC_PWM_CTL2 0xe0000000 - FORCE 0
<4>[    4.331065] is_backlight_combination_mode gen 4 IS_GEN2 0 BLC_PWM_CTL2 0xe0000000 - FORCE 0

--- linux-2.6.37/drivers/gpu/drm/i915/intel_panel.c	2011-01-13 09:12:14.909578189 +0100
+++ linux-2.6.37-intel/drivers/gpu/drm/i915/intel_panel.c	2011-01-13 09:13:00.023271233 +0100
@@ -116,11 +116,8 @@
 {
 	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;
+	printk("is_backlight_combination_mode gen %d IS_GEN2 %d BLC_PWM_CTL2 0x%x - FORCE 0\n",
+		INTEL_INFO(dev)->gen, IS_GEN2(dev), I915_READ(BLC_PWM_CTL2));
 
 	return 0;
 }

Created attachment 43422 [details]
fix_combination_mode.diff

Patrick, can you give this patch a try? It tries to follow the Intel PRM 
to the letter.

Follow the Intel PRM documentation more closely for backlight setting.
The current code doesn't always work properly for gen 2 and 4 chips.

- The current code seems to miss the if != 0xff check.

- When combination mode is enabled, the calculated register values are quite
  rubbish. This patch searches for the best ones given the desired level.

- The magic bit 1 << 16 for gen 2 hardware is not mentioned anywhere and
  breaks proper backlight setting for some hardware. Without the check 
  everything works properly.

- Get rid of the power-of-two rounding.

I hope point 1 will fix your problem, or else 2. Point 3 fixes my problems, 
and 4 should be harmless and is more a cleanliness thing.

Indan, with your fix_combination_mode.diff on top of both 2.6.37 and 2.6.37-with-Chris'-changes, everything seems fine for me:

- no dimming on boot
- manual adjustment using the brightness keys, works
- that manual adjustment persists when switching between X and text console
- the brightness knob in sysfs also works

Do you see any "set backlight LBPC:" in your dmesg, or is it just the 0xff check that fixed it for you? (You can tell if you boot with drm.debug=0x02 and check for the lbpc value printed out in the modified "get backlight PWM".)

Overall, I think my patch is a bit over the top and instead the driver shouldn't be bothered to handle combined mode specially at all. This would result in the driver never changing the (undocumented) PCI_LBPC register, and only changing BLC_PWM_CTL. At worst this means that the granularity of changing the backlight isn't as finegrained as it could have been (it goes in steps of 254 or less), but everything still should work. No tricky corner cases, just robust code. My impression is that LBPC is supposed to be set to a fixed value anyway, depending on the hardware, and that software is only supposed to fiddle with BLC_PWM_CTL. Only potential problem is when the boot value of LBPC * max is not bright enough, but that is very unrealistic, because it seems the whole point of LBPC is to just upscale the value, not to actively control the brightness with it.

All the current code does is saving and restoring the backlight value anyway, so no LBPC fiddling is usually needed. Only place where full backlight control might be needed is asle_set_backlight() in intel_opregion.c. I can't find any documentation about ASLE and have no idea what that is. Perhaps add a printk to asle_set_backlight() to see if it's called at all for our hardware? Because if it's mutually exclusive with combined mode, combined mode doesn't have to be handled specially at all.

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 9b0d9a8..2592ceb 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -160,6 +160,7 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 	intel_panel_set_backlight(dev, bclp * max / 255);
 	asle->cblv = (bclp*0x64)/0xff | ASLE_CBLV_VALID;
 
+	printk(KERN_WARNING "asle_set_backlight %d\n", bclp * max / 255);
 	return 0;
 }
 
Also, after Chris' patch, intel_panel_set_backlight() is only needed for asle_set_backlight(). The rest just disabled and restores the backlight.

So if Patrick can confirm that he doesn't get any "set backlight LBPC" I propose to just get rid of is_backlight_combination_mode() altogether.

Created attachment 43482 [details]
remove_is_backlight_combination_mode

drm/i915: Do not handle backlight combination mode specially.

The current code does not follow Intel documentation and 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, instead 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.

Signed-off-by: Indan Zupancic <indan@nul.nu>

Being sent here from bug 22672 I can confirm that the last patch (remove_is_backlight_combination_mode) solved the remaining backlight problem on my Dell Vostro. Thx for the pointer Indan!

Handled-By : Indan Zupancic <indan@nul.nu>
Patch : https://bugzilla.kernel.org/attachment.cgi?id=43482

For the record, this is still unfixed in 2.6.38-rc4. While the backlight no longer cycles down to 0 when repeatedly switching between X and text console, it still goes down perceptibly. Using the brightness-up key continues to work for me.

Created attachment 47302 [details]
remove_is_backlight_combination_mode.2.6.38.diff

I just tried 2.6.38-rc4 myself and it indeed still has the old weird behaviour, as expected. E.g. when in lowest brightness mode and doing "xset dpms force off/on" a few cycles gives results like this:

[  854.957349] [drm:intel_panel_get_backlight], get backlight PWM = 919179
[  854.957363] [drm:intel_panel_set_backlight], set backlight PWM = 0
[  855.957448] [drm:intel_panel_set_backlight], set backlight PWM = 919179
[  862.677218] [drm:intel_panel_get_backlight], get backlight PWM = 459567
[  862.677232] [drm:intel_panel_set_backlight], set backlight PWM = 0
[  863.451616] [drm:intel_panel_set_backlight], set backlight PWM = 459567
[  864.513133] [drm:intel_panel_get_backlight], get backlight PWM = 229782
[  864.513146] [drm:intel_panel_set_backlight], set backlight PWM = 0
[  865.311197] [drm:intel_panel_set_backlight], set backlight PWM = 229782
[  866.125146] [drm:intel_panel_get_backlight], get backlight PWM = 114891
[  866.125159] [drm:intel_panel_set_backlight], set backlight PWM = 0
[  866.938987] [drm:intel_panel_set_backlight], set backlight PWM = 114891

And the brightness values are still not always right after bootup or resume.

For me switching between console and X works, though I do get screen flickering and pipe a underruns.

So attached a patch for 2.6.38-rc4, and hopefully it will be merged upstream before the 38 release.

Also, Chris, your HIC poking patch didn't make it upstream yet either, which probably explains the screen corruptions I see now. You seem slightly swamped, perhaps try to delegate some work to other Intel devs?

Thanks, Indan! Your patch again fixes my symptoms.

I also see screen corruption, on the second (external) display, and only in text mode. While the second display still perfectly mirrors the laptop screen in that mode, the normally black area at the upper right corner of the screen is filled with a static green dot pattern, also persisting when switching between X and text mode.

Patch : https://bugzilla.kernel.org/attachment.cgi?id=47302
Ignore-Patch : https://bugzilla.kernel.org/attachment.cgi?id=43482

Care to submit the patch to the i915 maintainers or Andrew Morton?

Patrick, I see some new screen corruption as well, though a different kind.
I think it's a new bug and not related to this.

Rafael, I sent the patch to lkml, in reply to Alex's message. The i915
maintainers are CCed to this bug report. I don't mind handling bugs via 
lkml, but it seemed bugzilla was preferred. Bypassing Chris by sending
this patch to others seemed a bit rude to me, he's the one who knows
the code the best (he's CCed on the email too).

The Bugzilla is preferred for debugging/tracking, but patches intended for
inclusion should be submitted via e-mail eventually, so that the relevant
maintainers can pick them up (the patches also become more visible this way,
so people who are seeing the same issue may potentially find them more
easily).

*** Bug 25072 has been marked as a duplicate of this bug. ***

I found the commit in 2.6.38-rc5, and wondered whether this (or a part of these) problem is just a wrong calculation in intel_panel_get_backlight()?  Through the refactoring, a bogus bit-shift was introduced there only in the combination mode.

I fixed it yesterday for another bug (bfo#34524)
    https://bugs.freedesktop.org/show_bug.cgi?id=34524
and stumbled upon this today...

Created attachment 48652 [details]
One-liner to fix calculation in intel_panel_get_backlight()

My patch is in rc6 now, so this bug can be closed.

Yes, the bogus shift is the most visible bug. But there are real problems 
with that code, besides being just ugly:

- 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.

(You can try to prove the mathematical correctness of the calculations,
if you feel strongly about this code.)

Only thing that might not work by ignoring LBPC is when the BIOS fiddles
with LBPC while the kernel uses BLC_PWM_CTL, but that won't work correctly
with the buggy code either.

(In reply to comment #57)
> My patch is in rc6 now, so this bug can be closed.

Hey, it's a regression in 2.6.37, so this should be put to stable kernel properly ;)
 
> Yes, the bogus shift is the most visible bug. But there are real problems 
> with that code, besides being just ugly:
> 
> - 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.

I fully agree to remove this bit.  It makes things rather complex in comparison to its gain.
I stated my patch just for asking whether the one-liner is sufficient for fixing stable kernel.

> (You can try to prove the mathematical correctness of the calculations,
> if you feel strongly about this code.)

LOL.

> Only thing that might not work by ignoring LBPC is when the BIOS fiddles
> with LBPC while the kernel uses BLC_PWM_CTL, but that won't work correctly
> with the buggy code either.

Can't we simply initialize this?

The sad thing is that it was reported before 2.6.37 was released. :-(
Feel free to send an email to Greg KH to add it to 2.6.37.2. But the
other fixes made it into his tree by themselves...

The code was added in 2.6.37, it was totally absent in 2.6.36, so I don't
see why not removing it altogether would be in any way better or safer.

We can't simply initialize LBPC because it's undocumented, so it could be
anything. Maybe there are laptops out there with LBPC set to something 
else than 0xff, and setting it higher makes the screen too bright (or 
burns something, who knows.) That's another reason to remove it, we don't
have enough information about it. The buggy code used to make my screen
way too bright too sometimes.

We could add a sys interface to get and set the LBPC register directly,
that would help to get more info about it.

Well, if a Cc to stable@kernel.org was added in the patch changelog, this would have been picked up to 2.6.37.x automatically...

One concern is that you don't know whether this revert also would give another regression.  Judging from the bug mentioned in the affecting commit (bfo#29716), I doubt it.  But, for a regression fix for stable kernel, smaller is safer in general.  (But don't misunderstand, it's about generally speaking; I'm for removing these chunks, and if someone confirms it working with 2.6.37, it should go to stable tree, too.)

Regarding LBPC: yes, it sound messy.  I agree with a solution to provide some special setup for LBPC register (e.g. debugfs), or set up some default with exceptions for some machines (with blacklisting).

Yes, you're right, but it seemed hard enough to get into mainline, I didn't
think about stable. It's up to the Intel devs to include my patch or not,
but Linus added it directly. So I've no idea who's responsible for getting
it into stable. I think it's a good idea, as it fixes known bugs.

It can't give any regression pre 2.6.37 because all that backlight code is new.
2.6.37 can set weird LPBC values, which some laptops might remember between
boots, so the sooner the buggy code is gone, the better. I also checked 
xf86-video-intel, and it doesn't touch it either.

Any fiddling with LBPC can cause subtle, weird behaviour. It's laptop model
dependent what effect it has, so you can't just check the chipset or anything.
At least I miss information about it, if someone with Intel insider info can
tell more about LBPC, e.g. assure that 0xff is always the max brightness and
not too bright, we could set it to 0xff at boot. But before 2.6.37, as far as
I can tell, nothing in Linux touched LPBC.

Do you want me to send an email to stable@kernel.org?

Fixed by http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=951f3512dba5bd44cda3e5ee22b4b522e4bb09fb .

Indan, your path is still not quite OK. If I boot kernel (with your patch) but withouth i915 module ever inserted, then I reboot, boot again, this time i915 module is inserted, I got again only halved maximal brightness :(

Lukas, my patch changes the i915 module, so it sounds like you accidentally loaded the old i915 module instead of the patched one after the reboot. Can you double check?

I got another observation which is more precise. If I boot with your patch but without backlight set to maximum level. I got the symptoms:
maximum level is halved
starx causes backlight reset to 0 or 1 (to very low level anyway).

If I set backlight to maximum (in the situation above - i.e., maximum level is halved) and reboot the machine, it works as expected. But any boot with different level than maximum breaks behavior.

For more background, Lukas's case is described at bug 25072.

So let me summarize your case, to make sure I understood you correctly:

- Without my patch, backlight is often wrong.

- With my patch, it works perfectly if the brightness was set to maximum
  before reboot, but if not, then weird things start to happen.

What happens if you shutdown the laptop, and then boot it up again, instead
of rebooting? Does it work correctly, or do you get the same behaviour as 
with rebooting?

For what it's worth, can you also retry with 2.6.38-rc6?

There seems to be something weird within your machine, I wonder what it is.

It's getting late here, I'll send a debug patch tomorrow.

> So let me summarize your case, to make sure I understood you correctly:
> 
> - Without my patch, backlight is often wrong.
> 
> - With my patch, it works perfectly if the brightness was set to maximum
>   before reboot, but if not, then weird things start to happen.

yes, this is correct. 

> For what it's worth, can you also retry with 2.6.38-rc6?

it happens with this kernel and 2.6.37 + with your patch applied. Both of them.

I will try shutdown and let you know.

It seems you got a different bug than this one and it might take a lot
of CC spamming to nail it, so let's handle it via email and when we 
know more we'll go back to bug 25072.

(In reply to comment #62)
> Fixed by
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
> .

Running vanilla 2.6.38-rc6 now - confirmed!

Big Thanks Indan for getting this done!

(In reply to comment #56)
> Created an attachment (id=48652) [details]
> One-liner to fix calculation in intel_panel_get_backlight()

Takashi Iwai, you were right after all.

My patch should be reverted and Takashi Iwai's patch should be applied
instead. I missed the case where ASLE is used for backlight control 
and the BIOS uses LPBC to store and restore the backlight.

Intel made backlight control as complicated and stupid as possible.
I'd love to simplify the whole mess, but it's too late for that in
the release cycle now.

Confirmed to be fixed in 2.6.38-rc7 for me as well.

Almost fixed for me.

GPU: Intel Corporation Mobile 4 Series Chipset Integrated Graphics Controller (rev 07)

HP BIOS appears to use LBPC for backlight control.
The backlight "value" set in GRUB becomes a maximal value for booted linux system.
The sysfs knob (virtual acpi device) always reports 24 steps and always starts at 24th.

This also allows reaching much dimmer screen for night usage (LED backlight...)
It would be nice to switch between "day LBPC" and "night LBPC" somehow.
Thanks in advance

Oh.. forget about LBPC.. I haven't tested what BIOS does. just results.

also: the BIOS(EFI..) manages to preserve the value if the system is rebooted.
Causing reboots to modify maximal backlight value