ACPI / video: Default lcd_only to true on Win8 ready and newer machines

PATCH Applied Hans de Goede 3 participants 8 messages Dec 23, 2017 – Apr 17, 2018

Subsystems (2)

Stable branch ACPI

lore

Share & tools

lore

Hans de Goede

author

2017-12-23 18:41 UTC #0 lore raw

PATCH v2 1/1 ACPI / video: Default lcd_only to true on Win8 ready and newer machines

We're seeing a lot of bogus backlight interfaces on newer machines without a LCD such as desktops, servers and HDMI sticks. This causes userspace to show a non-functional brightness slider in e.g. the GNOME3 system menu, which is undesirable. More in general we should simply just not register a non functional backlight interface.

Checking the lcd flag causes the bogus acpi_video backlight interfaces to go away (on the machines this was tested on).

This commit enables the lcd_only option by default on any machines which are win8 ready, fixing this.

This is not entirely without risk of regressions, but video_detect.c already prefers native-backlight interfaces over the acpi_video one on win8 ready machines, calling acpi_video_unregister_backlight() as soon as a native interface shows up. This is done because the acpi backlight interface often is broken on win8 ready machines, because win8 does not seem to actually use it.

So in practice we already end up not registering the acpi backlight interface on (most) win8 ready machines with a LCD panel, thus this commit does not change anything for (most) machines with a LCD panel and on machines without a LCD panel we actually don't want to register any backlight interfaces.

This has been tested on the following machines and fixes a bogus backlight interface showing up there:
-Desktop with an Asrock B150M Pro4S/D3 m.b. using i5-6500 builtin gfx -Intel Compute Stick STK1AW32SC
-Meegopad T08 HDMI stick

Bogus backlight interfaces have also been reported on:
-Desktop with Asus H87I-Plus m.b.
-Desktop with ASRock B75M-ITX m.b.
-Desktop with Gigabyte Z87-D3HP m.b.
-Dell PowerEdge T20 desktop

BugLink https://bugzilla.redhat.com/show_bug.cgi?id=1097436

BugLink https://bugzilla.redhat.com/show_bug.cgi?id=1133327

BugLink https://bugzilla.redhat.com/show_bug.cgi?id=1133329

BugLink https://bugzilla.redhat.com/show_bug.cgi?id=1133646

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Simplify: "only_lcd = acpi_osi_is_win8() ? 1 : 0;" to:
"only_lcd = acpi_osi_is_win8();"

---
 drivers/acpi/acpi_video.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

drivers/acpi/acpi_video.c +12 -2

index 0972ec0e2eb8..f53ccc680238 100644
@@ -80,8 +80,8 @@ MODULE_PARM_DESC(report_key_events,
 static bool device_id_scheme = false;
 module_param(device_id_scheme, bool, 0444);
 
-static bool only_lcd = false;
-module_param(only_lcd, bool, 0444);
+static int only_lcd = -1;
+module_param(only_lcd, int, 0444);
 
 static int register_count;
 static DEFINE_MUTEX(register_count_mutex);

@@ -2136,6 +2136,16 @@ int acpi_video_register(void)
 		goto leave;
 	}
 
+	/*
+	 * We're seeing a lot of bogus backlight interfaces on newer machines
+	 * without a LCD such as desktops, servers and HDMI sticks. Checking
+	 * the lcd flag fixes this, so enable this on any machines which are
+	 * win8 ready (where we also prefer the native backlight driver, so
+	 * normally the acpi_video code should not register there anyways).
+	 */
+	if (only_lcd == -1)
+		only_lcd = acpi_osi_is_win8();
+
 	dmi_check_system(video_dmi_table);
 
 	ret = acpi_bus_register_driver(&acpi_video_bus);

Signature · 1 line

2.14.3

Rafael J. Wysocki

Applied

2018-01-03 11:56 UTC #1 lore raw

Quoted 73 lines · CET Hans de Goede

