Bug 81691

Do you have thinkpad_acpi loaded?

Yes, it's built into the kernel.

From dmesg:

[    0.610451] thinkpad_acpi: ThinkPad ACPI Extras v0.25
[    0.610452] thinkpad_acpi: http://ibm-acpi.sf.net/
[    0.610452] thinkpad_acpi: ThinkPad BIOS 83ET76WW (1.46 ), EC unknown
[    0.610453] thinkpad_acpi: Lenovo ThinkPad T420, model 4180PH1
[    0.610703] thinkpad_acpi: detected a 16-level brightness capable ThinkPad
[    0.610825] thinkpad_acpi: radio switch found; radios are disabled
[    0.610896] thinkpad_acpi: This ThinkPad has standard ACPI backlight brightness control, supported by the ACPI video driver
[    0.610896] thinkpad_acpi: Disabling thinkpad-acpi brightness events by default...

The dmesg messages for thinkpad_acpi do not change, regardless of video.use_native_backlight parameter.

Please attach acpidump and full dmesg without using any backlight related cmdline option, thanks.

BTW, which GUI are you using? If you have acpi_listen installed, please run it and then press the Fn key to see if any events get emitted.

Created attachment 145551 [details]
dmesg, xz-compressed

Created attachment 145561 [details]
acpidump, xz-compressed

Here you are.

acpi-listen tells me 
video/brightnessup BRTUP 00000086 00000000
video/brightnessdown BRTDN 00000087 00000000
so that looks good.

The gui is fluxbox, acpid is active
(but actually has just rules for the lid and power button).

When you use video.use_native_backlight=0 to have everything work, do you need any extra cmdline option that is related to ACPI/backlight? e.g. do you need acpi_osi="!Windows 2012"?

No, video.use_native_backlight=0 is all it requires.

The full commandline I tested with is 
i915.enable_rc6=7 root=/dev/sda2 i915.fastboot=1

I also tested without the fastboot option, it does not help.

So the problem is: when the hotkey is pressed, the event is generated but no one is responding to it(the GPU driver doesn't and your GUI doesn't either). And surprisingly, your Win8 compatible firmware has a working ACPI backlight interface so that when that interface is available, your hotkey works in that the ACPI video module responded to the event.

Need to think about how to solve this.

I just remembered Hans is planning to add this functionality into acpid:
http://marc.info/?l=linux-acpi&m=140549974416192&w=2
Is that acceptable to you?

Let me get up to speed here: The native backlight interface is not even _supposed_ to handle brightness control events from ACPI? ACPI just signals the events to userspace and is done with it?

Also, to avoid any confusion with "Ẃindows 8 compatibility": Note that the T420 predates Windows 8, and while it can run Win8 in compat mode, the bios is not secure boot capable, among other things. It's an EFI 1.x implementation. So this is _not_ a proper "Windows 8" machine, as in Microsoft would not allow shipping this box with Windows 8, AFAIK.

So I am somewhat surprised that this box is categorized just like the "real" Win8 machines. Perhaps that definition is too broad?

Anyway, in regard to Hans' proposition of fixing this in acpid, I cannot really offer much beside bikeshedding... I really think the kernel should just continue handle the brightness events by itself, as it "always" did. 

If it is supposed to be userspace's job to control brightness, Hans' proposal sounds good to me. The acpid needs to poll the nvram for the brightness keys anyway.  Relying on the DE to handle the brightness keys falls short as it doesn't work on consoles (I have fluxbox handle brightness events on my XO laptop, and find this annoying).                          

So, _if_ I understand you correctly, I will just go ahead and keep the video.use_native_backlight=0 in the command line. That's fine for me unless that's supposed to go away too, or be allowed to break.

I may want to switch to Hans' proposed solution once that has trickeled through to Debian stable userspace (I am limited to that or SLES at work), but wether this machine will live to that day and age is anybody's guess. So, really, I do not think a fix in userspace will help me much for years down the road.

That said, I will happily try acpid patches, of course.

Hi Andre,

Let me try to explain what is going on here.

The kernel has aprox. 40 or so different backlight drivers (only counting x86 drivers), acpi-video is a bit of an outlier compared to all the other drivers, since it is the _only_ one which corresponds to brightness keypresses all by itself. All the other drivers do not do this, and instead rely on userspace to respond to keypresses (*).

