Bug 25072

Summary: backlight level is not maintened during LID close/open
Product: Drivers Reporter: Lukas Hejtmanek (xhejtman)
Component: Video(DRI - Intel)Assignee: drivers_video-dri-intel (drivers_video-dri-intel)
Status: CLOSED DUPLICATE    
Severity: normal CC: acpi-bugzilla, florian, indan, maciej.rutecki, raa.lkml, rjw, xhejtman
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.37-rc5 Subsystem:
Regression: Yes Bisected commit-id:
Bug Depends on:    
Bug Blocks: 21782    

Description Lukas Hejtmanek 2010-12-17 11:56:37 UTC
as of 2.6.37-rcX kernels, the backlight level is not properly maintaned across LID close/open cycle. I use backlight level 4, when I close and open again the LID, I got backlight level 2 (although /sys/class/backlight/acpi_video0/brightness still reports level 4).

I have Lenovo ThinkPad T61 with i965GM.
Comment 1 Florian Mickler 2011-01-05 22:41:40 UTC
Do the symptoms match with bug #23472?
Comment 2 Lukas Hejtmanek 2011-01-05 23:15:51 UTC
It does not seem to be the same. I got the only problem with LID open/close. The mentioned patches are applied to my kernel and the xorg driver as well and the bug still occurs.
Comment 3 Indan 2011-01-11 01:43:22 UTC
What does your laptop do when you close the lid? Does it suspend?

Anyhow, I'm quite sure it's a duplicate of #23472, with the same root cause. The behaviour I've seen is a more or less exact halving of the brightness after each suspend/resume cycle, if I don't touch the brightness keys (then it jumps back to what it is supposed to be).

Can you try doing what I suggest in #23472 and either see if reverting commit a6c45cf013a57e32ddae43dd4ac911eb4a3919fd fixes it for you, of if adding a "return 0" or "return 1" at the start of is_backlight_combination_mode() in drivers/gpu/drm/i915/intel_panel.c fixes it?
Comment 4 Florian Mickler 2011-02-09 05:34:39 UTC
*ping*
Comment 5 Lukas Hejtmanek 2011-02-09 10:14:33 UTC
Well, the patch in #23472 seems to maintain backlight level. On the other hand, I'm still getting only 8 levels of backlight in Xfce. It used to be 16 levels. And more over, with the patch applied, I got the same backlight intensity after echo 15 > /sys/class/backlight/acpi_video0/brightness as I got after echo 5 > /sys/class/backlight/acpi_video0/brightness without the patch. Any ideas?
Comment 6 Lukas Hejtmanek 2011-02-09 10:16:53 UTC
As of your first question, my laptop does nothing when I close the lid. It does not suspend. Moreover, backlight level seems to be maintained across suspends.
Comment 7 Indan 2011-02-09 10:53:02 UTC
If it does nothing it probably just blanks the screen.

Is getting 8 levels instead of 16 something introduced by 
my remove_is_backlight_combination_mode patch? And is that
8 effective levels of brightness, or 8 detected levels of
brightness?

Give fix_combination_mode.diff a try instead. I hope it 
won't help, because it's quite ugly. But I fear it does.

The output of 

"dmesg | grep backlight"

with drm.debug=0x2 would be interesting if this patch helps.

The thing is, the old code did weird things when it detected
combined mode, so it doesn't surprise me that the backlight
levels are different. Question is, are they right now? Is the
lowest level dark enough and the highest bright enough?

Furthermore, if your laptop remembers the brightness between
reboots, then old, incorrect values may be restored as well,
and the patched kernel won't fiddle with messed registers.
So set the brightness at the maximum in the vanilla kernel 
before rebooting into a patched one.

So before you do anything, go into the broken kernel, set
brightness to maximum, reboot into the patched one and see
if everything is all right. (Or set brightness to max in 
the BIOS if possible.) If not, try the above mentioned patch.
Comment 8 Lukas Hejtmanek 2011-02-16 10:29:54 UTC
Aaaa, why don't I get email notifications from your comments?

Yes, it just blanks the screen.

Yeah, your patch removes 16 levels. I can actually write 0-15 values into /sys/class/backlight/acpi_video0/brightness. But it seems, that the value is halved by the kernel when setting the brightness. 

> Question is, are they right now? Is the
> lowest level dark enough and the highest bright enough?

With "remove_is_backlight_combination_mode", they don't. Lowes is dark enough, but the highest is at half brightness that is possible without the patch.

OK, I do as you request.
Comment 9 Lukas Hejtmanek 2011-02-16 10:44:17 UTC
Well, after booting with the "remove_is_backlight_combination_mode" patch with maximum brightness, it works fine. I got 16 levels back and brightness does not get changed after LID close/open. 

It also happened before that starting the X server resulted in resetting brightness to 0. Now it is OK also this one.
Comment 10 Alex Riesen 2011-02-16 19:41:57 UTC
I believe I have the problem. I may be even have a workaround for it:

http://marc.info/?l=linux-kernel&m=129788444720834&w=2

which can be wrong. I should have done more research before sending the
patch. I particular, I completely missed this bug report and the associated
discussion.
Comment 11 Indan 2011-02-17 01:13:15 UTC
(Lukas, you didn't get a notification mail because you weren't on the CC list.
I added you back.)

Okay, thanks for te feedback Lukas. All in all it seems that the LBPC register
value got stored and restored by your laptop. My patch doesn't touch that one,
so if it got lowered by the old code it would stay low, that's why rebooting
with max brightness "fixed it".

The bad thing about this is that if people switch to a good kernel they can
still have a messed up backlight. :-(

No idea about X setting the brightness to 0, might be a freak side-effect
of the buggy code, I don't know why my patch fixes that.

---

Alex, that patch doesn't look totally correct. It always sets brightness to 
the maximum at bootup, for one thing, and the rest seems a mix of Chris' 
patch and partially reverting another of his patches, right? In the end it 
boils down to reverting the introduction of the i915_read_blc_pwm_ctl()
helper function, but I don't see how that can ever make a difference 
because 'val' can't ever be zero, as far as I know.

The only other real change is the addition of:

@@ -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;
+

But calling this multiple times when the panel is already enabled should be
fine.

So see bug 23472, I think you got as far as me without my patch, which means
it mostly works, but not for the lowest or highest brightness after boot,
suspend or screen blanking.

Anyway, this bug should be closed as duplicate of bug 23472.
Comment 12 Florian Mickler 2011-02-20 13:50:50 UTC
Thanks, closing.

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