Bug 104121

Summary: Esprimo Mobile 9410: Two acpi_video* devices in sysfs, only one working
Product: ACPI Reporter: Christian Scharl (zahlsum-kernelbugs)
Component: Power-VideoAssignee: Aaron Lu (aaron.lu)
Status: CLOSED CODE_FIX    
Severity: normal CC: aaron.lu, jani.nikula, jwrdegoede
Priority: P1    
Hardware: x86-64   
OS: Linux   
Kernel Version: 4.2.0-0.rc5 Tree: Fedora
Regression: No
Attachments: acpidump of Esprimo Mobile M9410
Output of dmesg
Output of lshw
Only register backlight for LCD type video output device
Only register backlight for LCD type video output device
Only register backlight for LCD type video output device, also quirk device_id_scheme
Output of dmidecode
Screen output of crash
dmesg output with console=ttyS0,19200
dmesg output with console=ttyS0,19200 video.only_lcd=true video.device_id_scheme=true
Only register backlight for LCD type video output device

Description Christian Scharl 2015-09-06 19:09:45 UTC
Created attachment 186931 [details]
acpidump of Esprimo Mobile M9410

Under /sys/class/backlight there are two entries:
[xxx@yyy ~]$ ls -l /sys/class/backlight/
lrwxrwxrwx. 1 root root 0  6. Sep 2015  acpi_video0 -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0
lrwxrwxrwx. 1 root root 0  6. Sep 2015  acpi_video1 -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video1

When writing manually a new value to acpi_video0/brightness the backlight is not changing. Do the same with acpi_video1/brightness is changing the backlight.

Both entries are of type firmware.
As a result, the backlight can not be changed from the gnome shell slider (gnome 3.16), gsd-backlight-helper and xbacklight. xbacklight is working, when acpi_video1 is defined as the backlight device in an xorg.conf.d file.

Here the output of all files in the acpi_video* folder. In addition I attache an acpidump. If any additional information is needed, please let me know and I try to get it.