With the advent of win8 laptops, it turns out that many (most) new laptops now have a broken (non functional) acpi-video backlight interface. So in 3.16 we've disabled acpi-video for all laptops with a BIOS claiming win8 support, switching them over to use a native (e.g. intel, nvidia or ati) backlight interface instead.

This has the unforunate side-effect that for people who have a laptop which claims win8 compat and has a working acpi-video driver and who are not using a desktop environment like gnome, kde or xfce, that the brightness keys stop working since they were dependent on the acpi-video's backlight driver keypress handling.

The solution for this is obvious from a technical pov, acpi key events like pressing the power or suspend key are normally handled by acpid, so the natural thing to do is to make acpid also respond to brightness keypresses. This has the added advantage that this will also make brightness keys work for non gnome, etc, users, who could not use acpi-video on their laptop to begin with.

As for using video.use_native_backlight=0 as a workaround, yes that is fine, please do that, and that should keep working for the foreseeable future.

Regards,

Hans


*) Which matches the behavior of the volume keys, power and suspend keys, etc. acpi-video really is a weird exception here, normally the kernel never automatically reacts to keypresses.

Thanks for the clear explanation Hans.

One more thing about the 'Win8 compatibility' thing: we decide if this is a Win8 compatible machine by checking if the BIOS queries win8 support in ACPI table by doing a _OSI("Windows 2012") call, that at least means the machine's BIOS thinks it may be dealing with a Win8 OS, which also means it is Win8 aware. This doesn't necessarily mean the laptop will be running Win8 actually. So 'Win8 compatibility' here means if the BIOS is Win8 aware.

> --- Comment #11 from Hans de Goede <jwrdegoede@fedoraproject.org> ---
> Hi Andre,
>
> Let me try to explain what is going on here.
>
> The kernel has aprox. 40 or so different backlight drivers (only counting x86
> drivers), acpi-video is a bit of an outlier compared to all the other
> drivers,
> since it is the _only_ one which corresponds to brightness keypresses all by
> itself. All the other drivers do not do this, and instead rely on userspace
> to
> respond to keypresses (*).

Ah! Oh! I really did not realize acpi-video is unique in setting brightness.
I assumed the intel_backlight driver was _supposed_ to up/down brightness on ACPI events. But did not, and so had to fall back to acpi-video.
Ideally, that'd get fixed in intel_backlight, acpi-video can then retire.
Thus was my reasoning. My reasoning was flawed.
I stand corrected. Thanks!

> With the advent of win8 laptops, it turns out that many (most) new laptops
> now
> have a broken (non functional) acpi-video backlight interface. So in 3.16
> we've
> disabled acpi-video for all laptops with a BIOS claiming win8 support,
> switching them over to use a native (e.g. intel, nvidia or ati) backlight
> interface instead.
>
> This has the unforunate side-effect that for people who have a laptop which
> claims win8 compat and has a working acpi-video driver and who are not using
> a
> desktop environment like gnome, kde or xfce, that the brightness keys stop
> working since they were dependent on the acpi-video's backlight driver
> keypress
> handling.

I thought quite a few people would rather not have their DE run acpi controls? But really, I may be misguided by the state of the art a decade ago.
Which was when I ditched all of it and wrote my own few lines of acpid rules and scripts. Which worked just fine between then and now. I will just update them.

> The solution for this is obvious from a technical pov, acpi key events like
> pressing the power or suspend key are normally handled by acpid, so the
> natural
> thing to do is to make acpid also respond to brightness keypresses. This has
> the added advantage that this will also make brightness keys work for non
> gnome, etc, users, who could not use acpi-video on their laptop to begin
> with.

Yes, I can see that as the way to go.
Although I would still find it appealing if the kernel would handle brightness all by itself, so you can run random userspace and have these buttons work. For me, they are the most-used ACPI-buttons by a large measure. Once the acpid adaptation has trickled through to the slow distros, this argument magically disappears. Almost.

> As for using video.use_native_backlight=0 as a workaround, yes that is fine,
> please do that, and that should keep working for the foreseeable future.
>
> Regards,
>
> Hans
>
>
> *) Which matches the behavior of the volume keys, power and suspend keys,
> etc.
> acpi-video really is a weird exception here, normally the kernel never
> automatically reacts to keypresses.

@Aaron:
Yes, I think I see now. 
The native interface works as intended,
but I did not get it's intentions right.
So there is no need to tell it apart.

Created attachment 146531 [details]
acpid script to change intel_backlight using the brightness keys

