Bug 11750 - [thinkpad-acpi] for t43 should be set brightness_mode=1
Summary: [thinkpad-acpi] for t43 should be set brightness_mode=1
Status: CLOSED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Platform (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Henrique de Moraes Holschuh
URL:
Keywords:
Depends on:
Blocks: 56331
  Show dependency tree
 
Reported: 2008-10-13 03:53 UTC by Oleksij Rempel (fishor)
Modified: 2013-04-09 06:23 UTC (History)
2 users (show)

See Also:
Kernel Version: 2.6.12 and all later kernels
Subsystem:
Regression: No
Bisected commit-id:


Attachments
dmesg (EC & NVRAM) (43.05 KB, text/plain)
2008-10-13 03:54 UTC, Oleksij Rempel (fishor)
Details
Change defaults for IBM thinkpads to brightness_mode=2 (1.39 KB, patch)
2009-03-12 18:47 UTC, Henrique de Moraes Holschuh
Details | Diff

Description Oleksij Rempel (fishor) 2008-10-13 03:53:32 UTC
Latest working kernel version:
Earliest failing kernel version:
Distribution:ubuntu 8.10
Hardware Environment:thinkpad t43
Software Environment:
Problem Description:


By default thinkpad-acpi use brightness_mode=3 (EC & NVRAM) for all thinkpads. But this not working correctly with thinkpad t43, brightness is controlled by hotkeys but not by userspace tools. instead brightness_mode=1 (EC only) should be set to work properly. So after this brightness can be controled bay hotkeys and userspace tools.
Comment 1 Oleksij Rempel (fishor) 2008-10-13 03:54:41 UTC
Created attachment 18284 [details]
dmesg (EC & NVRAM)

dmesg with default setting, EC and NVRAM used.
Comment 2 Zhang Rui 2008-10-28 01:50:03 UTC
hmm, need a dmi check?
re-assign to henrique. :)
Comment 3 Henrique de Moraes Holschuh 2008-10-28 04:12:34 UTC
This is strange, and doesn't match my T43 (which wants either 1 or 3, EC or EC+nvram).  Without nvram, it won't store the brightness across sleep/shutdown.

Attach the output of dmidecode, please.  Remove UUIDs and serial numbers.
Comment 4 Henrique de Moraes Holschuh 2008-10-28 04:24:57 UTC
Never mind, found the data I needed in the dmesg.  You have an Intel graphics T43, yes, that's quite different from my ATI one.

Your talk about "controlled by userspace" worries me. Let's find out WHAT should be controlling the brightness in your thinkpad for real before we go any further, shall we.

Boot in SINGLE USER MODE.  No X.org.  No HAL.  No thinkpad-acpi (just unloading it should be OK for this test on a T43).   Check the hotkeys.  Do they work?

Now load thinkpad-acpi, brightness mode 1.  Check /proc/acpi/ibm/brightness (or the sysfs backlight class).  Does it work?   Do the hotkeys still work?

Now unload thinkpad-acpi, and reload it with brightness mode 3.  Repeat the two tests above.

The expected output (based on ATI T4x) is that hotkeys ALWAYS work, and that mode 1 and 3 work equally well in single user mode.
Comment 5 Oleksij Rempel (fishor) 2008-10-28 13:02:53 UTC
hmm... with latest ubuntu kernel i can't reproduce this issue any more. all optionc have almost same result. 
One strange issue was 
with this file /sys/class/backlight/thinkpad_screen/brightness .
it is not updated if hotkays or proc to change was used  brightness and if it differs to real brightness you can't change it with sys class. If brightness is same this interface working too.
Comment 6 Henrique de Moraes Holschuh 2008-10-29 03:57:06 UTC
Well, here's how it works:

1.  Feedback loops of the brightness keys into thinkpad-acpi is NOT supported. You do it, things break.  There are races with the firmware, and these cause weird things.  You do such a feedback loop with CMOS disabled, things break LESS (the race window is much shorter) but that doesn't mean they work right.  Ubuntu used to to this.  I don't know if they still do.

You set up a feedback loop if you ever touch /proc/acpi/ibm/brightness or the sysfs backlight class for thinkpad-acpi in respose to a brightness key event from thinkpad-acpi.

2.  Thinkpads with EC backlight control don't have a "disable automated key behaviour" switch in the BIOS (the hotkey_mask bits are ignored by the ACPI DSDT, unfortunately). This means you are NOT ALLOWED to defer to userspace the backlight handling. I mean it.  The only reason I let people enable these bits in the first place is because you CAN use them for OSD as long as you don't try to actually change the brightness (the firmware will be doing it by itself).

So it really boils down to whether your T43 18xx has EC-based backlight brightness handling or not.  The T43 26xx (with ATI GPU) has EC-based backlight brightness control. If /proc/acpi/ibm/brightness works on your T43 18xx in purely EC mode (without CMOS), yours is EC-based as well.

