Bug 42861

Summary: Samsung laptop can only change brightness when adding acpi_backlight=vendor
Product: ACPI Reporter: Luis Medinas (lmedinas)
Component: Power-VideoAssignee: Aaron Lu (aaron.lu)
Status: CLOSED CODE_FIX    
Severity: normal CC: aaron.lu, alan, artem.savkov, cheppes, rui.zhang
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: Subsystem:
Regression: No Bisected commit-id:
Attachments: dmidecode of my laptop
acpidump
acpidump
show some debug message about acpi power video
dmesg
Debug DSDT table to fix BRTL
DSDT table for Luis
Patch to enhance the setting of _BQC_use_index
Patch to enhance the quirk setting of _BQC
Patch to enhance the detection of buggy _BQC
Enhance the quirk detect logic of _BQC
Enhance the quirk detect logic of _BQC

Description Luis Medinas 2012-03-04 17:24:16 UTC
If i set this line on the bootloader i'm able to change the brightness using the FN keys. if this line isn't set i can only change brightness (using the FN keys) from 100% to 90%.

I'm using the intel driver. dmidecode attached.

What should i provide more to fix this bug ? I can also cook a patch if anyone help me to get started.
Thanks
Comment 1 Luis Medinas 2012-03-04 17:24:45 UTC
Created attachment 72537 [details]
dmidecode of my laptop
Comment 2 Cheppes 2012-08-28 17:29:35 UTC
I have a similar problem, it seems to come from a bug in the /sys interface. Here is what I have on boot:

# cd /sys/class/backlight
# grep -H '' */*(.)
acpi_video0/actual_brightness:7
acpi_video0/bl_power:0
acpi_video0/brightness:7
acpi_video0/max_brightness:7
acpi_video0/type:firmware
intel_backlight/actual_brightness:4648
intel_backlight/bl_power:0
intel_backlight/brightness:4648
intel_backlight/max_brightness:4648
intel_backlight/type:raw

And here is what happens when I change the value:

# echo 0 >acpi_video0/brightness
# grep -H '' */*(.)
acpi_video0/actual_brightness:7
acpi_video0/bl_power:0
acpi_video0/brightness:0
acpi_video0/max_brightness:7
acpi_video0/type:firmware
intel_backlight/actual_brightness:309
intel_backlight/bl_power:0
intel_backlight/brightness:4648
intel_backlight/max_brightness:4648
intel_backlight/type:raw

The ACPI backend doesn't return the updated value for actual_brightness. This throws off the user space tools that rely on this interface, like the intel xorg video driver.

PS: the "samsung" interface that appears when setting acpi_backlight=vendor is even buggier.
Comment 3 Zhang Rui 2012-11-13 07:54:38 UTC
Luis,
please attach the output of "grep . /sys/class/backlight/*/*".
if you boot without acpi_backlight=vendor, does "poking /sys/class/backlight" directly change the backlight?

Cheppes,
please attach the acpidump of your box.
Comment 4 Zhang Rui 2012-11-21 01:30:21 UTC
ping...
Comment 5 Zhang Rui 2012-11-28 10:28:31 UTC
ping...
Comment 6 Zhang Rui 2012-12-11 01:45:02 UTC
ping...
Comment 7 Luis Medinas 2012-12-11 19:39:58 UTC
Hi Rui,

sorry for long the delay.

grep . /sys/class/backlight/*/*
/sys/class/backlight/intel_backlight/actual_brightness:929
/sys/class/backlight/intel_backlight/bl_power:1020025576
/sys/class/backlight/intel_backlight/brightness:929
/sys/class/backlight/intel_backlight/max_brightness:4648
/sys/class/backlight/intel_backlight/type:raw
Comment 8 Luis Medinas 2012-12-11 19:40:31 UTC
Created attachment 88921 [details]
acpidump
Comment 9 Luis Medinas 2012-12-11 19:47:46 UTC
grep . /sys/class/backlight/*/*
/sys/class/backlight/acpi_video0/actual_brightness:7
/sys/class/backlight/acpi_video0/bl_power:0
/sys/class/backlight/acpi_video0/brightness:7
/sys/class/backlight/acpi_video0/max_brightness:7
/sys/class/backlight/acpi_video0/type:firmware
/sys/class/backlight/acpi_video1/actual_brightness:7
/sys/class/backlight/acpi_video1/bl_power:0
/sys/class/backlight/acpi_video1/brightness:7
/sys/class/backlight/acpi_video1/max_brightness:7
/sys/class/backlight/acpi_video1/type:firmware
/sys/class/backlight/intel_backlight/actual_brightness:4648
/sys/class/backlight/intel_backlight/bl_power:1021770472
/sys/class/backlight/intel_backlight/brightness:929
/sys/class/backlight/intel_backlight/max_brightness:4648
/sys/class/backlight/intel_backlight/type:raw


