Fix backlight issues on some Windows 8 systems

Drivers may need to make policy decisions based on the OS that the firmware believes it's interacting with. ACPI firmware will make a series of _OSI calls, starting from the oldest OS version they support and ending with the most recent. Add a function to return the last successful call so that drivers know what the firmware's expecting.

Based on a patch by Seth Forshee <[email protected]>

Signed-off-by Matthew Garrett <[email protected]>

Cc Seth Forshee <[email protected]>

---
 drivers/acpi/acpica/aclocal.h | 13 -------------
 drivers/acpi/acpica/utxface.c | 19 +++++++++++++++++++
 include/acpi/acpixf.h         | 22 ++++++++++++++++++++++
 3 files changed, 41 insertions(+), 13 deletions(-)

drivers/acpi/acpica/aclocal.h +0 -13

index d5bfbd3..8a2f532 100644
@@ -948,19 +948,6 @@ struct acpi_bit_register_info {
 
 /* Structs and definitions for _OSI support and I/O port validation */
 
-#define ACPI_OSI_WIN_2000               0x01
-#define ACPI_OSI_WIN_XP                 0x02
-#define ACPI_OSI_WIN_XP_SP1             0x03
-#define ACPI_OSI_WINSRV_2003            0x04
-#define ACPI_OSI_WIN_XP_SP2             0x05
-#define ACPI_OSI_WINSRV_2003_SP1        0x06
-#define ACPI_OSI_WIN_VISTA              0x07
-#define ACPI_OSI_WINSRV_2008            0x08
-#define ACPI_OSI_WIN_VISTA_SP1          0x09
-#define ACPI_OSI_WIN_VISTA_SP2          0x0A
-#define ACPI_OSI_WIN_7                  0x0B
-#define ACPI_OSI_WIN_8                  0x0C
-
 #define ACPI_ALWAYS_ILLEGAL             0x00
 
 struct acpi_interface_info {

drivers/acpi/acpica/utxface.c +19 -0

index 6505774..c1638c3 100644
@@ -389,6 +389,25 @@ ACPI_EXPORT_SYMBOL(acpi_install_interface_handler)
 
 /*****************************************************************************
  *
+ * FUNCTION:    acpi_osi_version
+ *
+ * PARAMETERS:  None
+ *
+ * RETURN:      Last OS version requested via _OSI
+ *
+ * DESCRIPTION: Returns the argument to the most recent _OSI query performed
+ *		by the firmware
+ *
+ ****************************************************************************/
+u8 acpi_osi_version(void)
+{
+	return acpi_gbl_osi_data;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_osi_version)
+
+/*****************************************************************************
+ *
  * FUNCTION:    acpi_check_address_range
  *
  * PARAMETERS:  space_id            - Address space ID

include/acpi/acpixf.h +22 -0

index 454881e..41d3ac1 100644
@@ -82,6 +82,22 @@ extern u8 acpi_gbl_truncate_io_addresses;
 extern u8 acpi_gbl_disable_auto_repair;
 
 /*
+ * Values returned by acpi_osi_version()
+ */
+#define ACPI_OSI_WIN_2000               0x01
+#define ACPI_OSI_WIN_XP                 0x02
+#define ACPI_OSI_WIN_XP_SP1             0x03
+#define ACPI_OSI_WINSRV_2003            0x04
+#define ACPI_OSI_WIN_XP_SP2             0x05
+#define ACPI_OSI_WINSRV_2003_SP1        0x06
+#define ACPI_OSI_WIN_VISTA              0x07
+#define ACPI_OSI_WINSRV_2008            0x08
+#define ACPI_OSI_WIN_VISTA_SP1          0x09
+#define ACPI_OSI_WIN_VISTA_SP2          0x0A
+#define ACPI_OSI_WIN_7                  0x0B
+#define ACPI_OSI_WIN_8                  0x0C
+
+/*
  * Hardware-reduced prototypes. All interfaces that use these macros will
  * be configured out of the ACPICA build if the ACPI_REDUCED_HARDWARE flag
  * is set to TRUE.

@@ -307,6 +323,12 @@ acpi_status acpi_install_notify_handler(acpi_handle device, u32 handler_type,
 					acpi_notify_handler handler,
 					void *context);
 
+#ifdef CONFIG_ACPI
+u8 acpi_osi_version(void);
+#else
+static inline u8 acpi_osi_version(void) { return 0; }
+#endif
+
 acpi_status
 acpi_remove_notify_handler(acpi_handle device,
 			   u32 handler_type, acpi_notify_handler handler);

Hi Matthew,

Quoted 7 lines · Matthew Garrett

Drivers may need to make policy decisions based on the OS that the firmware believes it's interacting with. ACPI firmware will make a series of _OSI calls, starting from the oldest OS version they support and ending with the most recent. Add a function to return the last successful call so that drivers know what the firmware's expecting.

Based on a patch by Seth Forshee <[email protected]>

Bob (CCed) would prefer us to access acpi_gbl_osi_data directly instead of adding the wrapper to ACPICA. He also thinks that the symbol definitions should go into include/acpi/actypes.h rather than into acpixf.h.

Then, the only ACPICA change would be to move the symbols and we can add a Linux-specific patch on top of that adding the acpi_gbl_osi_data wrapper.

Quoted 15 lines · Rafael J. Wysocki

Hi Matthew,

Quoted 7 lines · Matthew Garrett

Drivers may need to make policy decisions based on the OS that the firmware believes it's interacting with. ACPI firmware will make a series of _OSI calls, starting from the oldest OS version they support and ending with the most recent. Add a function to return the last successful call so that drivers know what the firmware's expecting.

Based on a patch by Seth Forshee <[email protected]>

Bob (CCed) would prefer us to access acpi_gbl_osi_data directly instead of adding the wrapper to ACPICA. He also thinks that the symbol definitions should go into include/acpi/actypes.h rather than into acpixf.h.

Quoted 3 lines

Then, the only ACPICA change would be to move the symbols and we can add a Linux-specific patch on top of that adding the acpi_gbl_osi_data wrapper.

That sounds good to me. Do you want to respin that, or should I send an updated set?

Quoted 16 lines · Matthew Garrett

Quoted 15 lines · Rafael J. Wysocki

Hi Matthew,

Quoted 7 lines · Matthew Garrett

Drivers may need to make policy decisions based on the OS that the firmware believes it's interacting with. ACPI firmware will make a series of _OSI calls, starting from the oldest OS version they support and ending with the most recent. Add a function to return the last successful call so that drivers know what the firmware's expecting.

Based on a patch by Seth Forshee <[email protected]>

Bob (CCed) would prefer us to access acpi_gbl_osi_data directly instead of adding the wrapper to ACPICA. He also thinks that the symbol definitions should go into include/acpi/actypes.h rather than into acpixf.h.

Quoted 6 lines

Quoted 3 lines

Then, the only ACPICA change would be to move the symbols and we can add a Linux-specific patch on top of that adding the acpi_gbl_osi_data wrapper.

That sounds good to me. Do you want to respin that, or should I send an updated set?

That one should be fine, no need to respin [1/3] and [3/3].

Drivers may need to make policy decisions based on the OS that the firmware believes it's interacting with. ACPI firmware will make a series of _OSI calls, starting from the oldest OS version they support and ending with the most recent. This patchset adds a function in ACPI OSL layer to return the last successful call so that drivers know what the firmware's expecting.

Based on a patch by Matthew Garrett <[email protected]>, which is again based on a patch by Seth Forshee <[email protected]>.

Changelog-by Matthew Garrett <[email protected]>

Signed-off-by Aaron Lu <[email protected]>

Cc Matthew Garrett <[email protected]>

Cc Seth Forshee <[email protected]>

---
 drivers/acpi/osl.c   | 6 ++++++
 include/linux/acpi.h | 6 ++++++
 2 files changed, 12 insertions(+)

drivers/acpi/osl.c +0 -0

index 6ab2c35..7ebf07d 100644

drivers/acpi/osl.c +1 -0

+EXPORT_SYMBOL_GPL(acpi_osi_version);

include/linux/acpi.h +6 -0

index 353ba25..fc3b5c2 100644
@@ -578,4 +578,10 @@ acpi_handle_printk(const char *level, void *handle, const char *fmt, ...) {}
 })
 #endif
 
+#ifdef CONFIG_ACPI
+u8 acpi_osi_version(void);
+#else
+static inline u8 acpi_osi_version(void) { return 0; }
+#endif
+
 #endif	/*_LINUX_ACPI_H*/

Quoted 18 lines · Aaron Lu

Drivers may need to make policy decisions based on the OS that the firmware believes it's interacting with. ACPI firmware will make a series of _OSI calls, starting from the oldest OS version they support and ending with the most recent. This patchset adds a function in ACPI OSL layer to return the last successful call so that drivers know what the firmware's expecting.

Based on a patch by Matthew Garrett <[email protected]>, which is again based on a patch by Seth Forshee <[email protected]>.

Changelog-by Matthew Garrett <[email protected]>

Signed-off-by Aaron Lu <[email protected]>

Cc Matthew Garrett <[email protected]>

Cc Seth Forshee <[email protected]>

---
 drivers/acpi/osl.c   | 6 ++++++
 include/linux/acpi.h | 6 ++++++
 2 files changed, 12 insertions(+)

drivers/acpi/osl.c void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context, · line 1806

Quoted 13 lines

drivers/acpi/osl.c +5 -0

index 6ab2c35..7ebf07d 100644
@@ -1799,3 +1799,9 @@ void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
 		kfree(hp_work);
 }
 EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);
+
+u8 acpi_osi_version(void)
+{
+	return acpi_gbl_osi_data;
+}

Actually, is there a reason not to make this static inline and put it into the header?

Quoted 19 lines · Rafael J. Wysocki

Quoted 18 lines · Aaron Lu