/sys/class/backlight/thinkpad_screen/brightness is used to request brightness changes.  You need the actual_brightness (or something like that) to get the current real value.  That's normal for the backlight class.

Can I close this bug?
Comment 7 Oleksij Rempel (fishor) 2008-10-29 06:35:21 UTC
I think not.
I will try to explayin in other way:
$cd /sys/class/backlight/thinkpad_screen/
$cat actual_brightness
will give -> 4
$cat brightness
will give -> 7
$echo 1 > brightness
nothin will be changed, steal 4 ---> this is a bug. may be it coused this issue by some userspace apps.

now i will set brightness to 7 with hotkay or true /proc/
$cat actual_brightness
will give -> 7
$cat brightness
7
$echo 1 > brightness
now acctual brightness is 1... if you continue to use only /sys/class/backlight/thinkpad_screen/brightness every thing is ok, until you use one time hotkey.
Comment 8 Oleksij Rempel (fishor) 2008-10-29 06:37:25 UTC
it's working only if /sys/.../actual_brightness == /sys/.../brightness
Comment 9 Len Brown 2008-11-12 01:05:13 UTC
i don't suppose that brightness works when the acpi video driver
is used instead of thinkpad-acpi?
Comment 10 Oleksij Rempel (fishor) 2008-11-12 01:48:02 UTC
no, acpi video do not working.
Comment 11 Henrique de Moraes Holschuh 2008-11-12 03:25:27 UTC
No, ACPI can't help.  The T43 has two variants:  Intel graphics, and ATI graphics.

It doesn't have *any* ACPI standard-based brightness control, it has just
direct EC control or some proprietary mess that we haven't all the quirks
reverse engineered, yet.

The ATI graphics variant, IBM ThinkPad T43/p model 26xx, has EC-based brightness control.  You adjust an EC register, that makes it select an internal table of parameters to send to a PWM channel that drives the inverter for the CCFL.  This requires pure-EC, or EC+UCMS ("CMOS") control of the backlight, performed by thinkpad-acpi.

On the Intel graphics variant, it could be either EC-based or GPU-based. Nobody offered to help me find it out yet, and the test is simple...

Other thinkpads with ATI or Intel graphics might need just the CMOS mode and do the EC programming in SMM in response to the SMI trap generated by the "CMOS mode" (which, in the T43 26xx, just updates the NVRAM as far as I can see... but we might be missing some magic that would cause it to behave like the T42).

I would really need to add intelligent state-machine-based autodetection for this, or I'd need extensive cooperation from users of many models of old ThinkPads to do it without the autodetection.  It is not simple, and it is error-prone.

For autodetection, one blacklist every Lenovo thinkpad to require ACPI or use the CMOS method (none of them seem to need EC).  Then one needs to check if CMOS and EC matches. If not, use CMOS (and inform users to notify by email if broken).  If they match, lock down all brightness control from userspace, and issue a CMOS command to increase brightness.  If about 1s later the EC register changed, use just CMOS.  Otherwise, sync EC and use CMOS+EC.  Unlock down brightness control.  Inform user of the autodetected mode, and request information if it doesn't work.

As you see, it is sort of fragile and not "transparent" to the user (it WILL need to try to mess with the backlight level).  It is in my TODO list, but at a low priority.
Comment 12 Zhang Rui 2008-12-16 21:22:19 UTC
henrique,
do we have a patch to fix it?
Comment 13 Henrique de Moraes Holschuh 2008-12-17 07:15:21 UTC
Not yet.  The current solution is for the user to override the automatically detected config using module parameters.

I will have to do some "user pool" to find out which config works best as a default.  It is easy enough for me to whitelist the 26xx T43 to mixed mode, and the 18xx T43 to just UCMS.  But I lack data for ever other IBM thinkpad (!).
Comment 14 Shaohua 2009-03-03 21:28:32 UTC
what's the status of the 'user pool'? Can we just push the list you know first and add others later? Sure this must not break systems immediately.
Comment 15 Zhang Rui 2009-03-11 00:45:00 UTC
hmmm, ping Henrique. :)
Comment 16 Henrique de Moraes Holschuh 2009-03-11 05:53:13 UTC
Writing the code today... will switch all models to UCMS, except for the TP-1Y BIOS (some T43, R52).

This might cause regressions, so it is best tested in the thinkpad-acpi out-of-tree early-adopters version before I send it towards mainline.
Comment 17 Henrique de Moraes Holschuh 2009-03-12 18:42:43 UTC
Ok, code is ready to set all models to mode 2 (UCMS), but keep the ATI-based T43 at mode 3 (UCMS + EC).

But reading this bug report, I can't seem to understand if mode 2 (UCMS) works fine on Alexey's Intel-based T43.

Alexey, can you tell me how it behaves when you start thinkpad-acpi with the "brightness_mode=2" parameter?

