Bug 51051

please attach the acpidump output of your laptop.

Created attachment 87881 [details]
acpidump of Acer Aspire S7-391

    Scope (_SB.PCI0.GFX0.DD02)
    {
        Method (_BCL, 0, NotSerialized)  // _BCL: Brightness Control Levels
        {
            Return (\BCLP)
        }

        Name (CBNL, Zero)
        Method (_BCM, 1, NotSerialized)  // _BCM: Brightness Control Method
        {
            Store (Arg0, CBNL)
        }

        Method (_BQC, 0, NotSerialized)  // _BQC: Brightness Query Current
        {
            Return (CBNL)
        }
    }
this shows that the ACPI backlight control for this laptop is totally fake.
so you always need acpi_backlight=vendor whenever you want to control the backlight.

Q1. does /sys/class/backlight/intel_backlight/ always works no matter if the parameter is added?
Q2. does the hotkey work if you boot with acpi_osi="!Windows 2012" acpi_osi="!Windows 2009" acpi_osi="!Windows 2006" altogether?

1. Yes as far as I can tell intel_backlight always works by manually echoing a value in to /sys/class/backlight/intel_backlight/brightness. 

2. Booting with all of those options allows the hotkeys to function although it brings back the acpi_video0 interface so even though they are registered they do not actually control the backlight as expected.