Drivers may need to make policy decisions based on the OS that the firmware believes it's interacting with. ACPI firmware will make a series of _OSI calls, starting from the oldest OS version they support and ending with the most recent. This patchset adds a function in ACPI OSL layer to return the last successful call so that drivers know what the firmware's expecting.

Based on a patch by Matthew Garrett <[email protected]>, which is again based on a patch by Seth Forshee <[email protected]>.

Changelog-by Matthew Garrett <[email protected]>

Signed-off-by Aaron Lu <[email protected]>

Cc Matthew Garrett <[email protected]>

Cc Seth Forshee <[email protected]>

---
 drivers/acpi/osl.c   | 6 ++++++
 include/linux/acpi.h | 6 ++++++
 2 files changed, 12 insertions(+)

Quoted 16 lines

drivers/acpi/osl.c void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context, · line 1806

Quoted 13 lines

drivers/acpi/osl.c +5 -0

index 6ab2c35..7ebf07d 100644
@@ -1799,3 +1799,9 @@ void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
 		kfree(hp_work);
 }
 EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);
+
+u8 acpi_osi_version(void)
+{
+	return acpi_gbl_osi_data;
+}

Actually, is there a reason not to make this static inline and put it into the header?

Ah right, thanks for the suggestion.

Patch updated:

From: Aaron Lu <[email protected]>
Subject: [PATCH updated 2/2] ACPI / OSL: add a wrapper function to return OSI version

Drivers may need to make policy decisions based on the OS that the firmware believes it's interacting with. ACPI firmware will make a series of _OSI calls, starting from the oldest OS version they support and ending with the most recent. This patchset adds a function in ACPI OSL layer to return the last successful call so that drivers know what the firmware's expecting.

Based on a patch by Matthew Garrett <[email protected]>, which is again based on a patch by Seth Forshee <[email protected]>.

Signed-off-by Aaron Lu <[email protected]>

---
 include/linux/acpi.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/acpi.h +6 -0

index 353ba25..7969bf8 100644
@@ -578,4 +578,10 @@ acpi_handle_printk(const char *level, void *handle, const char *fmt, ...) {}
 })
 #endif
 
+#ifdef CONFIG_ACPI
+static inline u8 acpi_osi_version(void) { return acpi_gbl_osi_data; }
+#else
+static inline u8 acpi_osi_version(void) { return 0; }
+#endif
+
 #endif	/*_LINUX_ACPI_H*/

Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems.

Signed-off-by Matthew Garrett <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +3 -0

index 3b315ba..23b6292 100644
@@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
 		acpi_video_register();
+		/* Don't use ACPI backlight functions on Windows 8 platforms */
+		if (acpi_osi_version() >= ACPI_OSI_WIN_8)
+			acpi_video_backlight_unregister();
 	}
 
 	if (IS_GEN5(dev))

Signature · 1 line

1.8.2.1

Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems.

Signed-off-by Matthew Garrett <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +0 -0

index 3b315ba..23b6292 100644

Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems.

Signed-off-by Matthew Garrett <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +3 -1

index 3b315ba..23b6292 100644
@@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
                /* Must be done after probing outputs */
                intel_opregion_init(dev);
                acpi_video_register();
+               /* Don't use ACPI backlight functions on Windows 8 platforms */
+               if (acpi_osi_version() >= ACPI_OSI_WIN_8)
+                       acpi_video_backlight_unregister();
        }
 
        if (IS_GEN5(dev))
--

1.8.2.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems.

Signed-off-by Matthew Garrett <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +0 -0

index 3b315ba..23b6292 100644

What about a priority based solution? We can introduce a new field named priority to backlight_device and instead of calling another module's function like the unregister one here(which cause unnecessary module dependency), we only need to boost priority for its own interface. This field will be exported to sysfs, so user can change it during runtime too. And we can also introduce a new kernel command line as backlight.force_interface=raw/firmware/platform, to overcome the limited functionality provided by acpi_backlight=video/vendor, which does not involve GPU's interface.

And we can place the quirk code in backlight layer instead of individual backlight functionality provider module. Suppose we have a backlight manager there, for all win8 systems, we can boost the raw type's priority on its registration, so no need to add code in intel/amd/etc./'s GPU driver code.

With priority based solution, all backlight control interfaces stay, the priority field is an indication given by kernel to user space.

Quoted 9 lines · Aaron Lu

What about a priority based solution? We can introduce a new field named priority to backlight_device and instead of calling another module's function like the unregister one here(which cause unnecessary module dependency), we only need to boost priority for its own interface. This field will be exported to sysfs, so user can change it during runtime too. And we can also introduce a new kernel command line as backlight.force_interface=raw/firmware/platform, to overcome the limited functionality provided by acpi_backlight=video/vendor, which does not involve GPU's interface.

How would that work with existing userspace?

Quoted 6 lines

And we can place the quirk code in backlight layer instead of individual backlight functionality provider module. Suppose we have a backlight manager there, for all win8 systems, we can boost the raw type's priority on its registration, so no need to add code in intel/amd/etc./'s GPU driver code.

But we'd need to add code to every piece of userspace that currently uses the backlight, right?

Quoted 3 lines

With priority based solution, all backlight control interfaces stay, the priority field is an indication given by kernel to user space.

We shouldn't export interfaces if we don't expect them to work.

Quoted 1 line · Matthew Garrett

How would that work with existing userspace?

User space tool will need to be updated to use this as stated in the gist page, I've patches for gsd-backlight-helper and xorg-x11-drv-intel, for others we can add I think if the priority based solution is deemed useful.

Quoted 1 line

We shouldn't export interfaces if we don't expect them to work.

It's not easy to decide if they work or not sometimes, e.g. I came across a system that claims win8 in ACPI table and has an Intel GPU, while its ACPI video interface also works. With this patch, the working ACPI video interface is removed, while with the priority based solution, the GPU's interface priority gets higher, but the ACPI video interface still stays.

Quoted 6 lines · Aaron Lu

It's not easy to decide if they work or not sometimes, e.g. I came across a system that claims win8 in ACPI table and has an Intel GPU, while its ACPI video interface also works. With this patch, the working ACPI video interface is removed, while with the priority based solution, the GPU's interface priority gets higher, but the ACPI video interface still stays.

Well, Windows 8 will only use the ACPI backlight interface if the GPU driver decides to, right? So the logic for deciding whether to remove the ACPI backlight control or not should be left up to the GPU. There's

no harm in refusing to expose a working method if there's another working method, but there is harm in exposing a broken one and expecting userspace to know the difference.

Quoted 3 lines · Matthew Garrett

Well, Windows 8 will only use the ACPI backlight interface if the GPU driver decides to, right? So the logic for deciding whether to remove the ACPI backlight control or not should be left up to the GPU. There's

I don't know this. From the document I googled, Microsoft suggests under win8, backlight should be controlled by the graphics driver for smooth brightness level change, instead of ACPI or other methods. So it is possible that OEM will not test the ACPI interface well and thus the interface is likely broken. I don't see why GPU driver has any better knowledge on which systems the firmware interface is broken or not.

Quoted 3 lines

no harm in refusing to expose a working method if there's another working method, but there is harm in exposing a broken one and expecting userspace to know the difference.

