Bug 9995 - 2.6.25-rc1 regression - backlight controls stopped working - ThinkPad T61
Summary: 2.6.25-rc1 regression - backlight controls stopped working - ThinkPad T61
Status: CLOSED CODE_FIX
Alias: None
Product: ACPI
Classification: Unclassified
Component: Power-Video (show other bugs)
Hardware: All Linux
: P1 blocking
Assignee: Zhang Rui
URL:
Keywords:
Depends on: 10221
Blocks: 9832
  Show dependency tree
 
Reported: 2008-02-15 04:51 UTC by Lukas Hejtmanek
Modified: 2008-07-03 03:38 UTC (History)
10 users (show)

See Also:
Kernel Version: 2.6.25-rc1
Subsystem:
Regression: Yes
Bisected commit-id:


Attachments
acpidump output (316.12 KB, text/plain)
2008-02-15 04:51 UTC, Lukas Hejtmanek
Details
debug patch (754 bytes, patch)
2008-02-21 21:52 UTC, Zhang Rui
Details | Diff
create symbol link from backligh device to acpi device node (1.12 KB, patch)
2008-02-21 22:06 UTC, Zhang Rui
Details | Diff
Patch to blacklist thinkpads to use thinkpad_acpi (1.58 KB, patch)
2008-03-18 09:54 UTC, Thomas Renninger
Details | Diff
Add opregion support to Intel DRM (16.36 KB, patch)
2008-05-28 07:22 UTC, Matthew Garrett
Details | Diff

Description Lukas Hejtmanek 2008-02-15 04:51:13 UTC
Latest working kernel version: 2.6.24
Earliest failing kernel version: 2.6.25-rc1
Distribution: Ubuntu/Hardy
Hardware Environment: ThinkPad T61
Software Environment: n/a
Problem Description: /sys/class/backlight/acpi_video0 does not control backlight, nothing happens if I change brightness.

Steps to reproduce: echo {15,5,0} > /sys/class/backlight/acpi_video0/brightness 
nothing happens.
Comment 1 Lukas Hejtmanek 2008-02-15 04:51:58 UTC
Created attachment 14851 [details]
acpidump output
Comment 2 Lukas Hejtmanek 2008-02-15 04:52:28 UTC
lspci
00:00.0 Host bridge: Intel Corporation Mobile PM965/GM965/GL960 Memory Controller Hub (rev 0c)
00:02.0 VGA compatible controller: Intel Corporation Mobile GM965/GL960 Integrated Graphics Controller (rev 0c)
00:02.1 Display controller: Intel Corporation Mobile GM965/GL960 Integrated Graphics Controller (rev 0c)
00:19.0 Ethernet controller: Intel Corporation 82566MM Gigabit Network Connection (rev 03)
00:1a.0 USB Controller: Intel Corporation 82801H (ICH8 Family) USB UHCI Contoller #4 (rev 03)
00:1a.1 USB Controller: Intel Corporation 82801H (ICH8 Family) USB UHCI Controller #5 (rev 03)
00:1a.7 USB Controller: Intel Corporation 82801H (ICH8 Family) USB2 EHCI Controller #2 (rev 03)
00:1b.0 Audio device: Intel Corporation 82801H (ICH8 Family) HD Audio Controller (rev 03)
00:1c.0 PCI bridge: Intel Corporation 82801H (ICH8 Family) PCI Express Port 1 (rev 03)
00:1c.1 PCI bridge: Intel Corporation 82801H (ICH8 Family) PCI Express Port 2 (rev 03)
00:1c.2 PCI bridge: Intel Corporation 82801H (ICH8 Family) PCI Express Port 3 (rev 03)
00:1c.3 PCI bridge: Intel Corporation 82801H (ICH8 Family) PCI Express Port 4 (rev 03)
00:1c.4 PCI bridge: Intel Corporation 82801H (ICH8 Family) PCI Express Port 5 (rev 03)
00:1d.0 USB Controller: Intel Corporation 82801H (ICH8 Family) USB UHCI Controller #1 (rev 03)
00:1d.1 USB Controller: Intel Corporation 82801H (ICH8 Family) USB UHCI Controller #2 (rev 03)
00:1d.2 USB Controller: Intel Corporation 82801H (ICH8 Family) USB UHCI Controller #3 (rev 03)
00:1d.7 USB Controller: Intel Corporation 82801H (ICH8 Family) USB2 EHCI Controller #1 (rev 03)
00:1e.0 PCI bridge: Intel Corporation 82801 Mobile PCI Bridge (rev f3)
00:1f.0 ISA bridge: Intel Corporation 82801HBM (ICH8M-E) LPC Interface Controller (rev 03)
00:1f.1 IDE interface: Intel Corporation 82801HBM/HEM (ICH8M/ICH8M-E) IDE Controller (rev 03)
00:1f.2 SATA controller: Intel Corporation 82801HBM/HEM (ICH8M/ICH8M-E) SATA AHCI Controller (rev 03)
00:1f.3 SMBus: Intel Corporation 82801H (ICH8 Family) SMBus Controller (rev 03)
03:00.0 Network controller: Intel Corporation PRO/Wireless 4965 AG or AGN Network Connection (rev 61)
15:00.0 CardBus bridge: Ricoh Co Ltd RL5c476 II (rev ba)
15:00.1 FireWire (IEEE 1394): Ricoh Co Ltd R5C832 IEEE 1394 Controller (rev 04)
15:00.2 SD Host controller: Ricoh Co Ltd R5C822 SD/SDIO/MMC/MS/MSPro Host Adapter (rev 21)
15:00.3 System peripheral: Ricoh Co Ltd R5C843 MMC Host Controller (rev 11)
15:00.4 System peripheral: Ricoh Co Ltd R5C592 Memory Stick Bus Host Adapter (rev 11)
15:00.5 System peripheral: Ricoh Co Ltd xD-Picture Card Controller (rev 11)
Comment 3 Anonymous Emailer 2008-02-15 12:21:58 UTC
Reply-To: akpm@linux-foundation.org

