Bug 42861

Created attachment 72537 [details]
dmidecode of my laptop

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.

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.

ping...

ping...

ping...

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

Created attachment 88921 [details]
acpidump

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 :(

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.

Created attachment 93381 [details]
acpidump

My acpidump, better late than never I guess.

(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?

(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.

> Is there any error reported from dmesg?

No.

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

Yes.

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.

Created attachment 94591 [details]
dmesg

Done.

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.

It fixes the problem. Thanks.

Hi Luis,

Are you still there? Can you please answer the questions in comment #12? Thanks.

(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

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.

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.

Hello Aaron,

works great! Thank you

Can we upstream this table ?

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.

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.

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

(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.

Hi Luis,

What about your case?

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.

(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.

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 :-)

*** Bug 56011 has been marked as a duplicate of this bug. ***

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'

(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.

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.

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