BTW, the proposed solution is not meant to solve win8 problems alone, it should make solving other problems easy and make individual backlight control interface provider module independent with each other such as the platform drivers will not need to check if ACPI video driver will control backlight or not and can always create backlight interface(its default priority is lower that ACPI video driver's so will not be taken by user space by default, showing the same behavior of the current code).

The current acpi_backlight=video/vendor kernel command line is pretty misleading, for laptops that do not have vendor backlight interface, adding acpi_backlight=vendor actually makes the system using the GPU's interface. Some laptops are using this switch to work around problems in ACPI video driver and users think they are using vendor interface. That's why I think we need a new command line as the backlight.force_interface=raw/firmware/platform.

Instead of letting individual driver to make decisions on which backlight interface this system should use(either in platform driver as we currently did, see acer-wmi and asus-wmi, or GPU driver as this case), I think we need a better and clear way to handle such things. For example, suppose an acer laptop: vendor does not support backlight, ACPI video's backlight interface is broken and GPU's works, to make it work, user will need to select acer-wmi module while this module does not have anything to do with the functionality, but simply because it serves as the backlight manager for acer laptops.

Quoted 11 lines · Aaron Lu

Quoted 3 lines · Matthew Garrett

Well, Windows 8 will only use the ACPI backlight interface if the GPU driver decides to, right? So the logic for deciding whether to remove the ACPI backlight control or not should be left up to the GPU. There's

I don't know this. From the document I googled, Microsoft suggests under win8, backlight should be controlled by the graphics driver for smooth brightness level change, instead of ACPI or other methods. So it is possible that OEM will not test the ACPI interface well and thus the interface is likely broken. I don't see why GPU driver has any better knowledge on which systems the firmware interface is broken or not.

The vendor will presumably have tested that backlight control works - if the GPU driver uses the ACPI interface and backlight control is broken, then the vendor would fix it.

Quoted 5 lines

Quoted 4 lines

no harm in refusing to expose a working method if there's another working method, but there is harm in exposing a broken one and expecting userspace to know the difference.

Quoted 7 lines

BTW, the proposed solution is not meant to solve win8 problems alone, it should make solving other problems easy and make individual backlight control interface provider module independent with each other such as the platform drivers will not need to check if ACPI video driver will control backlight or not and can always create backlight interface(its default priority is lower that ACPI video driver's so will not be taken by user space by default, showing the same behavior of the current code).

Sure, but it still requires the replacement of existing userspace. We need to fix the problem with existing userspace, too.

Quoted 8 lines

The current acpi_backlight=video/vendor kernel command line is pretty misleading, for laptops that do not have vendor backlight interface, adding acpi_backlight=vendor actually makes the system using the GPU's interface. Some laptops are using this switch to work around problems in ACPI video driver and users think they are using vendor interface. That's why I think we need a new command line as the backlight.force_interface=raw/firmware/platform.

No, I think we need to fix the bugs that currently require users to pass options.

Quoted 10 lines

Instead of letting individual driver to make decisions on which backlight interface this system should use(either in platform driver as we currently did, see acer-wmi and asus-wmi, or GPU driver as this case), I think we need a better and clear way to handle such things. For example, suppose an acer laptop: vendor does not support backlight, ACPI video's backlight interface is broken and GPU's works, to make it work, user will need to select acer-wmi module while this module does not have anything to do with the functionality, but simply because it serves as the backlight manager for acer laptops.

How do these machines work under Windows? Until we know the answer to that, we don't know what the correct way to handle the problem is under Linux.

Quoted 3 lines · Matthew Garrett

The vendor will presumably have tested that backlight control works - if the GPU driver uses the ACPI interface and backlight control is broken, then the vendor would fix it.

I don't think GPU driver uses ACPI interface, that's why for some systems, ACPI interface is broken while the GPU's is not.

Quoted 2 lines

Sure, but it still requires the replacement of existing userspace. We need to fix the problem with existing userspace, too.

Yes. The problem is two way. First, some of the backlight interface privoder module provides a broken interface; Two, the userspace picked a broken interface while there are usable ones. For example, in the win8 thinkpad case, the ACPI video driver provides broken interface and the GPU driver provides usable interface, but userspace choose ACPI video's interface. To make things complicated, if we make the broken interface disappear in ACPI video module, then the platform driver thinkpad_acpi will start to provide another broken interface. I have to say, solve it in the GPU code is a clever way, it's just a little weird to me for a broken interface to be blacklisted, we have to do it in another module, not the broken module itself.

Quoted 2 lines

No, I think we need to fix the bugs that currently require users to pass options.

For ACPI video driver, since it relies on firmware to do the right thing, if the functionality is broken, then it is, there is hardly anything we can do...(not always, we can quirk in some cases, but there are cases we can do nothing). In this case, we can:
1 Ask user to add acpi_backlight=vendor, so that ACPI video driver will
not create backlight interface and userspace will not pick it;
2 Add that model in DMI table in video_detect.c, eliminating the need for
that command line;
3 Add that model in DMI table in another module, let that module
unregister backlight interface provided by ACPI video as is done in acer-wmi, asus-wmi, and i915 in this case;
3 Use the backlight section in xorg.conf to make X uses another
backlight interface. This may not work for distros that use gsd-backlight-helper, which always prefers firmware.

Which one do you prefer?

Quoted 3 lines

How do these machines work under Windows? Until we know the answer to that, we don't know what the correct way to handle the problem is under Linux.

Do you mean we need to understand Windows behavior so that we can decide where to place the code to do the backlight management thing? So for win8 case, we know MS will use GPU driver to do backlight control, all this means to me is, ACPI video's and platform driver's interface are more likely broken on those systems, but it doesn't qualify why Linux' GPU driver should do the decision, it's not that the GPU driver can poke some GPU register and then decide oh, this model does not have working ACPI backlight interface. As this patch shows, we make the decision by OSI, which is not specific to GPU driver.

BTW, I don't think any module knows something better than another module, all quirk logic and DMI entry we currently have in Linux are got by user's feedback(bug reports), it doesn't seem to me some module is natrually the one that should make this decision. Or do I miss something?

firmware > raw seems to be good enough. Note that the explicit list in

Aside at the end: If the gnome tool indeed has its own backlight code and doesn't just use that as a fallback if the xrandr backligh property isn't available, then that's just a serious bug in gnome and should be fixed asap. But imo that's not something we should try to (nor do I see any way how to) work around in the kernel.

Hi all,

So to me it looks like the discussion is going in circles a bit, hence let me drop my maintainer-opinion here:

1. Matthew's patch series here looks reasonable, and if it fixes a bunch of systems (which it seems to) it has my Ack and imo should go in. If acpi maintainers can smash their Ack onto the acpi parts I'd very much like to merge this into drm-intel-next, that should give us the most coverage for intel systems.

Len, Rafael, are you ok with the acpi part of this and merging it through drm-intel-next?

2. Imo the current amount of quirking we expose to users (we have kernel options to disable the acpi interface, blacklist platform modules, all backlights can be tested through sysfs and on top of that xf86-video-intel has an xorg.conf to select the backlight used by the driver). I haven't spotted a compelling reason in this thread to add another one, what we have seems to be good enough to debug backligh issues.

3. Also, adding yet another backlight quirk mechanism isn't gonna magically fix broken systems.

We _really_ should strive to make this just work and not offer the angry user another roll of duct-tape for free.

4. The currently established priority selection for backlights of platform

Quoted 1 line

firmware > raw seems to be good enough. Note that the explicit list in

xf86-vidoe-intel is only used as a fallback for really old kernels which do not expose this information. We could probably rip it out.

5. We've recently looked at opregion again and couldn't spot a hint. Unfortnately it looks like both noodling better information out of Intel and trying to publish an updated opregion spec seem to be losing games :( We'll keep on trying though.

Aside at the end: If the gnome tool indeed has its own backlight code and doesn't just use that as a fallback if the xrandr backligh property isn't available, then that's just a serious bug in gnome and should be fixed asap. But imo that's not something we should try to (nor do I see any way how to) work around in the kernel.

Quoted 11 lines · Daniel Vetter

Hi all,

So to me it looks like the discussion is going in circles a bit, hence let me drop my maintainer-opinion here:

1. Matthew's patch series here looks reasonable, and if it fixes a bunch of systems (which it seems to) it has my Ack and imo should go in. If acpi maintainers can smash their Ack onto the acpi parts I'd very much like to merge this into drm-intel-next, that should give us the most coverage for intel systems.

Quoted 2 lines

Len, Rafael, are you ok with the acpi part of this and merging it through drm-intel-next?

It has to go through the ACPI tree because of the ACPICA patch that needs to be synchronized with the ACPICA upstream. Sorry.

That said, I'm going to take this patchset.

Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems.

Signed-off-by Matthew Garrett <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +0 -0

index 3b315ba..23b6292 100644

Quoted 25 lines · Matthew Garrett

Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems.

Signed-off-by Matthew Garrett <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +3 -0

index 3b315ba..23b6292 100644
@@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
 		acpi_video_register();
+		/* Don't use ACPI backlight functions on Windows 8 platforms */
+		if (acpi_osi_version() >= ACPI_OSI_WIN_8)
+			acpi_video_backlight_unregister();
 	}
 

drivers/gpu/drm/i915/i915_dma.c int i915_driver_load(struct drm_device *dev, unsigned long flags) · line 1669

Quoted 2 lines

drivers/gpu/drm/i915/i915_dma.c +0 -0

 	if (IS_GEN5(dev))

Well, this causes build failures to happen when the ACPI video driver is modular and the graphics driver is not.

I'm not sure how to resolve that, so suggestions are welcome.

Quoted 26 lines · Rafael J. Wysocki

Quoted 25 lines · Matthew Garrett

Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems.

Signed-off-by Matthew Garrett <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +3 -0

index 3b315ba..23b6292 100644
@@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
 		acpi_video_register();
+		/* Don't use ACPI backlight functions on Windows 8 platforms */
+		if (acpi_osi_version() >= ACPI_OSI_WIN_8)
+			acpi_video_backlight_unregister();
 	}
 

Quoted 7 lines

drivers/gpu/drm/i915/i915_dma.c int i915_driver_load(struct drm_device *dev, unsigned long flags) · line 1669

Quoted 2 lines

drivers/gpu/drm/i915/i915_dma.c +0 -0

 	if (IS_GEN5(dev))

Well, this causes build failures to happen when the ACPI video driver is modular and the graphics driver is not.

I'm not sure how to resolve that, so suggestions are welcome.

Actually, that happened with the radeon patch.

That said, ACPI_OSI_WIN_8 doesn't make much sense for !CONFIG_ACPI, for example.

What about making acpi_video_register() do the quirk instead? We could add an argument to it indicating whether or not quirks should be applied.

Quoted 37 lines · Rafael J. Wysocki

Quoted 26 lines · Rafael J. Wysocki

Quoted 25 lines · Matthew Garrett

Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems.

Signed-off-by Matthew Garrett <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +3 -0

index 3b315ba..23b6292 100644
@@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
 		acpi_video_register();
+		/* Don't use ACPI backlight functions on Windows 8 platforms */
+		if (acpi_osi_version() >= ACPI_OSI_WIN_8)
+			acpi_video_backlight_unregister();
 	}
 

Quoted 7 lines

drivers/gpu/drm/i915/i915_dma.c int i915_driver_load(struct drm_device *dev, unsigned long flags) · line 1669

Quoted 2 lines

drivers/gpu/drm/i915/i915_dma.c +0 -0

 	if (IS_GEN5(dev))

Well, this causes build failures to happen when the ACPI video driver is modular and the graphics driver is not.

I'm not sure how to resolve that, so suggestions are welcome.

Actually, that happened with the radeon patch.

Quoted 5 lines

That said, ACPI_OSI_WIN_8 doesn't make much sense for !CONFIG_ACPI, for example.

What about making acpi_video_register() do the quirk instead? We could add an argument to it indicating whether or not quirks should be applied.

Actually, I wonder what about the appended patch (on top of the Aaron's
https://patchwork.kernel.org/patch/2812951/) instead of [1-3/3] from this series.

Quoted 38 lines · Rafael J. Wysocki

Quoted 37 lines · Rafael J. Wysocki

Quoted 26 lines · Rafael J. Wysocki

Quoted 25 lines · Matthew Garrett

Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems.

Signed-off-by Matthew Garrett <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +3 -0

index 3b315ba..23b6292 100644
@@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
 		acpi_video_register();
+		/* Don't use ACPI backlight functions on Windows 8 platforms */
+		if (acpi_osi_version() >= ACPI_OSI_WIN_8)
+			acpi_video_backlight_unregister();
 	}
 