[xxx@yyy ~]$ grep . -r /sys/class/backlight/*
/sys/class/backlight/acpi_video0/type:firmware
/sys/class/backlight/acpi_video0/brightness:4
/sys/class/backlight/acpi_video0/power/control:auto
/sys/class/backlight/acpi_video0/power/async:disabled
/sys/class/backlight/acpi_video0/power/runtime_enabled:disabled
/sys/class/backlight/acpi_video0/power/runtime_active_kids:0
/sys/class/backlight/acpi_video0/power/runtime_active_time:0
grep: /sys/class/backlight/acpi_video0/power/autosuspend_delay_ms: Eingabe-/Ausgabefehler
/sys/class/backlight/acpi_video0/power/runtime_status:unsupported
/sys/class/backlight/acpi_video0/power/runtime_usage:0
/sys/class/backlight/acpi_video0/power/runtime_suspended_time:0
/sys/class/backlight/acpi_video0/bl_power:0
/sys/class/backlight/acpi_video0/max_brightness:10
/sys/class/backlight/acpi_video0/actual_brightness:4
/sys/class/backlight/acpi_video1/type:firmware
/sys/class/backlight/acpi_video1/brightness:7
/sys/class/backlight/acpi_video1/power/control:auto
/sys/class/backlight/acpi_video1/power/async:disabled
/sys/class/backlight/acpi_video1/power/runtime_enabled:disabled
/sys/class/backlight/acpi_video1/power/runtime_active_kids:0
/sys/class/backlight/acpi_video1/power/runtime_active_time:0
grep: /sys/class/backlight/acpi_video1/power/autosuspend_delay_ms: Eingabe-/Ausgabefehler
/sys/class/backlight/acpi_video1/power/runtime_status:unsupported
/sys/class/backlight/acpi_video1/power/runtime_usage:0
/sys/class/backlight/acpi_video1/power/runtime_suspended_time:0
/sys/class/backlight/acpi_video1/bl_power:0
/sys/class/backlight/acpi_video1/max_brightness:7
/sys/class/backlight/acpi_video1/actual_brightness:7
Comment 1 Aaron Lu 2015-09-08 03:09:29 UTC
Maybe due to you have two graphics card and acpi_video0 corresponds to the discrete one and since the backlight is actually controlled by the integrated one, so acpi_video0 doesn't work.

Please attach your dmesg.
Comment 2 Christian Scharl 2015-09-08 05:32:19 UTC
As far as I know, there is no discrete graphic card present in this laptop.
I add the aoutput of dmesg and lshw. 
lshw shows two displays, but I have not connected an external display to the laptop.
Comment 3 Christian Scharl 2015-09-08 05:33:02 UTC
Created attachment 187031 [details]
Output of dmesg
Comment 4 Christian Scharl 2015-09-08 05:33:31 UTC
Created attachment 187041 [details]
Output of lshw
Comment 5 Aaron Lu 2015-09-09 08:52:44 UTC
Hmm, not two graphics controller but one controller has two video output devices that claim to be capable of adjusting backlight.
Tough, not easy to decide which one is the right one.
Comment 6 Christian Scharl 2015-09-09 18:07:32 UTC
What exactly does "video output device" mean? Does this refer only to a physical monitor connected to the graphic controller (graphic card?) or can this be a virtual output, too?

Is there a way to identify which of these two video output devices is used for the actual video output (internal monitor panel of the laptop). Maybe this can be used to decide which device shall be exposed by the kernel.

At the moment I have no idea where the second output device is coming from. xrandr and gnome only list one monitor.
Comment 7 Aaron Lu 2015-09-10 01:37:31 UTC
It's a word used in the ACPI spec, see "Appendix B Video Extensions" of the ACPI spec if you are interested.

The video output device means the connected monitors, like LCD, CRT, etc and these devices may be connected through different ports like VGA, HDMI, etc.

There is information about which kind of device it is(LCD or CRT or), and your ACPI table defines two:
                Device (DD03)
                {
                    Name (_ADR, 0x0320)  // _ADR: Address
                    Name (_DCS, 0x1F)  // _DCS: Display Current Status
                    Name (_DGS, Zero)  // _DGS: Display Graphics State
                    Method (_BCM, 1, NotSerialized)  // _BCM: Brightness Control Method
                    {
                        Divide (Arg0, 0x0A, Local0, Local1)
                        If ((Local0 == 0x00))
                        {
                            BRTW (Arg0)
                        }
                    }
current kernel doesn't 
                }

                Device (DD04)
                {
                    Name (_ADR, 0x0410)  // _ADR: Address
                    Name (_DCS, 0x1F)  // _DCS: Display Current Status
                    Name (_DGS, Zero)  // _DGS: Display Graphics State
                    Method (_BCM, 1, NotSerialized)  // _BCM: Brightness Control Method
                    {
                        If (^^^LPCB.H_EC.ECON)
                        {
                            If (^^^LPCB.H_EC.ECLS)
                            {
                                BRIB = Arg0
                                ^^^LPCB.H_EC.MBX2 = Arg0
                                ^^^LPCB.H_EC.MBX1 = 0x10
                            }
                        }
                    }
                }

According to the spec, device DD04, whose _ADR is 0x0410 is the internal digital panel while the device DD03, whose _ADR is 0x0320 is an external digital monitor(DVI monitor?).

The acpi video module, who is responsible for the acpi_videoX interface, doesn't have the knowledge of which output device is currently connected, the graphics driver does.

OTOH, it doesn't seem possible for the backlight of the external monitor being controlled by the ACPI interface, Jani and Hans, do you have any information on this? 

I was thinking not creating acpi_videoX interface for video output devices that is not the type of internal digital panel to avoid the trouble since the majority of the people may not expect to control the backlight of the external monitor through laptop I guess and even if they want to do that, I wonder how that can work? Is there a standard protocol that all monitors support that the OS can use to do the backlight control?
Comment 8 Christian Scharl 2015-09-29 05:12:18 UTC
Is there any missing information I can provide to help solving the issue?
Comment 9 Aaron Lu 2015-09-29 05:59:36 UTC
No more information required, I just don't know how to fix it.
Comment 10 Hans de Goede 2015-09-29 08:17:49 UTC
Hi,

(In reply to Aaron Lu from comment #9)
> No more information required, I just don't know how to fix it.

In comment 7 you wrote: "I was thinking not creating acpi_videoX interface for video output devices that is not the type of internal digital panel", how about doing that, maybe combined with a quirk to only enable this behavior on some devices to avoid regressions ?

Regards,

Hans
Comment 11 Aaron Lu 2015-09-29 08:37:20 UTC
I can try that, but I have a feeling it may cause regressions for other systems...
Comment 12 Hans de Goede 2015-09-29 08:41:29 UTC
(In reply to Aaron Lu from comment #11)
> I can try that, but I have a feeling it may cause regressions for other
> systems...

Right, which is why I suggested enabling it via a quirk, and for now only setting that quirk (based on dmi matching) for the Esprimo Mobile 9410...

Not pretty, but it seems like something which may be useful on a hand of other models, so it is probably good to add support for this extra check, and by only enabling the extra check via quirk (and kernelcmdline option for testing) we avoid regressions.
Comment 13 Aaron Lu 2015-09-29 08:51:16 UTC
Created attachment 188991 [details]
Only register backlight for LCD type video output device

Worth a try :-)
Comment 14 Aaron Lu 2015-09-29 08:55:35 UTC
(In reply to Hans de Goede from comment #12)
> (In reply to Aaron Lu from comment #11)
> > I can try that, but I have a feeling it may cause regressions for other
> > systems...
> 
> Right, which is why I suggested enabling it via a quirk, and for now only
> setting that quirk (based on dmi matching) for the Esprimo Mobile 9410...

Oh, I didn't notice the latter part of comment #10.

> 
> Not pretty, but it seems like something which may be useful on a hand of
> other models, so it is probably good to add support for this extra check,
> and by only enabling the extra check via quirk (and kernelcmdline option for
> testing) we avoid regressions.

What about we just do it for all, and if people have problems then we add the quirk?
On another thought, it feels very likely to trigger regressions, so yes, I'll add the quirk(the patch in comment #13 didn't yet).
Comment 15 Aaron Lu 2015-09-30 06:35:44 UTC
Created attachment 189061 [details]
Only register backlight for LCD type video output device

As suggested by Hans, I've used the quirk now. Please test this patch, thanks.
Comment 16 Christian Scharl 2015-09-30 21:13:22 UTC
I have applied the patch from comment #15 on top of linux 4.3-rc3.
Booting this kernel, there are still two acpi_video* folders in /sys/class/backlight.
The behavior is the same as before. acpi_video1 can change the backlight, changes to acpi_video0 have no effect.

This is my first patched kernel. Is there a way to check if I applied the patch correctly?
(I checked that the source file contain the modification and I checked that the newly built kernel is loaded with uname -r)
Comment 17 Hans de Goede 2015-10-01 08:35:12 UTC
Hi,

(In reply to Christian Scharl from comment #16)
> I have applied the patch from comment #15 on top of linux 4.3-rc3.
> Booting this kernel, there are still two acpi_video* folders in
> /sys/class/backlight.
> The behavior is the same as before. acpi_video1 can change the backlight,
> changes to acpi_video0 have no effect.
> 
> This is my first patched kernel. Is there a way to check if I applied the
> patch correctly?
> (I checked that the source file contain the modification and I checked that
> the newly built kernel is loaded with uname -r)

Maybe the dmi mathcing for the quirk is not working, can you try adding video.only_lcd=1 to your kernel commandline ?

Regards,

Hans
Comment 18 Christian Scharl 2015-10-01 16:23:26 UTC
Thanks for the tip. When adding video.only_lcd=1 to the kernel command line there is no directory under /sys/class/backlight/.
Comment 19 Hans de Goede 2015-10-01 16:50:26 UTC
Hi,

(In reply to Christian Scharl from comment #18)
> Thanks for the tip. When adding video.only_lcd=1 to the kernel command line
> there is no directory under /sys/class/backlight/.

You mean no directory at all, not only one as we want, right ?

Aaron, it looks like your patch needs some work then ...

Regards,

Hans
Comment 20 Christian Scharl 2015-10-01 17:31:52 UTC
Exactly. The directory is empty and no acpi_video* directory is present.

Best regards
Christian
Comment 21 Aaron Lu 2015-10-08 02:07:45 UTC
Thanks for the input and sorry for the delay, it's National holiday here in China. I'll take a look at the patches now.
Comment 22 Aaron Lu 2015-10-08 05:41:39 UTC
                Name (_DOD, Package (0x04)  // _DOD: Display Output Devices
                {
                    0x00010410,
                    0x00010100,
                    0x00010320,
                    0x00010240
                })

I just noticed the above number doesn't have bit 31 set, which means if we can explain the other bits according to ACPI spec; if it's not set, it means it doesn't follow the format specified by the ACPI spec, so the code doesn't set LCD flag for the device...
Comment 23 Aaron Lu 2015-10-08 07:17:12 UTC
Created attachment 189701 [details]
Only register backlight for LCD type video output device, also quirk device_id_scheme

Please try this patch, and since last patch doesn't correctly handle DMI entry, please also attach the output of dmidecode. To skip the DMI issue, you can also add video.only_lcd=true video.device_id_scheme=true to your kernel cmdline.

I added a debug print for device_id_scheme, so if you see it in dmesg, it means you have patched the kernel well.

Changes since last version:
I also did a quirk for the device_id_scheme in this patch, I don't find anything better to deal with it but if you have any ideas, please let me know, thanks.
Comment 24 Aaron Lu 2015-10-15 02:09:25 UTC
Any news?
Comment 25 Christian Scharl 2015-10-16 05:01:05 UTC
Sorry for the delay. I tested the patch end of last week with 4.3-rc3, but the kernel was crashing at startup (with and without the provided cmdline parameters). Unfortunately I had no time to check if the crash happens without the patch, too.
I'll come back as soon as I could test without the patch.

Regards
Christian
Comment 26 Christian Scharl 2015-10-17 10:57:14 UTC
Created attachment 190421 [details]
Output of dmidecode

Please find attached the output of dmidecode.

Regards
Christian
Comment 27 Christian Scharl 2015-10-17 11:01:37 UTC
Created attachment 190431 [details]
Screen output of crash

I tested again with an unmodified version of 4.3-rc4 and with the applied patch.
The unmodified version boots without problems. With the applied patch and adding no parameter, I get the attached output on the screen and the system freezes (no new output after some minutes).
With the cmdline parameters the behavior is similar.

Please let me know if more information are needed or if I can test something.

Regards
Christian
Comment 28 Aaron Lu 2015-10-19 04:12:17 UTC
I can't tell what happened there with the screenshot, it missed the starting part of the oops...
I tried to reproduce it locally but failed(using a different system so may not mean anything).

Possible to get the full trace of the oops? Not sure if USB-serial can help here, that depends on if USB driver is loaded earlier than i915. Net console may also worth a try.
Comment 29 Christian Scharl 2015-10-21 20:18:57 UTC
Thanks for the tip. The docking port has a native serial port, so I will try to catch the oops over the serial port. Currently I am missing the correct cable, but I will get it on the weekend.

I'll report back, when I get the output working.

Regards
Christian
Comment 30 Christian Scharl 2015-10-25 21:10:35 UTC
When trying to get the kernel output over the serial port, it is starting without this crash.
I added the following parameters to the cmdline:
console=tty console=ttyS0,19200

When I add console=tty only, I get the crash. With the second parameter it does not happen. But I only get a console output and the window manager is not starting.

When booting with the second option, I checked the content of /sys/class/backlight and the directory is empty.
Is the second parameter preventing that the intel driver is loaded?

At the moment I do not know how to debug the problem further. If you have any idea, please let me know.

Please can you tell me with which kernel version you did the test? I want to try what results I get with this version.

Regards
Christian
Comment 31 Aaron Lu 2015-10-26 02:21:07 UTC
(In reply to Christian Scharl from comment #30)
> When trying to get the kernel output over the serial port, it is starting
> without this crash.
> I added the following parameters to the cmdline:
> console=tty console=ttyS0,19200

Did you get any serial log with console=ttyS0,19200?

> 
> When I add console=tty only, I get the crash. With the second parameter it
> does not happen. But I only get a console output and the window manager is
> not starting.
> 
> When booting with the second option, I checked the content of
> /sys/class/backlight and the directory is empty.
> Is the second parameter preventing that the intel driver is loaded?

No, it shouldn't.

> 
> At the moment I do not know how to debug the problem further. If you have
> any idea, please let me know.

dmesg with console=ttyS0,19200 please.

> 
> Please can you tell me with which kernel version you did the test? I want to
> try what results I get with this version.

It should be v4.3-rc5 when I did the test, but I don't think it matters much.

Thanks for the test.
Comment 32 Christian Scharl 2015-10-26 21:18:26 UTC
Created attachment 191181 [details]
dmesg output with console=ttyS0,19200

This is the dmesg output when booting with console=ttyS0,19200.
Comment 33 Christian Scharl 2015-10-26 21:22:44 UTC
Created attachment 191191 [details]
dmesg output with console=ttyS0,19200 video.only_lcd=true video.device_id_scheme=true

This is the dmesg output with console=ttyS0,19200 video.only_lcd=true video.device_id_scheme=true

In the dmesg and on the console there are a lot of messages of this type (repeating every few seconds):
video: `true' invalid for parameter `only_lcd'

Could it be that I have not applied the patch correctly?

Regards
Christian
Comment 34 Aaron Lu 2015-10-27 03:15:56 UTC
Created attachment 191211 [details]
Only register backlight for LCD type video output device

Sorry, my mistake.
The debug print I added in last patch didn't check for NULL pointer that caused the problem.
And the param needs to be set as:
video.only_lcd=1 instead of true.

Please use this patch, and test both with and without the "video.only_lcd=1 video.device_id_scheme=1" kernel cmdline options to see the effect, thanks.
Comment 35 Christian Scharl 2015-10-27 20:49:10 UTC
Dear Aaron,

thanks a lot for your work. The last patch works perfectly. I applied the patch on top of 4.3.0-rc7.

With and without the command line parameters, /sys/class/backlight contains only one folder and it is now possible to change the backlight level with the gnome shell slider and the hotkeys.

Do you need any additional information?

Thanks
Christian
Comment 36 Aaron Lu 2015-10-28 01:21:10 UTC
No more info needed, I'll add your reported-and-tested-by tag to the patch and submit it later, thanks a lot for the test.
Comment 37 Aaron Lu 2015-10-28 07:09:13 UTC
Patch sent out:
https://patchwork.kernel.org/patch/7507631/
Comment 38 Aaron Lu 2015-11-13 03:18:13 UTC
commit e50b9be14ab0ed10b0b3cd4112ff4bed0abf7b6f
Author: Aaron Lu <aaron.lu@intel.com>
Date:   Wed Oct 28 15:09:23 2015 +0800

    ACPI / video: only register backlight for LCD device

as v4.4-rc1 material.