On Fri, 15 Feb 2008 04:51:15 -0800 (PST)
bugme-daemon@bugzilla.kernel.org wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=9995
> 
>            Summary: 2.6.25-rc1 regression - IBM ACPI backlight controlls do
>                     not work
>            Product: ACPI
>            Version: 2.5
>      KernelVersion: 2.6.25-rc1
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: BIOS
>         AssignedTo: acpi_bios@kernel-bugs.osdl.org
>         ReportedBy: xhejtman@fi.muni.cz
> 
> 
> Latest working kernel version: 2.6.24
> Earliest failing kernel version: 2.6.25-rc1
> Distribution: Ubuntu/Hardy
> Hardware Environment: ThinkPad T61
> Software Environment: n/a
> Problem Description: /sys/class/backlight/acpi_video0 does not control
> backlight, nothing happens if I change brightness.
> 
> Steps to reproduce: echo {15,5,0} >
> /sys/class/backlight/acpi_video0/brightness 
> nothing happens.
> 

I've lost the plot on what's happening with backlight.  I think you're
about to find that this is not-a-bug, even though it stopped working.
Comment 4 Anonymous Emailer 2008-02-15 14:51:19 UTC
Reply-To: xhejtman@ics.muni.cz

On Fri, Feb 15, 2008 at 12:21:37PM -0800, Andrew Morton wrote:
> I've lost the plot on what's happening with backlight.  I think you're
> about to find that this is not-a-bug, even though it stopped working.

Why do you think that it is not-a-bug even if it stopped working? I don't
think it is a feature as acpi video driver should control backlight levels on
laptop that provides generic ACPI based backlight support which is the case of
ThinkPad T61. Or at least it used to be with up to 2.6.24 kernels.