We're seeing a lot of bogus backlight interfaces on newer machines without a LCD such as desktops, servers and HDMI sticks. This causes userspace to show a non-functional brightness slider in e.g. the GNOME3 system menu, which is undesirable. More in general we should simply just not register a non functional backlight interface.
Checking the lcd flag causes the bogus acpi_video backlight interfaces to go away (on the machines this was tested on).
This commit enables the lcd_only option by default on any machines which are win8 ready, fixing this.
This is not entirely without risk of regressions, but video_detect.c already prefers native-backlight interfaces over the acpi_video one on win8 ready machines, calling acpi_video_unregister_backlight() as soon as a native interface shows up. This is done because the acpi backlight interface often is broken on win8 ready machines, because win8 does not seem to actually use it.
So in practice we already end up not registering the acpi backlight interface on (most) win8 ready machines with a LCD panel, thus this commit does not change anything for (most) machines with a LCD panel and on machines without a LCD panel we actually don't want to register any backlight interfaces.
This has been tested on the following machines and fixes a bogus backlight interface showing up there:
(quoted diff) +0 -3
-Desktop with an Asrock B150M Pro4S/D3 m.b. using i5-6500 builtin gfx
-Intel Compute Stick STK1AW32SC
-Meegopad T08 HDMI stick
Bogus backlight interfaces have also been reported on:
(quoted diff) +0 -4
-Desktop with Asus H87I-Plus m.b.
-Desktop with ASRock B75M-ITX m.b.
-Desktop with Gigabyte Z87-D3HP m.b.
-Dell PowerEdge T20 desktop
BugLink https://bugzilla.redhat.com/show_bug.cgi?id=1097436
BugLink https://bugzilla.redhat.com/show_bug.cgi?id=1133327
BugLink https://bugzilla.redhat.com/show_bug.cgi?id=1133329
BugLink https://bugzilla.redhat.com/show_bug.cgi?id=1133646
Signed-off-by Hans de Goede <[email protected]>
---
Changes in v2:
(quoted diff) +0 -2
-Simplify: "only_lcd = acpi_osi_is_win8() ? 1 : 0;" to:
 "only_lcd = acpi_osi_is_win8();"
---
 drivers/acpi/acpi_video.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
drivers/acpi/acpi_video.c +5 -2
index 0972ec0e2eb8..f53ccc680238 100644
@@ -80,8 +80,8 @@ MODULE_PARM_DESC(report_key_events,
 static bool device_id_scheme = false;
 module_param(device_id_scheme, bool, 0444);
 
-static bool only_lcd = false;
-module_param(only_lcd, bool, 0444);
+static int only_lcd = -1;
+module_param(only_lcd, int, 0444);
 
 static int register_count;
 static DEFINE_MUTEX(register_count_mutex);
@@ -2136,6 +2136,16 @@ int acpi_video_register(void)
 		goto leave;
 	}
 
+	/*
+	 * We're seeing a lot of bogus backlight interfaces on newer machines
+	 * without a LCD such as desktops, servers and HDMI sticks. Checking

drivers/acpi/acpi_video.c int acpi_video_register(void) · line 2151

Quoted 10 lines

drivers/acpi/acpi_video.c +7 -0

+	 * the lcd flag fixes this, so enable this on any machines which are
+	 * win8 ready (where we also prefer the native backlight driver, so
+	 * normally the acpi_video code should not register there anyways).
+	 */
+	if (only_lcd == -1)
+		only_lcd = acpi_osi_is_win8();
+
 	dmi_check_system(video_dmi_table);
 
 	ret = acpi_bus_register_driver(&acpi_video_bus);

Applied, thanks!

James Hogan

2018-04-14 23:07 UTC #2 lore raw

PATCH v2 1/1 ACPI / video: Default lcd_only to true on Win8 ready and newer machines

In reply to Hans de Goede

Quoted 12 lines · Hans de Goede

We're seeing a lot of bogus backlight interfaces on newer machines without a LCD such as desktops, servers and HDMI sticks. This causes userspace to show a non-functional brightness slider in e.g. the GNOME3 system menu, which is undesirable. More in general we should simply just not register a non functional backlight interface.
Checking the lcd flag causes the bogus acpi_video backlight interfaces to go away (on the machines this was tested on).
This commit enables the lcd_only option by default on any machines which are win8 ready, fixing this.