So I extended my default.sh script to set intel_backlight via brightness keys.

A cursory search did not bring up any examples that actually work right, (as in not reaching min and max, writing illegal values, or both) so I decided to post mine. Hope it helps.

It should be sufficiently generic to handle any intel_backlight.
Changing /sys/class/backlight/intel_backlight/[max_]brightness to 
/sys/class/backlight/*/[max_]brightness also should be all it takes to make it driver-agnostic as long as there's just one backlight source.

I seem to have the same issue on my HP Compaq 6510b notebook, Intel IGP GMA X3100. FN Keys are rendered useless with 3.16.
I'm on Arch x64 - downgrading to kernel 3.15.8 fixes the issue. I've not tried if video.use_native_backlight=0 fixes it on 3.16 yet, but I will try it out soon.

Should I provide some logs here or open a new issue?
Please tell me what you need to debug it, I'll happily provide anything you need.

I've just noticed: after a standby(STR) the keys work! Just not after a fresh boot. video.use_native_backlight=0 does not change that, same behaviour.
Other FN-keys seem to work fine (Standby/STR).

I'm using Cinnamon as a DE without acpid.

I should also point out that this laptop is from 2007, so "Win8 compatibility" should not be an issue? It was built for Vista... ;-)

So you are looking at a different issue. 

Here, the FN-Keys always work (as shown by acpi_listen), the native backlight just needs to be prodded from userspace to switch brightness, while that was not needed for the acpi backlight driver.

Regarding "Win8 compatibility": My laptop also predates Windows 8. 
It was a bios upgrade that made my bios advertise the capability flag.
Which only allows running Win8 in legacy mode, btw, so that does not 
say much about the HW requirements for "native" Win8 operation.

Well, the last BIOS update for this machine came out long before W8 came along (2009).. I don't think it will advertise a capability flag.
Thanks anyway, I'll open a new issue then :)

@Andre: It looks like the issue has been resolved, do you agree?

At least it's "works for me", and on two different ways.
The "functional" regression bit (as in the native bl drivers do less than
the acpi one did) looks unavoidable and I'm fine with that.

But if I did understand Aaron right in Comment 9 https://bugzilla.kernel.org/show_bug.cgi?id=81691#c9, Hans wanted to teach the acpid in userspace 
to handle the backlights by itself (i.e. without providing a helper script like the one I wrote). If that is the case, this bug should stay open until that 
code is released.

I may have misunderstood that one, though. (And thinking about it, I do get my doubts :-) Then this can be closed alright.

Anyway, I've got no strong feelings one way or the other.

Thank all of you for looking into this,
Andre

(In reply to Andre Müller from comment #21)

> Hans wanted to teach the acpid in userspace 
> to handle the backlights by itself (i.e. without providing a helper script
> like the one I wrote).

Correct.

> If that is the case, this bug should stay open until
> that code is released.

Not sure if acpid functionality should be tracked in the kernel bug tracker though :)

Regards,

Hans

Not even if that future acpid bit fixes the kernel's functional regression?

Anyway, I seem to be rather alone with my view, and I don't want to be thick about it.

So I'll resolve it "'code fix' coming soonly"...? Feel free to change the status
if that's not the thing to do.

Thanks all,
Andre

Andre wrote:
> Anyway, I seem to be rather alone with my view, and I don't want to be
> thick about it.

Andre, you are not alone with your view!
I still think that is user space breakage. Before it worked even on the console while the kernel was booting, now help from a demon or the window manager is needed and it will not work during boot.

A lot of Laptop owners with a none-compliant Windows 8 bios will be hurt for a long time by using any kernel newer than 3.15 until distributions come out with a proper fix.

B.t.w. what is the rational of the other backlight drivers _not_ doing the backlight level setting by themself? At least on a notebook with display and keyboard as one physical unit this seems counterintuitive to me.

[a couple off-bugzilla mails later]

So, I'm reopening.

The argument that most people use a DE and the DE handles backlights is doubly flawed in my view. The DE should refrain from doing so, as it will not handle any other VT, plus _if_ there is a sane, system-wide handler, it will double the stepping.

There does not seem to be disagreement about this one, as Hans intends to teach the acpid, and that will reintroduce this issue (unless some racy hackery a la "wait for a little and if nothing happens, go ahead" is introduced). Nevertheless, this will hurt until the acpid version to come is used universally. But really, not all userspace _has_ an acpid in the first place.

While I don't know the rationale as to why acpi does not write to the native backlight interfaces, I suppose that this doubling, and then potential tripling (when the acpi_backlight is active as well) shall be avoided. Plus, the keypresses need to go to userspace anyway for anyone fancying on-screen messages. Also, some userspace may have it's own ideas about the brightness stepping and threshold values. Finally, a brightness sensor may also be consulted for setting the screen brightness.

Hans also argues that the brightness keys are the only keys the kernel (potentially) handles by itself, and getting rid of this adds to consistency. While I can see that, in my opinion the exception for the brightness keys is a well-chosen one: The brightness keys are the most-used acpi keys, and the most important ones at that. Not depending on any userspace conditions really is a huge win here. Also, I think it is the only event the kernel actually _can_ handle by itself without any userspace interaction. There is not much of befuddling the kernel with policy either, as it should just go from min to max in roundabout 15 steps, that is some sane convention that is in-kernel anyway.

Now, I was reluctant to insist here, as just reverting to the prior state using two backlight drivers (acpi and intel) left me with buggy cornercases anyway. E.g. everytime I switch to a console, the backlight actually is off, while the acpi driver thinks it's at maximum. A brightness event later the backlight is adjusted according to the acpi driver's (unapplied) state, in effect switching from off to max. So I can easily see that having two drivers compete for control of one device is an awful thing to have. In effect, I would go back to disabling the acpi backlight anyway if this got reverted.

Also, I did not want to insist in my views, as I can't provide any patches; that's beyond me.

Nevertheless, with a bit of prodding by Uwe, and not being thrilled myself with the perspective of being bitten by this for any userspace that does not handle this (when acpid is missing, inactive or not new enough) let me suggest the solution I would think is the right thing to do:

- The kernel (the ACPI subsystem) should handle acpid keypresses (as it did for the acpi backlight) by default and adapt the _native_ backlight interface's brightness by a value derived from the current and maximum brightness values (see below).

- There should be a switch in sysfs (one? or by device? the target is the laptop builtin screen, which tends to be one...) where userspace can toggle the kernel's own brightness handling on or off. That should default to on to match previous behaviour. So, if userspace wants to be in charge and do it's own voodoo, it can do so without any double stepping madness ensuing. (Alternatively, that could be a compile-time or boot-time switch, but that would leave distro kernels in a somewhat awkward position and fall short for multi-user machines.)

Thanks,
Andre

To round this off, here's a suggestion for generic brightness stepping (in bash) that should work with any single device:
 
sys_bl="/sys/class/backlight/.../brightness"
read max_brightness < /sys/class/backlight/.../max_brightness
read bl_current < $sys_bl

step_f=1.25
step_c=`echo "$max_brightness / 100" | bc`
[ $step_c = 0 ] && $(step_c=`echo "$max_brightness / 15" | bc`; step_f=1)
[ $step_c = 0 ] && step_c=1

# step_c is a constant to use in stepping
# step_f a factor for the same purpose.
# Here, max_brightness is 4437. With a constant stepping, the effect on
# brightness is too big for small values, and smallish for large values. Hence # I introduced a factor (which by itself shows the opposite problem).

bl_hi_cut=`echo "$max_brightness / $step_f - $step_c"`
bl_lo_cut=`echo "$max_brightness * $step_f" | bc | cut -d. -f1`

# [...] brightness up event
if [ $bl_current -gt $bl_hi_cut ]; then
        echo $max_brightness > $sys_bl
elif [ $bl_current -lt $bl_lo_cut ]; then
        echo $bl_lo_cut > $sys_bl
else    echo "$bl_current * $step_f + $step_c" | bc | cut -d. -f1 > $sys_bl
fi

# [...] brightness down event
if [ $bl_current -le $bl_lo_cut ]; then
        echo 0 > $sys_bl
        # or "echo $bl_lo_cut", if range should not include the off state.
else    echo "$bl_current / $step_f - $step_c" | bc > $sys_bl
fi

Does reverting commit

751109aad583 ACPI / video: Change the default for video.use_native_backlight to 1

help?

> Does reverting commit
> 
> 751109aad583 ACPI / video: Change the default for video.use_native_backlight
> to 1 help?

Rafael,

for me on a T520 running with fwvm2 and Opensuse 13.1, backlight control worked flawless until video.use_native_backlight was set to 1. No helper program was used or needed. With 3.17 rc1 it also works flawless if I append video.use_native_backlight = 0 to the kernel command line.

So everybody with a similar setup will see user space regression with 3.16+, even as no user space program is involved.

I see on the other hand the problem with backlight control not working with video.use_native_backlight = 0 for many users with newer laptops. And so the problem with the device exception list growing to set video.use_native_backlight = 1 for those laptops. But a solution to keep both parties happy is needed.

B.t.w. do those newer laptops react on the backlight keys while in the Bios?(In reply to Rafael J. Wysocki from comment #26)

While I have not tried to actually revert the commit, providing video.use_native_backlight=0 on the commandline gets me back to the old functionality with both the acpi and intel backlight interfaces, yes. Given that's all that commit does, that should answer your question?

Anyway, I can see no way to get rid of the acpi backlight control for any previously supported hareware without regressing userspace as long as the kernel will not implement backlight handling for native interfaces. Is there some evil involved in the kernel's doing so I cannot see? What would be wrong with a sysfs toggle to have the kernel drive or not drive the backlight changes?

(In reply to Andre Müller from comment #28)
> Anyway, I can see no way to get rid of the acpi backlight control for any
> previously supported hareware without regressing userspace as long as the
> kernel will not implement backlight handling for native interfaces. Is there
> some evil involved in the kernel's doing so I cannot see? What would be
> wrong with a sysfs toggle to have the kernel drive or not drive the
> backlight changes?

The problem is that this requires tying 2 drivers together, the one which is
getting the key eventss (which may be the acpi-video driver, but may also
be a completely different driver), and a driver often living in a completly different
subsystem. Besides it being impossible for the kernel to know which 2 devices
to tie together, this also brings some very tricky ordering problems. Making the
kernel do more key processing really is not the answer here.

As for the stuff about acpid doing brightness keypress handling causing double
handling of keypresses, that is already the case for things like the suspend key /
lid close, etc. In most distros this is solved by not enabling acpid when running
a DE which deals with things like this itself.

Regards,

Hans

The whole backlight keys handling thing has been an ad-hoc exercise so far without any overall design thought going into it as far as I can say.  For that, however, we need to talk to the graphics people seriously and I don't see a way forward other than reverting commit 751109aad583.

Sorry Hans.

> The problem is that this requires tying 2 drivers together, the one which is
> getting the key eventss (which may be the acpi-video driver, but may also
> be a completely different driver), and a driver often living in a completly
> different subsystem.

Yes. You think of a solution to handle all hardware. That won't materialize anytime soon. I am aware of that :-)

But this bug is quite a bit narrower in scope, as it can only affect machines _succesfully_ handled by acpi_video, with bioses claiming Windows8 capabilities. As acpi_video is the only driver setting brightness, any other hardware can't regress.

For all I can tell, a full "hands off" approach by the kernel would suffice to not regress _this_ hardware. The bios already handles it. Brightness can be set before the bios boots the kernel. The setting is even preserved through startup and into X.

(In reply to Rafael J. Wysocki from comment #30)
> The whole backlight keys handling thing has been an ad-hoc exercise so far
> without any overall design thought going into it as far as I can say.  For
> that, however, we need to talk to the graphics people seriously and I don't
> see a way forward other than reverting commit 751109aad583.

I'm afraid that that ship sort of has already sailed, 3.16 has shipped with the commit, and reverting it while fixing Andre's issue will likely break backlight control completely for a lot of users (all those who need native-backlight for it to work *at all*, for which we don't have a quirk yet). So reverting it after 3.16 has shipped with it will very likely cause a regressions for a ton of users.

But I do think I see another solution:

(In reply to Andre Müller from comment #31)
> But this bug is quite a bit narrower in scope, as it can only affect
> machines _succesfully_ handled by acpi_video, with bioses claiming Windows8
> capabilities. As acpi_video is the only driver setting brightness, any other
> hardware can't regress.

Correct, so how about the following. We add a new video_do_not_use_native_backlight quirk, and set this for laptops like this, where we want the default for native-backlight to be 0 rather then 1, so an inverted version of the current video_set_use_native_backlight quirk (*).

I realize having quirks is a pain, but this looks like it will be a reasonably limited list of laptops, unlike the current ever growing list of laptops needing native=1, for which a new 1 seems to pop up every other day.

Note it looks like we will need such a video_do_not_use_native_backlight quirk independent of the loosing kernel controller brightness problem being discussed here anyways since on some laptops claiming win8 support the native interface actually is broken, see e.g. https://bugs.freedesktop.org/show_bug.cgi?id=81515 .

I'm willing to whip up a patches for this for both Andre's laptop as well as the laptop from fdo#81515.

Andre, would such a fix, which would essentially make things work exactly as with 3.15 for your laptop without needing to specify anything on the kernel commandline work for you ?

Regards,

Hans


*) Now is probably also a good time to remove all the entries in the acpi-video quirk list using the video_set_use_native_backlight quirk since that is now the default.

> Andre, would such a fix, which would essentially make things work exactly as
> with 3.15 for your laptop without needing to specify anything on the kernel
> commandline work for you ?

Yes, of course.

(In reply to Andre Müller from comment #33)
> > Andre, would such a fix, which would essentially make things work exactly
> as
> > with 3.15 for your laptop without needing to specify anything on the kernel
> > commandline work for you ?
> 
> Yes, of course.

Great, can you please from a terminal run:

for i in /sys/class/dmi/id/*; do echo $i; cat $i 2> /dev/null; done

And then copy and paste (or attach) the output here ? Then I'll whip up a patch for you to test.

(In reply to Hans de Goede from comment #34)
> Great, can you please from a terminal run:
> 
> for i in /sys/class/dmi/id/*; do echo $i; cat $i 2> /dev/null; done
> 
> And then copy and paste (or attach) the output here ? Then I'll whip up a
> patch for you to test.

Erm, never mind I see you've already attached dmidecode output. I'll go work on a patch then.

(In reply to Hans de Goede from comment #34)
> 
> for i in /sys/class/dmi/id/*; do echo $i; cat $i 2> /dev/null; done
> 
For reference: This is the output for my T520:
/sys/class/dmi/id/bios_date
10/03/2013
/sys/class/dmi/id/bios_vendor
Intel Corp.
/sys/class/dmi/id/bios_version
RLH8710H.86A.0322.2013.1003.1952
/sys/class/dmi/id/board_asset_tag
To be filled by O.E.M.
/sys/class/dmi/id/board_name
DH87RL
/sys/class/dmi/id/board_serial
BQRL330000SM
/sys/class/dmi/id/board_vendor
Intel Corporation
/sys/class/dmi/id/board_version
AAG74240-401
/sys/class/dmi/id/chassis_asset_tag
        
/sys/class/dmi/id/chassis_serial
        
/sys/class/dmi/id/chassis_type
3
/sys/class/dmi/id/chassis_vendor
        
/sys/class/dmi/id/chassis_version
        
/sys/class/dmi/id/modalias
dmi:bvnIntelCorp.:bvrRLH8710H.86A.0322.2013.1003.1952:bd10/03/2013:svn:pn:pvr:rvnIntelCorporation:rnDH87RL:rvrAAG74240-401:cvn:ct3:cvr:
/sys/class/dmi/id/power
/sys/class/dmi/id/product_name
        
/sys/class/dmi/id/product_serial
        
/sys/class/dmi/id/product_uuid
75B35F94-F89C-11E2-B0B4-0013207A6641
/sys/class/dmi/id/product_version
        
/sys/class/dmi/id/subsystem
/sys/class/dmi/id/sys_vendor
        
/sys/class/dmi/id/uevent
MODALIAS=dmi:bvnIntelCorp.:bvrRLH8710H.86A.0322.2013.1003.1952:bd10/03/2013:svn:pn:pvr:rvnIntelCorporation:rnDH87RL:rvrAAG74240-401:cvn:ct3:cvr:

(In reply to Uwe Bonnes from comment #36)
> (In reply to Hans de Goede from comment #34)
> > 
> > for i in /sys/class/dmi/id/*; do echo $i; cat $i 2> /dev/null; done
> > 
> For reference: This is the output for my T520:

<snip>

Thanks, but I think you were ssh-ed to a different machine when doing this, as this is from this motherboard:
http://www.intel.com/content/www/us/en/motherboards/desktop-motherboards/desktop-board-dh87rl.html not from a T520. Please re-do on your T520.

(In reply to Hans de Goede from comment #37)

> Thanks, but I think you were ssh-ed to a different machine when doing this,
> as this is from this motherboard:

Silly me.

Will do this evening when sitting before the real laptop...

Sorry.

Created attachment 148491 [details]
Set of patches fixes the regression this bug is about

Hi Andre,

Here is a set of patches which should fix the regression you are seeing. You only need patches 1 and 2, but for completeness sake I've put the complete set of patches I'm working on in a tarbal.

Can you please build a 3.16 or 3.17 kernel with patches 1 & 2 applied, and then boot that without any special kernel commandline parameters, after this things should work the same as they did in 3.15.

Uwe, I've already found the dmi info I need on the T520 with a quick google, so these patches should also fix the regression for you. So if you could do the same that would be great.

Thanks & Regards,

Hans

Hi Hans,

I think I found a nit in the third patch in the series.
It's not needed for the T420, but anyway:

> +       /* The native backlight controls do not work on some older machines
> */
> +       {
> +        /* https://bugs.freedesktop.org/show_bug.cgi?id=81515 */
> +        .callback = video_disable_native_backlight,
> +        .ident = "ThinkPad T520",
You confused the machines here, this is not adressing a Thinkpad.

> +        .matches = {
> +               DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
> +               DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY 15 Notebook PC"),
+               },
+       },
        {}
 };