(In reply to comment #4)
> 2. Booting with all of those options allows the hotkeys to function although
> it
> brings back the acpi_video0 interface so even though they are registered they
> do not actually control the backlight as expected.

oh, as I mentioned in comment #3, you always need acpi_backlight=vendor added.
please try with these four parameters altogether.

So I tried with all of the boot options and acpi_backlight=vendor so only the intel backlight shows up but so far the backlight keys are still not functioning.

(In reply to comment #0)
> If the boot options are removed then the hot keys function but do
> not actually change the backlight.

then how do you know the hot key works?
can you get input event successfully?
please attach the output of acpi_listen when you press the hotkey in kernels both with and without acpi_osi="!Windows 2012"
acpi_osi="!Windows 2009" acpi_osi="!Windows 2006" and acpi_backlight=vendor.

I know the hotkeys work because without any boot options you can see the Gnome Brightness dialogue attempt to change the brightness. The output of acpi_listen without any acpi boot options is below:

Boot line: kernel /vmlinuz-3.6.7-4.fc17.x86_64 rd.lvm=0 rd.dm=0 rd.md.uuid=fd734d6e:c3705bec:0b9e4b74:93046508  KEYTABLE=us rd.md.uuid=39dae6e2:da0f271e:51a7cc88:8de15c75 rd.luks=0 LANG=en_US.UTF-8 ro root=UUID=0ff0b367-4a2a-4a13-84be-49f3877b76c2 rhgb quiet i915.i915_enable_rc6=1 raid=noautodetect pcie_aspm=force

[root@slim ~]# acpi_listen 
video/brightnessdown BRTDN 00000087 00000000
video/brightnessup BRTUP 00000086 00000000
video/brightnessup BRTUP 00000086 00000000
video/brightnessdown BRTDN 00000087 00000000


With the following boot line no acpi events are seen for the two brightness keys.

Boot line: kernel /vmlinuz-3.6.7-4.fc17.x86_64 rd.lvm=0 rd.dm=0 rd.md.uuid=fd734d6e:c3705bec:0b9e4b74:93046508  KEYTABLE=us rd.md.uuid=39dae6e2:da0f271e:51a7cc88:8de15c75 rd.luks=0 LANG=en_US.UTF-8 ro root=UUID=0ff0b367-4a2a-4a13-84be-49f3877b76c2 rhgb quiet acpi_osi=Linux acpi_backlight=vendor i915.i915_enable_rc6=1 raid=noautodetect pcie_aspm=force acpi_osi="!Windows 2012" acpi_osi="!Windows 2009" acpi_osi="!Windows 2006"

[root@slim ~]# acpi_listen 
^C
[root@slim ~]#

Created attachment 88131 [details]
custom DSDT

please apply this customized DSDT, and reboot with kernel parameter "acpi.aml_debug_output=1".
and then press the hotkey and see if it works.
please attach the dmesg output after you press the hotkey as well.

Hi,
   I've applied the custom DSDT and attached the full dmesg output. The output for the brightness up and down key presses are below as well.

[  759.489985] [ACPI Debug]  String [0x07] "In _Q87"
[  759.490008] [ACPI Debug]  String [0x04] "DSEN"
[  759.490052] [ACPI Debug]  Integer 0x00000004

[ 1064.549723] [ACPI Debug]  String [0x07] "In _Q86"
[ 1064.549746] [ACPI Debug]  String [0x04] "DSEN"
[ 1064.549792] [ACPI Debug]  Integer 0x00000004

Created attachment 88241 [details]
ACPI Debug output with custom DSDT

Requested output with the custom DSDT applied

I have been looking at this and trying to learn as I go along. With that being said before I started looking in to this I never really looked at the kernel source code so I may be incorrect in some of my statements.

I believe without acpi_backlight=vendor as a kernel parameter then the hotkeys are being captured by the acpi video driver but they are being passed to the acpi video driver instead of the Intel driver. With acpi_backlight=vendor set though there is nothing to capture the hotkey presses.

My normal distribution is Fedora but I decided to try Ubuntu to see if the hotkeys would work there with acpi_backlight=vendor set and it turns out that the hotkeys register and adjust the brightness for the Intel driver appropriately. /sys/class/backlight also only shows intel_backlight. I believe it is due to the fact that Ubuntu uses the 3.5 kernel instead of 3.6 or 3.7. 

I believe the answer may be in commit a60b21763cce01c64cc537869662b41429c62e5f in the acer-wmi.c file. It looks like before that commit acpi_video_backlight_support() equaled 0 so acpi_video_unregister is never called. After that commit acpi_video_backlight_support() still equals 0 but the else clause calls acpi_video_unregister which unloads the acpi video driver and that takes the hotkeys with it.

I'm not entirely sure if any of this helps but hopefully it will. :)

Hi Kevin,

Thanks for the finding, it definitely helps.

Can you please test the latest upstream kernel first? If it still has this problem, I think we can add Corentin Chary, who is the author of the commit, to take a look at this problem, thanks.

Hi Aaron,

I just tried the 3.9-rc2 kernel Saturday and the hotkeys do not work with that version either.

Thanks Kevin.

Hi Corentin,
It seems your commit a60b21763cce01c64cc537869662b41429c62e5f(drivers-platform-x86: use acpi_video_dmi_promote_vendor()) breaks the hotkey on Acer Aspire S7-391 as spotted by Kevin.

It looks like, for this system, when hotkey for brightness is pressed, there will be no keyboard scancode emitted, instead, EC will send out a notification. unregister acpi video driver would make such notification lost. So perhaps we need to keep acpi video driver's notification handling.

I'm not aware of the reason why acpi_video_unregister is added if acpi video driver will not take care of backlight control, I think it has other uses besides that, like output device switch, etc.

Well, the current code looks good to me.

	if (dmi_check_system(video_vendor_dmi_table))
		acpi_video_dmi_promote_vendor();
	if (acpi_video_backlight_support()) {
		interface->capability &= ~ACER_CAP_BRIGHTNESS;
		pr_info("Brightness must be controlled by acpi video driver\n");
	} else {
		pr_info("Disabling ACPI video driver\n");
		acpi_video_unregister();
	}


If the model is in video_vendor_dmi_table, or if the user ask for it, user the vendor driver for backlight instead of acpi/video.

If this model is only working with the generic video driver, then maybe it should not be in video_vendor_dmi_table ?

(In reply to comment #16)
> Well, the current code looks good to me.
> 
>     if (dmi_check_system(video_vendor_dmi_table))
>         acpi_video_dmi_promote_vendor();
>     if (acpi_video_backlight_support()) {
>         interface->capability &= ~ACER_CAP_BRIGHTNESS;
>         pr_info("Brightness must be controlled by acpi video driver\n");
>     } else {
>         pr_info("Disabling ACPI video driver\n");
>         acpi_video_unregister();
>     }
> 
> 
> If the model is in video_vendor_dmi_table, or if the user ask for it, user
> the
> vendor driver for backlight instead of acpi/video.
> 
> If this model is only working with the generic video driver, then maybe it
> should not be in video_vendor_dmi_table ?

This system is kind of interesting: acpi video driver can't handle backlight, while vendor driver can't handle hotkey... and to make it work, both drivers will be needed. Right?

Then maybe something smarter need to be added to load both drivers. But this is usually a very bad idea since userspace will try to use the generic one first, and it won't work.

Is there a BIOS update somewhere that could fix that ? Or a specific acpi_osi setting ?

Or what about if vendor driver is to be used to control backlight level, we do not unregister acpi video driver, so that it can still handle notification. As long as acpi_video_backlight_support returns false, acpi video driver will not create any sysfs backlight interface, so vendor driver doesn't need to do anything more I think.

I can understand the reason to unregister acpi video driver in the code, but unfortunately, vendor driver can't handle hotkey in this case.

(In reply to comment #18)
> Is there a BIOS update somewhere that could fix that ?
I don't know about this, but the chances are low I believe.

> Or a specific acpi_osi setting ?
The standard interface to control backlight level in this BIOS table is totally fake(I don't understand why they even bother to create a bogus one :-( ), it doesn't do anything real, so it's impossible for acpi video driver to control backlight, as illustrated in comment #3 by Rui, no matter what acpi_osi is given.

Oh .. in this case the acpi_video driver doesn't create a sysfs interface but still handle notifications ? If that's true, we could add a specific DMI match for this model, and do not undregister it in this specific case. It's still necessary to unregister it in other cases.

I won't have time to write such a patch in the near future, but I will be happy to review it.

(In reply to comment #20)
> Oh .. in this case the acpi_video driver doesn't create a sysfs interface but
> still handle notifications ? 

Yes. It will need to handle notifications, since the hotkey will not send out any scancode, but an acpi event. At least, this seems to be the case based on Kevin's description.

> If that's true, we could add a specific DMI match
> for this model, and do not undregister it in this specific case. It's still
> necessary to unregister it in other cases.

For other cases, is it because both scancode and acpi event will be emitted when hotkey is pressed?

> 
> I won't have time to write such a patch in the near future, but I will be
> happy
> to review it.

OK, I'll write a patch to handle this, thanks for the review.

Hi Kevin,

You dmidecode output please, thanks.

Created attachment 96291 [details]
DMIDECODE Output

Aaron I have attached the dmidecode output as requested. Thanks.

Created attachment 96451 [details]
Test patch to remove acpi_video_unregister call

Hi,

Please kindly test this patch, it is on top of 3.8.4, also applies to latest
Linus' git tree. You do not need to add any kernel command line parameter with this patch.

Please list /sys/class/backlight after test, thanks.

Hi Aaron,
   I was able to test the patch against 3.9.0-rc4. After applying the patch I can adjust the brightness successfully but there are some caveats. Also the only thing showing under /sys/class/backlight is the intel_backlight with and without acpi_backlight=vendor.

While the backlight does change with the hotkeys the key presses do not show up under showkey or acpi_listen. The other thing is that the Gnome On Screen Dialog does not register the key presses when changing backlight brightness (I think it may be because the keys are not being registered how Gnome expects them to be or because of the next point). One final thing is that if a brightness is echoed to /sys/class/backlight/intel_backlight/brightness then the brightness is not changed.

Thanks!

(In reply to comment #26)
> Hi Aaron,
>    I was able to test the patch against 3.9.0-rc4. After applying the patch I
> can adjust the brightness successfully but there are some caveats. Also the
> only thing showing under /sys/class/backlight is the intel_backlight with and
> without acpi_backlight=vendor.

This is expected, neither acpi video driver nor platform vendor driver can handle this system's backlight, only intel gpu driver can, so only that interface exists.

> 
> While the backlight does change with the hotkeys the key presses do not show
> up
> under showkey or acpi_listen.

Weird, if the hotkey doesn't make its way to user space, how can the backlight change?

> The other thing is that the Gnome On Screen
> Dialog does not register the key presses when changing backlight brightness
> (I
> think it may be because the keys are not being registered how Gnome expects
> them to be or because of the next point). One final thing is that if a
> brightness is echoed to /sys/class/backlight/intel_backlight/brightness then
> the brightness is not changed.

This suggests that the intel_backlight does not function. This patch shouldn't affect intel gpu driver, but to be safe, plase also test 3.9.0-rc4 without applying this patch, see if echo to intel_backlight/brightness works or not.

Created attachment 96531 [details]
Debug DSDT to dump some variable

Hi Kevin,

Please do one more test by using this DSDT table together with the patch, and attach the dmesg after you've pressed the brightness hotkey. Thanks. And you will need the acpi.aml_debug_output=1 kernel parameter for the debug output, no other acpi related parameter is needed.

(In reply to comment #25)
> Created an attachment (id=96451) [details]
> Test patch to remove acpi_video_unregister call
> 
> Hi,
> 
> Please kindly test this patch, it is on top of 3.8.4, also applies to latest
> Linus' git tree. You do not need to add any kernel command line parameter
> with
> this patch.
> 
> Please list /sys/class/backlight after test, thanks.


Why are you moving this to acpi directly ? Does it really need to be here and not in the vendor driver ? (this can happen if registering/unregistering the video driver confuse the DSDT, but I don't think it's the case for *every* model in this list).

 
> While the backlight does change with the hotkeys the key presses do not show
> up
> under showkey or acpi_listen. The other thing is that the Gnome On Screen
> Dialog does not register the key presses when changing backlight brightness
> (I
> think it may be because the keys are not being registered how Gnome expects
> them to be or because of the next point).

No, gnome is broken and listen to key press instead of uevent to display brightness changes: https://bugzilla.gnome.org/show_bug.cgi?id=672380

(In reply to comment #29)
> Why are you moving this to acpi directly ? Does it really need to be here and
> not in the vendor driver ? (this can happen if registering/unregistering the
> video driver confuse the DSDT, but I don't think it's the case for *every*
> model in this list).

I understand the reason to keep those dmi entries in vendor driver is to avoid a big dmi table for acpi video driver, which is indeed best to avoid, but in this case, I don't see how we can overcome this without moving this to acpi directly.

We can't simply not call acpi_video_unregister for these systems, as we really need to make sure acpi video driver does not create backlight device for them, or user will need to pass the acpi_backlight=vendor command line param.

An alternative would be, introduce a new interface, something like acpi_video_unregister_backlight, which does not unload the whole acpi video driver, but just unregister the backlight device it has created. So that its notification handler is still there.

But I would like to avoid this, it is adding yet another quirk on top of an existing quirk, it makes the code difficult to understand.

And moving all the DMI entries to acpi video also has the benefit of no unnecessary dependency between vendor driver and video driver.

But if you insist, I can do that. Just let me know your opinion, thanks.

Hi Kevin,

My understanding of your system backlight control is:
for brightness level change, you are using intel_backlight, which is provided by the gpu driver;
for hotkey notification, you are using acpi video driver.

But your comment #26 made me feel that this is not the case, or you should receive key press event and the intel_backlight interface should work using the echo command. So can you please verify if unload acer-wmi module will make a difference, thanks.

BTW, bug #35622 is a similar one.

Aaron,
   I have not had a chance to test 3.9 without the provided patches. I have tried it with the modified DSDT but it did not output anything in dmesg.

I unloaded acer-wmi with modprobe -r acer-wmi and while the on screen dialogue does not work the following occurs:

echo'ing a value to brightness updates the screen brightness as well as the actual_brightness value. Using the hotkeys only updates that actual_brightness value while leaving the brightness value untouched.

Kevin,

This really beats me... :-)
I would expect some similar output as in comment #10, I don't see why we lose those messages. Did you add acpi.aml_debug_output=1 to kernel command line?

Created attachment 97111 [details]
Compilation Errors

Aaron,
   It looks like I missed a few options that were necessary for adding the custom DSDT. I added the required options but when I try to compile I get the errors in the attachment to comment #36. Should this be expected? Also I apologize for the confusion. Looks like I should have drank some coffee first. :)

Created attachment 97141 [details]
0001-acer-wmi-add-Acer-Aspire-S7-391-to-video-vendor-list.patch

Similar with bko#35622, create this testing patch.

Hi Kevin, 

Please help to test this patch, it's still add Aspire-S7 to table but this time I add a new capability for avoid unregister acpi/video driver on your machine.

Please try this patch WITHOUT "acpi_backlight=vendor" and "acpi_osi=Linux" and other kernel parameter, I think this time acpi/video will keep there for emit keycode.

Please also use "lsmod | grep video" to check if video driver still there.

Thanks for your help!

(In reply to comment #37)
> Aaron,
>    It looks like I missed a few options that were necessary for adding the
> custom DSDT. I added the required options but when I try to compile I get the
> errors in the attachment to comment #36. Should this be expected? Also I
> apologize for the confusion. Looks like I should have drank some coffee
> first.
> :)

Hi Kevin,

Please follow this document: Documentation/acpi/initrd_table_override.txt, no need to recompile kernel for the DSDT, we can replace the DSDT in initrd.

Hi Aaron,
  Here are the debug messages that were printed. The first three lines occur when pressing the brightness down hotkey key and the last three were brightness up hotkey. I will also attach the full output of dmesg.

Brightness Down Outputs:
[  591.986654] [ACPI Debug]  String [0x04] "_Q86"
[  591.987156] [ACPI Debug]  Integer 0x00000001
[  591.988751] [ACPI Debug]  Integer 0x00000000

Brightness Up Outputs:
[  593.356288] [ACPI Debug]  String [0x04] "_Q87"
[  593.356789] [ACPI Debug]  Integer 0x00000001
[  593.358288] [ACPI Debug]  Integer 0x00000000

Thanks!

Created attachment 97221 [details]
Custom DSDT table ACPI Debug Output

Full output of dmesg with acpi debug dsdt.

(In reply to comment #38)
> Created an attachment (id=97141) [details]
> 0001-acer-wmi-add-Acer-Aspire-S7-391-to-video-vendor-list.patch
> 
> Similar with bko#35622, create this testing patch.
> 
> Hi Kevin, 
> 
> Please help to test this patch, it's still add Aspire-S7 to table but this
> time
> I add a new capability for avoid unregister acpi/video driver on your
> machine.
> 
> Please try this patch WITHOUT "acpi_backlight=vendor" and "acpi_osi=Linux"
> and
> other kernel parameter, I think this time acpi/video will keep there for emit
> keycode.
> 
> Please also use "lsmod | grep video" to check if video driver still there.
> 
> Thanks for your help!

Hi,
   I tested the patch and without acpi_backlight=vendor both intel and acpi_backlight show under /sys/class/backlight. The hotkeys do work but no keycode is emitted for X/Gnome Brightness to catch.

(In reply to comment #16)
> Well, the current code looks good to me.
> 
>     if (dmi_check_system(video_vendor_dmi_table))
>         acpi_video_dmi_promote_vendor();
>     if (acpi_video_backlight_support()) {
>         interface->capability &= ~ACER_CAP_BRIGHTNESS;
>         pr_info("Brightness must be controlled by acpi video driver\n");
>     } else {
>         pr_info("Disabling ACPI video driver\n");
>         acpi_video_unregister();
>     }
> 
> 
> If the model is in video_vendor_dmi_table, or if the user ask for it, user
> the
> vendor driver for backlight instead of acpi/video.
> 
> If this model is only working with the generic video driver, then maybe it
> should not be in video_vendor_dmi_table ?

Hi Corentin,

I misunderstand your words "generic video driver", I thought you are saying vendor driver.

So yes, this model only works with the generic video driver(i.e. the gpu driver), and I agree that it shouldn't be in video_vendor_dmi_table.

(In reply to comment #40)
> Brightness Up Outputs:
> [  593.356288] [ACPI Debug]  String [0x04] "_Q87"
> [  593.356789] [ACPI Debug]  Integer 0x00000001
1 here is for variable IGDS
> [  593.358288] [ACPI Debug]  Integer 0x00000000
0 here is for DSEN

It is interesting to see you have different values in comment #10(the DSEN is 4 there and 0 here). I don't why this is the case, but it has the consequences of hotkey directly changing backlight instead of sending out notification.

Hi Rui,

In the dmesg Kevin attached in comment #11, I saw:
...
[    5.351340] [ACPI Debug]  String [0x06] "In DOS"
[    5.351355] [ACPI Debug]  String [0x04] "DSEN"
[    5.351388] [ACPI Debug]  Integer 0x00000000
[    5.351398] [ACPI Debug]  String [0x04] "Arg0"
[    5.351407] [ACPI Debug]  Integer 0x00000004
[    5.351470] ACPI: Video Device [GFX0] (multi-head: yes  rom: no  post: no)
[    5.351589] input: Video Bus as /devices/LNXSYSTM:00/device:00/PNP0A08:00/LNXVIDEO:00/input/input6
...

The arg0 will be 4 only when we call acpi_video_bus_stop_devices, but obviously this dump here is from acpi_video_bus_start_devices. So I don't see why the arg0 will be 4 here. Do you have any idea? Thanks.

Hello Lee,

Do you know if acer-wmi module can deliver hotkey notification for this system? Thanks.

Created attachment 98041 [details]
Add Acer Aspire-S7 into video_detect_dmi_table

Hi Kevin,

I have prepared two patches for you to test, patch 1 is attached here, and patch 2 is here:
https://bugzilla.kernel.org/attachment.cgi?id=98031

Please apply patch 1 and then patch 2, on top of v3.8 or Linus' git tree.

Patch 1 adds this system into video_detect_dmi_table without touching any platform code(hopefully platform people will be happy), and patch 2 will turn on the flag for firmware to report notifications.

BTW, when building the kernel, please do not select acer-wmi for this test, thanks.

A more general solution than this will be required(as distros will always build those modules), but I want to know if this works or not.

Hi Aaron,
   If I do not build the acer-wmi module and apply the patches I am able to boot without the acpi_backlight=vendor option and everything works as expected. The hotkeys function properly and the key presses are passed to Gnome.

Hi Kevin,

Matthew submitted a patchset to remove ACPI video's backlight interface if the system claims support of Win8, as is the case of your system.

Can you please give it a try? They are here:
https://patchwork.kernel.org/patch/2695411/
https://patchwork.kernel.org/patch/2695391/
https://patchwork.kernel.org/patch/2695401/

And in addition to that, the mentioned patch 2 in comment #47 is still required to fix the notification delivery problem:
https://bugzilla.kernel.org/attachment.cgi?id=98031

So with the four patches, the intel_backlight will be the only interface left, while the notification delivery provided by ACPI video is fixed and usable. Let me know your test result, thanks.

Hi Aaron,
   I tested the 4 patches against 3.10.0 RC6. I booted with the following kernel command line: Command line: BOOT_IMAGE=/vmlinuz-3.10.0-rc6-kevin+ root=UUID=0d6d2e9b-811c-4851-8b57-bac3ec9010df ro rd.lvm=0 rd.dm=0 rd.md.uuid=fd734d6e:c3705bec:0b9e4b74:93046508 rd.md.uuid=39dae6e2:da0f271e:51a7cc88:8de15c75 rd.luks=0 rhgb quiet raid=noautodetect libahci.ignore_sss=1 elevator=noop LANG=en_US.UTF-8

With the patches everything worked as expected. The only backlight present was the intel_backlight and the hotkey notifications worked as well with GNOME.

Patches in Rafael's acpi-video branch.

https://patchwork.kernel.org/patch/2829342/  (expose OSI version)
https://patchwork.kernel.org/patch/2829344/  (Always call acpi_video_init_brightness() on init)
https://patchwork.kernel.org/patch/2829339/  (No ACPI backlight if firmware expects Windows 8)
https://patchwork.kernel.org/patch/2827878/  (no automatic brightness changes by firmware)

http://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git acpi-video

Patches in Linus' tree.

commit efaa14c7e981bdf8d3c8d39d3ed12bdc60faabb8
Author: Aaron Lu <aaron.lu@intel.com>
Date:   Tue Jul 16 13:08:05 2013 +0800

    ACPI / video: no automatic brightness changes by win8-compatible firmware

commit 8c5bd7adb2ce47e6aa39d17b2375f69b0c0aa255
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Date:   Thu Jul 18 02:08:06 2013 +0200

    ACPI / video / i915: No ACPI backlight if firmware expects Windows 8

commit c04c697cf1fe8f0962ccd3c2392a9b637a5307aa
Author: Matthew Garrett <matthew.garrett@nebula.com>
Date:   Tue Jul 16 17:08:16 2013 +0000

    ACPI / video: Always call acpi_video_init_brightness() on init

commit 242b2287cd7f27521c8b54a4101d569e53e7a0ca
Author: Aaron Lu <aaron.lu@intel.com>
Date:   Tue Jul 2 21:59:10 2013 +0800

    ACPICA: expose OSI version