Quoted 6 lines

This is not entirely without risk of regressions, but video_detect.c already prefers native-backlight interfaces over the acpi_video one on win8 ready machines, calling acpi_video_unregister_backlight() as soon as a native interface shows up. This is done because the acpi backlight interface often is broken on win8 ready machines, because win8 does not seem to actually use it.

This patch (in the form of commit 965736ee654d ("ACPI / video: Default lcd_only to true on Win8-ready and newer machines") in stable v4.15.17), breaks backlight control on my 2013 XPS13 laptop.

It normally uses the acpi backlight device, but after this patch that device no longer shows up in sysfs.

This isn't the first time the backlight has gotton broken on this system, though I think last time it was because the intel backlight driver got used instead of the ACPI one and that didn't work properly with it, so it needed a quirk to make it use ACPI instead.

Is some other quirk needed around here too?

Cheers
James

Hans de Goede

author

2018-04-15 12:04 UTC #3 lore raw

Hi,

Quoted 27 lines · James Hogan

Quoted 12 lines · Hans de Goede
We're seeing a lot of bogus backlight interfaces on newer machines without a LCD such as desktops, servers and HDMI sticks. This causes userspace to show a non-functional brightness slider in e.g. the GNOME3 system menu, which is undesirable. More in general we should simply just not register a non functional backlight interface.
Checking the lcd flag causes the bogus acpi_video backlight interfaces to go away (on the machines this was tested on).
This commit enables the lcd_only option by default on any machines which are win8 ready, fixing this.
Quoted 6 lines
This is not entirely without risk of regressions, but video_detect.c already prefers native-backlight interfaces over the acpi_video one on win8 ready machines, calling acpi_video_unregister_backlight() as soon as a native interface shows up. This is done because the acpi backlight interface often is broken on win8 ready machines, because win8 does not seem to actually use it.
This patch (in the form of commit 965736ee654d ("ACPI / video: Default lcd_only to true on Win8-ready and newer machines") in stable v4.15.17), breaks backlight control on my 2013 XPS13 laptop.
It normally uses the acpi backlight device, but after this patch that device no longer shows up in sysfs.

Quoted 6 lines

This isn't the first time the backlight has gotton broken on this system, though I think last time it was because the intel backlight driver got used instead of the ACPI one and that didn't work properly with it, so it needed a quirk to make it use ACPI instead.
Is some other quirk needed around here too?

Yes looks like it (sigh), but I cannot find an existing quirk for your laptop. When you say quirk do you mean you add something to the kernel commandline to work around this? Normally we would add a dmi based quirk so that future versions automatically do the right thing. I was looking for the existing quirk so that I could re-use the dmi strings, but I don't see an existing quirk.

Can you run:

sudo dmidecode > dell-xps13-2013-dmi.log

And then send me a *personal* mail with that file attached. It is best to not send this to the list as it will contain you machines serial number and other unique information.

Regards,

Hans

James Hogan

2018-04-15 22:54 UTC #4 lore raw

Quoted 35 lines · Hans de Goede

Hi,
Quoted 32 lines · James Hogan
Quoted 12 lines · Hans de Goede
We're seeing a lot of bogus backlight interfaces on newer machines without a LCD such as desktops, servers and HDMI sticks. This causes userspace to show a non-functional brightness slider in e.g. the GNOME3 system menu, which is undesirable. More in general we should simply just not register a non functional backlight interface.
Checking the lcd flag causes the bogus acpi_video backlight interfaces to go away (on the machines this was tested on).
This commit enables the lcd_only option by default on any machines which are win8 ready, fixing this.
Quoted 6 lines
This is not entirely without risk of regressions, but video_detect.c already prefers native-backlight interfaces over the acpi_video one on win8 ready machines, calling acpi_video_unregister_backlight() as soon as a native interface shows up. This is done because the acpi backlight interface often is broken on win8 ready machines, because win8 does not seem to actually use it.
This patch (in the form of commit 965736ee654d ("ACPI / video: Default lcd_only to true on Win8-ready and newer machines") in stable v4.15.17), breaks backlight control on my 2013 XPS13 laptop.
It normally uses the acpi backlight device, but after this patch that device no longer shows up in sysfs.
This isn't the first time the backlight has gotton broken on this system, though I think last time it was because the intel backlight driver got used instead of the ACPI one and that didn't work properly with it, so it needed a quirk to make it use ACPI instead.