I can read sane values from /sys/class/backlight/acpi_video0/* but changing them
has no effect on the backlight.
Comment 5 Anonymous Emailer 2008-02-15 15:19:20 UTC
Reply-To: mjg59@srcf.ucam.org

No, this one is a bug. I'll try to deal with it.
Comment 6 Zhang Rui 2008-02-17 22:22:13 UTC
Lukas,
you have two ACPI video devices shown in sys I/F in 2.6.24 and only one of them can work for you, right?
please revert 3fa2cdcc45a0176de15cac9dbf4ed2834ebf8932
and see if the sys I/F works again.
Comment 7 Chuck Ebbert 2008-02-18 10:41:22 UTC
After adding this patch to Fedora 8, we get the same kind of report:

https://bugzilla.redhat.com/show_bug.cgi?id=432587
Comment 8 Zhang Rui 2008-02-18 23:32:53 UTC
(In reply to comment #7)
> After adding this patch to Fedora 8, we get the same kind of report:
here "this patch" means commit 3fa2cdcc45a0176de15cac9dbf4ed2834ebf8932, right?
Comment 9 Lukas Hejtmanek 2008-02-19 14:57:47 UTC
(In reply to comment #6)
> you have two ACPI video devices shown in sys I/F in 2.6.24 and only one of
> them
> can work for you, right?

right.

> please revert 3fa2cdcc45a0176de15cac9dbf4ed2834ebf8932
> and see if the sys I/F works again.

no. Still only 1 device which is not working.
Comment 10 Chuck Ebbert 2008-02-20 14:44:05 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > After adding this patch to Fedora 8, we get the same kind of report:
> here "this patch" means commit 3fa2cdcc45a0176de15cac9dbf4ed2834ebf8932,
> right?
> 

We actually added:

3fa2cdcc45a0176de15cac9dbf4ed2834ebf8932
38531e6fe51ad5c7dfe72e0e066b5f54bc1921cd
Comment 11 Zdenek Kabelac 2008-02-21 05:47:05 UTC
well I don't even see any entry in the directory /sys/class/backlight  - it's got lost somewhere during 2.6.24-rc1 & rc2 in the current git tree.

However there is other way how can I get backlight control working again.
when I use this modprobe parameter:

options thinkpad_acpi brightness_enable=1

I get backlight working - but not via standard backlight control
(but it's usable as a temporal fix)
Comment 12 Zhang Rui 2008-02-21 21:52:56 UTC
Created attachment 14946 [details]
debug patch

Please apply this patch and attach the dmesg output.
Then please play with /sys/class/backlight/acpi_video0/... and /sys/class/backlight/acpi_video1/... and find out which acpi_video backligh sys I/F works for you.
Comment 13 Zhang Rui 2008-02-21 22:06:58 UTC
Created attachment 14947 [details]
create symbol link from backligh device to acpi device node

Please apply this patch, find which acpi_video device backlight sys I/F can work for you and attach the result of cat /sys/class/backlight/acpi_videoX/acpi_dev/path.
Comment 14 Zhang Rui 2008-02-21 22:08:29 UTC
zdenek,
please set CONFIG_ACPI_VIDEO and modprobe video after system boot.
Comment 15 Rafael J. Wysocki 2008-02-24 16:16:54 UTC
Regressions list annotation:
Handled-By : Zhang Rui <rui.zhang@intel.com>
Comment 16 Lukas Hejtmanek 2008-02-25 04:52:35 UTC
> Please apply this patch, find which acpi_video device backlight sys I/F can
> work for you and attach the result of cat
> /sys/class/backlight/acpi_videoX/acpi_dev/path.
> 

this one is in acpi_video0
\_SB_.PCI0.VID_.LCD0
this one is in acpi_video1
\_SB_.PCI0.AGP_.VID_.LCD0

acpi_video1 is correct and working.
Comment 17 Thomas Renninger 2008-02-25 06:48:20 UTC
I expect the problem is:
  - The external graphics ACPI device is using the traditional method to switch
    brightness, it also works for the internal (Intel) graphics device
  - The internal ACPI graphics device (Intel card) is using a feature that needs
    in kernel graphics driver support.

This needs fixing in BIOS and I expect Lenovo is willing to do this.
Intel should discuss this internally with their graphics drivers programmers and provide a OSI ability string.
Comment 18 Zhang Rui 2008-02-25 19:43:51 UTC
(In reply to comment #17)
> I expect the problem is:
>   - The external graphics ACPI device is using the traditional method to
>   switch
>     brightness, it also works for the internal (Intel) graphics device
We don't have an external graphics card on this laptop, how could the ACPI control methods work? This is a BIOS problem to me.
>   - The internal ACPI graphics device (Intel card) is using a feature that
> needs
>     in kernel graphics driver support.
> This needs fixing in BIOS and I expect Lenovo is willing to do this.
> Intel should discuss this internally with their graphics drivers programmers
> and provide a OSI ability string.
The Intel IGD Opregion Support is on the way.
But it mainly changes the behavior of events handling, like display switch hotkey events, lid close/open and dock/undock. A SCI is fired upon these events, notify the ACPI video device and ACPI video driver notify the Intel video driver to do the actual work by poking the proper OpRegion fields.
IMO, _BCM/_BCL/_BQC methods should work anyway.
I'll look at the AML code later.
Comment 19 Zdenek Kabelac 2008-02-26 13:17:06 UTC
For the Comment #14

Yes it gives back /sys/class/backlight/acpi_video0
however I could read/write values in the brightness 
and they are even reported by tpd - but they have
no effect on the actual brightness of the display.
Comment 20 Zhang Rui 2008-02-28 19:53:19 UTC
to Thomas:
The problem you described in comment #17 is right.
But I wonder if we need a OSI string to fix it. IMO, DRDY flag is enough to tell the BIOS is Intel IGD Opregion is supported.
Comment 21 Zhang Rui 2008-03-10 20:42:34 UTC
This bug can not be fixed until the graphics kernel mode driver can actually work. 
so we should either wait for the graphics driver to work or revert the patch 3fa2cdcc45a0176de15cac9dbf4ed2834ebf8932.
Comment 22 Adrian Bunk 2008-03-11 06:11:41 UTC
This is a regression from 2.6.24 to 2.6.25, so we need a fix now.

If there's no other solution available now a revert seems to be the right solution.
Comment 23 Len Brown 2008-03-12 19:53:33 UTC
Re: comment #7
Chuck, that URL points to a power-pc report,
which can't possibly be related to this issue
as power-pc doesn't have ACPI.

Re: comment #10
Chuck, 3fa2cdcc45a0176de15cac9dbf4ed2834ebf8932
went in at 2.6.25-rc1 and was not included
in 2.6.24.stable
FC8 only recently started shipping 2.6.24,
why does it include that patch?

38531e6fe51ad5c7dfe72e0e066b5f54bc1921cd
ditto
Comment 24 Len Brown 2008-03-17 23:02:49 UTC
3fa2cdcc45a0176de15cac9dbf4ed2834ebf8932 reverted from acpi test tree.
Comment 25 Len Brown 2008-03-18 01:38:16 UTC
0119509c4fbc9adcef1472817fda295334612976 reverted from acpi test tree.

per the comment in bug 9916, it is an earlier version of
3fa2cdcc45a0176de15cac9dbf4ed2834ebf8932 that shouldn't
have been merged.

With these two patches reverted,
/sys/class/backlight/acpi_video1/brightness works fine.
/sys/class/backlight/acpi_video0/brightness does nothing.

ditto for
/proc/acpi/video/VID1/brightness
/proc/acpi/video/VID/brightness

i notice that video isn't auto-loading like it did in 2.6.24 --
which is perhaps an additional regression.

another issue is that the brightness hot key -- presumably
causing an input event, causes brightness to go to 100%.
Unclear what my FC8 user-space is doing with that event....
Comment 26 Thomas Renninger 2008-03-18 07:44:12 UTC
Reverting the patch is wrong and breaks other machines.
ACPI video module has not been used much before ACPI autoloading..., this is not much of a regression. Better do it properly or we go in circles.

Please try to not load the ACPI video driver.
Instead load the thinkpad_acpi driver with:
brightness_enable=1
parameter. Does this work?
If that works, this is the right workaround for T61 with internal/on-board Intel graphics card.

I cannot reopen this one, can someone else do this, pls.
Comment 27 Adrian Bunk 2008-03-18 08:16:41 UTC
(In reply to comment #26)
> Reverting the patch is wrong and breaks other machines.

Does it break any machines where it worked with 2.6.24?

> ACPI video module has not been used much before ACPI autoloading..., this is
> not much of a regression. Better do it properly or we go in circles.
>...

If a revert doesn't make things worse than they were in 2.6.24 the right time for doing it properly is for 2.6.26.
Comment 28 Adrian Bunk 2008-03-18 09:23:05 UTC
now in Linus' tree as commit 77321e624b64f1e5985a20f3cd16b94c96d0dbb4
Comment 29 Thomas Renninger 2008-03-18 09:50:30 UTC
The patch (the reverted one) does not break anything.
Please try the thinkpad driver with:
brightness_enable=1
To be honest I didn't try it myself now, but I am pretty sure it works because it always worked.

The machine is known broken for the video driver (or say unsupported) it needs black listing to not be loaded and this can be done in userspace.

You can also use attached patch to be sure thinkpad_acpi does the brightness switching and video does not.

I also wonder why Henrique, the Thinkpad acpi maintainer is not in CC of this bug or at least asked.
Comment 30 Thomas Renninger 2008-03-18 09:54:44 UTC
Created attachment 15332 [details]
Patch to blacklist thinkpads to use thinkpad_acpi

It's ugly, but it is the correct way to workaround a bug on a specific subset of machines in last minute.
Breaking all others by registering a driver on a device which doesn't even exist, is defintely not.
Oh dear, what a mess.
This one is compile tested only.
Comment 31 Thomas Renninger 2008-03-18 10:18:26 UTC
Either you reopen this one or this one:
http://bugzilla.kernel.org/show_bug.cgi?id=9614 (and possibly some others).
Comment 32 Henrique de Moraes Holschuh 2008-03-18 14:11:21 UTC
For users of Intel Embedded Graphics on ANY thinkpad that does not respond do thinkpad-acpi brightness_mode=1 (pure EC control), which includes the T61:

Repeat after me:
 * I am to use X.org native backlight control!
 * I am to use X.org native backlight control!
 * I am to use X.org native backlight control!
 * I am to use X.org native backlight control!...

ad nauseum.

Current versions of X.org **enforces** this.  It tells the BIOS to stop controlling the backlight levels, in a much lower level than even the ACPI interface.

Using ACPI won't help.  Using thinkpad-acpi might (and I doubt it), but you are on your own if X.org decides to not like it and hard-crash your box.

So, use X.org native backlight control on the T61 with Intel graphics.  Thank you.
Comment 33 Henrique de Moraes Holschuh 2008-03-18 14:13:40 UTC
Err, forgot a note: we still need to blacklist video for those machines if it is causing extra trouble.

But better see if it isn't a bug in the X.org config that is trying kernel mode even when it MUST use native mode, because it found the ACPI video-exported backlight interface.
Comment 34 Lukas Hejtmanek 2008-03-18 14:46:04 UTC
(In reply to comment #33) 
> But better see if it isn't a bug in the X.org config that is trying kernel
> mode
> even when it MUST use native mode, because it found the ACPI video-exported
> backlight interface.

NO! native mode in X.org is broken. On my T61 with intel graphics, thinkpad brightness via thinkpad_acpi works OK (kernel 2.6.25-rc4).

Using the native mode, if I decrease the brightness to 0, I'm unable to return it to any higher value. With kernel mode, this does not happen.
Comment 35 Thomas Renninger 2008-03-19 01:19:00 UTC
What now switches the backlight is the int-10 method of the BCM, BQC, BCL functions of the Nvidia ACPI device (for which no graphics card is plugged in).
The thinkpad_acpi driver can also do this (not sure whether out of the box, it must take the right (or say wrong) VID device). This is still an ugly hack, but at least only for Thinkpads and does not break all others.

> Using the native mode, if I decrease the brightness to 0, I'm unable to
> return it to any higher value.
Why do you open a kernel bug, if you know it's X to blame?

> On my T61 with intel graphics, thinkpad brightness via thinkpad_acpi works OK
> (kernel 2.6.25-rc4)
I cannot see a regression..., this is the recommended way as long as X is broken and we do not have support for this new Intel graphics things.
You can take the blacklisting hack from comment #30 after a test. Lukas could you give this a last test, pls. When you load the video driver you should get a message that brightness control is ignored and it should work out of the box (without a param) when loading the thinkpad_acpi driver). Then you could even get video output switching on console and with vesafb working instead of writing to some arbitrary HW which does not exist.
Comment 36 Thomas Renninger 2008-03-19 01:22:58 UTC
> (not sure whether out of the box, it must take the right (or say wrong) VID
> device)
That was wrong from myself, it does not use the video device at all (for brightness switching), therefore it should always work. Henrique said something about not working brightness_mode=1 in comment #32..., but from Len's and Lukas' comments it just seem to work, right?
Comment 37 Lukas Hejtmanek 2008-03-19 01:48:35 UTC
(In reply to comment #36)
> That was wrong from myself, it does not use the video device at all (for
> brightness switching), therefore it should always work. Henrique said
> something
> about not working brightness_mode=1 in comment #32..., but from Len's and
> Lukas' comments it just seem to work, right?

No, I use brightness_enable=1, I do not use brightness_mode=1. So I cannot tell the latter works.

Though, I wonder why gnome increases or decreases brightness by 2, not just by 1, but it is most probably userspace issue.
Comment 38 Lukas Hejtmanek 2008-03-19 02:04:29 UTC
(In reply to comment #35)
> > Using the native mode, if I decrease the brightness to 0, I'm unable to
> > return it to any higher value.
> Why do you open a kernel bug, if you know it's X to blame?

the kernel bug was opened due to nonfuctional /sys/class/backlight/acpi_video0

> Lukas could you give this a last test, pls. When you load the video driver
> you should get a
> message that brightness control is ignored and it should work out of the box
> (without a param) when loading the thinkpad_acpi driver). Then you could even
> get video output switching on console and with vesafb working instead of
> writing to some arbitrary HW which does not exist.
 
with the patch #30 I got:
[   11.216611] thinkpad_acpi: Lenovo ThinkPad found. Taking control over brightness switching

so it seems it works even if X.org uses the kernel method instead of native one.
Comment 39 Henrique de Moraes Holschuh 2008-03-19 06:08:46 UTC
Better take off the kid gloves.

1. NVidia-GPU-based thinkpads are NOT Intel-GPU-based thinkpads. DO NOT CONFUSE WHAT IS SAID ABOUT ONE WITH THE OTHER. EVER.

2. *IF* you have an Intel GPU (*****NOT***** AN NVIDIA OR ATI GPU), FIX X.org.  It must either be completely out of the way, or working.  And right now X.org is only completely out of the way if it is NOT running.

3. *IF* AND ONLY *IF* thinkpad-acpi brightness_mode=1 *can* control your backlight, you can bypass X.org, ACPI and the BIOS completely and talk semi-directly to the backlight hardware.  In that case, you might be able to work around issues in ACPI video and X.org using thinkpad-acpi.   I DOUBT THIS HOLDS TRUE FOR ANY *61 THINKPAD.

(if it is not clear yet: thinkpad-acpi calls the same crap ACPI video ultimately uses in a ThinkPad, unless it is in brightness_mode=1.  It can be used to check if something hosed ACPI video (e.g. X.org), because if neither thinkpad-acpi nor ACPI video can change brightness, SOMETHING blocked the ThinkPad BIOS itself).

Now, about how to fix it.

Can we, for once, stop destroying any possibility of fixing backlight support and actually start fixing the underlying BUGS instead of band-aiding over it please?  Forget the "last minute" *anythings*.  Fix the real bugs and address the design issues, because the haphazard way this was done till in kernel, distros and X.org created a LOT of design incompatibilities between them.

ACPI Video and X.org HAVE to talk to each other.  X.org must be able to tell ACPI Video driver to go into an "I'm not here" mode, where it just acts as an ACPI notify->ACPI event repeater.  And X.org is NOT to do *anything* to backlight hardware registers UNLESS it disabled ACPI video first (if it is loaded).

X.org hardware drivers needs to be fixed, if they are not controlling backlight properly.  Until X.org is fixed, the user gets broken backlight.  Don't work around it breaking things further in the kernel side, especially while X.org still doesn't know how to tell the kernel to butt off, nor the kernel knows how to be told to go away.  Fix the bug WHERE IT REALLY IS.

ACPI video, if it is indeed not working, needs to be fixed.  BUT YOU WILL NEVER KNOW whether it is working right or not WHILE YOU HAVE X.org RUNNING.

After the above is done, we have a chance of actually doing the right thing in every X.org driver (enable or disable kernel support, use or don't use the kernel backlight interface).  Then, we will have ACPI video being called to *change* brightness when it is right to do so, and with exclusive access to the backlight control.  Or we will have X.org changing the brightness directly in hardware, and ACPI video will know it must *block* (even for reading!) any attempts to access the ACPI AML backlight methods.

And *PLEASE* remove ANY AND ALL backlight key control crap on distros that fucks up thinkpads.  NOTHING in userspace that could be shipped in a distro has ANY business listening to thinkpad-acpi backlight events *and* *acting* on them to *CHANGE* any brightness hardware. EVER.  You will either get events you can use like that through ACPI video, or the thinkpad already did it in firmware.

If you got to know of any backlight key presses through thinkpad-acpi, it can be used for on-screen-display ONLY.  Anything else is a distro bug from hell.  And mapping thinkpad-acpi brightness events to KEY_BRIGHTNESS_* or the HAL equivalent *WILL* *BREAK* *THINGS* because userspace as a rule does NOT know you really mean that as an on-screen-display event instead of "this is an order for you to change the brightness".

Also, if X.org is acting on the brightness keys by itself (ask X.org upstream about it, I don't know for sure), DON'T act on them in distro scripts either.  You will break things just as much, if not MORE than what was done to thinkpad-acpi.

Does *this* makes things a little more clear? Hmm?

PS: by distros I don't necessarily mean SuSE.
Comment 40 Calvin Walton 2008-03-19 09:34:36 UTC
(In reply to comment #32)
> Current versions of X.org **enforces** this.  It tells the BIOS to stop
> controlling the backlight levels, in a much lower level than even the ACPI
> interface.

Hmm? at least with xf86-video-intel-2.2.1, if a kernel backlight control is detected (ACPI video or thinkpad_acpi both I think, but I've only tested it with ACPI video), it disables the native interface and just makes calls to the kernel interface. I don't know whether it still does any nasty low-level stuff even in this mode, tho.
This can be changed at runtime to use the native interface, and it uses it by default if no kernel interface was found, of course.
Comment 41 Henrique de Moraes Holschuh 2008-03-19 11:23:40 UTC
If X.org is disabling the native stuff when trying to use the kernel, that's already something.  It still won't fix all failure modes, though. We really need to be able to let X.org tell the ACPI video device to block all accesses to the ACPI AML for backlight control, *including* those through the sysfs backlight class it exports.
Comment 42 Lukas Hejtmanek 2008-03-19 11:29:14 UTC
(In reply to comment #41)

if I understand correctly the intel driver, it *only* saves and restores backlight values at VT switches. The backlight is controlled by hal daemon using user space scripts. At least in Ubuntu/Hardy. Therefore there arose regression as the hal was unable to controll the backlight via acpi_video0 which was the only device in /sys/class/backlight/.
Comment 43 Henrique de Moraes Holschuh 2008-03-19 17:44:28 UTC
It also allows one to use xbacklight to control backlight level.

Please check if the Ubuntu HAL config is using xbacklight, or if it is trying to access sysfs backlight classes directly.

If HAL is being told to touch sysfs directly, there you have first of the root causes of the breakage.  It won't be the only one.
Comment 44 Calvin Walton 2008-03-19 19:54:55 UTC
'hald' runs as a system daemon with dropped priviledges, and doesn't have any dependency on X, so it can't run xbacklight directly. It would be possible to have a program running as your user connect to it to control the backlight (or even x connect directly), but AFAIK nothing does that yet. I'm pretty sure HAL uses sysfs to enumerate and control backlights currently.
Plus, it is nice to have a way to control the backlight from the command line on a text console without X running.
Comment 45 Zhang Rui 2008-03-19 19:55:21 UTC
(In reply to comment #39)
> Now, about how to fix it.
> ACPI Video and X.org HAVE to talk to each other.  X.org must be able to tell
> ACPI Video driver to go into an "I'm not here" mode, where it just acts as an
> ACPI notify->ACPI event repeater.  And X.org is NOT to do *anything* to
> backlight hardware registers UNLESS it disabled ACPI video first (if it is
> loaded).
Done. :)
"/sys/modules/video/parameters/brightness_switch_enabled" can be used to enable/disable the ACPI video actions upon brightness hotkey events.

> Fix the bug WHERE IT REALLY IS.
> ACPI video, if it is indeed not working, needs to be fixed.  BUT YOU WILL
> NEVER
> KNOW whether it is working right or not WHILE YOU HAVE X.org RUNNING.
> After the above is done, we have a chance of actually doing the right thing
> in
> every X.org driver (enable or disable kernel support, use or don't use the
> kernel backlight interface).  Then, we will have ACPI video being called to
> *change* brightness when it is right to do so, and with exclusive access to
> the
> backlight control.  Or we will have X.org changing the brightness directly in
> hardware, and ACPI video will know it must *block* (even for reading!) any
> attempts to access the ACPI AML backlight methods.
Yes, currently users can still access the ACPI backlight methods via the backlight sys I/F, even with "brightness_switch_enabled=N".
Another switch is needed to control the access via backligh sys I/F.
Comment 46 Zhang Rui 2008-03-19 20:00:18 UTC
(In reply to comment #45)
>>  Or we will have X.org changing the brightness directly in
> > hardware, and ACPI video will know it must *block* (even for reading!) any
> > attempts to access the ACPI AML backlight methods.
> Yes, currently users can still access the ACPI backlight methods via the
> backlight sys I/F, even with "brightness_switch_enabled=N".
> Another switch is needed to control the access via backligh sys I/F.

we don't have this problem for Intel GPUs with IGD OpRegion support.
The AML backlight methods won't change the backlight directly but try to notify the graphics driver instead. :)
Comment 47 Henrique de Moraes Holschuh 2008-03-19 20:56:16 UTC
The only problem is that we can't trust the AML methods to begin with :-) And that we soon will need the same functionality on many ATI cards...

IMO, we better just go for the complete job.
Comment 48 Thomas Renninger 2008-03-20 04:58:06 UTC
Do we all agree that Matthew's patch should get reactivated for 2.6.25?

This also is not a ThinkPad but IGD specific problem.
The problem with IGD is that we cannot determine whether it is used or not.
The general (not only ThinkPad) way to work around this is by black-/whitelisting (Yes blacklisting is the worst we can do/have, but does anyone have a better idea?) and set brightness_switch_enabled=0 through a dmi list in video.c. Or call it or at least use it like brightness_enabled=0 and video.c should not touch any of the brightness functions then?
> The AML backlight methods won't change the backlight directly but try to
> notify the graphics driver instead.
Hmm, maybe we do not even need the blacklisting in video.c? If nothing happens when video.c tries to switch.., but it sounds dangereous.
Unfortunately my patch to keep a global variable for brightness, video,... support to coordinate video driver vs native driver access has not made it in.
This one could get an additional flag IGD and video would set brightness_enabled=0 and thinkpad acpi and others do use brightness_control=1.
This is the same method Windows is doing it I expect?
I expect XP does not support IGD (even the BIOS does...), but is still switching brightness through propriatery drivers, right?

The next problem: X-Apps needed for switching is another one.
Hal should probably be able to do this all by itself without X running, but how this should finally be done or get workarounded on short term needs some userspace programmers to decide.
IMO it would be great if ThinkPad ACPI could still do the switching and all works as it worked before and how it should work: through /sys/../backlight.
On long-term (would be nice to know when this is planed to be supported) we get IGD support in kernel and nothing changes for userspace apps, right?
Comment 49 Matthew Garrett 2008-03-20 05:45:00 UTC
No, it should be reverted for 2.6.25 - it causes a clear functional regression, regardless of whether or not it worked by design or accident before.
Comment 50 Thomas Renninger 2008-03-20 06:35:47 UTC
Hmm, AFAIK even in the last RC fixing is preferred over reverting.
And patch from comment #30 fixes the issue.

If the patch is reverted, the driver should be marked experimental or broken, that's what it is then: known broken.
Comment 51 Thomas Renninger 2008-03-20 06:45:48 UTC
BTW, I explained this whole ThinkPad IGD problem with the ACPI video driver more than 4 months ago:
http://www.mail-archive.com/linux-acpi@vger.kernel.org/msg10476.html
(Take the last paragraph).
Comment 52 Adrian Bunk 2008-03-20 08:54:27 UTC
(In reply to comment #50)
> Hmm, AFAIK even in the last RC fixing is preferred over reverting.
>...

That's wrong.

Fixing can introduce new regressions.

If reverting makes the kernel not worse than 2.6.24 then reverting is the preferred solution.
Comment 53 Jesse Barnes 2008-03-20 15:50:44 UTC
Zhang, this bug scares me a little.  It sounds like the OpRegion based interface is correctly giving us hotkey notifier events, but not giving us a way to actually change backlight levels.  Is there a separate set of ACPI methods we need to use to actually change the backlight brightness?  Are we supposed to use the LBB & BLC_PWM_CTL regs?  Won't that conflict with the external controller in these machines?

I think you and Hong have tested this on T61s, so I'm curious to hear what you've found...
Comment 54 Zhang Rui 2008-03-24 02:09:55 UTC
This is a duplicate of 9614.
Comment 55 Len Brown 2008-03-25 20:35:14 UTC
This report was filed against the acpi video driver regression
on t61 due to the patch for bug 9614.  That (pair of patches)
was reverted between 2.6.25-rc6 and 2.6.25-rc6-git8,
so this report is now closed.
Comment 56 Sam Morris 2008-05-28 07:13:49 UTC
(In reply to comment #27)
> (In reply to comment #26)
> > Reverting the patch is wrong and breaks other machines.
> 
> Does it break any machines where it worked with 2.6.24?

Reverting 3fa2cdcc45a0176de15cac9dbf4ed2834ebf8932 causes Samsung Q45 laptops to freeze up when the video kernel module is loaded. See bug 10448.
Comment 57 Matthew Garrett 2008-05-28 07:22:45 UTC
Created attachment 16308 [details]
Add opregion support to Intel DRM

This patch adds opregion support to the Intel DRM driver, which fixes brightness key control on the T61 even with the remove-duplicate-drivers diff. I need Hong at Intel to confirm the copyright and licensing before it can go upstream.
Comment 58 Erik Boritsch 2008-07-03 03:38:33 UTC
After applying this patch I get following if I try to compile kernel 2.6.25.7:

make[3]: *** No rule to make target `drivers/char/drm/i915_opregion.o', needed by `drivers/char/drm/drm.o'.  Stop.
make[2]: *** [drivers/char/drm] Error 2
make[1]: *** [drivers/char] Error 2
make: *** [drivers] Error 2 

I've got no errors applying the patch. Makefile is changed too.

Note You need to log in before you can comment on or make changes to this bug.