Does it let you control brightness properly through the /proc interface?
   Please test it both in single-user, text mode, and also normally in X.org

Do the brightness keys work?
   Please test it both in single-user, text mode, and also normally in X.org

Does it lose sync with the current brightness when you use the keys?  If it loses sync when in X.org, does it also happens when in single-user, text mode?

I will attach the patch to change defaults to this bug report, but it is not needed in any way to perform the above tests.
Comment 18 Henrique de Moraes Holschuh 2009-03-12 18:47:35 UTC
Created attachment 20511 [details]
Change defaults for IBM thinkpads to brightness_mode=2
Comment 19 Oleksij Rempel (fishor) 2009-03-13 00:24:36 UTC
With latest git. brightness_mode=2 is out of sync for me. The kays working in all modes, but userspace working only in brightness_mode=1. In mode 2  i can monitore changes in /proc/acpi/ibm/brightness but i can't change the real brightness. In mode 1 it's working perfektly. In mode 3, there is some glitches. 
Comment 20 Henrique de Moraes Holschuh 2009-03-18 15:46:25 UTC
That patch is no good then.

Well, I have (probably) fixed it in a very different way.

I removed mode 3, wrote a new mode 3 that does only ec control (like mode 1, which we know to work well from your reports) but stores the end result in NVRAM on shutdown.   I have switched every thinkpad that knows what the EC backlight brightness register is to the new mode 3 by default, and all others to mode 2 (UCMS/CMOS).

That should fix it, as it simply removes any races with the firmware in mode 3 (they might clobber each other, but they won't race).  UCMS is probably race-free, since it is atomic (uses SMM).

It is a big patch, it doesn't make sense to add it to this bug report and it requires a few other patches not in mainline yet.  Instead, I will release soon a new BETA version of thinkpad-acpi through ibm-acpi.sf.net, and request testers and reports in the linux-thinkpad ML.

After it gets some field testing, I will send it to mainline (probably for 2.6.31, since I doubt I will have enough reports to have it ready for the 2.6.30 merge window).
Comment 21 Henrique de Moraes Holschuh 2009-03-24 17:35:29 UTC
Please test the latest thinkpad-acpi beta version, available at:

http://sourceforge.net/project/showfiles.php?group_id=117042&package_id=230205

And tell me if it fixes your brightness problems.
Comment 22 Henrique de Moraes Holschuh 2009-03-24 17:56:47 UTC
Comment on attachment 20511 [details]
Change defaults for IBM thinkpads to brightness_mode=2

This old patch won't help at all.
Comment 23 Oleksij Rempel (fishor) 2009-03-25 14:03:08 UTC
This patch working fine for me.

Thank you.
Comment 24 Henrique de Moraes Holschuh 2009-03-25 15:04:44 UTC
Closing bug as CODE_FIX.

Note that, however, I don't know how much time it will take until I can send the patches to mainline as nobody in the linux-thinkpad ML seems to be in the mood to cooperate and report if it didn't break their non-T43 thinkpads...
Comment 25 Len Brown 2009-04-02 04:32:43 UTC
A bug report is only "CLOSED" when the patch makes it upstream.

So RESOLVING" as "CODE_FIX" is what you mean.
This means that there is a patch available to test.

That said, exactly where is the exact patch to test?
Comment 26 Henrique de Moraes Holschuh 2009-04-02 19:28:21 UTC
In the thinkpad-acpi devel git tree, which I will eventually send to you for acpi-test.

I didn't get enough reports back to feel safe enough to send it to you so far.  It looks like it might well have to wait for the next merge window.  It doesn't help that rfkill is being rewritten, and I have to pay attention to that first, otherwise I will block a lot of stuff.

Anyway, it is _not_ a simple small patch, I rewrote 70% of the offending code, and it depends on other (non-critical) patches from my queue.  I could send it all for you by Sunday, HOWEVER, I am not sure it will not regress just about every IBM ThinkPad other than the T43.

Direct link to the patch (for reference, only):
http://repo.or.cz/w/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git?a=commit;h=b6ead2d2ce5c19f9281de1ecee93b2117aab7f62

(the above tree is based on 2.6.28.9 + rkill backport, you don't want to pull from that).
Comment 27 Henrique de Moraes Holschuh 2009-04-02 19:36:58 UTC
PS: the patches in that three are not the final form of what I am considering to submit.  It looks like I didn't push my latest tree, and I am away from my devel boxes right now.
Comment 28 Len Brown 2009-04-07 02:56:45 UTC
shipped in 2.6.30 merge window (2.6.29-git14)

closed.

commit 0e501834f8c2ba7de2a56e332d346dcf4ac0b593
Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Date:   Sat Apr 4 04:25:53 2009 +0000

    thinkpad-acpi: rework brightness support

    Refactor and redesign the brightness control backend...

    In order to fix bugzilla #11750...

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