Quoted 7 lines

drivers/gpu/drm/i915/i915_dma.c int i915_driver_load(struct drm_device *dev, unsigned long flags) · line 1669

Quoted 2 lines

drivers/gpu/drm/i915/i915_dma.c +0 -0

 	if (IS_GEN5(dev))

Well, this causes build failures to happen when the ACPI video driver is modular and the graphics driver is not.

I'm not sure how to resolve that, so suggestions are welcome.

Actually, that happened with the radeon patch.

Quoted 8 lines

Quoted 5 lines

That said, ACPI_OSI_WIN_8 doesn't make much sense for !CONFIG_ACPI, for example.

What about making acpi_video_register() do the quirk instead? We could add an argument to it indicating whether or not quirks should be applied.

Actually, I wonder what about the appended patch (on top of the Aaron's
https://patchwork.kernel.org/patch/2812951/) instead of [1-3/3] from this series.

Or even something as simple as this one.

---
 drivers/acpi/video_detect.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-pm/drivers/acpi/video_detect.c
===================================================================

drivers/acpi/video_detect.c +0 -1

 	} else {
 		status = acpi_bus_get_device(graphics_handle, &tmp_dev);
 		if (ACPI_FAILURE(status)) {
 
--

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 59 lines · Rafael J. Wysocki

Quoted 38 lines · Rafael J. Wysocki

Quoted 37 lines · Rafael J. Wysocki

Quoted 26 lines · Rafael J. Wysocki

Quoted 25 lines · Matthew Garrett

Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems.

Signed-off-by Matthew Garrett <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/i915/i915_dma.c +3 -0

index 3b315ba..23b6292 100644
@@ -1661,6 +1661,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
 		acpi_video_register();
+		/* Don't use ACPI backlight functions on Windows 8 platforms */
+		if (acpi_osi_version() >= ACPI_OSI_WIN_8)
+			acpi_video_backlight_unregister();
 	}
 

Quoted 7 lines

drivers/gpu/drm/i915/i915_dma.c int i915_driver_load(struct drm_device *dev, unsigned long flags) · line 1669

Quoted 2 lines

drivers/gpu/drm/i915/i915_dma.c +0 -0

 	if (IS_GEN5(dev))

Well, this causes build failures to happen when the ACPI video driver is modular and the graphics driver is not.

I'm not sure how to resolve that, so suggestions are welcome.

Actually, that happened with the radeon patch.

Quoted 8 lines

Quoted 5 lines

That said, ACPI_OSI_WIN_8 doesn't make much sense for !CONFIG_ACPI, for example.

What about making acpi_video_register() do the quirk instead? We could add an argument to it indicating whether or not quirks should be applied.

Actually, I wonder what about the appended patch (on top of the Aaron's
https://patchwork.kernel.org/patch/2812951/) instead of [1-3/3] from this series.

Or even something as simple as this one.

---
 drivers/acpi/video_detect.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-pm/drivers/acpi/video_detect.c
===================================================================

drivers/acpi/video_detect.c long acpi_video_get_capabilities(acpi_ha · line 208

Quoted 6 lines

drivers/acpi/video_detect.c +3 -0

@@ -203,6 +203,9 @@ long acpi_video_get_capabilities(acpi_ha
 		 */
 
 		dmi_check_system(video_detect_dmi_table);
+
+		if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8)
+			acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;

Then vendor driver(thinkpad_acpi) will step in and create a backlight interface for the system, which, unfortunately, is also broken for those win8 thinkpads.

So we will need to do something in thinkpad_acpi to also not create backlight interface for these systems too.

This actually doesn't feel bad to me, since the modules are blacklisting their own interfaces. The downside is of course, two quirk code exist.

BTW, unregistering ACPI video's backlight interface in GPU driver doesn't have this problem since it made the platform driver think the ACPI video driver will control the backlight and then use the newly added API to remove ACPI video created backlight interface.

Quoted 5 lines · Aaron Lu

Quoted 4 lines · Rafael J. Wysocki

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Rafael J. Wysocki

On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Actually, I wonder what about

Quoted 26 lines

Quoted 12 lines

Or even something as simple as this one.

---
 drivers/acpi/video_detect.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-pm/drivers/acpi/video_detect.c
===================================================================

drivers/acpi/video_detect.c long acpi_video_get_capabilities(acpi_ha · line 208

Quoted 6 lines

drivers/acpi/video_detect.c +3 -0

@@ -203,6 +203,9 @@ long acpi_video_get_capabilities(acpi_ha
 		 */
 
 		dmi_check_system(video_detect_dmi_table);
+
+		if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8)
+			acpi_video_support |= ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;

Then vendor driver(thinkpad_acpi) will step in and create a backlight interface for the system, which, unfortunately, is also broken for those win8 thinkpads.

So we will need to do something in thinkpad_acpi to also not create backlight interface for these systems too.

Quoted 7 lines

This actually doesn't feel bad to me, since the modules are blacklisting their own interfaces. The downside is of course, two quirk code exist.

BTW, unregistering ACPI video's backlight interface in GPU driver doesn't have this problem since it made the platform driver think the ACPI video driver will control the backlight and then use the newly added API to remove ACPI video created backlight interface.

Yes, I know this.

I think I also know how to introduce that change in a slightly cleaner way.

I'll post a patch for comments later today or tomorrow.

OK, the patch is appended. Please have a look and tell me what you think.

Thanks,
Rafael

---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

According to Matthew Garrett, "Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows [8] doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems".

There's a problem with that approach, however, because simply avoiding to register the ACPI backlight interface if the firmware calls _OSI for Windows 8 may not work in the following situations:
(1) The ACPI backlight interface actually works on the given system and the i915 driver is not loaded (e.g. another graphics driver is used).
(2) The ACPI backlight interface doesn't work on the given system, but there is a vendor platform driver that will register its own, equally broken, backlight interface if the ACPI one is not registered in advance.

Therefore we need to keep the ACPI backlight interface registered until the i915 driver is loaded which then will unregister it if the firmware has called _OSI for Windows 8.

For this reason, introduce an alternative function for registering ACPI video, acpi_video_register_with_quirks(), that will check whether or not the ACPI video driver has already been registered and whether or not the backlight Windows 8 quirk has to be applied. If the quirk has to be applied, it will block the ACPI backlight support and either unregister the backlight interface if the ACPI video driver has already been registered, or register the ACPI video driver without the backlight interface otherwise. Make the i915 driver use acpi_video_register_with_quirks() instead of acpi_video_register() in i915_driver_load().

This change is based on earlier patches from Matthew Garrett, Chun-Yi Lee and Seth Forshee.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/video.c            |   61 ++++++++++++++++++++++++++++++++++++----
 drivers/acpi/video_detect.c     |   11 +++++++
 drivers/gpu/drm/i915/i915_dma.c |    2 -
 include/acpi/video.h            |   11 ++++++-
 include/linux/acpi.h            |    6 +++
 5 files changed, 84 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/acpi/video.c
===================================================================

drivers/acpi/video.c +56 -5

@@ -1854,6 +1854,46 @@ static int acpi_video_bus_remove(struct
 	return 0;
 }
 
+static acpi_status video_unregister_backlight(acpi_handle handle, u32 lvl,
+					      void *context, void **rv)
+{
+	struct acpi_device *acpi_dev;
+	struct acpi_video_bus *video;
+	struct acpi_video_device *dev, *next;
+
+	if (acpi_bus_get_device(handle, &acpi_dev))
+		return AE_OK;
+
+	if (acpi_match_device_ids(acpi_dev, video_device_ids))
+		return AE_OK;
+
+	video = acpi_driver_data(acpi_dev);
+	if (!video)
+		return AE_OK;
+
+	acpi_video_bus_stop_devices(video);
+	mutex_lock(&video->device_list_lock);
+	list_for_each_entry_safe(dev, next, &video->video_device_list, entry) {
+		if (dev->backlight) {
+			backlight_device_unregister(dev->backlight);
+			dev->backlight = NULL;
+			kfree(dev->brightness->levels);
+			kfree(dev->brightness);
+		}
+		if (dev->cooling_dev) {
+			sysfs_remove_link(&dev->dev->dev.kobj,
+					  "thermal_cooling");
+			sysfs_remove_link(&dev->cooling_dev->device.kobj,
+					  "device");
+			thermal_cooling_device_unregister(dev->cooling_dev);
+			dev->cooling_dev = NULL;
+		}
+	}
+	mutex_unlock(&video->device_list_lock);
+	acpi_video_bus_start_devices(video);
+	return AE_OK;
+}
+
 static int __init is_i740(struct pci_dev *dev)
 {
 	if (dev->device == 0x00D1)

@@ -1885,14 +1925,25 @@ static int __init intel_opregion_present
 	return opregion;
 }
 
-int acpi_video_register(void)
+int __acpi_video_register(bool backlight_quirks)
 {
-	int result = 0;
+	bool no_backlight;
+	int result;
+
+	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
+
 	if (register_count) {
 		/*
-		 * if the function of acpi_video_register is already called,
-		 * don't register the acpi_vide_bus again and return no error.
+		 * If acpi_video_register() has been called already, don't try
+		 * to register acpi_video_bus, but unregister backlight devices
+		 * if no backlight support is requested.
 		 */
+		if (no_backlight)
+			acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+					    ACPI_UINT32_MAX,
+					    video_unregister_backlight,
+					    NULL, NULL, NULL);
+
 		return 0;
 	}
 

@@ -1908,7 +1959,7 @@ int acpi_video_register(void)
 
 	return 0;
 }
-EXPORT_SYMBOL(acpi_video_register);
+EXPORT_SYMBOL(__acpi_video_register);
 
 void acpi_video_unregister(void)
 {

Index: linux-pm/drivers/gpu/drm/i915/i915_dma.c
===================================================================

drivers/gpu/drm/i915/i915_dma.c +1 -1

@@ -1660,7 +1660,7 @@ int i915_driver_load(struct drm_device *
 	if (INTEL_INFO(dev)->num_pipes) {
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
-		acpi_video_register();
+		acpi_video_register_with_quirks();
 	}
 
 	if (IS_GEN5(dev))

Index: linux-pm/include/acpi/video.h
===================================================================

include/acpi/video.h +10 -1

@@ -17,12 +17,21 @@ struct acpi_device;
 #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
 
 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
-extern int acpi_video_register(void);
+extern int __acpi_video_register(bool backlight_quirks);
+static inline int acpi_video_register(void)
+{
+	return __acpi_video_register(false);
+}
+static inline int acpi_video_register_with_quirks(void)
+{
+	return __acpi_video_register(true);
+}
 extern void acpi_video_unregister(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 #else
 static inline int acpi_video_register(void) { return 0; }
+static inline int acpi_video_register_with_quirks(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
 static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 				      int device_id, void **edid)

Index: linux-pm/drivers/acpi/video_detect.c
===================================================================

drivers/acpi/video_detect.c +6 -0

+		return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(acpi_video_backlight_quirks);
+
 /* Promote the vendor interface instead of the generic video module.
  * This function allow DMI blacklists to be implemented by externals
  * platform drivers instead of putting a big blacklist in video_detect.c

Index: linux-pm/include/linux/acpi.h
===================================================================

include/linux/acpi.h +6 -0

@@ -196,6 +196,7 @@ extern bool wmi_has_guid(const char *gui
 
 extern long acpi_video_get_capabilities(acpi_handle graphics_dev_handle);
 extern long acpi_is_video_device(acpi_handle handle);
+extern bool acpi_video_backlight_quirks(void);
 extern void acpi_video_dmi_promote_vendor(void);
 extern void acpi_video_dmi_demote_vendor(void);
 extern int acpi_video_backlight_support(void);

@@ -213,6 +214,11 @@ static inline long acpi_is_video_device(
 	return 0;
 }
 
+static inline bool acpi_video_backlight_quirks(void)
+{
+	return false;
+}
+
 static inline void acpi_video_dmi_promote_vendor(void)
 {
 }

From: Rafael J. Wysocki <[email protected]>

According to Matthew Garrett, "Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows [8] doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems".

There's a problem with that approach, however, because simply avoiding to register the ACPI backlight interface if the firmware calls _OSI for Windows 8 may not work in the following situations:
(1) The ACPI backlight interface actually works on the given system and the i915 driver is not loaded (e.g. another graphics driver is used).
(2) The ACPI backlight interface doesn't work on the given system, but there is a vendor platform driver that will register its own, equally broken, backlight interface if not prevented from doing so by the ACPI subsystem.
Therefore we need to allow the ACPI backlight interface to be registered until the i915 driver is loaded which then will unregister it if the firmware has called _OSI for Windows 8 (or will register the ACPI video driver without backlight support if not already present).

For this reason, introduce an alternative function for registering ACPI video, acpi_video_register_with_quirks(), that will check whether or not the ACPI video driver has already been registered and whether or not the backlight Windows 8 quirk has to be applied. If the quirk has to be applied, it will block the ACPI backlight support and either unregister the backlight interface if the ACPI video driver has already been registered, or register the ACPI video driver without the backlight interface otherwise. Make the i915 driver use acpi_video_register_with_quirks() instead of acpi_video_register() in i915_driver_load().

This change is based on earlier patches from Matthew Garrett, Chun-Yi Lee and Seth Forshee and Aaron Lu's comments.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/internal.h         |   11 ++++++
 drivers/acpi/video.c            |   65 ++++++++++++++++++++++++++++++++++++----
 drivers/acpi/video_detect.c     |   21 ++++++++++++
 drivers/gpu/drm/i915/i915_dma.c |    2 -
 include/acpi/video.h            |   11 ++++++
 include/linux/acpi.h            |    1 
 6 files changed, 103 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/video.c
===================================================================

drivers/acpi/video.c +59 -6

@@ -44,6 +44,8 @@
 #include <linux/suspend.h>
 #include <acpi/video.h>
 
+#include "internal.h"
+
 #define PREFIX "ACPI: "
 
 #define ACPI_VIDEO_BUS_NAME		"Video Bus"

@@ -898,7 +900,7 @@ static void acpi_video_device_find_cap(s
 		device->cap._DDC = 1;
 	}
 
-	if (acpi_video_backlight_support()) {
+	if (acpi_video_verify_backlight_support()) {
 		struct backlight_properties props;
 		struct pci_dev *pdev;
 		acpi_handle acpi_parent;

@@ -1854,6 +1856,46 @@ static int acpi_video_bus_remove(struct
 	return 0;
 }
 
+static acpi_status video_unregister_backlight(acpi_handle handle, u32 lvl,
+					      void *context, void **rv)
+{
+	struct acpi_device *acpi_dev;
+	struct acpi_video_bus *video;
+	struct acpi_video_device *dev, *next;
+
+	if (acpi_bus_get_device(handle, &acpi_dev))
+		return AE_OK;
+
+	if (acpi_match_device_ids(acpi_dev, video_device_ids))
+		return AE_OK;
+
+	video = acpi_driver_data(acpi_dev);
+	if (!video)
+		return AE_OK;
+
+	acpi_video_bus_stop_devices(video);
+	mutex_lock(&video->device_list_lock);
+	list_for_each_entry_safe(dev, next, &video->video_device_list, entry) {
+		if (dev->backlight) {
+			backlight_device_unregister(dev->backlight);
+			dev->backlight = NULL;
+			kfree(dev->brightness->levels);
+			kfree(dev->brightness);
+		}
+		if (dev->cooling_dev) {
+			sysfs_remove_link(&dev->dev->dev.kobj,
+					  "thermal_cooling");
+			sysfs_remove_link(&dev->cooling_dev->device.kobj,
+					  "device");
+			thermal_cooling_device_unregister(dev->cooling_dev);
+			dev->cooling_dev = NULL;
+		}
+	}
+	mutex_unlock(&video->device_list_lock);
+	acpi_video_bus_start_devices(video);
+	return AE_OK;
+}
+
 static int __init is_i740(struct pci_dev *dev)
 {
 	if (dev->device == 0x00D1)

@@ -1885,14 +1927,25 @@ static int __init intel_opregion_present
 	return opregion;
 }
 
-int acpi_video_register(void)
+int __acpi_video_register(bool backlight_quirks)
 {
-	int result = 0;
+	bool no_backlight;
+	int result;
+
+	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
+
 	if (register_count) {
 		/*
-		 * if the function of acpi_video_register is already called,
-		 * don't register the acpi_vide_bus again and return no error.
+		 * If acpi_video_register() has been called already, don't try
+		 * to register acpi_video_bus, but unregister backlight devices
+		 * if no backlight support is requested.
 		 */
+		if (no_backlight)
+			acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+					    ACPI_UINT32_MAX,
+					    video_unregister_backlight,
+					    NULL, NULL, NULL);
+
 		return 0;
 	}
 

@@ -1908,7 +1961,7 @@ int acpi_video_register(void)
 
 	return 0;
 }
-EXPORT_SYMBOL(acpi_video_register);
+EXPORT_SYMBOL(__acpi_video_register);
 
 void acpi_video_unregister(void)
 {

Index: linux-pm/drivers/gpu/drm/i915/i915_dma.c
===================================================================

drivers/gpu/drm/i915/i915_dma.c +1 -1

@@ -1660,7 +1660,7 @@ int i915_driver_load(struct drm_device *
 	if (INTEL_INFO(dev)->num_pipes) {
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
-		acpi_video_register();
+		acpi_video_register_with_quirks();
 	}
 
 	if (IS_GEN5(dev))

Index: linux-pm/include/acpi/video.h
===================================================================

include/acpi/video.h +10 -1

@@ -17,12 +17,21 @@ struct acpi_device;
 #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
 
 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
-extern int acpi_video_register(void);
+extern int __acpi_video_register(bool backlight_quirks);
+static inline int acpi_video_register(void)
+{
+	return __acpi_video_register(false);
+}
+static inline int acpi_video_register_with_quirks(void)
+{
+	return __acpi_video_register(true);
+}
 extern void acpi_video_unregister(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 #else
 static inline int acpi_video_register(void) { return 0; }
+static inline int acpi_video_register_with_quirks(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
 static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 				      int device_id, void **edid)

Index: linux-pm/drivers/acpi/video_detect.c
===================================================================

drivers/acpi/video_detect.c +21 -0

@@ -38,6 +38,8 @@
 #include <linux/dmi.h>
 #include <linux/pci.h>
 
+#include "internal.h"
+
 #define PREFIX "ACPI: "
 
 ACPI_MODULE_NAME("video");

@@ -234,6 +236,17 @@ static void acpi_video_caps_check(void)
 		acpi_video_get_capabilities(NULL);
 }
 
+bool acpi_video_backlight_quirks(void)
+{
+	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
+		acpi_video_caps_check();
+		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
+		return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(acpi_video_backlight_quirks);
+
 /* Promote the vendor interface instead of the generic video module.
  * This function allow DMI blacklists to be implemented by externals
  * platform drivers instead of putting a big blacklist in video_detect.c

@@ -278,6 +291,14 @@ int acpi_video_backlight_support(void)
 }
 EXPORT_SYMBOL(acpi_video_backlight_support);
 
+/* For the ACPI video driver's use only. */
+bool acpi_video_verify_backlight_support(void)
+{
+	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
+		false : acpi_video_backlight_support();
+}
+EXPORT_SYMBOL(acpi_video_verify_backlight_support);
+
 /*
  * Use acpi_backlight=vendor/video to force that backlight switching
  * is processed by vendor specific acpi drivers or video.ko driver.

Index: linux-pm/include/linux/acpi.h
===================================================================

include/linux/acpi.h +1 -0

@@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *gui
 #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
+#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
 
 #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
 

Index: linux-pm/drivers/acpi/internal.h
===================================================================

drivers/acpi/internal.h +11 -0

@@ -164,4 +164,15 @@ struct platform_device;
 int acpi_create_platform_device(struct acpi_device *adev,
 				const struct acpi_device_id *id);
 
+/*--------------------------------------------------------------------------
+					Video
+  -------------------------------------------------------------------------- */
+#ifdef CONFIG_ACPI_VIDEO
+bool acpi_video_backlight_quirks(void);
+bool acpi_video_verify_backlight_support(void);
+#else
+static inline bool acpi_video_backlight_quirks(void) { return false; }
+static inline bool acpi_video_verify_backlight_support(void) { return false; }
+#endif
+
 #endif /* _ACPI_INTERNAL_H_ */

Quoted 37 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

According to Matthew Garrett, "Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows [8] doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems".

There's a problem with that approach, however, because simply avoiding to register the ACPI backlight interface if the firmware calls _OSI for Windows 8 may not work in the following situations:
(1) The ACPI backlight interface actually works on the given system and the i915 driver is not loaded (e.g. another graphics driver is used).
(2) The ACPI backlight interface doesn't work on the given system, but there is a vendor platform driver that will register its own, equally broken, backlight interface if not prevented from doing so by the ACPI subsystem.
Therefore we need to allow the ACPI backlight interface to be registered until the i915 driver is loaded which then will unregister it if the firmware has called _OSI for Windows 8 (or will register the ACPI video driver without backlight support if not already present).

For this reason, introduce an alternative function for registering ACPI video, acpi_video_register_with_quirks(), that will check whether or not the ACPI video driver has already been registered and whether or not the backlight Windows 8 quirk has to be applied. If the quirk has to be applied, it will block the ACPI backlight support and either unregister the backlight interface if the ACPI video driver has already been registered, or register the ACPI video driver without the backlight interface otherwise. Make the i915 driver use acpi_video_register_with_quirks() instead of acpi_video_register() in i915_driver_load().

Quoted 4 lines

This change is based on earlier patches from Matthew Garrett, Chun-Yi Lee and Seth Forshee and Aaron Lu's comments.

Signed-off-by Rafael J. Wysocki <[email protected]>

Reviewed-by Aaron Lu <[email protected]>

BTW, I also tested on a Toshiba laptop Z830 where its AML code claims support of win8, the result is as expected: ACPI video interface is removed, i915 Xorg driver picks intel_backlight.

Thanks for the fix.

Quoted 236 lines

---
 drivers/acpi/internal.h         |   11 ++++++
 drivers/acpi/video.c            |   65 ++++++++++++++++++++++++++++++++++++----
 drivers/acpi/video_detect.c     |   21 ++++++++++++
 drivers/gpu/drm/i915/i915_dma.c |    2 -
 include/acpi/video.h            |   11 ++++++
 include/linux/acpi.h            |    1 
 6 files changed, 103 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/video.c
===================================================================

drivers/acpi/video.c +59 -6

@@ -44,6 +44,8 @@
 #include <linux/suspend.h>
 #include <acpi/video.h>
 
+#include "internal.h"
+
 #define PREFIX "ACPI: "
 
 #define ACPI_VIDEO_BUS_NAME		"Video Bus"

@@ -898,7 +900,7 @@ static void acpi_video_device_find_cap(s
 		device->cap._DDC = 1;
 	}
 
-	if (acpi_video_backlight_support()) {
+	if (acpi_video_verify_backlight_support()) {
 		struct backlight_properties props;
 		struct pci_dev *pdev;
 		acpi_handle acpi_parent;

@@ -1854,6 +1856,46 @@ static int acpi_video_bus_remove(struct
 	return 0;
 }
 
+static acpi_status video_unregister_backlight(acpi_handle handle, u32 lvl,
+					      void *context, void **rv)
+{
+	struct acpi_device *acpi_dev;
+	struct acpi_video_bus *video;
+	struct acpi_video_device *dev, *next;
+
+	if (acpi_bus_get_device(handle, &acpi_dev))
+		return AE_OK;
+
+	if (acpi_match_device_ids(acpi_dev, video_device_ids))
+		return AE_OK;
+
+	video = acpi_driver_data(acpi_dev);
+	if (!video)
+		return AE_OK;
+
+	acpi_video_bus_stop_devices(video);
+	mutex_lock(&video->device_list_lock);
+	list_for_each_entry_safe(dev, next, &video->video_device_list, entry) {
+		if (dev->backlight) {
+			backlight_device_unregister(dev->backlight);
+			dev->backlight = NULL;
+			kfree(dev->brightness->levels);
+			kfree(dev->brightness);
+		}
+		if (dev->cooling_dev) {
+			sysfs_remove_link(&dev->dev->dev.kobj,
+					  "thermal_cooling");
+			sysfs_remove_link(&dev->cooling_dev->device.kobj,
+					  "device");
+			thermal_cooling_device_unregister(dev->cooling_dev);
+			dev->cooling_dev = NULL;
+		}
+	}
+	mutex_unlock(&video->device_list_lock);
+	acpi_video_bus_start_devices(video);
+	return AE_OK;
+}
+
 static int __init is_i740(struct pci_dev *dev)
 {
 	if (dev->device == 0x00D1)

@@ -1885,14 +1927,25 @@ static int __init intel_opregion_present
 	return opregion;
 }
 
-int acpi_video_register(void)
+int __acpi_video_register(bool backlight_quirks)
 {
-	int result = 0;
+	bool no_backlight;
+	int result;
+
+	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
+
 	if (register_count) {
 		/*
-		 * if the function of acpi_video_register is already called,
-		 * don't register the acpi_vide_bus again and return no error.
+		 * If acpi_video_register() has been called already, don't try
+		 * to register acpi_video_bus, but unregister backlight devices
+		 * if no backlight support is requested.
 		 */
+		if (no_backlight)
+			acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+					    ACPI_UINT32_MAX,
+					    video_unregister_backlight,
+					    NULL, NULL, NULL);
+
 		return 0;
 	}
 

@@ -1908,7 +1961,7 @@ int acpi_video_register(void)
 
 	return 0;
 }
-EXPORT_SYMBOL(acpi_video_register);
+EXPORT_SYMBOL(__acpi_video_register);
 
 void acpi_video_unregister(void)
 {

Index: linux-pm/drivers/gpu/drm/i915/i915_dma.c
===================================================================

drivers/gpu/drm/i915/i915_dma.c +1 -1

@@ -1660,7 +1660,7 @@ int i915_driver_load(struct drm_device *
 	if (INTEL_INFO(dev)->num_pipes) {
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
-		acpi_video_register();
+		acpi_video_register_with_quirks();
 	}
 
 	if (IS_GEN5(dev))

Index: linux-pm/include/acpi/video.h
===================================================================

include/acpi/video.h +10 -1

@@ -17,12 +17,21 @@ struct acpi_device;
 #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
 
 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
-extern int acpi_video_register(void);
+extern int __acpi_video_register(bool backlight_quirks);
+static inline int acpi_video_register(void)
+{
+	return __acpi_video_register(false);
+}
+static inline int acpi_video_register_with_quirks(void)
+{
+	return __acpi_video_register(true);
+}
 extern void acpi_video_unregister(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 #else
 static inline int acpi_video_register(void) { return 0; }
+static inline int acpi_video_register_with_quirks(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
 static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 				      int device_id, void **edid)

Index: linux-pm/drivers/acpi/video_detect.c
===================================================================

drivers/acpi/video_detect.c +21 -0

@@ -38,6 +38,8 @@
 #include <linux/dmi.h>
 #include <linux/pci.h>
 
+#include "internal.h"
+
 #define PREFIX "ACPI: "
 
 ACPI_MODULE_NAME("video");

@@ -234,6 +236,17 @@ static void acpi_video_caps_check(void)
 		acpi_video_get_capabilities(NULL);
 }
 
+bool acpi_video_backlight_quirks(void)
+{
+	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
+		acpi_video_caps_check();
+		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
+		return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(acpi_video_backlight_quirks);
+
 /* Promote the vendor interface instead of the generic video module.
  * This function allow DMI blacklists to be implemented by externals
  * platform drivers instead of putting a big blacklist in video_detect.c

@@ -278,6 +291,14 @@ int acpi_video_backlight_support(void)
 }
 EXPORT_SYMBOL(acpi_video_backlight_support);
 
+/* For the ACPI video driver's use only. */
+bool acpi_video_verify_backlight_support(void)
+{
+	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
+		false : acpi_video_backlight_support();
+}
+EXPORT_SYMBOL(acpi_video_verify_backlight_support);
+
 /*
  * Use acpi_backlight=vendor/video to force that backlight switching
  * is processed by vendor specific acpi drivers or video.ko driver.

Index: linux-pm/include/linux/acpi.h
===================================================================

include/linux/acpi.h +1 -0

@@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *gui
 #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
+#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
 
 #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
 

Index: linux-pm/drivers/acpi/internal.h
===================================================================

drivers/acpi/internal.h +11 -0

@@ -164,4 +164,15 @@ struct platform_device;
 int acpi_create_platform_device(struct acpi_device *adev,
 				const struct acpi_device_id *id);
 
+/*--------------------------------------------------------------------------
+					Video
+  -------------------------------------------------------------------------- */
+#ifdef CONFIG_ACPI_VIDEO
+bool acpi_video_backlight_quirks(void);
+bool acpi_video_verify_backlight_support(void);
+#else
+static inline bool acpi_video_backlight_quirks(void) { return false; }
+static inline bool acpi_video_verify_backlight_support(void) { return false; }
+#endif
+
 #endif /* _ACPI_INTERNAL_H_ */

Quoted 49 lines · Aaron Lu

Quoted 37 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

According to Matthew Garrett, "Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows [8] doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems".

There's a problem with that approach, however, because simply avoiding to register the ACPI backlight interface if the firmware calls _OSI for Windows 8 may not work in the following situations:
(1) The ACPI backlight interface actually works on the given system and the i915 driver is not loaded (e.g. another graphics driver is used).
(2) The ACPI backlight interface doesn't work on the given system, but there is a vendor platform driver that will register its own, equally broken, backlight interface if not prevented from doing so by the ACPI subsystem.
Therefore we need to allow the ACPI backlight interface to be registered until the i915 driver is loaded which then will unregister it if the firmware has called _OSI for Windows 8 (or will register the ACPI video driver without backlight support if not already present).

For this reason, introduce an alternative function for registering ACPI video, acpi_video_register_with_quirks(), that will check whether or not the ACPI video driver has already been registered and whether or not the backlight Windows 8 quirk has to be applied. If the quirk has to be applied, it will block the ACPI backlight support and either unregister the backlight interface if the ACPI video driver has already been registered, or register the ACPI video driver without the backlight interface otherwise. Make the i915 driver use acpi_video_register_with_quirks() instead of acpi_video_register() in i915_driver_load().

Quoted 4 lines

This change is based on earlier patches from Matthew Garrett, Chun-Yi Lee and Seth Forshee and Aaron Lu's comments.

Signed-off-by Rafael J. Wysocki <[email protected]>

Reviewed-by Aaron Lu <[email protected]>

BTW, I also tested on a Toshiba laptop Z830 where its AML code claims support of win8, the result is as expected: ACPI video interface is removed, i915 Xorg driver picks intel_backlight.

Quoted 1 line

Thanks for the fix.

Cool, thanks for testing this!

Can you please also ask bug reporters in the BZ entires related to this to test it too?

Rafael

Quoted 237 lines

Quoted 236 lines

---
 drivers/acpi/internal.h         |   11 ++++++
 drivers/acpi/video.c            |   65 ++++++++++++++++++++++++++++++++++++----
 drivers/acpi/video_detect.c     |   21 ++++++++++++
 drivers/gpu/drm/i915/i915_dma.c |    2 -
 include/acpi/video.h            |   11 ++++++
 include/linux/acpi.h            |    1 
 6 files changed, 103 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/video.c
===================================================================

drivers/acpi/video.c +59 -6

@@ -44,6 +44,8 @@
 #include <linux/suspend.h>
 #include <acpi/video.h>
 
+#include "internal.h"
+
 #define PREFIX "ACPI: "
 
 #define ACPI_VIDEO_BUS_NAME		"Video Bus"

@@ -898,7 +900,7 @@ static void acpi_video_device_find_cap(s
 		device->cap._DDC = 1;
 	}
 
-	if (acpi_video_backlight_support()) {
+	if (acpi_video_verify_backlight_support()) {
 		struct backlight_properties props;
 		struct pci_dev *pdev;
 		acpi_handle acpi_parent;

@@ -1854,6 +1856,46 @@ static int acpi_video_bus_remove(struct
 	return 0;
 }
 
+static acpi_status video_unregister_backlight(acpi_handle handle, u32 lvl,
+					      void *context, void **rv)
+{
+	struct acpi_device *acpi_dev;
+	struct acpi_video_bus *video;
+	struct acpi_video_device *dev, *next;
+
+	if (acpi_bus_get_device(handle, &acpi_dev))
+		return AE_OK;
+
+	if (acpi_match_device_ids(acpi_dev, video_device_ids))
+		return AE_OK;
+
+	video = acpi_driver_data(acpi_dev);
+	if (!video)
+		return AE_OK;
+
+	acpi_video_bus_stop_devices(video);
+	mutex_lock(&video->device_list_lock);
+	list_for_each_entry_safe(dev, next, &video->video_device_list, entry) {
+		if (dev->backlight) {
+			backlight_device_unregister(dev->backlight);
+			dev->backlight = NULL;
+			kfree(dev->brightness->levels);
+			kfree(dev->brightness);
+		}
+		if (dev->cooling_dev) {
+			sysfs_remove_link(&dev->dev->dev.kobj,
+					  "thermal_cooling");
+			sysfs_remove_link(&dev->cooling_dev->device.kobj,
+					  "device");
+			thermal_cooling_device_unregister(dev->cooling_dev);
+			dev->cooling_dev = NULL;
+		}
+	}
+	mutex_unlock(&video->device_list_lock);
+	acpi_video_bus_start_devices(video);
+	return AE_OK;
+}
+
 static int __init is_i740(struct pci_dev *dev)
 {
 	if (dev->device == 0x00D1)

@@ -1885,14 +1927,25 @@ static int __init intel_opregion_present
 	return opregion;
 }
 
-int acpi_video_register(void)
+int __acpi_video_register(bool backlight_quirks)
 {
-	int result = 0;
+	bool no_backlight;
+	int result;
+
+	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
+
 	if (register_count) {
 		/*
-		 * if the function of acpi_video_register is already called,
-		 * don't register the acpi_vide_bus again and return no error.
+		 * If acpi_video_register() has been called already, don't try
+		 * to register acpi_video_bus, but unregister backlight devices
+		 * if no backlight support is requested.
 		 */
+		if (no_backlight)
+			acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+					    ACPI_UINT32_MAX,
+					    video_unregister_backlight,
+					    NULL, NULL, NULL);
+
 		return 0;
 	}
 

@@ -1908,7 +1961,7 @@ int acpi_video_register(void)
 
 	return 0;
 }
-EXPORT_SYMBOL(acpi_video_register);
+EXPORT_SYMBOL(__acpi_video_register);
 
 void acpi_video_unregister(void)
 {

Index: linux-pm/drivers/gpu/drm/i915/i915_dma.c
===================================================================

drivers/gpu/drm/i915/i915_dma.c +1 -1

@@ -1660,7 +1660,7 @@ int i915_driver_load(struct drm_device *
 	if (INTEL_INFO(dev)->num_pipes) {
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
-		acpi_video_register();
+		acpi_video_register_with_quirks();
 	}
 
 	if (IS_GEN5(dev))

Index: linux-pm/include/acpi/video.h
===================================================================

include/acpi/video.h +10 -1

@@ -17,12 +17,21 @@ struct acpi_device;
 #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
 
 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
-extern int acpi_video_register(void);
+extern int __acpi_video_register(bool backlight_quirks);
+static inline int acpi_video_register(void)
+{
+	return __acpi_video_register(false);
+}
+static inline int acpi_video_register_with_quirks(void)
+{
+	return __acpi_video_register(true);
+}
 extern void acpi_video_unregister(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 #else
 static inline int acpi_video_register(void) { return 0; }
+static inline int acpi_video_register_with_quirks(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
 static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 				      int device_id, void **edid)

Index: linux-pm/drivers/acpi/video_detect.c
===================================================================

drivers/acpi/video_detect.c +21 -0

@@ -38,6 +38,8 @@
 #include <linux/dmi.h>
 #include <linux/pci.h>
 
+#include "internal.h"
+
 #define PREFIX "ACPI: "
 
 ACPI_MODULE_NAME("video");

@@ -234,6 +236,17 @@ static void acpi_video_caps_check(void)
 		acpi_video_get_capabilities(NULL);
 }
 
+bool acpi_video_backlight_quirks(void)
+{
+	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
+		acpi_video_caps_check();
+		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
+		return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(acpi_video_backlight_quirks);
+
 /* Promote the vendor interface instead of the generic video module.
  * This function allow DMI blacklists to be implemented by externals
  * platform drivers instead of putting a big blacklist in video_detect.c

@@ -278,6 +291,14 @@ int acpi_video_backlight_support(void)
 }
 EXPORT_SYMBOL(acpi_video_backlight_support);
 
+/* For the ACPI video driver's use only. */
+bool acpi_video_verify_backlight_support(void)
+{
+	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
+		false : acpi_video_backlight_support();
+}
+EXPORT_SYMBOL(acpi_video_verify_backlight_support);
+
 /*
  * Use acpi_backlight=vendor/video to force that backlight switching
  * is processed by vendor specific acpi drivers or video.ko driver.

Index: linux-pm/include/linux/acpi.h
===================================================================

include/linux/acpi.h +1 -0

@@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *gui
 #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
+#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
 
 #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
 

Index: linux-pm/drivers/acpi/internal.h
===================================================================

drivers/acpi/internal.h +11 -0

@@ -164,4 +164,15 @@ struct platform_device;
 int acpi_create_platform_device(struct acpi_device *adev,
 				const struct acpi_device_id *id);
 
+/*--------------------------------------------------------------------------
+					Video
+  -------------------------------------------------------------------------- */
+#ifdef CONFIG_ACPI_VIDEO
+bool acpi_video_backlight_quirks(void);
+bool acpi_video_verify_backlight_support(void);
+#else
+static inline bool acpi_video_backlight_quirks(void) { return false; }
+static inline bool acpi_video_verify_backlight_support(void) { return false; }
+#endif
+
 #endif /* _ACPI_INTERNAL_H_ */

Quoted 50 lines · Rafael J. Wysocki

Quoted 49 lines · Aaron Lu

Quoted 37 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

According to Matthew Garrett, "Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows [8] doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems".

There's a problem with that approach, however, because simply avoiding to register the ACPI backlight interface if the firmware calls _OSI for Windows 8 may not work in the following situations:
(1) The ACPI backlight interface actually works on the given system and the i915 driver is not loaded (e.g. another graphics driver is used).
(2) The ACPI backlight interface doesn't work on the given system, but there is a vendor platform driver that will register its own, equally broken, backlight interface if not prevented from doing so by the ACPI subsystem.
Therefore we need to allow the ACPI backlight interface to be registered until the i915 driver is loaded which then will unregister it if the firmware has called _OSI for Windows 8 (or will register the ACPI video driver without backlight support if not already present).

For this reason, introduce an alternative function for registering ACPI video, acpi_video_register_with_quirks(), that will check whether or not the ACPI video driver has already been registered and whether or not the backlight Windows 8 quirk has to be applied. If the quirk has to be applied, it will block the ACPI backlight support and either unregister the backlight interface if the ACPI video driver has already been registered, or register the ACPI video driver without the backlight interface otherwise. Make the i915 driver use acpi_video_register_with_quirks() instead of acpi_video_register() in i915_driver_load().

Quoted 4 lines

This change is based on earlier patches from Matthew Garrett, Chun-Yi Lee and Seth Forshee and Aaron Lu's comments.

Signed-off-by Rafael J. Wysocki <[email protected]>

Reviewed-by Aaron Lu <[email protected]>

BTW, I also tested on a Toshiba laptop Z830 where its AML code claims support of win8, the result is as expected: ACPI video interface is removed, i915 Xorg driver picks intel_backlight.

Quoted 6 lines

Quoted 1 line

Thanks for the fix.

Cool, thanks for testing this!

Can you please also ask bug reporters in the BZ entires related to this to test it too?

Sure.

To be clear, I actually tested the patch in your linux-next branch, which turned out to be a little different in that you have fixed the problem Igor raised here.

I'll ask reporters to test on a stable tree with the following two patches on top:
https://patchwork.kernel.org/patch/2812951/ (expose OSI version)
https://patchwork.kernel.org/patch/2827793/ (your updated patch) or your linux-next branch, whichever they like.

From: Rafael J. Wysocki <[email protected]>

According to Matthew Garrett, "Windows 8 leaves backlight control up to individual graphics drivers rather than making ACPI calls itself. There's plenty of evidence to suggest that the Intel driver for Windows [8] doesn't use the ACPI interface, including the fact that it's broken on a bunch of machines when the OS claims to support Windows 8. The simplest thing to do appears to be to disable the ACPI backlight interface on these systems".

There's a problem with that approach, however, because simply avoiding to register the ACPI backlight interface if the firmware calls _OSI for Windows 8 may not work in the following situations:
(1) The ACPI backlight interface actually works on the given system and the i915 driver is not loaded (e.g. another graphics driver is used).
(2) The ACPI backlight interface doesn't work on the given system, but there is a vendor platform driver that will register its own, equally broken, backlight interface if not prevented from doing so by the ACPI subsystem.
Therefore we need to allow the ACPI backlight interface to be registered until the i915 driver is loaded which then will unregister it if the firmware has called _OSI for Windows 8 (or will register the ACPI video driver without backlight support if not already present).

For this reason, introduce an alternative function for registering ACPI video, acpi_video_register_with_quirks(), that will check whether or not the ACPI video driver has already been registered and whether or not the backlight Windows 8 quirk has to be applied. If the quirk has to be applied, it will block the ACPI backlight support and either unregister the backlight interface if the ACPI video driver has already been registered, or register the ACPI video driver without the backlight interface otherwise. Make the i915 driver use acpi_video_register_with_quirks() instead of acpi_video_register() in i915_driver_load().

This change is based on earlier patches from Matthew Garrett, Chun-Yi Lee and Seth Forshee and Aaron Lu's comments.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/internal.h         |   11 ++++++
 drivers/acpi/video.c            |   65 ++++++++++++++++++++++++++++++++++++----
 drivers/acpi/video_detect.c     |   21 ++++++++++++
 drivers/gpu/drm/i915/i915_dma.c |    2 -
 include/acpi/video.h            |   11 ++++++
 include/linux/acpi.h            |    1 
 6 files changed, 103 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/video.c
===================================================================

drivers/acpi/video.c +59 -6

@@ -44,6 +44,8 @@
 #include <linux/suspend.h>
 #include <acpi/video.h>
 
+#include "internal.h"
+
 #define PREFIX "ACPI: "
 
 #define ACPI_VIDEO_BUS_NAME		"Video Bus"

@@ -898,7 +900,7 @@ static void acpi_video_device_find_cap(s
 		device->cap._DDC = 1;
 	}
 
-	if (acpi_video_backlight_support()) {
+	if (acpi_video_verify_backlight_support()) {
 		struct backlight_properties props;
 		struct pci_dev *pdev;
 		acpi_handle acpi_parent;

@@ -1854,6 +1856,46 @@ static int acpi_video_bus_remove(struct
 	return 0;
 }
 
+static acpi_status video_unregister_backlight(acpi_handle handle, u32 lvl,
+					      void *context, void **rv)
+{
+	struct acpi_device *acpi_dev;
+	struct acpi_video_bus *video;
+	struct acpi_video_device *dev, *next;
+
+	if (acpi_bus_get_device(handle, &acpi_dev))
+		return AE_OK;
+
+	if (acpi_match_device_ids(acpi_dev, video_device_ids))
+		return AE_OK;
+
+	video = acpi_driver_data(acpi_dev);
+	if (!video)
+		return AE_OK;
+
+	acpi_video_bus_stop_devices(video);
+	mutex_lock(&video->device_list_lock);
+	list_for_each_entry_safe(dev, next, &video->video_device_list, entry) {
+		if (dev->backlight) {
+			backlight_device_unregister(dev->backlight);
+			dev->backlight = NULL;
+			kfree(dev->brightness->levels);
+			kfree(dev->brightness);
+		}
+		if (dev->cooling_dev) {
+			sysfs_remove_link(&dev->dev->dev.kobj,
+					  "thermal_cooling");
+			sysfs_remove_link(&dev->cooling_dev->device.kobj,
+					  "device");
+			thermal_cooling_device_unregister(dev->cooling_dev);
+			dev->cooling_dev = NULL;
+		}
+	}
+	mutex_unlock(&video->device_list_lock);
+	acpi_video_bus_start_devices(video);
+	return AE_OK;
+}
+
 static int __init is_i740(struct pci_dev *dev)
 {
 	if (dev->device == 0x00D1)

@@ -1885,14 +1927,25 @@ static int __init intel_opregion_present
 	return opregion;
 }
 
-int acpi_video_register(void)
+int __acpi_video_register(bool backlight_quirks)
 {
-	int result = 0;
+	bool no_backlight;
+	int result;
+
+	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
+
 	if (register_count) {
 		/*
-		 * if the function of acpi_video_register is already called,
-		 * don't register the acpi_vide_bus again and return no error.
+		 * If acpi_video_register() has been called already, don't try
+		 * to register acpi_video_bus, but unregister backlight devices
+		 * if no backlight support is requested.
 		 */
+		if (no_backlight)
+			acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+					    ACPI_UINT32_MAX,
+					    video_unregister_backlight,
+					    NULL, NULL, NULL);
+
 		return 0;
 	}
 

@@ -1908,7 +1961,7 @@ int acpi_video_register(void)
 
 	return 0;
 }
-EXPORT_SYMBOL(acpi_video_register);
+EXPORT_SYMBOL(__acpi_video_register);
 
 void acpi_video_unregister(void)
 {

Index: linux-pm/drivers/gpu/drm/i915/i915_dma.c
===================================================================

drivers/gpu/drm/i915/i915_dma.c +1 -1

@@ -1660,7 +1660,7 @@ int i915_driver_load(struct drm_device *
 	if (INTEL_INFO(dev)->num_pipes) {
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
-		acpi_video_register();
+		acpi_video_register_with_quirks();
 	}
 
 	if (IS_GEN5(dev))

Index: linux-pm/include/acpi/video.h
===================================================================

include/acpi/video.h +10 -1

@@ -17,12 +17,21 @@ struct acpi_device;
 #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
 
 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
-extern int acpi_video_register(void);
+extern int __acpi_video_register(bool backlight_quirks);
+static inline int acpi_video_register(void)
+{
+	return __acpi_video_register(false);
+}
+static inline int acpi_video_register_with_quirks(void)
+{
+	return __acpi_video_register(true);
+}
 extern void acpi_video_unregister(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 #else
 static inline int acpi_video_register(void) { return 0; }
+static inline int acpi_video_register_with_quirks(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
 static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 				      int device_id, void **edid)

Index: linux-pm/drivers/acpi/video_detect.c
===================================================================

drivers/acpi/video_detect.c +21 -0

@@ -38,6 +38,8 @@
 #include <linux/dmi.h>
 #include <linux/pci.h>
 
+#include "internal.h"
+
 #define PREFIX "ACPI: "
 
 ACPI_MODULE_NAME("video");

@@ -234,6 +236,17 @@ static void acpi_video_caps_check(void)
 		acpi_video_get_capabilities(NULL);
 }
 
+bool acpi_video_backlight_quirks(void)
+{
+	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
+		acpi_video_caps_check();
+		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
+		return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(acpi_video_backlight_quirks);
+
 /* Promote the vendor interface instead of the generic video module.
  * This function allow DMI blacklists to be implemented by externals
  * platform drivers instead of putting a big blacklist in video_detect.c

@@ -278,6 +291,14 @@ int acpi_video_backlight_support(void)
 }
 EXPORT_SYMBOL(acpi_video_backlight_support);
 
+/* For the ACPI video driver's use only. */
+bool acpi_video_verify_backlight_support(void)
+{
+	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
+		false : acpi_video_backlight_support();
+}
+EXPORT_SYMBOL(acpi_video_verify_backlight_support);
+
 /*
  * Use acpi_backlight=vendor/video to force that backlight switching
  * is processed by vendor specific acpi drivers or video.ko driver.

Index: linux-pm/include/linux/acpi.h
===================================================================

include/linux/acpi.h +1 -0

@@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *gui
 #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
+#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
 
 #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
 

Index: linux-pm/drivers/acpi/internal.h
===================================================================

drivers/acpi/internal.h +2 -0

@@ -164,4 +164,15 @@ struct platform_device;
 int acpi_create_platform_device(struct acpi_device *adev,
 				const struct acpi_device_id *id);
 
+/*--------------------------------------------------------------------------
+					Video

On Sat, 2013-07-13 at 02:46 +0200, Rafael J. Wysocki wrote:

I can't build it. Where did I go wrong?