(In reply to Andre Müller from comment #40)
> Hi Hans,
> 
> I think I found a nit in the third patch in the series.
> It's not needed for the T420, but anyway:
> 
> > +       /* The native backlight controls do not work on some older machines
> */
> > +       {
> > +        /* https://bugs.freedesktop.org/show_bug.cgi?id=81515 */
> > +        .callback = video_disable_native_backlight,
> > +        .ident = "ThinkPad T520",
> You confused the machines here, this is not adressing a Thinkpad.
> 
> > +        .matches = {
> > +               DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
> > +               DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY 15 Notebook PC"),
> +               },
> +       },
>         {}
>  };

Heh,

Copy paste error thanks for catching this, fixed it in my local git tree.

Hi Hans,

the patches (0001 and 0002) work for me, they fix the regression here.

Tested on 3.16.1 as well as current git (>3.17-rc2, ff0c57ac), with and without i915.fastboot. 

Dank je wel,
Andre

Here the output from the real T520:
for i in /sys/class/dmi/id/*; do echo $i; cat $i 2> /dev/null; done/sys/class/dmi/id/bios_date
07/26/2013
/sys/class/dmi/id/bios_vendor
LENOVO
/sys/class/dmi/id/bios_version
8AET64WW (1.44 )
/sys/class/dmi/id/board_asset_tag
Not Available
/sys/class/dmi/id/board_name
42435GG
/sys/class/dmi/id/board_serial
1ZK6123S1DM
/sys/class/dmi/id/board_vendor
LENOVO
/sys/class/dmi/id/board_version
Not Available
/sys/class/dmi/id/chassis_asset_tag
No Asset Information
/sys/class/dmi/id/chassis_serial
R9N51TH
/sys/class/dmi/id/chassis_type
10
/sys/class/dmi/id/chassis_vendor
LENOVO
/sys/class/dmi/id/chassis_version
Not Available
/sys/class/dmi/id/modalias
dmi:bvnLENOVO:bvr8AET64WW(1.44):bd07/26/2013:svnLENOVO:pn42435GG:pvrThinkPadT520:rvnLENOVO:rn42435GG:rvrNotAvailable:cvnLENOVO:ct10:cvrNotAvailable:
/sys/class/dmi/id/power
/sys/class/dmi/id/product_name
42435GG
/sys/class/dmi/id/product_serial
R9N51TH
/sys/class/dmi/id/product_uuid
81C69FBA-5E51-CB11-B7F4-BA4869ABD1BB
/sys/class/dmi/id/product_version
ThinkPad T520
/sys/class/dmi/id/subsystem
/sys/class/dmi/id/sys_vendor
LENOVO
/sys/class/dmi/id/uevent
MODALIAS=dmi:bvnLENOVO:bvr8AET64WW(1.44):bd07/26/2013:svnLENOVO:pn42435GG:pvrThinkPadT520:rvnLENOVO:rn42435GG:rvrNotAvailable:cvnLENOVO:ct10:cvrNotAvailable:

Sorry again for the confusion.

Thanks for the testing, I've submitted the patchset upstream with a CC: stable for 3.16+

Created attachment 149721 [details]
DMI for Thinkpad X201s

Hi,

Debian just upgraded to 3.16, which means we get that bug too. I have a Thinkpad x201s laptop from 2010 (so before Windows 8), but the BIOS (through an update, I guess) now claims it supports Windows 8, while the native backlight interface doesn't work (and the acpi one worked just fine before).

I'm attaching the DMI information. Note that BIOS version might matter here, since previous BIOS versions didn't claim to support Windows 8 (but blacklisting the native interface shouldn't break anything, I guess).

Created attachment 149751 [details]
Disable native backlight for ThinkPad X201s

Please test this patch, thanks.
Apply on top of v3.17-rc4.

BTW, please also attach your acpidump.

Created attachment 149771 [details]
acpidump for Thinkpad x201s (with bios 6QET70WW (1.40 ))

Here's the acpidump.

I'll report back with test results once I built the relevant kernel.

It does work fine on console, and I indeed get an acpi_video0 back in /sys/class/backlight, but I still have a regression when xfce4-power-manager is running (which I didn't have on 3.14).

I'll try to investigate, but in any case it's still better than 3.16.

So it seems that the problem with xfce4-power-manager is with XRRGetOutputProperty().

When calling this function (in order to get the current brightness level) as a result of the brightness key press, the brightness is not changed.

This didn't happen on 3.14, but it might be completely unrelated to the current change (or the one in bug #51231). I'll try to rebuild a 3.16 with the x201s patch and see if it happens there.

Confirmed, 3.16 (3.16.2 mainline and Debian) booted with video.use_native_backlight=0 behaves well, so it looks like a 3.17 completely independant.

Aaron, I guess you can commit the patch (and CC: stable).

From your acpidump table, the ASL code used to control backlight is to pass the request to the GPU driver i915, so I don't see why the native backlight interface on your system doesn't work... But considering the hotkey in kernel process functionality, I agree that we should restore ACPI video interface for your system.

Considering the x201 patch, is it that with v3.17 + xfce, it's still broken? Any chance to find out why? Thanks.

(In reply to Aaron Lu from comment #52)
> Considering the x201 patch, is it that with v3.17 + xfce, it's still broken?
> Any chance to find out why? Thanks.

I'm unsure what you mean here. On 3.17+, the brightness change (using ACPI method, with keyboard brightness keys) doesn't work if at the same time you're checking the brightness using RandR (with the XRRGetOutputProperty()). That happens for example with xfce4-power-manager, but I guess there might be other ways.

I have no idea how to debug that further. I could try to bisect but I lack time a bit right now. In any case, it looks like a different bug so it might be worth porting that to another bugzilla entry.

Running Debian 3.16-2 on a Thinkpad X1 1st Generation, I've had to add acpi=\"!windows2012\" to grub to get the Fn F[89] brightness control keys to work.

I wonder if the changes made in acpi also affect the lid? For what it is worth: acpidump is attached here: 

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

Gijs

Hi Gijs,

(In reply to Gijs Hillenius from comment #54)
> Running Debian 3.16-2 on a Thinkpad X1 1st Generation, I've had to add
> acpi=\"!windows2012\" to grub to get the Fn F[89] brightness control keys to
> work.

"Getting the brightness control keys to work" is a bit vague. There can be multiple reasons why they do not work, basically:

1) The keys not emitting events
2) Nothing listening to the events (which desktop environment do you use?)
3) The backlight control interface itself not working

Can you please walk through the debug steps described here:

http://hansdegoede.livejournal.com/13889.html

Without the acpi=\"!windows2012\" on the kernel commandline, and I assume you mean acpi_osi=... ?  :

It would also be good to have the output of:

"ls /sys/class/backlight" for both a boot with and without the acpi_osi=... kernel commandline argument,

Regards,

Hans

Hello

It was a new install of Debian Unstable. I'm using i3 windowmanager and lightdm display manager. Hitting the Fn F8 and Fn F9 keys did at first do nothing. (The screen would not become brighter or dimmer).

I could set the screen brightness by echoing 

echo 1000 > /sys/class/backlight/intel_backlight/brightness

which survives across reboots.

After searching I found acpi=\"!windows2012\". I first tried adding acpi_backlight=vendor but that did not do anything. The combination of the two did also not work. And acpi=\"!windows2012\" at first took a few key presses before it would respond, but now it responds immediately. (that is also vague, yes).

I have not investigated if the key did not emit events. I'll do this, later. I will walk through your debug steps, asap. 

Currently - *with* acpi=\"!windows2012\":

ls /sys/class/backlight
acpi_video0  intel_backlight

Hi,

So it seems that your problem is case 2 of the 3 cases which I mentioned before, which really is a bug in your desktop environment.

Regards,

Hans