Quoted 4 lines

Quoted 1 line
Is some other quirk needed around here too?
Yes looks like it (sigh), but I cannot find an existing quirk for your laptop.

I did at one point have to revert commit bc0bb9fd1c78 ("drm/i915: remove QUIRK_NO_PCH_PWM_ENABLE") for a few kernel releases until 4.3, presumably fixed by commit b029e66fa8e3 ("drm/i915: Backlight control using CRC PMIC based PWM driver").

Quoted 2 lines

When you say quirk do you mean you add something to the kernel commandline to work around this?

No, it was automatic, in the source. I don't know this code well enough to know how they got it working.

Quoted 5 lines

Normally we would add
a dmi based quirk so that future versions automatically do the right thing. I was looking for the existing quirk so that I could re-use the dmi strings, but I don't see an existing quirk.

Quoted 7 lines

Can you run:
sudo dmidecode > dell-xps13-2013-dmi.log
And then send me a *personal* mail with that file attached. It is best to not send this to the list as it will contain you machines serial number and other unique information.

Will do.

Thanks
James

Hans de Goede

author

2018-04-16 11:07 UTC #5 lore raw

Hi,

Quoted 46 lines · James Hogan

Quoted 35 lines · Hans de Goede
Hi,
Quoted 32 lines · James Hogan
Quoted 12 lines · Hans de Goede
We're seeing a lot of bogus backlight interfaces on newer machines without a LCD such as desktops, servers and HDMI sticks. This causes userspace to show a non-functional brightness slider in e.g. the GNOME3 system menu, which is undesirable. More in general we should simply just not register a non functional backlight interface.
Checking the lcd flag causes the bogus acpi_video backlight interfaces to go away (on the machines this was tested on).
This commit enables the lcd_only option by default on any machines which are win8 ready, fixing this.
Quoted 6 lines
This is not entirely without risk of regressions, but video_detect.c already prefers native-backlight interfaces over the acpi_video one on win8 ready machines, calling acpi_video_unregister_backlight() as soon as a native interface shows up. This is done because the acpi backlight interface often is broken on win8 ready machines, because win8 does not seem to actually use it.
This patch (in the form of commit 965736ee654d ("ACPI / video: Default lcd_only to true on Win8-ready and newer machines") in stable v4.15.17), breaks backlight control on my 2013 XPS13 laptop.
It normally uses the acpi backlight device, but after this patch that device no longer shows up in sysfs.
This isn't the first time the backlight has gotton broken on this system, though I think last time it was because the intel backlight driver got used instead of the ACPI one and that didn't work properly with it, so it needed a quirk to make it use ACPI instead.
Quoted 4 lines
Quoted 1 line
Is some other quirk needed around here too?
Yes looks like it (sigh), but I cannot find an existing quirk for your laptop.
I did at one point have to revert commit bc0bb9fd1c78 ("drm/i915: remove QUIRK_NO_PCH_PWM_ENABLE") for a few kernel releases until 4.3, presumably fixed by commit b029e66fa8e3 ("drm/i915: Backlight control using CRC PMIC based PWM driver").

Quoted 5 lines

Quoted 2 lines
When you say quirk do you mean you add something to the kernel commandline to work around this?
No, it was automatic, in the source. I don't know this code well enough to know how they got it working.

Weird, if my only_lcd changes impact you then acpi_osi_is_win8() returns true for you and you should get the intel backlight interface by default, but maybe the i915 driver somehow detects that that is not appropriate on your device ... ?