yes poking /sys/class/backlight acpi_video0 and acpi_video1 works although i'm not able to change on X11 with FN-keys, but gnome-settings-daemon works.
This laptop contains nvidia optimus :(
Comment 10 Luis Medinas 2012-12-11 19:48:59 UTC
Another interesting thing is that if i load the samsung-laptop module all these backlight settings become useless and the display goes dark because samsung-laptop is not able to deal with this laptop.
Comment 11 Cheppes 2013-02-14 21:16:25 UTC
Created attachment 93381 [details]
acpidump

My acpidump, better late than never I guess.
Comment 12 Aaron Lu 2013-03-04 08:34:24 UTC
(In reply to comment #9)
> grep . /sys/class/backlight/*/*
> /sys/class/backlight/acpi_video0/actual_brightness:7
> /sys/class/backlight/acpi_video0/bl_power:0
> /sys/class/backlight/acpi_video0/brightness:7
> /sys/class/backlight/acpi_video0/max_brightness:7
> /sys/class/backlight/acpi_video0/type:firmware
> /sys/class/backlight/acpi_video1/actual_brightness:7
> /sys/class/backlight/acpi_video1/bl_power:0
> /sys/class/backlight/acpi_video1/brightness:7
> /sys/class/backlight/acpi_video1/max_brightness:7
> /sys/class/backlight/acpi_video1/type:firmware
> /sys/class/backlight/intel_backlight/actual_brightness:4648
> /sys/class/backlight/intel_backlight/bl_power:1021770472
> /sys/class/backlight/intel_backlight/brightness:929
> /sys/class/backlight/intel_backlight/max_brightness:4648
> /sys/class/backlight/intel_backlight/type:raw
> 
> 
> yes poking /sys/class/backlight acpi_video0 and acpi_video1 works although
> i'm

This means acpi can handle backlight.

> not able to change on X11 with FN-keys, but gnome-settings-daemon works.

Can you please check the value of /sys/firmware/acpi/interrupts/gpe17 both before and after you press the Fn-key? I want to know if EC fired an event or not, thanks.

BTW, do you boot with any acpi_osi= param?
Comment 13 Aaron Lu 2013-03-04 09:12:15 UTC
(In reply to comment #2)
> And here is what happens when I change the value:
> 
> # echo 0 >acpi_video0/brightness

Is there any error reported from dmesg?

> # grep -H '' */*(.)
> acpi_video0/actual_brightness:7

Each time actual_brightness is desired, we will evaluate an ACPI control method to get the result. So if the brightness is set correctly(at least from ACPI's perspective) but read the value out through another ACPI control method turned out to be not the case, I think there is a bug in the ACPI table.

> acpi_video0/bl_power:0
> acpi_video0/brightness:0
> acpi_video0/max_brightness:7
> acpi_video0/type:firmware
> intel_backlight/actual_brightness:309
> intel_backlight/bl_power:0
> intel_backlight/brightness:4648
> intel_backlight/max_brightness:4648
> intel_backlight/type:raw
> 
> The ACPI backend doesn't return the updated value for actual_brightness. This

Yes it returned, but the ACPI aml code thinks it is still at level 7(it determined this by reading some hardware register), even you have just changed the level successfully.
BTW, does the level really get changed after you echo 0?

> throws off the user space tools that rely on this interface, like the intel
> xorg video driver.
> 
> PS: the "samsung" interface that appears when setting acpi_backlight=vendor
> is
> even buggier.
Comment 14 Cheppes 2013-03-04 12:34:46 UTC
> Is there any error reported from dmesg?

No.

> BTW, does the level really get changed after you echo 0?

Yes.
Comment 15 Aaron Lu 2013-03-05 09:13:48 UTC
Created attachment 94531 [details]
show some debug message about acpi power video

Hi Cheppes,

Please apply this patch on top of v3.9-rc1 and attach the dmesg after boot, thanks.
Comment 16 Cheppes 2013-03-05 17:40:30 UTC
Created attachment 94591 [details]
dmesg

Done.
Comment 17 Aaron Lu 2013-03-13 06:56:16 UTC
Created attachment 95301 [details]
Debug DSDT table to fix BRTL

Hi Cheppes,

From the dsdt table, it looks like the asl code has a bug:
in the level set method _BCM, it will store the level value in BRTL like this:

   Method (_BCM, 1, NotSerialized)
   {
        If (LAnd (LGreaterEqual (Arg0, Zero), LLessEqual (Arg0, 0x64)))
        {
            AINT (One, Arg0)
            Store (Arg0, BRTL) -> it will be translated as \_SB\BRTL
        }
    }

But in the _BQC control method, it returned the value of \BRTL:
    Method (_BQC, 0, NotSerialized)
    {
        Return (\BRTL)
    }

Since there are two BRTL defined in your DSDT table, one is under the global namespace as \BRTL and the other under \_SB, and they have different addresses, so yes, I think _BQC will always return a constant value.

I've prepare a debug DSDT for you to test, where I've change the BRTL in _BCM to \BRTL. Let's see if it helps.
Comment 18 Cheppes 2013-03-13 23:56:07 UTC
It fixes the problem. Thanks.
Comment 19 Aaron Lu 2013-03-14 01:44:15 UTC
Hi Luis,

Are you still there? Can you please answer the questions in comment #12? Thanks.
Comment 20 Luis Medinas 2013-03-27 18:37:14 UTC
(In reply to comment #12)
> (In reply to comment #9)
> > grep . /sys/class/backlight/*/*
> > /sys/class/backlight/acpi_video0/actual_brightness:7
> > /sys/class/backlight/acpi_video0/bl_power:0
> > /sys/class/backlight/acpi_video0/brightness:7
> > /sys/class/backlight/acpi_video0/max_brightness:7
> > /sys/class/backlight/acpi_video0/type:firmware
> > /sys/class/backlight/acpi_video1/actual_brightness:7
> > /sys/class/backlight/acpi_video1/bl_power:0
> > /sys/class/backlight/acpi_video1/brightness:7
> > /sys/class/backlight/acpi_video1/max_brightness:7
> > /sys/class/backlight/acpi_video1/type:firmware
> > /sys/class/backlight/intel_backlight/actual_brightness:4648
> > /sys/class/backlight/intel_backlight/bl_power:1021770472
> > /sys/class/backlight/intel_backlight/brightness:929
> > /sys/class/backlight/intel_backlight/max_brightness:4648
> > /sys/class/backlight/intel_backlight/type:raw
> > 
> > 
> > yes poking /sys/class/backlight acpi_video0 and acpi_video1 works although
> i'm
> 
> This means acpi can handle backlight.
> 
> > not able to change on X11 with FN-keys, but gnome-settings-daemon works.
> 
> Can you please check the value of /sys/firmware/acpi/interrupts/gpe17 both
> before and after you press the Fn-key? I want to know if EC fired an event or
> not, thanks.
> 
> BTW, do you boot with any acpi_osi= param?

Hello Aaron,

sorry for the delay.

i dont boot with acpi_osi=

cat /sys/firmware/acpi/interrupts/gpe17 
     381   enabled

cat /sys/firmware/acpi/interrupts/gpe17 
     441   enabled
Comment 21 Luis Medinas 2013-03-27 18:43:15 UTC
The previous values was with acpi_backlight=vendor

this ones is without this option:

before:
cat /sys/firmware/acpi/interrupts/gpe17 
     301   enabled

after trigger FN+Brightness up

cat /sys/firmware/acpi/interrupts/gpe17 
     361   enabled


One more information, the samsung-laptop module does not support my laptop module. This laptop has nvidia optimus and intel.

This tests were done with Kernel 3.2.x from Ubuntu 12.04.
Comment 22 Aaron Lu 2013-03-28 03:09:54 UTC
Created attachment 96431 [details]
DSDT table for Luis

Hi Luis,

Please try this DSDT table, and do not use acpi_backlight=vendor with this table. Thanks.
Comment 23 Luis Medinas 2013-03-28 11:24:59 UTC
Hello Aaron,

works great! Thank you

Can we upstream this table ?
Comment 24 Aaron Lu 2013-03-29 06:47:46 UTC
Created attachment 96541 [details]
Patch to enhance the setting of _BQC_use_index

Hi luis and cheppes,

Please help to test this patch, you do not need to use the customized DSDT with this patch. Thanks.
Comment 25 Aaron Lu 2013-03-31 12:43:07 UTC
Created attachment 96761 [details]
Patch to enhance the quirk setting of _BQC

Please use this one, the previous one has a bug. Apply on top of v3.8. Thanks.
Comment 26 Cheppes 2013-03-31 18:03:04 UTC
Hi.

The bug in my DSDT table makes it always return the maximum value. The acpi_video_bqc_quirk function in your patch can't detect that because the maximum value is what it expects.

However, when changed to test for another value, the patch does detect the problem: the /sys/class/backlight/acpi_video0 no longer appears, and userspace tools fall back to /sys/class/backlight/intel_backlight
Comment 27 Aaron Lu 2013-04-01 01:12:04 UTC
(In reply to comment #26)
> Hi.
> 
> The bug in my DSDT table makes it always return the maximum value. The

This is quite unexpected, I don't know the firmware would initialize the global BRTL variable to be 100. Can you please check the content of the physical memory 0xAAF3FE80?

> acpi_video_bqc_quirk function in your patch can't detect that because the
> maximum value is what it expects.

Indeed, in this case, we can't detect the _BQC is broken.

> 
> However, when changed to test for another value, the patch does detect the

Some value like 80, 60, etc.?

> problem: the /sys/class/backlight/acpi_video0 no longer appears, and
> userspace
> tools fall back to /sys/class/backlight/intel_backlight

And this is unexpected too... since the patch is just to detect if _BQC is broken, it doesn't mean to disable acpi video backlight control functionality.
Comment 28 Aaron Lu 2013-04-02 05:42:03 UTC
Hi Luis,

What about your case?
Comment 29 Cheppes 2013-04-02 18:54:20 UTC
Created attachment 97081 [details]
Patch to enhance the detection of buggy _BQC

Attached is a small patch to apply on top of yours. It does this: if _BQC is already returning the maximum level, we test the minimum level instead.

It seems to work fine on my system. The /sys/class/backlight/acpi_video0 interface remains and works properly.

(In reply to comment #27)
> This is quite unexpected, I don't know the firmware would initialize the
> global
> BRTL variable to be 100. Can you please check the content of the physical
> memory 0xAAF3FE80?

What is the correct way of doing that ? I tried one but I'm not sure I got it right.
Comment 30 Aaron Lu 2013-04-03 03:10:19 UTC
(In reply to comment #29)
> Created an attachment (id=97081) [details]
> Patch to enhance the detection of buggy _BQC
> 
> Attached is a small patch to apply on top of yours. It does this: if _BQC is
> already returning the maximum level, we test the minimum level instead.
> 

OK, thanks for the suggestion. I'll prepare a new patch to do this, and please kindly give it a review and test.

> It seems to work fine on my system. The /sys/class/backlight/acpi_video0
> interface remains and works properly.
> 
> (In reply to comment #27)
> > This is quite unexpected, I don't know the firmware would initialize the
> global
> > BRTL variable to be 100. Can you please check the content of the physical
> > memory 0xAAF3FE80?
> 
> What is the correct way of doing that ? I tried one but I'm not sure I got it
> right.

# dd if=/dev/mem of=memdump bs=4096 count=1 skip=700223
then check the value at offset 0xe80.
Comment 31 Aaron Lu 2013-04-03 03:13:36 UTC
Created attachment 97121 [details]
Enhance the quirk detect logic of _BQC

Please apply this patch on top of Rafael's linux-next branch:

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

Since I'm going to propose it upstream, we had better use an upstream tree :-)
Comment 32 Aaron Lu 2013-04-03 03:16:35 UTC
*** Bug 56011 has been marked as a duplicate of this bug. ***
Comment 33 Artem Savkov 2013-04-03 06:09:13 UTC
Hi Aaron,

I've tested this on top of next-20130402 and everything seems fine to me, except that there is a typo in your patch resulting in the following compilation error:

drivers/acpi/video.c: In function ‘acpi_video_bqc_quirk’:
drivers/acpi/video.c:657:46: error: ‘struct acpi_video_device_brightness’ has no member named ‘level’

'->level' on that line should be '->levels'
Comment 34 Aaron Lu 2013-04-03 06:27:26 UTC
(In reply to comment #33)
> Hi Aaron,
> 
> I've tested this on top of next-20130402 and everything seems fine to me,
> except that there is a typo in your patch resulting in the following
> compilation error:
> 
> drivers/acpi/video.c: In function ‘acpi_video_bqc_quirk’:
> drivers/acpi/video.c:657:46: error: ‘struct acpi_video_device_brightness’ has
> no member named ‘level’
> 
> '->level' on that line should be '->levels'

Thanks for spotting this! I'll attach the updated patch here.
Comment 35 Aaron Lu 2013-04-03 06:32:31 UTC
Created attachment 97151 [details]
Enhance the quirk detect logic of _BQC

Change:
1 Fix a typo in the code as spotted by Artem;
1 Added Artem's reported-and-tested-by tag.
Comment 36 Aaron Lu 2013-05-06 01:00:30 UTC
commit a50188dae3089dcd15a6ae793528c157680891f1
Author: Aaron Lu <aaron.lu@intel.com>
Date:   Mon Apr 22 14:08:32 2013 +0200

    acpi: video: enhance the quirk detect logic of _BQC