Before I can start working on a quirk to fix this I've some questions:

1) With recent kernels without the 965736ee654d commit, what is the output of "ls /sys/class/backlight" ?

2) What is the output of "ls /sys/class/backlight" after commit 965736ee654d ?

3) Can you start a kernel with commit 965736ee654d in there and "video.only_lcd=0" on the kernel commandline and verify that this fixes backlight control for you ?

Quoted 7 lines

Quoted 7 lines
Normally we would add
a dmi based quirk so that future versions automatically do the right thing. I was looking for the existing quirk so that I could re-use the dmi strings, but I don't see an existing quirk.
Can you run:

Quoted 7 lines

Quoted 5 lines
sudo dmidecode > dell-xps13-2013-dmi.log
And then send me a *personal* mail with that file attached. It is best to not send this to the list as it will contain you machines serial number and other unique information.
Will do.

I've received it thanks.

Regards,

Hans

James Hogan

2018-04-16 13:02 UTC #6 lore raw

Quoted 10 lines · Hans de Goede

Quoted 6 lines · James Hogan
Quoted 2 lines · Hans de Goede
When you say quirk do you mean you add something to the kernel commandline to work around this?
No, it was automatic, in the source. I don't know this code well enough to know how they got it working.
Weird, if my only_lcd changes impact you then acpi_osi_is_win8() returns true for you

It rings a bell that this laptop claims win8 support.

Quoted 9 lines

you should get the intel backlight interface
by default, but maybe the i915 driver somehow detects that that is not appropriate on your device ... ?
Before I can start working on a quirk to fix this I've some questions:
1) With recent kernels without the 965736ee654d commit, what is the output of "ls /sys/class/backlight" ?

$ ls -l /sys/class/backlight
total 0
lrwxrwxrwx 1 root root 0 Apr 16 13:57 acpi_video0 -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0

Quoted 2 lines

2) What is the output of "ls /sys/class/backlight" after commit 965736ee654d ?

no devices show up there.

Quoted 3 lines

3) Can you start a kernel with commit 965736ee654d in there and "video.only_lcd=0" on the kernel commandline and verify that this fixes backlight control for you ?

Confirmed, acpi_video0 shows up and backlight control works as expected.

Cheers
James

Hans de Goede

author

2018-04-17 15:20 UTC #7 lore raw

Hi James,

Quoted 31 lines · James Hogan

Quoted 10 lines · Hans de Goede
Quoted 6 lines · James Hogan
Quoted 2 lines · Hans de Goede
When you say quirk do you mean you add something to the kernel commandline to work around this?
No, it was automatic, in the source. I don't know this code well enough to know how they got it working.
Weird, if my only_lcd changes impact you then acpi_osi_is_win8() returns true for you
It rings a bell that this laptop claims win8 support.
Quoted 9 lines
you should get the intel backlight interface
by default, but maybe the i915 driver somehow detects that that is not appropriate on your device ... ?
Before I can start working on a quirk to fix this I've some questions:
1) With recent kernels without the 965736ee654d commit, what is the output of "ls /sys/class/backlight" ?
$ ls -l /sys/class/backlight
total 0
lrwxrwxrwx 1 root root 0 Apr 16 13:57 acpi_video0 -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0
Quoted 2 lines
2) What is the output of "ls /sys/class/backlight" after
    commit 965736ee654d ?

Quoted 7 lines

no devices show up there.
Quoted 3 lines
3) Can you start a kernel with commit 965736ee654d in there and "video.only_lcd=0" on the kernel commandline and verify that this fixes backlight control for you ?
Confirmed, acpi_video0 shows up and backlight control works as expected.

Ok, so rather then DMI quirk our way out of this I've come up with a more generic solution.

I've just send out a "[RFC] ACPI / video: Only default only_lcd to true on Win8-ready _desktops_" patch which should fix this.

Can you give this patch a quick spin (on top of the troublesome commit) and let me know if it fixes things (without needing the kernel cmdline option).

Regards,

Hans